From fbf5313c8a554da8ab2178f870a355cd194bdf06 Mon Sep 17 00:00:00 2001 From: "wave-av-bot[bot]" Date: Mon, 24 Aug 2026 09:38:11 -0400 Subject: [PATCH] fix(pr-agent): classify timeouts per ATTEMPT, not on total job time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/pr-agent.yml | 50 ++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 6 deletions(-) diff --git a/.github/workflows/pr-agent.yml b/.github/workflows/pr-agent.yml index abb7b31a..b12f4d13 100644 --- a/.github/workflows/pr-agent.yml +++ b/.github/workflows/pr-agent.yml @@ -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 @@ -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' @@ -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 @@ -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)." exit 0