Repository navigation
fix: catch GitHub API 429s silently swallowed in review-one-pr.sh - #176
Merged
Merged
Conversation
…-pr.sh
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) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesRate-limit detection and error handling hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes three places in scripts/review-one-pr.sh where GitHub/AI 429 rate-limit signals were being silently swallowed, causing the cascade to proceed with empty data or skip rate-limit detection entirely.
Changes:
- Pre-fetch
gh pr view/gh pr diffnow route stderr to a temp file and runis_rate_limitedon it before falling back to empty defaults; on detection, exit 100 (skip, retry next run) instead of feeding{}and empty diff to triage. - The triage tier's
is_rate_limitedcheck is moved out from under therc != 0guard so it runs unconditionally — safe because the triage model has all tools denied and can't produce false-positive 429 strings from tool results. - The deep-review tier captures
wait $DEEP_PID's exit code intoDEEP_RCinstead of discarding it with|| true, and folds a non-zero process exit into the JSON-validity failure branch so rate-limit detection actually runs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e976ca01a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # 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 |
There was a problem hiding this comment.
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 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/review-one-pr.sh`:
- Around line 239-257: The current prefetch blocks for PR_METADATA and PR_DIFF
swallow all non-429 gh failures by falling back to '{}' and ""; instead, when gh
fails and is_rate_limited(...) returns false, stop the review run with a
non-zero exit (and a clear error JSON/message). Concretely, in the PR_METADATA
error handler (where PR_METADATA='{}' and _gh_meta_err is set) and in the
PR_DIFF failure branch (where PR_DIFF="" and _gh_diff_err/_gh_diff_tmp are
used), replace the silent fallback with logging an error/JSON similar to the
rate-limited branch and exit (e.g., echo an error object with
"reason":"gh-prefetch-failed" and exit non-zero) so auth/network/GraphQL
failures do not produce bogus reviews.
- Around line 294-304: The current unconditional rate-limit check using
is_rate_limited against TRIAGE_RESULT/TRIAGE_STDERR can false-positive when the
inlined PR diff mentions "429" or "rate limit"; update the logic so the check is
only applied after the triage command actually failed or after the triage JSON
has been parsed: either (A) add a guard to run the is_rate_limited check only
when TRIAGE_RC != 0 (mirror the single-review/deep-review behavior), or (B) move
the is_rate_limited check to after the code that parses TRIAGE_RESULT into JSON
(so you inspect parsed fields instead of raw text), and if keeping the pattern,
narrow it to provider-specific error payload signatures rather than generic
"429"/"rate[ -]?limit". Ensure you modify the block that calls is_rate_limited
with TRIAGE_RESULT/TRIAGE_STDERR and the surrounding exit 2 path accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b8b6827-a53c-4e28-97c6-b36f19c128f2
📒 Files selected for processing (1)
scripts/review-one-pr.sh
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of the PR review script by enhancing error handling and rate-limit detection during GitHub API calls and model execution. Key changes include capturing explicit exit codes for background processes and checking both stdout and stderr for usage-cap messages. Feedback was provided regarding the need to clean up temporary error log files and ensure that temporary files are not leaked during early exit paths.
Comment on lines
+259
to
+260
| rm -f "$_gh_diff_tmp" | ||
| unset _gh_meta_err _gh_diff_err _gh_diff_tmp |
There was a problem hiding this comment.
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
- When passing large JSON payloads to curl, use a temporary file and the --data-binary @file flag to avoid exceeding the ARG_MAX limit.
- Keep error routing logic inline when tiers have distinct requirements to avoid obscuring intent or introducing false positives.
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Collaborator
Author
|
Both CodeRabbit actionable comments addressed in the latest commit (10a7921): Comment 1 — Triage false positive: Reverted to gating Comment 2 — Non-429 gh prefetch failures: Both metadata and diff failures now exit 1 with an explicit error message instead of silently falling back to Temp file cleanup: All three temp files are removed before every exit path (rate-limit skip, non-rate-limit error, normal completion). Redundant |
|
This was referenced May 16, 2026
This was referenced May 17, 2026
This was referenced May 18, 2026
don-petry
added a commit
that referenced
this pull request
Jun 7, 2026
* fix: catch GitHub API 429s that were silently swallowed in review-one-pr.sh
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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* fix: address CodeRabbit review comments on 429 handling
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) <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
Jun 7, 2026
* fix: catch GitHub API 429s that were silently swallowed in review-one-pr.sh
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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* fix: address CodeRabbit review comments on 429 handling
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) <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
Jun 12, 2026
* fix: catch GitHub API 429s that were silently swallowed in review-one-pr.sh
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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* fix: address CodeRabbit review comments on 429 handling
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) <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
Jun 14, 2026
* fix: catch GitHub API 429s that were silently swallowed in review-one-pr.sh
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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* fix: address CodeRabbit review comments on 429 handling
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) <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
This was referenced Jul 19, 2026
This was referenced Oct 10, 2026
don-petry
added a commit
that referenced
this pull request
Oct 11, 2026
… comments, and set the cap from measured PR peaks (#2224) (#2229) * fix(budget): stop counting the Dependabot merge pipeline, and stop marked agent comments re-arming the budget (#2224) The per-PR automation budget (#926) decided who is automated from the login alone. That was wrong in two directions. On a PR authored by dependabot[bot], Dependabot's own commit and the dependabot-automerge App's update-branch merge commits and re-approvals were counted as agent activity. A Dependabot PR waiting behind a busy main reached the cap after three or four branch updates, was labelled needs-human-review and lost auto-merge (broodminder-data#176, held 36 hours). Those events are now flagged `mechanical` and neither count nor reset. Both keys are required: any other bot on a Dependabot PR, and the App on anyone else's PR, count as before. dev-lead, the personas and auto-rebase post as the owner account, so each of their comments read as a human interaction and reset the budget. On #176 dev-lead's hold notice took the count from 10 to 0 a minute after the escalation. A comment or review whose body carries one of our automation markers is now flagged `agent_marked` and never resets the budget. It does not count either; only bot logins count. gather_pr_automation_events sets the flags, so every consumer inherits the rule. No event is dropped, because the stall and merge-ready scans read .when from all of them. An unreadable PR author exempts nothing. Commits, unmarked comments and review-thread replies made under a human login still read as human. That limit is documented. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(reviews): correct budget known-limit — inline thread replies are not collected * fix(bot): correct budget marker regex to prevent false positives when humans quote keywords The agent_marked regex was too loose with [^>]*, allowing arbitrary text after keywords. This caused humans quoting marker patterns to be incorrectly tagged as agents, preventing them from resetting the automation budget. Fixed by requiring proper delimiters (space, colon, or dash) after keywords without built-in delimiters (pr-review-agent, pr-review-claim, dev-lead, dependency-advisory, pr-automation-budget). Updated test 57 to use proper delimiter patterns instead of artificial 'x y=z' suffixes, reflecting the more restrictive regex that prevents false positives while still matching all valid marker formats. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * fix(bot): agent_marked should not match markers quoted in backticks The agent_marked jq function was using test($re) which matches the regex anywhere in the body. This caused false positives when a human quoted one of our automation markers (e.g., in a code example), incorrectly flagging the human's comment as agent-authored and preventing budget reset. Fixed by stripping backtick-enclosed content before testing, so quoted markers are not detected. This allows humans to quote markers when discussing code without triggering the false positive. Fixes the CodeAnt finding in #2229 (#2224 false-positive fix). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * fix(budget): count marked agent comments, and set the cap from measured PR peaks (#2224) Agent comments posted under the owner login now count toward the per-PR automation budget as well as no longer resetting it. Before this, dev-lead, persona and auto-rebase activity was invisible to the breaker. The cap of 10 was never set against that activity, so MAX_PR_AUTOMATION_CYCLES goes to 50, chosen from measurement to block only extreme cases. Sample: 332 agent-authored PRs (every one created in the org from 2026-09-10 to 2026-10-10, plus those still open). Peak count between two human interactions: median 10, p95 25, p99 35; highest on a PR that merged, 35; one PR (93) above 50. At a cap of 10 with marked comments counted, 50.9% of the sample would have been escalated at least once; at 50, 0.3%. #860 peaks at 1,487 under the same rule and reaches 50 within ten minutes of its ack storm. Stopping marked comments resetting the budget while keeping the cap at 10, as the first commit on this branch did, would by itself have raised the share of PRs escalated at least once from 4.8% to 16.6%. The runaway detector's RUNAWAY_MAX_CYCLES goes from 10 to 25 (half the cap, about p95) so the daily report still names a PR well before the breaker stops it. Tests that pinned the old cap now follow the default: the threshold cases, the two #860 storm scenarios (scaled to the cap in force), three fixtures that built exactly ten bot events, and the detector's threshold cases. docs: review-thread replies do reset the budget. Each reply arrives as a review with an empty body, and that review is collected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(budget): pin the review-thread-reply limit, and say why the marker match is not anchored (#2224) Review feedback on #2229 questioned whether a review-thread reply can reset the budget, since /pulls/{pr}/comments is never read. It can: the reply arrives on /pulls/{pr}/reviews as a COMMENTED review with an empty body, which is collected and carries no marker. A gather-level test now pins that known limit. The marker regex stays unanchored. In the comments of the 54 open PRs, 495 of 2,300 marker-carrying agent comments have the marker somewhere other than the start of the body, so anchoring would read a fifth of agent comments as human. Quoted markers are handled by dropping backtick spans, as dev-lead's commit did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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
Three places in
scripts/review-one-pr.shsilently absorbed GitHub API 429s, allowing the cascade to proceed with empty/wrong data rather than detecting the rate limit:Pre-fetch calls used
2>/dev/null || echo '{}': a gh 429 was suppressed entirely, handing the triage model{}metadata and an empty diff — producing a garbage verdict with no PR context. Now routes stderr to a temp file and callsis_rate_limitedbefore falling back; exits 100 (skip, retry next run) on gh rate-limit.Triage
is_rate_limitedgated onrc != 0: when the AI service rate-limited the triage call and the model still exited 0, the check block never ran. Now runs unconditionally for the triage tier — safe because triage has all tools denied and cannot callghinternally, so any 429 in its output unambiguously comes from the AI service, not from PR diff content passing through a tool result.wait $DEEP_PID || truediscarded the deep-review exit code: a non-zero exit without a JSON file leftDEEP_RCat 0, causing the failure branch (which includes the rate-limit check and error logging) to be skipped. Now useswait $DEEP_PID || DEEP_RC=$?and folds the process-exit check into the JSON-validity condition.Test plan
gh API rate limited … skipping PRand exits 100 rather than proceeding with empty context🤖 Generated with Claude Code
Summary by CodeRabbit