feat(kyberforge): execute the plugin→APM conversion #95
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Defame1297/holocron#95
Reference in New Issue
Block a user
Delete Branch "feat/90-execute-apm-conversion"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #90
Closes #96
Closes #97
Converts all six plugins (
bin,core,git,gitea,kyberforge,lint) from hand-maintained.claude-plugin/plugin.json+.claude-plugin/marketplace.jsonto Microsoft APM (apm.yml+.apm/) as the authoring source of truth, per ADR-0015.plugin.json/marketplace.jsonare now compiled output.What changed
apm.yml; skills moved to.apm/skills/, agents re-authored as single-file.apm/agents/*.agent.mdper ADR-0016 (Claude-only fields stripped), hooks moved to.apm/hooks/.apm.ymlregisters all six local packages plus the remotemattpocock-skillssource;.claude-plugin/marketplace.json,.agents/plugins/marketplace.json(codex) and.github/plugin/marketplace.jsonare now compiled/synced outputs instead of hand-edited.plugin-author/marketplace-authorskills (superseded byapm-workflow); updated.pre-commit-config.yamlandscripts/check-scope-walkup-sync.shfor the new.apm/path shapes; updatedCONTEXT.mdand ADRs 0001/0006/0010; added ADR-0017 (content mirror bridges apm to host discovery).38f1ba4).6e77c11).5f42f57).Deliberate deviation from ADR-0017
9c140efre-injectsmcpServersinto.github/plugin/plugin.jsonafterapm packstrips it. ADR-0017 otherwise rejects patching apm's compiled output; this is a documented amendment, because apm's justification for stripping the field is contradicted by this repo's own researched Copilot schema — unlike the pointer fields ADR-0017 correctly left alone. It applies tobinonly (the one plugin with a non-empty.mcp.json).Unrelated fix riding along
925f04afixes a false negative inplugins/core/.apm/skills/agentsmd-audit/scripts/validate-secrets.sh: the placeholder allowlist was matched against the whole line before any secret pattern ran, so an incidentalexamplein a trailing comment suppressed detection of a real credential earlier on the same line. Flagged explicitly because it is a security fix unrelated to the APM conversion.Issues #96 and #97 folded in (fifth round)
Disclosed rather than silent, because an earlier review of this PR flagged undisclosed scope as a problem. Both issues were originally filed out of this PR for exactly that reason; folding them back in is a deliberate reversal, made at the author's request.
#96 widens the diff into
providers/, an area this PR did not previously touch. It is one commit (49d21bc) and is cleanly separable if a reviewer would rather it went tomainon its own.Five commits,
49d21bc..0f0ac58:49d21bcprovidersto the bash-3.2 scan73393b9shellcheck source=directives that resolved to nothing5a61b41hooks/directory under the mirror's ownershipc442f7e.vale.inireadability by reading it; stop paying for vale where nothing lints0f0ac58Four issue claims did not survive verification
.apm/prompts/as documented-but-never-mirrored.MIRROR_DIRSlists destination directories, and apm folds.apm/prompts/intocommands/(renaming*.prompt.md→*.md) — verified empirically against a scratch plugin. A plugin adding.apm/prompts/is mirrored today. Adding thepromptsentry the issue asks for would have named an output directory apm never emits. Pinned instead with a characterization test that fires if that mapping ever changes.tests/run-bats.sh:54(../scripts/lib/batch-run.sh) as "the correct spelling". Directives resolve against the source-path, which under pre-commit is the repo root, so../scripts/...escapes the repo and trips SC1091 exactly as the reportedlib/...does. Both were broken. The spelling satisfying both shellcheck andtest-vale-wrap.sh's two-candidate heuristic is repo-root-relative. A third instance the issue never mentioned,scripts/check-manifests.sh, was broken the same way.[[ -r ]]isaccess(2), which for uid 0 returns true even on a mode-000 file. This hook runs at pre-push and the environment is root, so it could never fire where it exists to fire. That is why no uid-independent test existed: there was nothing to test. Readability is now decided by actually reading.set -uon bash 5.x and aborts only on macOS's bash 3.2. A runtime test cannot demonstrate it without a 3.2 binary — which is why the enforcement is the static scan, and why its floor must never be dropped to zero.Two defects neither issue mentioned
scripts/check-manifests.shcarried the same unresolvablesource=directive (above).hooks/wipe trippedSC2115:set -uaborts on an unset variable but not an empty one, so an empty$target_dirwould make itrm -rf /hooks. Guarded with${target_dir:?}. This would have failed pre-commit, which pins--severity=warning.#97 item 5: fixed, but a net regression — read this before believing the 3.5×
test-check-vale-style-sync.shdrops 51s → 32s (~3.5× on its vale-bound cases) by masking vale in the 21 of 28 script runs that only assert.vale.initext, via thePATH_NO_VALEmechanism case 12 already builds. The helper falls back to an unmasked run rather than skipping, so a machine where masking is unavailable loses speed, never coverage.But the suite as a whole went 114s → 129s. The saving was more than eaten by the tests added here:
test-sync-plugin-content.shwent 37s → 64s, because each newhooks/-stray andpromptscase runs a realapm pack. The bottleneck moved rather than disappearing. Defensible for coverage on a generator whose output ships to users, but it is not a speedup and should not be recorded as one.The masking turned out to be a coverage gain, which matters more than the time. With vale on
PATH, cases 8 and 9 could not detect deletion of the assertions they were written to catch — a droppedStylesPathalso breaks the glob probe, so the script exited 1 for the wrong reason and both cases went green. Verified against the pre-change files: the same mutation was caught by one incidental assertion before, and by three after.Mutation evidence
Every new guard was verified to fail under the mutation it exists to catch, not merely to pass:
source=lib/batch-run.shFAIL … tests/run-tests.sh (0/1)"${parts[@]}"in the statuslineFAIL: bash-4-only construct(s) in 44 scanned script(s)providersglob at a wrong pathFAIL … derived 0 file(s), under its floor of 1hooks/wipe36 passed, 3 failedchecked_pathsback to the file39 passed, 2 failed.vale.inireadability guard →if false43 passed, 1 failedThe
.vale.inicase asserts the message, not the exit code: with the guard removed the script still exits 1, just blaming a missingStylesPathon a file that has one. An exit-code-only test would have been green with the guard deleted.Also caught by the new gate
The stricter
hooks/check found three emptyplugins/{git,gitea,core}/hooks/directories — untracked local cruft, not anything this PR ships (git does not track empty directories, so a fresh clone never had them).sync-plugin-content.sh --allcleared them and touched no tracked file.Review rounds
Four review rounds ran against this branch before the issues above. The fourth (two independent reviewers, ~15 sub-agents, per-cluster clean-context verification with re-fix loops) found and fixed 28 findings, committed as
f6cf83c,d4fa4b7,413a750,bb9158d,013b913.Highlights, because several were invisible to every earlier round:
tests/exclusion was depth-agnostic and deletedskill-author/assets/templates/tests/, a template the skill scaffolds from. The shipped scaffolder exited 2 in the installed plugin while the.apm/copy worked.--checkwas green about it.= false,= 0, lowercase= yesall bypassed a blocklist that should have been an allowlist), a drift check that never fired on compiled manifests or deletions, a mirror check that failed open, and an empty TAP stream reading as a pass.<plugin>/hooks.json, which Claude Code does not convention-scan — while ADR-0017 itself quotedhooks/hooks.jsonas the contract. Now relocated, with noplugin.jsonpointer added.json.dumps(indent=2, sort_keys=True)reproduced all three byte-for-byte, i.e.pretty-format-json --autofixhad rewritten them before an exclusion existed. Corrected and prevented from recurring.marketplace-author, is deleted by this PR. Now written intoapm-workflow's reference files.Most of the added lines are tests. Mutation testing showed the previous suite was green with the stale-skill wipe, three
MIRROR_DIRSentries, or the hooks newline normalization deleted; the current suite kills 18 of 19 mutations.Verification
There is no CI in this repo — no
.github/workflows/, no Gitea Actions. All gates are local pre-push hooks in.pre-commit-config.yaml, so merging via the Gitea web UI runs nothing. Merge from a local push, or runpre-commit run --hook-stage pre-push --all-filesfirst.Last run on
0f0ac58, all green:bash tests/run-tests.sh→ 16/16 scripts, 0 failuresapm marketplace check(network),apm audit --ci,apm pack --check-clean, and bothclaude-CLI validatorsshellcheck --severity=warningclean on all nine changed shell filesNote
apm audit --cicurrently runs a single check that passes because no dependencies are declared, and warns that org-policy enforcement was skipped — it is a weaker gate than its name suggests.Known accepted consequences
displayNameis dropped from all six compiledplugin.jsonfiles andowner.emailfrom the marketplace manifest —apm.ymlhas no equivalent key. Optional fields, so--strictvalidation still passes. Recorded in ADR-0015.mattpocock-skillsis now version-pinned (^1.2.0→v1.2.3) rather than tracking the default branch, and must be bumped manually.tools:restriction per ADR-0016;lint-runneris report-only by instruction, not capability.🤖 Generated with Claude Code
apm pack silently skips regenerating plugin.json when it already exists ("already exists; skipping plugin.json generation. Re-run with --force to overwrite it."). sync-plugin-content.sh never passed --force, so .claude-plugin/plugin.json and .github/plugin/plugin.json were written once on first sync and never refreshed again -- a name/version/description edit in a plugin's apm.yml would silently stop propagating, with no gate catching it (check-manifests.sh only checks presence; check-plugin-content- sync explicitly excludes plugin.json; apm-pack-check-clean is scoped to marketplace.json only). Pass --force on real-mode syncs only (--check must not mutate the plugin root, so plugin.json staleness there stays a known, undetected gap until the next real sync -- documented in the script's header comment). Regenerating surfaced a second, unrelated bug: apm's writer and pretty-format-json's --autofix disagree on JSON key order (semantic vs. alphabetical), so every real sync would otherwise flip plugin.json's key order right back after a commit re-alphabetized it. Excluded the four apm-generated manifests (plugin.json x2, marketplace.json x2) from pretty-format-json -- their exact formatting is dictated by apm's own writer as compiled output, not this repo's generic JSON prettifier. Verified idempotent: a second real sync after this fix produces zero further diff. Refs: #90Review-and-fix cycle complete
A code review of this branch (the issue #90 apm-conversion migration) surfaced 10 findings, all now fixed and independently re-validated against the diff (several re-tested empirically, not just via the test suite).
Findings fixed:
plugins/bin/.github/plugin/plugin.jsonwas missingmcpServers— apm's Copilot-ecosystem builder unconditionally strips it, contradicting this repo's own researched Copilot schema docs. Narrowly reinjected on real syncs.scripts/sync-plugin-content.sh's--checkclaimed to never mutate the plugin root, butapm packstill wrote manifest files into it on first-time creation.--checknow packs a throwaway copy instead.check-plugin-content-sync's pre-commit hook hardcoded the six plugin directories instead of deriving them. Added an--allflag (parsesmarketplace.json, matchingcheck-manifests.sh's approach) and simplified the hook to use it..apm/yet were both treated as SKIP/success. A missing directory now FAILs.tests/run-bats.sh's new parallel dispatcher usednproc/wait -n(bash 4.3+/GNU-only), breaking macOS bash-3.2 compat that its siblingtests/run-tests.shexplicitly targets. Ported the samegetconf+batched-waitpattern.tests/test-sync-plugin-content.shhad two competingtrap ... EXITstatements, silently leaking a tmp dir every run. Adopted the existingtrack()/CLEANUP_DIRSpattern fromtests/test-check-release-needed.sh.CONTEXT.md's Vale-prefilter glossary quoted stale pre-.apm/-migration regexes (plus a third occurrence the original review missed).sync-plugin-content.sh's dispatch loop — added the same bash-3.2-safe cap as (5).Note on finding #1's fix: reinjecting
mcpServersis a deliberate, narrow exception to ADR-0017's stated rejection of patching apm's compiled output. apm's own justification for stripping the field is factually wrong per this repo's documented Copilot schema, unlike the fields ADR-0017 correctly left alone (skills/agents/commands, which really are host-auto-discovered). Documented as an amendment to ADR-0017 rather than silently deviating from it.Commits:
9c140ef— sync-plugin-content.sh hardening + mcpServers reinjection + ADR-0017 amendment4003c6a— run-bats.sh bash-3.2 compat7c3c867— CONTEXT.md doc fixFull test suite, shellcheck, and all pre-push gates (including
apm audit --ci,apm marketplace check, plugin validation) pass.A prior review of this PR found it ready to merge overall, with one minor finding and one nit outstanding. Both are closed out by two follow-up commits just pushed:
Finding (minor) — fixed in
2c731eb:scripts/sync-marketplace-mirror.shhad no automated test coverage, unlike its siblingsync-plugin-content.sh. Addedtests/test-sync-marketplace-mirror.shwith 11 test cases (missing-source no-ops, drift detection in--checkmode, real-sync copy correctness, idempotency, usage-error handling). Verified the new tests actually catch regressions by mutating the script twice and confirming the right tests failed, then restored it. Full suite now 15/15 shell scripts + 165/165 bats tests passing.Nit — addressed in
560154c: the live discoverability probe cited in ADR-0017 (claude --plugin-dir plugins/kyberforge -p "list your skills and agents") hadn't been independently re-run. Re-ran it and confirmed it reproduces the documented fixed state (7 kyberforge skills + 1 agent discovered, matching.apm/1:1). Found that running it literally from the repo root gives a contaminated signal, since this repo's own project-level.claude/settings.jsonenables all 6 holocron plugins, conflating kyberforge's discoverability with the other 5 plugins'. Documented as a reproduction-note caveat in ADR-0017's Consequences section: rerun from a neutral cwd outside the repo with an absolute--plugin-dirpath to isolate the signal.No blockers remain from the review; PR is ready to merge as far as this review is concerned.
force_flag and plugin_dirs expanded unguarded under set -euo pipefail, tripping "unbound variable" on bash 3.2 (macOS) whenever the array is empty -- which is the default --check invocation wired into the pre-commit drift gate. Apply the same ${arr[@]+"${arr[@]}"} guard already used for seen_names in this file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DTRan a full review (4 parallel sub-agents + verification pass). Found and fixed 4 issues, all pushed in commits
b0b1470and911dadd:Fixed:
scripts/sync-plugin-content.sh:force_flagarray expanded unguarded ("${force_flag[@]}") underset -euo pipefail. Empty in--checkmode, which is exactly how the pre-commit drift gate invokes this script (--check --all) — trips "unbound variable" on bash 3.2 (macOS default), silently defeating the ADR-0017 drift check. Fixed with the same${arr[@]+"${arr[@]}"}guard the file already uses forseen_names.plugin_dirs, at 3 loop sites. Guarded identically.CONTEXT.md: skill-composition example list still citedplugin-author, which this same PR deletes (ADR-0015 successor isapm-workflow). Swapped in.CONTEXT.md: "Gitea is the canonical issue tracker" cited ADR-0017 (unrelated, this PR's new mirror-bridge ADR) instead of ADR-0007. Corrected.Verified clean: manifest parity across all 6 plugins,
.apm/content integrity (byte-for-byte spot-check vs main), mcpServers reinjection scoping, provenance chains, agent single-file format compliance, no secrets/injection issues, Conventional Commits on all commits.Tests: full local suite green both before and after fixes (15/15 scripts, bats). Push just now re-ran all pre-push hooks including
Check plugin content sync— passed, confirming the fix resolves the gate it protects.Recommendation: approve — no remaining blockers.
--check's throwaway pack copy seeded .claude-plugin/plugin.json and .github/plugin/plugin.json from the real plugin dir, then packed without --force -- apm pack silently skips regenerating a plugin.json that already exists, so the diff always compared the copy against itself and never caught drift in the compiled name/version/description/mcpServers. --force is now always passed; in check mode it forces regeneration inside the throwaway copy only, which sync_plugin_manifest() then diffs against the real committed manifest. sync_hooks_json() returned early whenever .apm/hooks/ was missing, without checking whether a stale hooks.json was still sitting at the plugin root from a prior sync -- unlike sync_dir(), which already detects that kind of orphaned mirrored output. It now mirrors sync_dir()'s shape: flagged as drift in --check, removed on a real sync. Running the corrected --check --all against this repo's own plugins surfaced 3 real orphans: plugins/{git,gitea,core}/hooks.json, empty stubs added in4edaaaconly to satisfy an old plugin.json pointer-field check that no longer exists (their compiled plugin.json has never had a hooks field, and none of the three ever had .apm/hooks/). Removed as part of this fix since they're exactly the drift the corrected check now catches -- leaving them would break the sync-plugin-content pre-push gate on this branch. Also extracts two shared helpers into scripts/lib/, sourced by this script and others so a future bug fix doesn't need hand-applying three times: - marketplace-plugins.sh: walks marketplace.json for local plugin dirs (this script's --all branch and check-manifests.sh had near-identical copies) - batch-run.sh: the bounded-batch concurrent job runner (this script, tests/run-tests.sh, and tests/run-bats.sh each hand-rolled the same core-count-capped wait loop independently) Extended tests/test-sync-plugin-content.sh with coverage for both drift cases (plugin.json version-bump drift, orphaned-hooks.json drift), including that a re-sync clears each. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT`parts` is seeded empty and all seven appends are conditional, so "${parts[@]}" at the join loop can expand an empty array. install.sh deploys this file to every user machine. Two things had to both hold for the bare form to be safe: this file enabling no `set -u`, and the shell being bash 4.4+, which stopped treating an empty-array expansion as unbound. On bash 3.2 -- macOS's system bash, an explicit repo target -- adding `set -u` aborts here. That is also why the hazard is unreproducible on a modern dev box and why the enforcement is a static scan rather than a runtime test. Adds the `providers` glob to test-vale-wrap.sh's bash-3.2 scan, which excluded it precisely because of this defect. Floor is 1 rather than "count minus slack": the glob holds one file, so any slack at all means a floor of 0, which passes vacuously on a renamed directory. Also adds case 27, the regression test for the stale `shellcheck source=` directives fixed in the next commit (#97 item 2). It lives in this file because that is where the exemption it guards lives. Closes #96 Refs #97 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DTreinject_mcp_servers copied .mcp.json's mcpServers into the compiled Copilot manifest verbatim via jq. apm's own path does not: collect_mcp_servers runs _sanitize_mcp_servers(), which drops env/headers/authorization and redacts secret-shaped keys, because copying them into a committed manifest exfiltrates them into the distributed artefact. The re-injection was the only route around that sanitizer, and it wrote to a tracked, marketplace-distributed file. Both host schemas document mcpServers as "string or object -- config path or inline definitions", so the pointer form is valid and carries no resolved content. It also preserves the ${VAR} indirection the sanitizer strips. Also in this pass: - mktemp+mv left the manifest at 0600 while --check compared content only, so a real sync silently demoted a mode the gate could not see - --check --all exited 0 when the marketplace yielded zero plugins, including on unparseable JSON: the one gate whose work list comes from a generated file could be silenced by regenerating its own input - sync_dir took an unguarded $target_dir despite a comment claiming otherwise - basename '.'/'..' escaped $SCRATCH_ROOT and made bundle selection arbitrary - path_manifest compared only the exec bit, so check and sync disagreed - sync-marketplace-mirror.sh fell back to pwd outside a worktree and reported no drift on a tree it never identified Mode comparison is deliberately files-only: directory modes come from umask on one side and checkout on the other and git tracks neither, so comparing them reports the runner's umask rather than a property of the mirror. Tests: 44 -> 67 and 15 -> 19 assertions, each verified to fail under the mutation it exists to catch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DTmattpocock-skills was pinned as the range ^1.2.0 with no lockfile, so apm re-resolved it against upstream on every pack. An upstream v1.2.4 would invalidate the committed ref/sha and fail apm-pack-check-clean with exit 4, blocking every push in the repo at an unrelated moment, triggered by a third party. ADR-0015 claimed the opposite -- that nothing advances it. Pinned to 1.2.3, which resolves to the already-committed sha, so the only compiled change is the version key the remote entry alone was missing. marketplace.owner.email was dropped on a false premise: ADR-0015 said apm has no key for it, but yml_schema.py defines _AUTHOR_OBJECT_KEYS as {name, email, url} and the key compiles through. Restored. (displayName is genuinely unsupported and stays dropped.) ADR-0016 dropped per-agent tools: because the allowlist shape is unportable -- Claude takes a comma list, Copilot a {Tool: true} map. That holds. But a denylist has no such conflict: disallowedTools is honoured by Claude Code and is absent from its plugin-subagent ignore list, and Copilot copies agent frontmatter verbatim so an unknown key is inert. gitea-orchestrate, apm-orchestrate and lint-runner were all write-denied on main and lost that fence silently; only lint-runner's loss was disclosed, and only lint-runner had prose to fall back on. All three regain the fence, and the two with no no-edit language gain three statements each. git-orchestrate is untouched -- it legitimately had edit. Four plugins shipped changed compiled output under unchanged versions, against the policy this PR itself wrote: bin 1.1.1->1.1.2, git 1.3.2->1.3.3, gitea 1.3.3->1.3.4, lint 1.1.5->1.1.6, each in both the plugin manifest and the root packages[] entry. Root catalog 0.3.3->0.3.4: patch, because the set of entries is unchanged and what moved is the owner block and four versions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DTapm's bundle exporter drops symlinks entirely, so a symlink under .apm/ never reaches the mirror -- and no gate could see it, because every existing check diffs the live mirror against a bundle-derived copy and both sides lack the file. It is an absence with nothing left to mismatch against, the only class of .apm/ content that vanishes without a trace. check_apm_symlinks reads the .apm/ source tree, where the loss is visible, and fails both modes. Reported rather than resolved: dereferencing would make a real sync emit content the bundle does not contain, which is the reimplementation ADR-0017 rejects. --check --all could also pass having verified fewer plugins than the marketplace lists: a plugin whose .apm/ had gone was SKIPped rather than counted, and the earlier floor only caught zero. The count is now checked against the marketplace's own local-package list. There is no exempt state -- ADR-0015 makes .apm/ the sole authoring source for every local plugin, so a listed plugin without one is drift. On the Copilot hooks gap, the decision is to document, not implement. Copilot declares no hooks path and apm emits none, which looks like the mcpServers case -- but that exception holds because .mcp.json is one host-agnostic format both ecosystems read, so a pointer to it is true whatever it contains. Hooks have no shared format: Claude expects nested matcher groups under PascalCase events, Copilot requires version: 1, camelCase, and a bash/powershell split. apm merges .apm/hooks/*.json into exactly one file, at Claude's convention path. A pointer would assert a Claude-shaped file is Copilot-shaped -- an incomplete manifest traded for a wrong one -- and it is not inert today either, since {"hooks": {}} lacks Copilot's mandatory version key. A test pins the decision, so restoring the pointer fails until someone confronts the schema mismatch. Tests: 77 -> 92 assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DTFifth review round: 15 commits (
0f0ac58..a4a075b)A multi-agent review ran against this branch, then an independent second round reviewed the fixes with agents that had not written them. That second round is where most of the value was — it found a regression the first round introduced, and several fixes that were narrower than their own commit messages claimed.
All 15 pre-push hooks pass.
run-testsnow runs in--strictmode, so that result means 18/18 suites actually executed rather than silently skipping.The security defect
reinject_mcp_serverscopied.mcp.json'smcpServersverbatim into the compiled Copilot manifest, bypassing apm's_sanitize_mcp_servers()— which exists because copying that file's contents into a committed manifest exfiltrates credentials into the distributed artefact. Proven: adding"env": {"TOKEN": "…"}toplugins/bin/.mcp.jsonproduced a stripped Claude manifest and a Copilot manifest carrying the live token. No gate caught it, becausecheck-plugin-content-synccompared the re-injected output against itself.Now re-injects the string pointer
".mcp.json". Both host schemas type the fieldstring | object, so the host resolves it at load time and no resolved content — sanitized or otherwise — enters the manifest. This also preserves the${VAR}indirection apm's sanitizer would strip.A regression this round introduced, then fixed
Widening the mirror's mode check from the exec bit to full permissions made
check-plugin-content-syncfail at pre-push on a pristine tree for anyone on a umask-002 distro — and unfixable by committing, since git records no non-exec mode.hooks/hooks.jsonis written by the sync at the runtime umask, not copied from the bundle.Resolved with a rule rather than a patch: record a mode for a path the pipeline copies, never for one it writes. Verified on clones checked out at umask 022, 002 and 077.
Gates that passed while verifying nothing
Nine, all confirmed by constructing the failure condition and watching the gate exit 0:
sync-plugin-content --check --allcheck-manifestssource(null != "string"is true, so both passes skipped it at once), or the manifest was missing entirelycheck-vale-style-syncvalewas off PATH — the probe self-disabled on the exact one-character glob typo it exists to catchcheck-scope-walkup-sync.apm/was relocated (no floor existed, contrary to an earlier claim that one did)run-tests.shrun-bats.shwas missing, non-executable, or empty — 166 bats tests vanished with a green summaryrun-bats.sh.batsfiles were discovered, or a status file was empty ([[ "" -eq 0 ]]is arithmetic-true)run-tests(pre-push)vale— the gate reported success having run 14 of 17apm-audit-ci--allrun-bats.sh's failure-detection line survived three separate mutations because real bats emits both a nonzero exit andnot oklines, so each signal masked the other. The new stubs emit each without the other.An operational time bomb
mattpocock-skillswas pinned as the range^1.2.0with no lockfile, re-resolved on everyapm pack. An upstreamv1.2.4would have invalidated the committed ref/sha and failedapm-pack-check-cleanwith exit 4 — blocking every push in the repo, triggered by a third party, at an unrelated moment. ADR-0015 claimed the opposite ("nothing advances it"). Now pinned exactly.Silently dropped safety restrictions
gitea-orchestrate,apm-orchestrateandlint-runnerwere all write-denied onmainand lost that fence in the conversion. Onlylint-runner's loss was disclosed, and onlylint-runnerhad prose to fall back on.ADR-0016 dropped
tools:because the allowlist shape is unportable — Claude takes a comma list, Copilot a{Tool: true}map. That reasoning holds. A denylist has no such conflict, so the fence is restored viadisallowedTools, plus explicit no-edit prose on the two agents that had none.git-orchestrateis untouched — it legitimately hadedit.Reconciling that took six more files than expected:
agent-authorwould have stripped the fence it had just been given, and the validator's permit-list cannot catch an absent field. Rosters are now pointers tofield-inventory.md— the same data the validator reads — and a bats case enforces all-or-nothing, since a partial roster is the shape that goes stale silently.Two decisions to not fix
The Copilot hooks gap is documented, not patched. Copilot declares no hooks path and apm emits none, which looks like the
mcpServerscase — but that exception holds because.mcp.jsonis one host-agnostic format both ecosystems read, so the pointer is true whatever it contains. Hooks have no shared format: Claude expects nested matcher groups under PascalCase events, Copilot requiresversion: 1, camelCase, and a bash/powershell split. apm merges into exactly one file, at Claude's convention path. A pointer would assert a Claude-shaped file is Copilot-shaped — an incomplete manifest traded for a wrong one — and it is not inert today either, since{"hooks": {}}lacks Copilot's mandatoryversionkey. A test pins this, so restoring the pointer fails until someone confronts the schema mismatch.policy.fetch_failure_default: blockwas rejected on evidence. apm's org-policy discovery understands github.com and Azure DevOps; this repo's remote is self-hosted Gitea, so no policy source is discoverable and the setting makes the hook exit 1 on every push forever. Fail-closed is right when there is a control to fail closed on — a permanently red gate is one people learn toSKIP=.One flake, investigated and not fixed
A probe failure reproduced once under parallel load. 1680 probes across three contention setups, including an offline namespace, came back clean. It was also misattributed — reported against
tests/test-vale-wrap.sh, which never invokes the script that emits it; the assertion belongs tocheck-vale-style-sync.shand reaches a log through a different suite.No speculative fix was made to a pre-push gate. Instead the reason it was undiagnosable is fixed: the probe discarded vale's exit code and output, so a failed exec, an OOM-killed vale or a full
TMPDIRwas indistinguishable from a real glob defect. The message now separatesvale exited 0 … in 0 files(genuine glob defect) fromvale exited 137; output: <empty>(vale died).Corrections to this PR's own prior claims
.vale.inireadability fix is not uid-independent — as root,caton a mode-000 file still succeeds. The fix is still an improvement (failure attribution is fixed and mutation-verified), butc442f7e's stated rationale was wrong.hooksfield. The real list is(agents, skills, commands, instructions)—hooksabsent,instructionsunmentioned — and the loop can never fire, becausesynthesize_plugin_json_from_apm_ymlemits only eight identity fields.owner.emailwas dropped on a false premise;yml_schema.pyaccepts it. Restored.apm-pack-check-cleanalso needs the network, and the same paragraph forbade adding anything else toSKIP.PROBES_CHECKEDguard); the guard's two reachable triggers had no test and now do.New gates
check-apm-agents-valid—agent-audit's validator had never run against the four real agent files it governs, only synthetic fixtures. That is precisely why an amended ADR and a validator that rejected the field it blessed could disagree unnoticed.run-bats.shderives its expected.batsset fromgit ls-filesinstead of a hardcoded floor — the old floor of 8 against a real 10 let two files and eleven tests disappear green.Pre-push: 12 → 13 repo-defined hooks (15 total). AGENTS.md's counts, hook list and offline note updated to match.
Verification
Every behavioural fix is mutation-verified — test written, fix reverted, test observed RED, fix reapplied, GREEN. A test nobody watched fail was not counted.
bash tests/run-tests.sh --strict→ 18 passed, 0 skipped, 0 failed; 166 bats testssync-plugin-content --check --allexits 0 under umask 022, 002 and 077shellcheck -x --severity=warningclean across all changed shell filesapm pack→ both syncs → emptygit statusKnown-open, deliberately out of scope
Three doc defects that pre-date this PR:
ADR-0011's staleplugins/bin/skills/gitea/reference,LESSONS.md's citations ofscripts/gitleaks.tomlandtests/test-plugin-validate.sh(neither exists), andarchitecture.md:45calling the enforcement layer "Phase 2 (planned)" while 15 hooks run today.There is still no CI in this repo. Merging via the web UI runs nothing. Merge from a local push, or run
pre-commit run --hook-stage pre-push --all-filesfirst.🤖 Generated with Claude Code