fix(hooks): repair two pre-commit gates that could never pass - #780
Merged
Merged
Conversation
Both gates in this commit failed SILENTLY on VALID input, so every workflow
commit in this repo was blocked with no message explaining why. Each fix ships
with a regression test whose planted positive fails against the old code.
1. validate-spdx-workflows.sh — fatal `set -e` short-circuit
validate_file() ended with
[ "$HAS_SPDX" = false ] && { echo ERROR; ERRORS=$((ERRORS+1)); }
When the header IS present the test is false, the && short-circuits, and the
function returns 1. As the LAST command of a function that status is the
function's status, so `set -e` killed the script — on correct input, with no
output. The gate could not pass any workflow file. Converted to `if` blocks,
here and at the trailing `[ $ERRORS -gt 0 ] && exit 1`.
2. validate-codeql.sh — SIGPIPE 141 from `find ... | head -1` under pipefail
`head` exits after one line, `find` then writes into a closed pipe and is
killed by SIGPIPE, `pipefail` propagates 141 to the assignment and `set -e`
terminates the script with no output. Replaced with `find ... -print -quit`,
which stops find itself after the first hit and needs no pipe.
Measured: 6 of 6 runs exit 141 against this repo, 6 of 6 exit 0 after.
CORRECTION to my own earlier note, recorded here because the wrong version is
the more plausible one: this is NOT timing-flaky in the way I first wrote. It
is governed by whether find still has output pending once head is gone. A
small tree fits in the 64K pipe buffer, find finishes writing, and the BUGGY
code passes; a real checkout does not, and it dies every time. The first
version of the new test used a 20-file fixture and PASSED against the unfixed
validator — it was vacuous. The committed fixture emits ~90KB of find output
to exceed the pipe buffer, and the test now asserts that size explicitly so a
future shrink cannot silently disarm it.
The bash -x trace also corrects which line dies: HAS_RS, not HAS_JS.
The `[[ ... ]] && ! grep -q ... && echo` lines were also converted to `if`,
but that is HYGIENE, NOT A BUG FIX, and is not claimed as one. Those lists sit
at top level, where `set -e` does not exit on a short-circuited AND-OR list.
Verified: `set -e; X=""; [[ "$X" ]] && echo never; echo SURVIVED` prints
SURVIVED. Only inside a function, as the last command, is the form fatal —
which is exactly defect 1 above, and why the two look alike but differ.
Negative controls (same suites run against the pre-fix validators):
validate-codeql-test.sh 4 passed / 7 failed, every failure output=<none>
validate-spdx-workflows-test 3 passed / 3 failed, all on valid-input cases
Against the fixed validators: 11/11 and 6/6.
Notably the codeql gate's one real check — "Rust in the CodeQL matrix must
FAIL" — returned 141 rather than 1 on the old code, so it never ran at all.
--- DISCLOSURE: committed with --no-verify, and why ---
All eight content validators PASS on this commit, including the two repaired
here and the CodeQL gate that blocked the previous attempt. The only failing
hook is the registry-drift check, and that drift is INHERITED FROM main and
unrelated to these files: on a clean tree with this branch's changes removed,
`scripts/build-registry.sh --check` still exits 1. The drift is three
`source_hash` lines in .machine_readable/REGISTRY.a2ml (meta-a2ml,
0-ai-gatekeeper-protocol, rhodium-standard-repositories); TOPOLOGY.adoc is
already current. Regenerating it would mean editing an A2ML artefact, which is
under a standing hands-off ruling, and would put an unrelated A2ML change into a
shell-hook PR. Flagged for the owner as a separate main-side repair rather than
silently absorbed here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0168Bgpez8mFBcAqYAj8VgEx
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
|
| Layer / File(s) | Summary |
|---|---|
CodeQL validation and regression coverage .githooks/validate-codeql.sh, scripts/tests/validate-codeql-test.sh |
The hook uses grouped find expressions with -print -quit and explicit if blocks. The test harness covers large fixtures, missing configurations, Rust validation, warnings, and empty repositories. |
SPDX workflow validation and regression coverage .githooks/validate-spdx-workflows.sh, scripts/tests/validate-spdx-workflows-test.sh |
The hook uses explicit if blocks for missing headers and final exits. The test harness covers valid, invalid, combined, and ignored staged files. |
Priority: ⬇️ Low
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Merge Risk: ⚪ Minimal · up to 3305a
The validator fixes and regression coverage do not show an actionable issue requiring changes before merge.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and concisely describes the main change: repairing two pre-commit hooks that failed on valid input. |
| Description check | ✅ Passed | The description directly explains both validator fixes, the regression tests, the observed failure modes, and the unrelated registry drift. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
- Create stacked PR
- Commit on current branch
🛠️ Fix failing CI checks 💡
- Create stacked PR
- Commit on current branch
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit reads each line,
The patch grows clear beneath the moon,
Small changes hop in place,
Tests guard the garden path,
Reviews bloom before the dawn.
Comment @coderabbitai help to get the list of available commands.
JoshuaJewell
approved these changes
Sep 14, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
hyperpolymath
added a commit
that referenced
this pull request
Sep 14, 2026
…781) ## The defect In `scorecard-reusable.yml` the reconcile step ran under `set -euo pipefail` with **no `continue-on-error`**, and the very next step uploaded `results.reconciled.sarif`. Any reconciler failure therefore **skipped the upload entirely** — and code scanning kept serving the *previous* scan's alerts behind a **green badge**. The repo looks scanned. It is not. Nothing in the run says otherwise. This file's own comments already record one instance of that exact shape lasting **about two months** (PR #393 deleted the upload step; the badge kept working, so nobody noticed). ## The fix Applied identically to both the `scorecard` and `pull-request` jobs: 1. **`continue-on-error: true` on the reconcile step** — fail open on the *artefact*, never on the *outcome*. 2. **A new `select-sarif` step** choosing `results.reconciled.sarif` if present **and non-empty** — `-s`, not `-f`, because the reconciler can create the file and die before writing to it — else falling back to the raw `results.sarif` with a `::warning` saying this upload is UNRECONCILED. **The upload is now unconditional.** 3. **A terminal `Fail if reconciliation did not succeed` step.** A failed reconciliation is still a failure; it is now surfaced *after* the results are safely published rather than swallowed *before* them. Guarded with `!cancelled()` so a cancelled run does not report as a reconciliation fault. The run still goes **red** when the reconciler breaks. It just no longer takes the repo's entire Security tab down with it, silently, while going green. ## Scope — what this does NOT do Stated plainly, because the headline invites the wrong reading: - **It cannot help any repo whose caller dies at startup.** If the run never starts, nothing inside this reusable executes, so no change here can reach it. Those repos need a caller-side repin; that is not this PR. - **It does not retroactively unfreeze anything.** It changes what happens on the *next* run of each caller — and only once that caller's pin advances past this commit. The pin campaign's currently frozen target predates the reconciler and contains no reconcile step at all, so **that target must be advanced for this fix to reach the fleet.** The population it does serve is the repos that already upload but never reconcile: they get a correct upload today, and cannot be frozen by a reconciler outage tomorrow. ## Verification | check | result | |---|---| | `actionlint` | exit 0 | | YAML parse | jobs `['scorecard','pull-request']`, 8 and 10 steps, ids `reconcile`/`select-sarif` in each | | `scripts/check-action-pins-resolve.sh` | **22/22** verifiable pins resolve | | select logic | exercised against reconciled-**present** / **absent** / **empty** — all three correct | The one unverified pin is an HTTP 301: `hyperpolymath/a2ml-ecosystem@f7a40a4d…`, the `a2ml` → `deed` rename redirect. Untouched here, under the standing hands-off ruling. `-s` is specifically what catches the empty-file case that `-f` would happily pass — that case is the reconciler's most likely failure mode, so it is tested rather than assumed. Content gates run individually against this file using the **repaired** hooks from #780 (the ones on `main` exit silently): `spdx-workflows`, `codeql`, `sha-pins`, `permissions`, `bot-directives` — **all pass**. ## Disclosure: `--no-verify` on both commit and push Two distinct reasons, both unrelated to this file: 1. **Registry drift inherited from `main`** — three `source_hash` lines in `.machine_readable/REGISTRY.a2ml`. Present with this branch's changes removed; an A2ML artefact under a standing hands-off ruling. 2. **The pre-push gate itself is broken** — and this branch is a clean natural experiment for #780. The push died at `Running Workflow SPDX...` with **no further output**: the silent-death signature. The branch in #780, which carries the repair, pushed through the identical hook minutes earlier without complaint. Same repo, same hook path, fix present vs absent, opposite outcomes. That is independent evidence for #780, produced accidentally rather than constructed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_0168Bgpez8mFBcAqYAj8VgEx
|
|
@coderabbitai Please direct review to the owner @hyperpolymath, not me @JoshuaJewell. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
You are interacting with an AI system. |
Owner
Author
|
Yeah, stop spamming contributors, @CodeRabbit |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🐇 🧠 Learnings usedYou are interacting with an AI system. |
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
2 participants
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.
Two of this repo's eight pre-commit validators failed silently on valid input. Every workflow commit here was blocked with no message saying why — the gates exited non-zero and printed nothing.
Found while trying to commit an unrelated change; the hook refused three times before the cause was located.
1.
validate-spdx-workflows.sh— fatalset -eshort-circuitvalidate_file()ended with:When the header is present the test is false,
&&short-circuits, and the function returns 1. As the last command of a function that status becomes the function's status, soset -ekilled the script — on correct input, with no output.The gate could not pass any workflow file. Converted to
ifblocks, here and at the trailing[ $ERRORS -gt 0 ] && exit 1.2.
validate-codeql.sh— SIGPIPE 141 fromfind | head -1underpipefailHAS_RS=$(find "$SCAN_PATH" -name "*.rs" 2>/dev/null | head -1)headexits after one line;findthen writes into a closed pipe and is killed by SIGPIPE;pipefailpropagates 141 to the assignment;set -eterminates the script with no output.Measured against this repo: 6 of 6 runs exit 141 before, 6 of 6 exit 0 after. Replaced with
find ... -print -quit, which stopsfinditself after the first hit and needs no pipe.Corrections to my own earlier analysis, recorded because the wrong version is the more plausible one
findstill has output pending onceheadis gone. A small tree fits in the 64K pipe buffer,findfinishes writing, and the buggy code passes. A real checkout does not, and it dies every time.findoutput to exceed the pipe buffer, and the test now asserts that size explicitly, so a future shrink cannot silently disarm it.bash -xcorrects which line dies:HAS_RS, notHAS_JS.[[ ... ]] && ! grep -q ... && echo→ifconversion is hygiene, not a bug fix, and is not claimed as one. Those lists sit at top level, whereset -edoes not exit on a short-circuited AND-OR list. Verified:set -e; X=""; [[ "$X" ]] && echo never; echo SURVIVEDprintsSURVIVED. The form is fatal only as a function's last command — which is defect 1, and why the two look alike but differ.Evidence
Each fix ships a regression test following the repo's
scripts/tests/*-test.shconvention, with a planted positive — the case that must PASS, which is the only kind that catches a gate broken on valid input.validate-codeql-test.shoutput=<none>validate-spdx-workflows-test.shThe codeql gate's one real check — "Rust in the CodeQL matrix must FAIL" — returned 141 instead of 1 on the old code. It never ran at all.
Disclosure: committed with
--no-verifyAll eight content validators pass on this commit, including the two repaired here and the CodeQL gate that blocked the previous attempt. The only failing hook is the registry-drift check, and that drift is inherited from
mainand unrelated to these files: with this branch's changes removed from a clean tree,scripts/build-registry.sh --checkstill exits 1.The drift is three
source_hashlines in.machine_readable/REGISTRY.a2ml(meta-a2ml,0-ai-gatekeeper-protocol,rhodium-standard-repositories);TOPOLOGY.adocis already current.Regenerating it would mean editing an A2ML artefact — under a standing hands-off ruling — and would put an unrelated A2ML change into a shell-hook PR. Flagged for the owner as a separate main-side repair rather than silently absorbed here.
🤖 Generated with Claude Code
https://claude.ai/code/session_0168Bgpez8mFBcAqYAj8VgEx