refactor(skills): retrofit the corpus to the ADR-0020 context contract #129
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Defame1297/holocron#129
Reference in New Issue
Block a user
Delete Branch "refactor/adr0020-skill-retrofit"
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?
Retrofits all 39 skills to ADR-0020's description/body context contract, then fixes what six rounds of independent review found in that retrofit — including four ways the hot gate itself failed open.
Closes #99, #107, #108, #110, #111, #114, #115, #120.
The retrofit (waves 1-5)
Kyberforge.CompositionNoteUnder the 12,000-char success criterion. Per-wave detail is on #99.
The review fixes
The gate failed open four ways, three of them found after the retrofit shipped. An unrecognised follower token made a dangling target vanish. A skill directory with no
SKILL.mdresolved as a valid target, so a commit could be green locally and red in a fresh clone — three existing fixtures were relying on that, one of which made the install-leak A/B pass vacuously. Then the free-standing/namesweep turned out to be gated on the sentence carrying a boundary marker, so route notation in any other sentence was invisible — not an ERROR, not a SUGGESTION, not an INFO — which left the documented "/namealways blocks" promise false from a second direction. All four fixed and pinned.Two checks were silently not running.
validate-provenance.shchecks 7-8 were dead across nine skills. Waking them exposed a deeper problem: they assumeResearch doc:names a source index, but 30 of 121 entries point at topic content documents, so every new check-7 INFO was a false positive and check 8 was saved from a false-FAIL flood only by an unannounced skip. Checks 7/8 are now scoped to source indexes and every skip announces itself (#121).The retrofit's own anti-goal, four times. ADR-0020 warns that a blunt gate gets satisfied by deleting content rather than relocating it.
diagnoseandskill-auditrelocated prose and then read it unconditionally;prototypeandvale-configdeleted rules outright that survived nowhere. All four addressed.Verification
bash tests/run-tests.sh --strict— 24 suites, 0 skipped, 0 failedbash tests/run-bats.sh— 325 tests, 0 failurespre-commit run --all-files— 17/17pre-commit run --hook-stage pre-push --all-files— 16/16, withapm marketplace checkandapm pack --check-cleanrun against the remote, not skippedscripts/skill-size-check.shover all 39 skills — rc 0, 0 ERROR/FAIL, SUGGESTION-onlyPer-commit sync, stated accurately: the ten commits from the latest review round each pass
check-plugin-content-syncin isolation, verified by checking each out in a detached worktree with a clean between. The earlier gitea window (dfacf05..bedbd1d, nine commits) does not — its mirror was regenerated in one batch atbbc7300. An earlier revision of this description claimed the property held for every commit; it does not, and a bisect through that window lands on a red commit. Squash-merge to collapse it, or accept that this range is not bisectable.Version bump
Six plugins and the catalog take a patch, not a minor. The branch is 89 commits — 40
fix/ 30refactor/ 12docs/ 5chore/ 2test— zerofeat, zero!, zeroBREAKING CHANGE— and adds no skill, agent, command or hook. (Two earlier revisions of this section cited a stale histogram, most recently 78 commits; the figures above are measured at HEAD.) Both rules this repo ships (forge/references/version-bump.md, landing in this PR, andgit-commits/references/conventional-commits-spec.md) make that a patch, and the catalog set is unchanged at 7 entries.Not settled by that: four published files were removed from the installed tree, three moved, and
cavemangaineddisable-model-invocation, retiring its old triggers. Under a strict reading those are major-class and currently ship underrefactor:with no marker. Whether the deployed skill surface is a public contract is written down nowhere — worth deciding, but it outlives this PR.Deliberately not in scope
#112 (cherry-pick ownership, now resolved in favour of
git-commits), #113 (rtk gitnormalisation), #116 (research fan-out), #101 (audit-skill merge), #122 (non-spec skill-root files), #123 (no PRD producer) stay open. #117 is the one worth reading: the contract's remedy is to move prose intoreferences/, which is exactly where neither the size gate nor Vale looks — and the blind spot is wider than #117 currently records, since there is no root.vale.iniat all, so every ADR,CONTEXT.mdandREADME.mdis unlinted too.That blind spot let this branch carry two
level: errorKyberforge.SentenceOpenerThereIsviolations intoreferences/files it created —provider-adapter-author/references/provider-matrix.md:31andagent-audit/references/finding-criteria.md:95. Both are reworded inafadaae, confirmed by routing each file through the audit's ownvale-wrap.sh(1 error each before, 0 after). Five further occurrences sit inreferences/files already onmain; those are the pre-existing corpus and stay with #117, which is the real fix.Also unfixed and not this PR's:
apm installappends a duplicateSessionStartentry to.claude/settings.json, so a fresh clone cannot get pre-push green without an edit AGENTS.md warns against. Reproduces identically onmain.tests/test-adr0020-targets.sh pinned the corpus dangling set as exactly {gitea-labels, neuledge-context} and tests/test-skill-size-check.sh probed both individually. The gitea-issues retrofit cut the composition sentence whose '>'-folded scalar produced 'gitea-labels- milestones', so that target no longer exists and both suites went red. EXPECTED_DANGLING is now {neuledge-context} and the gitea-issues probe is removed rather than skipped, per the rule the probe file states about itself: a probe whose fixture has been retrofitted is a pin that needs updating, not an assertion-free pass counted in the totals. The exact-set assertion stays. An empty expected set is still valid and still pins that no new dangling target appeared -- which is what it becomes once research is retrofitted. Both loops carry a shellcheck SC2043 waiver for the same reason: one entry is the expected steady state, not bad quoting. Refs #99The header promised explicit route notation always blocks. It did not: /name reached extraction only behind a ROUTE_VERB, so a target with no verb before it was never extracted at all -- exit 0, no output. Taking the SUGGESTION's own advice ('write it as /name and it will be checked properly') was the one edit that blinded the gate. Adds two notation sweeps gated on BOUNDARY_MARKER and routed through _add, plus a path guard so file paths and URLs are not read as routes. Also excises the matched pointer span before the REFERENCE_PAST sweep, so a reference file can no longer exempt itself by its own filename, and guards the agent branch with the isfile test the skills branch already had. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJJrm5YmacbwMdzZpXcotiReview + fixes (7 commits,
5b80f30..971e148)Full review across seven parallel reviewers, then fixed what it found. Verification of the retrofit itself first, since that is the bulk of the PR and it holds up.
The headline claims verified
Measured independently, not taken from the description: 39 skills, 0 descriptions over 400, 0 bodies over 900, preload tax 9,450 chars (under the 12,000 criterion). Per-run SKILL.md body volume down 29% (26,016 → 18,343 words) while reference volume is up 4,434 — content genuinely relocated, not deleted.
run-tests.sh --strict24/24, 0 skipped.pre-push --all-files16/16 including both network hooks. 71 commits, all Conventional. No secrets. Theremotes.mdandexamples.mdsplits are lossless item-for-item, and the 22 new provenance cases are all non-vacuous — each dies under a targeted mutation of the behaviour it names.The gate was still failing open — three ways
This is the one that mattered. The PR closed two fail-opens; the class stayed open.
A dangling
/namestill vanished when no route verb preceded it. The fix addressed tokens after the target, but extraction required aROUTE_VERBbefore it — so— /no-such-skill instead.,; /no-such-skill handles that.,(/no-such-skill covers it).and friends were all exit 0, zero output. The documented "/namealways blocks" promise was still false, just from the other side.Worse, following the gate's own printed remedy silenced the finding. The SUGGESTION says "write it as
/nameand it will be checked properly" — taking that advice made the finding disappear entirely. Same shape as the buggates.md:164-170says was already fixed.The
(none)-vs-unparsable fix reached only check 8. Checks 4/5 used a truthiness test, so a parse failure disabled them silently — exactly what the parser's own docstring warns against. Two live entries were skipping: a sweep of all 32sources.mdfound 134 entries, exactly 2 parsing toNone, both ingitea-files(one heading carried an inline parenthetical that defeated both regexes; one(none)lacked its bullet).All fixed in
c232e69and00daf28, with false-positive guards so paths and URLs aren't read as routes, andagent-auditgaining the INFO tier it never had.Content loss — the specific answer
Aggregate volume is up, but there were real losses, concentrated where the word gate does not look.
Descriptions were cut to a mean of 242 against a 400 budget — 6,150 chars unspent corpus-wide, 24 of 39 under the soft floor — and shed routing with them.
gitea-prslost the issue/PR number-space directive the whole gitea suite is architected around, at 163/400.git-workflowcalled itself a router and named 2 of its 6 domains.gitea-workflowhad a boundary clause with no target at all.Three outright deletions, each ADR-0020's stated anti-goal:
provider-adapter-author's standalone/composed-into rule (deleted in the same commit that broadened its composed path),git-submodules'foreachvariable semantics ($sm_pathvs$displaypathdiffer only by cwd — unrecoverable from the names), andgitea-releases' tag-creation hedge plus its verify step, leavingconventions.mdasserting the unconfirmed behaviour as fact.One factual contradiction:
gitea-issuesstated flatly that a merge never closes an issue, whilegitea-prs'merging.mddocuments that closing keywords in commits landing on the default branch do. Both now agree.researchmoved its body out and read both references unconditionally — word count moved, per-run context did not. Five bin skills had their boundaries in README.md, which nothing loads, while the gate still reported them boundary-less.Fixed in
1a971ee,ae79178,4011d14,a8cd5e8. Also: BOM hid the adapter import line (failing with "no reference to AGENTS.md" on a file whose first line is@AGENTS.md), and usage errors shared exit 1 with real findings.Two claims in the description that don't hold
bbc7300. Proven:sync-plugin-content.sh --check --allexits 1 withDRIFTatbedbd1d. My 7 commits each pass the check in isolation.mdx2vastinstalled to reach the native MDX path). But only one replaced a wrong statement; two replaced silence. Worth correcting, because that commit message is the audit trail for the newhouse-vale-3-15-2-reproslug.Docs corrected in
971e148: ADR-0020 amended (dated amendment, per the ADR-0016/0017 convention) for the notation-vs-follower reversal, the "exhaustive" Enforcement table completed with thedisable-model-invocationcarve-out, the stalezoom-outclaim de-pinned;gates.md's ERROR row fixed, thereferences/Vale blind spot documented with both its independent causes, and the undocumentedskill-frontmatterhook written up.Left for you to decide
The version bump. All six plugins took a minor, but
version-bump.md— shipped in this PR — says minor is for new capability and patch for fix/refactor, and the branch is 30refactor, 25fix, 10docs, 4chore, 2test, zerofeat. Pulling the other way, four published files were deleted from the installed tree andcavemanstopped answering its old triggers, which a strict reading calls major. Minor is defensible only if the public surface is "the set of deployed skill names" — which is written down nowhere. Either bump to patch per the shipped rule, or record that contract soversion-bump.mdstops arguing against the release.Scope creep worth naming:
rename_branch, three PR review-thread methods, thelist_issuestype/milestonesreversal, and a full inversion of the label-exclusivity rule (which changes what gets written to live repos) all rode in on a formatting refactor. With 2 eval files for 39 rewritten routing descriptions, nothing would catch a routing regression.Also found, not fixed: a wider blind spot than #117 records — Vale lints no repo doc outside skill files at all, so
gates.md, the ADRs andCONTEXT.mdget no prose linting from any hook.Verification after the fixes
run-tests.sh --strict24 passed, 0 skipped, 0 failed · bats 260, 0 failures (+6) ·pre-push --all-files16/16 · corpus gate 0 FAILs, rc=0 · provenance corpus FAIL 0→0 · each of the 7 commits passescheck-plugin-content-syncstandalone. Every new test case proven non-vacuous by mutation.🤖 Generated with Claude Code
The branch is 31 fix, 30 refactor, 11 docs, 4 chore, 2 test — zero feat, zero `!`, zero BREAKING CHANGE — and it adds no skill, agent, command or hook. Two rules shipped in this repo both make that a patch: forge's version-bump.md, which lands in this very branch ("minor for new capability, patch for a fix/refactor"), and git-commits' conventional-commits-spec.md, which maps fix to PATCH and refactor to none. Minor was the one answer neither rule produces, and the release was arguing with a policy it was simultaneously introducing. bin 1.1.6, core 1.1.2, git 1.3.6, gitea 1.3.7, kyberforge 1.6.1, lint 1.1.7 The catalog goes 0.5.0 to 0.4.6 for the same reason: apm-workflow's marketplace.md reserves a catalog minor for a packages[] entry added or removed and assigns patch to an existing entry's version moving. The set is 7 entries on both sides with unchanged names, so only the patch trigger applies. Each number is +1 patch on the pre-bump value rather than stacked on the minor, and executables.allow follows kyberforge to 1.6.1 so the ADR-0019 SessionStart hook does not orphan. Note what this does not settle: four published files were removed from the installed tree on this branch, three more moved, and caveman gained disable-model-invocation, which retires its old triggers. Under a strict reading of the repo's own breaking rule those are major-class and they currently ship under refactor: with no marker. Patch is right for the code; whether the deployed skill surface is a public contract is still unwritten, and that question outlives this commit.Second full review + fixes (10 commits,
971e148..0427422)Nine parallel reviewers over disjoint scopes, then eight implementation agents plus a cleanup pass. 25 of 29 findings are fixed in this round; the remaining four are either already tracked or not this PR's.
The headline claims re-verified
Measured independently at the new HEAD: 24 suites / 0 skipped / 0 failed, 325 bats / 0 failures (was 260), 17/17 pre-commit, 16/16 pre-push with both network hooks genuinely run, gate rc 0 / 0 ERROR. All ten new commits pass
check-plugin-content-syncin isolation, checked out one at a time in a detached worktree.Two claims from the previous round were wrong in the PR's favour and are corrected in the description:
references/grew +16,440 words, not +4,434 — off by ~3.7×, and it lands entirely in the corpus nothing lints — and the branch was 78 commits, not 71.The gate was still failing open, a fourth way
The previous round closed three. The class stayed open: the free-standing
/namesweep sat insideif boundary:, so route notation in a sentence carrying no boundary marker was never extracted at all.Found independently by two reviewers. The precondition existed only in a code comment while
gates.md:190,:206and the ADR amendment all promised the opposite. Only/nameis ungated —-> namealso writes a process chain and a code span cites tools, files and skills alike, so ungating either fires on prose.And a new false positive, the first blocking one rather than a fail-open. The path guard used
\b, which still holds after a hyphen, so the engine backtracked to a shorter prefix whenever the lookahead rejected the full segment.see /api-docs/v2.md for the schemaraised blocking ERRORs for'api'and'api-docs'— names no author wrote, with no corroboration escape.The fix that closed the previous three was pinned by nothing. Deleting the entire sweep left every suite green: 22/22, 53/53, 48/48, 23/23, 22/22, 7/7. Every
/namefixture also carried a route verb, so an older path caught them all. It now kills eight assertions, and the suite has its first slash-path and URL fixtures in either direction."0 new provenance FAILs" was true by accident
Checks 7/8 assume
Research doc:names a source index. 30 of 121 entries point at topic content documents whose H2s are topics like## Core Philosophy— so every new check-7 INFO was a false positive, and check 8, aimed at documents carrying noStatus:line at all, would have emitted a false-FAIL flood. The only thing preventing it was an unannounced skip. The clean bill rested on exactly the fail-open class this branch exists to remove, and fixing that skip naively would have turned the PR red.Now scoped to source indexes with every skip announced: 36 INFOs of which 30 were false → 56 of which none are, FAILs still 0. Six further silent-pass paths closed, three of which this branch had already fixed one directory over without porting (BOM handling, UTF-8 pinning, exit-code split). The
agent-auditcopy carried all of them unfixed and is now ported — minus four that are genuinely N/A at agent scope, and preserving its silent exit 0 for a file outside plugin scope, which is a verdict rather than a skip and is pinned bycheck-scope-walkup-sync.sh.Documents the whole corpus was retrofitted against were wrong
Five files —
contract.md×2,retrofit.md,description-quality.md,finding-criteria.md— stated that a prose-form dangling target blocks. Verified on fixtures:/nameand-> nameare blocking ERRORs, prose form is SUGGESTION at exit 0 unless corroborated.skill-auditwas additionally self-contradictory, importingvalidate.sh's SUGGESTIONs into Structure while its own rubric graded the same target a FAIL, so one target got reported twice at two tiers.Content deleted, not relocated — twice more
prototype/references/logic.mdlost three anti-patterns including "Don't generalise", surviving nowhere, in a file already 117 words under the suggestion tier.4011d14restored the byte-identical defect in the siblingui.mdwith exactly that reasoning and left this one.vale-configlost "settings in a glob section only apply to files matching that glob" — the most common Vale misconfiguration.Corrections that write to real systems
gitea-files' always-loaded Gotcha was false.withLines: truereturns plain JSON text while the response still reports"encoding":"base64"— verified live. An agent following the recommendation two sentences later decodes garbage, with the response's own field confirming the wrong answer.git rebase --autosquash HEAD~Nwithout-iis a silent no-op on git 2.39.5, and was the preferred flow. Reproduced: "Successfully rebased",fixup!untouched, same SHA.git fetch origin main:probefast-forwards the local branch,+force-updates it.git worktree add --orphandoes not exist before 2.42; on 2.39.5 it is exit 129.git-workflowclaimed all requests resolve to one of six skills while rebase, reset and stash had no owner —git resetappeared nowhere in the plugin. Those now have owners and procedures.gitea-orchestratenever learnedrename_branch, so an agent caller got "unknown operation" — added behind the confirm gatebranches.mddemands, not just to the enum.Two earlier findings resolved in the PR's favour
The label-exclusivity inversion is correct at HEAD and
mainwas wrong — everyKind/*label on this instance isexclusive: false, everyPriority/*/Status/*istrue, so main's blanket replace-within-scope would strip a co-appliedKind/Bug+Kind/Security. Same for thelist_issuestype/milestonesreversal. Both verified against the live v1.7.0 schema. The scope-creep objection stands on process, not accuracy. All four Vale corrections also reproduce against 3.15.2.Version bump
Reverted to patch across all six plugins and the catalog, per both rules this repo ships — one of which lands in this PR. Details in the description.
Left open deliberately
#112, #113, #116, #101, #122, #123 stay as they are. #117 needs its scope widened: there is no root
.vale.iniat all, so the blind spot is every non-skill markdown file in the repo, not justreferences/— and this branch demonstrated it by introducing alevel: errorviolation atprovider-adapter-author/references/provider-matrix.md:31that no hook can see.Not fixed because it is not this PR's:
apm installappends a duplicateSessionStartentry to.claude/settings.json, failingapm-audit-cion a fresh clone. Reproduces identically onmain.🤖 Generated with Claude Code