fix(ci): stop the security-evidence guard failing every out-of-scope PR - #2523
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 1m 54s —— View job Security review
ScopeReviewed both changed files against
This is itself a security-guard script (the fail-closed supplement for #2337 that gates the required FindingsNo security issues found. Specifically checked for the failure modes this class of change tends to introduce:
The PR's own description of the bug (bare non-zero return under |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d0b8d2c3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Last security-reviewed head: |
`verify-security-review-evidence.sh` runs under `set -euo pipefail` and
called its scope helper bare:
pr_touches_security_paths "$base_ref"
local in_scope=$?
The helper signals OUT-of-scope by RETURNING NON-ZERO. Under `set -e` a
bare call with a non-zero return aborts the shell immediately, so the
script died right there — exit 1, no message — and the "guard not
applicable" branch below it was unreachable dead code. Every out-of-scope
pull request FAILED the guard instead of being waved through.
Observed on run 31637054526 (PR #2512): the diff touches only
`docs/conventions/loop-lane/*.md`, the lane correctly SKIPPED, and
`security-review-evidence` failed beside it with a step log containing
nothing but `##[error]Process completed with exit code 1`. The empty log
is the signature — `set -e` aborts before any echo can run.
The obvious one-line fix (`|| in_scope=$?`) clears the red check but
leaves something worse, and ShellCheck says so via SC2310, which this
repo enables deliberately in `.shellcheckrc` ("catches the bash trap
where errexit silently stops working"). The helper runs `python3`, and a
crashing interpreter also exits 1 — so with out-of-scope encoded as
"returns 1", a BROKEN scope check is indistinguishable from a NEGATIVE
one, and the guard would skip itself and silently pass a pull request it
exists to check. Fail-open is the wrong direction for a security guard.
Separate the channels instead: the helper prints its verdict
(`in-scope` / `out-of-scope`) on stdout and reserves a non-zero exit for
a genuine fault. The caller consumes it through command substitution,
which keeps `set -e` live for the helper, and treats any unrecognised
verdict as a fault rather than a pass.
Verified: self-tests 6 pass / 0 fail, including a regression that models
the bare call in a separate `bash -c` process (a `( … )` subshell will
not do — bash suppresses `set -e` for the whole dynamic extent of a
command whose status is tested, so `$( … )` inside `[[ … ]]` makes the
bug unreproducible in the very harness meant to catch it), plus static
guards that fail if either older shape returns. `shellcheck --rcfile
.shellcheckrc` clean on both files, with SC2310 no longer firing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KeEvWwrK8KdQTFb5n9CrUP
When origin/$base_ref cannot be resolved, git diff exits non-zero and the scope helper now surfaces that as a fault instead of printing out-of-scope. Replace GNU-only \s in the static guard grep with [[:space:]]. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
If origin/$base_ref is missing or git diff otherwise fails, emit stderr and exit non-zero instead of treating an empty diff as out-of-scope. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
0d0b8d2 to
755c270
Compare
|
Claude finished @cursor[bot]'s task in 1m 36s —— View job Security review
ScopeThis PR now sits at HEAD
This is the fail-closed supplement for #2337 gating the required FindingsNo security issues found. Codex's P1 finding on the earlier revision — that an unresolvable Re-checked the rest of the surface from the earlier revision, still holds at this HEAD:
The fix closes the fail-open hole described in the PR body without introducing a new one, and the Codex-flagged gap in the diff-failure path is now handled correctly. |
|
Claude finished @cursor[bot]'s task in 3m 58s —— View job Code review (
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if diff.returncode != 0: | ||
| sys.stderr.write( | ||
| diff.stderr | ||
| or f"git diff --name-only origin/{base_ref}...HEAD failed (exit {diff.returncode})\n" | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
Test-coverage gap: this fail-closed branch has no regression test.
This block was added in the third commit (755c270, "fail closed when scope git diff cannot resolve base ref") specifically to close the fail-open hole flagged in review — a git diff failure (unresolvable origin/$base_ref, shallow-fetch merge-base failure, etc.) previously fell through to out-of-scope with exit 0.
Every other behavior change in this PR got a dedicated regression test in verify-security-review-evidence.sh.test.sh (the bare-call-under-set -e repro, the ||-suppression static guard, the unrecognised-verdict catch-all). This branch didn't — diffing the follow-up commit against its parent shows verify-security-review-evidence.sh.test.sh wasn't touched:
git diff 777f17c3..755c2703 -- scripts/verify-security-review-evidence.sh.test.sh
# (empty)
Nothing currently exercises "the Python helper's git diff subprocess fails" and asserts the guard fails closed (nonzero exit, not a silent out-of-scope). Given this is precisely the failure mode this block exists to prevent, and the rest of the PR's own bar is regression coverage for every fail-open path found, this seems worth closing — e.g. a case that runs pr_touches_security_paths (or an equivalent harness) against a base_ref that can't resolve, and asserts nonzero exit.
Not a blocker on an already-merged PR, just flagging for a follow-up.
| if diff.returncode != 0: | ||
| sys.stderr.write( | ||
| diff.stderr | ||
| or f"git diff --name-only origin/{base_ref}...HEAD failed (exit {diff.returncode})\n" | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
Test-coverage gap: this fail-closed branch has no regression test.
This block was added in the third commit (755c270, "fail closed when scope git diff cannot resolve base ref") specifically to close the fail-open hole @chatgpt-codex-connector flagged — a git diff failure (unresolvable origin/$base_ref, shallow-fetch merge-base failure, etc.) previously fell through to out-of-scope with exit 0.
Every other behavior change in this PR got a dedicated regression test in verify-security-review-evidence.sh.test.sh (the bare-call-under-set -e repro, the ||-suppression static guard, the unrecognised-verdict catch-all). This branch didn't: git diff 777f17c3..755c2703 -- scripts/verify-security-review-evidence.sh.test.sh is empty — the test file wasn't touched by the follow-up commit.
Nothing here currently exercises "the Python helper's git diff subprocess fails" and asserts the guard fails closed (nonzero exit, not a silent out-of-scope). Given this is precisely the failure mode the block exists to prevent, and the PR's own stated bar is regression coverage for every fail-open path found, this seems worth closing — e.g. a case in the test script that runs pr_touches_security_paths (or an equivalent harness) against a base_ref that can't resolve, and asserts nonzero exit.
Not a blocker on a merged PR, just flagging for a follow-up.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
2 participants
Summary
scripts/verify-security-review-evidence.shruns underset -euo pipefailand called its scope helper bare:The helper signals OUT-of-scope by returning non-zero. Under
set -e, a bare call with a non-zero return aborts the shell immediately — so the script died on that line, exit 1, no message, and the"diff does not touch security-relevant paths — guard not applicable"branch below was unreachable dead code.Every out-of-scope pull request FAILED the guard instead of being waved through.
Observed on run 31637054526 (PR #2512): the diff touches only
docs/conventions/loop-lane/*.md,security-review / security-reviewcorrectly SKIPPED, andsecurity-review-evidencefailed beside it with a step log containing nothing but##[error]Process completed with exit code 1. The empty log is the signature —set -eaborts before anyechocan run. Reproduced standalone:Fix — and why not the one-liner
The obvious fix is
pr_touches_security_paths "$base_ref" || in_scope=$?. It clears the red check and introduces something worse, which ShellCheck flags directly via SC2310 — a check this repo enables on purpose in.shellcheckrc("catches the bash trap where errexit silently stops working inside if/while/&&/||").The helper runs
python3, and a crashing interpreter also exits 1. With out-of-scope encoded as "returns 1", a broken scope check is indistinguishable from a negative one — the guard would skip itself and silently pass a PR it exists to check. Fail-open is the wrong direction for a security guard.So the two channels are separated instead: the helper prints its verdict (
in-scope/out-of-scope) on stdout and reserves a non-zero exit for a genuine fault. The caller consumes it via command substitution, which keepsset -elive for the helper, and treats any unrecognised verdict as a fault rather than a pass.Verification
Run in this worktree — commands I actually ran, with real output:
bash scripts/verify-security-review-evidence.sh.test.sh-> 6 pass, 0 fail,All checks passed.shellcheck --rcfile .shellcheckrc scripts/verify-security-review-evidence.sh scripts/verify-security-review-evidence.sh.test.sh-> clean, with SC2310 no longer firing on the guard (it fired on the||variant, which is how the fail-open hole was found).New regression cases:
set -ekills the script — modelled in a separatebash -cprocess deliberately. A( … )subshell will not do: bash suppressesset -efor the whole dynamic extent of a command whose status is being tested, and$( … )inside[[ … ]]is exactly that context, which made the bug unreproducible in the very harness meant to catch it. My first attempt at this test failed for that reason, not because the fix was wrong.||-suppressed call) returnsNot verified here: that the three currently-red PRs (#2512, #2504, #2499) go green. They will only pick this up once their branches carry it — the evidence workflow runs from the PR head.
Related
Third distinct defect found in this one file today, which is itself the finding: it infers another workflow's behaviour by reading that workflow's logs. #2517 (grep matched the lane action's echoed source) only became reachable once the
cursor[bot]outage was fixed; this one only became reachable once #2517 let execution past the grep.No linked issue — filing was interrupted; the detail is captured here instead.