fix(pr-agent): classify timeouts per ATTEMPT, not on total job time - #34
Open
yakimoto wants to merge 1 commit into
Open
fix(pr-agent): classify timeouts per ATTEMPT, not on total job time#34yakimoto 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_97a53224-61a8-4e10-8559-ab0d899d57a8) |
|
Warning Review limit reachedNext included review available in 29 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
ApprovabilityVerdict: Would Approve Macroscope's review found this PR approvable — This is a self-contained CI fix that corrects timeout classification from total job duration to the longest individual attempt. Existing triggers, retries, neutral rendering, and application behavior remain unchanged. Not approved because:
Review your spending limits in Billing settings. You can add or adjust custom eligibility rules. Learn more. |
PR Summary by QodoFix pr-agent timeout classification to use per-attempt duration
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
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 measure per attempt rather than cumulative job time, preventing false hang reports on retried runs. 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. Timeout slack too large
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+213
to
+217
| # 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 | ||
| echo "::warning::pr-agent TIMED OUT — the longest attempt ran ${LONGEST}s against a ${STEP_BUDGET_S}s per-attempt budget (attempt 1 ${A1}s, attempt 2 ${A2}s), so it was killed by its step timeout rather than returning an error. A hang, NOT a rate limit. Rendering NEUTRAL: an advisory reviewer must not block the PR (#3128)." |
There was a problem hiding this comment.
1. Timeout slack too large 🐞 Bug ≡ Correctness
The verdict step treats any longest-attempt duration ≥ (STEP_BUDGET_S - 15s) as “TIMED OUT”, which can misclassify genuine long-running failures (e.g., an error at ~350s) as a hang/step-timeout kill. This undermines the PR’s stated goal of avoiding confidently-wrong root-cause messages.
Agent Prompt
### Issue description
The verdict classifier declares a timeout when `LONGEST >= STEP_BUDGET_S - SLACK` with `SLACK=15`. This can label a non-timeout failure that happens late (but still before the step timeout) as “killed by step timeout / hang”, reintroducing the misclassification the PR is trying to fix.
### Issue Context
The workflow uses wall-clock stamps around each attempt and then classifies failures based on attempt duration.
### Fix Focus Areas
- `.github/workflows/pr-agent.yml[213-217]`
Suggested direction:
- Reduce `SLACK` to a very small value (e.g., 1–3s), or
- Use a two-tier message: only say “killed by step timeout” when `LONGEST >= STEP_BUDGET_S`, and use a less definitive message for near-timeout durations.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
2. Unconditional attempt2 end stamp 🐞 Bug ☼ Reliability
The workflow stamps ATTEMPT2_END with if: always() even when attempt 2 did not start, and the verdict computes A2 = ATTEMPT2_END - ATTEMPT2_START defaulting missing START to 0. If ATTEMPT2_START is absent for any reason while ATTEMPT2_END is set, A2 becomes an epoch-sized number and forces a bogus “TIMED OUT” classification.
Agent Prompt
### Issue description
`ATTEMPT2_END` is recorded unconditionally, but `A2` is computed as `ATTEMPT2_END - ATTEMPT2_START` with missing values defaulting to 0. If `ATTEMPT2_END` is present but `ATTEMPT2_START` is not, `A2` becomes extremely large and makes `LONGEST` exceed the timeout threshold, producing a wrong verdict.
### Issue Context
Attempt-2 steps are conditional on attempt-1 failure, but the end-stamp step is currently unconditional.
### 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 direction:
- Change “stamp attempt 2 end” to only run when attempt 2 was actually entered (e.g., `if: always() && steps.agent.outcome == 'failure'`).
- Make `A2` compute only when both start and end stamps are present; otherwise force `A2=0`.
ⓘ 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 GuideUpdates the pr-agent workflow’s diagnostic classifier to measure the longest individual attempt against the per-attempt timeout budget, preventing healthy retries plus backoff from being misreported as hangs while preserving existing retry and non-blocking behavior. Sequence diagram for per-attempt timeout classificationsequenceDiagram
participant Workflow
participant Agent as PR-Agent
participant Classifier
Workflow->>Workflow: stamp attempt 1 start
Workflow->>Agent: run attempt 1
Agent-->>Workflow: success or failure
Workflow->>Workflow: stamp attempt 1 end
alt attempt 1 failed
Workflow->>Workflow: sleep 45s
Workflow->>Workflow: stamp attempt 2 start
Workflow->>Agent: run attempt 2
Agent-->>Workflow: success or failure
Workflow->>Workflow: stamp attempt 2 end
end
Workflow->>Classifier: calculate A1, A2, and LONGEST
alt LONGEST >= STEP_BUDGET_S - 15
Classifier-->>Workflow: report TIMED OUT and render NEUTRAL
else neither attempt reached budget
Classifier-->>Workflow: report failed/rate-limit likely and render NEUTRAL
end
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.
User description
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 diagnostic-message fix only; retry, timeouts, and non-blocking check behavior are unchanged.
Overview
Fixes a misclassification in the pr-agent verdict step: two slow-but-healthy attempts plus backoff (~405s) were labeled as a hang because total job time was compared to the per-attempt 360s budget.
Each attempt now gets its own start/end stamps (
if: always()so a timeout-killed step still records an end). The classifier uses the longest attempt, with 15s slack for runner kill lag. Wall time is still logged but no longer decides hang vs rate-limit. Retry and NEUTRAL-never-block behavior are unchanged.Reviewed by Cursor Bugbot for commit cf58dad. Bugbot is set up for automated code reviews on this repo. Configure here.
CodeAnt-AI Description
Classify PR-Agent timeouts using individual attempts
What Changed
Impact
✅ Accurate timeout explanations✅ Fewer false hang reports after retries✅ Clearer distinction between timeouts and upstream errors💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by Sourcery
Classify PR-Agent failures using per-attempt durations to prevent healthy retries from being reported as hangs while preserving advisory, non-blocking behavior.
Bug Fixes:
Enhancements: