feat(kyberforge): ADR-0020 context contract for skills and agents #103
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Defame1297/holocron#103
Reference in New Issue
Block a user
Delete Branch "refactor/trim-skills-agents-context"
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?
What
Skill
name+descriptionpairs are preloaded into every session — ~6,200 tokens across 39 skills before any skill is invoked. This branch sets a contract that bounds that, adds blocking gates for it, and retrofits kyberforge's own four author/audit skills to comply.Full rationale in
docs/adr/0020-skill-description-and-body-context-contract.md.Why the descriptions grew
Not drift — the rules mandated it.
skill-author/SKILL.md:104required indirect triggers anddescription-quality.md:21required authors to "err toward being pushy", both enforced. The rule that would have deflated them,skill-author/SKILL.md:102("not the skill's internal mechanics"), was judgment-only with no FAIL condition behind it. The enforced rules inflated; the deflating rule never bit.There is also a correctness argument, not only a token one.
writing-skills/SKILL.md:154-158records a measured failure where an agent followed a description's workflow summary instead of reading the body.git-commitswas exactly that shape — 74% capability enumeration, including a rules table an agent could act on without ever loading the skill.Gates (blocking, no baseline file)
---The pre-existing 500-line / 2,770-word spec backstop is unchanged and still counts the whole file including frontmatter. These are two independent gate families; a file can sit well inside one and fail the other.
Also added: every boundary-clause routing target must resolve to a real skill or agent. It reports 3 findings across all 43 artifacts with zero false positives, and caught
gitea-labels- milestones(a stray space from YAML folding) without being told about it.Agents take the description gates but deliberately no body gate — a skill body competes with the caller's live conversation, an agent body becomes the system prompt of a fresh context. A test pins that absence so it cannot be "fixed" into consistency.
Result
skill-authoragent-authoragent-auditskill-auditAll four now use the dispatch pattern — body carries the dispatch table plus common gates, each branch self-contained in
references/.apm-workflowwas the exemplar.Two pre-existing gate failures fixed along the way
Both were red at
HEADbefore this work started, and both were invisible in an ordinary local run:RUN_TESTS_STRICTleaked intotest-run-tests.sh's fixture children. The meta-test is itself a suite the runner discovers, so under the gate's own invocation the variable propagated down and flipped fixtures strict. One control case failed; six more were silently asserting against the wrong stream.bash tests/run-tests.shwas green while pre-push was red for everyone.pretty-format-json --autofixwas re-sorting apm's output..claude/settings.jsonis apm-owned (ADR-0018/0019) but was missing from the hook's exclude list, so since2e395a4it has been committed in a key order apm would never write — permanentapm audit --cidrift on a file with an emptygit diff. Content was always byte-identical; only key order differed.Verification
All 16 pre-push hooks pass. Test suite green in all three modes (
bash tests/run-tests.sh,--strict, andRUN_TESTS_STRICT=1).Follow-ups
pre-commit run skill-size-check --all-fileslists every violation with its measured value.skill-audit+agent-audit; reopens ADR-0008.Note the gates ship hot with no baseline, so editing any non-compliant skill requires retrofitting it first. That is deliberate, and #99 is prioritised accordingly.
Three ways the gates could report green having measured nothing. All three were invisible to a passing test suite, because pre-commit prints nothing at all for a hook that exits 0 — a gate that declines to check and a gate that checked and passed produce the identical signal. - A UTF-8 BOM, a leading blank line, a trailing space after a `---` marker or CRLF line endings defeated the `^---\n` frontmatter matcher. Every ADR-0020 check was then skipped and the file passed: measured at the time, a 550-character description with a 1,000-word body exited 0 behind a BOM. All four shapes are now tolerated, and frontmatter that genuinely cannot be parsed is a hard ERROR rather than a silent skip. - An agent file with a valueless `description:` followed by another key let a line regex capture the *next* key, which looked non-empty, so the missing-or-empty branch never fired and every gate below it early-returned on the empty folded value — zero output, exit 0, on a blocking gate. The one field this contract is entirely about was the one field a gate could fail to notice was absent. Presence is now decided on the YAML-folded value and nowhere else, and a missing or empty description is a hard FAIL in all three validators. - The hand-rolled frontmatter fallback disagreed with PyYAML across the FAIL boundary on folded scalars, so which reader happened to be available decided the verdict. A fallback that mis-parses a scalar shape reports a vacuous pass, which is worse than not running, so it is deleted: python3 and PyYAML are hard requirements that fail loudly with an install pointer. Boundary-target resolution no longer derives its universe from its own location. A `${BASH_SOURCE}`-relative repo root leaked this repo's 39-skill universe into every consumer repo running the hook through pre-commit, so a consumer skill routing to `skill-audit` resolved against a plugin it had never installed. The interim form resolved through `.claude/` and `.agents/`, which are gitignored `apm install` output — the same commit reported 2 dangling targets on a machine that had run the install and 6 on a fresh clone. Resolution now walks up from the file being checked to an authoring root (nearest ancestor holding `plugins/*/.apm/{skills,agents}`, else the nearest `.git`, in two passes so a nested `.git` cannot outrank a real monorepo root); the universe is every skill and agent under `<root>/plugins/*/` plus the file's own apm package and that package's declared `dependencies.apm`. Deployed trees are consulted only when no authoring root exists at all — the consumer case. One commit now gets one verdict, which a gate shipping hot with no baseline file has to. Narrowed in the same pass: a routing target inferred from the prose boundary form and corroborated by nothing else reports at SUGGESTION instead of blocking. A blocking check with no escape hatch is the wrong trade when the inference from prose is the weak part of it. New deterministic checks, all previously untested or absent: every `references/<file>.md` a body names must exist (ERROR — a broken pointer is not a style opinion); a description with no boundary clause at all, a Gotchas section over five entries, and a Gotchas section over 25% of the body are SUGGESTIONs. Where no universe can be determined the target check prints `INFO ... DID NOT RUN` rather than passing quietly. Each prose-scanning check needed its own false-positive fix — a fenced example of a Gotchas section was being read as the section itself — and those fixes are pinned rather than assumed. The resolver is one block copied verbatim into all three scripts between BEGIN/END markers, because a cache-installed plugin's scripts cannot read outside their own plugin directory. Nothing asserted the copies were still identical; a one-line edit to a single copy passed every constant-agreement assertion, since constants are not what drifts. Tests land here rather than in a later commit. The existing suites assert the old behaviour and go red against these scripts, so splitting them would leave a commit whose own `run-tests` pre-push gate fails in isolation. Refs: ADR-0020Review, and the fixes it produced
Ran a multi-dimension review over this PR — gate logic, constant duplication, test quality, generated-content integrity, retrofit content conservation, doc accuracy, kyberforge self-compliance, and readiness to actually drive #99. It produced ~50 findings, of which 41 are fixed here, 3 are deferred with a record, and the rest were corrections to the review's own claims.
The seven commits from
b6e68e9toe7ebc66are that work.Three defects passed a fully green gate
This is the part worth taking seriously. All 16 pre-push hooks and all 200 bats tests were green while each of these was live:
re.match(r'^---\n', ...)missed, and the code hitcontinuewith no output. ASKILL.mdwith a 550-character description and a 1,000-word body exited 0 with zero findings. A leading blank line, a trailing space after the opening---, or a missing closing---did the same. The code comment deferred the case toskill-frontmatter— which is two greps that pass on all of those inputs. The deferral was to nothing.description:passed a blocking pre-push gate with no output at all.^description:\s*(.+)underre.MULTILINElet\s*cross the newline and capture the following key, sodesc_valwas truthy and even the pre-existing "description is missing" FAIL never fired..claude/skills/, which is gitignoredapm installoutput. Same commit, 2 dangling targets on a developer machine and 6 on a fresh clone. Four cross-plugingitea → gittargets resolved only through deployed output.None of these was reachable by running the test suite. Each needed someone deliberately trying to break the gate.
What changed
b6e68e9${BASH_SOURCE}-derived resolution replaced with an authoring-root walk-up from the file being checked, and the dangling-target check narrowed so an uncorroborated prose target reports at SUGGESTION instead of blocking a commit with no escape hatch. Tests ship in the same commit.a85bdbeskill-audit's manual structural fallback and E100 diagnostic. Without them it audited nothing structural whenpython3was absent while still printing a coverage line, and misread a vale config error as "vale missing".agent-audithad kept both — the asymmetry was the bug.2540e50skill-author/references/retrofit.md. Four dry-run retrofits established that the previous instruction mandated a retrofit and supplied no procedure, so each agent invented six to ten decisions. Following the new file tookgit-historyfrom 450 chars / 1,044 words to 189 / 247 with both validators green.311e7cdagent-audit's own vale rule makes it a hard error for that file type, and the exemplar mis-cited as a 554-word body whenapm-workflow's body is 421 (554 is whole-file — the exact conflation this ADR exists to stop).d02765dRUN_TESTS_STRICTfixed at its source. Also corrects the narrative:--strictnever leaked (it sets a shell local), only the env spelling did, and the blast radius was two assertions, not six.64ffb9fskill-audit+agent-audit" decision is now recorded as DEFERRED with an issue number rather than stated as done.e7ebc66executables.allowkey moves in the same commit — splitting it silently stops kyberforge'sSessionStarthook deploying.False positives removed
The gate ships hot with no baseline and no suppression mechanism, so a false positive blocks a commit with no way out. Two classes were found and closed:
Do not use for running hooks — run `pre-commit` instead.Ten of ten plausible descriptions tripped it, and #99 will drive ~26 authors through description rewrites wherepc-run,vale-runand theapm-*skills are about hyphenated tools.references/file — the unqualified form failed the hook, the absolute-path form is graded FAIL byfile-structure.md.Both true positives (
research→neuledge-context,gitea-issues→gitea-labels) are pinned by test so a future FP fix cannot delete them.State
diff -rcleanpre-commit run --all-filesis deliberately red on two hooks —skill-size-check(37 errors) andKyberforge.CompositionNote(10 errors across fourgitea-*skills). That is the intended ship-hot state, tracked in #99.Deferred, with records
forge,apm-workflow,apm-install) — including thatapm-workflowis cited as the dispatch exemplar while failing the gate.skill-audit+agent-audit; this PR deepens the split, which is now stated in the ADR rather than left implicit.— see /etc/hosts instead) extracts/etc. Pre-existing, narrow.Release tag
check-release-neededfails any push tomainuntil a tag covers.pre-commit-hooks.yaml's paths — 21 changed sincev1.0.0. A v2.0.0 is prepared but deliberately not pushed: it would point at a branch commit rather than a merge commit, unlikev1.0.0. It should be cut against the merge commit after this lands. The bump is major because all three exported hooks can now fail a file that passed atv1.0.0, and PyYAML became a hard dependency.Review: request changes
This is strong work — the ADR is the most rigorously argued in the repo, the numbers in it reproduce exactly, and the gates it specifies are mostly implemented as specified. The problems below are concentrated in one place: the resolver and frontmatter parser in
scripts/skill-size-check.shhave three paths that exit 0 without measuring, and one that produces hard false failures in exactly the consumer case ADR-0020 §Decision was written to protect. For a gate shipping hot with no baseline, those are the defects that matter most, so they should land before merge rather than as follow-ups.Everything reproduced below was confirmed by execution, not read off the source.
Blocker
B1 — In a consumer repo the deployed
.claude/.agentstrees are never consulted, because the.gitfallback always wins.scripts/skill-size-check.sh:414-419._authoring_root()falls back to the nearest.gitancestor, so it returns truthy in any git repo._collect_authoring_root()then contributes zero names (noplugins/*/.apm/), and theelse: _deployed_roots(start)branch is unreachable. The consumer code path is dead in every git-tracked repo — i.e. in the only case it exists for.Reproduced:
Deleting
.gitfixes the failure, which is backwards. This contradicts ADR-0020:110-111 ("Deployed trees are consulted only when no authoring root exists — the consumer case") and defeats the guarantee at :118-127. A consumer installing holocron through pre-commit gets an unblockable hard FAIL on any skill with a boundary clause pointing at an installed sibling. Suggested fix: fall through to_deployed_rootswhen the authoring root contributes no names, or union it in when the root holds noplugins/*.Major — vacuous-green paths
B2 — An indented
---inside the frontmatter silently truncates it.:694.FRONTMATTER_REcloses on\r?\n[ \t]*---[ \t]*, so a---at block-scalar indentation ends the frontmatter early and the rest of the description is reclassified as body, with no warning. Reproduced: a folded description containing an indented---, a ~700-character total, and a dangling/nonexistent-target— output is one spurious "no boundary clause" SUGGESTION and rc=0. Both a description FAIL and an ERROR-tier dangling target pass silently. Anchoring the closing marker at column 0 is safe, since block-scalar content must be indented.B3 — A non-string
descriptionisstr()-coerced and measured as a Python repr.:733, shared verbatim by all three validators. Reproduced, all rc=0:description:+- alpha/- beta"['alpha', 'beta']"(17 chars)description: true"True"(4 chars)description:+a: 1"{'a': 1}"(8 chars)A mis-indented folded scalar collapsing into a block sequence is one of the likeliest YAML slips in the exact field this ADR exists for. The inconsistency is sharp: valueless,
null,'',""and an empty>are all hard FAILs with tests pinning them (tests/test-adr0020-frontmatter.sh:253-266) — a list, mapping or bool is not. Should raiseFrontmatterError.B4 — An unreadable file kills the batch; the bash half records a silent pass.
:208-217catches onlyUnicodeDecodeError, soPermissionErrorpropagates and aborts the run mid-loop — every file after it goes unmeasured. Independently,:114discards awk's exit status, so on an awk read failurelines/wordscome back empty, bash arithmetic reads them as 0, and both spec ceilings pass in total silence. Confirmed under a non-root uid.B5 — An unterminated code fence disables the ERROR-tier
references/*.mdexistence check for the rest of the body.:785-804treats an unclosed fence as running to EOF and blanks everything after it. Confirmed: a body with an unclosed fence followed by a pointer to a nonexistent reference → rc=0, no error; close the fence and the ERROR fires. Same suppression hitsgotcha_stats.Major — cross-script divergence
B6 — The whole-file line/word ceilings are measured by two implementations that disagree.
:114uses awkNR/NF;skill-audit/scripts/validate.sh:901,909uses Pythonsplitlines()/split(). Python splits on\x0b \x0c \x1c \x85and all Unicode spaces; awk does not. Confirmed: a body padded with U+2028 → hook reports 6 lines, rc=0;validate.shreportsFAIL … 606 lines, rc=1. With U+00A0: awk 814 words (silent pass) vs Python 3013 (FAIL … exceeds 2770). This is exactly the "author fixes one gate and is blocked by the other" bug, on the two axes nothing tests —tests/test-adr0020-differential.sh:290deliberately excludesMAX_LINES/MAX_WORDSfrom the cross-script comparison. Latent today (no corpus file has non-ASCII whitespace), but the header comment at:111-113asserts the equivalence that does not hold. Moving the counts into the Python block that already reads the file fixes B4 and B6 together.Major — content lost or mis-gated in the retrofit
agent-audit/references/description-quality.md:113-116states "No script checks this for an agent file —validate.shresolves boundary targets for skills only". False —check_boundary()is called at both scopes (validate.sh:1124,:1213) and fires on real agent files. The auditor will hand-resolve what the script already resolved, and can contradict it.skill-audit/SKILL.md:38routes toreferences/validation-scripts.mdwhen the script "fails, cannot run, or reports something needing interpretation".validate.shexits 1 on ordinary content FAILs — the normal outcome for the entire #99 population — so 1,302 words of troubleshooting prose load on nearly every real audit.main:SKILL.md:40scoped this to "cannot execute". Narrow it to "exits non-zero for a reason other than findings".toolsguidance (main:agent-author/SKILL.md:109— the restrict half); the improve-flow regression check (main:skill-author/SKILL.md:300, "No previously-passing audit checks were broken"); and the agent-body sizing heuristic (main:agent-author/SKILL.md:247). The last matters because 3 of 4 agents already sit at 933/1,080/1,199 body words and ADR-0020 removed the word gate — that heuristic was the only remaining brake, and the delegation check only catches procedures an invocable skill owns.agent-author/SKILL.md:44("read only the file for the resolved scope") now hides two scope-independent rules behind project/user scope: themcp__<server>__*glob syntax fordisallowedTools(only atproject-user-scope.md:36, yetdisallowedToolsis the only permitted fence at plugin/APM scope), and the five tools no subagent ever receives (AskUserQuestion,EnterPlanMode,ExitPlanMode,ScheduleWakeup,WaitForMcpServers).Minor
:578-587— any/wordafter a route verb is an unconditional hard FAIL with no suppression, so a description legitimately naming/compact,/clearor/initcannot be committed.:255,296-297,385— glob metacharacters in the checkout path ([,],*,?) silently disable the whole resolver; it degrades to the "DID NOT RUN" INFO with rc=0.:532— corroboration leaks across sentence boundaries when a sentence starts lowercase or with a backtick, promoting an unrelated unresolvable name from SUGGESTION to blocking ERROR.re.Iis applied inconsistently across the extraction patterns (:521-527), so`Skill-Audit`in a boundary sentence is never extracted. Recall gap only.agent-audit/scripts/validate.sh:946— narrowingextract_fieldto[^\S\r\n]*regressedtools:as a YAML block sequence: main emits the subagent-unavailable SUGGESTION, HEAD emits nothing. Block sequence is the shape Copilot files use, and no test covers it.agent-audit/scripts/validate.sh:1062,1166— a nonexistent agent file exits 1 with a bareFileNotFoundErrortraceback and noFAILline. This is the pathcheck-apm-agents-valid.shtakes for a file deleted from the worktree but still tracked.skill-audithandles it cleanly at:65-67.tests/test-skill-size-check.sh:539-566— theskill-improveprobe ships already-stale (fixed by this very change), so that iteration permanently takes apass "SKIP: …"that asserts nothing while counting toward the pass total. It also contradictstest-adr0020-targets.sh:247, which correctly pins the live set at{gitea-labels, neuledge-context}.tests/test-adr0020-frontmatter.sh:83-84,239-240— theyaml-nonefixture (---\n---\n) failsFRONTMATTER_REoutright and never reaches thedata is Nonebranch it is labelled for; it passes because both messages contain the wordfrontmatter.AGENTS.md(14-hook paragraph) andscripts/check-executables-allow-sync.shassert a version-exactexecutables.allowmatch that apm 0.28.0 does not implement —_map_grantsstrips the version and compares the bare name, so anykyberforge#*entry grants. Verified by setting the key to a nonexistentkyberforge#9.9.9and watching theSessionStarthook still deploy. Pre-existing and not in this diff, but the PR bumps the key on that premise. The bump is correct and required by the local gate; only the stated rationale is stale..pre-commit-hooks.yaml— the external ADR-0014 contract — changed, butcheck-release-needed.sh:20returns 0 unlessPRE_COMMIT_REMOTE_BRANCH == refs/heads/main, which a Gitea merge-button merge never sets. Consumers pinningrev: v1.0.0get none of ADR-0020. Fine if tagging is a deliberate post-merge step; flagging it because the gate that would remind you is inert on this path.Untested (working today, verified by hand)
The two-pass walk-up itself has no fixture anywhere placing a
.gitinside a plugin — the specific case ADR-0020:104-107 says the two passes exist for. Also uncovered: the.gitfallback root, all ~55 lines of_declared_dependency_dirs,normalize_targetnamespace stripping,read_text/EncodingError,REFERENCE_QUALIFIER, and the non-file path branches (missing path, directory namedSKILL.md, broken symlink) in both the bash preamble and the Python loop — the case the script asserts most loudly about itself, in duplicate.Verification
bash tests/run-tests.sh(plain,--strict,RUN_TESTS_STRICT=1)skill-size-check --all-filesKyberforge.CompositionNotegitea-*skillsDescriptionOpener(^This\b)executables.allowbumped in lockstep.claude/settings.jsonapm installin a scratch copy regenerated it byte-for-byte; the new 6thpretty-format-jsonexclude is load-bearing (sorted output differs from committed bytes)diff -rclean excepttests/, whichapm packexcludes by designrun-tests.sh:54); mutation-tested — reverting it turnstest-run-tests.shredf9b919dThe
RUN_TESTS_STRICTandpretty-format-jsonfixes are both real and both correctly diagnosed.One design note, not blocking
The ADR rejects a shrinking baseline file "in favour of hot gates" in a single clause — the least-argued decision in an otherwise exhaustively-argued document. A baseline would have delivered identical convergence pressure (fail on growth) without making two-thirds of the corpus un-editable, and the ADR itself names the resulting risk at :287-289: a gate expensive enough to be inconvenient gets bypassed with
SKIP=and loses its authority. With 26 descriptions, 9 bodies and 10 Vale errors outstanding, the first person who needs a one-linegitea-prsfix meets a two-gate retrofit. Worth a sentence of justification in the ADR even if the decision stands.Related: this bundles the contract, the four-skill retrofit, and two unrelated pre-existing CI fixes into 126 files. The CI fixes were blocking, so bundling is defensible — but
7607522andd02765dwere independently mergeable and would have unblocked everyone else's pushes days earlier.@@ -50,0 +110,4 @@- **Vague capabilities** ("helps with agents" where "audits an agent definition pair" wasavailable). `Kyberforge.VagueWording` catches the known filler; imprecision outside that list isjudgment.- **A boundary clause naming a target that does not resolve** to a real skill directory or agentThis is factually wrong about the validator.
check_boundary()is called at both scopes —agent-audit/scripts/validate.sh:1124(plugin/APM) and:1213(project/user) — and I confirmed it fires on a real agent file (apm-orchestrate.agent.mdemits a boundary SUGGESTION).Consequence: the auditor hand-resolves what the script already resolved, and a hand-derived verdict can contradict the script's on the same file.
@@ -41,2 +36,3 @@`validate.sh` findings become the `### Structure` dimension — its FAILs and its SUGGESTIONs both.Note any Provenance FAILs and INFO findings from `validate-provenance.sh` — they surface in the report as a `### Provenance` dimension (separate from `### Structure`). The script embeds full FAIL/INFO format with Why and Fix per finding; surface them verbatim.If any of the three fails, cannot run, or reports something needing interpretation, read `references/validation-scripts.md` — it carries the manual fallback and the misleading exit codes.This trigger is too broad.
validate.shexits 1 on ordinary content FAILs — the normal outcome of auditing any non-compliant skill, i.e. the entire #99 population — so "if any of the three fails" loads 1,302 words of script-troubleshooting prose on nearly every real audit. That is a context-budget regression inside the skill that enforces the context budget, and the reference file itself opens "Nothing here is needed on a clean run".main:SKILL.md:40scoped this precisely: "cannot execute (python3 unavailable, Bash denied, or permission error)". Suggest narrowing to "cannot run, or exits non-zero for a reason other than findings".Two problems on this line.
1 — the exit status is discarded. On an awk read failure
lines/wordscome back empty, bash arithmetic treats both as 0, and the 500-line and 2,770-word ceilings both record a silent pass — in a script whose stated rule (:92,:855) is that a measurement not taken must never be quiet.2 — this disagrees with
skill-audit/scripts/validate.sh:901,909, which measures the same two ceilings with Pythonsplitlines()/split(). Python splits on\x0b \x0c \x1c \x85and every Unicode space; awk splits on neither. Confirmed: a body padded with U+2028 → hook reports 6 lines, rc=0, whilevalidate.shreportsFAIL … 606 lines, rc=1. Padded with U+00A0 → awk 814 words (silent pass) vs Python 3013 (FAIL … exceeds 2770).That is the "fix one gate, get blocked by the other" bug, on the two axes nothing tests —
tests/test-adr0020-differential.sh:290deliberately excludesMAX_LINES/MAX_WORDSfrom the cross-script comparison. The header comment at:111-113asserts the equivalence that does not hold (wc -wmatches awk only underLC_ALL=C).Moving both counts into the Python block that already reads the file fixes this and the
PermissionErrorabort at:208-217together.@@ -71,0 +411,4 @@for dep_dir in _declared_dependency_dirs(package):_collect_package(dep_dir, names)root = _authoring_root(start)Blocker.
_authoring_root()falls back to the nearest.gitancestor, so it returns truthy in any git repo._collect_authoring_root()then contributes zero names (noplugins/*/.apm/), and thiselsebranch never runs —_deployed_rootsis dead code in every git-tracked repo, which is the only case it was written for.Reproduced in a bare consumer repo with
.claude/skills/alpha/SKILL.mdrouting to/betaagentand.agents/agents/betaagent.agent.mdpresent:Deleting
.gitfixes the failure. This contradicts ADR-0020:110-111 and defeats the install-independence guarantee at :118-127 — a consumer running this hook through pre-commit gets an unblockable hard FAIL on any boundary clause naming an installed sibling.Fix: fall through to
_deployed_rootswhen the authoring root contributes no names, or union it in when the root holds noplugins/*.@@ -71,0 +691,4 @@# description with a 1,000-word body exited 0 behind a BOM). A file that cannot# be measured must never report green, so every caller of these two ERRORs on a# miss instead of moving on.FRONTMATTER_RE = re.compile(Vacuous green. The closing marker
\r?\n[ \t]*---[ \t]*matches at any indentation, so a---line inside a block scalar terminates the frontmatter early and the remainder of the description is silently reclassified as body.Reproduced — a folded
description:containing an indented---, ~700 chars total, ending inuse /nonexistent-target instead:A description FAIL and an ERROR-tier dangling target both pass, with the only output being a spurious suggestion that the boundary clause is missing. This is the #1 failure mode the ADR is written against.
Anchoring the close at column 0 (
\r?\n---[ \t]*) is safe — block-scalar content must be indented.@@ -71,0 +730,4 @@value = data.get('description')if value is None:return ''if not isinstance(value, str):Vacuous green, and shared verbatim by all three validators. A non-string YAML value is
str()-coerced and then measured as a Python repr. All rc=0:description:+- alpha/- beta"['alpha', 'beta']"(17 chars)description: true"True"(4 chars)description:+a: 1"{'a': 1}"(8 chars)A mis-indented folded scalar collapsing into a block sequence is one of the two likeliest YAML slips in precisely the field this ADR exists for — and the boundary/routing extraction then runs over the repr.
The inconsistency is sharp: valueless,
null,'',""and an empty>are all hard FAILs, withtests/test-adr0020-frontmatter.sh:253-266pinning all five. A list, mapping or bool is not. Should raiseFrontmatterError.Submitted as a comment review — Gitea rejects REQUEST_CHANGES from the PR author's own account. Treat it as request-changes: B1 is a confirmed blocker and B2/B3/B5 are confirmed vacuous-green paths.
@@ -50,0 +110,4 @@- **Vague capabilities** ("helps with agents" where "audits an agent definition pair" wasavailable). `Kyberforge.VagueWording` catches the known filler; imprecision outside that list isjudgment.- **A boundary clause naming a target that does not resolve** to a real skill directory or agentThis is factually wrong about the validator.
check_boundary()is called at both scopes —agent-audit/scripts/validate.sh:1124(plugin/APM) and:1213(project/user) — and I confirmed it fires on a real agent file (apm-orchestrate.agent.mdemits a boundary SUGGESTION).Consequence: the auditor hand-resolves what the script already resolved, and a hand-derived verdict can contradict the script's on the same file.
@@ -41,2 +36,3 @@`validate.sh` findings become the `### Structure` dimension — its FAILs and its SUGGESTIONs both.Note any Provenance FAILs and INFO findings from `validate-provenance.sh` — they surface in the report as a `### Provenance` dimension (separate from `### Structure`). The script embeds full FAIL/INFO format with Why and Fix per finding; surface them verbatim.If any of the three fails, cannot run, or reports something needing interpretation, read `references/validation-scripts.md` — it carries the manual fallback and the misleading exit codes.This trigger is too broad.
validate.shexits 1 on ordinary content FAILs — the normal outcome of auditing any non-compliant skill, i.e. the entire #99 population — so "if any of the three fails" loads 1,302 words of script-troubleshooting prose on nearly every real audit. That is a context-budget regression inside the skill that enforces the context budget, and the reference file itself opens "Nothing here is needed on a clean run".main:SKILL.md:40scoped this precisely: "cannot execute (python3 unavailable, Bash denied, or permission error)". Suggest narrowing to "cannot run, or exits non-zero for a reason other than findings".Two problems on this line.
1 — the exit status is discarded. On an awk read failure
lines/wordscome back empty, bash arithmetic treats both as 0, and the 500-line and 2,770-word ceilings both record a silent pass — in a script whose stated rule (:92,:855) is that a measurement not taken must never be quiet.2 — this disagrees with
skill-audit/scripts/validate.sh:901,909, which measures the same two ceilings with Pythonsplitlines()/split(). Python splits on\x0b \x0c \x1c \x85and every Unicode space; awk splits on neither. Confirmed: a body padded with U+2028 → hook reports 6 lines, rc=0, whilevalidate.shreportsFAIL … 606 lines, rc=1. Padded with U+00A0 → awk 814 words (silent pass) vs Python 3013 (FAIL … exceeds 2770).That is the "fix one gate, get blocked by the other" bug, on the two axes nothing tests —
tests/test-adr0020-differential.sh:290deliberately excludesMAX_LINES/MAX_WORDSfrom the cross-script comparison. The header comment at:111-113asserts the equivalence that does not hold (wc -wmatches awk only underLC_ALL=C).Moving both counts into the Python block that already reads the file fixes this and the
PermissionErrorabort at:208-217together.@@ -71,0 +411,4 @@for dep_dir in _declared_dependency_dirs(package):_collect_package(dep_dir, names)root = _authoring_root(start)Blocker.
_authoring_root()falls back to the nearest.gitancestor, so it returns truthy in any git repo._collect_authoring_root()then contributes zero names (noplugins/*/.apm/), and thiselsebranch never runs —_deployed_rootsis dead code in every git-tracked repo, which is the only case it was written for.Reproduced in a bare consumer repo with
.claude/skills/alpha/SKILL.mdrouting to/betaagentand.agents/agents/betaagent.agent.mdpresent:Deleting
.gitfixes the failure. This contradicts ADR-0020:110-111 and defeats the install-independence guarantee at :118-127 — a consumer running this hook through pre-commit gets an unblockable hard FAIL on any boundary clause naming an installed sibling.Fix: fall through to
_deployed_rootswhen the authoring root contributes no names, or union it in when the root holds noplugins/*.@@ -71,0 +691,4 @@# description with a 1,000-word body exited 0 behind a BOM). A file that cannot# be measured must never report green, so every caller of these two ERRORs on a# miss instead of moving on.FRONTMATTER_RE = re.compile(Vacuous green. The closing marker
\r?\n[ \t]*---[ \t]*matches at any indentation, so a---line inside a block scalar terminates the frontmatter early and the remainder of the description is silently reclassified as body.Reproduced — a folded
description:containing an indented---, ~700 chars total, ending inuse /nonexistent-target instead:A description FAIL and an ERROR-tier dangling target both pass, with the only output being a spurious suggestion that the boundary clause is missing. This is the #1 failure mode the ADR is written against.
Anchoring the close at column 0 (
\r?\n---[ \t]*) is safe — block-scalar content must be indented.@@ -71,0 +730,4 @@value = data.get('description')if value is None:return ''if not isinstance(value, str):Vacuous green, and shared verbatim by all three validators. A non-string YAML value is
str()-coerced and then measured as a Python repr. All rc=0:description:+- alpha/- beta"['alpha', 'beta']"(17 chars)description: true"True"(4 chars)description:+a: 1"{'a': 1}"(8 chars)A mis-indented folded scalar collapsing into a block sequence is one of the two likeliest YAML slips in precisely the field this ADR exists for — and the boundary/routing extraction then runs over the repr.
The inconsistency is sharp: valueless,
null,'',""and an empty>are all hard FAILs, withtests/test-adr0020-frontmatter.sh:253-266pinning all five. A list, mapping or bool is not. Should raiseFrontmatterError.Review findings addressed — pushed as
79c9089Four commits on top of
e7ebc66. Every blocker and major from the review above is closed; the two items I left open are design decisions, called out at the bottom.f7cc279fix(kyberforge)— close the vacuous-green and consumer-resolution defectsb0d6d08test— pin the nine ADR-0020 gate defects that shipped untestedede3f06fix(kyberforge)— restore the authoring rules the ADR-0020 trim dropped79c9089docs— make the resolution contract match what the gate actually doesThe blocker, and a second one found while fixing it
B1 is fixed, but the first fix was wrong and the review wave caught it — worth recording because the wrong version is the obvious one.
_authoring_root()fell back to the nearest.git, so it returned truthy in any git repo and the deployed-tree branch was dead code. The intuitive repair is "fall through when the authoring root contributed no new names". That is wrong: a single-plugin monorepo re-collects its own package, adds no new name, so the delta reads zero and_deployed_roots()runs — pulling in every.claude//.agents/tree up to ten levels, including the user's global~/.claude/skills. Same commit, two verdicts, decided by whetherapm installhad been run. That is ADR-0020:118-127's failure one layer down.It now keys on which of the two walk-up passes matched, which is what the comment always claimed. Verified by instrumenting
_deployed_roots(): zero calls across the corpus on the real working tree, and a bareplugins/+apm.ymltree produces findings identical to it with zeroINFOlines.Vacuous-green paths closed
B2, B3 and B5 all exited 0 while measuring nothing:
---inside a block scalar truncated the frontmatter — a 815-char description and a dangling/nonexistent-targetboth passed, the only output a spurious "no boundary clause" SUGGESTION;descriptionwasstr()-coerced, sodescription: truemeasured as the 4-char"True"— sharpened by the fact that valueless/null/''/""/empty->were already hard FAILs with tests pinning them;references/check and the gotcha counts.B4 and B6 are fixed together: both whole-file counts now run in the Python block that already reads the file. This removes the discarded awk exit status and the awk-vs-
splitlines()disagreement on Unicode whitespace._non_adr_hook_error()is deleted from the differential test — it excludedMAX_LINES/MAX_WORDSfrom the cross-script comparison on an untested assumption, which is exactly why the divergence stayed invisible.Also closed: the
glob.escapegap, there.Iinconsistency,tools:as a YAML block sequence, and theFileNotFoundErrortraceback. A type error also no longer reports itself asfrontmatter is not valid YAML.Authoring rules restored
Three rules existed on
mainand existed nowhere after the trim: least-privilegetools(the restrict half), the improve-flow regression check, and the agent-body "would the agent get this wrong without it?" heuristic. The last is the one I'd have pushed back hardest on — ADR-0020 deliberately sets no body word gate for agents, three of four already sit at 933/1,080/1,199 words, and the delegation check only fires on procedure a skill already owns. That heuristic was the only brake left. It turned outagent-authorhad also lost its own regression check, so that one is restored on both halves of the pair.Two rules were reachable only from the wrong scope (the
mcp__glob syntax fordisallowedTools, and the five tools no subagent receives) —disallowedToolsis the only permitted fence at plugin/APM scope, so the scope needing the syntax most couldn't reach it.Two documents were actively wrong:
agent-audittold auditorsvalidate.shresolves boundary targets for skills only (it runs at both scopes), andskill-auditloaded 1,302 words of script-troubleshooting prose whenevervalidate.sh"fails" — which is every ordinary content FAIL, i.e. the whole #99 population.Tests
Nine regression tests, one per defect, each proven non-vacuous by mutating a scratch copy and watching it go red — done independently twice, by the author and again by a reviewer who re-ran every mutation itself rather than trusting the report.
Two gaps worth naming: no fixture anywhere placed a
.gitinside a plugin, the exact case ADR-0020:104-107 says the two-pass walk-up exists for; and only the no-.gitconsumer case had ever been tested, which is precisely why B1 was invisible. Both now have fixtures.Two existing assertions were repairs. The
skill-improveprobe had been fixed by this branch, so its iteration permanently took an assertion-freeSKIPthat still counted as a pass. And theyaml-nonefixture emitted---/---, which never matched the frontmatter pattern at all — it passed on the bare word "frontmatter", present in both messages, while never reaching the branch it was named for.Verification
bash tests/run-tests.sh --strict→ 24 passed, 0 skipped, 0 failedbash tests/run-bats.sh→ 208 tests, 0 failures (was 202)CompositionNoteerrors, zero new findings on touched filesAGENTS.mdand ADR-0020 both said deployed trees are consulted "only when no authoring root exists". That stopped being true, so both now describe the shipped rule and record why the name-count delta is not equivalent — the prose here is load-bearing and the next agent reads it instead of the code.Still open — your call, not mine
/compactfalse positive. Any/wordafter a route verb is an unconditional hard FAIL, so a description legitimately naming a host built-in (/clear,/compact,/init) cannot be committed. Fixing it needs either a known-builtins allowlist or a suppression mechanism, and the gate has neither by design.:532). Changes how targets get promoted from SUGGESTION to blocking ERROR, so it can move the documented corpus counts.Also unchanged, per your call: no release tag.
check-release-needed.sh:20returns 0 unlessPRE_COMMIT_REMOTE_BRANCH == refs/heads/main, which a merge-button merge never sets — so the gate will not remind you, and consumers pinningrev: v1.0.0get none of ADR-0020 until it is tagged manually.The design note from the review still stands and is not addressed here: the ADR rejects a shrinking baseline in a single clause, and it is the least-argued decision in the document.
Both open design decisions closed — pushed as
54d7bd8The two items left open at the bottom of the previous comment are resolved. Two commits on top of
79c9089.75a13c8fix(kyberforge)— scope corroboration to a real sentence boundary54d7bd8docs— rule host built-ins out of the routing target universe1. The
/compactfalse positive is not a false positiveRecorded as needing an allowlist or a suppression mechanism. It needs neither. The routing universe is the apm marketplace: a target resolves to a skill or an agent, or it does not resolve.
/compact,/clearand/initare Claude Code slash commands with no counterpart in Copilot CLI or Codex, and.apm/source compiles for all three — so a vendor-neutral description routing to one is a portability defect and the hard FAIL is correct.The allowlist was rejected on a concrete failure mode rather than taste: it answers a different question ("does this exist on some host?"), it cannot answer that portably from a single source file, and it goes stale the next time a host ships a command — reintroducing exactly the same-commit-two-verdicts failure ADR-0020:118-127 already closed for deployed trees.
Nothing is blocked today. Zero of the 43 descriptions name a host built-in; the four
/-targets in the corpus are/caveman,/gitea,/gitea-workflowand/skill-author. An author who genuinely needs to mention one writes it un-slashed —the `compact` built-in— which is not route notation and carries no routing claim.Recorded in ADR-0020 and in both author-facing
references/contract.mdfiles, since the ADR is not what an author reads mid-write.2. The deferral reason for the corroboration leak was false
It was deferred because it "can move the documented corpus counts". Measured before touching anything, in both directions:
Findings byte-identical, not merely equinumerous. Worth flagging that the first version of that measurement was itself wrong — the lookbehind was off by the final period (
(?<!\be\.g)never fires at the position aftere.g., which is.g.), so it measured a no-op and would have "confirmed" zero delta for a patch that did nothing. Re-measured with(?<!\be\.g\.).What the leak actually was
Blocking is scoped to a sentence — a prose-form target earns a hard ERROR only when its own sentence names another target that resolves. That makes
SENTENCE_SPLITpart of the contract, not a detail of it, and "period, space, capital" was wrong in both directions:e.g. "…"ends no sentence, but the quote looks like a start. The clause was cut in half, the corroborator stranded on the far side, and a genuinely dangling target silently demoted to SUGGESTION — a measurement taken and then discarded, which is the same vacuous-green family as B2/B3/B5. Seven such splits are live in the current corpus.The splitter now excludes the five abbreviations that occur in routing prose and admits a backtick or lowercase letter as an opener. Applied byte-identically to all three copies of the shared resolver;
check-plugin-content-syncand the contract test both green.Landing it here rather than after #99 is deliberate. The exposure is entirely to descriptions that don't exist yet — and #99 is about to write 26 of them.
Tests
Three regression cases in
tests/test-adr0020-targets.sh, one per direction plus the backtick opener. Each proven non-vacuous by reverting only the splitter and confirming it goes red with the right symptom: the abbreviation case downgrades ERROR → SUGGESTION, and both opener cases upgrade SUGGESTION → ERROR. The reverted run is 36 passed / 3 failed; the fixed run is 39 / 0.Verification
bash tests/run-tests.sh --strict→ 24 passed, 0 skipped, 0 faileddiffcleanSKILL.mdor.agent.mdwas touchedStill outstanding — unchanged from before
No release tag.
check-release-needed.sh:20returns 0 unlessPRE_COMMIT_REMOTE_BRANCH == refs/heads/main, which a merge-button merge never sets, so the gate will not remind you. v2.0.0 must be cut manually against the merge commit or consumers pinningrev: v1.0.0get none of ADR-0020.The design note from the original review also still stands and is not addressed here: the ADR rejects a shrinking baseline in a single clause, and that remains the least-argued decision in the document.