Skip to content
81 changes: 73 additions & 8 deletions .github/workflows/qwen-autofix.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2573,7 +2573,7 @@ jobs:
if: |-
${{ needs.review-scan.outputs.has_targets == 'true' }}
runs-on: 'ubuntu-latest'
timeout-minutes: 150
timeout-minutes: 300
permissions:
contents: 'read'
strategy:
Expand Down Expand Up @@ -3374,13 +3374,48 @@ jobs:
# revalidation in prepare) — no agent run, no marker, no comment.
if: |-
${{ steps.prepare.outputs.stale != 'true' }}
# Bound the agent well below the 150-minute job timeout so a runaway agent
# fails THIS step (not the whole job), leaving the always() verify and
# report steps time to run and post a handoff. A job-level timeout would
# cancel those steps too and leave the loop silent. The 80-minute primary
# attempt plus one 20-minute repair leaves ~50 minutes for setup, two
# verification passes, and reporting.
timeout-minutes: 80
# Bound the agent below the job timeout so a runaway agent fails THIS
# step (not the whole job), leaving the always() verify and report
# steps time to run and post a handoff. A job-level timeout would
# cancel those steps too and leave the loop silent.
#
# This step timeout is the BACKSTOP for a runaway that ignores the
# agent's own timer; QWEN_TIMEOUT_MS below is the real budget. The two
# had drifted 30 minutes apart — run-agent.mjs defaults to 50 minutes
# against what was an 80-minute cap — so a third of the step was
# unreachable and every "ran out of time" round ended for a reason
# this file never mentioned.
#
# Every number below is measured, from one scan (run 30646547838):
#
# setup (12 steps, ends at 'Post autofix status comment') 5-7m
# Triage and address #8005 round 9 50m03s (its own timer)
# #8211 12m45s
# Verification gate #8211 22m48s <- the big one, and
# it had NO step bound of its own until now
# push + report + finalize 3-4s
#
# Setup runs in EARLIER steps, so it never competes with the agent for
# this cap. The old arithmetic ("80 + 20 leaves ~50 for setup, two
# verification passes, and reporting") was wrong once verification was
# measured: 7 + 80 + 23 + 20 + 23 = 153 against a 150-minute job, so
# the worst case already overran the job timeout — the silent-loop
# failure this whole design exists to avoid.
#
# Sized for the PRs that actually time out (47 and 35 changed files),
# since those are the ones the budget is for:
#
# setup 7
# Triage and address 130 (120 budget + 10 margin)
# Verification gate 60 (2.6x the measured 22m48s)
# Repair 20
# Repair verification 60
# report 3
# -------------------------------
# worst case 280 => job timeout 300, and the job runs
# on ubuntu-latest, whose own ceiling
# is 360.
timeout-minutes: 130
env:
PR: '${{ env.PR }}'
ISSUE: '${{ env.ISSUE }}'
Expand All @@ -3389,6 +3424,26 @@ jobs:
OPENAI_MODEL: '${{ vars.QWEN_AUTOFIX_MODEL || vars.QWEN_PR_REVIEW_MODEL }}'
NO_PROXY: '127.0.0.1,localhost,::1'
QWEN_HOME: '${{ runner.temp }}/qwen-autofix-review-home'
# The primary attempt's real budget. Left implicit until now, so it
# silently took run-agent.mjs's 50-minute default and left 30 of the
# step's 80 minutes unreachable. Stated here, next to the cap it
# must stay under, because a budget nobody can see is a budget
# nobody re-checks when the cap moves.
#
# 120m, with a 10-minute margin under the step backstop above —
# enough for the internal kill path, which only writes
# `agent-timeout` and exits (measured: the 3 seconds between the
# agent's death and the job's last step). That file is how the
# report step tells a timeout apart from a crash, so if the step
# cap ever fires first the round is misreported.
#
# Raising it does NOT make a doomed round cheaper — the workflow's
# own breakers exist because each exhausted budget pushes nothing —
# so TIMEOUT_WINDOW_CAP still stops the PR after 3 in a window. This
# buys rounds that were close to finishing, not rounds that were
# never going to. QWEN_AUTOFIX_TIMEOUT_MS overrides it without a
# code change; a malformed value falls back to the runner default.
QWEN_TIMEOUT_MS: '${{ vars.QWEN_AUTOFIX_TIMEOUT_MS || 7200000 }}'
CONFLICT: '${{ steps.prepare.outputs.conflict }}'
BASE: 'main'
SETTINGS_JSON: |-
Expand Down Expand Up @@ -3456,6 +3511,14 @@ jobs:
if: |-
${{ always() && steps.prepare.outputs.stale != 'true' }}
continue-on-error: true
# Unbounded until now, and the largest consumer in the job (22m48s
# measured on #8211). Left unbounded it eats the job timeout, and a
# JOB timeout cancels the always() reporters — the silent round this
# design exists to prevent. Bounded here it degrades to the ordinary
# verification-failure path instead: continue-on-error keeps the job
# alive, 'Finalize verification' sees an empty outcome, falls through
# its case to exit 1, and the always() report step posts.
timeout-minutes: 60
run: |-
bash "${RUNNER_TEMP}/run-autofix-review-verification.sh"

Expand Down Expand Up @@ -3560,6 +3623,8 @@ jobs:
if: |-
${{ always() && steps.repair.outputs.attempted == 'true' }}
continue-on-error: true
# Same bound as the first pass, for the same reason.
timeout-minutes: 60
run: |-
bash "${RUNNER_TEMP}/run-autofix-review-verification.sh"

Expand Down
93 changes: 85 additions & 8 deletions scripts/tests/qwen-autofix-workflow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8318,18 +8318,95 @@ describe('qwen-autofix workflow', () => {
});

it('replays the handoff decision and terminal-round transitions under bash', () => {
// The primary + repair steps are bounded below the 150-minute job timeout
// so a runaway
// agent fails the STEP, not the job, leaving the always() report step time to
// run (a job-level timeout would cancel that step too and go silent).
// 150 is the review-address job timeout (unique; other jobs use 5/15/180).
expect(workflow).toContain('timeout-minutes: 150');
// Every long step is bounded BELOW the job timeout so a runaway fails its
// own STEP, leaving the always() report step time to run — a job-level
// timeout cancels that step too and the round goes silent.
//
// The invariant is the SUM, not any single number, and asserting the
// numbers individually is what let it break: with an unbounded
// verification gate measured at 22m48s, the worst case was
// 7 + 80 + 23 + 20 + 23 = 153 against a 150-minute job, and every
// assertion here still passed. Derive the whole budget from the workflow
// and check it fits.
const addressStep =
workflow.match(
/- name: 'Triage and address'[\s\S]*?(?=\n {6}- name: )/,
)?.[0] ?? '';
expect(addressStep).toContain('timeout-minutes: 80');
expect(repairDeterministicRejectionStep).toContain('timeout-minutes: 20');
// review-address is the LAST job in the file, so the terminator has to
// accept end-of-input as well as the next job header.
const jobBlock =
workflow.match(
/\n {2}review-address:\n[\s\S]*?(?=\n {2}\w[\w-]*:\n|$)/,
)?.[0] ?? '';
expect(jobBlock).toBeTruthy();
const jobCapMin = Number(
jobBlock.match(/\n {4}timeout-minutes: (\d+)/)?.[1],
);
expect(Number.isFinite(jobCapMin)).toBe(true);

// Sum every bounded step in the job. `always()` steps all run, so the
// worst case is the sum, not the max.
const stepCaps = [
...jobBlock.matchAll(/\n {8}timeout-minutes: (\d+)/g),
].map((m) => Number(m[1]));
// Primary, first verification, repair, repair verification. A step that
// loses its bound drops out of this list and shrinks the sum, so the
// count is asserted too.
expect(stepCaps).toHaveLength(4);
// Measured on run 30646547838: setup 5-7m in earlier steps, reporting 3-4s.
const SETUP_AND_REPORT_MIN = 10;
const worstCaseMin =
stepCaps.reduce((a, b) => a + b, 0) + SETUP_AND_REPORT_MIN;
expect(worstCaseMin).toBeLessThanOrEqual(jobCapMin);
Comment on lines +8366 to +8368

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined — this one is a false positive. The assertions in this file do gate changes in CI: ci.yml runs npm run test:ci as its main unit-test step (the Run tests and generate reports step, plus the macOS/Windows merge-queue jobs), and the root package.json defines test:ci as ... && npm run test:scripts, which collects scripts/tests/qwen-autofix-workflow.test.js. So a future change that breaks the budget-sum invariant fails CI on every PR today; the failure scenario (regression ships silently because none of the assertions run) cannot occur. A separate step would be a redundant duplicate of that existing gate, so I'm leaving it out to avoid diff growth. Happy to add a dedicated step anyway if a maintainer wants the script tests visible as their own CI line rather than chained inside test:ci.

中文说明

拒绝——这一条是误报。本文件的断言确实在 CI 中把关:ci.yml 把 npm run test:ci 作为其主要单测步骤运行(Run tests and generate reports 步骤,外加 macOS/Windows 的 merge-queue job),而根 package.json 将 test:ci 定义为 ... && npm run test:scripts,后者会收集 scripts/tests/qwen-autofix-workflow.test.js。因此,未来任何破坏预算求和不变量的改动今天都会在每个 PR 的 CI 上失败;其失败场景(因为断言从不运行而静默合入回归)不可能发生。单独新增一个步骤只是对现有这道门的冗余重复,因此为避免 diff 膨胀我不加入。如果维护者希望把脚本测试作为独立的一条 CI 显示、而不是链在 test:ci 内部,我也乐意照做。

// ubuntu-latest cannot run a job longer than 6 hours whatever we write.
expect(jobCapMin).toBeLessThanOrEqual(360);

// Both verification gates must stay bounded: unbounded, the larger one
// consumed the job budget and took the always() reporters down with it.
for (const name of ['Verification gate', 'Repair verification gate']) {
const gate =
jobBlock.match(
new RegExp(`- name: '${name}'[\\s\\S]*?(?=\\n {6}- name: )`),
)?.[0] ?? '';
expect(gate, `${name} is missing from review-address`).toBeTruthy();
expect(gate).toMatch(/\n {8}timeout-minutes: \d+/);
// continue-on-error is what makes bounding them a graceful degrade
// rather than a new way to kill the job.
expect(gate).toContain('continue-on-error: true');
}

// The step cap is only a backstop for a runaway that ignores the agent's
// own timer; QWEN_TIMEOUT_MS is the budget that actually ends a round.
// Leaving it implicit let the two drift 30 minutes apart — run-agent.mjs
// defaults to 50 minutes against an 80-minute cap, so a third of the
// step was unreachable and every "ran out of time" round stopped early
// for a reason no one could see in this file. Derive BOTH numbers from
// the workflow rather than restating them: a pair asserted as two
// literals cannot catch the pair drifting.
const stepCapMin = Number(addressStep.match(/timeout-minutes: (\d+)/)?.[1]);
const budgetMs = Number(
addressStep.match(
/QWEN_TIMEOUT_MS: '\$\{\{[^}]*\|\|\s*(\d+)\s*\}\}'/,
)?.[1],
);
expect(Number.isFinite(stepCapMin)).toBe(true);
expect(Number.isFinite(budgetMs)).toBe(true);
const marginMin = stepCapMin - budgetMs / 60000;
// Under the cap, or the cap fires first and the internal kill path never
// writes `agent-timeout` — the file the report step reads to tell a
// timeout apart from a crash.
expect(marginMin).toBeGreaterThan(0);
// ...and far enough under it that the kill path (which only writes that
// file and exits — measured at 3s on run 30646547838) always completes.
expect(marginMin).toBeGreaterThanOrEqual(5);
// The repair attempt stays inside its own smaller step the same way.
const repairCapMin = Number(
repairDeterministicRejectionStep.match(/timeout-minutes: (\d+)/)?.[1],
);
const repairMs = Number(
repairDeterministicRejectionStep.match(/QWEN_TIMEOUT_MS: '(\d+)'/)?.[1],
);
expect(repairCapMin - repairMs / 60000).toBeGreaterThan(0);

// Replay the ACTUAL POST_HANDOFF decision extracted from the workflow so the
// state transitions are exercised, not merely string-matched.
Expand Down
Loading