fix(claude-review): count only reviews that ran (A2: repoint + gate flip)

[kyle-sexton]

Second half of #363's remediation — the A2 of the A1/A2 composite delivery shape. The A1 change (PR #387, merged as 7415d4e) landed the outcome composite's review-ran output and class=skipped-validation emission unreferenced; this PR repoints the review lane's outcome-composite pin at that merge SHA and flips the two consuming gates in the same commit — the pairing that cannot be split, because a gate reading review-ran against a pin that predates it evaluates false on every run and silently skips the step it guards (empirically demonstrated on PR #387's own self-review run before the A1 revert, job 93087360214).

What changes

• Pin repoint: claude-lane-outcome@b5d54bf… → claude-lane-outcome@7415d4e8e33564b7997763805583e5ad1b062dbe (the fix(claude-review): make a workflow-validation skip visible, and stop crediting it #387 merge SHA) on the review lane's outcome step. Only this pin moves — the freshness and marker-comment composite pins are unchanged by fix(claude-review): make a workflow-validation skip visible, and stop crediting it #387 and stay put, and the security/e2e lanes' outcome pins are deliberately left for the security lane: validation self-skip turns the REQUIRED check green with nothing reviewed #388 fail-closed work, where their repoint composes with the gating decision tracked there.
• Gate flip (finding 2 of claude-review: a workflow-validation skip is an undetectable silent no-review, plus a false review count and false marker copy #363): the review-count upsert and the stale-failure-comment clear now gate on review-ran == 'true' instead of review-failed == 'false'. A validation skip is "not failed" but reviewed nothing, so it no longer increments the "Claude has reviewed this PR N times" comment, no longer charges the max-reviews-per-pr budget on runs that reviewed nothing, and no longer clears a failure warning it did nothing to resolve.
• Wiring test (verifier's optional hardening, precedent: claude-security-review-fail-closed.test.cjs): new claude-review-outcome-wiring.test.cjs pins the seam from both ends — the two literal gate conditions; every consumed steps.review-outcome output declared by the composite's working-tree source; and the same check against the composite at the pinned SHA via git show (skips cleanly on shallow clones where the pinned object is unreachable, so the tree check is the always-on floor and the pin check the deep-clone guard). Validated by construction: before this PR's repoint, the two declaration checks fail on exactly the A1-window divergence; after it, all three pass.

Verification

• node --test .github/scripts/*.test.cjs — 519 pass, 0 fail (includes the three new wiring tests).
• node --test .github/actions/claude-lane-outcome/*.test.cjs — 14 pass, 0 fail.
• actionlint clean on claude-review.yml.

Related

No linked issue. For reference: #363 (finding 2 delivered here; findings 1 and 3 landed via #387), #387 (the A1 half this repoints to), #388 (tracks the security lane's fail-closed decision and its own pin repoint).

🤖 Generated with Claude Code

https://claude.ai/code/session_01HCJfVqDNYt92YRyvKUMgYW

[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.

[claude]

Claude finished @kyle-sexton's task in 2m 26s —— View job

---

Review: PR #389 — count only reviews that ran (A2: repoint + gate flip)

No inline findings — the change is narrowly scoped and I confirmed the two things that actually matter for correctness:

1. Pin repoint is sound. git show 7415d4e8e33564b7997763805583e5ad1b062dbe:.github/actions/claude-lane-outcome/action.yml confirms the composite at that exact pinned SHA already declares the review-ran output the new gate reads — so the "gate reads an output the pin predates" failure mode this PR exists to close (per PR #387's own self-review incident) cannot recur here. Only the outcome step's pin moved (claude-review.yml#L804); the freshness and marker-comment pins correctly stay at b5d54bf since #387 didn't touch those composites.

2. Gate flip logic is correct by the composite's own contract. Per claude-lane-outcome/action.yml, review-ran=true only on success-with-execution-evidence, while a validation skip yields review-failed=false and review-ran=false — so review-failed == 'false' and review-ran == 'true' diverge exactly on the validation-skip case, which is the bug being fixed. The two flipped gates (claude-review.yml#L847, #L864) now correctly exclude that case; the still-review-failed-gated "Comment on genuine review failure" step (#L823) was correctly left alone since it should still fire on genuine failures. I also checked the stale comment above "Report review outcome" (#L790-798), which wasn't touched by this diff and still describes "review-failed empty ⇒ both marker-comment steps skip" — that statement remains true after the flip, since a skipped outcome step leaves all its outputs (including review-ran) empty, so it's not stale.

3. New wiring test (claude-review-outcome-wiring.test.cjs) is well-constructed. Verified independently by reading the source, not just trusting the PR description: the declaredOutputs() slice between \noutputs:/\nruns: is safe (both keys appear exactly once at top level in the composite's action.yml), the stepSource() helper's step-name/indentation assumptions match the actual file (6-space - name:, 8-space if:), and the pinned-revision regex has exactly one claude-lane-outcome@ match to anchor on. The shallow-clone skip path is a reasonable, explicitly-commented design choice given ci.yml's node --test .github/scripts/*.test.cjs glob will already pick up this file with no wiring changes needed there.

What I could not independently verify: I don't have node/actionlint execution available in this sandbox, so the PR body's claimed "519 pass, 0 fail" and "actionlint clean" are author-claimed and unverified by me — I instead verified the underlying logic by direct source/git show inspection rather than by running the suite.

CLAUDE.md alignment: no security-lane-relevant surface touched (no trigger/permissions/pin-layer/secret changes beyond the documented SHA repoint), consistent with this repo's review-lane split — left to the security lane, which has nothing to flag here anyway.

Nothing to fix. This is a precise, well-justified two-line-logic change with matching test coverage.

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Review .github/workflows/claude-review.yml changes
• Review .github/scripts/claude-review-outcome-wiring.test.cjs
• Check for injection surfaces, token/permission handling, gating logic flaws
• Cross-check against zizmor's coverage (supply-chain, triggers, permissions, template injection) to avoid duplicate reporting
• Post final findings

No security issues found. This PR is a scoped, low-risk change: it repoints one uses: pin and flips two if: gate conditions in claude-review.yml, plus adds a pure wiring test.

What I checked, beyond zizmor's lane (unpinned actions, dangerous triggers, permission grants, template injection in run: blocks — not re-litigated here):

• Gate logic direction is strictly tightening, not loosening. review-failed == 'false' → review-ran == 'true' narrows the condition under which the review-count upsert and stale-comment clear run — it can no longer fire on a validation-skip outcome. No new path was opened for either step to run in a case it couldn't reach before; both remain continue-on-error: true housekeeping with no write beyond the PR comment/count, gated on the trusted composite's own outputs (not on PR-controlled text).
• Pin repoint is a same-repo, already-merged commit, not an external or attacker-influenced ref: 7415d4e8e33564b7997763805583e5ad1b062dbe resolves locally to fix(claude-review): make a workflow-validation skip visible, and stop crediting it (#387), and it's a full 40-char SHA. This is a within-repo history move, not a new external supply-chain dependency, so I'm leaving the pin-freshness/format concern to zizmor's static lane as instructed.
• New test file (claude-review-outcome-wiring.test.cjs) has no unsafe execution surface. It calls execFileSync("git", [...]) with a static argv array (no shell: true, no string interpolation into a shell), and the pinned SHA it interpolates into the git show <sha>:<path> arg is first constrained by a /[0-9a-f]{40}/ regex match against the workflow's own pinned uses: line before use — not taken from PR-controlled input. stdio: ["ignore", "pipe", "ignore"] suppresses stderr rather than leaking it, and a missing/unreachable object is caught and turns into a clean t.skip, not a crash or a bypassed assertion.
• No permissions:, trigger (on:), or secrets handling changed anywhere in this diff — the job-level permissions block, the pull_request-only trigger tripwire, and secret usage are all untouched.
• No PR-authored content is newly read, executed, or interpolated. Both changed steps only branch on outputs already produced by the trusted claude-lane-outcome composite (itself unchanged by this PR beyond the pin move); nothing here reaches into github.event.pull_request.title/body or other attacker-controlled fields.

Nothing to flag at CRITICAL, IMPORTANT, or SUGGESTION severity.
· Branch: fix/review-outcome-repoint

[github-actions]

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