feat(check): add ci-bash-suites validator — close the gate-divergence gap - #291
Merged
Conversation
… gap /craft:check never ran the shell suites CI invokes directly, because pytest does not collect them. The local gate and CI measured different things, and the local one reported green for a surface it never touched. This is the root cause behind two of the three CI blockers the v4 folio-split branch carried. T3.5.2 deleted commands/git/ and cited "full pytest 2575 passed" as its verification evidence — true and irrelevant: tests/test_git_shim_correctness.sh is a CI-REQUIRED check that asserted those exact files still existed. It had been red for days. Nothing local said so; CI rejected the PR. Implemented as a new hot-reload validator rather than a change to check.md — /craft:check already discovers .claude-plugin/skills/validation/*.md and forks each with CRAFT_MODE, so no existing logic is disturbed. Derives the suite list FROM .github/workflows/*.yml (direct `bash <path>` / `sh <path>` invocations under a test dir) rather than hardcoding it: - craft has 22 tests/*.sh but CI runs only 6 — running all 22 would invent a gate CI never asked for and fail on deliberately-manual suites. - A hardcoded list is the exact drift this validator exists to prevent. Add a suite to a workflow and it is picked up with no edit here. - Non-test scripts (scripts/docs-staleness-check.sh, scripts/aggregator-sync.sh) are excluded — build steps, or they have their own validators. Mode behavior follows the documented release-gated contract (same as version-check, skill-standards): advisory in default, blocking in release. CRAFT_MODE cannot distinguish --for pr from --for commit (both resolve to `default`), so blocking at that tier would block every commit on a ~3-5 min run. Advisory is enough to have caught T3.5.2: the failure becomes visible at check time instead of at PR rejection. Verified by planted defect, not just a green run: replay the REAL T3.5.2 bug (restored the pre-fix suite from d41de18) -> caught; WARN in default, exit 1 in release suite exits 1 -> caught; default-mode exit 0 (advisory, correct) workflow references a missing suite -> caught ("referenced by a workflow but not found on disk") + remediation note repo with no .github/workflows -> clean SKIP, exit 0 (generic-repo safe) clean baseline before and after -> 6/6 green Discovery verified exact: finds CI's 6 suites, excludes the 2 non-test scripts and the 16 manual-only suites. No count cascade (validators sit outside the counted surface); validate-counts.sh exits 0 at 47/40/2. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Data-Wise
added a commit
that referenced
this pull request
Jul 16, 2026
…nce closed .STATUS still claimed "Counts unchanged 95/45/8" and last_session 2026-07-15, while dev is at 47/40/2 after today's two merges — the project's own state record contradicting the repo by ~48 commands. Records: PR #290 (v4 folio split, merge-commit 4af10f2, craft 94->47), PR #291 (ci-bash-suites validator, squash 88b221d), 5034aef (2 stale docs/commands.md count claims), and the local branch-guard hook re-sync that cleared the last known test failure. The session recap keeps the reusable part rather than just the outcome: the three CI blockers had three unrelated causes (stale branch / stale plan number / real debt) and triaging them as one bucket would have produced three wrong fixes. Also records the evidence discipline that paid off — planted defects over green runs, reading the CI log rather than the badge, baselining before blaming, and treating a recommendation refuted mid-execution as refuted rather than authorized. Open items carried forward: Phase 4, tasks/todo.md's nonexistent plan-of-record path, docs/commands.md:10's "17 Categories" definition question, and 2 branch-guard false positives. R09 status-drift verified back to its pre-existing baseline: my first draft said "preserved for the v4.0.0 tag", which tripped Check B — .STATUS's milestone is ONE line containing "SHIPPED", so every vX.Y.Z token on it must resolve to a real git tag, and v4.0.0 is not cut yet. Reworded to "the eventual v4 release tag". The 2 remaining warnings (v2.61.1, v1.16.0) are pre-existing historical prose, confirmed by running the checker against HEAD's .STATUS in the real repo. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Data-Wise
added a commit
that referenced
this pull request
Jul 16, 2026
… positives (#292) * fix(guard): strip quoted spans + scope writes to PROJECT_ROOT branch-guard.sh's Bash write-through detection (Patterns 1-4) coarse-scanned the raw command string for shell metacharacters with plain grep, which has no notion of quoting. Three false positives fired live in one session, including during an audit of this exact bug class: awk 'NR>=203 && ...' -> '>' inside a single-quoted awk program read as a redirect grep -E '>[^=]' ... -> same, inside a grep pattern grep -n '...\"cp \|...' ... -> literal "cp " substring inside a quoted search pattern read as a cp invocation Same failure class Group 14c (2026-07-10) already fixed for heredoc bodies via HAS_HEREDOC; this generalizes it to any quoted span, not just heredoc bodies. COMMAND_SCAN strips quoted-span CONTENTS before the coarse `grep -q` presence checks; extraction (`grep -oE`) still runs against the ORIGINAL $COMMAND, so a real target quoted for spaces is unaffected. A fourth, unrelated bug fired alongside it: `cp <file> /tmp/...` was flagged as "creates a new code file on dev" even though /tmp is nowhere near the repo — only /dev/* was excluded. Added a PROJECT_ROOT prefix check on the resolved write target. PROJECT_ROOT is a realpath (`git rev-parse --show-toplevel`); $CWD often isn't (macOS's /tmp -> /private/tmp, and `mktemp -d` returns the /tmp form) — a naive literal-prefix compare broke 6 pre-existing in-repo-write tests during development. Fixed by canonicalizing the target's directory via `cd && pwd -P` (POSIX, avoids realpath/readlink -f, which aren't universally available) before comparing. Verified by planted defect AND by replaying the actual live false positives verbatim against the fixed hook (all resolve to exit=0), plus an isolated confirmation that a genuine redirect on a fresh dev-branch repo still returns exit=2 (blocked). New Group 22 (9 tests): 5 catch the false-positive class (including the 2 real historical commands), 4 are regression guards proving the fix doesn't weaken real detection (quoted target with spaces, cp inside the repo, a real touch alongside an unrelated quoted decoy). Full suite: 133/133 (was 124/133 mid-development, before the pwd -P fix). E2E: 30/30 (1 skipped, unrelated). Full pytest: 2601 passed / 1 failed (the 1 is the expected repo-vs-installed-hook divergence this branch itself causes, cleared by install-guards.sh — same pattern as PRs #290/#291). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(guard-audit): correct the skill's fictional config schema Discovered while running this skill to audit the false positives fixed in 7b51969. Step 5's documented config schema (nested "branches" object, "version": 2, allowed_extensions, pr_body_scan, force_push_allow, dev_allowed_extensions) is read ZERO times by scripts/branch-guard.sh — grepped for every one of those keys, all 0 matches. The real schema is a flat branch-name -> protection-level map, read via a single lookup, _json_get ".\"${BRANCH}\"" (branch-guard.sh:234). Following the skill's own instructions would have produced a config file the guard silently ignores. Also removed a fabricated "destructive git in PR body" rule (grepped the script for pr_body/PR body/--body: 0 matches — no such scanning exists) and replaced Step 4's fictional recommendations with the two real friction classes this session actually found: quoted-span misdetection and missing path scoping, neither fixable via any config key. Added a classification step (Step 4) splitting false positives into branch-policy (config-fixable) vs. detection-logic (requires a code PR to branch-guard.sh, not a config change) — the skill's "never modify branch-guard.sh" constraint was previously silent on what to do when the bug IS in branch-guard.sh; now it says so explicitly rather than implying a config workaround exists for everything. Verified: skill_standards_audit.py 100/100, markdownlint clean, description 400 chars (well under the 1536 cap). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf(guard): collapse COMMAND_SCAN to one sed invocation 7b51969's quote-stripping added printf | sed | sed (3 forks) to every Bash tool call on dev/main. dev's own dogfood perf budget (test_branch_guard_under_200ms) went from 207ms (pre-existing, already marginal on this machine — same class as the test_performance_cached_run xfail note re: unreliable wall-clock timing on shared runners) to 236ms. Collapsed to a single sed invocation (two -e expressions, here-string instead of printf|pipe) -> 3 forks to 1. Re-measured 0.80-0.90s per pytest invocation across 3 runs, consistently under budget. Full suite (133/133), E2E (30/30), and full pytest (2602 passed / 0 failed) all still green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(guard): correct cross-boundary quote-pairing regression in COMMAND_SCAN Adversarial review of PR #292's own Group 22 tests surfaced a real false negative the quote-stripping fix introduced: two independent sed passes (single-quote pass, then double-quote pass) let a single-quote-pair span across two unrelated double-quoted strings that each contain one apostrophe (e.g. "it's" ... "don't"), erasing a real redirect sitting between them from COMMAND_SCAN. Confirmed live: a genuine `> new_file.py` write-through on dev went completely undetected. Fix: combine both quote types into one alternation pattern (`'...'|"..."`) so quote-type resolves at the first quote character encountered and can never pair across a boundary of the other type. Verified via planted-defect replay: the new regression test fails against the reverted two-pass sed and passes against this fix; full suite 134/134. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gap
/craft:checknever ran the shell suites CI invokes directly, because pytest doesn't collectthem. The local gate and CI measured different things — and the local one reported a
comfortable green for a surface it never touched.
This is the root cause behind two of the three CI blockers the v4 branch carried (PR #290).
T3.5.2 deleted
commands/git/and cited "full pytest 2575 passed" as its verificationevidence. That was true and irrelevant:
tests/test_git_shim_correctness.sh— a CI-requiredcheck — asserted those exact files still existed. It had been red for days. Nothing local ever
said so; CI rejected the PR.
The general failure is gate divergence: CI runs checks the local gate doesn't.
Approach
A new hot-reload validator, not a change to
check.md./craft:checkalready discovers.claude-plugin/skills/validation/*.mdand forks each withCRAFT_MODE, so no existing logic isdisturbed — this plugs into the extension point that already exists.
The suite list is derived from
.github/workflows/*.yml, not hardcoded. That's the wholepoint:
tests/*.shbut CI runs 6. Running all 22 would invent a gate CI neverasked for and fail on deliberately-manual suites.
workflow and it's picked up on the next run, no edit here.
scripts/docs-staleness-check.sh,scripts/aggregator-sync.sh) areexcluded — build steps, or they have their own validators.
This generalises: any repo whose CI shells out to test suites gets the same protection, matching
how
test-coverage.mddetects python/node/R/go rather than hardcoding one.Modes
Follows the documented release-gated contract (same as
version-check,skill-standards):CRAFT_MODEdefault(--for commit/--for pr)release(--for deploy)Advisory-in-default is deliberate.
check.mdstates validators receive their tier only viaCRAFT_MODE, which resolves todefaultfor both--for commitand--for pr— so blockingat that tier would block every commit on a ~3–5 min run. Advisory is enough to have caught
T3.5.2: the failure becomes visible at check time instead of at PR rejection.
Test evidence — planted defects, not just a green run
The decisive one replays the real historical bug: I restored the pre-fix
test_git_shim_correctness.shfromd41de18ee(the version that sat red for days behind a greenpytest) and confirmed the validator catches it.
Discovery verified exact: finds CI's 6 suites (
test_branch_guard,test_branch_guard_e2e,test_no_switch_guard,test_git_shim_correctness,test_bump_version,test_post_release_sweep), excludes the 2 non-test scripts and the 16 manual-only suites.Note this caught something I had wrong: I'd been tracking 4 CI bash suites all along. Parsing
the workflows found 6 — there's a second step at
ci.yml:194–195running the release-pipelinesuites. Deriving from CI beats trusting a list, which is the point of the design.
Other gates:
The 1 pytest failure is
test_branch_guard_dogfood.py::TestHookInstallation::test_repo_copy_matches_installed— pre-existing on
dev, unrelated to this PR (verified: it fails identically on a cleandevcheckout with no validator present). Cause: PR #290 merged the v4 hook text into
dev, so thelocally-installed
~/.claude/hooks/branch-guard.shnow predates the repo copy. CI runsinstall-guards.shatci.yml:151before the suites, so CI syncs them. Locally it's fixed byre-running
install-guards.sh— deliberately not done here, as that's global machine state.🤖 Generated with Claude Code