Files
holocron/docs/adr/0014-vale-prefilter-ships-from-the-plugin.md
Defame1297 1f3d4f9962 docs: correct stale resolver, status and duplication claims
ADR-0014 gains a dated correction: skill-size-check now sources the
boundary resolver from kyberforge (ef27c97), so restoring the external
hook contract needs it made self-contained first. ADR-0017's status
reflects its supersession, architecture.md and gates.md carry the
current duplication counts and reason, gates.md defines vacuous green
inline, and the gitleaks lesson is marked historical.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-16 15:27:29 +00:00

23 KiB
Raw Blame History

Kyberforge's Vale prefilter ships from the plugin, with .pre-commit-hooks.yaml for external git-hook/CI enforcement

Resolves: ADR-0013's deferred "styles-portability" consequence — .vale.ini/styles/ moving out of the repo root was deliberately deferred there, not fixed. ADR-0013's other content (rule scope, level: error model, SentenceOpenerThereIs/VagueQualifier trial outcomes) is unaffected and remains in force.

Amended by ADR-0025 (2026-09-15). The reasoning below is not reversed; its precondition is gone. The two skill-scoped Vale copies this ADR mandates — agent-audit/assets/vale/ (canonical) and skill-audit/assets/vale/ (subset) — existed because the no-cross-skill-sharing rule made it impossible for one audit skill to read the other's config. ADR-0025 merges the two skills into factory-audit, so there is no boundary left to duplicate across: there is now one copy, at plugins/kyberforge/.apm/skills/factory-audit/assets/vale/, carrying both styles and the single-file .vale.ini — [**/SKILL.md], [**/agents/*.md], [**/*.agent.md] — that this ADR's "One hook per file-scope" section had split in two. scripts/check-vale-style-sync.sh, decided on below and wired at pre-push, is deleted with the copy it diffed. Nothing it asserted about the config was lost. Its six-row glob-coverage probe table is now tests/test-vale-wrap.sh cases 28-30, run against the merged config. Case 31 carries across the per-rule override allowlist, and case 0 carries across the "config loads" guards. Its cross-manifest files: drift check is ported as case 33. The original keyed each hook's record on entry:, which stopped working once both vale hooks shared one entry, so the port pairs the hooks by id: instead. Of the script's 17 assertion sites, 6 compared the two copies and are moot, 10 are rehomed and 1 is ported. ADR-0025 gives the per-assertion mapping; read the "six" here as probe rows, not as a share of those 17. What does not change: the two exported hook IDs, kyberforge-vale-audit-skill and kyberforge-vale-audit-agent, keep their IDs and their files: regexes — external consumers pin them by name — and the argument-free entry: contract is untouched. Read the two-copy table, the sync-check paragraph, and the tests/test-vale-wrap.sh Consequences bullet below as the state this ADR established, not as current layout.

Amended (2026-09-16): the .pre-commit-hooks.yaml export and its release tags are retired. The runtime half of this ADR — Vale config, styles and wrapper bundled inside the skill (now factory-audit), self-located from ${BASH_SOURCE[0]} — stands. The external git-hook/CI half does not: the manifest, check-release-needed and the tag-cutting consequence are gone. See the amendment at the end of this file before reading any paragraph above or below that names .pre-commit-hooks.yaml, a rev: tag, check-release-needed, case 33, or the two exported hook IDs as current. That includes the ADR-0025 amendment directly above: case 33 is deleted (its one-plugin narrowing guard is now a property of case 32), and no external consumer pins the exported hook IDs any more.

skill-audit/agent-audit's Step 1 called "$(git rev-parse --show-toplevel)/scripts/vale-wrap.sh" --config "$(git rev-parse --show-toplevel)/.vale.ini" — which resolves to whichever repo the skill happens to be running in. Inside ai-development that's this repo; in any external repo that installs kyberforge@holocron as a plugin, it's that repo's own root, which has no .vale.ini or vale-wrap.sh. The prefilter silently fell back to full LLM judgment every time outside this repo — the exact gap ADR-0013 named and deferred.

Decision

Runtime (a live Claude Code session): the Vale config, styles, and wrapper script move into the plugin itself, following the no-cross-skill-path rule already established in skill-author/references/deployment-modes.md (a plugin's cache-install only copies each skill's own files; there is no plugin-level shared directory). agent-audit needs both Kyberforge and KyberforgeCopilot (it lints .agent.md files), so plugins/kyberforge/.apm/skills/agent-audit/assets/vale/ is the canonical, superset copy. skill-audit needs a second, smaller copy (plugins/kyberforge/.apm/skills/skill-audit/assets/vale/, Kyberforge only) since it cannot reference agent-audit's copy across the skill boundary. Both skills' Step 1 now resolve scripts/vale-wrap.sh/assets/vale/.vale.ini relative to their own directory, the same way scripts/validate.sh <skill-dir> already does — no new resolution mechanism, just applying the existing one consistently.

git hooks / CI outside a Claude Code session have no plugin cache and no ${CLAUDE_PLUGIN_ROOT} — a CI runner in particular is guaranteed not to have one. The mechanism that works there for any consumer, with or without Claude Code installed, is pre-commit's own hook-repo protocol: this repo now ships a root-level .pre-commit-hooks.yaml exposing kyberforge-vale-audit-skill, kyberforge-vale-audit-agent, and kyberforge-skill-size-check. Any external repo adds repo: <this-repo-url>, rev: <tag> to its own .pre-commit-config.yaml and gets all three, fully decoupled from Claude Code. CI is the identical pre-commit run --all-files call, so the same manifest covers "possibly CI" from the original ask.

This repo's own dev-time gate consumes the same plugin-bundled copies instead of a third root-level copy — per explicit instruction, this repo should be set up like any other consumer would be, not dogfood a special root-only path. The existing repo: local hook is retargeted (not removed): entry: now points at plugins/kyberforge/.apm/skills/{skill-audit,agent-audit}/scripts/vale-wrap.sh. repo: local is kept rather than switching to a pinned self-reference (repo: <own-url>, rev: <tag>) — a pinned self-reference would lint working-tree edits against the last tagged release, not the change actually being made, which is wrong for the repo that is the source of the hook. This mirrors standard practice among hook-author repos (pre-commit's own pre-commit-hooks, shellcheck-py): repo: local for self-consumption, .pre-commit-hooks.yaml for everyone else, same underlying files and commands either way.

One hook per file-scope, not one combined hook. The old root .vale.ini had both the [**/SKILL.md] and [**/agents/*.md]/[**/*.agent.md] glob sections in a single file, so one pre-commit hook covered both. Splitting the config into two skill-scoped copies means a single hook entry pointed at only one copy would silently 0-file-skip the other file type. Both the local .pre-commit-config.yaml hooks and the external-facing .pre-commit-hooks.yaml therefore define separate -skill/-agent hook IDs, each with a files: regex matching exactly what its target copy's glob covers. (Confirmed empirically before deleting the root files: retargeting a single hook at agent-audit's copy silently scanned 0 SKILL.md files.)

The hook entry: is the wrapper alone; the wrapper self-locates its config. pre-commit prefixes only entry[0] with the hook-repo clone path (cmd = (prefix.path(cmd[0]), *cmd[1:])); every later argument is handed to the process untouched and so resolves against the consuming repo's root. A --config plugins/kyberforge/.apm/skills/…/assets/vale/.vale.ini in .pre-commit-hooks.yaml therefore named a path no consumer has, and every external run died with E100 [--config] Runtime error. The external-consumer contract this ADR exists to establish cannot be expressed as a --config argument at all — the config path has to be derived inside the process, from the script's own location. vale-wrap.sh accordingly defaults to its sibling assets/vale/.vale.ini, resolved from ${BASH_SOURCE[0]}, whenever no --config is supplied; an explicit --config from any other caller still wins and still resolves against the caller's cwd. Both audit skills' Step 1 passes no --config either, for the same reason and one more: a relative --config assets/vale/.vale.ini resolves against the cwd, not against the skill directory the wrapper path was resolved from, so it yields E100 Runtime error … does not exist and exit 2 — which both skills' fallback misreads as "vale unavailable" and silently downgrades to full LLM judgment, the exact failure the self-location exists to prevent. Both SKILL.md Step 1 sections say so explicitly ("Pass no --config"), and both manifests now carry the identical argument-free entry:. Keeping them identical is part of the decision: the local repo: local hook resolved its --config correctly only because the consuming repo was this repo, and that one difference is why three review rounds exercised a code path no external consumer ever takes.

Vale's StylesPath resolves relative to the .vale.ini file's own location, confirmed against docs.vale.sh/keys/stylespath — so a config path into the plugin finds that ini's sibling styles/ regardless of the caller's cwd, whether it arrives as an explicit --config or as the wrapper's self-located default. No extra path-juggling is needed beyond vale-wrap.sh's cwd-relative --config/path-argument handling and that fallback.

A sync-check catches drift between the two copies. scripts/check-vale-style-sync.sh diffs scripts/vale-wrap.sh and assets/vale/styles/Kyberforge/ between skill-audit and agent-audit (not .vale.ini — those legitimately differ, scoped to different glob sections), wired at pre-push alongside check-manifests. .vale.ini itself isn't diffed since divergence there is by design.

External .pre-commit-hooks.yaml consumers pin rev: to a tag, not a commit SHA. This repo had no tags before this change; going forward, a vX.Y.Z tag is cut whenever hook-relevant files change, matching how every other repo: entry in this repo's own .pre-commit-config.yaml already pins (v2.4.0, v8.21.2, ...).

Considered options

Keep a third root-level copy, dogfooded specially (rejected). Simpler in that this repo's own hook wouldn't need retargeting at all. Rejected on explicit instruction: this repo should consume the same portability path an external repo would, not carve out a special root-only case that never gets exercised the way external consumers exercise it.

Publish styles as a hosted Vale package via Packages = <zip-url> (deferred, not rejected). Vale supports fetching a style from a direct .zip URL via vale sync, fully decoupled from Claude Code and from pre-commit's hook-repo protocol — usable by any repo, even ones that never install kyberforge at all. This is a larger, separate investment (a release/versioning pipeline for the package itself) not required to satisfy the current ask; noted here so a future reader doesn't wonder if it was overlooked.

Consequences

  • Root .vale.ini, styles/, scripts/vale-wrap.sh are deleted. Two copies remain: plugins/kyberforge/.apm/skills/agent-audit/assets/vale/ (canonical, superset) and plugins/kyberforge/.apm/skills/skill-audit/assets/vale/ (subset, Kyberforge only).
  • plugins/kyberforge's plugin.json and .claude-plugin/plugin.json both patch-bump for every shipped content change (per ADR-0006's version-parity invariant): 1.2.5 for the relocation itself, 1.2.6 for the self-locating vale-wrap.sh that followed. Amended 2026-09-14 (ADR-0024): a record of what was done then, not current practice. Both manifests are deleted and apm.yml's version: is a plugin's only version field; ADR-0015 retired the parity/patch-bump rule this bullet invokes.
  • .pre-commit-hooks.yaml entries are a bare script path and nothing else — a constraint, not a house style, and it binds every future hook here, not just the Vale two. Since pre-commit rewrites only entry[0] into the hook-repo clone, no argument token in any entry can reference a file this repo ships: a relative path resolves against the consuming repo and hard-fails, and the absolute path is unknowable at author time. A hook that needs one of its own bundled files must have the script self-locate it from $0/${BASH_SOURCE[0]}, exactly as vale-wrap.sh now does for .vale.ini. Anything else rediscovers this as another E100. .pre-commit-config.yaml stays byte-identical to the shipped manifest on those entry: lines so the local gate keeps exercising the same resolution path a consumer does.
  • tests/test-vale-wrap.sh now exercises skill-audit's copy specifically — its fixtures are all SKILL.md-shaped, and only skill-audit's .vale.ini has the matching glob section. (State as of this ADR. Since ADR-0025 there is one vale-wrap.sh and one .vale.ini under factory-audit/, and that suite exercises all three glob sections of the merged config — see cases 28-30.)
  • The first vX.Y.Z tag is cut once this change and its tests pass, giving external .pre-commit-hooks.yaml consumers something to pin.
  • Cutting the tag is not left to memory. scripts/check-release-needed.sh, wired at pre-push, hard-fails — but only when PRE_COMMIT_REMOTE_BRANCH (set by pre-commit's hook-impl for pre-push hooks) is refs/heads/main — if any path .pre-commit-hooks.yaml exposes changed since the last tag reachable from HEAD. It is a silent no-op on every other branch: hard-failing on feature-branch pushes mid-review would force a premature tag on a commit that might not survive a squash-merge, the exact problem repo: local (above) already avoids for this repo's own dev-time gate. A tag not existing at all is also a hard fail on main, covering the very first release. This is deterministic tooling, not a standing instruction to remember — consistent with check-manifests.sh/check-vale-style-sync.sh (the latter deleted by ADR-0025, see the amendment at the top of this file) already using the same pre-push, main-agnostic-elsewhere pattern.
  • Known limitation, not yet closed: check-release-needed.sh only fires when a human runs git push locally with pre-commit's hooks installed — PRE_COMMIT_REMOTE_BRANCH is set by pre-commit's client-side hook-impl script parsing git push's stdin protocol. A PR merged through Gitea's merge button (server-side, no local push) or a CI runner invoking pre-commit run --hook-stage pre-push directly never sets it, so the gate silently doesn't run in either path. This repo has no CI workflow yet (has_actions is enabled but unused), so closing this gap needs a server-side job re-running the same script on merge to main — deferred as a separate piece of infrastructure, not fixed here. RELEASE_PATHS is derived from .pre-commit-hooks.yaml's own entry: lines rather than hand-maintained, so at least the set of paths it checks can't drift from the manifest on its own.
  • Dropping --config moved the release gate's path derivation too. check-release-needed.sh used to reach each hook's bundled assets through the dirname of its --config target. With no --config token left, that loop went dead and silently dropped both assets/vale/ trees from release coverage — a Vale rule change could then land on main without demanding a tag, leaving consumers pinned to an old rev: running stale rules while the gate stayed green. The script now derives the bundle's assets/ tree from tokens[0] instead (double-dirname, guarded on the candidate existing and on not resolving to .), which is the only derivation compatible with the argument-free entry: contract above.
  • Accepted residual in the release gate (closed — see the update below): deleting a hook's entire assets/ tree is not flagged — the derived candidate path stops existing, so the guard drops it before it reaches the pathspec. Deleting individual files inside a surviving tree is flagged, and tested.

Update (commit 14c2c91): the accepted residual above no longer holds and is recorded here only as the state at the time this ADR was written. check-release-needed.sh no longer derives release-relevant paths from the worktree alone. It runs collect_release_paths twice — once over the worktree's .pre-commit-hooks.yaml, once over the manifest read back from $LAST_TAG via git cat-file -p "$LAST_TAG:$HOOKS_MANIFEST" — and unions the two path sets, so a path the tag exposed stays in the pathspec even after the worktree's -d guard drops it. Wholesale deletion of a hook's bundled assets/ tree is therefore flagged, and tests/test-check-release-needed.sh (case 12) asserts exit 1 for exactly that case. The union does not over-fire: any manifest edit that makes the two disagree already touches $HOOKS_MANIFEST, itself a release-relevant path. An unreadable tagged tree (shallow clone, truncated fetch) fails closed rather than silently degrading to worktree-only derivation; a manifest simply absent at the tag — legitimate, it was added since — does not.

Update — the flattener rewrites no characters. This ADR never recorded it as a decision, but vale-wrap.sh's flattener carried a lossy last-resort branch: when a description needed quoting and held an ASCII apostrophe and held a double quote or backslash, it substituted U+2019 (’) for every ' before writing the scratch copy, on the stated rationale that no verbatim YAML scalar could carry that combination. The rationale was wrong. A |- literal block with a single indented content line carries ', ", \ and : byte for byte — a block scalar's body has no escape syntax at all — and vale's text.frontmatter.description scope still matches and fires rules on it (verified against vale 3.15.2; it is the same property that makes the | blocks in the wrapper's header safe to leave unflattened). The branch fired on 12 of the 54 in-scope files in this repo, silently disabling every rule whose token contains an apostrophe on each of them. The flattener now emits that literal block instead, so its output is verbatim in all four forms and no Vale rule can be silently disabled by the prefilter. The |- form is two physical lines where the three inline forms are one, so the blank-line pad that preserves later line numbers drops by one — reachable only when the original span is already two or more lines, so the pad count stays non-negative. tests/test-vale-wrap.sh case 20 asserts an apostrophe-bearing token actually fires on a flattened description in all three apostrophe-carrying branches, and case 20b pins the pad arithmetic against a body line's true line number.

Amendment (2026-09-16): the external hook contract is retired

Root .pre-commit-hooks.yaml, scripts/check-release-needed.sh, tests/test-check-release-needed.sh and tests/test-vale-hooks-consumer.sh are deleted, and the check-release-needed pre-push hook is removed from .pre-commit-config.yaml. The three exported hook IDs — kyberforge-vale-audit-skill, kyberforge-vale-audit-agent and kyberforge-skill-size-check — no longer exist, and no new vX.Y.Z tag is cut when hook files change. (Simplification audit finding 36.)

Three reasons, any one of which would have been enough to ask the question:

  • No consumer was found. The Gitea instance holds two repos. The other one pins seven hook repos, and none of them is this one. None of the 13 commits that touched the mechanism came from a consumer report; all were found by this repo's own tests. Clones outside the instance cannot be counted, but ADR-0024 accepted the same standard when it deleted the mirror.
  • The mechanism was already failing at its one job. scripts/skill-size-check.sh changed on main after v2.0.1, and no tag was cut, so a consumer pinning rev: v2.0.1 already ran a stale hook. The gate could not have caught it. It acted only when pre-commit reported a push to refs/heads/main, and PRs here merge through Gitea's server-side merge button, which runs no local hook. The script's own header said that closing the gap needed a server-side CI job the repo does not have.
  • The README already contradicted it. Its "For external consumers" section says apm is the only supported install path and never mentions .pre-commit-hooks.yaml or rev: pinning.

What is unaffected. This repo's repo: local hooks — skill-size-check, vale-audit-prefilter-skill and vale-audit-prefilter-agent — were always wired separately from the export, so no internal lint coverage is lost. The two prefilter hook IDs stay separate for the file-scope reason in "One hook per file-scope" above, not for an external contract. tests/test-vale-wrap.sh case 33, the cross-manifest files: drift check that ADR-0025 ported, went with the manifest it compared against. Its one guard that did not need a second manifest, a local regex narrowed to a single plugin, is now a third property of case 32. The v1.0.0, v2.0.0 and v2.0.1 tags are left in place. They are inert: nothing reads them, and apm's per_package versioning never consults tagPattern.

What is preserved for a return. The entry[0]-only constraint in the Consequences above, and its incident records at LESSONS.md:101 and :105, stay as written. That constraint says a published entry is a bare script path, with every bundled file located from ${BASH_SOURCE[0]}, and it took three review rounds to find. Both hook scripts still meet it: vale-wrap.sh takes no --config, and skill-size-check.sh keeps its embedded resolver copy. If a consumer appears, restore the manifest under that constraint, and restore test-vale-hooks-consumer.sh with it: it was the only test that exercised the entry-resolution path that once shipped broken. Restore a release gate only once a server-side job can run it on merge.

Correction (2026-09-16, later the same day). The paragraph above is wrong about skill-size-check.sh. ef27c97 removed its embedded resolver copy: the hook now sources plugins/kyberforge/.apm/skills/factory-audit/scripts/lib-boundary-resolver.sh by path and fails closed without it (ADR-0020's 2026-09-16 amendment; docs/spec/gates.md, "Duplicated constants"). An external consumer's checkout has no such file, so restoring the manifest as described would ship a hook that fails for every consumer — the defect recorded at LESSONS.md:101. Only vale-wrap.sh still meets the entry[0]-only constraint. A return must first make skill-size-check.sh self-contained again, by re-embedding the resolver or shipping the library beside the hook, and restore a consumer test that proves it.

Superseded statements elsewhere. ADR-0022's notes that the version-bump gate "is not exported through .pre-commit-hooks.yaml" and that it shares its gaps with check-release-needed, and ADR-0025's point 5 ("Both exported Vale hook IDs survive unchanged") and its case-33 port, describe the state before this amendment.