fix(lint): harden check-release-needed.sh, script the vale-style sync

A review of PR #85's last two commits (1164f3a, 4d018af) found the new
release-gate script fails open in four separate ways, and the new drift
check for the duplicated Vale styles only ever detects drift after a
human already hand-edited both copies out of sync.

check-release-needed.sh:
- The `-e` existence filter dropped a RELEASE_PATHS entry from the diff
  pathspec once it was deleted from the tree, so deleting a path exposed
  via .pre-commit-hooks.yaml since the last tag passed the gate clean —
  exactly the breakage the gate exists to catch. git diff reports
  deletions fine without an existence check; the filter is gone.
- `git diff ... 2>/dev/null || true` turned any git failure (a shallow
  clone missing the tag's objects, a corrupted ref) into an empty,
  falsely-clean diff. The diff result is no longer swallowed: a failure
  now hard-fails with the underlying git error visible.
- RELEASE_PATHS was a hand-maintained array duplicating
  .pre-commit-hooks.yaml's entry: paths with only a comment holding them
  in sync, and was already over-broad (it swept in validate.sh /
  validate-provenance.sh, which no hook entry references). It's now
  parsed straight from .pre-commit-hooks.yaml's entry: lines at
  runtime, so it can't drift from the manifest and only tracks what a
  hook actually exposes.
- `git describe --tags --abbrev=0` accepted any tag reachable from HEAD
  as the diff baseline, not just release tags. Added
  `--match 'v[0-9]*.[0-9]*.[0-9]*'` so an incidental checkpoint tag
  can't shift the baseline and mask a real release-relevant change.

check-vale-style-sync.sh still only detects drift between skill-audit's
and agent-audit's duplicated vale-wrap.sh/styles/Kyberforge copies
(both copies must exist independently per the plugin's no-cross-skill-
path packaging rule — a symlink would break at install time). Added
scripts/sync-vale-styles.sh to regenerate skill-audit's copy from
agent-audit's canonical one on demand, and pointed the sync check's
failure message at it, so fixing drift is one command instead of a
hand diff across two files.

Also recorded, rather than silently left unfixed: check-release-needed.sh
only fires on a local `git push` through pre-commit's pre-push hook — a
PR merged via Gitea's merge button, or CI invoking
`pre-commit run --hook-stage pre-push` directly, never sets
PRE_COMMIT_REMOTE_BRANCH and skips the gate entirely. Closing that needs
a server-side CI job this repo doesn't have yet; documented as a known
limitation in ADR-0014 rather than papered over.

Separately, 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 into
core/instructions/testing.md and fixed the marker.

tests/test-check-release-needed.sh gained 4 regression tests, one per
check-release-needed.sh fix above, each verified to fail against the
pre-fix script and pass against the current one.

Verification: bash tests/run-tests.sh (11 scripts + 125 bats, all
passing), pre-commit run --all-files, and
pre-commit run --all-files --hook-stage pre-push all clean.

ADR: 0014
This commit is contained in:
2026-08-09 11:20:55 +00:00
parent 4d018af03c
commit acd2f1d422
7 changed files with 161 additions and 33 deletions

View File

@@ -140,7 +140,7 @@ During write-skill refactor, an "open thread" note (about a deferred research st
## 2026-08-08 — A clean linter result can mean "nothing was checked"
Three separate times in one PR (#85), a check reported success because it had silently not run. (1) Vale's `text.frontmatter.description` scope stops matching once the value is a multi-line YAML block scalar — the style most skills here use — so a repo-wide sweep returned 0 alerts across 49 files and was read as a clean repo. (2) Five of six rules were `level: warning`, but Vale's exit code keys on `error` alone and pre-commit hides output from passing hooks, so those rules were invisible and blocked nothing for two review rounds while the ADR described them as "enforcing immediately." (3) `.vale.ini`'s globs matched no file outside `plugins/`, so Vale printed "0 files" and exited 0, which both audit skills read as "no findings" and used to skip their own judgment passes. Each time the green result was worse than no check at all, because it was cited as positive evidence of cleanliness. Fix: for any new check, prove it fails before trusting that it passes — run it against a deliberately-bad fixture, confirm the failure, then run the real corpus. Where a check can scan zero inputs, assert on the input count, not just the exit code. **Graduated** (4th instance below).
Three separate times in one PR (#85), a check reported success because it had silently not run. (1) Vale's `text.frontmatter.description` scope stops matching once the value is a multi-line YAML block scalar — the style most skills here use — so a repo-wide sweep returned 0 alerts across 49 files and was read as a clean repo. (2) Five of six rules were `level: warning`, but Vale's exit code keys on `error` alone and pre-commit hides output from passing hooks, so those rules were invisible and blocked nothing for two review rounds while the ADR described them as "enforcing immediately." (3) `.vale.ini`'s globs matched no file outside `plugins/`, so Vale printed "0 files" and exited 0, which both audit skills read as "no findings" and used to skip their own judgment passes. Each time the green result was worse than no check at all, because it was cited as positive evidence of cleanliness. Fix: for any new check, prove it fails before trusting that it passes — run it against a deliberately-bad fixture, confirm the failure, then run the real corpus. Where a check can scan zero inputs, assert on the input count, not just the exit code. **[graduated → core/instructions/testing.md]** (4th instance below, kept for audit trail).
**4th instance (2026-08-09, ADR-0014):** splitting the single root `.vale.ini` into two skill-scoped copies (skill-audit: `SKILL.md` only; agent-audit: agent files only) meant a single retargeted pre-commit hook pointed at agent-audit's copy alone would have silently scanned 0 `SKILL.md` files and exited 0 — caught only because the full corpus was dry-run against both the old and new config and the outputs diffed before the old config was deleted, not because any test asserted on file counts. Standing practice going forward: when a Vale (or any linter) config that serves multiple file-glob scopes is split or moved, dry-run the full corpus through both the old and new config and diff the outputs before removing the superseded source — a hook silently scanning 0 files looks identical to a clean pass.