mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-22 07:04:58 +00:00
feat(autofix): stop a PR that fails to push for N rounds in a row (#7482)
* 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 <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
This commit is contained in:
Shaojin Wen
GitHub
parent
cbf1c55595
commit
ca084dd11f
2 changed files with 211 additions and 1 deletions
62
.github/workflows/qwen-autofix.yml
vendored
62
.github/workflows/qwen-autofix.yml
vendored
|
|
@ -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("<!-- autofix-eval "))
|
||||
| select(
|
||||
((.body // "") | contains("win=" + $win + " -->"))
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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<!-- autofix-eval ts=x acted=y round=z${win ? ` win=${win}` : ''} -->`,
|
||||
};
|
||||
}),
|
||||
),
|
||||
);
|
||||
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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue