fix(pr-agent): classify timeouts per ATTEMPT, not on total job time - #40
Open
yakimoto wants to merge 1 commit into
Open
fix(pr-agent): classify timeouts per ATTEMPT, not on total job time#40yakimoto wants to merge 1 commit into
yakimoto wants to merge 1 commit into
Conversation
Re-syncs this repo to wave-foundation-public#72, which landed after the inline lane was adopted here. THE DEFECT. The adopted template stamped AGENT_START once, before attempt 1, then compared TOTAL job time — attempt 1 + the 45s backoff + attempt 2 — against STEP_BUDGET_S=360, a budget its own comment calls PER-ATTEMPT. Two healthy-but-slow attempts (~180s each, ~405s together) therefore reported "pr-agent TIMED OUT ... A hang, NOT a rate limit." sending the next reader to debug a hang that never happened; the else-branch lied the other way, asserting the run was "well inside the budget" from the same misused total. Found by qodo review on wave-monitor#48 and confirmed against the file before acting. THE FIX. Stamp each attempt separately and classify on the LONGEST attempt, with if: always() end stamps so an attempt killed BY its step timeout still records one — exactly the case the classifier exists to catch. Total wall time is still reported as context but no longer decides the verdict. NOT URGENT, NOT IGNORABLE. The defect is in a MESSAGE, not in behaviour: the lane still retries, still renders NEUTRAL, still never blocks a PR. But that verdict step exists precisely because "a confidently wrong cause is worse than no cause", so shipping a classifier that can misname a hang defeats its purpose. Job id pr_agent and every on: trigger unchanged — the job id is the check-run context and branch protection matches on it. Refs wave-av/wave-pen#417, wave-av/wave-pen#388
🤖 CodeAnt AI — Review Status
|
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b4197ca9-82e5-4ffe-9e09-8e22e62020e7) |
|
Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 91 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
codeant-ai
Bot
added
the
size:M
PR Summary by QodoFix pr-agent timeout classification to use per-attempt duration
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
ApprovabilityVerdict: Would Approve Macroscope's review found this PR approvable — This is a self-contained, one-file CI diagnostic fix that changes timeout classification from total job time to per-attempt time. Existing review, retry, permissions, and non-blocking behavior remain unchanged, with no production or schema impact. Not approved because:
Review your spending limits in Billing settings. You can add or adjust custom eligibility rules. Learn more. |
|
Note Automatic reviews are paused because your team has used its included automatic processing for this billing period (headroom scales with your seat count). You can still comment "Gitar review" to run one anytime, and automatic reviews resume on their own by September 1. Add seats for more headroom. Code Review ✅ ApprovedRefactors PR-agent timeout classification to evaluate duration per attempt rather than total job time, preventing false hang reports on slow retries. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
Code Review by Qodo
1. Attempt2 end stamped unconditionally
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+167
to
+169
| - name: stamp attempt 2 end | ||
| if: always() | ||
| run: echo "ATTEMPT2_END=$(date +%s)" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
1. Attempt2 end stamped unconditionally 🐞 Bug ≡ Correctness
stamp attempt 2 end runs with if: always() even when attempt 2 never started, so ATTEMPT2_END can be set while ATTEMPT2_START is unset and A2 becomes a huge epoch-seconds duration. If the verdict duration math runs in any scenario where the retry did not actually execute, the classifier can incorrectly report a timeout/hang due to the inflated A2.
Agent Prompt
### Issue description
The workflow stamps `ATTEMPT2_END` unconditionally (`if: always()`), but `ATTEMPT2_START` is only set when attempt 2 actually starts. This creates a state where `ATTEMPT2_END` is populated and `ATTEMPT2_START` is unset, and the verdict’s `A2=$(( end - start ))` calculation can become an enormous epoch-seconds value, contaminating `LONGEST` and causing misclassification.
### Issue Context
Attempt 2 is only supposed to exist when `steps.agent.outcome == 'failure'`, but the end stamp currently runs even when attempt 2 was never started.
### Fix Focus Areas
- .github/workflows/pr-agent.yml[136-139]
- .github/workflows/pr-agent.yml[167-169]
- .github/workflows/pr-agent.yml[206-212]
### Suggested fix
- Make `stamp attempt 2 end` conditional, e.g. `if: steps.agent.outcome == 'failure'` (and optionally also require that attempt2 start happened).
- Additionally harden the verdict math so `A2=0` unless both `ATTEMPT2_START` and `ATTEMPT2_END` are set (or unless `ATTEMPT2_START>0`). This prevents any future reordering/partial execution from inflating `A2`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+213
to
+216
| # SLACK because a step killed AT its timeout records a hair under the | ||
| # budget — the runner's kill is not instantaneous. | ||
| SLACK=15 | ||
| if [ "$LONGEST" -ge $(( STEP_BUDGET_S - SLACK )) ]; then |
There was a problem hiding this comment.
2. Slack can mislabel timeouts 🐞 Bug ≡ Correctness
The verdict treats any attempt lasting ≥ STEP_BUDGET_S - 15 seconds as “TIMED OUT … killed by its step timeout,” which is not guaranteed (an attempt can legitimately fail with an upstream error at 345–359s). This reintroduces confidently-wrong classification on slow failures, undermining the stated goal of accurate cause labeling.
Agent Prompt
### Issue description
The classifier uses `SLACK=15` and triggers the “TIMED OUT … killed by its step timeout” message when `LONGEST >= STEP_BUDGET_S - SLACK`. That condition can be true even when the step was not killed by the timeout (e.g., an upstream error returned after 350s), so the message can still be confidently wrong.
### Issue Context
`STEP_BUDGET_S` is set to 360s and corresponds to `timeout-minutes: 6`. The current threshold is effectively 345s, but only durations at/near the actual timeout boundary should justify the hard claim “killed by its step timeout.”
### Fix Focus Areas
- .github/workflows/pr-agent.yml[181-182]
- .github/workflows/pr-agent.yml[213-217]
### Suggested fix
- Reduce slack to a minimal value (e.g. 1–3s) or compute slack as a small percentage capped to a few seconds.
- Alternatively (or additionally) change the warning wording to avoid asserting a timeout as fact when using a slack-based heuristic (e.g., “likely hit the step timeout” / “duration was within Xs of the timeout”).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo Fixer✅ Merged (0) · ☑ Fixed (0) Process
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideFixes the pr-agent workflow’s timeout classifier by measuring the longest individual attempt instead of total job time, preventing multiple healthy slow attempts plus backoff from being mislabeled as a hang while preserving retry, advisory, and non-blocking behavior. Sequence diagram for per-attempt timeout classificationsequenceDiagram
participant Workflow
participant Agent as PR_Agent
participant Verdict
Workflow->>Workflow: stamp attempt 1 start
Workflow->>Agent: run attempt 1
Workflow->>Workflow: stamp attempt 1 end
alt attempt 1 fails
Workflow->>Workflow: sleep 45
Workflow->>Workflow: stamp attempt 2 start
Workflow->>Agent: run attempt 2
Workflow->>Workflow: stamp attempt 2 end
end
Workflow->>Verdict: calculate A1, A2, LONGEST
alt LONGEST >= STEP_BUDGET_S - 15
Verdict-->>Workflow: render NEUTRAL: TIMED OUT
else
Verdict-->>Workflow: render NEUTRAL: failed / likely rate limit
end
Flow diagram for longest-attempt timeout decisionflowchart TD
A[Agent attempts complete] --> B[Read attempt start and end stamps]
B --> C[Calculate A1 and A2]
C --> D[Select LONGEST attempt]
D --> E{LONGEST >= budget minus 15s slack?}
E -->|Yes| F[Report TIMED OUT]
E -->|No| G[Report failed, likely upstream rate limit]
F --> H[Render NEUTRAL; do not block PR]
G --> H
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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
1 participant
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.
Re-syncs this repo to wave-foundation-public#72, which landed after the inline pr-agent lane was adopted here. Tracked as wave-pen#417.
The defect
The adopted template stamped
AGENT_STARTonce, before attempt 1, then compared total job time — attempt 1 + the 45s backoff + attempt 2 — againstSTEP_BUDGET_S=360, a budget its own comment calls per-attempt.Two healthy-but-slow attempts (~180s each, ~405s together) therefore reported:
…sending the next reader to debug a hang that never happened. The else-branch lied the other way, asserting the run was "well inside the budget" from the same misused total.
Found by qodo review on wave-monitor#48 and confirmed against the file before acting.
The fix
Stamp each attempt separately and classify on the longest attempt, with
if: always()end stamps so an attempt killed by its step timeout still records one — exactly the case the classifier exists to catch. Total wall time is still reported as context but no longer decides the verdict.failed after 2 attempts✅TIMED OUT✅Not urgent, not ignorable
The defect is in a message, not behaviour — the lane still retries, still renders NEUTRAL, still never blocks a PR. But that verdict step exists precisely because "a confidently wrong cause is worse than no cause", so a classifier that can misname a hang defeats its own purpose.
Job id
pr_agentand everyon:trigger unchanged — the job id is the check-run context and branch protection matches on it.Refs wave-av/wave-pen#417, wave-av/wave-pen#388
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Note
Low Risk
CI classifier messaging only; retry, NEUTRAL rendering, and job identity are unchanged, so PRs are not gated differently.
Overview
Fixes a misclassification in the advisory
pr-agentworkflow: two slow-but-healthy attempts plus backoff (~405s) were compared against the per-attempt 360s step budget and reported as a hang.Each attempt now gets its own start/end stamps (
if: always()so a timeout-killed step still records an end). The verdict uses the longest attempt, with 15s slack for runner kill lag. Wall time is still logged but no longer decides hang vs 429. Retry behavior and NEUTRAL (non-blocking) outcomes are unchanged.Reviewed by Cursor Bugbot for commit f12afa1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by Sourcery
Classify pr-agent failures using per-attempt execution time so slow retries are not incorrectly reported as timeouts while retaining non-blocking retry behavior.
Bug Fixes:
Enhancements: