fix(hooks): validate-spdx silently skipped paths containing a space - #961
Merged
Merged
Conversation
`.githooks/validate-spdx.sh` iterated its file list with an unquoted `for file in $FILES_TO_CHECK`, which splits on $IFS. A path containing a space became two paths, each of which then failed `[ -f "$file" ]` and was `continue`d: no error, no increment of $CHECKED, exit 0. The validator reported a pass on a file it had never opened. That is not hypothetical. The estate contains a literal-space directory `_RSR _SET`, so every source file beneath it passed this gate unread. Replaced with a `while IFS= read -r` loop fed by `<<< "$FILES_TO_CHECK"`. `<<<` and not a `| while` pipeline, because a pipeline runs the loop in a subshell and would discard $ERRORS and $CHECKED. The tests had to change in the same commit, and that is the finding worth recording: this suite could never have caught the bug, because it encoded the bug as its contract. Its multi-file cases passed STAGED_FILES SPACE-separated (`"good.sh bad.sh"`), which only ever worked because of the word-splitting being fixed here. The real and only caller, `.githooks/pre-commit`, builds the list with `git diff --cached --name-only --diff-filter=ACM` — one path per LINE. The suite asked a different question than its consumer. Multi-file cases are now newline-delimited, matching the caller. Four new `_RSR _SET` controls are the regression proof, and they kill the mutant: against the validator as it stood on main, all four fail and the other thirteen still pass — so the conversion is a contract correction, not a weakening. One of the four asserts the DENOMINATOR via a new `ck_count`, because a skipped file and a checked file both exit 0 and only the count tells them apart. Refs #912 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ji1bq3TypfycfUPAR7hSxR
Contributor
|
Warning Review limit reachedNext included review available in 53 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
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. Comment |
|
hyperpolymath
added a commit
that referenced
this pull request
Sep 22, 2026
…blocks red main) (#965) ## `main` is red, and round 2 only half-landed `main` has been red on **both** Hypatia checks since #954 (`9c256b67`) — `scan / Hypatia Neurosymbolic Analysis` and `governance / Validate Hypatia Baseline`. Every PR opened since inherits it, including #961. The cause is not the lockfile. It is that **one defect is emitted by two rule modules, and round 2 acknowledged only one of them.** | | acknowledged in #954 | still blocking | |---|---|---| | `rule_module` | `workflow_audit` | **`workflow_hardening`** | | `file` | `actions.lock` | `.github/workflows/actions.lock` | | `type` | `invalid_actions_lock` | `invalid_actions_lock` | | `severity` | `high` | `high` | | `reason` | transitive_dependencies_missing, asana/push-signed-commits@d615 | *identical* | `scripts/apply-baseline.sh:188` matches by **exact string equality**: ```jq .severity == $finding.severity and .rule_module == $finding.rule_module # <- exact, so workflow_audit != workflow_hardening and .type == $finding.type ``` The `file_pattern` was never the problem. `**actions.lock` tokenises to `\A.*actions\.lock\z`, which matches the full path correctly. ## Evidence Reproduced locally against **main's own committed** `apply-baseline.sh`, schema and baseline, using the finding copied verbatim from the failing run [35716729172](https://github.com/hyperpolymath/standards/actions/runs/35716729172): ``` before: kept=1 suppressed=0 ::error::Gate failed: 1 unfiltered finding(s) at or above 'high'. exit=1 after: kept=0 suppressed=1 exit=0 ``` The `before` line is byte-identical to what CI printed. **Four negative controls** confirm the entry suppresses only this exact finding — all four stay `KEPT`: | control | result | |---|---| | same finding at `critical` | KEPT | | same finding, `rule_module: some_other_module` | KEPT | | `type: unpinned_action` on the same file | KEPT | | same finding on `.github/workflows/release.yml` | KEPT | **Suites, all green on this branch:** `apply-baseline-test.sh` 15/0 · `filter-sarif-by-baseline-test.sh` 13/0 · `hypatia-blocking-gate-test.sh` 4/4. Full pre-commit hookset passes. **Exemption ratchet, both directions:** ``` with the trailer: OK (declared) .hypatia-baseline.json: 211 -> 212 [Ratchet-exception present] exit=0 without the trailer: Exemption ratchet: FAILED. exit=1 ``` The mutant was a throwaway branch, asserted and deleted — the gate has teeth here, it is not being taken on trust. ## What this does *not* do It does **not** accept new exposure. The underlying risk is unchanged and still owned by #951: `asana/push-signed-commits@d615` (immutable pin, ref `v1.3`) declares a transitive `actions/setup-python@v2`, which floats upstream and can never be a lock key. `gh-actions-lock` v0.1.6 `verify` reports `valid:true` on the same file — Hypatia is stricter than the authoritative tool. The exits remain exactly #951's: Asana pins `setup-python`, we replace the action, or Hypatia downgrades float-transitives. The diff is **+9 / −0**, a pure append. No existing entry is touched. ## Follow-up worth considering (not in this PR) One triage decision now needs **two** baseline entries that differ in a single field, and nothing warns you when you write only one — which is precisely how `main` went red. Letting `rule_module` accept a list would make one decision one entry. That is a schema + `apply-baseline.sh` + test change, so it is deliberately out of scope here; filing separately. --- 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Ji1bq3TypfycfUPAR7hSxR Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com> Co-authored-by: Claude Opus 5 <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
1 participant
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.
Closes #912.
What was wrong
.githooks/validate-spdx.shiterated its file list with an unquotedfor file in $FILES_TO_CHECK, which splits on$IFS. A path containing a spacebecame two paths, each of which then failed
[ -f "$file" ]and wascontinued.Silently: no error, no increment of
$CHECKED, exit 0.The validator reported a pass on a file it had never opened.
That is not hypothetical here. The estate contains a literal-space directory
_RSR _SET, so every source file beneath it has been passing this gate unread.The fix
A
while IFS= read -rloop fed by<<< "$FILES_TO_CHECK".<<<and deliberately not a... | while readpipeline: a pipeline runs the loopin a subshell, which would discard
$ERRORSand$CHECKEDand turn every failureinto a silent pass — the same class of bug in a new place.
Why the tests had to change in the same commit
This is the part worth reading. The existing suite could never have caught this,
because it encoded the bug as its contract.
Its multi-file cases passed
STAGED_FILESspace-separated:which only ever worked because of the word-splitting being fixed here. But the
real and only caller,
.githooks/pre-commit, builds the list withgit diff --cached --name-only --diff-filter=ACM— one path per line. Thesuite asked a different question than its consumer, so committing the loop fix
alone would have reddened it.
Multi-file cases are now newline-delimited, matching the caller exactly.
Mutant kill
Four new
_RSR _SETcontrols, run against the validator as it stands onmain:and against this branch:
All four mutant failures print the same line —
✅ All 0 source files have SPDX headers— which is the tell, and the reason one of the four asserts thedenominator through a new
ck_counthelper rather than the exit code: askipped file and a checked file both exit 0, and only the count distinguishes
"read it and it was fine" from "never read it".
The other thirteen controls still pass against the mutant, so the newline
conversion is a contract correction, not a weakening of what they assert.
Scope
Two files. No workflow, no gate wiring, no licence text, no third-party or
vendored content touched. Read-only validator behaviour throughout — nothing here
writes or fixes anything.
Checked the sibling validators for the same shape while here:
validate-spdx-workflows.sh,validate-sha-pins.sh,validate-permissions.shand
validate-bot-directives.shall already usewhile IFS=$'\n' read -r.validate-spdx.shwas the only one with the word-split loop.Related standing decision surface: #787.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Ji1bq3TypfycfUPAR7hSxR