[bawoodruff]

Addresses items 1 and 2 of #41. Ported from the Cortexa-LLC/mcp implementation, which has been running these across 8+ live reviews on 5 PRs — the reporter offered the port back and it is better evidence than anything I'd write fresh.

Items 3–5 (the shepherd's model of this reviewer) are a separate theme and follow in their own PR.

Item 1 — --output-format text made a max-turns exhaustion undiagnosable

text emits the final assistant message and nothing else. A run that ends without a final message exits non-zero with both streams empty. PR #35's stderr capture cannot reach this case — there is no stderr to capture. The observed shape was ~3 minutes of work, exit 1, and nothing to explain it.

--output-format json always carries subtype / is_error / num_turns. Now:

• review text comes from .result
• a non-zero exit and an is_error envelope are independent failure signals — either is fatal
• the envelope is logged on every run, healthy or not, so the turn budget is visible before it becomes a problem
• unparseable json defaults is_error to true, so it fails closed

Item 2 — --max-turns 15 was too low and failed silently

Four real runs, same reviewer persona:

lines changed	turns	outcome
584	24	approved
2,704	35	posted a review
848	41	hit the cap
989	41	hit the cap

Turn count tracks how far the reviewer wanders, not diff size — the largest diff used the fewest turns. And two of four stopped exactly at the cap, so the true ceiling is unknown and a snug cap is the wrong shape.

Raised to 120, treated as a runaway guard rather than a budget; timeout-minutes 20 → 30 since the two bound the same run (~6s/turn puts 120 turns near 12 minutes, inside a 30-minute wall clock).

This is the item with teeth: exceeding the cap yields no review at all, not a short one, and the review job is continue-on-error — so the reviewer could have stopped posting on every PR while everything stayed green.

Verification

The invocation and failure-handling block was extracted from the workflow and driven with a stubbed CLI across six envelope shapes:

Case	Result
healthy review	exit 0, body extracted from .result
max-turns exhaustion	exit 1, "Reviewer hit the turn cap", turn count named
error_during_execution	exit 1, same treatment
exit 0 with is_error=true	exit 1 — both signals honored independently
unparseable json	exit 1, fails closed
healthy envelope, .result null	exit 1, caught before posting

Plus: YAML parses, bash -n on the extracted run block, actionlint clean, and the backslash-continuation guard from #40 passes.

Note

No version bump — .github/ only, nothing under plugin/, which the new gate from #44 confirms is the right call.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WDPKxAK1dJvyn3gXaGfsFc

[cortexa-llc-reviewer]

Review Summary

• Files reviewed: .github/workflows/claude-pr-review.yml
• Overall verdict: APPROVE

Critical Issues

None.

Major Issues

None.

Minor Issues

• .github/workflows/claude-pr-review.yml:442 — RAW_JSON="$(mktemp)" is not added to the trap 'rm -f ...' EXIT that covers SYSTEM_FILE (line 248). Both REVIEW_RAW and CLAUDE_ERR (line ~423) have the same omission. In the containerized runner this is harmless since the process environment is ephemeral, but it's inconsistent with the existing SYSTEM_FILE pattern. Fix: extend the trap to trap 'rm -f "$SYSTEM_FILE" "$RAW_JSON" "$CLAUDE_ERR" "$REVIEW_RAW" "$REVIEW_BODY"' EXIT (or set it after all mktemps are created).

• .github/workflows/claude-pr-review.yml:499 — the error path does cat "$RAW_JSON" which dumps the entire JSON envelope, including .result, to the Actions log. The comment at line 484 says "Never log the review text — that gets posted." On a failure with partial output, .result can be non-empty. In a failure case no review is posted so there's no real confusion, but the comment and the behaviour are in tension. Suppress .result in the error dump, or note the exception in the comment.

• .github/workflows/claude-pr-review.yml:506 — the subtype strings error_max_turns and error_during_execution are undocumented CLI internals (per the PR's own prose on --max-turns). If the CLI emits a different string (e.g. max_turns_reached), the specific annotation fires on the *) catch-all instead, which is still a loud failure — just with a less actionable message. Not a silent failure. Worth noting in the comment alongside the --max-turns undocumented-flag warning so future maintainers know both names may drift.

• .github/workflows/claude-pr-review.yml:104 — the comment "At the observed ~6s/turn, 120 turns is ~12 minutes, well inside this" understates tail latency. Tool-call-heavy turns (filesystem reads, gh calls) can take 20–30 s; 120 of them at 25 s/turn is 50 minutes, well outside 30. The timeout-minutes wall clock is the real bound and does the right thing regardless, but the comment's arithmetic should acknowledge P99, not just the median, to avoid giving future maintainers false confidence that timeout-minutes has headroom to spare.

Security Findings

None.

Positive Observations

• The core fix is correct and well-motivated: switching to --output-format json and capturing CLAUDE_RC independently of is_error closes a real silent-failure mode that had already been observed in production.
• The fail-closed default for unparseable JSON (|| echo true) is the right choice.
• Structured ::group:: / ::endgroup:: blocks are a clean upgrade over bare cat dumps.
• The null-result guard ([ "$(cat "$REVIEW_RAW")" = "null" ]) correctly handles the jq -r behaviour when .result is absent or JSON null.
• Turn-cap reasoning (treat as runaway guard, not budget; size from observed P99 at the cap, not mean) is sound, and the inline data table makes the rationale auditable.
• The set +e / CLAUDE_RC=$? / set -e bracket is the right pattern for capturing a non-zero exit without triggering set -e prematurely.