Skip to content

fix(pr-agent): classify timeouts per ATTEMPT, not on total job time #44

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

Open
yakimoto wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(pr-agent): classify timeouts per ATTEMPT, not on total job time #44

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
50 changes: 44 additions & 6 deletions .github/workflows/pr-agent.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,17 @@ jobs:
# Stamped so the verdict step can tell a TIMED-OUT attempt from a fast
# upstream error. Both arrive as outcome == 'failure' and GitHub exposes no
# step-level "timed_out", so elapsed time is the only discriminator there is.
- name: stamp attempt start
run: echo "AGENT_START=$(date +%s)" >> "$GITHUB_ENV"
#
# PER-ATTEMPT, and that is the whole fix. This used to be one AGENT_START
# stamped before attempt 1, with the verdict comparing TOTAL job time
# against STEP_BUDGET_S — a budget its own comment calls per-attempt. Two
# slow-but-healthy attempts (~180s each) plus the 45s backoff total ~405s
# and were reported as "TIMED OUT … a hang, NOT a rate limit", sending the
# next reader to debug a hang that never happened. The else-branch was
# equally wrong the other way, asserting the run was "well inside the
# budget" from a total that spans both attempts.
- name: stamp attempt 1 start
run: echo "ATTEMPT1_START=$(date +%s)" >> "$GITHUB_ENV"

- name: PR-Agent (OSS qodo-merge)
id: agent
Expand Down Expand Up @@ -114,10 +123,20 @@ jobs:
# 429s from the LLM router rendered this check RED with no retry, and
# pr-agent is an ADVISORY reviewer — it annotates, it never gates
# correctness — so a flaked reviewer must never block a PR.
# `if: always()` so an attempt KILLED by its step timeout still records an
# end stamp — that is precisely the case the classifier needs to see.
- name: stamp attempt 1 end
if: always()
run: echo "ATTEMPT1_END=$(date +%s)" >> "$GITHUB_ENV"

- name: backoff before retry
if: steps.agent.outcome == 'failure'
run: sleep 45

- name: stamp attempt 2 start
if: steps.agent.outcome == 'failure'
run: echo "ATTEMPT2_START=$(date +%s)" >> "$GITHUB_ENV"

- name: PR-Agent retry (attempt 2)
id: agent_retry
if: steps.agent.outcome == 'failure'
Expand Down Expand Up @@ -145,6 +164,10 @@ jobs:
pr_code_suggestions.suggestions_score_threshold: "7"
pr_code_suggestions.num_code_suggestions: "6"

- name: stamp attempt 2 end
if: always()
run: echo "ATTEMPT2_END=$(date +%s)" >> "$GITHUB_ENV"

# FOUR outcomes, not three (wave-pen#386). Branching on {success, failure,
# empty} alone sweeps everything else into "most commonly an upstream 429",
# so a job timeout and a concurrency supersede both report a rate limit that
Expand Down Expand Up @@ -174,10 +197,25 @@ jobs:
echo "::warning::pr-agent was CANCELLED, not failed — a newer run superseded this one via the concurrency group, or the job hit timeout-minutes. Not a reviewer or rate-limit fault (wave-pen#386)."
exit 0
fi
ELAPSED=$(( $(date +%s) - ${AGENT_START:-$(date +%s)} ))
if [ "$ELAPSED" -ge "$STEP_BUDGET_S" ]; then
echo "::warning::pr-agent TIMED OUT — ${ELAPSED}s against a ${STEP_BUDGET_S}s per-attempt budget, so an attempt 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)."
# PER-ATTEMPT durations, not total job time. STEP_BUDGET_S is the
# per-attempt step timeout; comparing it against a total spanning
# attempt 1 + 45s backoff + attempt 2 misclassified two healthy-but-slow
# attempts (~180s each, ~405s together) as a hang. Every value is
# defaulted so the arithmetic can never fail this step and turn the
# classifier into an error of its own.
NOW=$(date +%s)
A1=$(( ${ATTEMPT1_END:-0} - ${ATTEMPT1_START:-0} ))
A2=$(( ${ATTEMPT2_END:-0} - ${ATTEMPT2_START:-0} ))
Comment on lines +207 to +208

@qodo-code-review qodo-code-review Bot Aug 24, 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.

Remediation recommended

2. Attempt2 duration inflated 🐞 Bug ≡ Correctness

The workflow stamps ATTEMPT2_END unconditionally but only stamps ATTEMPT2_START when the retry is
scheduled, so A2 can become a large positive value (END-0) if attempt 2 never actually started. That
can drive LONGEST above the timeout threshold and incorrectly emit the "TIMED OUT" classification
for non-timeout failures.
Agent Prompt 
## Issue description
`ATTEMPT2_END` is stamped with `if: always()` even when attempt 2 never runs, while `ATTEMPT2_START` is only stamped when the retry is scheduled. In the verdict step, `A2=$((ATTEMPT2_END - ATTEMPT2_START))` then becomes a large positive value when `ATTEMPT2_START` is unset (defaults to 0), which can incorrectly make `LONGEST` exceed the step budget and misclassify a non-timeout failure as `TIMED OUT`.

## Issue Context
- `stamp attempt 2 start` is conditional on `steps.agent.outcome == 'failure'`.
- `stamp attempt 2 end` currently runs unconditionally (`if: always()`), so it can set `ATTEMPT2_END` even when attempt 2 never started.
- Verdict arithmetic defaults missing vars to `0`, which turns the missing-start case into a huge positive duration.

## Fix Focus Areas
- .github/workflows/pr-agent.yml[136-169]
- .github/workflows/pr-agent.yml[206-217]

## Suggested changes
1) Make `stamp attempt 2 end` conditional on attempt 2 being intended (while still using `always()`):
   - Example: `if: always() && steps.agent.outcome == 'failure'`
2) Defensively compute `A2` as `0` unless `ATTEMPT2_START` is set to a non-zero epoch value (or unless `AGENT_RETRY_OUTCOME` is non-empty):
   - Example:
     - `A2=0`
     - `if [ "${ATTEMPT2_START:-0}" -gt 0 ] && [ "${ATTEMPT2_END:-0}" -gt 0 ]; then A2=$((ATTEMPT2_END-ATTEMPT2_START)); fi`
This keeps `LONGEST` meaningful and prevents false timeout classification when the retry didn’t actually run.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[ "$A1" -lt 0 ] && A1=0
[ "$A2" -lt 0 ] && A2=0
LONGEST=$A1; [ "$A2" -gt "$LONGEST" ] && LONGEST=$A2
ELAPSED=$(( NOW - ${ATTEMPT1_START:-$NOW} ))
# 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)."
Comment on lines +216 to +217

@qodo-code-review qodo-code-review Bot Aug 24, 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.

Remediation recommended

1. Missing pr-agent changelog entry 📘 Rule violation ⚙ Maintainability

This PR changes the pr_agent check’s user-visible warning classification/text for timeout vs
failure in .github/workflows/pr-agent.yml, but CHANGELOG.md has no corresponding entry under `##
[Unreleased]`. This violates the requirement to document user-facing changes in the changelog.
Agent Prompt 
## Issue description
The PR introduces a user-visible change to PR check output/classification (`pr_agent` timeout vs failure messaging) but does not add an entry under `## [Unreleased]` in `CHANGELOG.md`.

## Issue Context
The compliance checklist requires updating `CHANGELOG.md` for user-facing changes.

## Fix Focus Areas
- CHANGELOG.md[7-29]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

exit 0
fi
echo "::warning::pr-agent failed after 2 attempts (45s backoff, ${ELAPSED}s total — well inside the ${STEP_BUDGET_S}s budget, so it returned an error rather than hanging) — most commonly an upstream 429/rate-limit from the LLM router. Rendering NEUTRAL: an advisory reviewer must not block the PR (#3128)."
echo "::warning::pr-agent failed after 2 attempts (attempt 1 ${A1}s, attempt 2 ${A2}s, ${ELAPSED}s wall including the 45s backoff — NEITHER attempt reached the ${STEP_BUDGET_S}s per-attempt budget, so it returned an error rather than hanging) — most commonly an upstream 429/rate-limit from the LLM router. Rendering NEUTRAL: an advisory reviewer must not block the PR (#3128)."
exit 0
Loading