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