Skip to content

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

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
+44 −6
Open

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

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} ))
[ "$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)."
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)."
Comment on lines +217 to +220

@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 changelog for verdict text 📘 Rule violation § Compliance

This PR changes user-visible GitHub Actions warning text for the pr-agent verdict, but
CHANGELOG.md has no Unreleased entry documenting the change. This violates the requirement to
document user-facing changes in the Unreleased section.
Agent Prompt 
## Issue description
The PR changes user-visible workflow output (the `::warning::pr-agent ...` verdict messages) but there is no corresponding entry under `## [Unreleased]` in the root `CHANGELOG.md`.

## Issue Context
Compliance requires documenting user-facing changes in the `Unreleased` section.

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

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

exit 0
Loading