Skip to content

fix(pr-review): robust rate-limit detection and fallback across all tiers #225

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 6 commits into from
May 17, 2026
Merged

fix(pr-review): robust rate-limit detection and fallback across all tiers #225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9110690
fix(ci): ensure gemini-cli installs in reusable dev-lead workflow
May 17, 2026
a28aec7
test: dummy PR for testing rate limit fallback
May 17, 2026
b29afbb
fix(pr-review): robust rate-limit detection across all tiers
May 17, 2026
1c5d56a
test: bypass CI check
May 17, 2026
298d01b
fix(pr-review): robust rate-limit detection and fallback across all t…
May 17, 2026
57c83c6
fix(pr-review): remove CI bypass hack
May 17, 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
6 changes: 6 additions & 0 deletions .github/workflows/dev-lead-reusable.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,15 @@ jobs:
fi

case "$DEV_LEAD_ENGINE" in
claude)
npm install -g @google/gemini-cli || true
if ! gh copilot --version >/dev/null 2>&1; then
gh extension install github/gh-copilot || true
fi ;;
gemini)
npm install -g @google/gemini-cli || true ;;
copilot)
npm install -g @google/gemini-cli || true
if ! gh copilot --version >/dev/null 2>&1; then
gh extension install github/gh-copilot || true
fi ;;
Expand Down
Empty file added dummy.txt
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions scripts/engine.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ is_rate_limited() {
_pat="$_pat|claude.*usage|usage.*claude" # Claude-specific cap
_pat="$_pat|plan.*limit|subscription.*limit|billing.*limit|daily.*limit|monthly.*limit"
_pat="$_pat|([^0-9]|^)402([^0-9]|$)" # HTTP 402 (payment)
_pat="$_pat|tokens_limit_reached|body too large|([^0-9]|^)413([^0-9]|$)" # Context / Request size
printf '%s\n' "$text" | grep -qiE "($_pat)"
}

Expand Down
88 changes: 72 additions & 16 deletions scripts/review-one-pr.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,22 +473,20 @@ wait $DEEP_PID || DEEP_RC=$?
# Validate primary deep review (required).
OUTPUT_FILE="/tmp/cascade/deep.json"
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
# code or comments.
# Check both stdout and stderr for a rate-limit or CLI-error message.
DEEP_STDOUT_CONTENT=$(cat /tmp/cascade/deep-stdout.txt 2>/dev/null || true)
DEEP_STDERR_CONTENT=$(cat /tmp/cascade/deep.log 2>/dev/null || true)
kill $DUCK_PID 2>/dev/null || true
wait $DUCK_PID 2>/dev/null || true
# CLI invocation errors are per-PR failures (exit 1), not engine fallbacks (exit 2).
if is_cli_error "$DEEP_STDOUT_CONTENT"; then
if is_cli_error "$DEEP_STDOUT_CONTENT" || is_cli_error "$DEEP_STDERR_CONTENT"; then
echo " [error] CLI format error — treating as per-PR failure (not a rate limit)"
echo "$DEEP_STDOUT_CONTENT"
echo "${DEEP_STDOUT_CONTENT:-}${DEEP_STDERR_CONTENT:+ (stderr: $DEEP_STDERR_CONTENT)}"
exit 1
fi
if is_rate_limited "$DEEP_STDOUT_CONTENT"; then
if is_rate_limited "$DEEP_STDOUT_CONTENT" || is_rate_limited "$DEEP_STDERR_CONTENT"; then
echo " [tier2] rate limit detected — exiting with code 2 for engine fallback"
echo "$DEEP_STDOUT_CONTENT"
echo "${DEEP_STDOUT_CONTENT:-}${DEEP_STDERR_CONTENT:+ (stderr: $DEEP_STDERR_CONTENT)}"
exit 2
fi
[ "$DEEP_RC" -ne 0 ] && echo "::warning::deep review process exited $DEEP_RC"
Expand Down Expand Up @@ -529,7 +527,7 @@ if [ "$DUCK_VALID" = "true" ]; then
OUTPUT_FILE="/tmp/cascade/combined.json"
export DEEP_RESULT DUCK_RESULT OUTPUT_FILE
run_agentic prompts/synthesize-duck.md "$ENGINE_ACTION_MODEL" \
> /tmp/cascade/synth.log 2>&1 || true
> /tmp/cascade/synth-stdout.txt 2>/tmp/cascade/synth.log || true

if [ -s "$OUTPUT_FILE" ] && jq empty "$OUTPUT_FILE" 2>/dev/null; then
COMBINED_DECISION=$(jq -r '.decision' "$OUTPUT_FILE")
Expand All @@ -538,8 +536,20 @@ if [ "$DUCK_VALID" = "true" ]; then
COMBINED_ESCALATE=$(jq -r '.escalate_to_opus' "$OUTPUT_FILE")
echo " [tier2b] combined: decision=$COMBINED_DECISION risk=$COMBINED_RISK agreement=$COMBINED_AGREEMENT escalate_to_opus=$COMBINED_ESCALATE"
else
SYNTH_STDOUT=$(cat /tmp/cascade/synth-stdout.txt 2>/dev/null || true)
SYNTH_STDERR=$(cat /tmp/cascade/synth.log 2>/dev/null || true)
if is_cli_error "$SYNTH_STDOUT" || is_cli_error "$SYNTH_STDERR"; then
echo " [error] CLI format error (synthesis) — treating as per-PR failure"
echo "${SYNTH_STDOUT:-}${SYNTH_STDERR:+ (stderr: $SYNTH_STDERR)}"
exit 1
fi
if is_rate_limited "$SYNTH_STDOUT" || is_rate_limited "$SYNTH_STDERR"; then
echo " [tier2b] rate limit detected during synthesis — exiting with code 2 for engine fallback"
echo "${SYNTH_STDOUT:-}${SYNTH_STDERR:+ (stderr: $SYNTH_STDERR)}"
exit 2
fi
echo " [tier2b] synthesis failed — falling back to deep review only"
cat /tmp/cascade/synth.log 2>/dev/null || true
[ -n "$SYNTH_STDERR" ] && echo " stderr: $SYNTH_STDERR"
OUTPUT_FILE="/tmp/cascade/deep.json"
DUCK_VALID=false
COMBINED_ESCALATE=$(jq -r '.escalate_to_opus' "$OUTPUT_FILE")
Expand All @@ -562,8 +572,23 @@ if [ "$COMBINED_ESCALATE" != "true" ]; then
VERDICT_JSON="/tmp/cascade/cascade-action-verdict.json"
OUTPUT_FILE="$VERDICT_JSON"
export OUTPUT_FILE
run_agentic prompts/cascade-action.md "$ENGINE_ACTION_MODEL" > "$VERDICT_JSON.raw"
extract_verdict_json "$VERDICT_JSON.raw" "$VERDICT_JSON" || { echo "::error::cascade-action did not produce valid JSON"; exit 1; }
run_agentic prompts/cascade-action.md "$ENGINE_ACTION_MODEL" > "$VERDICT_JSON.raw" 2>/tmp/cascade/action.log || true

@chatgpt-codex-connector chatgpt-codex-connector Bot May 17, 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.

P1 Badge Clear stale verdicts before ignoring action failures

When review-batch.sh reviews multiple PRs in one job, /tmp/cascade is reused and this path does not remove the previous /tmp/cascade/cascade-action-verdict.json before running the action step. Because the new || true suppresses a non-zero engine exit, extract_verdict_json can immediately accept the old $VERDICT_JSON file and skip the rate-limit/error checks, causing a failed or rate-limited action on the current PR to post a stale verdict from an earlier PR instead of falling back.

Useful? React with 👍 / 👎.

if ! extract_verdict_json "$VERDICT_JSON.raw" "$VERDICT_JSON"; then
ACTION_STDOUT=$(cat "$VERDICT_JSON.raw" 2>/dev/null || true)
ACTION_STDERR=$(cat /tmp/cascade/action.log 2>/dev/null || true)
if is_cli_error "$ACTION_STDOUT" || is_cli_error "$ACTION_STDERR"; then
echo " [error] CLI format error (action) — treating as per-PR failure"
echo "${ACTION_STDOUT:-}${ACTION_STDERR:+ (stderr: $ACTION_STDERR)}"
exit 1
fi
if is_rate_limited "$ACTION_STDOUT" || is_rate_limited "$ACTION_STDERR"; then
echo " [action] rate limit detected — exiting with code 2 for engine fallback"
echo "${ACTION_STDOUT:-}${ACTION_STDERR:+ (stderr: $ACTION_STDERR)}"
exit 2
fi
echo "::error::cascade-action did not produce valid JSON"
exit 1
fi
rm -f "$VERDICT_JSON.raw"

# Post the review using the verdict
Expand All @@ -582,12 +607,27 @@ DEEP_RESULT="$TIER2_RESULT"
export DEEP_RESULT
OUTPUT_FILE="/tmp/cascade/audit.json"
export OUTPUT_FILE

# Audit tier: separate stdout and stderr to safely detect rate limits without
# false positives from PR diff content in stdout.
run_agentic prompts/security-audit.md "$ENGINE_AUDIT_MODEL" \
> /tmp/cascade/audit.log 2>&1
> /tmp/cascade/audit-stdout.txt 2>/tmp/cascade/audit.log || true
Comment on lines 613 to +614

@chatgpt-codex-connector chatgpt-codex-connector Bot May 17, 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.

P1 Badge Remove stale audit output before suppressing failures

In batch runs, /tmp/cascade/audit.json can still contain the previous PR's security-audit result because this script never clears /tmp/cascade between PRs. With the new || true, a current audit invocation that exits non-zero before writing its own JSON is no longer stopped by set -e; the following validation can accept the old audit file and proceed with the prior PR's audit decision instead of detecting the current rate limit or failure.

Useful? React with 👍 / 👎.


if [ ! -s "$OUTPUT_FILE" ] || ! jq empty "$OUTPUT_FILE" 2>/dev/null; then
AUDIT_STDOUT=$(cat /tmp/cascade/audit-stdout.txt 2>/dev/null || true)
AUDIT_STDERR=$(cat /tmp/cascade/audit.log 2>/dev/null || true)
if is_cli_error "$AUDIT_STDOUT" || is_cli_error "$AUDIT_STDERR"; then
echo " [error] CLI format error (tier 3) — treating as per-PR failure"
echo "${AUDIT_STDOUT:-}${AUDIT_STDERR:+ (stderr: $AUDIT_STDERR)}"
exit 1
fi
if is_rate_limited "$AUDIT_STDOUT" || is_rate_limited "$AUDIT_STDERR"; then
echo " [tier3] rate limit detected — exiting with code 2 for engine fallback"
echo "${AUDIT_STDOUT:-}${AUDIT_STDERR:+ (stderr: $AUDIT_STDERR)}"
exit 2
fi
echo "::warning::security audit did not produce valid JSON at $OUTPUT_FILE"
cat /tmp/cascade/audit.log || true
[ -s "/tmp/cascade/audit.log" ] && { echo " stderr:"; cat /tmp/cascade/audit.log; }
echo "::error::cascade failed at tier 3 for $PR_URL"
exit 1
fi
Expand All @@ -604,8 +644,24 @@ export FINAL_TIER="audit"
VERDICT_JSON="/tmp/cascade/cascade-action-verdict-audit.json"
OUTPUT_FILE="$VERDICT_JSON"
export OUTPUT_FILE
run_agentic prompts/cascade-action.md "$ENGINE_ACTION_MODEL" > "$VERDICT_JSON.raw"
extract_verdict_json "$VERDICT_JSON.raw" "$VERDICT_JSON" || { echo "::error::cascade-action (audit) did not produce valid JSON"; exit 1; }

run_agentic prompts/cascade-action.md "$ENGINE_ACTION_MODEL" > "$VERDICT_JSON.raw" 2>/tmp/cascade/action-audit.log || true

@chatgpt-codex-connector chatgpt-codex-connector Bot May 17, 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.

P1 Badge Clear stale audit verdicts before ignoring action failures

For PRs that reach the security-audit path, /tmp/cascade/cascade-action-verdict-audit.json is also reused across PRs and is not removed before this action invocation. If the current action step exits non-zero before producing a verdict, the added || true lets extract_verdict_json accept the old audit-action verdict and post that stale review for the current PR, bypassing the intended rate-limit/error handling.

Useful? React with 👍 / 👎.

if ! extract_verdict_json "$VERDICT_JSON.raw" "$VERDICT_JSON"; then
ACTION_STDOUT=$(cat "$VERDICT_JSON.raw" 2>/dev/null || true)
ACTION_STDERR=$(cat /tmp/cascade/action-audit.log 2>/dev/null || true)
if is_cli_error "$ACTION_STDOUT" || is_cli_error "$ACTION_STDERR"; then
echo " [error] CLI format error (action audit) — treating as per-PR failure"
echo "${ACTION_STDOUT:-}${ACTION_STDERR:+ (stderr: $ACTION_STDERR)}"
exit 1
fi
if is_rate_limited "$ACTION_STDOUT" || is_rate_limited "$ACTION_STDERR"; then
echo " [action] rate limit detected — exiting with code 2 for engine fallback"
echo "${ACTION_STDOUT:-}${ACTION_STDERR:+ (stderr: $ACTION_STDERR)}"
exit 2
fi
echo "::error::cascade-action (audit) did not produce valid JSON"
exit 1
fi
rm -f "$VERDICT_JSON.raw"

# Post the review using the verdict
Expand Down
Loading