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 "