diff --git a/.github/scripts/run-autofix-review-verification.sh b/.github/scripts/run-autofix-review-verification.sh index 92a89ec52e5..da474d4852a 100755 --- a/.github/scripts/run-autofix-review-verification.sh +++ b/.github/scripts/run-autofix-review-verification.sh @@ -43,25 +43,166 @@ git checkout "${BRANCH}" GATE_LOG="${WORKDIR}/gate-output.log" : > "${GATE_LOG}" reject_fix() { - echo "❌ ${1}" + local label="${1}" + local preexisting="${2:-false}" + local retryable="${3:-true}" + echo "❌ ${label}" # Declare the verdict before writing its detail. An empty outcome on a failed # step means the gate itself crashed, so losing the detail file must not turn # a deterministic rejection into an infrastructure retry. echo "outcome=failed" >> "${GITHUB_OUTPUT}" - echo "retryable=true" >> "${GITHUB_OUTPUT}" + if [[ "${preexisting}" == 'true' ]]; then + # NOT retryable: the repair agent is only allowed to amend this round's + # fix, and a failure that exists without the fix is outside that boundary + # by definition — the 18-minute repair budget cannot reach it. The remedy + # is a base update (merge main into the branch), not a repair. + echo "preexisting=true" >> "${GITHUB_OUTPUT}" + elif [[ "${retryable}" == 'true' ]]; then + echo "retryable=true" >> "${GITHUB_OUTPUT}" + fi + # The evidence tail flexes so the WHOLE document stays under the report + # step's head -c 3900 render cap: truncating the finished document from + # the outside cuts the closing fence and malforms everything after it in + # the posted comment. Budget = 3300 minus the preamble, floored at 500. + local preamble tail_budget + preamble="**${label}**" + if [[ "${preexisting}" == 'true' ]]; then + # shellcheck disable=SC2016 + preamble+="$(printf '\n\nMeasured fact: the same check also fails at `origin/%s` (the branch as pushed, before this round) in this environment, with a matching failure signature. The repair pass may only amend the round'"'"'s own fix, so it cannot reach this failure. If the branch is behind `main`, a base update (merge main) is the usual cure; otherwise the failure lives in the branch'"'"'s own pre-round commits.' "${BRANCH}")" + fi + tail_budget=$(( 3300 - ${#preamble} )) + (( tail_budget < 500 )) && tail_budget=500 { - echo "**${1}**" + printf '%s\n' "${preamble}" echo # Captured output can contain triple-backtick fences. echo '````' - tail -c 3000 "${GATE_LOG}" 2> /dev/null + tail -c "${tail_budget}" "${GATE_LOG}" 2> /dev/null echo '````' } > "${WORKDIR}/gate-rejection.md" || echo "::warning::could not write the gate rejection detail; the verdict stands." exit 1 } +baseline_also_fails() { + # A deterministic rejection is only chargeable to this round if the same + # check passes WITHOUT the round's commits. Measured counterexample, run + # 31276008548: PR #8614's branch predated #8693's tsconfig guard while + # node_modules came from the post-#8693 trusted base, so `npm run build` + # was just as red at origin/ — 63 minutes of accepted agent work + # were discarded and an 18-minute repair burned on a failure the repair + # agent is forbidden to touch, thirteen rounds in a row. + # Returns 0 (pre-existing) only when the SAME command demonstrably fails + # at the pre-round ref; any A/B infrastructure problem returns 1 so the + # rejection keeps today's semantics (fail closed toward "charge the fix"). + local current baseline rc + current="$(git rev-parse HEAD)" || return 1 + baseline="$(git rev-parse --quiet --verify "origin/${BRANCH}^{commit}")" || + return 1 + # No round commit (the core-rebuild check runs before the commit gate and + # is A/B-eligible) — the baseline IS the tree under test; nothing to + # compare. + [[ "${baseline}" != "${current}" ]] || return 1 + echo "🔁 Baseline A/B: re-running the failed check at origin/${BRANCH}" \ + "(${baseline})" | tee -a "${GATE_LOG}" + git checkout --quiet --detach "${baseline}" 2>> "${GATE_LOG}" || return 1 + # The baseline transcript goes to a SIDE log: gate-rejection.md renders + # the dynamic `tail_budget` tail of GATE_LOG as the evidence window, and + # on a green baseline a chatty success transcript would fill it and push the actual + # failure text out — misdirecting the repair agent, the PR comment, and + # the next round's LAST_REJECTION block all at once. + local ab_log="${GATE_LOG}.baseline" + : > "${ab_log}" + rc=0 + if ! "$@" >> "${ab_log}" 2>&1; then + rc=1 + fi + if ! git checkout --quiet "${BRANCH}" 2>> "${GATE_LOG}"; then + # The tree is no longer the one under verification and nothing after + # this point may trust it. Not retryable either: the repair agent works + # in this very checkout and performs no git recovery, so on a detached + # tree its commit would land on the baseline and be orphaned. The round + # ends here; the next one starts clean from the trusted checkout. + reject_fix 'could not restore the verification tree after the baseline check' \ + false false + fi + if [[ "${rc}" -ne 1 ]]; then + echo "🔁 baseline is green — the failure belongs to this round" \ + | tee -a "${GATE_LOG}" + return 1 + fi + # A nonzero baseline is NOT enough: the branch can fail there for reason A + # while the round fails for reason B, and an infrastructure hiccup in the + # baseline leg is a nonzero exit too. Pre-existing requires the round's + # failing signatures to be a SUBSET of the baseline's — compiler + # diagnostics normalized to file + error code + message (line/column shift + # with the round's edits): a round that ADDS a diagnostic charges the + # failure to the round even when it also shares baseline diagnostics. The + # difference is captured before testing — piping `comm` into `grep -q` + # exits `grep` at the first match and SIGPIPEs `comm` under pipefail once + # the shared output outruns the pipe buffer, flipping identical large + # failure sets to NO-MATCH. No diagnostics on either side means identity + # cannot be established, and the rejection stays charged to the round + # (fail closed). + local sig_head sig_base new_in_round + # `|| true`: grep exits 1 on the NORMAL no-match case, and these + # assignments only survive `set -e` today because this function is called + # from an `if` condition (which suspends errexit). A future unconditional + # call site would otherwise turn the documented fail-closed path into a + # verdict-less gate crash. + sig_head="$(fail_signature "${GATE_LOG}.check")" || true + sig_base="$(fail_signature "${ab_log}")" || true + new_in_round="$(comm -23 <(printf '%s\n' "${sig_head}") <(printf '%s\n' "${sig_base}"))" || + return 1 + if [[ -z "${sig_head}" || -z "${sig_base}" ]] || [[ -n "${new_in_round}" ]]; then + echo "🔁 baseline fails for a DIFFERENT reason — charged to the round" \ + | tee -a "${GATE_LOG}" + return 1 + fi + # Only a FAILING baseline transcript with a matching signature is + # evidence — merge its tail into the window, where it backs the label. + tail -c 1500 "${ab_log}" >> "${GATE_LOG}" 2> /dev/null || true + return 0 +} +fail_signature() { + # Stable identity of a failed check: tsc-style diagnostics with the + # position stripped but the MESSAGE kept ("src/a.ts: error TS2504: …"). + # Position strips because line/column shift with the round's edits; the + # message stays because file + code alone collide — two unrelated defects + # in one file sharing a common code (TS2339 is everywhere) would compare + # as "the same failure" and skip a repair that could have worked. A + # message naming a round-renamed identifier then under-matches — the + # fail-closed direction. Sorted unique so two transcripts compare with + # comm(1). KNOWN LIMIT: only tsc diagnostics carry identity; vite/esbuild + # failures yield an empty signature and deliberately fail closed (charged + # to the round) — widening needs their position formats normalized first. + grep -oE "[^ '\"]+\([0-9]+,[0-9]+\): error TS[0-9]+.*" "${1}" 2> /dev/null \ + | sed -E 's/\([0-9]+,[0-9]+\)//' | sort -u +} run_check() { - # pipefail makes the pipeline carry the command's status, not tee's. + # pipefail makes the pipeline carry the command's status, not tee's. The + # side copy holds THIS check's transcript alone — the identity comparison + # must not match diagnostics an earlier check left in the shared log. + local label="${1}" + shift + : > "${GATE_LOG}.check" + if ! "$@" 2>&1 | tee -a "${GATE_LOG}" "${GATE_LOG}.check"; then + if baseline_also_fails "$@"; then + reject_fix "${label} (pre-existing: also fails without this round's commit)" 'true' + fi + reject_fix "${label}" + fi +} +run_check_no_ab() { + # A/B-exempt: for checks whose baseline re-run would compare a DIFFERENT + # computation than the one that failed, so a baseline verdict proves + # nothing. The contracts check consumes its file list from stdin, which + # the first run drains — the baseline leg would re-check an empty list + # and pass vacuously. The schema check reads packages/core/dist, which + # the core-rebuild guard built from the ROUND's sources and which, + # being gitignored, survives the detach and confounds the baseline. Their + # rejections stay charged to the round — which is also where the repair + # agent can actually act on them (generate:settings-schema is in its + # allowlist). local label="${1}" shift if ! "$@" 2>&1 | tee -a "${GATE_LOG}"; then @@ -112,10 +253,10 @@ fi # that predates the script does not contain it (bash would exit 127 # and kill the gate with no outcome), and the gate logic must come # from the trusted base, not the branch under verification. -run_check 'settings schema is stale on the agent-committed fix' \ +run_check_no_ab 'settings schema is stale on the agent-committed fix' \ bash "${RUNNER_TEMP}/check-settings-schema.sh" CHANGED_FILES="$(git diff --name-only "origin/main...${BRANCH}")" -run_check 'cross-package contract verification failed' \ +run_check_no_ab 'cross-package contract verification failed' \ bash "${RUNNER_TEMP}/check-autofix-contracts.sh" <<< "${CHANGED_FILES}" assert_verification_tree @@ -140,8 +281,14 @@ fi echo '🔬 Re-running deterministic checks (independent of the agent)...' run_check 'build failed on the agent-committed fix' npm run build -run_check 'typecheck failed on the agent-committed fix' npm run typecheck -run_check 'lint failed on the agent-committed fix' npm run lint +# Typecheck consumes core's dist (sdk-typescript resolves +# @qwen-code/qwen-code-core through the package exports to ./dist/*.d.ts), +# and dist is gitignored — it survives the baseline detach carrying the +# ROUND's build, so a baseline typecheck would run reverted sources against +# round-built declarations. Probe-verified three-arm flip on this tree. Same +# class as the schema check: A/B-exempt. +run_check_no_ab 'typecheck failed on the agent-committed fix' npm run typecheck +run_check_no_ab 'lint failed on the agent-committed fix' npm run lint # Test changed/related files for the packages this PR touches. # --changed follows the import graph so transitive breakage is caught. @@ -172,7 +319,14 @@ else continue fi echo "🧪 Testing ${p} (changed files only)..." - run_check "tests failed in ${p}" \ + # A/B-exempt: package tests resolve sibling workspaces through their + # dist exports (channels/github -> @qwen-code/channel-base/dist), and + # dist survives the baseline detach carrying the ROUND's build — a + # baseline leg would test reverted sources against round-built + # dependencies. (A round-ADDED workspace also has no baseline at all: + # npm exits 1 there with "No workspaces found".) Their rejections stay + # charged to the round, where the repair agent can act. + run_check_no_ab "tests failed in ${p}" \ npm run test --workspace "${p}" --if-present -- --changed origin/main --passWithNoTests done fi diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index e327a592a80..1dc6bf46291 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -825,6 +825,36 @@ jobs: fetch-depth: 0 persist-credentials: false + - name: 'Remove stale sandbox containers' + run: |- + # run-agent.mjs's budget kill removes the container it launched, + # but a JOB timeout still reaps only the HOST-side docker client, + # not the container: a killed sandbox can keep running on this + # persistent runner. Observed directly — a hung leg's container + # name counter found qwen-code-0.21.8-0 already occupied and + # picked -1. But the docker DAEMON is per host while this pool + # runs several runner registrations on one OS, so a RUNNING + # qwen-code-* container can belong to a job executing on another + # registration of this same host — reaping it would destroy a + # live sandbox mid-run. Reap only provably-dead containers + # (exited/dead), before the sandbox picks a name (and before the + # leftovers can wedge the daemon). Every docker call here is + # tolerated: this step is hygiene, and a daemon blip, a racing + # reap on another registration, or a container that refuses + # removal must not kill the round at setup. Every call also runs + # under `timeout` (GNU coreutils on the ubuntu runners): a daemon + # that is alive but wedged blocks `docker ps` indefinitely, and + # `|| STALE=''` catches only a nonzero exit, not a hang — the + # step would sit until the job timeout, a silent round + # reintroduced ahead of the very idle watchdog this PR adds. + command -v docker > /dev/null || exit 0 + STALE="$(timeout 30 docker ps -aq --filter 'name=qwen-code-' --filter 'status=exited' --filter 'status=dead' 2>/dev/null)" || STALE='' + if [ -n "${STALE}" ]; then + echo "removing stale sandbox containers:" + timeout 30 docker ps -a --filter 'name=qwen-code-' --filter 'status=exited' --filter 'status=dead' --format ' {{.Names}} ({{.Status}})' || true + printf '%s\n' "${STALE}" | xargs -r -I{} timeout 30 docker rm -f {} > /dev/null 2>&1 || true + fi + - name: 'Reset autofix workspace' run: |- rm -rf "${WORKDIR}" @@ -3497,6 +3527,36 @@ jobs: fetch-depth: 0 persist-credentials: false + - name: 'Remove stale sandbox containers' + run: |- + # run-agent.mjs's budget kill removes the container it launched, + # but a JOB timeout still reaps only the HOST-side docker client, + # not the container: a killed sandbox can keep running on this + # persistent runner. Observed directly — a hung leg's container + # name counter found qwen-code-0.21.8-0 already occupied and + # picked -1. But the docker DAEMON is per host while this pool + # runs several runner registrations on one OS, so a RUNNING + # qwen-code-* container can belong to a job executing on another + # registration of this same host — reaping it would destroy a + # live sandbox mid-run. Reap only provably-dead containers + # (exited/dead), before the sandbox picks a name (and before the + # leftovers can wedge the daemon). Every docker call here is + # tolerated: this step is hygiene, and a daemon blip, a racing + # reap on another registration, or a container that refuses + # removal must not kill the round at setup. Every call also runs + # under `timeout` (GNU coreutils on the ubuntu runners): a daemon + # that is alive but wedged blocks `docker ps` indefinitely, and + # `|| STALE=''` catches only a nonzero exit, not a hang — the + # step would sit until the job timeout, a silent round + # reintroduced ahead of the very idle watchdog this PR adds. + command -v docker > /dev/null || exit 0 + STALE="$(timeout 30 docker ps -aq --filter 'name=qwen-code-' --filter 'status=exited' --filter 'status=dead' 2>/dev/null)" || STALE='' + if [ -n "${STALE}" ]; then + echo "removing stale sandbox containers:" + timeout 30 docker ps -a --filter 'name=qwen-code-' --filter 'status=exited' --filter 'status=dead' --format ' {{.Names}} ({{.Status}})' || true + printf '%s\n' "${STALE}" | xargs -r -I{} timeout 30 docker rm -f {} > /dev/null 2>&1 || true + fi + - name: 'Reset autofix workspace' run: |- rm -rf "${WORKDIR}" @@ -4575,10 +4635,25 @@ jobs: REPAIR_OUTCOME: '${{ steps.verify_repair.outputs.outcome }}' REPAIR_COMMITTED: '${{ steps.verify_repair.outputs.committed }}' REPAIR_VERIFIED_HEAD: '${{ steps.verify_repair.outputs.verified_head }}' + FIRST_PREEXISTING: '${{ steps.verify.outputs.preexisting }}' + REPAIR_PREEXISTING: '${{ steps.verify_repair.outputs.preexisting }}' run: |- OUTCOME="${FIRST_OUTCOME}" COMMITTED="${FIRST_COMMITTED}" VERIFIED_HEAD="${FIRST_VERIFIED_HEAD}" + # The flag travels WITH the attempt whose outcome is selected: the + # first pass can fail a round-caused check, the repair fixes it, and + # the repair verification can then hit a pre-existing failure — that + # final classification is the one the report must render. Forwarded + # so the failure report can say "base update needed" instead of the + # generic gate-rejection clause. + PREEXISTING="${FIRST_PREEXISTING}" + if [[ "${REPAIR_ATTEMPTED}" == 'true' ]]; then + PREEXISTING="${REPAIR_PREEXISTING}" + fi + if [[ "${PREEXISTING}" == 'true' ]]; then + echo "preexisting=true" >> "${GITHUB_OUTPUT}" + fi if [[ "${REPAIR_ATTEMPTED}" == 'true' ]]; then OUTCOME="${REPAIR_OUTCOME}" COMMITTED="${REPAIR_COMMITTED:-${FIRST_COMMITTED}}" @@ -5035,6 +5110,7 @@ jobs: env: OUTCOME: '${{ steps.final_verify.outputs.outcome }}' COMMITTED: '${{ steps.final_verify.outputs.committed }}' + PREEXISTING: '${{ steps.final_verify.outputs.preexisting }}' CONFLICT: '${{ steps.prepare.outputs.conflict }}' DRY_RUN: '${{ needs.route.outputs.dry_run }}' GITHUB_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}' @@ -5218,9 +5294,16 @@ jobs: # A timeout evaluated nothing, so the feedback is unaddressed # and stays live for the retry. On a big / heavily-reviewed PR # this is usually a one-off; the last automatic attempt names - # the real fix (split the PR or raise the budget). + # the real fix (split the PR or raise the budget). An IDLE + # timeout is a different failure class: the sandbox went + # silent — more budget cannot cure it, so the terminal advice + # points at the sandbox, not the budget knobs. CAUSE="ran out of time before finishing (${AGENT_TIMEOUT})" - LAST_FIX="a human should split the PR or raise the agent time budget and its step backstop, then re-arm" + if [[ "${AGENT_TIMEOUT}" == 'idle-timeout'* ]]; then + LAST_FIX="a maintainer should check the sandbox image and runner docker daemon, then re-arm — raising the time budget cannot cure a silent sandbox" + else + LAST_FIX="a human should split the PR or raise the agent time budget and its step backstop, then re-arm" + fi elif [[ -z "${DETAIL_FILE}" ]]; then CAUSE="crashed before it could evaluate the feedback" LAST_FIX="a human should take over this PR" @@ -5306,6 +5389,27 @@ jobs: # wording-doesn't-match-behaviour bug this PR fixes. GATE_CLAUSE='' [[ -s "${WORKDIR}/gate-rejection.md" ]] && GATE_CLAUSE=' — the verification gate rejected the attempt' + # Pre-existing failures get the honest clause: the rejection + # is not the agent's and the repair was deliberately skipped. + # The remedy depends on WHY it pre-exists, and this branch + # only renders when the stale-base auto-update above did NOT + # fire — which includes a branch current with main whose own + # pre-round commits carry the failure, where "merge main" + # changes nothing. CMP_R is assigned only when BOTH gh api + # calls above succeed (each swallows failure into ''), so + # an EMPTY CMP_R means the compare never ran — "measured + # not-behind" and "never measured" get separate clauses: + # the latter cannot assert the branch's own code is at + # fault. + if [[ "${PREEXISTING}" == 'true' ]]; then + if [[ "${CMP_R:-}" == 'behind' || "${CMP_R:-}" == 'diverged' ]]; then + GATE_CLAUSE=' — the verification gate hit a PRE-EXISTING failure (present without this round'"'"'s commit); the repair pass may only amend the round'"'"'s own fix, so it cannot reach it — the branch needs a base update (merge main)' + elif [[ -z "${CMP_R:-}" ]]; then + GATE_CLAUSE=' — the verification gate hit a PRE-EXISTING failure (present without this round'"'"'s commit); the repair pass may only amend the round'"'"'s own fix, so it cannot reach it — the base state could not be compared (compare API failed), so check whether merging main resolves it before auditing the branch'"'"'s own pre-round code' + else + GATE_CLAUSE=' — the verification gate hit a PRE-EXISTING failure (present without this round'"'"'s commit); the repair pass may only amend the round'"'"'s own fix, so it cannot reach it — the branch'"'"'s own pre-round code needs attention' + fi + fi HEADLINE="🤖 Could not produce a passing fix for this feedback (round ${MARK_ROUND}/${MAX_ROUNDS})${GATE_CLAUSE}. This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own." fi fi @@ -5431,6 +5535,13 @@ jobs: if [[ -n "${AGENT_TIMEOUT:-}" ]]; then TIMEOUT_N=$(( TIMEOUT_N + 1 )) fi + # Idle (silent-sandbox) timeouts share the census — they burn + # the same full budget — but no budget increase cures them, so + # when the window contains any, the breaker says so. + IDLE_N="$(grep -c 'idle-timeout' <<< "${PRIOR_HEADS}" || true)" + if [[ "${AGENT_TIMEOUT:-}" == 'idle-timeout'* ]]; then + IDLE_N=$(( IDLE_N + 1 )) + fi if [[ "${TIMEOUT_N}" -ge "${TIMEOUT_WINDOW_CAP}" ]]; then MARK_ROUND="${MAX_ROUNDS}" # The headline states what the census MEASURED — the @@ -5439,7 +5550,18 @@ jobs: # failed differently (a gate rejection landing on a window # that already carries the cap — the exact rollout state # of #7929/#7846). - HEADLINE="🤖 AutoFix stopped: this counting window now contains ${TIMEOUT_N} time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is ${TIMEOUT_N} full agent runs that pushed nothing. A human should split or reduce the PR (or raise the agent time budget AND its step backstop together), then comment \`${RETRY_COMMAND}\` to re-arm. Until then future scans will skip this PR." + IDLE_CLAUSE='' + if [[ "${IDLE_N}" -gt 0 ]]; then + IDLE_CLAUSE=" ${IDLE_N} of those were silent-sandbox (idle) timeouts that no budget increase can cure — investigate the sandbox image and runner docker daemon for those." + fi + # Mirror the round-level split: when EVERY counted timeout + # was idle, the closing remedy must not prescribe the + # budget increase the clause above just declared useless. + REMEDY='split or reduce the PR (or raise the agent time budget AND its step backstop together)' + if [[ "${IDLE_N}" -ge "${TIMEOUT_N}" ]]; then + REMEDY='investigate the sandbox image and runner docker daemon' + fi + HEADLINE="🤖 AutoFix stopped: this counting window now contains ${TIMEOUT_N} time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is ${TIMEOUT_N} full agent runs that pushed nothing.${IDLE_CLAUSE} A human should ${REMEDY}, then comment \`${RETRY_COMMAND}\` to re-arm. Until then future scans will skip this PR." fi fi fi @@ -5481,9 +5603,13 @@ jobs: echo '' echo "**Why it was not pushed:**" echo - # Must stay >= reject_fix's tail -c 3000 + label + two four-backtick - # fences (~3.1 KB), or the closing fence is silently truncated. - head -c 3500 "${WORKDIR}/gate-rejection.md" | iconv -f utf-8 -t utf-8 -c | sed 's/' fi echo diff --git a/.qwen/skills/autofix/scripts/run-agent.mjs b/.qwen/skills/autofix/scripts/run-agent.mjs index 7884108f6b8..9bc251f5cff 100755 --- a/.qwen/skills/autofix/scripts/run-agent.mjs +++ b/.qwen/skills/autofix/scripts/run-agent.mjs @@ -19,6 +19,24 @@ const skillPath = resolve( 'SKILL.md', ); const QWEN_TIMEOUT_MS = Number(process.env.QWEN_TIMEOUT_MS) || 50 * 60 * 1000; +// Idle watchdog: a wedged sandbox produces NOTHING — four observed hangs +// (#8663 x2, #8761 r3, #8763 r4) each printed their last byte at docker +// container entry and then sat silent for the whole absolute budget, +// burning 2 hours per round for zero work. Legitimate runs are never that +// quiet: the longest silence the fleet tolerates elsewhere is the review +// pipeline's 10-minute stream-idle window for thinking phases on ~1M-token +// contexts, so twice that is the default. Distinct from QWEN_TIMEOUT_MS so +// the failure comment says which limit fired; a leg whose absolute budget is +// shorter than this window (the review workflow's 18-minute repair pass) +// always reaches the absolute timer first. +const parsedIdleTimeoutMs = Number(process.env.QWEN_IDLE_TIMEOUT_MS); +// Reject negative/0/NaN: Number('-1') is truthy, so a bare `|| default` +// guard would arm a sub-second window and kill every agent at the first +// idle tick. +const QWEN_IDLE_TIMEOUT_MS = + Number.isFinite(parsedIdleTimeoutMs) && parsedIdleTimeoutMs > 0 + ? parsedIdleTimeoutMs + : 20 * 60 * 1000; const specs = { 'assess-candidates': { inputs: ['candidates.json'], @@ -176,8 +194,17 @@ function runQwen(options, prompt) { let loopDetected = false; let settled = false; let timedOut = false; + let idleTimedOut = false; + let lastOutputAt = Date.now(); + // The sandbox launcher prints the container name before the container + // starts (packages/cli/src/utils/sandbox.ts), so the FIRST match is this + // run's own container — the kill-path reap below relies on that ownership. + let sandboxName = ''; + let lineCarry = ''; let timer; let killTimer; + let idleTimer; + let sandboxRemoval = null; return new Promise((resolve) => { const child = spawn(options.qwenBin, ['--yolo', '--prompt', prompt], { @@ -190,15 +217,18 @@ function runQwen(options, prompt) { settled = true; clearTimeout(timer); clearTimeout(killTimer); + clearInterval(idleTimer); const apiErrorInfo = recoverableApiError(outputTail); const payload = { ...result, timedOut, + idleTimedOut, loopDetected: loopDetected || isLoopGuardOutput(outputTail), // A RECOVERABLE model error means qwen never evaluated the feedback — // the workflow retries it rather than advancing the watermark. apiError: apiErrorInfo.error, apiErrorKind: apiErrorInfo.kind, + sandboxRemoval, }; if (log.destroyed) { resolve(payload); @@ -208,7 +238,24 @@ function runQwen(options, prompt) { }; const record = (chunk, stream) => { + lastOutputAt = Date.now(); const text = chunk.toString('utf8'); + if (!sandboxName) { + lineCarry += text; + const lastNewline = lineCarry.lastIndexOf('\n'); + if (lastNewline === -1) { + lineCarry = lineCarry.slice(-256); + } else { + const complete = lineCarry.slice(0, lastNewline + 1); + lineCarry = lineCarry.slice(lastNewline + 1).slice(-256); + const name = complete.match( + /^ContainerName(?: \(regular\))?: (\S+)$/m, + )?.[1]; + if (name && /^qwen-code-[A-Za-z0-9][A-Za-z0-9._-]*$/.test(name)) { + sandboxName = name; + } + } + } outputTail = (outputTail + text).slice(-20_000); if (!loopDetected && isLoopGuardOutput(outputTail)) loopDetected = true; log.write(chunk); @@ -222,13 +269,66 @@ function runQwen(options, prompt) { finish({ error: null, status, signal }), ); - timer = setTimeout(() => { - timedOut = true; + // Killing the host-side docker client leaves the sandbox container + // RUNNING on this persistent runner, and the startup reaper may not + // touch running containers (one can belong to a concurrent job on + // another registration of the same host). Remove it HERE, where + // ownership is unambiguous: the name was captured from this run's own + // launcher line. Best-effort — a daemon blip must not mask the kill. + const escalateKill = () => { killQwen(child, 'SIGTERM'); killTimer = setTimeout(() => { if (!settled) killQwen(child, 'SIGKILL'); }, 10_000); + if (sandboxName) { + // Async, not spawnSync: a synchronous removal blocks the event loop + // for up to the spawn timeout, holding up the SIGKILL backstop + // queued above and the child's close/finish/log-flush — in exactly + // the wedged-daemon scenario this kill path exists for. The main + // flow awaits sandboxRemoval, so the leak warning and the kill-path + // reap stay deterministic without blocking the backstop. + const rm = spawn('docker', ['rm', '-f', '--', sandboxName], { + stdio: 'ignore', + timeout: 30_000, + }); + sandboxRemoval = new Promise((resolveRemoval) => { + let warned = false; + const warnLeak = () => { + if (warned) return; + warned = true; + process.stderr.write( + `warning: leaked sandbox container ${sandboxName} could not be removed; it keeps running on this host\n`, + ); + }; + rm.on('error', warnLeak); + rm.on('close', (code) => { + if (code !== 0) warnLeak(); + resolveRemoval(); + }); + }); + } + }; + + timer = setTimeout(() => { + timedOut = true; + escalateKill(); }, QWEN_TIMEOUT_MS); + // Poll rather than reset-a-timeout-per-chunk: chunks arrive far more + // often than the watchdog needs to look, and a busy stream would then + // spend its time re-arming timers. The tick shrinks with the window so + // tiny test values still fire promptly. + const idleTick = Math.max( + 250, + Math.min(30_000, Math.floor(QWEN_IDLE_TIMEOUT_MS / 4)), + ); + idleTimer = setInterval(() => { + if (settled || timedOut || idleTimedOut) return; + if (Date.now() - lastOutputAt >= QWEN_IDLE_TIMEOUT_MS) { + idleTimedOut = true; + timedOut = true; + escalateKill(); + } + }, idleTick); }); } @@ -290,14 +390,20 @@ if (missingInputs.length > 0) { } const result = await runQwen(options, prompt); +// Await the kill-path container removal (bounded by its own 30s spawn +// timeout) so the leak warning and the removal itself settle before this +// process exits and the next step inspects the host. +if (result.sandboxRemoval) await result.sandboxRemoval; if (result.error || result.signal || result.status !== 0) { const detail = result.error ? result.error.message - : result.timedOut - ? `timeout (${QWEN_TIMEOUT_MS}ms)` - : result.signal - ? `signal ${result.signal}` - : `status ${String(result.status)}`; + : result.idleTimedOut + ? `idle-timeout (no output for ${QWEN_IDLE_TIMEOUT_MS}ms — the sandbox likely hung at startup)` + : result.timedOut + ? `timeout (${QWEN_TIMEOUT_MS}ms)` + : result.signal + ? `signal ${result.signal}` + : `status ${String(result.status)}`; if (!existsSync(file(options.workdir, 'failure.md'))) { if (result.loopDetected) { writeFailure( diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index c98ffbcfa73..8209f14e617 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -18,6 +18,8 @@ import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { describe, expect, it } from 'vitest'; +import { getWorkflowJob } from './workflow-helpers.js'; + const workflow = readFileSync('.github/workflows/qwen-autofix.yml', 'utf8'); const ciWorkflow = readFileSync('.github/workflows/ci.yml', 'utf8'); const releaseWorkflow = readFileSync('.github/workflows/release.yml', 'utf8'); @@ -8134,10 +8136,10 @@ exit 1 reviewVerificationRunner.match(/retryable=true/g) ?? [], ).toHaveLength(1); expect(reviewVerificationRunner).toContain( - "run_check 'settings schema is stale on the agent-committed fix'", + "run_check_no_ab 'settings schema is stale on the agent-committed fix'", ); expect(reviewVerificationRunner).toContain( - "run_check 'cross-package contract verification failed'", + "run_check_no_ab 'cross-package contract verification failed'", ); expect(pushAndReportStep).toContain( "steps.final_verify.outputs.outcome == 'fixed'", @@ -8158,10 +8160,10 @@ exit 1 'VERIFICATION_HEAD="$(git rev-parse HEAD)"', ); const schemaCheck = reviewVerificationRunner.indexOf( - "run_check 'settings schema is stale on the agent-committed fix'", + "run_check_no_ab 'settings schema is stale on the agent-committed fix'", ); const contractCheck = reviewVerificationRunner.indexOf( - "run_check 'cross-package contract verification failed'", + "run_check_no_ab 'cross-package contract verification failed'", ); const coreRebuild = reviewVerificationRunner.indexOf( "run_check 'core rebuild failed on the agent-committed fix'", @@ -8770,6 +8772,19 @@ exit 1 expect(timedOutCapped).toContain( 'split the PR or raise the agent time budget', ); + // An IDLE timeout at the cap gets the sandbox remedy, not budget + // advice: more minutes cannot cure a sandbox that produced nothing. + const idleCapped = run({ + OUTCOME: 'failed', + AGENT_TIMEOUT: + 'idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)', + ROUND: '4', + }); + expect(idleCapped).toContain('this was the last automatic attempt'); + expect(idleCapped).toContain( + 'raising the time budget cannot cure a silent sandbox', + ); + expect(idleCapped).not.toContain('split the PR or raise'); // At the cap the gate crash names the operator fix rather than promising a // retry the scan's round gate would refuse. @@ -9122,6 +9137,50 @@ exit 1 expect(interleaved.terminal).toBe(true); expect(interleaved.headline).toContain('time-budget exhaustions'); expect(interleaved.headline).toContain('/retry'); + // Idle (silent-sandbox) timeouts share the census — each burns a full + // budget — and when the window contains any, the breaker's advice says + // a budget increase cannot cure them. + const IDLE_HEAD = + '🤖 AutoFix ran out of time before finishing (idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)) (attempt 2/100) — it will retry on the next scan.'; + const idleMixed = run([IDLE_HEAD, PUSH, IDLE_HEAD, PUSH], { + agentTimeout: + 'idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)', + }); + expect(idleMixed.terminal).toBe(true); + expect(idleMixed.headline).toContain('time-budget exhaustions'); + expect(idleMixed.headline).toContain( + 'silent-sandbox (idle) timeouts that no budget increase can cure', + ); + // An ALL-idle window swaps the closing remedy for the sandbox + // investigation — mirroring the round-level split — instead of + // prescribing the budget increase the clause above declared useless. + expect(idleMixed.headline).toContain( + 'A human should investigate the sandbox image and runner docker daemon', + ); + expect(idleMixed.headline).not.toContain('raise the agent time budget'); + // A MIXED window (any real budget timeout) keeps the budget remedy. + const idleSome = run([TIMEOUT_HEAD, PUSH, IDLE_HEAD, PUSH], { + agentTimeout: + 'idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)', + }); + expect(idleSome.terminal).toBe(true); + expect(idleSome.headline).toContain('2 of those were silent-sandbox'); + expect(idleSome.headline).toContain('raise the agent time budget'); + // The CURRENT round's idle timeout is counted by the increment, not + // the grep: cap-1 budget priors plus an idle current round render + // "1 of those were silent-sandbox". Deleting the IDLE_N increment + // suppresses the clause entirely (the grep sees no idle prior) and + // must fail here. + const idleCurrentOnly = run(Array(timeoutCap - 1).fill(TIMEOUT_HEAD), { + agentTimeout: + 'idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)', + }); + expect(idleCurrentOnly.terminal).toBe(true); + expect(idleCurrentOnly.headline).toContain( + '1 of those were silent-sandbox (idle) timeouts', + ); + // A window WITHOUT idle rounds keeps today's advice untouched. + expect(interleaved.headline).not.toContain('silent-sandbox'); // One short of the cap keeps retrying (current round not a timeout). expect(run([TIMEOUT_HEAD, PUSH, TIMEOUT_HEAD])).toMatchObject({ terminal: false, @@ -9184,6 +9243,9 @@ exit 1 expect(reviewAddressReportStep).toContain( 'TIMEOUT_N="$(grep -c \'AutoFix ran out of time before finishing\' <<< "${PRIOR_HEADS}" || true)"', ); + expect(reviewAddressReportStep).toContain( + 'IDLE_N="$(grep -c \'idle-timeout\' <<< "${PRIOR_HEADS}" || true)"', + ); // 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. @@ -9265,12 +9327,12 @@ exit 1 // is captured for the retry. for (const check of [ "run_check 'core rebuild failed on the agent-committed fix'", - "run_check 'settings schema is stale on the agent-committed fix'", - "run_check 'cross-package contract verification failed'", + "run_check_no_ab 'settings schema is stale on the agent-committed fix'", + "run_check_no_ab 'cross-package contract verification failed'", "run_check 'build failed on the agent-committed fix' npm run build", - "run_check 'typecheck failed on the agent-committed fix' npm run typecheck", - "run_check 'lint failed on the agent-committed fix' npm run lint", - 'run_check "tests failed in ${p}"', + "run_check_no_ab 'typecheck failed on the agent-committed fix' npm run typecheck", + "run_check_no_ab 'lint failed on the agent-committed fix' npm run lint", + 'run_check_no_ab "tests failed in ${p}"', ]) { expect(gate).toContain(check); } @@ -11227,3 +11289,709 @@ exit 1 expect(skill).toContain('label event'); }); }); + +describe('review verification gate: baseline A/B on deterministic rejection', () => { + // The A/B re-runs a failed check at the pre-round ref and reports + // pre-existing ONLY when the baseline fails with a MATCHING failure + // signature (tsc diagnostics normalized to file + code): a bare nonzero + // baseline can be a different defect or an infrastructure hiccup, and + // gitignored dist survives the detach — which is why only the builds + // (root `npm run build` and the pre-commit core rebuild, which remake + // dist from checked-out sources) are A/B-eligible; typecheck, lint, and + // package tests consume + // round-built dist and are exempt. These tests execute the REAL script in + // a real git repo, config-isolated (a global core.hooksPath or + // pre-commit hook must not reach the fixture), with a stubbed npm whose + // failures and diagnostics are keyed by commit SHA. + const GIT_ISOLATION = { + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_CONFIG_SYSTEM: '/dev/null', + GIT_CONFIG_NOSYSTEM: '1', + }; + const runGate = ({ + failAt = [], + agentCommit = true, + schemaFail = false, + typecheckFail = false, + addWorkspace = false, + noisySuccess = false, + touchCore = false, + baselineCode = '', + baselineMsg = '', + headMsg = '', + extraRoundDiag = false, + extraBaselineDiag = false, + restoreClash = false, + hugeFail = false, + }) => { + const dir = mkdtempSync(join(tmpdir(), 'gate-ab-')); + try { + const sh = (cmd, cwd) => + execFileSync('bash', ['-c', cmd], { + cwd, + encoding: 'utf8', + env: { ...process.env, ...GIT_ISOLATION }, + }); + const origin = join(dir, 'origin.git'); + const work = join(dir, 'work'); + sh(`git init -q --bare '${origin}'`, dir); + sh(`git clone -q '${origin}' '${work}'`, dir); + const g = (cmd) => sh(cmd, work); + g('git config user.email t@t && git config user.name t'); + g('echo base > f.txt && git add . && git commit -qm base'); + g('git branch -M main && git push -q origin main'); + g('git checkout -qb feature'); + if (touchCore) { + // Reaches the core-rebuild run_check BEFORE the commit gate, so a + // failure there exercises the no-round-commit guard. + g('mkdir -p packages/core/src && echo x > packages/core/src/x.ts'); + g('git add . && git commit -qm core'); + } else { + g('echo branch > f.txt && git commit -qam branch'); + } + g('git push -q origin feature'); + if (agentCommit) { + if (addWorkspace) { + // The round ADDS a workspace — it does not exist at the baseline. + g('mkdir -p packages/newpkg'); + g( + `printf '{"name":"newpkg","scripts":{"test":"vitest run"}}' > packages/newpkg/package.json`, + ); + g('git add . && git commit -qm agent'); + } else if (restoreClash) { + // A file the branch TRACKS but the baseline lacks: the baseline + // leg recreates it untracked, and the restore checkout refuses. + g('echo tracked > clash.txt && git add . && git commit -qm agent'); + } else { + g('echo agent > f.txt && git commit -qam agent'); + } + } + const shaOf = (ref) => g(`git rev-parse ${ref}`).trim(); + const failShas = failAt.map(shaOf).join(' '); + const baselineSha = shaOf('origin/feature'); + + // Stub npm. A failing `run build` prints a marker AND a tsc-style + // diagnostic — the identity the A/B compares. BASELINE_CODE switches + // the diagnostic code on the baseline SHA so a different-cause + // baseline can be staged. `run test --workspace` mirrors measured npm: + // exit 1 "No workspaces found" for a missing workspace. NOISY_SUCCESS + // makes a PASSING build print >3 KB — the evidence-window flood shape. + const bin = join(dir, 'bin'); + mkdirSync(bin); + writeFileSync( + join(bin, 'npm'), + [ + '#!/bin/bash', + 'if [[ "$1" == "run" && "$2" == "build" ]]; then', + ' head="$(git rev-parse HEAD)"', + ' for s in ${FAIL_BUILD_SHAS}; do', + ' if [[ "$s" == "$head" ]]; then', + ' code=9999', + ' pos="(1,1)"; msg="stub failure"', + ' if [[ "$head" == "${BASELINE_SHA:-}" ]]; then', + // The baseline leg emits a SHIFTED position — the strip is what + // makes the two legs comparable, so the fixture must exercise it. + ' pos="(7,3)"', + ' if [[ -n "${BASELINE_CODE:-}" ]]; then code="${BASELINE_CODE}"; fi', + ' if [[ -n "${BASELINE_MSG:-}" ]]; then msg="${BASELINE_MSG}"; fi', + ' if [[ "${RESTORE_CLASH:-}" == "1" ]]; then echo untracked > clash.txt; fi', + ' fi', + ' if [[ "$head" != "${BASELINE_SHA:-}" ]]; then', + ' if [[ -n "${HEAD_MSG:-}" ]]; then msg="${HEAD_MSG}"; fi', + ' fi', + ' echo "stub build FAILED at $head"', + ' echo "src/f.ts${pos}: error TS${code}: ${msg}"', + ' if [[ "${HUGE_FAIL:-}" == "1" ]]; then', + ' for i in $(seq 1 200); do echo "verbose failure context line $i ****************************************"; done', + ' echo "root cause marker line"', + ' fi', + ' if [[ "$head" != "${BASELINE_SHA:-}" && "${EXTRA_ROUND_DIAG:-}" == "1" ]]; then', + ' echo "src/g.ts(2,2): error TS7777: round-introduced defect"', + ' fi', + ' if [[ "$head" == "${BASELINE_SHA:-}" && "${EXTRA_BASELINE_DIAG:-}" == "1" ]]; then', + ' echo "src/g.ts(7,3): error TS8888: baseline-only defect"', + ' fi', + ' exit 1', + ' fi', + ' done', + ' if [[ "${NOISY_SUCCESS:-}" == "1" ]]; then', + ' for i in $(seq 1 200); do echo "baseline build banner line $i — all green, nothing to see"; done', + ' fi', + 'fi', + 'if [[ "$1" == "run" && "$2" == "typecheck" && "${TYPECHECK_FAIL:-}" == "1" ]]; then', + ' echo "stub typecheck FAILED"; exit 1', + 'fi', + 'if [[ "$1" == "run" && "$2" == "test" && "$3" == "--workspace" ]]; then', + ' if [[ ! -d "$4" ]]; then echo "npm error No workspaces found: --workspace=$4"; exit 1; fi', + ' if [[ "${WORKSPACE_TEST_FAIL:-}" == "1" ]]; then echo "stub workspace tests FAILED in $4"; exit 1; fi', + 'fi', + 'exit 0', + ].join('\n'), + ); + chmodSync(join(bin, 'npm'), 0o755); + const rt = join(dir, 'rt'); + mkdirSync(rt); + writeFileSync( + join(rt, 'check-settings-schema.sh'), + 'if [[ "${SCHEMA_FAIL:-}" == "1" ]]; then echo "schema stale"; exit 1; fi\nexit 0\n', + ); + writeFileSync( + join(rt, 'check-autofix-contracts.sh'), + 'cat > /dev/null\nexit 0\n', + ); + writeFileSync( + join(rt, 'resolve-owning-packages.sh'), + 'cat > /dev/null\nprintf "%s" "${RESOLVED_PKGS:-}"\n', + ); + const workdir = join(dir, 'wd'); + mkdirSync(workdir); + writeFileSync(join(workdir, 'address-summary.md'), 'summary\n'); + const outFile = join(dir, 'gh-output'); + writeFileSync(outFile, ''); + + const res = spawnSync( + 'bash', + [resolve('.github/scripts/run-autofix-review-verification.sh')], + { + cwd: work, + encoding: 'utf8', + env: { + ...process.env, + ...GIT_ISOLATION, + PATH: `${bin}:${process.env.PATH}`, + BRANCH: 'feature', + WORKDIR: workdir, + RUNNER_TEMP: rt, + GITHUB_OUTPUT: outFile, + FAIL_BUILD_SHAS: failShas, + BASELINE_SHA: baselineSha, + BASELINE_CODE: baselineCode, + BASELINE_MSG: baselineMsg, + HEAD_MSG: headMsg, + EXTRA_ROUND_DIAG: extraRoundDiag ? '1' : '', + EXTRA_BASELINE_DIAG: extraBaselineDiag ? '1' : '', + RESTORE_CLASH: restoreClash ? '1' : '', + SCHEMA_FAIL: schemaFail ? '1' : '', + TYPECHECK_FAIL: typecheckFail ? '1' : '', + NOISY_SUCCESS: noisySuccess ? '1' : '', + HUGE_FAIL: hugeFail ? '1' : '', + WORKSPACE_TEST_FAIL: addWorkspace ? '1' : '', + RESOLVED_PKGS: addWorkspace ? 'packages/newpkg' : '', + }, + }, + ); + return { + status: res.status, + stdout: `${res.stdout}\n${res.stderr}`, + outputs: readFileSync(outFile, 'utf8'), + rejection: existsSync(join(workdir, 'gate-rejection.md')) + ? readFileSync(join(workdir, 'gate-rejection.md'), 'utf8') + : '', + headAfter: sh('git rev-parse --abbrev-ref HEAD', work).trim(), + baselineSha, + }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + + it('charges a failure to the round when the baseline is green', () => { + const r = runGate({ failAt: ['feature'] }); + expect(r.status).toBe(1); + expect(r.outputs).toContain('outcome=failed'); + expect(r.outputs).toContain('retryable=true'); + expect(r.outputs).not.toContain('preexisting=true'); + // The A/B genuinely ran — the verdict is measured, not assumed. + expect(r.stdout).toContain('Baseline A/B'); + expect(r.rejection).not.toContain('pre-existing'); + // The tree is back on the branch for anything that reads it afterwards. + expect(r.headAfter).toBe('feature'); + }); + + it('reports pre-existing only on a matching failure signature, with the baseline transcript as evidence', () => { + const r = runGate({ failAt: ['feature', 'origin/feature'] }); + expect(r.status).toBe(1); + expect(r.outputs).toContain('outcome=failed'); + expect(r.outputs).toContain('preexisting=true'); + // retryable stays unset: the repair step keys on it and must not run. + expect(r.outputs).not.toContain('retryable=true'); + expect(r.rejection).toContain('pre-existing'); + expect(r.rejection).toContain('base update (merge main)'); + // The baseline leg's own transcript is the ONLY proof behind the + // verdict — it must reach the rejection document. + expect(r.rejection).toContain(`stub build FAILED at ${r.baselineSha}`); + expect(r.headAfter).toBe('feature'); + }); + + it('charges the round when the codes match but the messages differ', () => { + // Identity is file + code + MESSAGE: common codes (TS2339) collide + // across unrelated defects in one file, and a code-only signature would + // skip a repair that could have produced a green fix. + const r = runGate({ + failAt: ['feature', 'origin/feature'], + baselineMsg: 'an entirely different defect', + }); + expect(r.status).toBe(1); + expect(r.outputs).toContain('retryable=true'); + expect(r.outputs).not.toContain('preexisting=true'); + expect(r.stdout).toContain('DIFFERENT reason'); + }); + + it('rejects WITHOUT retry when the baseline leg breaks the restore', () => { + // The baseline run recreates (untracked) a file the branch tracks, so + // `git checkout` back refuses — the tree can no longer be trusted. No + // pre-existing label (a transient git failure is not a verdict about + // the failure's origin) and no retry either: the repair agent works in + // this very checkout and performs no git recovery, so on the detached + // tree its commit would land on the baseline and be orphaned. The next + // round starts clean from the trusted checkout instead. + const r = runGate({ + failAt: ['feature', 'origin/feature'], + restoreClash: true, + }); + expect(r.status).toBe(1); + expect(r.outputs).toContain('outcome=failed'); + expect(r.outputs).not.toContain('retryable=true'); + expect(r.outputs).not.toContain('preexisting=true'); + expect(r.stdout).toContain('could not restore the verification tree'); + }); + + it('keeps the full message past the first n (the bracket class ate it)', () => { + // In an ERE bracket expression `\` is literal, so the earlier + // `[^\n]*` meant "neither backslash nor the letter n" and truncated + // every message at its first 'n' — collapsing "Cannot find module + // './foo'" and "'./bar'" into one signature and skipping the only + // repair that could fix the round-caused one. `.*` keeps the message. + const r = runGate({ + failAt: ['feature', 'origin/feature'], + headMsg: "Cannot find module './foo'", + baselineMsg: "Cannot find module './bar'", + }); + expect(r.status).toBe(1); + expect(r.outputs).toContain('retryable=true'); + expect(r.outputs).not.toContain('preexisting=true'); + }); + + it('charges the round when it ADDS a diagnostic to a failing baseline', () => { + // Pre-existing means the round's failing set is a SUBSET of the + // baseline's: sharing one signature with the baseline while adding + // another is a round-caused failure the repair can still fix — an + // intersection test labeled it pre-existing and skipped the repair. + const r = runGate({ + failAt: ['feature', 'origin/feature'], + extraRoundDiag: true, + }); + expect(r.status).toBe(1); + expect(r.outputs).toContain('retryable=true'); + expect(r.outputs).not.toContain('preexisting=true'); + }); + + it('reports pre-existing when the baseline fails with a SUPERSET of the round signatures', () => { + // The other arm of the subset semantics: every failing signature of + // the round also fails at the baseline, which ADDITIONALLY carries a + // baseline-only diagnostic. Still pre-existing — the repair may only + // amend the round's fix, so the extra baseline diagnostic is equally + // beyond its reach. A set-equality comparator instead of the subset + // check would flip this round to retryable. + const r = runGate({ + failAt: ['feature', 'origin/feature'], + extraBaselineDiag: true, + }); + expect(r.status).toBe(1); + expect(r.outputs).toContain('preexisting=true'); + expect(r.outputs).not.toContain('retryable=true'); + expect(r.rejection).toContain('pre-existing'); + expect(r.headAfter).toBe('feature'); + }); + + it('charges the round when the baseline fails for a DIFFERENT reason', () => { + // A nonzero baseline is not identity: reason A there, reason B here — + // reducing both to rc=1 would skip the only repair allowed to fix B. + const r = runGate({ + failAt: ['feature', 'origin/feature'], + baselineCode: '8888', + }); + expect(r.status).toBe(1); + expect(r.outputs).toContain('retryable=true'); + expect(r.outputs).not.toContain('preexisting=true'); + expect(r.stdout).toContain('DIFFERENT reason'); + }); + + it('keeps the green path intact', () => { + const r = runGate({ failAt: [] }); + expect(r.status).toBe(0); + expect(r.outputs).toContain('outcome=fixed'); + expect(r.outputs).not.toContain('preexisting'); + }); + + it('keeps the failure text in the evidence window past a chatty green baseline', () => { + const r = runGate({ failAt: ['feature'], noisySuccess: true }); + expect(r.status).toBe(1); + expect(r.outputs).toContain('retryable=true'); + expect(r.rejection).toContain('stub build FAILED'); + }); + + it('keeps the closing fence when the failure saturates the evidence window', () => { + // The report renders head -c 3900 of the FINISHED document; reject_fix + // sizes its own tail against the preamble so the closing fence survives + // even when the captured failure dwarfs the window. A future preamble + // or constant change that breaks the invariant fails here, not in a + // posted comment. + const r = runGate({ failAt: ['feature'], hugeFail: true }); + expect(r.status).toBe(1); + expect(r.rejection.length).toBeLessThanOrEqual(3900); + expect(r.rejection.endsWith('````\n')).toBe(true); + expect(r.rejection).toContain('root cause marker line'); + }); + + it('never A/Bs a check with no round commit to remove', () => { + const r = runGate({ + agentCommit: false, + touchCore: true, + failAt: ['feature'], + }); + expect(r.status).toBe(1); + expect(r.outputs).toContain('retryable=true'); + expect(r.outputs).not.toContain('preexisting=true'); + expect(r.stdout).not.toContain('Baseline A/B'); + }); + + it('never A/Bs the dist-coupled and stdin-fed checks', () => { + // schema (round-built core dist), contracts (drained stdin), and now + // typecheck (sdk-typescript resolves core d.ts from dist) are exempt. + for (const opts of [{ schemaFail: true }, { typecheckFail: true }]) { + const r = runGate(opts); + expect(r.status).toBe(1); + expect(r.outputs).toContain('retryable=true'); + expect(r.outputs).not.toContain('preexisting=true'); + expect(r.stdout).not.toContain('Baseline A/B'); + } + }); + + it('never A/Bs package tests (dist-resolving dependencies)', () => { + const r = runGate({ addWorkspace: true }); + expect(r.status).toBe(1); + expect(r.rejection).toContain('tests failed in packages/newpkg'); + expect(r.outputs).toContain('retryable=true'); + expect(r.outputs).not.toContain('preexisting=true'); + expect(r.stdout).not.toContain('Baseline A/B'); + }); +}); + +describe('review verification gate: preexisting output is consumed', () => { + it('Finalize verification selects the flag from the same attempt as the outcome', () => { + expect(workflow).toContain( + "FIRST_PREEXISTING: '${{ steps.verify.outputs.preexisting }}'", + ); + expect(workflow).toContain( + "REPAIR_PREEXISTING: '${{ steps.verify_repair.outputs.preexisting }}'", + ); + expect(workflow).toMatch( + /PREEXISTING="\$\{FIRST_PREEXISTING\}"\n\s*if \[\[ "\$\{REPAIR_ATTEMPTED\}" == 'true' \]\]; then\n\s*PREEXISTING="\$\{REPAIR_PREEXISTING\}"/, + ); + }); + + it('the failure report reads it and picks the clause by the compare', () => { + expect(workflow).toContain( + "PREEXISTING: '${{ steps.final_verify.outputs.preexisting }}'", + ); + // behind/diverged → base update; a MEASURED not-behind → the branch's + // own pre-round code (an up-to-date branch cannot be cured by merging + // main); an EMPTY CMP_R means the compare never ran (a transient API + // failure) and must not assert either — it says the base state could + // not be compared. + expect(workflow).toContain('needs a base update (merge main)'); + expect(workflow).toContain('the base state could not be compared'); + // The YAML embeds the apostrophe via shell quoting, so match around it. + expect(workflow).toContain('own pre-round code needs attention'); + expect(workflow).toMatch( + /if \[\[ "\$\{CMP_R:-\}" == 'behind' \|\| "\$\{CMP_R:-\}" == 'diverged' \]\]; then/, + ); + expect(workflow).toMatch(/elif \[\[ -z "\$\{CMP_R:-\}" \]\]; then/); + // Correspondence, not just existence: swapping the clause bodies must + // fail. Each {0,600} bound keeps the match inside this if/elif/else — + // a swapped clause puts its anchor on the wrong side of the arm it + // belongs to, more than 600 chars away. + expect(workflow).toMatch( + /if \[\[ "\$\{CMP_R:-\}" == 'behind' \|\| "\$\{CMP_R:-\}" == 'diverged' \]\]; then[\s\S]{0,600}?needs a base update \(merge main\)[\s\S]{0,600}?elif \[\[ -z "\$\{CMP_R:-\}" \]\]; then[\s\S]{0,600}?could not be compared[\s\S]{0,600}?else[\s\S]{0,600}?own pre-round code needs attention/, + ); + }); + + it('the evidence window flexes so the document clears the render cap', () => { + // The report renders head -c 3900 of the finished document; the script + // sizes the tail against its preamble so the closing fence survives. + expect(workflow).toContain('head -c 3900 "${WORKDIR}/gate-rejection.md"'); + expect(reviewVerificationRunner).toContain( + 'tail_budget=$(( 3300 - ${#preamble} ))', + ); + expect(reviewVerificationRunner).toContain( + 'tail -c "${tail_budget}" "${GATE_LOG}"', + ); + }); +}); + +describe('run-agent idle watchdog', () => { + // Four observed sandbox hangs (#8663 x2, #8761 r3, #8763 r4) printed their + // last byte at docker container entry and then sat SILENT for the whole + // 2-hour absolute budget — four different runners, two image versions, so + // the watchdog lives in the runner script, not the environment. A wedged + // sandbox produces nothing; a legitimate run is never silent for 20 + // minutes (the fleet's longest tolerated quiet is the review pipeline's + // 10-minute stream-idle window). These tests execute the REAL script with + // a stub agent whose only difference is whether it keeps talking. + const runAgent = ({ stub, idleMs, timeoutMs = 60_000 }) => { + const dir = mkdtempSync(join(tmpdir(), 'agent-idle-')); + try { + const workdir = join(dir, 'wd'); + mkdirSync(workdir); + writeFileSync(join(workdir, 'feedback.md'), 'feedback\n'); + const bin = join(dir, 'qwen'); + writeFileSync(bin, stub); + chmodSync(bin, 0o755); + const res = spawnSync( + process.execPath, + [ + resolve('.qwen/skills/autofix/scripts/run-agent.mjs'), + '--mode', + 'address-review', + '--pr', + '1', + '--issue', + '1', + '--qwen-bin', + bin, + '--workdir', + workdir, + ], + { + encoding: 'utf8', + timeout: 30_000, + env: { + ...process.env, + AGENT_WORKDIR: workdir, + QWEN_IDLE_TIMEOUT_MS: String(idleMs), + QWEN_TIMEOUT_MS: String(timeoutMs), + }, + }, + ); + return { + status: res.status, + failure: existsSync(join(workdir, 'failure.md')) + ? readFileSync(join(workdir, 'failure.md'), 'utf8') + : '', + timeoutSentinel: existsSync(join(workdir, 'agent-timeout')) + ? readFileSync(join(workdir, 'agent-timeout'), 'utf8') + : null, + }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + + it('kills a silent agent at the idle window, naming the idle limit', () => { + // The hang shape: one line at startup, then nothing, ever. + const r = runAgent({ + stub: '#!/bin/bash\necho "entering sandbox"\nsleep 600\n', + idleMs: 1200, + }); + expect(r.status).not.toBe(0); + expect(r.failure).toContain('idle-timeout (no output for 1200ms'); + // NOT the absolute-budget wording: the comment must say which limit + // fired, or the operator tunes the wrong knob. + expect(r.failure).not.toContain('timeout (60000ms)'); + // The sentinel routes the round to RETRY; deleting `timedOut = true` + // from the idle branch must fail here (the absolute-timeout test pins + // the same sentinel for its own path). + expect(r.timeoutSentinel).toContain('idle-timeout (no output for 1200ms'); + }); + + it('never fires while the agent keeps talking, however slowly', () => { + // Output every 400ms with a 1500ms idle window: an absolute-timer + // regression disguised as an idle watchdog would kill this run. + const r = runAgent({ + stub: [ + '#!/bin/bash', + 'for i in $(seq 1 8); do echo "tick $i"; sleep 0.4; done', + // The mode's output contract: a real run ends by writing its + // summary, and the script fails a run that produced neither output. + 'echo summary > "${AGENT_WORKDIR}/address-summary.md"', + 'echo done', + 'exit 0', + ].join('\n'), + idleMs: 1500, + }); + expect(r.status).toBe(0); + expect(r.failure).toBe(''); + }); + + it('never fires while the agent talks on stderr only', () => { + // The sandbox launcher emits ContainerName on stderr, and a cold + // runner's image pull or docker progress is a realistic stderr-only + // span with quiet stdout — the liveness contract covers both streams. + const r = runAgent({ + stub: [ + '#!/bin/bash', + 'for i in $(seq 1 8); do echo "tick $i" >&2; sleep 0.4; done', + 'echo summary > "${AGENT_WORKDIR}/address-summary.md"', + 'echo done', + 'exit 0', + ].join('\n'), + idleMs: 1500, + }); + expect(r.status).toBe(0); + expect(r.failure).toBe(''); + }); + + it('ignores a non-positive or non-numeric QWEN_IDLE_TIMEOUT_MS instead of arming it', () => { + // Number('-1') is truthy, so a bare `|| default` guard would arm a + // negative window: Date.now() - lastOutputAt >= -1 is instantly true + // and every agent dies at the first idle tick. `0` (an operator's + // "disable") arms a zero-length window that is true at the first tick, + // and NaN arms one too — every rejection class named in the parse + // guard's comment must fall back to the default. + for (const idleMs of [-1, 0, Number.NaN]) { + const r = runAgent({ + stub: [ + '#!/bin/bash', + 'for i in $(seq 1 8); do echo "tick $i"; sleep 0.4; done', + 'echo summary > "${AGENT_WORKDIR}/address-summary.md"', + 'echo done', + 'exit 0', + ].join('\n'), + idleMs, + }); + expect(r.status).toBe(0); + expect(r.failure).toBe(''); + } + }); +}); + +describe('stale sandbox container cleanup', () => { + // Two layers. The kill path: run-agent.mjs captures the container name + // its child's launcher printed and force-removes exactly that container + // when a budget/idle kill fires — ownership is unambiguous there, which + // is why the startup reap below may not touch running containers. The + // startup reap: a JOB timeout still reaps only the host-side docker + // client, so both sandboxed jobs reap before the sandbox picks a name + // (observed: a later leg's name counter found qwen-code-0.21.8-0 + // occupied) — but the docker daemon is per HOST and this pool runs + // several registrations on one OS, so a RUNNING container can belong to + // a concurrent job on another registration: the reap is restricted to + // provably-dead states. + it('both agent jobs remove stale qwen-code containers at start', () => { + const step = "- name: 'Remove stale sandbox containers'"; + expect(workflow.split(step).length - 1).toBe(2); + for (const jobId of ['issue-autofix', 'review-address']) { + const j = getWorkflowJob(workflow, jobId); + expect(j, jobId).toContain(step); + expect(j, jobId).toContain( + "timeout 30 docker ps -aq --filter 'name=qwen-code-' --filter 'status=exited' --filter 'status=dead'", + ); + // Best-effort hygiene under bash -eo pipefail: a daemon blip, a + // racing reap on another registration, or a container that refuses + // removal must not kill the round at setup — and an alive-but-wedged + // daemon must not block the step until the job timeout, so every + // docker call runs under `timeout`. + expect(j, jobId).toContain( + "STALE=\"$(timeout 30 docker ps -aq --filter 'name=qwen-code-' --filter 'status=exited' --filter 'status=dead' 2>/dev/null)\" || STALE=''", + ); + expect(j, jobId).toContain( + 'xargs -r -I{} timeout 30 docker rm -f {} > /dev/null 2>&1 || true', + ); + // Before the sandbox can pick a colliding name. + expect(j.indexOf(step)).toBeLessThan( + j.indexOf("- name: 'Reset autofix workspace'"), + ); + } + }); + + // The kill path is shared by the idle watchdog and the absolute budget + // timer (both fire escalateKill). Pin BOTH branches: a future edit that + // keeps the container removal on only one of them leaks the other's + // sandbox while a single-branch test still passes. + const runKillPath = (idleTimeoutMs, timeoutMs) => { + const dir = mkdtempSync(join(tmpdir(), 'agent-orphan-')); + try { + const workdir = join(dir, 'wd'); + mkdirSync(workdir); + writeFileSync(join(workdir, 'feedback.md'), 'feedback\n'); + const bin = join(dir, 'bin'); + mkdirSync(bin); + writeFileSync( + join(bin, 'docker'), + '#!/bin/bash\necho "$@" >> "${AGENT_WORKDIR}/docker-calls.txt"\nexit 0\n', + ); + chmodSync(join(bin, 'docker'), 0o755); + // The launcher line exactly as packages/cli/src/utils/sandbox.ts + // prints it, then the wedge shape: one line, then silence. + const stub = join(dir, 'qwen'); + writeFileSync( + stub, + '#!/bin/bash\necho "ContainerName (regular): qwen-code-9.9.9-9" >&2\nsleep 600\n', + ); + chmodSync(stub, 0o755); + const res = spawnSync( + process.execPath, + [ + resolve(autofixRunnerScriptPath), + '--mode', + 'address-review', + '--pr', + '1', + '--issue', + '1', + '--qwen-bin', + stub, + '--workdir', + workdir, + ], + { + encoding: 'utf8', + timeout: 30_000, + env: { + ...process.env, + AGENT_WORKDIR: workdir, + PATH: `${bin}:${process.env.PATH}`, + QWEN_IDLE_TIMEOUT_MS: idleTimeoutMs, + QWEN_TIMEOUT_MS: timeoutMs, + }, + }, + ); + return { + status: res.status, + failure: existsSync(join(workdir, 'failure.md')) + ? readFileSync(join(workdir, 'failure.md'), 'utf8') + : '', + calls: existsSync(join(workdir, 'docker-calls.txt')) + ? readFileSync(join(workdir, 'docker-calls.txt'), 'utf8').trim() + : '', + }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + + it('an idle kill removes only the running sandbox its own agent launched', () => { + // The startup reaper stays restricted to exited/dead containers (a + // running one can belong to a concurrent job on another registration), + // so the running orphan a kill creates is removed by the kill path + // itself: run-agent.mjs captures the container name its child's + // launcher printed and force-removes exactly that one. + const r = runKillPath('1200', '60000'); + expect(r.status).not.toBe(0); + expect(r.failure).toContain('idle-timeout (no output for 1200ms'); + // The ONLY docker call is the owned container's removal. + expect(r.calls.split('\n')).toEqual(['rm -f -- qwen-code-9.9.9-9']); + }); + + it('a budget kill removes only the running sandbox its own agent launched', () => { + // The idle window sits far above the absolute budget, so the budget + // timer is the branch that fires here (the idle variant above pins the + // shared kill path from the other side). + const r = runKillPath('600000', '1200'); + expect(r.status).not.toBe(0); + expect(r.failure).toContain('timeout (1200ms)'); + expect(r.failure).not.toContain('idle-timeout'); + expect(r.calls.split('\n')).toEqual(['rm -f -- qwen-code-9.9.9-9']); + }); +});