security-review.yml is one-shot per PR: a gate that cannot fail, reporting green

[chiibitsu]

What it is

security-review.yml scans once per pull request. Every run after the first finds a marker, skips the scan entirely, and still reports success.

That is the defect class this toolkit exists to remove — a check that did not run is never a pass — sitting in the security lane, where it costs the most. Unlike the cases the toolkit already handles, this one leaves no red mark and no grey one: the check is green.

Measured on chiibitsu/nextjs-supabase-template#6, twice.

Evidence — both job logs

Run 1 — the one that actually scanned. job/103112538557, head f0d6032:

CLAUDECODE_FINDINGS: 0
Cache saved with key: claudecode-1359977084-pr-6-a500fc5ae1d9c50bf7480d6a37a01654ba8463d9

Real scan, Comment PR with findings executed, marker written.

Run 2 — a later commit on the same PR. job/103113313760, head 6a39833:

ClaudeCode has already run on PR #6 (found marker file), forcing disable to avoid false positives
ClaudeCode is disabled for this run
...
Run ClaudeCode scan → outcome=skipped
No files were found with the provided path: findings.json
Comment PR with findings → outcome=skipped

Job conclusion: success. Duration ~20 seconds.

Reproduced a third time on head 0e57315 (job/103148787642) — same skip, same green.

Why it is per-PR and not per-commit

The cache key is claudecode-<id>-pr-<number>-<sha>. The lookup matches on the pr-<number> prefix, so any run after the first on that PR finds a marker written for a different commit. Consequence: one commit is scanned per pull request lifetime. On a PR with seven commits, six are unscanned and all seven show green.

Compounding it: the workflow checks out with fetch-depth: 2, so even the run that does scan sees only the last push.

Failure scenario

A contributor opens a PR, CI scans commit 1 clean, then pushes a commit introducing a credential-handling bug or an unauthenticated route. The review check goes green in 20 seconds having read nothing. Nobody looking at the PR page can tell — the skip is visible only by opening the job log and reading past the setup noise.

Proposed fix direction

Not prescribing the implementation, but the shape:

1. Key the marker by commit, not by PR — claudecode-<id>-<head_sha> — so the dedupe still prevents double-scanning one commit while letting each new commit be scanned. This looks like the smallest correct change.
2. If a scan is deliberately skipped, the job must not report success. Emit UNKNOWN and go red, matching review.yml's existing convention (::error title=UNKNOWN (review)::…). A skipped scan is an absence; the toolkit's own rule says an absence is not a pass.
3. Consider whether fetch-depth: 2 should be 0, so the scan sees the full PR range rather than the last push.

(1) and (2) are independent — (2) is worth doing regardless, because it converts a silent green into a loud unknown even if the dedupe logic is later changed again.

Fixture

Per this repo's rule, the fix needs a known-bad fixture showing the gate fails before it counts. The natural one: a two-commit PR fixture where the second commit carries a planted finding, asserting the scan runs on both — and asserting the job is not green when a scan is skipped.

Not fixed in the observing PR

Found while running the draft-reviewer fixture procedure in nextjs-supabase-template#6. Recorded in that repo's product/audits/review-lane-adoption.md §5 and filed here, where the fix and its fixture belong.