From acd2f1d4220bf41bad10a5ae519bf3890fa12e2e Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Sun, 9 Aug 2026 11:20:55 +0000 Subject: [PATCH] fix(lint): harden check-release-needed.sh, script the vale-style sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- LESSONS.md | 2 +- core/instructions/testing.md | 1 + ...14-vale-prefilter-ships-from-the-plugin.md | 10 ++ scripts/check-release-needed.sh | 62 ++++++++---- scripts/check-vale-style-sync.sh | 2 +- scripts/sync-vale-styles.sh | 21 ++++ tests/test-check-release-needed.sh | 96 ++++++++++++++++--- 7 files changed, 161 insertions(+), 33 deletions(-) create mode 100755 scripts/sync-vale-styles.sh diff --git a/LESSONS.md b/LESSONS.md index d1da2ba..234d3d9 100644 --- a/LESSONS.md +++ b/LESSONS.md @@ -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. diff --git a/core/instructions/testing.md b/core/instructions/testing.md index 69566d5..33fa97b 100644 --- a/core/instructions/testing.md +++ b/core/instructions/testing.md @@ -4,3 +4,4 @@ - Automate everything automatable. Manual testing only for nuanced UI/UX or agent interaction behaviour requiring human judgment. - Test observable end-state, not implementation internals. Tests must survive refactoring. - No test is better than a wrong test. A passing mock that masks a real failure is actively harmful. +- A clean result can mean nothing ran. Before trusting a new check, prove it fails against a deliberately-bad fixture, then run it against the real target. Where a check can scan zero inputs, assert on the input count, not just the exit code — a zero-file run and a real clean pass look identical otherwise. diff --git a/docs/adr/0014-vale-prefilter-ships-from-the-plugin.md b/docs/adr/0014-vale-prefilter-ships-from-the-plugin.md index 13b175e..10ac2c0 100644 --- a/docs/adr/0014-vale-prefilter-ships-from-the-plugin.md +++ b/docs/adr/0014-vale-prefilter-ships-from-the-plugin.md @@ -107,3 +107,13 @@ doesn't wonder if it was overlooked. `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` 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. diff --git a/scripts/check-release-needed.sh b/scripts/check-release-needed.sh index 3f2c5a7..8f1f285 100755 --- a/scripts/check-release-needed.sh +++ b/scripts/check-release-needed.sh @@ -8,6 +8,12 @@ set -euo pipefail # for pre-push hooks; on every other branch (feature work mid-review) this is a # silent no-op — pushing WIP commits there must not be blocked on cutting a # premature tag (see ADR-0014's repo: local vs pinned self-reference decision). +# +# Known gap: this only fires on a local `git push` through pre-commit's pre-push +# hook. A PR merged via Gitea's merge button (server-side, no local push) or a +# CI runner invoking `pre-commit run --hook-stage pre-push` directly does not set +# PRE_COMMIT_REMOTE_BRANCH and will not trigger this check — closing that +# requires a server-side CI job, which this repo does not have yet. TARGET_BRANCH="refs/heads/main" @@ -18,18 +24,34 @@ fi REPO_ROOT="$(git rev-parse --show-toplevel)" cd "$REPO_ROOT" -# Paths whose content .pre-commit-hooks.yaml exposes to external consumers. -# Keep in sync with .pre-commit-hooks.yaml's entry: paths. -RELEASE_PATHS=( - .pre-commit-hooks.yaml - scripts/skill-size-check.sh - plugins/kyberforge/skills/skill-audit/scripts - plugins/kyberforge/skills/skill-audit/assets/vale - plugins/kyberforge/skills/agent-audit/scripts - plugins/kyberforge/skills/agent-audit/assets/vale -) +HOOKS_MANIFEST=".pre-commit-hooks.yaml" -LAST_TAG="$(git describe --tags --abbrev=0 2>/dev/null || true)" +if [[ ! -f "$HOOKS_MANIFEST" ]]; then + exit 0 +fi + +# Derive release-relevant paths from .pre-commit-hooks.yaml's own entry: lines +# instead of hand-maintaining a parallel list — the manifest is the single +# source of truth for what external consumers actually pull at a pinned rev, +# so a hook added/removed/renamed there can't silently drift out of sync here. +# Each entry is "