From ca084dd11f390c729ebf5e570ee7121d7397a8c4 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 22 Jul 2026 20:14:24 +0800 Subject: [PATCH] feat(autofix): stop a PR that fails to push for N rounds in a row (#7482) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(autofix): stop a PR that fails to push for N rounds in a row Under takeover the round cap is 100, which is right for a PR that needs many PRODUCTIVE rounds. It is wrong for one that fails every round: #6723 ran 7 consecutive failed rounds (3 agent timeouts at 50 min, 4 gate rejections whose fix broke tests) over 8 hours, heading for round 100, because it is a 5700-line, 47-file, 5-day-old PR racing a fast-moving main — every round re-resolves a conflict it cannot finish or that fails the gate. Retrying at the same per-round budget will not converge; a human has to rebase or split it. Adds CONSECUTIVE_FAILURE_CAP (5), distinct from the total round cap. The handoff step already runs only when a round did NOT push, so it counts the unbroken run of prior failure markers — stopping at the first push ("Addressed the latest review feedback") or legitimate no-op ("no changes needed"), either of which proves progress and resets the streak. At the cap it forces the terminal round even under takeover, with a handoff that names the real fix (rebase/split, then /retry). Cause- agnostic: a timeout and a gate rejection both count. * fix(autofix): address review feedback on consecutive-failure circuit breaker (#7482) - Fix misleading comment: the walk is oldest-first (API order) with reset-on-success, not newest-first with early stop - Prefer the already-fetched ic.json over a redundant gh api call, falling back to the API only when the file is missing - Filter eval markers by re-arm window (win=) so pre-re-arm failures do not immediately re-terminate a re-armed PR - Add test coverage for the MARK_ROUND == MAX_ROUNDS guard and for window-scoped streak counting * fix(autofix): exempt transient model errors from consecutive-failure breaker (#7482) --------- Co-authored-by: wenshao Co-authored-by: qwen-code-dev-bot --- .github/workflows/qwen-autofix.yml | 62 ++++++++ scripts/tests/qwen-autofix-workflow.test.js | 150 +++++++++++++++++++- 2 files changed, 211 insertions(+), 1 deletion(-) diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 2d528dbf93..5e498ba004 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -161,6 +161,17 @@ env: # explicitly delegated work; removing the label restores the strict cap, # and re-engaging opens a fresh counting window (see REARM_KEY below). TAKEOVER_MAX_ROUNDS: '100' + # Consecutive-failure sub-cap, distinct from the total round cap above. The + # total cap bounds how many PRODUCTIVE rounds a PR may take; this bounds how + # many rounds may fail IN A ROW with nothing pushed. Under takeover a PR gets + # up to 100 rounds, but a PR that fails to push this many times running is not + # iterating, it is stuck — a too-large / fast-conflicting PR whose fix keeps + # timing out or failing the gate. Retrying at the same budget will not fix + # that; a human has to rebase or split it. Any pushed round OR a legitimate + # "no changes needed" no-op resets the streak, so this only ever fires on an + # unbroken run of failures. Observed on #6723: 7 straight failed rounds (3 + # timeouts, 4 gate rejections) over 8 hours, heading for 100. + CONSECUTIVE_FAILURE_CAP: '5' # Do not claim more issues when too many existing autofix PRs are still open. MAX_OPEN_AUTOFIX_PRS: '5' @@ -3344,6 +3355,57 @@ jobs: MARK_ROUND="${MAX_ROUNDS}" HEADLINE="🤖 AutoFix could not start evaluation — it crashed or timed out before reading the feedback, so no fix was attempted. This PR is now marked terminal and future scans (including forced dispatch) will skip it. To recover: delete this bot's terminal \`autofix-eval\` marker comment, then re-trigger if the failure looked transient." fi + + # Consecutive-failure circuit breaker, distinct from the round cap. + # Reaching this step at all means this round did NOT push (the push + # and no-op paths report from "Push and report"), so this round is a + # failure. Count how many failures precede it WITHOUT a break: walk + # the bot's prior eval markers in API order (oldest-first, pinned + # by sort_by so a stray reorder cannot corrupt the streak) and + # reset the streak at each push ("Addressed the latest review + # feedback") or deliberate no-op ("no changes needed"). After the + # full walk, CONSEC_FAIL holds failures since the last progress + # point plus one for this round. If the unbroken + # streak (this round included) reaches the cap, stop retrying even + # under takeover: a PR that fails this many times running is stuck + # on something a re-run at the same budget will not fix (observed on + # #6723: 7 straight failures, 3 timeouts + 4 gate rejections). Only + # overrides a would-be RETRY — a round already terminal for another + # reason keeps its own headline. + # Transient model errors (429/5xx) are exempt: the CAUSE_MAX logic + # above deliberately gives them the full round budget because they + # self-heal once the provider recovers. Letting the breaker override + # that would mark every in-flight PR terminal at once during a + # provider outage — the failures are not the PR's fault and DO + # self-heal. Auth errors are NOT exempt (they never self-heal). + if [[ "${MARK_ROUND}" != "${MAX_ROUNDS}" ]] && { [[ -z "${API_ERROR_DETAIL}" ]] || [[ "${API_ERROR_KIND}" == 'auth' ]]; }; then + CONSEC_FAIL=1 + if [[ -f "${WORKDIR}/ic.json" ]]; then + COMMENTS_JSON="$(cat "${WORKDIR}/ic.json")" + else + COMMENTS_JSON="$(gh api "repos/${REPO}/issues/${PR}/comments" --paginate 2> /dev/null || true)" + fi + PRIOR_HEADS="$(jq -r --arg ab "${AUTOFIX_BOT}" --arg win "${WINDOW:-none}" ' + [.[] | select((.user.login // "") == $ab) + | select((.body // "") | contains("")) + or ($win == "none" and (((.body // "") | contains("win=")) | not)))] + | sort_by(.created_at) | .[] + | (.body | gsub("\r"; "") | split("\n")[0])' <<< "${COMMENTS_JSON}" 2> /dev/null || true)" + while IFS= read -r H; do + [[ -n "${H}" ]] || continue + if [[ "${H}" == *"Addressed the latest review feedback"* || "${H}" == *"no changes needed"* ]]; then + CONSEC_FAIL=1 + else + CONSEC_FAIL=$(( CONSEC_FAIL + 1 )) + fi + done <<< "${PRIOR_HEADS}" + if [[ "${CONSEC_FAIL}" -ge "${CONSECUTIVE_FAILURE_CAP}" ]]; then + MARK_ROUND="${MAX_ROUNDS}" + HEADLINE="🤖 AutoFix stopped after ${CONSEC_FAIL} consecutive rounds that failed to push anything (timeouts and/or gate rejections). Retrying at the same per-round budget is not converging — this usually means the PR is too large or conflicts with a fast-moving \`main\`. A human should rebase, split, or reduce it, then comment \`${RETRY_COMMAND}\` to re-arm. Until then future scans will skip this PR." + fi + fi { echo "${HEADLINE}" echo diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 090745de7f..a4c3921d1c 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -3945,8 +3945,11 @@ describe('qwen-autofix workflow', () => { // judged the work at all; advancing there strands a fix the agent had // already written, which is exactly how the nested-package ENOENT stranded // #7329/#7336 until a human deleted the marker. + // Ends at the crash decision's own closing `fi`; the consecutive-failure + // block that follows is a separate unit with its own test, so anchor on it + // rather than the report `{` (which it now sits before). const decision = reviewAddressReportStep.match( - /(GATE_CRASHED=false\n[\s\S]*?\n {12}fi)\n {12}\{/, + /(GATE_CRASHED=false\n[\s\S]*?\n {12}fi)\n\n {12}# Consecutive-failure/, )?.[1]; expect(decision).toBeTruthy(); const SENTINEL = '9999-12-31T23:59:59Z'; @@ -4057,6 +4060,151 @@ describe('qwen-autofix workflow', () => { ).toContain('attempt 3/5'); }); + it('stops a PR that fails to push for CONSECUTIVE_FAILURE_CAP rounds in a row', () => { + // The total round cap bounds productive iteration; this bounds an UNBROKEN + // run of failures under takeover, where the strict cap does not apply. + // Observed on #6723: 7 straight failed rounds (3 timeouts, 4 gate + // rejections) heading for round 100, each ~50 min. Any push or legitimate + // no-op resets the streak; only consecutive failures count. + const cap = Number(workflow.match(/CONSECUTIVE_FAILURE_CAP: '(\d+)'/)?.[1]); + expect(cap).toBeGreaterThan(0); + // The sub-cap must be below the takeover cap or it never binds there. + const takeoverCap = Number( + workflow.match(/TAKEOVER_MAX_ROUNDS: '(\d+)'/)?.[1], + ); + expect(cap).toBeLessThan(takeoverCap); + + const block = reviewAddressReportStep.match( + /if \[\[ "\$\{MARK_ROUND\}" != "\$\{MAX_ROUNDS\}" \]\] && \{ \[\[ -z "\$\{API_ERROR_DETAIL\}" \]\] \|\| \[\[ "\$\{API_ERROR_KIND\}" == 'auth' \]\]; \}; then\n {14}CONSEC_FAIL=1\n[\s\S]*?\n {14}fi\n {12}fi\n/, + )?.[0]; + expect(block).toBeTruthy(); + const script = block.replace(/^ {12}/gm, ''); + + const FAIL = + '🤖 Could not address the latest feedback automatically (round 3/100).'; + const FAIL_TIMEOUT = '🤖 AutoFix could not reach the model (attempt 2/3)'; + const PUSH = '🤖 Addressed the latest review feedback (round 2/100).'; + const NOOP = '🤖 Reviewed the latest feedback — no changes needed.'; + + const run = ( + priorHeadlines, + { window, markRound = 7, apiErrorDetail = '', apiErrorKind = '' } = {}, + ) => { + const dir = mkdtempSync(join(tmpdir(), 'consec-')); + const bin = join(dir, 'bin'); + mkdirSync(bin); + writeFileSync( + join(dir, 'ic.json'), + JSON.stringify( + priorHeadlines.map((h, i) => { + const headline = typeof h === 'string' ? h : h.headline; + const win = typeof h === 'string' ? undefined : h.win; + return { + user: { login: 'qwen-code-dev-bot' }, + created_at: `2026-01-01T00:${String(i).padStart(2, '0')}:00Z`, + body: `${headline}\n`, + }; + }), + ), + ); + writeFileSync( + join(bin, 'gh'), + `#!/usr/bin/env bash\ncat ${JSON.stringify(join(dir, 'ic.json'))}\n`, + ); + chmodSync(join(bin, 'gh'), 0o755); + const out = execFileSync( + 'bash', + [ + '-c', + `set -uo pipefail\nWORKDIR='${dir}'\nMARK_ROUND=${markRound}\nMAX_ROUNDS=100\nCONSECUTIVE_FAILURE_CAP=${cap}\nCONSEC_FAIL=0\nREPO=o/r\nPR=1\nAUTOFIX_BOT=qwen-code-dev-bot\nRETRY_COMMAND='@qwen-code /retry'\nAPI_ERROR_DETAIL='${apiErrorDetail}'\nAPI_ERROR_KIND='${apiErrorKind}'\n${window !== undefined ? `WINDOW='${window}'\n` : ''}HEADLINE=orig\n${script}\nprintf '%s|%s|%s' "$MARK_ROUND" "${'${CONSEC_FAIL}'}" "$HEADLINE"`, + ], + { + env: { ...process.env, PATH: `${bin}:${process.env.PATH}` }, + encoding: 'utf8', + }, + ); + rmSync(dir, { recursive: true, force: true }); + const [mark, consec, headline] = out.split('|'); + return { + mark, + consec: Number(consec), + terminal: mark === '100', + headline, + }; + }; + + // This round alone (no prior failures) never terminates. + expect(run([])).toMatchObject({ consec: 1, terminal: false }); + // cap-1 prior failures + this round = cap → terminal, with the structural + // handoff, not the ordinary "could not address". + const capped = run(Array(cap - 1).fill(FAIL)); + expect(capped).toMatchObject({ consec: cap, terminal: true }); + expect(capped.headline).toContain('consecutive'); + expect(capped.headline).toContain('/retry'); + // One short of the cap keeps retrying. + expect(run(Array(cap - 2).fill(FAIL))).toMatchObject({ terminal: false }); + // A push resets the streak — failures before it do not count. + expect(run([FAIL, FAIL, PUSH, FAIL, FAIL])).toMatchObject({ + consec: 3, + terminal: false, + }); + // A legitimate no-op resets it too (the loop was caught up, not stuck). + expect(run([...Array(cap).fill(FAIL), NOOP, FAIL])).toMatchObject({ + consec: 2, + terminal: false, + }); + // Prior-round headlines are cause-agnostic: timeouts and gate rejections + // both count toward the streak. + expect(run([FAIL, FAIL_TIMEOUT, FAIL, FAIL_TIMEOUT])).toMatchObject({ + consec: cap, + terminal: true, + }); + // A transient (non-auth) model error on the CURRENT round skips the + // breaker entirely — the CAUSE_MAX logic above gives it the full budget + // because it self-heals, and the breaker must not override that. + expect( + run(Array(cap - 1).fill(FAIL), { + apiErrorDetail: 'terminated', + apiErrorKind: 'transient', + }), + ).toMatchObject({ terminal: false, headline: 'orig' }); + // An auth error on the current round is NOT exempt — it never self-heals. + expect( + run(Array(cap - 1).fill(FAIL), { + apiErrorDetail: 'access denied', + apiErrorKind: 'auth', + }), + ).toMatchObject({ consec: cap, terminal: true }); + // Already-terminal rounds skip the circuit breaker entirely. + expect(run(Array(cap).fill(FAIL), { markRound: 100 })).toMatchObject({ + terminal: true, + headline: 'orig', + }); + // The reset detector keys on literal substrings; pin them to the actual + // "Push and report" emit lines so a reword breaks this test, not silently + // the streak reset in production. + const pushEmit = pushAndReportStep.match( + /echo "(🤖 Addressed the latest review feedback[^"]*)"/, + ); + expect(pushEmit).toBeTruthy(); + expect(pushEmit[1]).toContain('Addressed the latest review feedback'); + const noopEmit = pushAndReportStep.match( + /echo "(🤖 Reviewed the latest feedback — no changes needed[^"]*)"/, + ); + expect(noopEmit).toBeTruthy(); + expect(noopEmit[1]).toContain('no changes needed'); + // Window filtering: pre-re-arm failures don't count after a re-arm. + expect( + run( + [ + ...Array(cap - 1).fill({ headline: FAIL, win: 'old-window' }), + { headline: FAIL, win: 'current-window' }, + ], + { window: 'current-window' }, + ), + ).toMatchObject({ consec: 2, terminal: false }); + }); + it('makes every known gate rejection declare its verdict', () => { // The retry/advance split above is only sound while each real rejection // writes outcome=failed; an unwired check would read as a gate crash and be