From e976ca01a93bd08b21c87cea91b1c2efe277232d Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Thu, 14 May 2026 20:41:18 -0500 Subject: [PATCH 1/3] fix: catch GitHub API 429s that were silently swallowed in review-one-pr.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three bugs caused gh rate-limit errors to pass through undetected: 1. Pre-fetch calls used `2>/dev/null || echo '{}'` — a gh 429 wrote nothing visible and silently fell back to empty metadata/diff, sending the triage model in with no PR context. 2. The triage `is_rate_limited` check was gated on `TRIAGE_RC -ne 0`. When the triage model exited 0 but emitted 429 text (the AI service rate-limited the call), the check block never ran. Triage is safe to check unconditionally because it runs with all tools denied — no gh calls happen inside it, so any 429 in its output is unambiguously from the AI service, not from PR diff content passing through a tool result. 3. `wait $DEEP_PID || true` discarded the deep-review exit code entirely. A non-zero exit without a JSON file would show `DEEP_RC=0` in the condition, causing the "did not produce valid JSON" branch to be skipped and the script to proceed on stale or absent output. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- scripts/review-one-pr.sh | 54 ++++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 11 deletions(-) diff --git a/scripts/review-one-pr.sh b/scripts/review-one-pr.sh index 136b519f7..a9db9299c 100644 --- a/scripts/review-one-pr.sh +++ b/scripts/review-one-pr.sh @@ -234,8 +234,30 @@ echo " [tier1] triage ($ENGINE_TRIAGE_MODEL)" # but `claude --print` does not interpolate shell variables into the prompt, # so the model only ever saw the literal text "$PR_METADATA" and reported # the data as missing. Inlining the values is the fix.) -PR_METADATA=$(gh pr view "$PR_URL" --json number,title,body,author,isDraft,baseRefName,headRefName,headRefOid,url,headRepository,headRepositoryOwner,labels,reviewDecision,mergeable,mergeStateStatus,statusCheckRollup,reviewRequests,reviews,comments,commits,closingIssuesReferences,additions,deletions,changedFiles,files 2>/dev/null || echo '{}') -PR_DIFF=$(gh pr diff "$PR_URL" 2>/dev/null | head -3000 || echo "") +_gh_meta_err=/tmp/cascade/gh-meta-prefetch.err +mkdir -p /tmp/cascade +PR_METADATA=$(gh pr view "$PR_URL" --json number,title,body,author,isDraft,baseRefName,headRefName,headRefOid,url,headRepository,headRepositoryOwner,labels,reviewDecision,mergeable,mergeStateStatus,statusCheckRollup,reviewRequests,reviews,comments,commits,closingIssuesReferences,additions,deletions,changedFiles,files 2>"$_gh_meta_err") || { + if is_rate_limited "$(cat "$_gh_meta_err" 2>/dev/null || true)"; then + echo " gh API rate limited on metadata fetch — skipping PR (will retry next run)" + echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"gh-rate-limited\"}" + exit 100 + fi + PR_METADATA='{}' +} +_gh_diff_err=/tmp/cascade/gh-diff-prefetch.err +_gh_diff_tmp=/tmp/cascade/gh-diff-raw.txt +if gh pr diff "$PR_URL" > "$_gh_diff_tmp" 2>"$_gh_diff_err"; then + PR_DIFF=$(head -3000 "$_gh_diff_tmp") +else + if is_rate_limited "$(cat "$_gh_diff_err" 2>/dev/null || true)"; then + echo " gh API rate limited on diff fetch — skipping PR (will retry next run)" + echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"gh-rate-limited\"}" + exit 100 + fi + PR_DIFF="" +fi +rm -f "$_gh_diff_tmp" +unset _gh_meta_err _gh_diff_err _gh_diff_tmp # Build the triage prompt: static template + inlined PR context. TRIAGE_PROMPT_FILE="/tmp/cascade/triage-prompt.md" @@ -269,6 +291,17 @@ unset PR_DIFF PR_METADATA # Claude Code writes its usage-cap error to stdout (captured in TRIAGE_RESULT); # some other providers write to stderr (TRIAGE_LOG) — check both channels. TRIAGE_STDERR=$(cat "$TRIAGE_LOG" 2>/dev/null || true) +# Rate-limit check runs unconditionally (not gated on rc != 0) for the triage +# tier because the model runs with ALL tools denied — it cannot call gh or any +# external API. A 429 in the triage output therefore always comes from the AI +# service itself being rate-limited, never from PR diff content passing through +# a tool call. (Agentic tiers keep the rc-gated check to avoid false positives +# from PRs that contain 429 error-handling code visible in Bash tool results.) +if is_rate_limited "$TRIAGE_RESULT" || is_rate_limited "$TRIAGE_STDERR"; then + echo " [tier1] usage/rate limit detected — exiting with code 2 for engine fallback" + echo " limit message: ${TRIAGE_RESULT:-}${TRIAGE_STDERR:+ (stderr: $TRIAGE_STDERR)}" + exit 2 +fi if [ "$TRIAGE_RC" -ne 0 ]; then # CLI invocation errors (bad flags, wrong syntax) are per-PR failures — exit 1 # so the session can continue with the next PR rather than aborting entirely. @@ -277,12 +310,6 @@ if [ "$TRIAGE_RC" -ne 0 ]; then echo " error message: ${TRIAGE_RESULT:-}${TRIAGE_STDERR:+ (stderr: $TRIAGE_STDERR)}" exit 1 fi - # Rate-limit / overload — exit 2 so the caller can fall back to a different engine. - if is_rate_limited "$TRIAGE_RESULT" || is_rate_limited "$TRIAGE_STDERR"; then - echo " [tier1] usage/rate limit detected — exiting with code 2 for engine fallback" - echo " limit message: ${TRIAGE_RESULT:-}${TRIAGE_STDERR:+ (stderr: $TRIAGE_STDERR)}" - exit 2 - fi fi # Hard-fail on triage process exit. Previously this silently synthesized a @@ -414,11 +441,15 @@ DUCK_OUTPUT="/tmp/cascade/rubber-duck.json" DUCK_PID=$! # Wait for deep review first — if it fails, kill the duck and exit early. -wait $DEEP_PID || true +# Capture the exit code explicitly; `|| true` would swallow it and prevent +# rate-limit detection when the process exits non-zero without writing JSON. +DEEP_RC=0 +wait $DEEP_PID || DEEP_RC=$? -# Validate primary deep review (required) +# Validate primary deep review (required). +# Check on process failure OR missing/invalid JSON — whichever fires first. OUTPUT_FILE="/tmp/cascade/deep.json" -if [ ! -s "$OUTPUT_FILE" ] || ! jq empty "$OUTPUT_FILE" 2>/dev/null; then +if [ "$DEEP_RC" -ne 0 ] || [ ! -s "$OUTPUT_FILE" ] || ! jq empty "$OUTPUT_FILE" 2>/dev/null; then # Check the model's stdout for a rate-limit or CLI-error message. We # intentionally do NOT check deep.log (stderr/process noise) to avoid false # positives from PR diff content that happens to mention these keywords in @@ -437,6 +468,7 @@ if [ ! -s "$OUTPUT_FILE" ] || ! jq empty "$OUTPUT_FILE" 2>/dev/null; then echo "$DEEP_STDOUT_CONTENT" exit 2 fi + [ "$DEEP_RC" -ne 0 ] && echo "::warning::deep review process exited $DEEP_RC" echo "::warning::deep review did not produce valid JSON at $OUTPUT_FILE" cat /tmp/cascade/deep-stdout.txt 2>/dev/null || true cat /tmp/cascade/deep.log 2>/dev/null || true From 60a3725bc71fe278398fc255579fef2b42b75e5d Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Thu, 14 May 2026 21:12:54 -0500 Subject: [PATCH 2/3] fix: don't gate deep-review on DEEP_RC when JSON output is valid Adding `DEEP_RC -ne 0` as an OR condition on the failure branch caused a regression: if claude exits non-zero after the agent has already written valid JSON to OUTPUT_FILE (e.g. OOM kill, timeout on a post-write step), the valid verdict was discarded and the PR review aborted. Keep the DEEP_RC capture so the exit code is visible in logs, but only treat it as an error when the JSON output is actually missing or invalid. Log a warning and proceed when JSON is valid despite a non-zero exit. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- scripts/review-one-pr.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/review-one-pr.sh b/scripts/review-one-pr.sh index a9db9299c..44a00a1fa 100644 --- a/scripts/review-one-pr.sh +++ b/scripts/review-one-pr.sh @@ -447,9 +447,8 @@ DEEP_RC=0 wait $DEEP_PID || DEEP_RC=$? # Validate primary deep review (required). -# Check on process failure OR missing/invalid JSON — whichever fires first. OUTPUT_FILE="/tmp/cascade/deep.json" -if [ "$DEEP_RC" -ne 0 ] || [ ! -s "$OUTPUT_FILE" ] || ! jq empty "$OUTPUT_FILE" 2>/dev/null; then +if [ ! -s "$OUTPUT_FILE" ] || ! jq empty "$OUTPUT_FILE" 2>/dev/null; then # Check the model's stdout for a rate-limit or CLI-error message. We # intentionally do NOT check deep.log (stderr/process noise) to avoid false # positives from PR diff content that happens to mention these keywords in @@ -475,6 +474,9 @@ if [ "$DEEP_RC" -ne 0 ] || [ ! -s "$OUTPUT_FILE" ] || ! jq empty "$OUTPUT_FILE" echo "::error::cascade failed at tier 2 for $PR_URL" exit 1 fi +# JSON is valid — if the process still exited non-zero log it but proceed: +# the agent may have written the verdict before a non-fatal wrapper error. +[ "$DEEP_RC" -ne 0 ] && echo "::warning::deep review process exited $DEEP_RC but produced valid JSON — proceeding" # Wait for duck to finish (deep succeeded) wait $DUCK_PID || true From 10a7921c7a458994e1844d22eb2c3028f34dbf9c Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Thu, 14 May 2026 21:20:14 -0500 Subject: [PATCH 3/3] fix: address CodeRabbit review comments on 429 handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues raised in review: 1. Triage rate-limit false positives: TRIAGE_RESULT was checked unconditionally, but the triage prompt inlines the full PR diff — a PR adding "429 rate-limit handling" could produce valid JSON with matching text and incorrectly trigger engine fallback. Fix: check TRIAGE_STDERR unconditionally (process output, never PR content) and gate TRIAGE_RESULT check on rc != 0, restoring CLI-before-rate-limit precedence inside the failure block. 2. Non-429 gh prefetch failures still silently produced empty context: auth errors, network failures, and GraphQL errors fell through to PR_METADATA='{}' and PR_DIFF='', sending the triage model incomplete context and producing a bogus review. Now exit 1 with an explicit error message for any non-rate-limited prefetch failure. 3. Temp file leaks on early exit: _gh_diff_tmp and error log files were not cleaned up before exit 100 paths. Now all exit paths (rate-limit skip, non-rate-limit failure, and normal completion) clean up all three temp files. Removed the redundant mkdir -p /tmp/cascade (already called at line 226). Co-Authored-By: Claude Sonnet 4.6 (1M context) --- scripts/review-one-pr.sh | 48 ++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/scripts/review-one-pr.sh b/scripts/review-one-pr.sh index 44a00a1fa..c2fb38f45 100644 --- a/scripts/review-one-pr.sh +++ b/scripts/review-one-pr.sh @@ -235,29 +235,38 @@ echo " [tier1] triage ($ENGINE_TRIAGE_MODEL)" # so the model only ever saw the literal text "$PR_METADATA" and reported # the data as missing. Inlining the values is the fix.) _gh_meta_err=/tmp/cascade/gh-meta-prefetch.err -mkdir -p /tmp/cascade +_gh_diff_err=/tmp/cascade/gh-diff-prefetch.err +_gh_diff_tmp=/tmp/cascade/gh-diff-raw.txt PR_METADATA=$(gh pr view "$PR_URL" --json number,title,body,author,isDraft,baseRefName,headRefName,headRefOid,url,headRepository,headRepositoryOwner,labels,reviewDecision,mergeable,mergeStateStatus,statusCheckRollup,reviewRequests,reviews,comments,commits,closingIssuesReferences,additions,deletions,changedFiles,files 2>"$_gh_meta_err") || { - if is_rate_limited "$(cat "$_gh_meta_err" 2>/dev/null || true)"; then + _gh_meta_err_content=$(cat "$_gh_meta_err" 2>/dev/null || true) + if is_rate_limited "$_gh_meta_err_content"; then + rm -f "$_gh_meta_err" "$_gh_diff_err" "$_gh_diff_tmp" echo " gh API rate limited on metadata fetch — skipping PR (will retry next run)" echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"gh-rate-limited\"}" exit 100 fi - PR_METADATA='{}' + echo "::error::gh pr view failed during metadata prefetch for $PR_URL" + [ -n "$_gh_meta_err_content" ] && echo " $_gh_meta_err_content" + rm -f "$_gh_meta_err" "$_gh_diff_err" "$_gh_diff_tmp" + exit 1 } -_gh_diff_err=/tmp/cascade/gh-diff-prefetch.err -_gh_diff_tmp=/tmp/cascade/gh-diff-raw.txt if gh pr diff "$PR_URL" > "$_gh_diff_tmp" 2>"$_gh_diff_err"; then PR_DIFF=$(head -3000 "$_gh_diff_tmp") else - if is_rate_limited "$(cat "$_gh_diff_err" 2>/dev/null || true)"; then + _gh_diff_err_content=$(cat "$_gh_diff_err" 2>/dev/null || true) + if is_rate_limited "$_gh_diff_err_content"; then + rm -f "$_gh_meta_err" "$_gh_diff_err" "$_gh_diff_tmp" echo " gh API rate limited on diff fetch — skipping PR (will retry next run)" echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"gh-rate-limited\"}" exit 100 fi - PR_DIFF="" + echo "::error::gh pr diff failed during prefetch for $PR_URL" + [ -n "$_gh_diff_err_content" ] && echo " $_gh_diff_err_content" + rm -f "$_gh_meta_err" "$_gh_diff_err" "$_gh_diff_tmp" + exit 1 fi -rm -f "$_gh_diff_tmp" -unset _gh_meta_err _gh_diff_err _gh_diff_tmp +rm -f "$_gh_meta_err" "$_gh_diff_err" "$_gh_diff_tmp" +unset _gh_meta_err _gh_diff_err _gh_diff_tmp _gh_meta_err_content _gh_diff_err_content # Build the triage prompt: static template + inlined PR context. TRIAGE_PROMPT_FILE="/tmp/cascade/triage-prompt.md" @@ -291,15 +300,14 @@ unset PR_DIFF PR_METADATA # Claude Code writes its usage-cap error to stdout (captured in TRIAGE_RESULT); # some other providers write to stderr (TRIAGE_LOG) — check both channels. TRIAGE_STDERR=$(cat "$TRIAGE_LOG" 2>/dev/null || true) -# Rate-limit check runs unconditionally (not gated on rc != 0) for the triage -# tier because the model runs with ALL tools denied — it cannot call gh or any -# external API. A 429 in the triage output therefore always comes from the AI -# service itself being rate-limited, never from PR diff content passing through -# a tool call. (Agentic tiers keep the rc-gated check to avoid false positives -# from PRs that contain 429 error-handling code visible in Bash tool results.) -if is_rate_limited "$TRIAGE_RESULT" || is_rate_limited "$TRIAGE_STDERR"; then +# TRIAGE_STDERR is always process output — safe to check unconditionally. +# TRIAGE_RESULT is gated on rc != 0 to avoid false positives: the triage prompt +# inlines the full PR diff, so a PR adding "429 rate-limit handling" could +# produce valid JSON with matching text in signals/summary, incorrectly +# triggering engine fallback on a healthy call that exited 0. +if is_rate_limited "$TRIAGE_STDERR"; then echo " [tier1] usage/rate limit detected — exiting with code 2 for engine fallback" - echo " limit message: ${TRIAGE_RESULT:-}${TRIAGE_STDERR:+ (stderr: $TRIAGE_STDERR)}" + echo " limit message: (stderr: $TRIAGE_STDERR)" exit 2 fi if [ "$TRIAGE_RC" -ne 0 ]; then @@ -310,6 +318,12 @@ if [ "$TRIAGE_RC" -ne 0 ]; then echo " error message: ${TRIAGE_RESULT:-}${TRIAGE_STDERR:+ (stderr: $TRIAGE_STDERR)}" exit 1 fi + # Rate-limit / overload — exit 2 so the caller can fall back to a different engine. + if is_rate_limited "$TRIAGE_RESULT"; then + echo " [tier1] usage/rate limit detected — exiting with code 2 for engine fallback" + echo " limit message: ${TRIAGE_RESULT:-}" + exit 2 + fi fi # Hard-fail on triage process exit. Previously this silently synthesized a