Skip to content

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

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 #40

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"
Comment on lines +167 to +169

@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. 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


# 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} ))
[ "$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
Comment on lines +213 to +216

@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. 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

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)."
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