Defame1297/holocron
0
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases 3 Wiki Activity

docs: correct the claims this review found false

Browse Source 
AGENTS.md told an offline agent to push with SKIP=apm-marketplace-check and
asserted that hook was "the only one whose failure mode is 'no network'".
Running all 12 pre-push hooks under a network namespace shows two fail, for
one shared cause: apm-pack-check-clean resolves the same remote entry. An
exact pin does not remove the ls-remote, so both hooks are named now.

AGENTS.md also said everything in a plugin root except .apm/ is generated.
Plugin roots carry hand-authored README.md, docs/, bin/, sources.md and
.mcp.json, so an agent would hunt for an .apm/ source that does not exist or
refuse the edit. The rule is positional: immunity belongs to the plugin root,
and anything inside a mirrored directory is still rm -rf'd.

ADR-0017 said apm strips a hooks field. The real loop is (agents, skills,
commands, instructions) -- hooks absent, instructions never mentioned -- and
it can never fire, because synthesize_plugin_json_from_apm_yml only emits the
eight identity fields. The decision stands; the mechanism was overstated. Its
mcpServers amendment is rewritten for the pointer payload and now records the
real reason: inlining bypassed apm's credential sanitizer.

ADR-0015's owner.email and version-pin passages are corrected against the apm
source, and ADR-0016 gains the disallowedTools amendment. agent-audit's
allowlist is data, so it gains disallowedTools too -- the ADR and the
validator that enforces it had come apart.

architecture.md described a root CLAUDE.md that imports two files (it imports
one, plus an RTK block) and pointed at an ADR index that does not exist.
Seven skill READMEs listed tests/ files the mirror strips, promising installed
users files their install lacks; those rows are marked source-only, with the
depth-4 template tests explicitly called out as surviving. And
plugins/kyberforge/hooks/README.md, deleted during the conversion and
preserved nowhere, is restored to a path the mirror does not own -- verified
by running a sync against a scratch copy.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT
This commit is contained in:
Defame1297
2026-08-14 11:04:56 +00:00
parent 7ba3d9cf1d
commit 430f46b8e8
19 changed files with 313 additions and 97 deletions
Download Patch File Download Diff File Expand all files Collapse all files

125
docs/adr/0017-plugin-content-mirror-bridges-apm-to-host-discovery.md
View File

@@ -98,18 +98,29 @@ governance status as `.claude-plugin/plugin.json`/`marketplace.json`:
## Considered options
**Patch `plugin.json`'s `skills`/`agents`/`commands`/`hooks` fields to point directly at `.apm/`
paths (rejected).** Claude Code's manifest schema documents these as legitimate override fields
that accept custom paths —
`plugins/kyberforge/docs/research/docs/claude-code-plugins/configuration.md` shows a real example
(`"skills": "./custom/skills/"`, `"agents": ["./custom/agents/reviewer.md"]`), so the host side of
this would work. Rejected because apm's compiler is not a passive pass-through: `build_plugin_manifest`
unconditionally strips these keys from every manifest it generates, on the stated assumption that
convention directories are always host-auto-discovered and therefore never need an explicit
pointer. Honoring this option would mean post-processing apm's compiled output on every
`apm pack` run to re-inject fields apm actively removes — fighting a stable, intentional apm code
path indefinitely — rather than reusing `plugin_exporter.py`'s bundle-export mapping, which already
does the right thing and only needed its output redirected to a path the installer reads.
**Patch `plugin.json`'s content-pointer fields to point directly at `.apm/` paths (rejected).**
Claude Code's manifest schema documents these as legitimate override fields that accept custom
paths — `plugins/kyberforge/docs/research/docs/claude-code-plugins/configuration.md` shows a real
example (`"skills": "./custom/skills/"`, `"agents": ["./custom/agents/reviewer.md"]`), so the host
side of this would work. Rejected because apm never emits such a pointer and would have to be
worked around on every run to make it do so.
Be precise about the mechanism, because an earlier revision of this ADR overstated it. apm 0.28.0's
`build_plugin_manifest` (`apm_cli/core/plugin_manifest.py`) does carry a strip loop, but its field
list is `("agents", "skills", "commands", "instructions")` — `hooks` is **not** in it, and
`instructions` **is**, which this ADR previously did not mention. More to the point, that loop can
never fire: the manifest it operates on comes from `synthesize_plugin_json_from_apm_yml`
(`apm_cli/deps/plugin_parser.py`), which only ever emits `name`, `version`, `description`,
`author`, `license`, `homepage`, `repository` and `keywords`. The pointer fields are absent from
apm's output because `apm.yml` has no schema for them, not because apm actively removes them — the
`pop` loop is defensive dead code against a manifest shape apm does not produce.
The rejection is unaffected by that correction, only its framing. Honoring this option would still
mean post-processing apm's compiled output on every `apm pack` run to add fields apm's schema has
no way to express, rather than reusing `plugin_exporter.py`'s bundle-export mapping, which already
does the right thing and only needed its output redirected to a path the installer reads. What it
is *not* is a fight against a load-bearing apm code path — the honest statement is that apm has no
input for these fields, and inventing one downstream is a workaround this ADR did not need.
**Point `marketplace.json`'s `source:` at `apm pack`'s `build/<name>-<version>/` output directly
(rejected).** Would reuse the bundle exporter's correct mapping without adding a new script.
@@ -121,36 +132,50 @@ source and scans directories; it does not execute a package manager's build comm
Copying the relevant subset back to the stable `plugins/<name>/` path — where `marketplace.json`
already points — needed no change to the marketplace source model at all.
## Amendment (2026-08-13): `mcpServers` is narrowly reinjected into Copilot's `plugin.json`
## Amendment (2026-08-13, revised 2026-08-14): Copilot's `plugin.json` gets an `mcpServers` *path*
PR #95's review (a follow-on to this same issue #90 workstream) found a second field apm's
compiler strips for the Copilot ecosystem: `build_plugin_manifest` unconditionally removes
`mcpServers` from every Copilot-ecosystem `plugin.json`, its docstring stating the field is "not
part of the Copilot plugin manifest schema." That claim is contradicted by this repo's own
researched documentation — `plugins/kyberforge/docs/research/docs/github-copilot-plugins/
configuration.md:49` documents `mcpServers` as a valid, optional `plugin.json` field for Copilot.
compiler drops for the Copilot ecosystem: `build_plugin_manifest` runs
`manifest.pop("mcpServers", None)` on every Copilot-ecosystem `plugin.json`, its docstring stating
the field is "not part of the Copilot plugin manifest schema." That claim is contradicted by this
repo's own researched documentation —
`plugins/kyberforge/docs/research/docs/github-copilot-plugins/configuration.md:49` documents
`mcpServers` as a valid, optional `plugin.json` field, typed **"string or object — MCP server
config path or inline definitions."**
This is not the same situation "Considered options" above rejected. That rejection concerned
fields apm strips *correctly*, on a stable and accurate premise: convention directories
(`skills/`, `agents/`, `commands/`) are host-auto-discovered, so an explicit pointer is redundant
by design. Here, apm's own stated justification for stripping `mcpServers` is factually wrong
against documented Copilot behavior — there is no host-auto-discovery mechanism that makes an
explicit `mcpServers` declaration redundant, the way there is for skills/agents/commands. Applying
the same "don't fight a stable, intentional apm code path" reasoning here would mean shipping a
plugin manifest known to be missing a field Copilot actually reads.
This is not the same situation "Considered options" above rejected. There, apm emits no pointer
because its schema has no input for one and the host auto-discovers the directories anyway, so
nothing is missing. Here a field Copilot actually reads is actively removed on a premise that is
wrong against documented Copilot behavior, and there is no auto-discovery mechanism that makes it
redundant. Shipping the manifest as apm produces it would ship a manifest known to be incomplete.
Given that, `scripts/sync-plugin-content.sh`'s `reinject_mcp_servers()`, called from `sync_one()`,
narrowly re-injects `mcpServers` into `.github/plugin/plugin.json` after `apm pack` runs, sourced
from the plugin's own `.mcp.json`, and only when it declares at least one server — matching apm's
own Claude-ecosystem builder, which omits the field entirely rather than emitting
`mcpServers: {}`. **Both modes re-inject**, not just real syncs: real mode writes into the plugin
root directly, `--check` into its throwaway copy first, so the manifest diff compares against the
same content a real sync would actually produce (see the script's own header). A check-mode
re-injection is what keeps `--check` from reporting permanent phantom drift on every plugin that
ships an `.mcp.json`. This is scoped to one field found to be incorrectly stripped, not a
reversal of the broader position above: the rejection of patching
`skills`/`agents`/`commands`/`hooks` pointers still holds, since apm's premise for stripping those
remains accurate.
`scripts/sync-plugin-content.sh`'s `reinject_mcp_servers()`, called from `sync_one()`, therefore
sets `mcpServers` on `.github/plugin/plugin.json` after `apm pack` runs — to the **string
`".mcp.json"`**, the path form of the documented type, not the resolved server objects. Only when
the plugin's `.mcp.json` declares at least one server, matching apm's own Claude-ecosystem builder,
which omits the field entirely rather than emitting `mcpServers: {}`.
**The payload is a path because an inlined object is a credential-leak path.** The original
implementation copied `.mcp.json`'s resolved `mcpServers` object into the manifest with `jq`. That
route bypasses apm's own `_sanitize_mcp_servers()` (`apm_cli/core/plugin_manifest.py`), which
strips credential keys and redacts secret values out of `.mcp.json` precisely because — in its own
words — "copying them verbatim into a committed `plugin.json` would exfiltrate them into the
distributed artefact." Today's `.mcp.json` files here carry no `env` block, so nothing leaked; the
first one that did would have written a live token into a tracked, published manifest, with the
sanitizer sitting one code path away and never invoked. A path reference cannot carry a secret at
all: the manifest names a file, and resolution happens in the host at load time. This also matches
apm's documented posture for MCP secrets — `microsoft-apm/configuration.md:96-98` requires `${VAR}`
indirection so secrets are "never committed to the manifest."
**Both modes re-inject**, not just real syncs: real mode writes into the plugin root directly,
`--check` into its throwaway copy first, so the manifest diff compares against the same content a
real sync would actually produce (see the script's own header). A check-mode re-injection is what
keeps `--check` from reporting permanent phantom drift on every plugin that ships an `.mcp.json`.
This remains scoped to one field found to be incorrectly dropped. It does not reopen the
content-pointer option rejected above: those fields stay absent because apm has no schema input for
them and the host needs no pointer, which is a different situation from a documented field being
actively removed.
Consequence: if a future apm release corrects the Copilot `mcpServers` omission, `reinject_mcp_servers()`
and its call site become dead code and should be deleted — nothing else in this ADR depends on the
@@ -166,13 +191,15 @@ A function name is stable enough to grep for; a line number in an ADR is stale b
As originally executed, `sync-plugin-content.sh` wrote the merged hooks file to
`plugins/<name>/hooks.json`. That path is scanned by nothing. Claude Code convention-scans
`hooks/hooks.json`, and the "Plugin Directory Layout" table this ADR's own root-cause analysis
quotes above says so on the same line it says "All content directories must be at the plugin root,
not inside `.claude-plugin/`"
(`plugins/kyberforge/docs/research/docs/claude-code-plugins/configuration.md:100`). The
implementation read "at the plugin root" and dropped the file there; the row it was reading names
`hooks/hooks.json`. So this ADR shipped with the contract quoted correctly in its diagnosis and
violated in its output — the flat mirror bridged skills and agents into discovery and left hooks
exactly as undiscoverable as before the fix.
quotes above says so:
`plugins/kyberforge/docs/research/docs/claude-code-plugins/configuration.md:100` is the row naming
`hooks/hooks.json`, six lines below the table's preamble at `:94` — "All content directories must
be at the plugin root, not inside `.claude-plugin/`". The two are not the same line; an earlier
revision of this amendment said they were. The implementation read the preamble's "at the plugin
root" and dropped the file there, without reading the row that names the path. So this ADR shipped
with the contract quoted correctly in its diagnosis and violated in its output — the flat mirror
bridged skills and agents into discovery and left hooks exactly as undiscoverable as before the
fix.
The merged file therefore moves to `plugins/<name>/hooks/hooks.json`. A root-level `hooks.json`
left over from a prior sync is stale output: a real sync deletes it, `--check` reports it as
@@ -184,9 +211,11 @@ only those two grow a mirrored hooks file at all.
This does **not** reopen the "patch `plugin.json` pointer fields" option rejected above. The move
needs no `hooks` pointer in `plugin.json`: `hooks/hooks.json` *is* the convention path, so the
host finds it by auto-discovery, exactly as it finds `skills/` and `agents/`. The rejection stands
for the reason it was made — apm's `build_plugin_manifest` strips pointer fields unconditionally
and is right to, because convention directories need no pointer. Writing to the convention path is
what makes that premise true here rather than something to fight.
for the reason it was made, once stated accurately — apm emits no pointer field for any of these,
because `apm.yml` has no key that produces one, and none is needed when content sits at the
convention path. (`hooks` was never in `build_plugin_manifest`'s strip list at all; see the
corrected mechanism note under "Considered options".) Writing to the convention path is what makes
the no-pointer premise true here rather than something to work around.
## Consequences