fix(claude-review): surface real review failures instead of a silent green

[kyle-sexton]

Summary

Every claude-review run across all consumer repos has been silently reporting green for the last 27+ hours while the underlying Claude Agent SDK call fails with is_error: true (0 cost, 1 turn, <1s). No red check, no warning, just a permanently stuck "I'll analyze this and get back to you" placeholder comment on every PR.

Root cause of the invisibility: our pinned anthropics/claude-code-action version (v1.0.165→166) predates upstream fix #1496 for #1495 ("Action reports success when result is is_error:true"), which first shipped in v1.0.172. Before that fix, steps.claude-review.outcome was always success regardless of the SDK result, so our existing "Report review outcome" step's failure branch was dead code.

This PR does not fix why the SDK call itself is failing — that's bisected to an Anthropic-account-side condition on the CLAUDE_CODE_OAUTH_TOKEN credential (two independent tokens broke simultaneously with no local change; 27+ hours of continuous failure rules out a simple 5-hour rate-limit reset). That requires checking the Anthropic Console, which isn't something this PR can address. This PR makes that failure visible instead of silently green, which is the actual gap that let it run unnoticed for over a day.

Changes

• Bump anthropics/claude-code-action pin from v1.0.166 → v1.0.174 (past the #1495 fix; also picks up an unrelated ghu_ token-redaction hardening from the same release range — reviewed every intermediate commit v1.0.172→v1.0.175 via gh api compare, nothing else in range).
• Rewrite "Report review outcome": on a real failure, read the action's own execution_file output (no show_full_output needed) for the last SDK result, emit a real ::error:: annotation (was ::warning::, and never fired), and expose review_failed/review_detail step outputs.
• New "Comment on genuine review failure" step: posts (sticky, via --edit-last --create-if-none) an explicit PR comment labeling any lingering placeholder as an incomplete infra failure — not "no findings" — with the run URL and last SDK result.
• continue-on-error: true stays on the review step for now — this is a visibility fix, not a new merge gate. Left as a deliberate follow-up decision whether a persistent failure should eventually block.

Verification

• actionlint — clean.
• zizmor — one pre-existing artipacked medium finding, confirmed unchanged from main (unrelated to this change, already accepted by design per the checkout step's own comment).
• Dry-ran the new shell logic locally against three cases (clean review / is_error with execution file / failure with missing execution file) — all three branch and format correctly.
• Not yet verified against a live GitHub Actions run — the account-side failure currently affecting every repo means the very next PR push anywhere downstream will exercise the failure path for real once this rolls out.

Scope notes

• Did not bundle the still-unconsumed runner self-hosted-routing input (from 4dbb0df) — that's a separate, deliberately staged multi-repo rollout per its own commit message.
• Follow-up once merged: bump the pinned SHA in each of the 6 caller repos (dotfiles, standards, claude-code-plugins, provisioning, github-iac, medley).

🤖 Generated with Claude Code

https://claude.ai/code/session_01QSCqyt1o7XnDXCgyp8ejKT

[claude]

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

---

PR Review

• Gather context and understand the request
• Read the changed workflow file
• Analyze the diff for correctness, security, and CLAUDE.md alignment
• Post review feedback

---

Summary: Well-scoped, correctly reasoned fix. The diagnosis is sound, the action bump is necessary and sufficient, and the new steps are defensively coded. A few minor correctness edge cases worth flagging, but nothing blocking.

---

Security

SHA pinning — ✓
The bump from f87768c6d25f92ae6efa7175e223ef77d4cbf97f # v1.0.166 to 12531344451323133b0493233c759991ac61da12 # v1.0.174 maintains the full SHA + version-comment pattern required by CLAUDE.md.

EXECUTION_FILE path — ✓
The execution file path comes from steps.claude-review.outputs.execution_file, which is action-controlled (not user-controlled), so path-traversal risk is negligible. The [ -f "$EXECUTION_FILE" ] guard is correct.

REVIEW_DETAIL → GITHUB_OUTPUT → env → PR comment body
jq -c produces compact single-line JSON, so the echo "review_detail=$detail" write to GITHUB_OUTPUT is safe against newline injection. However, backticks in JSON string values survive jq -c as literal backticks (JSON doesn't escape them), so `$REVIEW_DETAIL` in the markdown body could produce broken inline-code formatting if the SDK result contains a backtick. This is a cosmetic concern, not a security concern — the SDK result is Anthropic-controlled, not PR-author-controlled.

--edit-last only touches bot comments — ✓
gh pr comment --edit-last edits the most recent comment by the authenticated token owner (github-actions[bot]), not any human's comment. The concern about accidentally overwriting a human's comment doesn't apply here.

Permissions / security model unchanged — ✓
No new permissions. Tripwire step untouched. display_report/show_full_output remain off.

---

Correctness

jq -c '.[-1] // empty' — ✓
Safely handles empty arrays (returns nothing → empty string) and malformed files (2>/dev/null || true). The fallback chain is correct.

skipped outcome produces a spurious failure comment
If the claude-review step is skipped (e.g., the reject-privileged-triggers tripwire fires and kills the job — checkout and claude-review both skip, then review-outcome runs via if: always()), REVIEW_OUTCOME will be skipped, which falls through to the failure path and sets review_failed=true. A failure comment would be posted on the PR even though the actual issue was a wrong-trigger misconfiguration.

In practice this only fires if a consumer incorrectly uses pull_request_target/workflow_run, which is already an error condition, so the spurious comment is noise on top of a hard failure. Still worth noting. A minimal fix would be:

-          if [ "$REVIEW_OUTCOME" = "success" ]; then
+          if [ "$REVIEW_OUTCOME" = "success" ] || [ "$REVIEW_OUTCOME" = "skipped" ]; then

...though that arguably buries the "real" outcomes. Alternatively, handle skipped separately with a different message.

Fix this →

cancelled outcome also triggers failure comment
Same issue: a cancelled job would produce review_failed=true and post a comment labeling the run as an "infrastructure failure." Noisy if the job is cancelled deliberately (e.g., concurrency group cancellation on a new push). The cancel case may be acceptable given the cancel-in-progress: true concurrency config — a cancelled review and a new push would immediately start a fresh run — but it's worth being explicit about.

review_detail output when review_failed=true but detail-not-set
If the script exits before echo "review_detail=$detail" (not currently possible given the flow, but defensive-programming note), the Comment on genuine review failure step would render `(empty)`. Not a real risk with the current code, just a theoretical gap.

Code comment "never twice for the same head SHA thanks to --edit-last"
Line 222–224: The comment slightly overstates --edit-last's guarantee. --edit-last gives you one comment (updates-in-place), not SHA-level deduplication. On a second run for the same SHA, it correctly edits the existing failure comment, but the "same head SHA" framing isn't the mechanism — the mechanism is "single sticky bot comment." Minor documentation nit.

---

CLAUDE.md alignment

All load-bearing rules are preserved:

• pull_request only / privileged-trigger tripwire: untouched ✓
• Full SHA pins with version comment: maintained ✓
• Least-privilege permissions: no new grants ✓
• display_report/show_full_output off: unchanged ✓
• No untrusted checkout before the action step: unchanged ✓

---

Overall

The skipped/cancelled outcome edge cases are the only actionable findings. Both are low-likelihood in normal operation (skipped requires a wrong-trigger misconfiguration; cancelled is the concurrency-group preempt path that immediately retries). The core change — bumping the action past #1496 and surfacing the failure via ::error:: annotation + sticky PR comment — is correct, minimal, and the right fix for the described problem.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 61196e02c3

ℹ️ 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 3m 45s —— View job

---

Claude finished re-reviewing after the follow-up commits —— View job

---

Re-review (commits 05a8b3e + b6c7033)

• Gather context and understand the request
• Read the full diff against origin/main
• Review the follow-up commits addressing prior findings
• Post updated review feedback

---

The two follow-up commits address every actionable finding from the first pass and both Codex P1/P2 items. Summary of what changed and what remains.

---

Codex P1 — SDK result sanitization ✓

05a8b3e replaced the raw SDKResultMessage dump with a jq projection of structured-only metadata fields:

$last | {subtype, is_error, num_turns, duration_ms, total_cost_usd}

No result field, no model-authored free text. CLAUDE.md's display_report/show_full_output-off invariant is preserved. ✓

---

Codex P1 — Fork PR guard ✓

Line 246 adds:

github.event.pull_request.head.repo.full_name == github.repository

Fork-triggered runs (read-only token, no secrets) correctly skip the comment step. The ::error:: annotation still fires — a write-free signal in the job log — which is appropriate. ✓

---

Codex P2 — Marker-based comment targeting ✓

--edit-last is gone. b6c7033 targets the specific `` marker comment by ID via the REST API, creating it only when absent:

comment_id=$(gh api "repos/$REPO/issues/$PR_NUMBER/comments" --paginate |
  jq -r --arg marker "$marker" '
    [.[] | select(.user.login == "github-actions[bot]" and (.body | startswith($marker)))]
    | last | .id // empty
  ')

if [ -n "$comment_id" ]; then
  gh api "repos/$REPO/issues/comments/$comment_id" -X PATCH -f "body=$body" --silent
else
  gh pr comment "$PR_NUMBER" --body "$body"
fi

No unrelated bot comment can be clobbered. The sticky-update-or-create semantics are correct. ✓

---

Remaining: skipped / cancelled still map to review_failed=true

This was flagged in the first review and is still present. Both skipped (tripwire fires, killing earlier steps) and cancelled (concurrency preemption) fall through to the failure path in line 196–231.

• cancelled: The comment is posted, but the immediately-following new run updates the same marker comment, so it's self-healing. Low noise risk.
• skipped: Posts a comment saying "infrastructure failure, not a review verdict" — technically accurate (the review didn't run), but the consumer may find the label confusing when the real cause is a misconfigured trigger. The job log already shows a hard failure from the tripwire, so the added PR comment is redundant noise in that case.

A guard like this would eliminate the spurious skipped comment without affecting the failure path:

-          if [ "$REVIEW_OUTCOME" = "success" ]; then
+          if [ "$REVIEW_OUTCOME" = "success" ] || [ "$REVIEW_OUTCOME" = "skipped" ]; then

But given skipped only occurs during a consumer misconfiguration that already hard-fails the job, this is low-priority. Not blocking.

---

Minor: --silent suppresses PATCH errors without a fallback

Line 274:

gh api "repos/$REPO/issues/comments/$comment_id" -X PATCH -f "body=$body" --silent

If the PATCH fails (comment deleted between lookup and update, or a transient API error), it fails silently with no fallback to gh pr comment. Since the ::error:: annotation is the primary signal and the comment is supplementary, this is acceptable. Worth being aware of. Not blocking.

---

CLAUDE.md alignment

All load-bearing rules remain intact:

• pull_request only / privileged-trigger tripwire: untouched ✓
• SHA pin + version comment (@<40-char-sha> # vX.Y.Z): maintained ✓
• Least-privilege permissions: no new grants ✓
• display_report/show_full_output off: unchanged ✓
• No untrusted checkout before the action step: unchanged ✓

---

Overall

The three follow-up commits cleanly close every actionable finding from the first review and both Codex P1/P2 reports. The core change — action bump past #1496 + ::error:: annotation + marker-based sticky PR comment — is correct and minimal. The skipped edge case and the silent PATCH are noted but not blocking. Approved to merge from a code-review perspective.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b6c70331de

ℹ️ 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 1m 38s —— View job

---

Claude finished re-reviewing after commit 4ab4f71 — View job

---

Re-review (commit 4ab4f71)

• Gather context: read the full PR diff
• Review latest commit — stale failure marker clearing on success
• Check remaining open findings from prior reviews
• Post review feedback

---

What this commit does

Adds a symmetric success-path step ("Clear stale failure comment after successful review") to delete the `` marker comment once a retry succeeds, addressing the Codex P2 finding that the failure-only gate left stale warning comments on the PR indefinitely.

---

Correctness — ✓

review_failed == 'false' gate is correct, not a double-negative
review_failed is explicitly set to 'false' in "Report review outcome" only when REVIEW_OUTCOME = success. For skipped/cancelled/failure, it's set to 'true'. If "Report review outcome" is itself skipped (hard job cancellation), its outputs are unset — 'false' evaluates to false, so "Clear stale" correctly does nothing. This is safer than != 'true'.

Marker lookup is consistent with the failure step — ✓
Same --paginate, same github-actions[bot] + marker filter, same last | .id pattern as b6c7033. The no-op case (comment absent) exits cleanly. ✓

Fork guard is consistent with the failure step — ✓
github.event.pull_request.head.repo.full_name == github.repository check is present and identical to the failure step. ✓

--silent on DELETE, same as PATCH, no || true
If the marker comment is deleted between the lookup and the DELETE (a very narrow race), gh api DELETE returns a 404 non-zero exit and the step fails. The same issue was noted on the failure step's PATCH in the last review and judged acceptable there — the ::error:: annotation is the primary signal and the comment is supplementary. Same verdict here: acceptable, worth being aware of.

---

Remaining open: skipped / cancelled → review_failed=true

Previously flagged twice (first review, second review), not addressed by this commit and not claimed to be. Quick restatement for completeness:

• REVIEW_OUTCOME=cancelled → failure comment posted; self-healing because the next run (concurrency group preemption always triggers a new push) immediately updates the same marker comment. Acceptable.
• REVIEW_OUTCOME=skipped → failure comment posted with "infrastructure failure" framing; the real cause (wrong-trigger misconfiguration) is already surfaced as a hard failure in the job log. Redundant but not harmful.

Not blocking.

---

CLAUDE.md alignment — ✓

All load-bearing rules remain intact:

• SHA pin + version comment: unchanged ✓
• No new permissions ✓
• No new secret access ✓
• display_report/show_full_output off: unchanged ✓
• Tripwire and fork-PR safety model: unchanged ✓

---

Overall

Commit 4ab4f71 is a clean, minimal close of the last open Codex finding. The success/failure step pair now has symmetric behavior — the marker comment is created-or-updated on failure and deleted on success — with the same fork guard and marker-targeting pattern in both paths. All four prior review findings that were actionable have been addressed across the four commits. Approved to merge.

[claude]

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

---

Claude finished re-reviewing after commit 715498c —

---

Re-review (commit 715498c)

• Gather context: read the full diff and latest commit
• Review for correctness, security, and CLAUDE.md alignment
• Post review feedback

---

What this commit does

Drops a duplicate "Clear stale failure comment" step introduced during the self-merge in c8df011. Kyle had pushed a local copy (3c78233) and the remote already had the superior version (4ab4f71, previously reviewed). The latest commit keeps the upstream one (fork-guarded, marker-scoped DELETE) and removes the 21-line duplicate.

The removal is correct. The retained step at L286–L306 has the proper fork guard (github.event.pull_request.head.repo.full_name == github.repository), the same marker-based ID lookup, and issues a DELETE rather than a PATCH — the right semantics for clearing a stale warning.

---

Open findings from prior reviews

Finding	Status
P1: Sanitize SDK result before publishing	✅ Fixed in 05a8b3e
P1: Skip failure comment on fork/read-only token	✅ Fixed in b6c7033
P2: Target marker comment by ID, not --edit-last	✅ Fixed in b6c7033
P2: Clear stale comment on successful retry	✅ Fixed in 4ab4f71, duplicate resolved in 715498c
skipped/cancelled outcome → review_failed=true	⚠️ Unchanged, low priority (see below)
--silent on PATCH/DELETE, no fallback	⚠️ Unchanged, acceptable

skipped/cancelled edge case (previously flagged twice, still present): REVIEW_OUTCOME=skipped falls through to the failure path and posts a "infrastructure failure" PR comment. This only happens when the reject-privileged-triggers tripwire fires (i.e., a consumer already has a broken trigger), so it's noise on top of a hard failure — not a normal-operation concern. REVIEW_OUTCOME=cancelled posts a comment that gets immediately overwritten by the next push's run. Neither is blocking; both are accepted at low priority.

---

CLAUDE.md alignment — ✓

All load-bearing rules remain intact after this commit:

• SHA pin + version comment (@12531344451323133b0493233c759991ac61da12 # v1.0.174): maintained ✓
• pull_request only / tripwire: untouched ✓
• Least-privilege permissions: no new grants ✓
• display_report/show_full_output off: unchanged ✓
• No untrusted checkout before action step: unchanged ✓

---

Overall

Commit 715498c is a clean merge-artifact cleanup with no new concerns. The PR as a whole — five substantive commits plus two merge/cleanup commits — has addressed every actionable finding from three prior review rounds. The action bump past #1496 and the failure-surfacing logic (annotation + marker comment + stale-comment clearing) are correct and minimal. Approved to merge.