Reliability hardening: session circuit breaker, timeouts, retry, dedup - #20
Merged
Merged
Conversation
…ard-fail Reliability hardening for the PR review agent. 1. Session circuit breaker (.github/workflows/pr-review.yml): on any non-zero, non-100 exit from review-one-pr.sh (general failure or rate limit on the fallback engine), break the per-PR loop, log a clear error annotation naming the failing PR and reason, and exit the step with code 1 so the run shows red. Prevents one systemic problem from silently burning the entire candidate pool. 2. Per-tier timeouts (scripts/engine.sh): triage/deep/audit/action/duck each get their own bounded timeout (180/600/600/300/300s defaults, env-overridable). Previously only the duck had a timeout — a hung tier could burn the whole 60min job budget. 3. Retry-with-backoff on transient errors (scripts/engine.sh): triage retries once on 124/137/143 (timeout / signal kill) since its caller captures stdout via $(...) so retries are safe. Deliberately NOT applied to run_agentic/run_duck where stdout is redirected to a file — a retry there would corrupt the partial first-attempt output. 4. Triage non-JSON now hard-fails (scripts/review-one-pr.sh): replaces the silent fallback that synthesized a fake "escalate=MEDIUM" verdict and proceeded to deep review. With the new circuit breaker, loud failure is the right call — masking a broken triage was burning tokens on every PR while the workflow looked healthy. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two bugs were causing the agent to leave multiple comments on the same PR. Together they produced 10 stacked APPROVED reviews on petry-projects/ContentTwin#100. Bug A — idempotency check is order-dependent (review-one-pr.sh): The previous marker-discovery code did: ((.reviews // []) + (.comments // [])) | .[].body | grep marker | tail -1 This relies on the array concatenation order, not chronological order. When old agent comments existed alongside newer agent reviews, tail -1 picked the comment-array marker (older) over the review-array marker (newer), causing the script to think the head SHA hadn't been reviewed and re-run. Replaced with a single jq pipeline that tags each item with submittedAt / createdAt, sorts by timestamp, and takes the actual most-recent marker. Bug B — no cleanup of prior agent items (post-pr-review.sh): After successfully posting a new review/comment, prior agent items were left in place, accumulating forever. Added mark_prior_agent_items_obsolete which, after a successful post: - dismisses prior APPROVED/COMMENTED/CHANGES_REQUESTED agent reviews via the GitHub dismissal API (UI shows them struck-through as Dismissed) - edits prior agent comments to wrap their body in a collapsed <details> block with a "Superseded by re-review at <SHA>" summary, plus a `<!-- pr-review-agent superseded -->` sentinel for idempotency All cleanup API calls are best-effort — failures don't break the workflow. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
don-petry
force-pushed
the
claude/practical-cannon-f1a4d4
branch
from
April 29, 2026 23:24
81a6c88 to
c2204aa
Compare
left a comment
Contributor
There was a problem hiding this comment.
Pull request overview
Reliability/stability hardening for the PR review agent by improving session-level failure behavior, adding per-tier execution bounds, and preventing duplicate/stacked reviews on the same PR.
Changes:
- Make session runs fail-fast via a workflow “session circuit breaker” when a PR processing step fails or hits fallback-engine rate limits.
- Add per-tier
timeoutwrappers and transient retry/backoff for triage invocations. - Fix idempotency marker detection ordering and add cleanup that dismisses/obsoletes prior agent reviews/comments after posting.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
scripts/review-one-pr.sh |
Fixes marker discovery to use timestamp sorting; hard-fails on triage exit/non-JSON to avoid silent escalation. |
scripts/post-pr-review.sh |
Adds cleanup routine intended to dismiss prior agent reviews and collapse prior agent comments after posting. |
scripts/engine.sh |
Introduces per-tier timeout env vars and transient retry loop for triage; wraps agentic tiers in timeout. |
.github/workflows/pr-review.yml |
Adds session circuit breaker behavior so systemic failures stop the candidate loop and fail the run. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+54
to
+88
| # Reviews: dismiss every prior agent review except the newest. State must | ||
| # be APPROVED, COMMENTED, or CHANGES_REQUESTED to be dismissable; DISMISSED | ||
| # ones are already handled, and PENDING ones aren't ours. | ||
| local stale_review_ids | ||
| stale_review_ids=$(echo "$reviews_json" | jq -r ' | ||
| sort_by(.submitted_at) | ||
| | map(select(.body != null and (.body | test("<!-- pr-review-agent v1 sha=[a-f0-9]+")))) | ||
| | .[:-1] | ||
| | .[] | ||
| | select(.state == "APPROVED" or .state == "COMMENTED" or .state == "CHANGES_REQUESTED") | ||
| | .id | ||
| ' 2>/dev/null || true) | ||
|
|
||
| if [ -n "$stale_review_ids" ]; then | ||
| while IFS= read -r review_id; do | ||
| [ -z "$review_id" ] && continue | ||
| echo " dismissing prior agent review $review_id (superseded by $PR_HEAD_SHA)" | ||
| gh api -X PUT "repos/$owner_repo/pulls/$pr_num/reviews/$review_id/dismissals" \ | ||
| -f message="Superseded by automated re-review at $PR_HEAD_SHA." \ | ||
| -f event=DISMISS >/dev/null 2>&1 || true | ||
| done <<< "$stale_review_ids" | ||
| fi | ||
|
|
||
| # Comments: edit each prior agent comment to wrap its body in a collapsed | ||
| # <details> block. The sentinel `<!-- pr-review-agent superseded -->` | ||
| # prevents re-wrapping on subsequent runs. | ||
| local stale_comments | ||
| stale_comments=$(echo "$comments_json" | jq -c ' | ||
| sort_by(.created_at) | ||
| | map(select(.body != null and (.body | test("<!-- pr-review-agent v1 sha=[a-f0-9]+")))) | ||
| | .[:-1] | ||
| | .[] | ||
| | select(.body | test("<!-- pr-review-agent superseded -->") | not) | ||
| | {id: .id, body: .body} | ||
| ' 2>/dev/null || true) |
There was a problem hiding this comment.
mark_prior_agent_items_obsolete is documented as preserving “the newest agent item by timestamp”, but the implementation preserves the newest review and newest comment independently (.[:-1] in each list). This can leave a stale agent comment uncollapsed after a new review is posted, or leave a stale agent review undismissed after a new fix-request comment is posted. Consider either (a) computing the newest agent item across reviews+comments by timestamp and obsoleting everything else, or (b) passing in what was just posted (review/comment id) and explicitly preserving only that item while obsoleting the rest.
Comment on lines
+72
to
+73
| -f message="Superseded by automated re-review at $PR_HEAD_SHA." \ | ||
| -f event=DISMISS >/dev/null 2>&1 || true |
There was a problem hiding this comment.
The dismissal API call includes -f event=DISMISS when calling PUT repos/.../pulls/.../reviews/.../dismissals. That endpoint only accepts a dismissal message payload; sending an unsupported event field will likely cause the request to fail (and the stale reviews won’t be dismissed). Drop the event field and rely on the HTTP method/endpoint to perform the dismissal.
Suggested change
| -f message="Superseded by automated re-review at $PR_HEAD_SHA." \ | |
| -f event=DISMISS >/dev/null 2>&1 || true | |
| -f message="Superseded by automated re-review at $PR_HEAD_SHA." >/dev/null 2>&1 || true |
Comment on lines
+144
to
148
| # Per-tier timeout from DEEP_TIMEOUT_SEC (also applies to action/audit calls; | ||
| # they're all the same agentic shape and similarly priced). | ||
| run_agentic() { | ||
| local prompt_file="$1" | ||
| local model="$2" |
There was a problem hiding this comment.
AUDIT_TIMEOUT_SEC and ACTION_TIMEOUT_SEC are defined above, but run_agentic always uses timeout "$DEEP_TIMEOUT_SEC" regardless of whether the caller is running deep/action/audit. This makes the per-tier timeout env vars ineffective. Consider passing a timeout value into run_agentic (or adding tier-specific wrappers) and using the appropriate timeout at each call site.
…e JSON Three fixes to mark_prior_agent_items_obsolete from the review of PR #20: 1. ::warning:: annotations on every cleanup API failure (review/comment list-fetch, individual review dismissal, individual comment fetch+edit). Previously these were silenced with `|| true`, so a permissions change on the dismissal endpoint would let duplicates stack indefinitely with no signal in the Actions UI. Cleanup is still non-fatal — the new post has already landed — but failures are now visible. 2. Preserve the globally-latest agent item across BOTH categories, not the newest of each category separately. The earlier code split reviews and comments and applied `[:-1]` to each, which left a stale fix-request comment in place when the new post was a review (or vice versa). The one-off cleanup of ContentTwin#100 hit exactly this case: 12 stacked reviews collapsed to 1, but a stale comment from 2026-04-25 (SHA cd9132d6) was preserved as "newest comment" even though the latest review at SHA 3af8c8ee was newer overall. Now: compute the max timestamp across both feeds, exclude items at that timestamp. 3. Stage API responses to disk (`mktemp` + `jq <file>`) instead of routing through `--argjson "$var"`. The old approach broke on rare unescaped control chars in user-authored comment bodies (jq refused to parse the resulting shell-vared JSON). File-based input sidesteps the shell pipeline entirely. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced May 16, 2026
This was referenced Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reliability and stability hardening for the PR review agent, motivated by an audit that surfaced multiple silent-failure modes — the agent could burn an entire candidate pool without producing a review while the workflow looked green, and was stacking duplicate reviews on the same PR (10 stacked APPROVED reviews observed on petry-projects/ContentTwin#100).
Session-level error handling
.github/workflows/pr-review.yml: any non-zero / non-100 exit fromreview-one-pr.sh(general failure or rate-limit on the fallback engine) breaks the per-PR loop, logs::error::Session aborted early...with the failing PR / reason / count of skipped candidates, and exits the step with code 1 so the run shows red.scripts/review-one-pr.sh): replaces the silent fallback that synthesized a fakeescalate=MEDIUMverdict and proceeded to the deep tier — masking a tier-1 model regression behind a successful tier-2 review and silently doubling cost on every broken triage.Per-tier timeouts and bounded retry
timeoutwrappers inscripts/engine.shfor triage / deep / audit / action / duck (180/600/600/300/300s defaults, env-overridable). Previously only the duck had a timeout — a hung tier could burn the whole 60min job budget.run_triagewhere stdout is captured by$(...)so retries are safe. Deliberately not applied torun_agentic/run_duck— callers redirect stdout to a file and a retry would corrupt the partial first-attempt output. Those tiers fall through to the session circuit breaker on transient failure.Stop stacking duplicate reviews on the same PR
Two distinct bugs were causing it; both fixed:
review-one-pr.sh. The previous marker-discovery did((.reviews // []) + (.comments // [])) | tail -1, which depends on array order, not chronological order. When old comments with markers existed alongside newer reviews with markers,tail -1returned a stale SHA → the script thought the head was un-reviewed and re-ran every hour. Replaced with a single jq pipeline that tags each item withsubmittedAt/createdAt, sorts by timestamp, and takes the actual most-recent marker.post-pr-review.sh. Addedmark_prior_agent_items_obsolete, called after every successful post:APPROVED/COMMENTED/CHANGES_REQUESTEDagent reviews → dismissed via the GitHub dismissal API (UI shows "Dismissed" with strikethrough).<details><summary>Superseded by re-review at <SHA></summary>...</details>, with a<!-- pr-review-agent superseded -->sentinel preventing recursive nesting.|| true) so cleanup failures don't break the workflow.Test plan
[SESSION ABORTED EARLY]summary3af8c8eeinstead of stalecd9132d6Follow-ups (separate PRs in flight)
MAX_REVIEW_CYCLESenforcement (currently dead config)🤖 Generated with Claude Code