feat(lint): wire Vale as deterministic prefilter for skill-audit/agent-audit #85
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Defame1297/holocron#85
Reference in New Issue
Block a user
Delete Branch "feat/84-vale-audit-prefilter"
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?
Summary
Implements issue #84: moves pattern-matchable qualitative checks in
skill-audit/agent-audit(description opener phrasing, vague-capability wording, padding phrases, Copilot's "Use proactively" check) from LLM re-derivation to a deterministic Vale pass, per the repo's "prefer deterministic code for repeatable tasks" governance principle..vale.iniplus aKyberforgestyle and aKyberforgeCopilotstyle scoped to.agent.mdfiles. Every rule islevel: error— Vale's exit code keys onerroralerts alone, so any other level prints an alert and still exits 0, which makes it invisible behind a passing pre-commit hook. No ignorable tier, matching shellcheck and the test suite.skill-audit/agent-auditStep 1 alongside the existingvalidate.shstructural checks. Every Vale alert is a FAIL. A zero-file Vale run is treated as NOT RUN, not as clean..vale.ini's section globs are path-agnostic ([**/SKILL.md],[**/agents/*.md],[**/*.agent.md]) and deliberately do no scoping — Vale's*crosses/, soplugins/*/-prefixed globs never scoped anything either. Scoping is the pre-commit hook'sfiles:regex.lintplugin (lint-runneragent,vale-config/vale-runskills, bundled Vale research docs) and register it in both marketplace manifestspre-commithook, plus a siblingskill-size-checkhook enforcing the 500-lineSKILL.mdceilingscripts/vale-wrap.shworks around a confirmed Vale 3.15.2 bug:text.frontmatter.descriptionsilently stops matching once the description is a multi-line YAML block scalar, which is how most skills here are writtenkyberforgeplugin itself (round 4), so the prefilter also works for repos that installkyberforge@holocronas an external plugin, not just this repoCloses #84
⚠️ This branch was rebased — old commit links are dead
Rebased onto
mainafter #86 landed, so SHAs cited in earlier review comments no longer exist on the branch. Subjects are unchanged, so the mapping is 1:1 by commit message:0bbb965bbb0dcd4d6f31357bdfa955dc065792d3e10c0d51f59ad2a36c0afb7210b192cb2257d8b007282eb13f73324a73e4abe23d1afdbe28058d3544392b88888c45e226727118ace8d5629075f65d8cbc33d9e1a5403f326df4Review history
Seven rounds, all resolved:
vale-rundocumented Vale's exit-code model backwards; a zero-file run read as cleanrealpath -m, directory args skipping flatteningdeclare -Aregression introduced by round 6's own caching refactor (the file wasn't in round 6's bash-3.2 scan list), a version-bump gap on thegiteaplugin (the same bug class was caught 4× elsewhere in this PR but missed here), and a silent-success gap whencheck-vale-style-sync.shis given a bad path argumentStyles portability (moving
styles//.vale.iniout of the repo root so the prefilter travels to external installs) — resolved in round 4 (#1339, commit1164f3a) and formalized in ADR-0014, which supersedes ADR-0013's deferred-portability consequence.Test plan
bash tests/run-tests.sh— 12 scripts, 0 failed (vale-wrap.shsub-suite: 39 cases;check-vale-style-sync.shsub-suite: 21 cases)pre-commit run --all-filesforvale-audit-prefilter,skill-size-check,shellcheck— all passscripts/check-manifests.shandclaude plugin validate --strict— clean on every plugin🤖 Generated with Claude Code
https://claude.ai/code/session_01FxG5T8EJDgkABXxuneuFfn
Review (read-only — no edits made)
Verified locally by running
valedirectly (v3.15.2, installed at/root/.local/bin/vale) against the actual repo tree on this branch, rather than just reading the diff. Full test suite (bash tests/run-tests.sh) andscripts/check-manifests.shboth pass as claimed.🔴 Blocking: multi-line folded descriptions silently defeat
text.frontmatter.descriptionscopeKyberforge.DescriptionOpenerandKyberforge.VagueWordingare scoped totext.frontmatter.description. Confirmed working whendescription:is a single physical line. But the moment the folded scalar (description: >) spans two or more physical lines — which is the style used by the large majority of skills in this repo, includingskill-audit/agent-auditthemselves — the scope silently stops matching. Reproduced directly:That's an obvious
DescriptionOpener+ twoVagueWordinghit, and Vale reports clean. A single-line version of the identical text triggers correctly. This means a full sweep of the repo's own 49 skill/agent files currently returns 0/0/0 — not because the repo is clean, but because the check that's supposed to cover most of the Description dimension never fires for anything using the folded style. Sinceskill-audit/agent-audittreat a clean Vale run as "no additional findings" for that dimension, this is a silent false negative, not a no-op — worse than not having the check at all, since it now reads as evidence of cleanliness.Worth a real regression test asserting Vale fires against a fixture with a known-bad multi-line description — nothing in the added test coverage would have caught this, which is presumably how it shipped.
🟠 Gap: pre-commit hook glob crosses directory boundaries the audit-invocation path deliberately avoids
CONTEXT.md/the SKILL.md edits explicitly acknowledge that
plugins/*/agents/*.md-style globs match nested paths likedocs/research/examples/**/agents/*.md, and mitigate it by always scopingskill-audit/agent-audit's Step 1valeinvocation to a specific target file rather than a sweep. Confirmed that mitigation holds for the audit-skill path.It does not hold for the new
.pre-commit-config.yamlhook (vale-audit-prefilter), whosefilesregex (^plugins/.*/(skills/.*/SKILL\.md|agents/.*\.md)$) has the same cross-boundary behavior and is not manually scoped — pre-commit runs it automatically against whatever staged files match. Confirmed this matches (and would lint) e.g.plugins/kyberforge/docs/research/examples/skill-write/skill-creator/agents/*.md(upstream reference material, explicitly documented as "not shipped with the plugin") andplugins/kyberforge/skills/skill-author/assets/templates/SKILL.md(a placeholder template). Also confirmed Vale exits 1 on anyerror-level alert, whichlanguage: unsupportedpre-commit hooks treat as hook failure.Net effect: a future commit touching either of those paths — upstream example content or the FILL-IN template — that happens to contain house-style-violating phrasing (plausible for both, since neither is written to Kyberforge's own description conventions) will get blocked by a hook whose stated purpose is auditing Kyberforge-authored skills/agents, not arbitrary reference docs. Suggest tightening the
filesregex (e.g. excludingdocs/research/andassets/) to match the audit path's scoping intent.🟡 Minor / worth confirming before merge
plugins/lint/agents/lint-runner.agent.mdusestools: ["execute", "read", "search"]. The two existing orchestrator agent pairs (git-orchestrate,gitea-orchestrate) only ever useexecute/read/edit—searchis a new tool identifier with no precedent elsewhere in the repo. Worth double-checking it's a real Copilot CLI tool value (mapped toGrep, Globon the CC side) rather than an invented one, since nothing infield-inventory.mdenumerates validtoolsvalues to cross-check against.Everything else checked out
.vale.iniscope syntax (scope: text.frontmatter.description) is a real Vale feature, confirmed against current docs — not a made-up config key.PaddingPhrase(scope:text, not frontmatter-scoped) fires correctly regardless of line-wrapping, confirmed.lintis structurally consistent with other plugins (version1.1.0matches the repo's convention for newly-added plugins; CC vs Copilot manifest split matches the established pattern).vale --config .vale.iniagainst the two editedskill-audit/agent-auditSKILL.md files themselves returns clean.Pushed
0bbb965addressing the correctness/scoping items from both reviews. Deferred the larger architecture questions (movingstyles/into the plugin, genericizing the plugin/agent naming, expanding coverage via research) to a follow-up — flagging here rather than rolling them into this PR silently.🔴 Blocking bug (my review) — fixed. Confirmed and reproduced:
text.frontmatter.descriptionsilently stops matching once the description spans 2+ physical lines (a YAML block scalar), which is how ~30 of the ~44 skill/agent files in this repo write it — meaning the repo-wide sweep was reading as clean while the check simply wasn't running. Addedscripts/vale-wrap.sh, which flattens the description to one physical line in a scratch copy (padding with blank lines so no other line number shifts) before handing off to realvale. Both audit skills' Step 1 and the pre-commit hook now call it instead ofvaledirectly.tests/test-vale-wrap.shregression-tests it — verified it fails without the fix and passes with it. Also added the gotcha tovale-config's SKILL.md so it's documented for any other project using this style, not just here.🟠 Pre-commit glob gap (my review) — fixed. Tightened
^plugins/.*/...to^plugins/[^/]+/(skills/[^/]+/SKILL\.md|agents/[^/]+\.md)$— single path segments instead of.*, so it can't cross intodocs/research/examples/**/agents/*.mdorassets/templates/SKILL.mdthe way the old glob did. Verified against the tracked tree: matches exactly the 45 real skill/agent files, excludes both problem paths.🟡
lint-runner'ssearchtool (my review) — no change, confirmed correct.searchisagent-author's own documented Copilot alias forGrep/Glob(seeassets/templates/copilot.agent.md).git-orchestrate/gitea-orchestratejust never needed Grep/Glob in CC, so they never had precedent for it — not a sign this was wrong.Manual review item 1 (why split the Copilot check) — by design, not a gap:
KyberforgeCopilot.ProactivePhraseflagsUse proactively, which is CC-specific phrasing that's meaningless in a Copilot description. There's nothing to flag in the CC file, so it isn't scoped there. Rewordedagent-audit's Step 1 prose to state this directly instead of leaving it implicit.Manual review item 2 (trim the Vale-styles prose) — done, in both audit skills' Step 1.
Manual review item 6 (cross-check vale-config/vale-run against actual wiring) — done.
vale-rundidn't need changes.vale-configwas missing the frontmatter multi-line gotcha entirely (see above) — added.Deferred, not in this push:
styles//.vale.iniintoplugins/lint/for portability to other repos) — real restructuring, needs its own pass.plugin.json/lint-runnernaming away from Vale-specific) — same.Full test suite (
bash tests/run-tests.sh) andscripts/check-manifests.shstill pass; ran the actualpre-commit run vale-audit-prefilteragainst real files to confirm the hook config change works end-to-end.Resolves the deferred "expand Vale coverage" item from this PR's review, per grill session + ADR-0013 (
docs/adr/0013-vale-harness-scope-and-rule-sources.md):write-good/alexagainst the real SKILL.md/agent-file corpus; cherry-picked 2 low-noise rules intostyles/Kyberforge/(VagueQualifier,SentenceOpenerThereIs) — 7 other candidate rules rejected as too noisy on this repo's terse, imperative-instruction style (e.g.write-good.Passive,alex.ProfanityUnlikelyflagging words like "hook"/"crash").skill-size-checkpre-commit hook enforcing agentskills.io's 500-line/5,000-tokenSKILL.mdceiling (previously unenforced).governance.md,docs/research/governance_principles/CONTROLS.md) were evaluated and explicitly excluded as rule sources — they're org/CI-infrastructure controls, not prose patterns Vale can express.File scope and enforcement model are unchanged (SKILL.md + agent files only, rules enforce immediately via the existing pre-commit hook, no new trial tier).
Review round 2 (read-only — no edits made)
Re-reviewed
0bbb965,28058d3,2eb13f7against the round-1 findings. Verified by execution against the real tree with vale 3.15.2, not by reading the diff: full suite is green (9 passed, 0 failed; bats 125/125),check-manifests.shexit 0,claude plugin validate --strictclean on all six plugins, branch 11 ahead / 0 behindmain(fast-forward).The test suite passing is not evidence the harness works. Three of the four items below are things the suite asserts and the tree does not do.
🔴 Blocking 1 — the round-1 blocking bug is still live for 58% of the corpus
scripts/vale-wrap.sh:68fixes the folded-scalar bug and then reintroduces it through the escaping.json.dumpsemits\"for embedded quotes,\\for backslashes,\uXXXXfor non-ASCII. Vale'stext.frontmatter.descriptionscope stops matching entirely once the double-quoted value contains any backslash escape. The file still parses as valid YAML — so the inline comment at:65-67("always a well-formed YAML value regardless of colons, quotes, or backslashes") is correct about validity and wrong about the outcome. Vale lints nothing and exits 0.Reproduced on a fixture whose description carries the same
"audit this skill"quoting style the real audit skills use:Delete the two
"characters and nothing else — same wrapper, same config:Blast radius, computed over the 45 tracked in-scope files:
That set includes
skill-audit,agent-audit,skill-author,agent-author,forge,marketplace-author,plugin-author, all 7 gitea skills, all 6 git skills, all 3 core skills, and both newlintskills. Round 1's finding was "a clean Vale run reads as evidence of cleanliness when the check never fired." That is still true, for most of the repo, with the fix in place.The reason
tests/test-vale-wrap.shpasses: its fixture puts a colon in the description but no double quote. A colon alone survivesjson.dumpsunescaped, so the test exercises the one punctuation class that happens to work.Fix direction: emit a single-quoted YAML scalar (
'…'with''doubling) rather thanjson.dumps, and add a fixture with",\, and a non-ASCII character.🔴 Blocking 2 — a linter false positive silently changed a shipped skill's behavior
plugins/bin/skills/caveman/SKILL.md:18is caveman mode's list of words it instructs the agent to delete.2eb13f7rewrote it:simplyandof coursewere flagged as uses when they were mentions inside a drop-list, and the fix removed them from the list. Caveman mode no longer strips "simply" or "of course". Nothing about that line was vague prose — the rule had nothing legitimate to catch, and the edit is a functional regression to a shipped skill made solely to silence it.This is the failure mode to design against, not a one-off: a prose linter over instruction files will keep hitting use/mention conflation, because these files quote the words they govern. Revert this line and guard it (
TokenIgnores, or<!-- vale off -->).Same root cause, meaning-changing, lower severity:
write-docs/SKILL.md:64—non-obvious invariants→hidden invariants. Different concept: "non-obvious" = present but easy to miss; "hidden" = not visible at all. The instruction now asks the writer to extract what by definition can't be read out of the code.non-obviousis house vocabulary here — it's incore/instructions/coding.md:4,ADR-FORMAT.md,commit-template.md, andcontent-guide.md. The rule flags the repo's own term of art, because-is a word boundary.tdd/SKILL.md:94—move complexity behind simple interfaces→narrow interfaces. That's Ousterhout's canonical phrasing, and "Deepen modules" on the same line is the deliberate callback. "Narrow" is a different property (few methods). The phrase no longer matches any sibling doc.gitea-workflow/SKILL.md:59— "requests that name a capability but not obviously which skill owns it" → "without a clear owning skill". Inverts the precondition: the table directly below assigns an owner to every capability, so a router reading the new wording concludes the index never applies.marketplace-author/SKILL.md:185—State clearly before proceeding:→State the following before proceeding:, on a destructive marketplace-removal confirmation gate. Lost the directive about how to state it.forge/SKILL.md:35— "if the intent genuinely spans several" → "spans multiple".severalis a standalone pronoun;multipleis not. Dangling determiner.Five more are pure synonym dodges where the rule caught nothing real (
usually→typically,tiny→minimal, etc.), and two leave a lintedSKILL.mdcontradicting its own unlintedreferences/file it delegates to (prototype/UI.md:3still says "several";gitea-issues/references/enrichments.md:50,53still says "clearly fits" while SKILL.md:75 was weakened to "well-matched").🔴 Blocking 3 — the Step 1 command block cannot execute from any working directory
skill-audit/SKILL.md:34-38andagent-audit/SKILL.md:36-40:Lines 1–2 resolve
scripts/to the skill's own bundled directory. Line 3 resolvesscripts/to the repo root.vale-wrap.shis not in either audit skill'sscripts/(confirmed: onlyvalidate.shandvalidate-provenance.share). There is no cwd where all three run:Introduced by
0bbb965— the pre-fix line wasvale --config .vale.ini …, which needed no path. The prose atskill-audit/SKILL.md:44("vale-wrap.shruns from the repo root") contradicts lines 35–36 in the same section.🔴 Blocking 4 —
VagueQualifierhas a ~100% false-positive rate on this repo's own voiceRan the two new rules over the 280 tracked
.mdfiles outside the linted globs — same authors, same register:Every context for the top eight tokens was read. Essentially none identify vague writing:
obvious— 24 unique contexts, all correct:non-obvious(7×),isn't obvious,stating the obvious,deviations from the obvious path.substantially— includes "substantially similar", the copyright legal term of art (ai-governance-research.md:435,455), plus quantitative research findings. Flagging a legal term of art as filler is a category error.clearly—core/AGENTS.md:8"When disagreeing, say so clearly";enrichments.md:50"If a milestone clearly fits". Both are precise confidence thresholds.usually—"usually main or develop","500 | usually transient". Correct frequency claims.ADR-0013 claims these rules are "proven low-noise against the existing corpus." They are low-noise only against the 16-file linted subset, and there is no trial artifact in the tree to reproduce the claim —
.vale.inihas noPackages, and write-good/alex were never synced, so which rules were rejected and why is unrecorded. The commit message cites "the ADR's rejected-rule list"; no such list exists in the ADR.Suggest cutting the tokens that measure as pure noise here —
obvious,obviously,clearly,usually,mostly,several,various,substantially,significantly,relatively,largely,fairly,simple,easy— and keeping the defensible filler:of course,everyone knows,interestingly,surprisingly,remarkably,exceedingly,very,quite,huge,vast,excellent.🟠 Gaps
Only 1 of 6 rules can actually block a commit. Vale exits non-zero on
erroralerts only.DescriptionOpeneriserror; the other five arewarning.MinAlertLevelaffects display, not exit code — and pre-commit discards hook stdout on success, so warnings are invisible:ADR-0013 and
CONTEXT.md:77describe the hook as "blocking immediately." Empirically it blocks onDescriptionOpeneronly. Set--minAlertLevel=warningon the hook entry, or raise the rule levels.A blank line inside a folded description corrupts the scratch copy and hard-fails the commit. The body regex at
vale-wrap.sh:61can't cross a blank line, so a two-paragraph description matches only the first; the second is left orphaned after the scalar. Output is invalid YAML →yaml: line 5: did not find expected key, wrapper exit 2, hook fails against a valid file. 0/45 files have this shape today, but "Use when… / Do not use when…" split across paragraphs is natural for exactly these descriptions.--config=X(equals form) breaks. Only the two-argv form is rewritten to an absolute path; the equals form passes through and dies against the temp cwd (E100 path '.vale.ini' does not exist). Bare vale accepts it.Zero file arguments hangs forever.
exec vale "${vale_args[@]}"with no positional falls through to stdin and blocks indefinitely (verified:timeout 8→ exit 124). Reachable whenever a caller's file list filters to empty. Needs a guard plus</dev/null.Absolute paths silently no-op.
vale-wrap.sh:32-36requires-f "$repo_root/$arg", so an absolute path never entersfiles[], is never flattened, and can't match.vale.ini's anchored globs → exit 0, no output. Same for a typo'd path and for invocation from a subdirectory. This matters because the Step 1 instruction isscripts/vale-wrap.sh … <skill-dir>/SKILL.mdand an agent substituting an absolute skill dir — routine — gets a clean pass and reports "no Vale findings."Item 3 (styles in the plugin) is still open, and it leaves the prefilter dead for every external consumer.
styles/and.vale.iniexist only at repo root.skill-audit/SKILL.md:37andagent-audit/SKILL.md:39hardcodescripts/vale-wrap.sh --config .vale.ini. Verified in a scratch external repo:No such file or directory (127), and with the wrapper present but no config,E100 path '…/.vale.ini' does not exist. So for anyone installingkyberforge@holocronelsewhere, Step 1 degrades to "skip and fall back to judgment" and can never fire. Fine to defer — but it's recorded nowhere, and ADR-0013 should carry it as a known limitation. Precedent for the fix exists inskill-author/assets/templates/+${CLAUDE_PLUGIN_ROOT}.Item 4 is incomplete — both marketplace manifests were missed.
.claude-plugin/marketplace.json:43and.github/plugin/marketplace.json:43still read "…running linters, starting with Vale." while bothplugin.jsonfiles were genericized.lintis now the only plugin in the repo whose marketplace description diverges from itsplugin.json— the other five match byte-for-byte.check-manifests.shdoesn't catch this (it validates path resolution only).vale-runandlint-runnerbypass the wrapper.vale-run/SKILL.md:30-31documentsvale <path-or-glob>;lint-runner.md:21routes to it. Neither mentions the wrapper. Alint-runnersweep therefore reports clean on files the commit hook would reject.CONTEXT.md:75enumerates the wrapper's call sites as "both skills' Step 1, and the pre-commit hook" — these two are absent, which reads as an oversight rather than a decision. Related:vale-run/references/troubleshooting.md:57-70recommends the upstreamerrata-ai/valepre-commit hook, which callsvaledirectly — correct generic advice, but following it here reintroduces the bug the wrapper exists to fix.The
valebinary is an undocumented hard prerequisite. With vale offPATH:scripts/vale-wrap.sh: line 79: vale: command not found, exit 127, surfaced by pre-commit as a bare hook failure with no install pointer.AGENTS.mdSetup mentions onlypre-commit install. A fresh clone hard-blocks every commit touching a SKILL.md or agent file until the developer works out that Vale is needed. Should be named in AGENTS.md Setup.skill-size-checkis ~70% more permissive than the ceiling it advertises.scripts/skill-size-check.sh:10-13argues the word proxy is "conservative." The inequality is backwards — words are always fewer than tokens. Measured on this repo's 39-file SKILL.md corpus: 6.80 chars/word → ~1.70 tokens/word, soMAX_WORDS=5000≈ 8,500 tokens against a stated 5,000.skill-author/SKILL.mdis at 81% of the real token budget but only 50% of the word budget — the hook cannot warn before it blows through. The error string at:30prints a word count and calls it a token ceiling. Either setMAX_WORDS≈ 2,900–3,800, or drop the token claim. (Source citation itself checks out:skill-authoring.md:163says "under 500 lines and 5,000 tokens" verbatim.)The two new rules are not wired into the audit skills' mapping prose.
skill-audit/SKILL.md:56andagent-audit/SKILL.md:55-57,64name onlyDescriptionOpener/VagueWording/PaddingPhrase/ProactivePhrase. An audit run has no instruction for what to do with aVagueQualifierorSentenceOpenerThereIsalert.lint-runnerhas no provenance chain. Per ADR-0010, plugin-scope agents carrysource_keysagainst a plugin-rootsources.md.git-orchestrate(6 keys) andgitea-orchestrate(4 keys) both comply;plugins/lint/sources.mddoesn't exist and the agent pair has nosource_keys. Invisible to tooling —validate-provenance.shexits 0 silently when no provenance data exists. Thelintskills are wired correctly, so the agent is the only artifact in the plugin without it.Docs contradict the commit that added them.
CONTEXT.md:77calls the write-good/alex rules "(still pending implementation)" — same commit implements them.CONTEXT.md:73still says Vale "only" covers the four #84 checks and that "body discipline stays LLM judgment," which the two new body-wide rules now falsify. Neither new rule is named anywhere in CONTEXT.md.skill-size-check), and a "follow-up work — not part of this ADR" list whose every item is in2eb13f7.plugins/kyberforge/docs/README.md:20still indexesresearch/docs/vale/, which moved toplugins/lint/ine1a5403. The directory doesn't exist;plugins/lint/docs/has no replacement index.🟡 Minor
SentenceOpenerThereIs.yml:7— first regex alternative is dead.(?:[;-]\s)There\s(is|are)is fully subsumed by the\bThere\s(is|are)\balternative: identical alert count, identical flagged sites. Its only effect is widening the captured span, which corrupts%sintoDon't start a sentence with '; There is'. Also[;-]is ASCII hyphen — this repo's prose uses em dashes, so even the intended target is the wrong character.^, noscope: sentence;ignorecase: falseis the only proxy for sentence-initial position. Over-matches headings, mid-sentence, table cells, link text; under-matchesthere islowercase,There's,There exist.VagueQualifierandSentenceOpenerThereIslint all YAML frontmatter keys. They're the only two rules using barescope: text; the other four aretext.frontmatter.description. Verified a skill namedvery-simple-skillgets flagged on itsname:line — unfixable without renaming the directory. No current name collides, but it also means the full 30-token list is now live against every skill'sdescription:.|.CONTEXT.md:75andvale-config/SKILL.md:25both say the scope breaks for "a block scalar (>/|)". Only>(folded) breaks — bare vale matches every line of a|literal block.vale-configis shipped content agents treat as ground truth.vale-config/SKILL.md:22asserts a fresh config "will fail or find nothing untilvale syncruns" — this repo declares noPackagesand works fine;vale syncis a no-op here. Scope the claim to package-based styles.skill-size-check.shboundary is inclusive (>), so exactly 500 lines passes where the source says "under 500";wc -lundercounts a file with no trailing newline, disagreeing withskill-audit/scripts/validate.sh:137(splitlines()) at the boundary; nonexistent paths, directories, and zero args all exit 0 silently.lint-runner's genericization is a naming convention, not dispatch — no registry, no discovery, sample size of one. The<linter>-config/<linter>-runguard at:24is the substantive part and is good. But frontmatter:4still namesvale-config/vale-runconcretely, contradicting the<linter>-*body two lines down.lint-runneris the repo's only agent pair with a non-1:1 tool mapping (Grep+Glob→ singlesearch). Plausibly correct, but it's the sole precedent and unverified againstfield-inventory.md.Test coverage
Both new suites assert the shape of the implementation, not the spec.
test-vale-wrap.sh— fixture deliberately includes a colon but no double quote, which is exactly why it passes while 26 real files silently fail. Uncovered: quotes/backslashes/unicode, blank line in a folded block,--config=form, zero args, absolute/nonexistent/subdirectory invocation,|and>-/|+variants, plain and quoted multi-line descriptions, exit-code fidelity. Test 3's line-preservation assertion (MAX_LINE <= ORIG_LINES) passes trivially if flattening drops all alerts.test-skill-size-check.sh— fixtures are 10 lines / 604 lines / ~6,004 words, all 20%+ from the threshold. Flipping>to>=, or changingMAX_LINESto 550, passes all three tests. No boundary pairs (500/501, 5000/5001), no zero-arg/missing-path/empty-file cases.Credit where due:
test-vale-wrap.shis a real regression test for the bug it was written for — neutering the flattening in a copy flips test 2 to FAIL. The gap is coverage, not construction.What genuinely holds
docs/research/examples/**files andassets/templates/SKILL.md, misses zero legitimate ones.>-,description:not first key, nestedmetadata: description:correctly left alone,---rules in body, CRLF, and line-number preservation.KyberforgeCopilotscoping is correct and the round-1 rationale (item 1) is right:Use proactivelyis live in CC, dead in Copilot. Verified it fires on.agent.mdand not.md.scope: text.simpledoes not flagsimplify,variousdoes not flagvariation. The separatesimple/simplyentries are each necessary.%sinterpolation correct for bothtokensandraw.lintagrees on name/version/paths; agent pair diff is a single line (the tools mapping);agent-audit's own validators pass on it.Recommendation: request changes. Blocking 1 is the round-1 finding unfixed — same silent-clean signature, 58% of the corpus. Blocking 2 is a shipped skill that behaves differently now because a linter was wrong. Blocking 4 says the rule generating those edits is wrong ~100% of the time on this repo's voice, which makes the 28-violation cleanup a net negative on prose quality. I'd land 1 and 3 as fixes, revert the 2/4 prose edits wholesale, cut the token list down before re-running, and split the size-check into its own PR.
Pushed
55dc065, addressing everything open from both round-1 and round-2 review — grilled the open design questions against ADR-0013/CONTEXT.md/ADR-0010 first, then implemented in parallel across the affected plugins.Blocking (round 2):
vale-wrap.shswitched fromjson.dumps()to a single-quoted YAML scalar. One step further than the suggested fix:''-doubling for embedded apostrophes turned out to have the same silent-truncation bug (Vale's frontmatter scanner isn't a full YAML parser), reproduced and confirmed. Fixed by substituting a Unicode right single quote for apostrophes in the scratch-only copy instead. New fixtures cover quotes, apostrophes, backslash+non-ASCII, and a blank line inside a folded description (which was also silently truncating the body capture — fixed via indentation-based parsing).bin/git/gitea/kyberforge. Guarded caveman's filler-word line withvale offcomments so a mention can't be mistaken for a use again.vale-wrap.sh/.vale.iniviagit rev-parse --show-toplevel, independent of caller cwd.simplyhad to be cut, since it's literally the word caveman's own filler-list quotes as a mention (missed by the original list, and load-bearing for reverting the caveman regression without immediately re-breaking it).Gaps:
--minAlertLevel=warningwired into the hook and Step 1 (severities left alone — raising them would've collapsed the FAIL/SUGGESTION mapping the audit skills rely on);--config=equals-form, absolute-path no-op, and zero-arg stdin hang all fixed invale-wrap.sh;vale-run/lint-runnernow prefer a documented wrapper over barevale; both new rules wired into the audit skills' Body-discipline mapping;plugins/lint/sources.mdadded forlint-runner's provenance (ADR-0010); bothmarketplace.jsons synced toplugin.json's wording (+ patch bump, permarketplace-author's own convention);skill-size-check.sh'sMAX_WORDSretuned 5000→2900 (measured this repo's actual chars/word ratio — the old value was gating at ~8,500 tokens against a stated 5,000 ceiling) plus its>/>=boundary andwc -ltrailing-newline bugs;valedocumented as anAGENTS.mdSetup prerequisite;SentenceOpenerThereIs's dead regex branch and missing sentence-anchor fixed; staledocs/research/docs/vale/pointer removed from kyberforge's docs README; ADR-0013's Consequences rewritten past-tense.Deliberately deferred (not silently dropped): styles-portability (moving
styles//.vale.iniintoplugins/lint/for external installs) stays repo-root for now — intentional per this ADR, tracked as a known limitation, revisiting in a separate session. Filing issues for this repo's other pre-commit linters (shellcheck etc.) is likewise out of scope for this PR.Verification:
bash tests/run-tests.sh— 9 scripts + 125 bats assertions, all passing (9 newvale-wrap.shfixtures + boundary-pair tests forskill-size-check.shadded).scripts/check-manifests.shandclaude plugin validate --strictboth clean. All of this ran through the actual pre-commit/pre-push hooks on the way in, including the newly-fixedvale-audit-prefilterandskill-size-checkhooks linting their own fix.🤖 Generated with Claude Code
Review round 3 (read-only — no edits made)
Verified empirically against the actual tree with
vale3.15.2 rather than reading the diff — ran the wrapper, the repo-wide sweep, and injected known-bad fixtures for each rule. Full test suite passes (9 scripts, 0 failed).What the last commit did
6c0afb7addsplugins/lint/docs/README.md— a 27-line index for the Vale research docs mirroringplugins/kyberforge/docs/README.md. Accurate against the actual directory contents, no issues.The round-2 fixes hold up
Re-tested the things #1294/#1309 claim to have fixed; they genuinely work:
description: >containingThis skill,helps with,utilize,used for,assists withfired all five. The original blocking bug is dead.DescriptionOpener,VagueWording,PaddingPhrase,VagueQualifier,SentenceOpenerThereIs,KyberforgeCopilot.ProactivePhrase), andProactivePhrasecorrectly does not fire on the CC.mdfile.docs/research/examples/andassets/templates/.🔴 Blocking: the "enforcing" hook doesn't enforce — Vale exits 0 on warnings
Five of the six rules are
level: warning. OnlyDescriptionOpeneriserror. Verified:vale --helpconfirms the model:--no-exit Don't return a nonzero exit code **on errors**.--minAlertLevelcontrols display only, not the exit code.Because pre-commit suppresses output from passing hooks, the net effect is worse than advisory: a commit introducing
helps with,utilize,Use proactively,very, orThere isproduces no output at all and exits 0. The warnings are not just non-blocking, they're invisible.This contradicts three places in this PR:
styles/Kyberforge, enforcing immediately" and :68 — "goes live in the blocking pre-commit hook immediately"CONTEXT.md:77— "rules land directly instyles/Kyberforge, blocking immediately, no trial tier"ADR-0013 explicitly rejected a report-only trial tier in favour of immediate enforcement. What actually shipped is a report-only tier whose reports nobody sees — the rejected option, arrived at by accident.
#1309 says severities were "left alone — raising them would've collapsed the FAIL/SUGGESTION mapping the audit skills rely on." Sound for the audit path, but it doesn't transfer to the hook path: the audit skills read Vale's severity strings, which are independent of the process exit code. You can have both. Options, in order of preference:
--output=lineor--output=JSON), keeping severities intact for the audit mapping. Cleanest — decouples the two consumers.errorand key the audit skills' mapping off rule identity rather than severity.🔴 Blocking:
vale-run's documented exit-code semantics are factually wrongplugins/lint/skills/vale-run/SKILL.md, first Gotcha:Disproved by the run above:
MinAlertLevel = suggestion, two warnings found, exit 0. This is shipped, installable guidance — an agent following it will build a CI gate that silently passes on everything excepterror-level alerts. It's the same wrong premise that produced the finding above, so I'd fix the skill and the hook together rather than as separate items.The
--no-exitrow in the flag table ("Forces exit code0regardless of findings") inherits the same error, as doesreferences/troubleshooting.md:53.🔴 Blocking:
0 filesreads as "clean" — the round-1 false-negative class, in a new place.vale.ini's globs areplugins/*/skills/*/SKILL.mdandplugins/*/agents/*.md. A skill anywhere else gets zero files scanned and a green checkmark:The fixture contained
This skill,helps with,very, andThere is.skill-audit/agent-auditare plugin skills — they audit skills in any repo, plus project-scope (.claude/skills/) and user-scope skills in this one. For every one of those, Step 1's Vale pass returns a clean bill of health, and the SKILL.md then instructs the auditor to "report them as findings without re-deriving by judgment" for the Description, Patterns, and Body-discipline sub-checks. Those checks get skipped in both directions.The existing escape hatch — "Skip and fall back to judgment if vale or
.vale.iniis unavailable" — doesn't catch this, because Vale is available and does succeed. It just scanned nothing.This is the deferred styles-portability item's blast radius, and it's larger than ADR-0013:81-87 frames it. Deferring the fix (moving
styles/intoplugins/lint/) is defensible; deferring the guard isn't. Minimum viable fix in this PR: have Step 1 assert Vale reported ≥1 file scanned, and fall back to Step 2/3 judgment when it didn't.🟠 The documented Markdown suppression syntax doesn't work
vale-run/SKILL.mdGotcha 3 andreferences/troubleshooting.md:10-15both give{/* vale off */}as the "Markdown/MDX" syntax. Tested side by side in one.mdfile:{/* vale Kyberforge.VagueQualifier = NO */}<!-- vale Kyberforge.VagueQualifier = NO -->{/* */}is MDX-only. This repo's own fix — the caveman guard inplugins/bin/skills/caveman/SKILL.md— correctly uses<!-- -->, so the shipped skill contradicts the working practice introduced in the same PR. An agent followingvale-runwould add a suppression that silently does nothing, which is how the caveman regression happened in the first place.🟠 The branch doesn't merge
Gitea reports
mergeable: false. One real conflict, inAGENTS.md: main's #86 rewrote the Setup section, and this branch's vale-binary prerequisite lands inside the removed block. Worth flagging because a naive resolution loses content in both directions — main's rewrite already referenceslint:vale-config/lint:vale-run, skills that only exist on this branch, while dropping the vale-install prerequisite that makes them usable. Resolution needs the prerequisite re-homed into main's new "Setup and testing" section, not either side taken wholesale.CONTEXT.mdalso changed on both sides but merges cleanly.🟠
vale-wrap.shisn't the drop-in replacement its docstring claimsLines 18-39 resolve every relative
--configpath againstrepo_root, not the caller's cwd:The header comment says "Drop-in replacement for calling
valedirectly: same args, same exit code," and both audit SKILL.mds tell agents it "runs correctly regardless of the caller's cwd." Both are true only for repo-root-relative or absolute paths — an undocumented invented convention.tests/test-vale-wrap.sh:214-215blesses it by only ever testing the root-relative form from a subdirectory.Low real-world impact (both call sites use absolute paths, and it fails loudly), but the docstring and the two SKILL.md sentences overstate the contract. Either resolve relative paths against
$PWD— the actually-drop-in behaviour — or document the convention as a constraint.🟡 Minor
skill-audit/SKILL.md:70documentsKyberforge.VagueQualifieras flagging "vague filler like 'clearly', 'obviously'". Neither token is instyles/Kyberforge/VagueQualifier.yml— the list iseasily, everyone knows, exceedingly, excellent, extremely, huge, interestingly, of course, quite, remarkably, surprisingly, vast, very. Pick real examples from the list, or add those two tokens.scripts/skill-size-check.sh:29fails atlines >= 500;skill-audit/scripts/validate.sh:138passes atline_count <= 500. A 500-line SKILL.md passes its own audit and is blocked by the commit hook. Pick one boundary. (TheMAX_WORDS=2900retune is fine — largest file in the corpus is 2,489 words / 292 lines, real headroom.)AGENTS.md: "then runvale syncto pull the styles declared in.vale.ini." There's noPackageskey —vale syncreportsSynced 0 package(s).Kyberforge/KyberforgeCopilotare committed directories. Harmless but misleading; drop it or say the styles are vendored.Also checked, no issues
plugins/lint/{plugin.json,.claude-plugin/plugin.json,.mcp.json,hooks.json}match the established per-plugin pattern (empty{"mcpServers":{}}/{"hooks":{}}stubs are the repo convention, not omissions); bothmarketplace.jsons are in sync at0.3.1with matching descriptions;sources.mdprovenance satisfies ADR-0010; ADR-0013's structure matches ADR-0010's; thelint-runnerpair is consistent across providers with correctsearch→Grep, Globmapping; the 27 prose edits acrossbin/gitea/kyberforgeare genuine improvements, not token-churn.Recommendation: the three 🔴 items share one root cause — an incorrect model of Vale's exit-code and file-matching behaviour — so they're one focused fix, not three. Hold the merge for those plus the
AGENTS.mdconflict resolution; the 🟠/🟡 items are safe to batch into the same pass.6c0afb7c81toaff5b6c4c8Pushed
149d564+aff5b6c, resolving everything from round-3 review. Grilled the open design questions first — the outcome changed two of my own recommendations, noted below rather than quietly swapped.⚠️ Branch was rebased — old commit links are dead
This branch was rebased onto
main(which had moved on via #86) rather than merged, so every pre-existing SHA reference in this thread now points at a commit that is no longer on the branch. Mapping, oldest last:0bbb965bbb0dcd55dc065792d3e16c0afb7210b192Full remap for the rest of the branch:
2eb13f7→3324a73,28058d3→544392b,7118ace→8d56290,75f65d8→cbc33d9,e1a5403→f326df4,4d6f313→57bdfa9,0c0d51f→59ad2a3,cb2257d→8b00728,e4abe23→d1afdbe,88888c4→5e22672. Subjects are unchanged, so the mapping is 1:1 by commit message. The same table is now in the PR description so it survives thread scrolling.The
AGENTS.mdconflict is resolved and the PR ismergeableagain. Resolution needed care in both directions:main's rewrite had already addedlint:vale-config/lint:vale-runreferences while dropping the vale-install prerequisite, so the prerequisite was re-homed into main's new "Setup and testing" section rather than either side being taken wholesale.🔴 #1 — the gate didn't gate. Fixed, and the fix is smaller than proposed
Confirmed exactly as reported. The fix went a different way than my own suggestion: rather than wrapping the hook to count alerts, every rule is now
level: error, so Vale's own exit code is correct and the hook entry collapses to a barescripts/vale-wrap.sh --config .vale.ini— no wrapper-around-the-wrapper, no JSON parsing, nojq.That means the graded
error→FAIL /warning→SUGGESTION mapping is gone: every Vale alert is a FAIL, in the commit gate and the audit alike. #1309's objection — that raising severities would collapse the mapping the audit skills rely on — is real, but the code says the dependency isskill-author's must-fix gate (SKILL.md:223,:288), not report cosmetics. Making prose violations must-fix is the intended behaviour, and it matches every other gate here: shellcheck, the test suite, and conventional-pre-commit all have no ignorable tier. One rule set, one verdict, nothing to keep in sync.ADR-0013 now records why graded severities cannot gate, so this can't be reintroduced by accident. Its "Considered options" also notes that shipping graded severities accidentally recreated the report-only trial tier that same section had rejected.
🔴 #2 —
vale-run's exit-code model. Fixed, plus a third instanceCorrected in
vale-run/SKILL.md, its--no-exitand--minAlertLevelflag rows, and — not in the original report — a third copy of the same false claim inreferences/troubleshooting.md's "CI failing unexpectedly" section, which would have re-propagated it.Also fixed alongside: the documented Markdown suppression syntax.
{/* vale off */}is MDX-only and does nothing in a plain.mdfile — verified side by side; the alert still fires. Markdown needs<!-- vale off -->. Both the skill and the reference now show the two forms separately.🔴 #3 —
0 filesreads as clean. Fixed at the root, not with a guardInvestigating this turned up something better than the proposed guard:
.vale.ini's globs were scoping nothing at all. Vale's*crosses/, so[plugins/*/agents/*.md]already matchedplugins/*/docs/research/examples/**/agents/*.md, and[plugins/*/skills/*/SKILL.md]already matchedassets/templates/SKILL.md— the two paths CONTEXT.md claimed they excluded. Verified against fixtures. All real scoping is, and always was, the pre-commit hook'sfiles:regex.agent-audit's own Gotchas already say plugin scope is detected byplugin.jsonpresence, "not by the file path pattern" — the Vale step was violating that. So the globs are now path-agnostic ([**/SKILL.md],[**/agents/*.md],[**/*.agent.md]), which makes the prefilter work on project-scope and other-repo skills instead of silently reporting them clean. The hook'sfiles:regex is untouched and still does all the scoping. The 0-files guard is in both audit skills too, as defence-in-depth: a zero-file run is now NOT RUN, not clean.🟠
VagueQualifierdeleted — measured, not assumedNot in the original report; it came out of grilling whether the rule earns a blocking severity. Measured against the 41 skill/agent files as they stood before the rule ever ran:
VagueQualifierSentenceOpenerThereIsThe false positive is
caveman/SKILL.md, which quotes "of course" as an example of filler — a mention, not a use, unfixable by rewriting. It forced the only Vale suppression comments in the repo, two of which were dead anyway (they suppressedVagueWording, a frontmatter-scoped rule, on a body line). On 273 held-out markdown files it fired 15 times: 9 inside out-of-scopedocs/research/examples/, and the remaining 6 all the word "very" in two idioms in a single research doc, each already sitting next to the hard number carrying the fact.One marginal catch per 41 files doesn't pay for a permanent suppression comment, so the rule is gone and all four suppression lines with it — the repo is back to zero suppressions.
SentenceOpenerThereIssurvives on its own evidence (22 held-out hits, no suppressions needed).New house convention recorded in CONTEXT.md and
vale-run: banned phrasing that must be mentioned rather than used goes in backticks or a fenced code block — verified that Vale skips code spans and fences, so no suppression is needed at all. Inline<!-- vale Rule = NO -->is the fallback only where backticking is impossible.🟠 / 🟡 remainder
vale-wrap.shnow resolves relative--configvalues and relative file arguments against the caller's cwd, asvaledoes. The file-argument half was worse than the--confighalf I reported: a relative path that didn't resolve from the repo root fell through andexec'd barevale, silently skipping the frontmatter flattening the wrapper exists for. Absolute paths inside the cwd are relativized so reports cite resolvable paths rather than scratch ones.skill-size-check.shfails only above 500 lines, agreeing withvalidate.sh's<= 500.VagueQualifierdoc mismatch ("clearly"/"obviously", neither of which was ever in the token list) removed along with the rule.AGENTS.md'svale syncinstruction dropped — there is noPackageskey, so it syncs 0 packages; the styles are committed understyles/.bin1.1.1,kyberforge1.2.4,lint1.1.3. Shipped skill content changed in all three without a manifest change, so installed copies would have kept serving from cache. It matters most forlint: anyone at 1.1.2 has a cachedvale-run/SKILL.mdtelling them Vale exits non-zero on warnings.Still deferred
Styles portability — moving
styles/and.vale.iniintoplugins/lint/so the prefilter travels to repos that installkyberforge@holocronexternally. Unchanged from #1309 and still recorded as a known limitation in ADR-0013. The path-agnostic globs above reduce its urgency (the config now works wherever it's pointed) but don't remove it: an external install still has no.vale.inito point at.Verification
bash tests/run-tests.sh— 9 scripts, 0 failed; thevale-wrap.shsub-suite is 15 cases, including three new ones for the cwd-relative--configand file-argument regressions. Those three were checked for non-vacuity by running them against the previous script: they fail there and pass here.pre-commit run --all-filespasses forvale-audit-prefilter,skill-size-checkandshellcheck;scripts/check-manifests.shandclaude plugin validate --strictclean on every plugin. All of it went in through the real pre-commit and pre-push hooks, including the newly-strict Vale gate linting its own fix.One pre-existing breakage surfaced and was fixed rather than worked around:
tests/test-vale-wrap.shbroke the moment rules becameerror, because itspipefailassertions inverted once Vale started exiting 1. Fixed with a helper that separates alert-text assertions from exit-code assertions.🤖 Generated with Claude Code
Added two follow-up commits addressing the styles-portability limitation ADR-0013 deliberately deferred:
1164f3a— make Vale prefilter portable via the pluginskill-audit/agent-auditresolvedvale-wrap.sh/.vale.iniviagit rev-parse --show-toplevel, which only works inside this repo — installed as an external plugin, the prefilter silently fell back to full LLM judgment. Vale's config/styles/wrapper now ship inside the plugin itself (plugins/kyberforge/skills/agent-audit/assets/vale/canonical,plugins/kyberforge/skills/skill-audit/assets/vale/subset). A new root.pre-commit-hooks.yamllets external repos enforce the same rules via git hooks/CI, independent of Claude Code entirely. This repo's own pre-commit hook now dogfoods the same plugin-bundled copies (repo: local, split into-skill/-agenthooks after confirming a combined hook would silently 0-file-skip one file type).scripts/check-vale-style-sync.shguards the two copies against drift.4d018af— hard-fail on main when a release tag is neededExternal consumers pin
rev: <tag>, soscripts/check-release-needed.shnow hard-fails atpre-push, but only when pushing tomain— it checks whether files exposed via.pre-commit-hooks.yamlchanged since the last tag (or no tag exists yet) and fails if so. Silent no-op on feature branches, so it never forces a premature tag on a commit that might not survive a squash-merge.Full details in
docs/adr/0014-vale-prefilter-ships-from-the-plugin.md. First release tag is intentionally deferred until this merges tomain— tracked in #87.Review of the two follow-up commits (
1164f3a,4d018af) — read-only, then fixedRan a full code review against
1164f3a^..4d018af(the portability move + the release-tag hard-fail hook added after round 3 closed). 7 findings, all in the newcheck-release-needed.sh/check-vale-style-sync.shmachinery — nothing in the portability move itself. Fixed all 7 inacd2f1d.scripts/check-release-needed.sh— the gate could fail open four ways-eexistence filter dropped aRELEASE_PATHSentry from the diff pathspec once it no longer existed at HEAD — so deleting a path.pre-commit-hooks.yamlexposes, between tags, passed the gate clean.git diffreports deletions fine without an existence check; the filter is gone.git diff ... 2>/dev/null || trueturned any git error (shallow clone missing the tag's objects, a bad ref) into an empty, falsely-clean diff. Verified by deleting the tagged commit's tree object directly (describestill resolves the tag name;diffagainst it then fails) — the gate now hard-fails on that instead of passing. Same "clean means nothing ran" shape as the round-1/round-3 findings on this PR.RELEASE_PATHSwas hand-maintained and already over-broad — a parallel array duplicating.pre-commit-hooks.yaml'sentry:paths with only a comment keeping them in sync, and it swept invalidate.sh/validate-provenance.sh, which no hook entry actually references. Now parsed straight from.pre-commit-hooks.yaml'sentry:lines at runtime, so it can't drift from the manifest and only tracks what's actually exposed.git describe --tags --abbrev=0accepted any tag reachable from HEAD, so an incidental checkpoint/experiment tag could shift the diff baseline and mask a real release-relevant change. Added--match 'v[0-9]*.[0-9]*.[0-9]*'.git pushthrough pre-commit's pre-push hook. A PR merged via Gitea's merge button (server-side, no local push — how this repo's PRs actually land) or CI invokingpre-commit run --hook-stage pre-pushdirectly never setsPRE_COMMIT_REMOTE_BRANCH, so the gate silently doesn't run in either path. Closing it needs a server-side CI job this repo doesn't have yet — recorded as a known limitation in ADR-0014's Consequences instead of overstating coverage.Added 4 regression tests to
tests/test-check-release-needed.sh, one per fix above, each checked for non-vacuity (fails against the pre-fix script, passes against the current one).scripts/check-vale-style-sync.sh— detection without a fixIt only ever detected drift between skill-audit's and agent-audit's duplicated
vale-wrap.sh/styles/Kyberforgecopies after a human had already hand-edited them out of sync (both copies must exist independently — a plugin's cache-install only copies each skill's own files, so a symlink would break at install time). Addedscripts/sync-vale-styles.shto regenerate skill-audit's copy from agent-audit's canonical one on demand, and pointed the sync check's failure message at it — fixing drift is now one command instead of a hand diff across two files.Also cleaned up in the same pass
LESSONS.md's "a clean check can mean nothing ran" entry was marked Graduated without ever being promoted per the repo's own graduation rule (3+ instances → a standing doc, marked[graduated → target file]). Actually promoted it intocore/instructions/testing.mdand fixed the marker.Verification:
bash tests/run-tests.sh(11 scripts + 125 bats, all passing),pre-commit run --all-files, andpre-commit run --all-files --hook-stage pre-pushall clean — including the newly-hardenedcheck-release-needed/check-vale-style-synchooks passing against their own fix.Review round 4 — validated
Two-pass review: an adversarial review agent, then an independent validator tasked with falsifying each finding. The validator rejected 2 findings outright, downgraded 2, and raised 1 — the list below is post-validation only.
Verdict: needs work. One Critical, one High. The in-repo path is solid —
tests/run-tests.sh11 passed / 0 failed (144 assertions),check-manifests.shexit 0,pre-commit run --all-filesall 17 hooks pass, all 9 rule files verifiablylevel: error, and flattening holds across CRLF, apostrophes, backslashes, non-ASCII, blank lines in folded scalars,|literal scalars, spaces in paths, absolute paths and odd cwd. Both defects that matter are at the boundary: the external-consumer contract, and non-Linux hosts.1. Critical —
.pre-commit-hooks.yaml:4,11— the external-consumer contract is non-functionalBoth Vale hook entries pass
--config plugins/kyberforge/…/assets/vale/.vale.inias an argument. pre-commit prefixes onlyentry[0]with the hook-repo clone path —pre_commit/languages/unsupported_script.py:Remaining args are passed verbatim and resolve against the consuming repo's root. Reproduced against a real
file://bare remote (not just a local path), with both aSKILL.mdand an agent file present so both hooks actually fire:So 2 of the 3 hooks ADR-0014 promises are unusable, and the premise of
check-release-needed.sh(gatemainso consumers can pin a workingrev:) does not hold.Why three rounds missed it: the byte-identical entry string passes in-repo, because
repo: localmakes prefix == cwd == repo root. The defect is structurally invisible from inside the repo, and nothing intests/exercises.pre-commit-hooks.yamlas a hook repo. That is the exact failure mode ADR-0014:79-80 states it was designing against ("the same portability path an external repo would, not … a special root-only case that never gets exercised the way external consumers exercise it"). The relocation did not achieve its stated goal.Fix: make the wrapper self-locating and drop
--configfrom the manifest.entry[0]is resolved into the clone (proven — the script ran), so default the config invale-wrap.shto$(cd "$(dirname "${BASH_SOURCE[0]}")/../assets/vale" && pwd)/.vale.iniwhen none is supplied. Then add a test that stands up a throwaway consumer repo and runspre-commit run --all-filesagainst the manifest.2. High —
vale-wrap.sh:81,96—realpath -mis GNU-only, no fallbackrealpath -m(resolve without requiring components to exist) is a GNU coreutils extension; macOSrealpathis FreeBSD-derived and has no-m. No fallback, no platform detection (grep -E 'greadlink|uname|Darwin|command -v realpath'→ no matches).-mis load-bearing, not incidental — at line 96destdoes not exist yet (mkdir -pis line 107):Under
set -euo pipefailthat aborts the script. GNU coreutils is documented nowhere in the repo (grep -rn coreutilsover all.md/.sh/.yaml→ zero hits), whileAGENTS.md:25lists macOS first for these exact hooks (brew install vale(macOS)). A macOS contributor following the repo's own setup docs has every commit touching aSKILL.mdor agent file blocked with an opaqueillegal option -- m.Raised from Medium during validation: the repo explicitly claims macOS support for this code path, the failure is total, and there is no documented workaround. BSD behavior is reasoned from the FreeBSD/coreutils divergence, not measured — no Mac available.
Fix: replace both calls with
python3 -c 'import os,sys;print(os.path.abspath(sys.argv[1]))'(python3 is already a hard dependency of this script) or a pure-bash normalizer.3. Medium —
vale-wrap.sh:58— a directory argument silently skips flatteningThe classifier
[[ "$arg" != -* && -f "$arg" ]]is false for anything that is not a regular file, so a directory lands invale_args,file_argsstays empty, and line 78execs barevale— skipping the flattening the script exists to perform. Same file, two invocation forms:Lowered from High during validation. Not reachable via pre-commit (
run_xargspasses explicit filenames) nor via either audit skill's Step 1 (both prescribe explicit file paths). Reachable only through thelintplugin'svale-runskill /lint-runneragent, whose documented default form isvale <path-or-glob>(vale-run/SKILL.md:32) and whose Gotcha (line 25) tells an agent to reuse the same arguments with a wrapper. Both realistic sweep forms under-report:Fix: walk a directory argument and flatten each candidate, or refuse it with a clear error. (A nonexistent path is not a defect — it behaves identically to bare vale, which is the wrapper's stated drop-in contract.)
4. Low —
plugins/lint/docs/research/docs/vale/cli-reference.md:29— exit-code model stated backwardsDisproved with a
level: warningrule underMinAlertLevel = suggestion:Non-zero keys on
error-level alerts alone;MinAlertLevelis display-only. This is the precise misconceptionLESSONS.md's "One signal, two consumers" entry records as costing two review rounds — and this file is the cited provenance source for the skills that now state it correctly. A future author who extends the harness from the research doc rather than the skill ships an invisible non-gating rule.Correction to the first-pass review: it also flagged
vale-run/references/troubleshooting.md:56. That was misattributed — that file is correct and precise at line 68. Only the research siblingdocs/research/docs/vale/troubleshooting.md:56is loosely worded in the same direction.5. Low —
CONTEXT.md— the NOT-RUN rationale is factually wrongIt matches fine:
[**/SKILL.md]matches any path ending inSKILL.md— as the same paragraph concedes two sentences earlier ("Vale's*crosses/"). Reads like a stale leftover from a path-scoped glob. The same paragraph also cites the localfiles:regex^plugins/[^/]+/skills/[^/]+/SKILL\.md$as the scoping mechanism, where.pre-commit-hooks.yamlactually ships(^|/)SKILL\.md$to external consumers.6. Nit —
tests/test-vale-wrap.sh:21-24The whole Vale suite reports green when the
valebinary is absent (run-tests.sh:44incrementsPASSEDon exit 0), so the summary reads11 passed, 0 failedeither way. This PR added the Vale install pointer toAGENTS.mdprecisely because people hit "command not found". Consider aSKIPPEDcount in the summary.7. Info —
scripts/check-vale-style-sync.sh:20-22if [[ ! -d "$SKILL_AUDIT" || ! -d "$AGENT_AUDIT" ]]; then exit 0; fi— the||means one copy missing also passes vacuously, where the intended no-op is both missing. Deliberate and tested for the both-missing case; bounded impact.Rejected during validation
Recorded so they don't resurface next round:
0 filesguard is correct. The first pass claimed explicit file paths matching no glob section emitin stdin.rather than0 files, defeating the guard. False — an existing file matching no section emits✔ 0 errors … in 0 files., exactly what the guard checks.in stdin.appears only for a nonexistent path, which is identical to bare vale's behavior.check-release-needed.shis unpassable because tagging the merge commit requires the blocked push. Tagging is a purely local operation that precedes the push: tagging local HEAD then running the gate as if pushing tomain→ exit 0. The gate is also a no-op on this branch (noPRE_COMMIT_REMOTE_BRANCH→ exit 0), and the script's own header (lines 12-16) already documents the Gitea merge-button hole.vale-wrap.sh:78) — reachable only with zero arguments, which is a broken invocation on any bash.python3as an undocumented dependency — pre-existing repo-wide onmain(agentsmd-audit,provider-adapter-author, both audit skills'validate.sh), and both audit skills already name "python3 unavailable" as a fallback condition.Verified as genuinely fixed from rounds 1-3
vale-run/SKILL.md:22,references/troubleshooting.md:68,ADR-0013:55-56all correct now (one stale copy remains — finding 4).warning— all 9 rule files across both copies arelevel: error; a deliberately-brokenSKILL.mdreturns exit 1 with all 4 rules firing.--configcwd resolution — works from/, from nested subdirectories via../../../../, in both--config Xand--config=Xforms, and with spaces in paths.>-only narrowing is safe: bare Vale genuinely does match|literal scalars, so test 11'swrapped == bareassertion is non-vacuous (baseline confirmed to be a real 3-alert report, not empty).skill-size-checkboundary —MAX_LINES=500with> MAX_LINES; tests assert both the exactly-500 pass and the 501 fail, fixture line counts self-verified.check-vale-style-sync.shexits 0; tests cover differing wrapper, differing rule content, and a rule present in only one copy. Bothvale-wrap.shcopies are byte-identical (blob918f32e).Checked clean
skill-size-check.sh(unterminated-final-line counting, inclusive boundaries, exit propagation) ·check-release-needed.shparsing (RELEASE_PATHSderivation,dirnamecapturing the siblingstyles/tree,git describe --matchexcluding non-release tags, fail-closed on shallow clone) · all 5 rules firing on a purpose-built bad fixture with correct line/column/rule-ID ·KyberforgeCopilot.ProactivePhrasefiring on.agent.mdand correctly not on plainagents/*.md, per ADR-0013 ·.vale.iniStylesPathrelative resolution and section precedence ·.pre-commit-config.yamlfiles:regexes anchored and non-/-crossing,check-hooks-applyandcheck-useless-excludespass · wrapper scratch-tree handling (mktemp -d+trap, no leaks across ~20 invocations,..-escape guard,< /dev/nullstdin guard, block-scalar parser across>-/>+/blank lines/indented keys) · executable bits on all new scripts · manifests,claude plugin validate --strict, marketplace parity across both files, version bumps per ADR-0006 ·lint-runneragent pair (CC/Copilot differ only intools:, report-only) · new tests exercise both pass and fail paths with self-verifying boundary fixtures · governance/secrets: no credentials, tokens, connection strings or generated cryptographic material; no copyleft fragments ·shellcheck --severity=warningclean.🤖 Two-agent review (adversarial pass + independent validation) via Claude Code
Round 4 resolved —
acd2f1d→cc5f366All seven findings from the round-4 report are addressed across five commits. Fixing the Critical finding introduced a regression elsewhere, which is called out below rather than folded in silently.
8c570e9realpath -mis GNU-only8c570e98c570e9e9234f6CONTEXT.mdglob claimse9234f68c570e9714e8a0348dd9f1. Critical — the external-consumer contract now works
vale-wrap.shself-locates its config from${BASH_SOURCE[0]}when no--configis supplied;--configis dropped from both manifests. An explicit--configstill wins in all three argv forms and stays cwd-relative, so both audit skills' Step 1 is unaffected.Same consumer repo, before and after:
Still gates correctly there — injecting
helps with/utilizeinto a folded description yields exit 1 with twoKyberforge.VagueWordingalerts..pre-commit-config.yamlalso drops the argument, deliberately. Keeping the twoentry:lines byte-identical is the actual root-cause fix: the localrepo: localhook resolved its--configcorrectly only because the consuming repo was this repo, sopre-commit run --all-filesexercised a path no external consumer takes. That divergence is the whole reason this survived three review rounds. Making them identical means the local run now reproduces what consumers get.New
tests/test-vale-hooks-consumer.shcovers the manifest as a hook repo for the first time. It builds the hook repo from the working tree and commits it, then points a throwaway consumer atfile://…— so it tests uncommitted changes, which a bare clone of HEAD could not.2. High —
realpath -mreplaced-mis load-bearing (destdoes not exist yet;mkdir -pcomes later), so a BSDrealpathaborts underset -e.tmpdirnow usescd "$(mktemp -d)" && pwd -P;destuses anabspath()helper backed bypython3 -c 'os.path.abspath', already a hard dependency. Thecase "$dest" in "$tmpdir"/*)escape guard still refuses a 40-deep../climb with exit 2.The regression test shadows
realpathwith a stub that rejects-mand mimics BSD'sillegal option -- m, so macOS is simulated rather than assumed.3. Medium — directory arguments are walked
Classifier widened to
( -f "$arg" || -d "$arg" )— deliberately not-e, so fifos and devices keep the old pass-through. A directory is mirrored whole into the scratch tree (NUL-delimitedfind/read, so spaces and newlines survive;.gitpruned at any depth), then every*.mdin the copy is flattened. Mirroring the whole tree rather than a filtered list is intentional: vale applies its own format filtering, so any file dropped here would be silently unlinted — the same defect class as the finding itself.A non-existent path still falls through untouched, matching bare vale, per the wrapper's drop-in contract.
One hardening this made necessary: the flattener now uses
encoding='utf-8', errors='surrogateescape'. Without it, a single non-UTF-8.mdanywhere under a directory argument would abort the hook with a Python traceback — a failure mode the walk itself would have introduced.4–5. Docs corrections
cli-reference.md:29now states that onlyerror-level alerts make Vale exit non-zero and thatMinAlertLevelfilters display in both directions; the--no-exitand--minAlertLevelrows were corrected too, as was the same misconception in the researchtroubleshooting.md. The rest ofdocs/research/docs/vale/was swept —configuration.md:34andexamples.md:23are accurate and untouched.CONTEXT.md's false claim is confirmed as a stale leftover: atcbc33d9the root.vale.inigenuinely did use path-scoped globs ([plugins/*/skills/*/SKILL.md]), and149d564changed them to[**/SKILL.md]without updating the prose. The NOT-RUN0 filesguard is unchanged — it is correct, and only the rationale explaining when Vale reports 0 files was wrong. The paragraph now also distinguishes the localfiles:regex from the layout-agnostic one shipped to consumers.LESSONS.md:143was checked and deliberately left alone: its near-identical wording is accurate history of thecbc33d9design, not a live claim.ADR-0014 was corrected in place rather than appended to. It was introduced by
1164f3aon this same unmerged branch with no tags cut, so there is no shipped decision history to preserve; an amendment note would record a draft state no reader ever saw. It now records theentry[0]-only prefixing constraint as the reason the self-locating design is required, and forbids either manifest'sentry:from growing a repo-internal path argument.6–7. Test harness and sync check
run-tests.shtreats exit 77 as a distinct SKIPPED outcome, surfaced in the summary and listed by name — a skipped suite can no longer read as a pass. Verified nothing parses the old output format first.check-vale-style-sync.sh's||became&&, so exactly one copy missing now exits 1 naming the missing side; the both-missing no-op and its test are preserved.Regression introduced by fix 1, and closed
Dropping
--configfrom the manifest leftcheck-release-needed.shderiving release-relevant paths from a token that no longer exists, silently removing bothassets/vale/trees from coverage:A Vale rule change could then land on
mainwithout demanding a release tag, leaving consumers pinned to an olderrev:with stale rules — precisely the drift the gate exists to prevent. Coverage now derives fromtokens[0](double-dirnamefor the..normalization, norealpath -mreintroduced), guarded on the tree existing and on the bundle root not resolving to.soskill-size-check.shcannot invent a bogus path. Demonstrated on a scratch repo with a styles-only commit afterv1.0.0:exit=0before,exit=1with the offending file named after.The
--configparsing branch was removed rather than kept as harmless dead code. It is not merely unused but structurally unusable: since pre-commit rewrites onlyentry[0], no argument in any entry can ever reference a file this repo ships. The manifest'sentry:contract is now "bare script path only," recorded in ADR-0014.Accepted gap: deleting a hook's entire
assets/tree is not flagged, because the candidate path stops existing and never enters the pathspec. Deletions within a surviving tree are flagged and tested. Closing the whole-tree case needs the last-tag tree consulted rather than the worktree.Non-vacuity
Every new test was run against the old code and shown to fail:
Case 11 of the release-gate suite is vacuous against the old script but caught a real bug in the first iteration of the new one — the missing
bundle_root != "."guard, which would have made any future top-levelassets/falsely demand a release tag.Gates
Versions: kyberforge
1.2.5→1.2.6(shippedvale-wrap.shchanged), lint1.1.3→1.1.4(a consumer cached at 1.1.3 holds docs that lead to building a gate which passes everything). Marketplace entries carry no per-plugin version, so neither manifest changed — matching the precedent inaff5b6c.🤖 Fixes applied by parallel subagents, verified independently, via Claude Code
Under set -u, "${arr[@]}" on an empty array aborts on bash before 4.4, which is what macOS ships as /bin/bash. Three expansion sites now use ${arr[@]+"${arr[@]}"} consistently. The hazard is not currently reachable: verified on a bash 3.2.57 built from source that all seven invocation shapes succeed against the previous code, including zero args, flags-only and an empty directory. vale_args is provably non-empty at every site because the default --config branch always appends first. The guard is kept because that invariant is non-local and untested, so an edit to the default-config branch would reintroduce a macOS-only crash silently. Test fidelity is deliberately mixed. Case 16 is static and is the only one that fails against the previous code, since no bash 5 host can reproduce the abort at runtime. Case 17 runs the emptiest invocations under the oldest bash it can find and names that shell in its output so it cannot overclaim. Case 18 guards against the tempting wrong fix of dropping the quotes, which also silences the abort but word-splits a path containing a space. No other bash 4.x construct is present; swept for mapfile, declare -A, case modification, negative indices, globstar, wait -n and namerefs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCQ648fLSFXPHGZdQ8gn58Accepted gaps closed —
cc5f366→afc2b7fThe two gaps logged as accepted in the round-4 resolution are now fixed, plus the round-4 lessons are recorded. One of these did not turn out the way the original finding described it, so that is corrected below rather than quietly shipped.
14c2c9116c038bafc2b7f1. Release gate — whole-tree deletions now flagged
Coverage was derived from the worktree alone, so the
-dguard on a hook's bundledassets/tree meant deleting the entire tree removed it from the pathspec rather than flagging it. The gate stayed silent about a change that breaks every external consumer at the nextrev:.The path set is now derived twice — from the worktree manifest and from the manifest at
$LAST_TAG— and unioned. Union rather than intersection, deliberately: a path the tag exposed but HEAD no longer does is a removal that pinned consumers must be told about, while a path only HEAD exposes is new contract surface they cannot reach without a new tag. Both directions need flagging, and the union cannot over-fire on its own, since any manifest edit that makes the two disagree already changes.pre-commit-hooks.yaml, which is itself in the path set.A single
collect_release_paths <scope>helper serves both derivations —worktreeprobes with-d, a rev probes withgit cat-file -e "$rev:$bundle_root/assets"— so there is one parsing path, not two that can drift. Both existing guards still apply on both sides:scripts/skill-size-check.shcontributes noassetspath, and a future top-levelassets/still cannot falsely demand a release.Fail-closed behavior is disambiguated: an unreadable tagged tree (shallow clone) hard-fails with the existing
git fetch --unshallowpointer, while a readable root tree with no manifest is treated as "manifest added since the tag."tokens[0]was investigated as the suspected twin of this bug, and is not one. It is added unconditionally and never carried an existence guard, so both deletion cases — script deleted with the entry surviving, and entry plus script removed together — already exited non-zero. What was wrong was the reporting: a fully retired hook could no longer be named in the failure message, understating the breakage. The tagged manifest fixes that, and a test now asserts it.tests/test-check-release-needed.sh: 11 → 14 cases.Case 13 passes against both old and new code and is labelled in-file as a characterization test, not a fix — it pins the fact that
tokens[0]carries no existence guard, so a future refactor of the union cannot silently add one and reopen the holeassets/had.2. bash 3.2 — the original finding was wrong about reachability
The round-4 report filed this as an unreachable nit. That was right to be skeptical, but the reasoning behind it was incomplete, and the honest result is worth recording.
The hazard was verified unreachable on a real bash 3.2.57 built from source, not argued from documentation. All seven invocation shapes succeed against
cc5f366: zero args, flags-only, unknown-flag-only, file argument, directory argument, empty directory, and a directory containing no.mdfiles. The two shapes I specifically expected to be reachable — flags-only, and a directory with no matching files — do not trip it.The reason is a non-local invariant:
vale_argsis provably non-empty at every expansion site, because eitherconfig_givenis false and the default--configis appended, orconfig_givenwas set by one of three branches that each append first.Three sites are guarded anyway with a single consistent idiom (
${arr[@]+"${arr[@]}"}), verified byte-faithful on both 3.2 and 5.2 for empty arrays, embedded spaces, empty-string elements, glob characters, tabs, and a hostileIFS=:. The reasoning for keeping a fix with no live failure behind it: the invariant that makes the current code safe is non-local and untested, so an edit to the default-config branch would reintroduce a macOS-only crash with nothing to catch it.bash 3.2 is genuinely achievable for this file — no blocker. Confirmed on the 3.2 binary that
${#arr[@]}on an empty array,${arg#"$cwd"/}nested-quote pattern removal,read -r -d '', process substitution,+=append, andBASH_SOURCEall work. A full bash-4.x sweep found nothing: nomapfile/readarray,declare -A,${var,,}/${var^^}, negative indices, globstar,wait -n, namerefs,coproc,printf -v,${!...}, orPIPESTATUS.Test fidelity, stated plainly rather than rounded up.
tests/test-vale-wrap.sh: 19 → 22 cases.cc5f366(it names all three sites). Static by design: no bash 5 host can reproduce the abort at runtime, so absence of the construct is the only assertable property.bash 5.2. It cannot fail againstcc5f366even on real 3.2 — the honest consequence of the hazard being unreachable. It proves the guarded form does not break anything, not that it fixes a live crash.Real bash 3.2 coverage requires pointing
VALE_WRAP_TEST_BASHat a 3.2 binary. macOS coverage is not claimed — a Linux-built 3.2.57 is the right shell version, not the right platform.3. Lessons recorded
Two entries in
LESSONS.md, in the format established by864e7c6:repo: localcollapses the clone prefix, cwd, and repo root into one directory, so a byte-identicalentry:string worked locally for a reason that exists only locally. The local run was not weaker evidence of the same thing — it was evidence of a different thing, and the two were indistinguishable by reading either file.Checked against the adjacent existing entries and kept separate with reasons: "A clean linter result can mean 'nothing was checked'" covers a check that never ran, whereas here the local hook ran and checked a different resolution path; "One signal, two consumers" covers a live signal diverging, not a token's removal killing an undocumented parser. Worth noting the first of those already carries a
[graduated → core/instructions/testing.md]marker and took a 4th instance on 2026-08-09 — it and the new local-mode entry are one instance away from being the same cluster.Gates
kyberforge
1.2.6→1.2.7for the shipped wrapper change;marketplace.jsonuntouched, as entries carry no per-plugin version.Remaining, unchanged
Still open by design, all pre-existing: a hook renamed between tag and HEAD is over-covered rather than tracked as one rename (harmless — the manifest edit fires regardless); the tag-time
assetsprobe is tree-existence, not recursive, so a treeless partial clone could false-negative; theassetsbundle-directory name is a hardcoded convention; and the gate only fires on a localgit push, so a server-side merge still bypasses it.That last one matters for landing this PR.
git tag -lis empty, and merging through Gitea's button does not run the pre-push hook — so the gate will not fire, the merge will succeed, and.pre-commit-hooks.yamlwill expose hooks to external consumers with norev:anyone can pin. Cuttingv1.0.0onmainafter the merge needs to be a deliberate step; nothing will prompt for it.🤖 Fixes applied by parallel subagents, verified independently, via Claude Code
Round 5 — review, independent validation, and fixes
Ran a full review of this branch, then a second independent pass that re-derived every empirical claim rather than trusting the first. All findings confirmed (no false positives), three regraded. Everything is now fixed in
aa8cc22,4ae2429,57654c4.The root cause was one rule, not four bugs
Vale locates a frontmatter description by matching the parsed YAML value back against the source text. Any scalar whose value isn't spelled out verbatim loses the
text.frontmatter.descriptionscope entirely. That single rule explains every symptom:|literal>foldedThe wrapper only flattened
>. The other three broken forms passed silently with exit 0 — indistinguishable from a genuine clean pass, in a prefilter whose callers are told not to re-derive its verdict by judgment. No live instances in this repo (corpus is 32 folded / 19 single-line), so this was latent exposure via the shipped.pre-commit-hooks.yamlcontract, not a regression.This also corrected a factual error in three shipped docs:
|literal blocks were documented as broken. They are not — line breaks survive, so the value stays verbatim-matchable. An agent following the old text would have rewritten a working|description into a plain multi-line scalar, which genuinely does break. The remediation was inverted.Fixed
Wrapper (
aa8cc22) — flattening covers all four broken forms via one shared continuation generator;|still untouched. The flattened value is now emitted in whichever scalar form needs no escape at all, because any escape re-breaks the verbatim match. That retires the blanket'→ U+2019 substitution, which had made apostrophe-bearing rule tokens unmatchable across 63% of the corpus. Note the obvious fix here does not work:''doubling kills the scope outright — measured, not assumed. Also fixed in the same pass: continuations now terminate at a line flush with the key (adescription:followed by a flush-left line previously swallowed the rest of the frontmatter); vale's value-taking flags are routed explicitly instead of targets being inferred by file existence (--output tmpl.tmplwas being linted as a target and reordering argv into a hard E100); relative--output/--pathare absolutized like--configalready was, since the runcds into the scratch mirror; symlinks are followed when walking a directory argument; and a nonexistent path now fails loudly instead of inheriting bare vale's fallback to stdin, which rendered a typo'd path as0 errors … in stdin, exit 0 — a string the callers'0 filesNOT-RUN guard cannot match.Audit/hook alignment (
4ae2429) —validate.shnow enforces the same 500-line and 2900-word pair asskill-size-check.sh, so a skill can no longer pass its own audit and then be rejected by the commit hook. Both audit skills' Step 1 drops the redundant--config: the wrapper self-locates its sibling config, and an agent resolving the script path but not the config path got E100/exit 2, which the fallback clause misread as "vale unavailable" and silently downgraded to full LLM judgment. The external-consumer test now exercises all three shipped hooks — verified by mutation,chmod 644on the copied script turns three passes into two failures.Docs (
57654c4) — the|correction above; both size ceilings documented in CONTEXT.md and ADR-0013; ADR-0014's "accepted residual" (wholesaleassets/deletion unflagged) marked closed by14c2c91with an amendment following ADR-0005's precedent rather than a silent rewrite; thevale syncself-contradiction invale-config/SKILL.mdscoped to package styles; and AGENTS.md repointed at the sevengitea:*skills, replacing a deadbin:gitearoute.Tests
tests/test-vale-wrap.shgoes 22 → 33 assertions. The new form-matrix case asserts each multi-line spelling reports exactly what its single-line spelling reports, each guarded by a check that bare vale reports nothing — so a silently-skipped flattening cannot pass vacuously. The pre-existing|test was strengthened the same way; it previously compared wrapper output to bare vale output and would have passed with both empty. New coverage also for the apostrophe token, the lossy-fallback liveness case, symlinks, separated flag values, and the typo'd path.The bash-3.2 static check now scans
skill-size-check.shandcheck-release-needed.shtoo. It flagged two lines in the latter; on inspection they are not hazards (RELEASE_PATHSis seeded non-empty at declaration and never reset, and the 3.2 abort only fires on an empty array), so rather than a name allowlist that goes stale silently, the exemption is encoded structurally.Full suite green: 12 suites, 125 bats, 0 failures. All six pre-push gates pass. Live corpus clean — 39
SKILL.mdand 6 agent files, 0 alerts — so the widened flattening surfaces nothing new in real files.Two open items, neither blocking
MAX_WORDS=2900is calibrated to the corpus median with zero margin. Measured density is min 5.97 / median 6.79 / max 7.22 chars per word; at the densest observed ratio the gate permits ~5,240 tokens, above the 5,000 it proxies for. Holding the worst case under 5,000 needs ≈2770, which blocks nothing today (largest skill is 2,489 words). Left unchanged — tightening a blocking commit gate is a judgment call. The comment is now honest that the guarantee holds for typical prose density, not any file.^plugins/[^/]+/skills/[^/]+/SKILL\.md$. Caught during this round when an edit pushed CONTEXT.md to 2,905 words with nothing to flag it; trimmed to 2,816 by hand.One residual is accepted by design: the lossy U+2019 fallback survives for the single combination no YAML scalar can carry verbatim (needs quoting and contains
'and contains"or\). It's documented at the emission site and covered by a test asserting the scope stays alive there.🤖 Generated with Claude Code
MAX_WORDS=2900 was calibrated to the corpus median density and carried no margin: at the densest observed 7.22 chars/word (~1.81 tokens/word) it permits ~5,240 tokens against the 5,000 it proxies for. 2770 holds the worst observed density under the ceiling. The largest SKILL.md is 2,489 words, so the change costs nothing today — 281 words of margin — and the header comment now argues the new calibration rather than swapping the digits. Both enforcement points move together, and a new test asserts they agree, since a SKILL.md passing its own audit while the commit hook blocks it is the disagreement this pair exists to prevent. CONTEXT.md is deliberately left ungated: it is 2,816 words, and gating it would block the build. Recorded here so the omission reads as a decision rather than an oversight. skill-audit's manual-fallback path listed only the line ceiling, so an agent taking that path passed an oversized SKILL.md the hook then rejected. The word ceiling is now named alongside it. agent-audit is deliberately unchanged: the size hook scopes to SKILL.md only and agent-audit's validate.sh has no word gate, so claiming it there would be false. The Vale research doc still showed the MDX {/* vale off */} form under a Markdown heading, contradicting CONTEXT.md and vale-run's troubleshooting reference — that form suppresses nothing in plain .md. Fixed in both places it appeared. tests/run-tests.sh used mapfile (bash 4.0+) with unguarded array expansion, though AGENTS.md tells contributors to run it and macOS ships bash 3.2. It now collects via a while-read loop over process substitution and guards every expansion. The newline-delimited find|sort pipeline is kept rather than -print0 with sort -z, whose BSD portability is the weaker link, and which matches mapfile -t's previous behaviour exactly. Refs: #85 ADR: 0013Round 6 — review findings addressed (
57654c4..9a3f72b)A full review of the branch (commit history + final state) produced 12 findings; each was then cross-checked against this thread before any code was written. Two did not survive that check and are recorded here rather than acted on:
main, and that tagging pre-merge is insufficient because "all paths are new relative to that tag". #1344's validator pass already rejected this, and I reproduced the rejection:git tag v1.0.0at HEAD, then run the gate as if pushing tomain→ exit 0. Re-reporting it would have been noise. The genuine residues (no tags today; Gitea's merge button never runs the pre-push hook) remain documented atcheck-release-needed.sh:12-16, ADR-0014, and #1350.Fixed
>=2) across both hooks, and the SKILL.md fixture alone raises two — so one working hook satisfied it. Retargeting agent-audit's glob to match nothing left the suite reporting3 passedunder the message "both hooks flatten and flag". Nothing read either.vale.ini— that was the enabling half. Alerts are now attributed per hook by path, with distinct trigger tokens per fixture, and the sync check probes each glob section by asking Vale to lint a representative path.emit()no longer rewrites'→U+2019. A `PRE_COMMIT_TO_REF; multi-tokenentry:values now fail loudly instead of dropping a hook's whole surface from the gate.1.2.7→1.2.8, lint1.1.4→1.1.5.{/* vale off */}claim corrected;run-tests.shmade bash-3.2 safe; case-19's vacuous-pass guard closed.Ceiling tightened
2900 → 2770(open item 1 from #1355) in both enforcement points, now pinned by a parity assertion so they cannot drift. Largest SKILL.md is 2,489 words — 281 to spare.CONTEXT.mddeliberately left ungated (open item 2). At 2,816 words it sits above the new ceiling, so gating it would block the build. Recorded as a decision, not an oversight.Two things worth flagging
The gate rejected its own push. After all six commits,
git pushfailed:test-check-release-needed.shpassed standalone and in a full-suite run, but failed 13/20 under the pre-push hook.run_checkset thePRE_COMMIT_*vars it needed but never cleared what was already in the environment — under pre-push, pre-commit exports refs of the real repo, the fixtures inherited them, and the script resolved a rev that does not exist in the fixture. The suite passed in every context except the only one that matters. Same shape as the--configregression: the local invocation exercised a different thing than the shipped one. Fixed in9a3f72b; found by the hook, not by any test.Reverse mutation sweeps are now standing practice. Beyond "break the artifact, confirm the suite fails", each new assertion was neutered in turn to confirm exactly one case fails. That exposed two assertions in
check-vale-style-sync.shbound to no failing case at all, one masked by a stronger check running first — and one release-gate test that passed with its guard removed, because a different guard produced a similar message.Both patterns recorded in
LESSONS.md(the aggregate-assertion one as the 5th instance of the "a clean result can mean nothing ran" family).Full suite: 12 passed, 0 failed;
check-manifests.shandcheck-vale-style-sync.shclean; wrapper copies byte-identical.🤖 Generated with Claude Code
Pre-merge review (round 4)
Ran four parallel review passes against the branch (verification gate, correctness/simplification/efficiency code review, security review, and a dogfooded
skill-audit/agent-auditpass on the new/modified skill and agent files), spot-checked the higher-severity claims by hand, then fixed and pushed the confirmed findings.Clean:
skill-size-checkcheck-manifests.shandclaude plugin validate --strictclean on all 4 pluginsvale-config,vale-run, and thelint-runneragent pair: clean auditsFixed in
0a41b2c,050aec4,6910f1b:fix(kyberforge)—skill-audit/SKILL.mdhad lost the manual "action-verb opening" fallback check thatagent-audit/SKILL.mdstill has. Vale'sDescriptionOpenerrule only matches the literalThis skill...pattern, so other non-imperative openers were going unflagged.docs(kyberforge)—skill-audit/README.mdandagent-audit/README.mdfile tables predated this PR's Vale wiring and never listedscripts/vale-wrap.shor theassets/vale/style tree.fix(lint)—scripts/check-release-needed.sh'sgit describe --match 'v[0-9]*.[0-9]*.[0-9]*'is a shell glob, not a regex, so a tag likev1.2.3-checkpointalso satisfied it and could get picked as the release baseline instead of the true last release. Added--exclude '*-*'plus a regression test (test-check-release-needed.shtest 21) that fails against the old script and passes against the fix.Reviewed and deliberately not changed (findings from the code-review pass that didn't hold up on inspection):
check-vale-style-sync.sh's regex-union approach for scope-coverage checking — the two pre-commit manifests'files:regexes differ by design (this repo's own layout vs. the generic layout shipped to external consumers), so unioning them is correct, not a masking bug.run-tests.shtreating exit 77 as SKIPPED rather than FAILED — working as documented (automake convention), and the summary lists skipped scripts by name rather than hiding them.Follow-up round: efficiency/reuse cleanups
Fixed the remaining low-severity findings from the code-review pass, in
680aa4fande62f68a:refactor(kyberforge)—vale-wrap.sh's separated (--config X) and joined (--config=X) argument branches duplicated ~20 lines of path-absolutization logic; extracted intoabs_config_value(). Also collapsed twopython3subprocess spawns into one for the common single-file case (flatten()now optionally doesabspath+ the flatten in the same process). Editedagent-audit's canonical copy, then regeneratedskill-audit's copy viasync-vale-styles.sh— never hand-edited the second copy, to guarantee byte parity. No hardening (bash 3.2 compat, surrogateescape, symlink guards) touched.test-vale-wrap.sh39/39.refactor(lint)—check-vale-style-sync.sh'shook_file_regexes()reparsed both pre-commit manifests on every call; the validation loop calls it 3 times (agent-audit twice, for its two file shapes), so agent-audit's regex set was being parsed twice for nothing. Now cached per skill in a lazily-populated associative array.test-check-vale-style-sync.sh20/20.refactor(lint)—skill-size-check.shread the target file twice (separateawk/wc -wcalls) for line/word counts; now oneawkpass. Also documented, next to the constants, why they're duplicated againstskill-audit/scripts/validate.sh's Python implementation rather than unified (cross-language/cross-context tradeoff, same pattern asvale-wrap.sh, guarded by a drift test).test-skill-size-check.sh9/9.Investigated, no change made: the reported "missing
source_keys" inagent-audit/references/README.mddidn't hold up — it already hassource_keys: [], matching an established, repeated convention in this plugin for pure-index reference files with no independent research content (e.g.agent-author/references/README.mdandagent-author/references/scripts.mdfollow the same pattern). Populating it with unrelated slugs would have misattributed provenance. Leaving as-is.Full suite (12/12) and all pre-commit/pre-push gates green after both pushes. Branch is clean and up to date. Recommend merging.
check-vale-style-sync.sh used `declare -A` for a per-skill regex cache. Associative arrays are bash 4.0+; this script runs as an always-run pre-push hook with `language: system`, so it inherits whatever bash is first on the invoking user's PATH. On macOS's stock bash 3.2, `declare -A` at top level aborts immediately under `set -euo pipefail` — every push would hard-fail before the sync check ran anything. Replaced with two parallel indexed arrays (HOOK_REGEX_CACHE_KEYS/_VALS), linear-scanned by index — same caching behavior (avoids re-parsing both pre-commit manifests when agent-audit is probed twice), but only ever uses ${#arr[@]} and index access, never a bare ${arr[@]} expansion. Extended test-vale-wrap.sh's existing bash-3.2 hazard sweep to scan this file too, and added a check for `declare -A` itself — it previously only caught unguarded ${arr[@]} expansions and mapfile/readarray, so this exact regression had no test that would have caught it. Refs: #85Independent review + fixes —
389a4f0→76e0df6Ran three parallel review passes against the branch as it stood at
e62f68a(verification of round-2's blocking items/gaps against the current tree, a fresh scripts/tests review, and a fresh docs/config/manifests review), then validated every finding against the full commit history and comment thread by hand before touching anything — a couple of the raw findings turned out to be wrong on inspection and were dropped rather than acted on.Confirmed still fixed (round 2, re-verified by execution, not by reading the diff): the
json.dumpsescaping regression, the caveman/write-docs/tdd/etc. meaning-changing prose edits, the Step 1 cwd mismatch, andVagueQualifier's false-positive rate are all genuinely resolved. This branch has actually been through 6 review rounds, not the 3 the PR body's history table lists — that table is stale but it's a documentation nit, not a code issue.Two real, previously-uncaught issues fixed here:
389a4f0—check-vale-style-sync.shuseddeclare -A(bash 4.0+ associative arrays) for its per-skill regex cache. This script runs as an always-runpre-pushhook withlanguage: system, inheriting whateverbashis first on PATH — on macOS's stock bash 3.2 this aborts immediately underset -euo pipefail, hard-failing every push before the sync check does anything. Introduced by the PR's own most recent commit (e62f68a's caching refactor), after this repo's round-4 bash-3.2 sweep (#1350) had already verified the other three scripts clean — this one was never in that scanned list. Fixed with two parallel indexed arrays instead (same caching behavior, only${#arr[@]}/index access, no bare${arr[@]}expansion). Also extendedtest-vale-wrap.sh's hazard sweep to scan this file and to check fordeclare -Aitself, since the existing sweep only caught unguarded${arr[@]}andmapfile/readarray— this exact regression had no test that would have caught it.76e0df6—plugins/gitea/plugin.json/.claude-plugin/plugin.jsonwere never version-bumped despite shipped content changes togitea-issues,gitea-prs, andgitea-releasesSKILL.md back in round 1 (3324a73). The identical bug class was independently caught and fixed four separate times elsewhere in this PR for bin/kyberforge/lint — gitea slipped through all four passes. Bumped1.3.2→1.3.3; marketplace.json untouched per the established convention (no per-plugin version there).Investigated and NOT changed (raised by the review passes, didn't hold up):
"very different"→"fundamentally different"(prototype/SKILL.md) and"There is no auto-pagination"→"The MCP layer does no auto-pagination"/"completely"→"every page"(gitea-releases/SKILL.md) edits looked untraceable to any shipped rule at first pass. They aren't — both are genuineVagueQualifier/SentenceOpenerThereIshits from before that rule was dropped. The"very different"one is even named explicitly in149d564's commit message ("one marginal") as a deliberate keep, not an oversight.files:regex only matchingagents/*.md(missing bare*.agent.mdelsewhere, e.g.assets/templates/copilot.agent.md) — consistent with round 1's explicit precedent of excluding template files from the local dev hook. Likely intentional, not a gap.Gates:
tests/run-tests.sh12/12 (39/39 vale-wrap cases),check-manifests.shclean,claude plugin validate --strictclean on all 6 plugins, full pre-push suite green on both commits.🤖 Review + fixes via Claude Code
Follow-up —
cf5de2b, plus PR description correctedcf5de2b— the third finding from my last pass, previously left as low-severity/optional:check-vale-style-sync.shsilently exited 0 when given a nonexistentREPO_ROOTargument, falling through to the "neither copy present" no-op guard rather than erroring. That guard exists for a repo legitimately missing the kyberforge plugin, not for a typo'd or stale path — same "clean result can mean nothing was checked" shape as several earlier findings in this thread. Now fails loudly with the bad path named in the message. Only the documented manual-invocation mode was affected; the shipped pre-push hook always calls with zero args, which resolves viagit rev-parse --show-topleveland was never at risk. Added a regression test (test-check-vale-style-sync.sh, now 21 cases).PR description — updated the "Review history" section, which still said "Three rounds, all resolved" against what's actually 7 rounds of comments in this thread. Also corrected the "Deferred and tracked in ADR-0013: styles portability" line — that item was resolved back in round 4 (
1164f3a) and formalized in ADR-0014, which explicitly supersedes ADR-0013's deferred-portability consequence; the PR body just hadn't caught up.Gates:
tests/run-tests.sh12/12 (check-vale-style-sync.shsub-suite 21/21),check-manifests.shclean, full pre-push suite green oncf5de2b.🤖 via Claude Code