fix(pr-patrol): reduce compute waste with issue-type budgets and timeout abandonment#1826
fix(pr-patrol): reduce compute waste with issue-type budgets and timeout abandonment#1826
Conversation
|
Warning Rate limit exceeded
⌛ 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. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
|
This PR shares 1 file(s) with PR #1825:
Coordinate to avoid merge conflicts. Posted by PR Patrol — informational only. |
|
This PR shares 2 file(s) with PR #1824:
Coordinate to avoid merge conflicts. Posted by PR Patrol — informational only. |
…out abandonment PR Patrol was burning 90%+ of compute on failed attempts: - Timeouts (30 min each) didn't count toward abandonment, causing infinite retries - All issue types got the same 40-turn / 30-min budget regardless of complexity - No early-exit guidance in prompts, so Claude kept trying unfixable issues - Reflection used expensive Sonnet model for simple log analysis Changes: - Unify failure tracking: timeouts now count toward abandonment (2 failures = abandoned) - Add per-issue-type budgets: missing-issue-ref gets 5 turns/3 min, ci-failure gets 25/15, etc. - Add "when to stop early" section to prompts with clear unfixable-scenario detection - CI failure prompt now explicitly lists human-required checks to skip immediately - Reflection uses haiku model with 5-min timeout instead of sonnet/30-min - Include pr-patrol tests in vitest config - Add computeBudget tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
OAGr
force-pushed
the
claude/stupefied-chaum
branch
from
2990c70 to
a7a01cd
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
🤖 PR Patrol ran for 98s (40 max turns, model: sonnet). Issues detected: missing-issue-ref Result: |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crux/pr-patrol/index.ts (1)
1325-1336:⚠️ Potential issue | 🟠 MajorDon't record stop-early / no-op runs as
fixed.Any
exitCode === 0lands in the success path, even when the new prompt tells Claude to stop because the issue needs human action, is pre-existing onmain, or requires no change. Since nothing verifies the issue disappeared, those PRs just cool down and then get queued again unchanged. Please give “stopped early” a distinct outcome and persist that state so patrol can skip or abandon it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crux/pr-patrol/index.ts` around lines 1325 - 1336, When an exitCode === 0 run actually stopped early/no-op and did not fix the issue, don't mark it as 'fixed'—add a distinct outcome value (e.g., outcome = 'stopped_early') and branch before the existing success branch (the block that checks result.exitCode === 0 && !result.hitMaxTurns) to detect the early-stop/no-op condition (e.g., result.stoppedEarly or a heuristic on result.output/result.issues) and set outcome accordingly; update the PR comment logic (the githubApi POST that currently posts the success summary using result.output, effectiveMaxTurns and config.model) to reflect the stopped_early state and persist that outcome where PR statuses are saved (the same place other outcomes are recorded so patrol can skip/abandon these PRs on subsequent runs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crux/pr-patrol/index.ts`:
- Around line 188-208: The abandonment counter
(getFailCount/recordFailure/isAbandoned) is treated as "consecutive failures"
but is monotonic and never cleared; update the logic so the counter is reset or
scoped to the current run context: either clear the failures-<key> file after a
successful/no-op run (call an explicit reset that removes or writes 0 for
failures-<key> when checkMainBranch or the PR processing path succeeds), or
incorporate a run identifier (e.g., current run SHA or current relevant issue
set) into the key used by getFailCount/recordFailure/isAbandoned (e.g., use
MAIN_BRANCH_KEY+runSha) so counts are per-run; modify checkMainBranch and any
success path to call the reset function or to pass the scoped key to
isAbandoned/recordFailure.
- Around line 1446-1451: The current flow treats any resolved spawnClaude()
result as a successful reflection; modify the code after calling
spawnClaude(prompt, {...}) to treat runs where result.timedOut or
result.hitMaxTurns are true as incomplete/failed: do not append a normal
"Reflection complete" record or mark success in persistent state when those
flags are set, instead append/mark an incomplete result and log a
timeout/max-turns message. Check result.timedOut and result.hitMaxTurns
immediately after spawnClaude returns and branch accordingly before calling
whatever record-append or success-log logic currently emits "✓ Reflection
complete".
---
Outside diff comments:
In `@crux/pr-patrol/index.ts`:
- Around line 1325-1336: When an exitCode === 0 run actually stopped early/no-op
and did not fix the issue, don't mark it as 'fixed'—add a distinct outcome value
(e.g., outcome = 'stopped_early') and branch before the existing success branch
(the block that checks result.exitCode === 0 && !result.hitMaxTurns) to detect
the early-stop/no-op condition (e.g., result.stoppedEarly or a heuristic on
result.output/result.issues) and set outcome accordingly; update the PR comment
logic (the githubApi POST that currently posts the success summary using
result.output, effectiveMaxTurns and config.model) to reflect the stopped_early
state and persist that outcome where PR statuses are saved (the same place other
outcomes are recorded so patrol can skip/abandon these PRs on subsequent runs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a11e43b3-172f-4c86-a97b-087e53bd04f4
📒 Files selected for processing (2)
crux/pr-patrol/index.test.tscrux/pr-patrol/index.ts
| function getFailCount(key: number | string): number { | ||
| // Check both new and legacy file names for backwards compat | ||
| const newFile = join(STATE_DIR, `failures-${key}`); | ||
| const legacyFile = join(STATE_DIR, `max-turns-${key}`); | ||
| if (existsSync(newFile)) { | ||
| return parseInt(readFileSync(newFile, 'utf-8').trim(), 10) || 0; | ||
| } | ||
| if (existsSync(legacyFile)) { | ||
| return parseInt(readFileSync(legacyFile, 'utf-8').trim(), 10) || 0; | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| function recordMaxTurnsFailure(key: number | string): number { | ||
| const count = getMaxTurnsFailCount(key) + 1; | ||
| writeFileSync(join(STATE_DIR, `max-turns-${key}`), String(count)); | ||
| function recordFailure(key: number | string): number { | ||
| const count = getFailCount(key) + 1; | ||
| writeFileSync(join(STATE_DIR, `failures-${key}`), String(count)); | ||
| return count; | ||
| } | ||
|
|
||
| function isAbandoned(key: number | string): boolean { | ||
| return getMaxTurnsFailCount(key) >= 2; | ||
| return getFailCount(key) >= 2; |
There was a problem hiding this comment.
Reset or scope abandonment state instead of keeping it forever.
This counter is monotonic, but the rest of the flow treats it as “2 consecutive failures”. Once MAIN_BRANCH_KEY reaches 2, checkMainBranch() stops querying GitHub and returns isRed: false, so a later red-main incident can be skipped and normal merge processing continues. The same stale state also makes a later unrelated PR failure look like the second consecutive miss. Please clear the counter after a successful/no-op run, or scope it to the current run SHA / issue set before calling isAbandoned().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crux/pr-patrol/index.ts` around lines 188 - 208, The abandonment counter
(getFailCount/recordFailure/isAbandoned) is treated as "consecutive failures"
but is monotonic and never cleared; update the logic so the counter is reset or
scoped to the current run context: either clear the failures-<key> file after a
successful/no-op run (call an explicit reset that removes or writes 0 for
failures-<key> when checkMainBranch or the PR processing path succeeds), or
incorporate a run identifier (e.g., current run SHA or current relevant issue
set) into the key used by getFailCount/recordFailure/isAbandoned (e.g., use
MAIN_BRANCH_KEY+runSha) so counts are per-run; modify checkMainBranch and any
success path to call the reset function or to pass the scoped key to
isAbandoned/recordFailure.
| const result = await spawnClaude(prompt, { | ||
| ...config, | ||
| maxTurns: 10, // Reflection needs fewer turns | ||
| model: 'haiku', // Reflection is log analysis — doesn't need sonnet | ||
| timeoutMinutes: 5, // Should complete quickly | ||
| }); |
There was a problem hiding this comment.
Handle timed-out reflections as incomplete, not successful.
spawnClaude() resolves on timeout/max-turns, so with the new 5-minute cap this path can still append a normal record and log ✓ Reflection complete after a killed run. Check result.timedOut / result.hitMaxTurns before recording success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crux/pr-patrol/index.ts` around lines 1446 - 1451, The current flow treats
any resolved spawnClaude() result as a successful reflection; modify the code
after calling spawnClaude(prompt, {...}) to treat runs where result.timedOut or
result.hitMaxTurns are true as incomplete/failed: do not append a normal
"Reflection complete" record or mark success in persistent state when those
flags are set, instead append/mark an incomplete result and log a
timeout/max-turns message. Check result.timedOut and result.hitMaxTurns
immediately after spawnClaude returns and branch accordingly before calling
whatever record-append or success-log logic currently emits "✓ Reflection
complete".
- Reset failure counter on successful fix (was monotonic/never cleared) - Handle timed-out reflections as incomplete instead of success - Add 'no-op' outcome for stop-early runs (heuristic detection) - No-op runs increment failure counter to avoid infinite retries - 8 new tests for looksLikeNoOp detection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🛑 CodeRabbit Security GateAction required: This PR has 1 unresolved Critical/Major CodeRabbit finding(s). Either resolve each thread in the GitHub review interface, or add the Unresolved Critical/Major Findings (1)
|
- Split 1,560-line index.ts into 8 focused modules: types.ts, state.ts, detection.ts, scoring.ts, merge.ts, prompts.ts, execution.ts, reflection.ts - Add pr-patrol/**/*.test.ts to vitest config (tests were silently ignored) - Fix empty catch blocks violating error-handling rules - Add missing stale PR section in buildPrompt() - Fix summary comment mid-line truncation (take last ~500 chars by line) - Fix overlap comment replace() to use replaceAll() - Add staleHours CLI option (was env-var only) - Move state persistence from /tmp to ~/.cache/pr-patrol/state/ with backward-compatible migration from legacy location - Parallelize overlap detection file fetches (Promise.allSettled) - Include PR #1826 budget/timeout improvements in the decomposed modules - Add 46 new tests (scoring, prompts, detection) → 74 total Closes #1830 Closes #1831 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Split 1,560-line index.ts into 8 focused modules: types.ts, state.ts, detection.ts, scoring.ts, merge.ts, prompts.ts, execution.ts, reflection.ts - Add pr-patrol/**/*.test.ts to vitest config (tests were silently ignored) - Fix empty catch blocks violating error-handling rules - Add missing stale PR section in buildPrompt() - Fix summary comment mid-line truncation (take last ~500 chars by line) - Fix overlap comment replace() to use replaceAll() - Add staleHours CLI option (was env-var only) - Move state persistence from /tmp to ~/.cache/pr-patrol/state/ with backward-compatible migration from legacy location - Parallelize overlap detection file fetches (Promise.allSettled) - Include PR #1826 budget/timeout improvements in the decomposed modules - Add 46 new tests (scoring, prompts, detection) → 74 total Closes #1830 Closes #1831 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Split 1,560-line index.ts into 8 focused modules: types.ts, state.ts, detection.ts, scoring.ts, merge.ts, prompts.ts, execution.ts, reflection.ts - Add pr-patrol/**/*.test.ts to vitest config (tests were silently ignored) - Fix empty catch blocks violating error-handling rules - Add missing stale PR section in buildPrompt() - Fix summary comment mid-line truncation (take last ~500 chars by line) - Fix overlap comment replace() to use replaceAll() - Add staleHours CLI option (was env-var only) - Move state persistence from /tmp to ~/.cache/pr-patrol/state/ with backward-compatible migration from legacy location - Parallelize overlap detection file fetches (Promise.allSettled) - Include PR #1826 budget/timeout improvements in the decomposed modules - Add 46 new tests (scoring, prompts, detection) → 74 total Closes #1830 Closes #1831 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Split 1,560-line index.ts into 8 focused modules: types.ts, state.ts, detection.ts, scoring.ts, merge.ts, prompts.ts, execution.ts, reflection.ts - Add pr-patrol/**/*.test.ts to vitest config (tests were silently ignored) - Fix empty catch blocks violating error-handling rules - Add missing stale PR section in buildPrompt() - Fix summary comment mid-line truncation (take last ~500 chars by line) - Fix overlap comment replace() to use replaceAll() - Add staleHours CLI option (was env-var only) - Move state persistence from /tmp to ~/.cache/pr-patrol/state/ with backward-compatible migration from legacy location - Parallelize overlap detection file fetches (Promise.allSettled) - Include PR #1826 budget/timeout improvements in the decomposed modules - Add 46 new tests (scoring, prompts, detection) → 74 total Closes #1830 Closes #1831 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
PR Patrol was spending 90%+ of compute time on failed attempts — only 35 minutes out of 6.2 hours produced successful fixes. The rest was timeouts and max-turns failures, often on the same unfixable PRs repeatedly.
Root causes and fixes:
missing-issue-ref(a 10-secondgh pr edit) got the same budget as resolving merge conflicts. Now each issue type has a scaled budget:conflict: 40 turns / 30 minci-failure,bot-review-major: 25 turns / 15 minstale: 10 turns / 5 minmissing-testplan,bot-review-nitpick: 8 turns / 5 minmissing-issue-ref: 5 turns / 3 mincheck-protected-pathsneeding a human label). Prompt now has explicit "when to stop early" section listing unfixable scenarios.Test plan
computeBudgettests)pr-patrol/**/*.test.tsto vitest config (was missing)Related to #1791
🤖 Generated with Claude Code