Skip to content

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

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

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

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


# 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
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 +213 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. 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

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