chore: enforce CI/local parity and close the test.sh bypass - #187
Merged
Conversation
Three gaps left over from the previous pass, all the same silent-drift shape it was meant to fix. **CI bypassed the test aggregator.** The workflow ran `bash tests/test.sh` while the docs and validate-all.sh ran `bash test.sh`. The top-level script is the aggregator — it already parses pytest and cargo output and is built to hold more than one suite root — so a second root added there would never have run in CI. Point CI at `bash test.sh`. **Nothing enforced the parity that validate-all.sh claims.** Its header asked humans to keep its list in step with the workflow, which is precisely the convention that rotted last time (CLAUDE.md said four checks; CI ran five; the missing one had no local equivalent at all). Add check-ci-parity.sh, which extracts the workflow's check commands and fails if validate-all.sh does not run each of them. Wired into both validate-all and the CI plugin-structure job, so it is enforced from either direction. **pre-commit did not catch the mock recursion pattern.** Add the cheap grep for `command git|gh|tea` under tests/. The full shellcheck pass takes ~8s, too slow for every commit, so that stays in CI and lint-shell.sh. Verified the parity check actually catches drift, not just passes: with CI reverted to `bash tests/test.sh` it reports `MISSING bash tests/test.sh` and exits 1. validate-all.sh: six checks, all pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EddJxaTMXyj7o4ndrqfCxa
There was a problem hiding this comment.
🟡 Changes recommended
The new parity script and pre-commit check use GNU-only \s/--include grep/sed features that can break local validation/hooks on macOS/BSD environments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens the repo’s “CI and local validation must match” guarantee by making CI run the same test aggregator as local workflows, and by adding an automated parity check that prevents the CI workflow and validate-all.sh from drifting apart.
Changes:
- Update CI to run
bash test.sh(the top-level test aggregator) instead ofbash tests/test.sh. - Add
.github/scripts/check-ci-parity.shand wire it into both CI andvalidate-all.shto enforce CI/local check parity. - Extend the pre-commit hook with a fast grep-based guard against the known
command git|gh|teamock-recursion pattern.
File summaries
| File | Description |
|---|---|
CLAUDE.md |
Documents the added parity check and clarifies why test.sh (aggregator) must be used. |
.github/workflows/ci.yml |
Runs the aggregator (bash test.sh) and adds a CI step to enforce parity with validate-all.sh. |
.github/scripts/validate-all.sh |
Adds the parity check as a first-class local validation step. |
.github/scripts/check-ci-parity.sh |
New script that extracts CI “check commands” and verifies validate-all.sh includes them. |
.githooks/pre-commit |
Adds a lightweight pre-commit guard to prevent reintroducing the command <tool> mock-recursion pattern. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+33
to
+34
| grep -oE '^\s+run: (bash [^ ]+\.sh|rumdl check \.)$' "$WORKFLOW" | | ||
| sed -E 's/^\s+run: //' | sort -u |
| # bypasses functions and aliases but not PATH lookup, so the mock re-executes | ||
| # itself without bound. This is the cheap grep only; the full shellcheck pass | ||
| # (~8s) stays in CI and `lint-shell.sh` rather than on every commit. | ||
| if grep -rn --include='*.sh' -E '\)\s*command (git|gh|tea) ' "$repo_root/tests/" 2>/dev/null; then |
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.
Follow-up to #186, closing three gaps of the same silent-drift shape that PR was meant to eliminate.
1. CI bypassed the test aggregator
The workflow ran
bash tests/test.shwhile the docs andvalidate-all.shranbash test.sh. Those are not the same thing: the top-level script is the aggregator — it already parses pytest and cargo output and is built to register more than one suite root.A second suite root added to
test.shwould have run locally and silently never run in CI. Pointed CI atbash test.sh.2. Nothing enforced the parity
validate-all.shclaimsIts header asked humans to keep its list in step with the workflow. That convention is exactly what rotted last time — CLAUDE.md said "four checks", CI ran five, and the missing one had no local equivalent at all.
check-ci-parity.shextracts the workflow's check commands and fails ifvalidate-all.shdoesn't run each. Wired in from both directions: as a check insidevalidate-all.sh, and as a step in the CI plugin-structure job.3. pre-commit didn't catch the mock recursion pattern
Added the cheap
command git|gh|teagrep overtests/. The full shellcheck pass takes ~8s — too slow to put on every commit — so it stays in CI andlint-shell.sh.Test plan
bash tests/test.shand confirmed it reportsMISSING bash tests/test.shand exits 1; restored, back to 5/5 ok.validate-all.sh— six checks, all passshellcheck --severity=errorclean on the new script and the modified hook