Skip to content

fix: catch GitHub API 429s silently swallowed in review-one-pr.sh #176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
don-petry merged 3 commits into from
May 15, 2026
Merged

fix: catch GitHub API 429s silently swallowed in review-one-pr.sh #176

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e976ca0
fix: catch GitHub API 429s that were silently swallowed in review-one…
don-petry May 15, 2026
60a3725
fix: don't gate deep-review on DEEP_RC when JSON output is valid
don-petry May 15, 2026
10a7921
fix: address CodeRabbit review comments on 429 handling
don-petry May 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 43 additions & 11 deletions scripts/review-one-pr.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=""
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
fi
rm -f "$_gh_diff_tmp"
unset _gh_meta_err _gh_diff_err _gh_diff_tmp

@gemini-code-assist gemini-code-assist Bot May 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The temporary error log files _gh_meta_err and _gh_diff_err are never removed, and _gh_diff_tmp is leaked if the script exits early at line 255. These files should be cleaned up at the end of the block and before any early exit calls to avoid leaving stale data in /tmp/cascade. Additionally, the mkdir -p /tmp/cascade at line 238 is redundant as it is already performed at line 226.

Suggested change
rm -f "$_gh_diff_tmp"
unset _gh_meta_err _gh_diff_err _gh_diff_tmp
rm -f "$_gh_diff_tmp" "$_gh_meta_err" "$_gh_diff_err"
unset _gh_meta_err _gh_diff_err _gh_diff_tmp
References
  1. When passing large JSON payloads to curl, use a temporary file and the --data-binary @file flag to avoid exceeding the ARG_MAX limit.
  2. Keep error routing logic inline when tiers have distinct requirements to avoid obscuring intent or introducing false positives.


# Build the triage prompt: static template + inlined PR context.
TRIAGE_PROMPT_FILE="/tmp/cascade/triage-prompt.md"
Expand Down Expand Up @@ -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

@chatgpt-codex-connector chatgpt-codex-connector Bot May 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid treating successful triage text as a rate limit

Because this check now runs even when run_triage exits 0, any valid triage JSON whose summary or signals mention common reviewed-code terms like rate-limit, too many requests, or 429 is classified as an engine rate limit and exits 2. The triage prompt includes the PR diff and asks the model to summarize/signals it, so a normal PR that changes HTTP 429/rate-limit handling can repeatedly trigger engine fallback instead of being reviewed; this should remain scoped to failed invocations or inspect provider error channels separately from successful JSON content.

Useful? React with 👍 / 👎.

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
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
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.
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down