fix(security-evidence): decide from the lane's declared verdict, not its log

[kyle-sexton]

Closes #2541

Summary

The security-evidence guard inferred a verdict the lane had already computed, through two channels that were wrong in the same way.

It downloaded the lane's job log and grepped it. The lane's Report review outcome step is an inline github-script whose SOURCE is echoed into that same log and contains the skip phrases as string literals, so the grep matched every successful in-scope pull request and reddened exactly the ones the guard exists to approve (#2517). Anchoring to ##[warning]/##[error] lines removed the false positive but left the guard coupled to log text nothing upstream pins.

It also re-derived the PR's security scope in its own Python fnmatch implementation while the lane matches with git check-ignore — two matchers that disagree on exactly the patterns that distinguish them, and where the set -e scope-verdict defect lived.

Fix

Both channels are replaced by the lane's declared outputs (melodic-software/ci-workflows#461), passed in from needs.security-review:

lane says	guard does
job skipped / cancelled	not applicable
job not success	defer to the job's own red
relevant: false	out of scope, not applicable
review-ran: true	evidence OK
review-ran: false, review-failed: true	defer — the lane rules this GREEN on purpose so a provider outage cannot lock every merge; the guard says loudly that nothing was reviewed and does not overturn it
review-ran: false, review-failed: false	FAIL — the action skipped itself; merging the caller change clears it, a re-run cannot
no verdict at all	FAIL, unless the live head has moved — a run retired as superseded is the one legitimate cause, and it is identified by comparing the live head against the event head rather than assumed
LANE_RESULT empty	FAIL — the guard is not wired to the lane, so it can determine nothing

The guard is now coupled to the pin: it fails closed when the outputs are absent at an unmoved head, so re-pinning BACKWARDS past the release carrying them turns it red rather than quietly blind. The caller's pin is bumped in this PR for exactly that reason.

actions: read is gone with the log read; pull-requests: read covers the one remaining API call.

Verification

• scripts/verify-security-review-evidence.sh.test.sh rewritten to EXECUTE the guard against environment fixtures in an env -i child shell, instead of re-implementing its regexes inside the harness and asserting on the copies — the shape that let the tests agree with a guard that was failing every pull request. 20 cases, all pass locally, covering every row of the table above plus two static checks (no log read, no second paths matcher).
• shellcheck --rcfile .shellcheckrc clean on both scripts, including the deliberately-enabled SC2310: the absent-verdict classifier reports on stdout with exit reserved for faults, the same verdict/status split the scope check needed.
• The tests now run in CI, in the guard's own job, ahead of the guard. They ran nowhere before.

Related

• security-evidence guard fails every successful in-scope PR by matching the lane's echoed script source #2517 — the log-grep false positive this removes the possibility of
• ci(security-review): the required security-review check reported pass in 16s on #2287 while both reviewer jobs failed — a required gate that can fail open #2337 — the false-pass class the guard exists to catch
• feat(claude-security-review): declare the lane's execution verdict as workflow outputs ci-workflows#461 — the declared outputs this consumes

[cursor]

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.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1351f780ee

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 5m 40s —— View job

---

Code review — /review:code-review

• Gather context (diff vs origin/main, workflow YAML, guard script, self-test harness, REVIEW.md/CLAUDE.md)
• Run /review:code-review procedure (correctness/maintainability lane — security scope deferred to claude-security-review, per this repo's REVIEW.md)
• Post findings

This is a well-scoped rewrite: it replaces two independently-wrong verdict channels (log-grep, local fnmatch scope re-derivation) with the lane's declared workflow_call outputs, and the guard's control flow (case "${LANE_RESULT}" → relevance → review-ran/review-failed → absent-verdict classifier) matches the truth table in the PR description exactly, including the fail-closed defaults on every unrecognised branch. Traced the main()/classify_absent_verdict() logic and the run_guard test harness (the env -i … bash -c 'source "$1"; live_head_sha() { … }; main' substitution trick) by hand line-by-line — both are internally consistent and the harness genuinely executes the guard rather than re-asserting copies of its regexes, which is the exact failure mode (#2517) this rewrite is fixing.

Two findings posted inline:

1. .local/state/gh/device-id — this is new to git history as of this branch (not present on origin/main) and reads as accidental local gh CLI device-flow state, unrelated to the PR's stated purpose. Not covered by .gitignore. Recommend git rm before merge.
2. scripts/verify-security-review-evidence.sh#L13 (and L105) — the script cites ci-workflows#460 for the declared-output contract in two places, while the workflow file's comment and this PR's own description both cite ci-workflows#461 for what reads as the same thing. One of the two numbers is likely wrong; worth reconciling since line 105's text is exactly the breadcrumb an on-call reader follows when the guard fires.

Not independently verified: I could not execute bash scripts/verify-security-review-evidence.sh.test.sh or shellcheck --rcfile .shellcheckrc in this sandbox — Bash execution beyond a few read-only git commands required approval that wasn't available in this session. The PR's claims of "20 cases, all pass locally" and a clean shellcheck run are author-stated and unverified by me; static reading of both scripts found no syntax or logic issues, but I'd flag this gap so a run with Bash access (or CI itself) is the actual confirmation.

No other correctness, error-handling, or test-coverage issues found in the diff. Security-scoped concerns (if any) are intentionally left to the claude-security-review lane per this repo's REVIEW.md.

[github-actions]

Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @kyle-sexton's task in 3m 34s —— View job

---

Code review — /review:code-review

• Gather context (diff vs origin/main via gh pr diff, workflow YAML, guard script, self-test harness)
• Run /review:code-review procedure (correctness/maintainability lane — security scope deferred to claude-security-review, per this repo's REVIEW.md)
• Post findings

Re-reviewed at the current head (ed04931), which is a synchronize push that landed two follow-up commits since my last pass. Both findings from that earlier review are now resolved:

1. .local/state/gh/device-id — no longer present. Confirmed absent from HEAD and from every commit on this branch's history.
2. ci-workflows#460 vs #461 citation drift — resolved by ed04931 "name the release, not a cross-repo issue ref": both citation sites (scripts/verify-security-review-evidence.sh lines 13 and 105) now point at ci-workflows v0.14.2 instead of an issue number, matching the pin bump on .github/workflows/claude-security-review.yml line 34. Good fix — a version is also more durable than an issue-number breadcrumb.

I re-traced the guard's full state machine against the PR description's truth table by hand, this pass focused on the two new commits and the interaction between .github/workflows/claude-security-review.yml and scripts/verify-security-review-evidence.sh:

• security-review-evidence's if: always() && ... != 'cancelled' (workflow L87-90) already filters the cancelled case at the job level, so the script's own cancelled branch (script L138-141) is currently unreachable in CI (and untested — the new suite covers skipped/failure/empty but not cancelled). Harmless defense-in-depth, not a bug — not flagging as a blocking finding, just noting it's dead in practice.
• classify_absent_verdict's fault/verdict split (local head declared separately from head="$(live_head_sha)", L93-94) correctly avoids the classic local var=$(cmd) pitfall that would swallow set -e — a real gh api fault (network/auth) still aborts the script rather than being silently absorbed into the "blind" verdict.
• Every row of the truth table (skip-listed actor, LANE_RESULT ∈ {skipped, cancelled, "", success, other}, LANE_RELEVANT=false, LANE_REVIEW_RAN ∈ {true, false×{failed,not-failed}, absent}, retired-vs-blind on absent verdict) traces to the exit code and message the description claims. No fallthrough, no unreachable-but-assumed-safe branch, no case where an unrecognized value reads as a pass.
• Grepped the diff for leftover references to the removed GITHUB_RUN_ID/MIN_REVIEW_SECONDS/PATHS_FILE/pr_touches_security_paths — nothing orphaned outside the test file's own negative-assertion string.

No new correctness, error-handling, or maintainability issues found. Security-scoped concerns are intentionally left to the claude-security-review lane per REVIEW.md.

Not independently verified: as in my prior pass, I could not execute bash scripts/verify-security-review-evidence.sh.test.sh or shellcheck --rcfile .shellcheckrc in this sandbox — both required Bash approval that wasn't available in this session. The "20 cases, all pass locally" and clean-shellcheck claims remain author-stated and unverified by me; static reading found no syntax or logic issues in either script.
· branch fix/evidence-guard-declared-outputs

[claude]

Claude finished @kyle-sexton's task in 3m 11s —— View job

---

Code review — /review:code-review

• Gather context (diff vs origin/main, workflow YAML, guard script, self-test harness)
• Run /review:code-review procedure (correctness/maintainability lane — security scope deferred to claude-security-review, per this repo's REVIEW.md)
• Post findings

Re-reviewed at the current head (3aebf73), a synchronize push consisting of one docs-only commit since my prior pass (ed04931). No new findings.

Both previously-flagged issues remain resolved:

1. .local/state/gh/device-id — confirmed absent from HEAD and from every commit's tree (git grep/git ls-tree across history turn up nothing).
2. ci-workflows#460 vs #461 citation drift — and the newest commit (3aebf73) finishes the job started in ed04931: the workflow comment now also names the release (ci-workflows v0.14.2) instead of a PR number, so the header comment, the caller-workflow comment, and the guard's own production error message all cite the same durable reference.

Full re-trace of the state machine (main() → LANE_RESULT case → LANE_RELEVANT → LANE_REVIEW_RAN/LANE_REVIEW_FAILED → classify_absent_verdict()) against the PR description's truth table, on the actual current file contents:

• scripts/verify-security-review-evidence.sh#L88-L108 — classify_absent_verdict correctly avoids the classic local var=$(cmd) pitfall (local head declared separately from head="$(live_head_sha)" at L93-94), so a genuine gh api fault (network/auth) still trips set -e and aborts rather than being silently absorbed into the "blind" verdict.
• Every row of the PR's table — skip-listed actor, LANE_RESULT ∈ {skipped, cancelled, "", success, other}, LANE_RELEVANT=false, LANE_REVIEW_RAN ∈ {true, false×{failed,not-failed}, absent}, retired-vs-blind on absent verdict — traces to the exit code and message claimed, with no fallthrough and no unrecognised value reading as a pass.
• Workflow permissions (contents: read, pull-requests: read) match what the script actually uses (gh api repos/.../pulls/N); actions: read is genuinely gone with the log read.
• The if: always() && ... != 'cancelled' job condition still makes the script's own cancelled branch unreachable in CI (noted in my prior pass, harmless defense-in-depth, not a bug — not re-flagging).

Not independently verified (same sandbox limitation as my prior two passes): I still could not execute bash scripts/verify-security-review-evidence.sh.test.sh or shellcheck --rcfile .shellcheckrc — both require Bash approval unavailable in this session. Static reading of both scripts found no syntax or logic issues; the "20 cases, all pass locally" and clean-shellcheck claims remain author-stated. CI's own new "Run security-review evidence guard tests" step (workflow L110-111) is the actual confirmation once this runs.

No correctness, error-handling, or maintainability issues found. Security-scoped concerns are intentionally left to the claude-security-review lane per REVIEW.md.
· branch fix/evidence-guard-declared-outputs