fix(ci): watchdog silent sandbox hangs and reap the containers they leak - #8816
Conversation
A deterministic rejection in the autofix verification gate is only chargeable to the round if the same check passes without the round's commit. The gate charged every red to the fix unconditionally, and run 31276008548 measured what that costs when the premise is false: PR 8614's branch predated #8693's tsconfig guard while node_modules came from the post-#8693 trusted base, so `npm run build` was equally red at origin/<branch> — 63 minutes of accepted agent work discarded, an 18-minute repair burned on a failure the repair agent is forbidden to touch (it may only amend the round's own fix), thirteen rounds in a row, and the same again on the #8616 leg. On rejection the gate now re-runs the failing check at origin/<branch> (the branch as pushed, before the round) in the same environment: - baseline green: today's path exactly — outcome=failed, retryable=true, the repair pass gets its chance. - baseline red too: outcome=failed with preexisting=true and NO retryable. The repair step keys on retryable and is skipped — it cannot reach a failure outside the round's diff by construction — and gate-rejection.md says outright that the branch needs a base update (merge main), which flows into the failure comment as-is. Fail-closed toward today's semantics: any A/B infrastructure problem (missing ref, checkout failure) charges the fix as before, and a restore failure after the baseline run rejects outright since the tree can no longer be trusted. The round's work is still not pushed — this changes the verdict's honesty and cost, not the push policy. Tested by executing the real script in a real two-remote git repo with an npm stub whose failures are keyed by commit SHA: round-caused red (baseline green), pre-existing red (both red), and the untouched green path. Mutation-tested, 3 of 3 caught: skipping the A/B, claiming pre-existing without measuring, and dropping the tree restore.
All seven findings verified before fixing; the three Criticals were each a way the A/B compared something other than the check that failed. R1-1 — the contracts check feeds on stdin, which its first run drains; the baseline leg re-ran against EOF and checked an empty file list. R1-3 — the schema check's verdict rides on packages/core/dist, which the core-rebuild guard built from ROUND sources and which, being gitignored, survives the detach. Both checks are now A/B-exempt (run_check_no_ab): their baseline verdicts prove nothing, and their rejections stay where the repair agent can actually act on them. R1-2 — a workspace the round ADDS does not exist at the baseline, and npm exits 1 there with "No workspaces found" (measured; --if-present forgives a missing script, not a missing workspace) — a round-caused failure misread as pre-existing, skipping the one repair that can fix the round's own package. The per-package loop now A/Bs only when the workspace exists at origin/<branch>. R1-4 — a chatty PASSING baseline used to flood the tail -c 3000 evidence window and push the actual failure text out of gate-rejection.md, the sole carrier into the repair feedback, the PR comment, and the next round's LAST_REJECTION. The baseline transcript now goes to a side log and only a FAILING tail is merged back, where it is the evidence. R1-5 — the pre-existing paragraph pushed gate-rejection.md past the report's head -c 3500 cap, truncating the closing fence for branch names past 44 characters. Cap raised to 3900, invariant comment updated with the new arithmetic. R1-6 — preexisting=true had no read site. It now flows verify → Finalize verification → the failure report, whose headline swaps the generic gate clause for "PRE-EXISTING failure … needs a base update (merge main)". R1-7 — the no-round-commit guard was unpinned (deleting it kept all tests green). Now exercised through the core-rebuild path, the one A/B-eligible check that runs before the commit gate. Four new behavioral scenarios (chatty baseline, no-commit round, A/B-exempt checks, round-added workspace) plus workflow pins for the forwarding, the clause, and the cap. Mutation-tested, 4 of 4 caught: schema back to A/B (3 tests), guard dropped, side log reverted, no-commit guard dropped.
Ten findings across two rounds, each verified before fixing. The three deepest share one lesson: the A/B is only sound for a check whose inputs travel entirely with the git ref, and whose failure it can IDENTIFY, not merely observe. R2-1 — rc=1 at both legs does not make them the same failure: the branch can fail for reason A while the round fails for reason B, and a baseline infrastructure hiccup is a nonzero exit too. Pre-existing now requires a MATCHING failure identity — tsc diagnostics normalized to file + error code (positions shift with the round's edits), compared via comm(1) on a per-check transcript. No diagnostics on either side means identity cannot be established and the round stays charged. R2-2 / R2-7 — gitignored dist survives the detach carrying the ROUND's build, so any dist-consuming check A/Bs reverted sources against round-built artifacts: package tests (channel-base resolved through dist exports) and typecheck (sdk-typescript resolves core's d.ts — probe-verified three-arm flip). Both are now A/B-exempt, as is lint, leaving `npm run build` — the incident class, and the one check that rebuilds its own inputs from the checked-out sources — as the sole A/B candidate. The workspace-existence guard dissolves with it. R2-3 — the fixture inherited the caller's global git config; a failing global pre-commit hook broke all seven cases. The harness now isolates GIT_CONFIG_GLOBAL/SYSTEM for every git child, and the suite is proven green under a deliberately hostile hooksPath. R2-4 — Finalize verification now selects preexisting from the same attempt whose outcome it selects (repair verification included). R2-5 / R2-8 — the "merge main" advice is now conditional at both layers: the script paragraph states the measured fact and hedges the remedy; the report headline uses the compare the step already ran — behind/diverged gets the base-update clause, an up-to-date branch is told its own pre-round code needs attention. R2-6 — the rejection document now sizes its evidence tail against its preamble (floor 500 bytes, total under the 3900-byte render cap), so the closing fence can no longer be truncated off by a long branch name. R2-9 — dissolved by R2-2: package tests no longer A/B, the guard and its uncovered positive branch are gone. R2-10 — the baseline-evidence merge is now pinned: the pre-existing scenario asserts the baseline leg's own failure line (keyed by its SHA) reaches gate-rejection.md. Eight behavioral scenarios; mutation-tested 5 of 5: identity dropped, typecheck re-enrolled, package tests re-enrolled, evidence merge dropped, fixed tail restored.
…c prose Nine findings, all refinements — the design held, the edges did not. Identity now keeps the diagnostic MESSAGE (file + code collide: two unrelated TS2339s in one file compared equal, skipping a repair that could have shipped — probe-reproduced by the review), and the fixture emits a SHIFTED position on the baseline leg so the position strip is load-bearing instead of decorative (deleting the sed survived every test before; it fails one now). vite/esbuild failures still yield an empty signature by design — documented as the fail-closed limit rather than half-widened. The fail_signature assignments take `|| true`: grep exits 1 on the normal no-match case and survives errexit today only because the caller sits in an if-condition — a future unconditional call site would crash the gate verdict-less. The restore-failure branch is now stageable and staged: the baseline leg recreates (untracked) a file the branch tracks, the checkout back refuses, and the test pins retryable-not-preexisting with the 'could not restore' label. Relaxing the branch to `|| true` fails it. Prose synced to the mechanisms that replaced it: the render-cap invariant restates against the dynamic tail budget (the old 3000-based arithmetic would misguide the next retune), the no-round-commit guard comment names the core rebuild (schema/contracts left the A/B last round), the describe wording counts both A/B-eligible builds, and the pre-existing clauses no longer claim "the repair pass was skipped" — with REPAIR_PREEXISTING forwarded, repair may have RUN; they now state the invariant that is true either way: repair may only amend the round's own fix, so it cannot reach this failure. Mutation-tested, 3 of 3 caught: position strip dropped, message dropped from the identity, restore rejection relaxed.
Four autofix rounds have died the same way (#8663 twice, #8761 r3, #8763 r4): the agent's last output is the sandbox wrapper's "ContainerName (regular): …" line at docker container entry, then nothing — not one event — until the 2-hour absolute budget kills the round. Four different runners, two image versions: systemic, not a bad machine. Where exactly the container wedges is still unknown (that needs docker state on the runner); what is certain from the logs is the shape — a wedged sandbox produces NOTHING, and a legitimate run is never silent for long (the fleet's longest tolerated quiet is the review pipeline's 10-minute stream-idle window for thinking phases). Two mitigations, each aimed at a measured half of the damage: - run-agent.mjs gains an idle watchdog (QWEN_IDLE_TIMEOUT_MS, default 20 minutes = 2x that longest legitimate silence): zero output for the window kills the agent with a distinct "idle-timeout … the sandbox likely hung at startup" detail, so the failure comment names the right knob and a hung round costs 20 minutes instead of 120. Polled, not reset-per-chunk — a busy stream should not spend its time re-arming timers. - Both sandboxed jobs reap stale qwen-code-* containers at job start: a budget kill reaps the HOST-side docker client, not the container, so every killed sandbox keeps running on the persistent runner — observed directly when a later leg's container-name counter found qwen-code-0.21.8-0 already occupied and picked -1. One job per runner at a time makes any container alive at job start stale by definition. Tested by executing the real run-agent.mjs end to end with stub agents: the hang shape (one line, then silence) dies at the idle window naming the idle limit, and a slow-but-talking agent that outputs every 400ms across a 1500ms window survives to a clean exit — the test that distinguishes a watchdog from a disguised absolute timer. Mutation- tested, 3 of 3 caught: watchdog disabled, last-output tracking dropped (the disguised-timer regression), cleanup dropped from a job.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
Thanks for the PR! Re-run at the current head — gate re-checked, still passing. Template looks good ✓ Problem: observed, not theoretical. Four autofix rounds died identically (#8663 ×2, #8761 r3, #8763 r4): last byte printed at docker container entry, then two full hours of silence, across four runners and two image versions. The container leak is directly observed too (a later leg's name counter found Direction: aligned — reliability of the repo's own autofix pipeline, measurably burning runner-hours. CI infra, no CHANGELOG surface, clearly in scope. Size: no core paths touched. At the current head the total diff is +1186/−32 across 4 files, inflated by the stacking: roughly half is open PR #8765's A/B-gate work, and this PR's own four commits churn the rest (~790 of those lines are tests). Production logic is well under every advisory threshold. Approach: the two mitigations remain the right shape — an output-activity watchdog (with a distinct failure detail and retry routing, not a disguised second absolute timer) plus container cleanup split by ownership: the kill path removes the container its own agent launched, the job-start reaper touches only exited/dead containers since a running one can belong to a concurrent job on another registration of the same host. Review rounds since the last run addressed the prior findings (hang-bound every docker call in the reaper, made the kill-path removal async so it can't block the SIGKILL backstop, pinned the previously unpinned test arms). One update from the prior run: #8765 has since advanced one more commit (its round 5, Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献！已在最新 head 上重跑——门禁复查通过。 模板完整 ✓ 问题： 已观测到的问题，不是理论性的。四个 autofix 轮次以相同方式死亡（#8663 ×2、#8761 r3、#8763 r4）：最后一个字节打印在进入 docker 容器时，之后整整两小时沉默，横跨四台 runner 和两个镜像版本。容器泄漏也有直接观察证据（后续 leg 的命名计数器发现 方向： 对齐——仓库自身 autofix 流水线的可靠性，有可测量的 runner-小时损耗。CI 基建，不涉及 CHANGELOG 面，明显在范围内。 规模： 未触及核心路径。当前 head 的总 diff 为 4 个文件 +1186/−32，被堆叠放大：约一半是 open PR #8765 的 A/B 门禁工作，本 PR 自己的四个提交贡献其余部分（其中约 790 行是测试）。生产代码远低于所有提示阈值。 方案： 两项缓解形态依然正确——基于输出活动的看门狗（独立的失败详情与重试路由，不是伪装成看门狗的第二个绝对计时器），加上按所有权拆分的容器清理：kill 路径删除自己 agent 启动的容器，job 启动收割只碰 exited/dead 容器——运行中的容器可能属于同一宿主上另一个注册的并发 job。上次运行以来的 review 轮次已解决先前发现的问题（收割的每个 docker 调用都加了超时上限、kill 路径的删除改为异步以免阻塞 SIGKILL 兜底、补齐了未钉住的测试分支）。相比上次运行的一个更新：#8765 又前进了一个提交（其第 5 轮 风险： 无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
@qwen-code /takeover |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no action takenThe only feedback item this round is the failed check Evidence
There were no review, inline, or issue-level findings from trusted maintainers or the automated reviewer this round (the round was triggered by the 中文说明Autofix 评审轮次：无需处理本轮唯一的反馈项是失败检查 证据
本轮没有来自受信维护者或自动评审器的 review、行内或 issue 级发现（本轮由 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
[Critical] .github/scripts/run-autofix-review-verification.sh:69 SC2016 — Expressions don't expand in single quotes, use double quotes for that. [lint]
[Critical] .github/scripts/run-autofix-review-verification.sh:69 SC2016 — Expressions don't expand in single quotes, use double quotes for that. [lint]
中文说明
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
[Critical] .github/scripts/run-autofix-review-verification.sh:69 SC2016 — Expressions don't expand in single quotes, use double quotes for that. [lint]
[Critical] .github/scripts/run-autofix-review-verification.sh:69 SC2016 — Expressions don't expand in single quotes, use double quotes for that. [lint]
— qwen3.8-max via Qwen Code /review (v0.21.8)
| grep -oE "[^ '\"]+\([0-9]+,[0-9]+\): error TS[0-9]+[^\n]*" "${1}" 2> /dev/null \ | ||
| | sed -E 's/\([0-9]+,[0-9]+\)//' | sort -u |
There was a problem hiding this comment.
[Critical] R1-1: [^\n]* in this ERE bracket expression does not mean "rest of line" — in a POSIX bracket expression \ is literal, so [^\n] matches "any character except backslash and the letter n", truncating every tsc message at its first n and collapsing distinct same-file/same-code failures into identical signatures. — Failure scenario: the round fails npm run build with src/x.ts(10,5): error TS2307: Cannot find module './foo' while the baseline leg fails with a different defect src/x.ts(8,12): error TS2307: Cannot find module './bar'. Both reduce to the identical signature src/x.ts: error TS2307: Ca (cut at the n of "Cannot"), comm -12 intersects, and the gate emits preexisting=true for a round-caused failure — the repair pass that could have fixed it is skipped and the comment misdirects to "merge main". Probe-verified end to end against the real script; nearly every real tsc message contains an early n ("Cannot find name", "is not assignable", "does not exist on type"), and the new tests pass only because the stub message stub failure contains no n.
| grep -oE "[^ '\"]+\([0-9]+,[0-9]+\): error TS[0-9]+[^\n]*" "${1}" 2> /dev/null \ | |
| | sed -E 's/\([0-9]+,[0-9]+\)//' | sort -u | |
| grep -oE "[^ '\"]+\([0-9]+,[0-9]+\): error TS[0-9]+.*" "${1}" 2> /dev/null \ | |
| | sed -E 's/\([0-9]+,[0-9]+\)//' | sort -u |
(grep is line-oriented, so .* is exactly "rest of the line"; also add a fixture whose two messages differ only after their first n.)
中文说明
[Critical] R1-1：此 ERE 括号表达式中的 [^\n]* 并非"行剩余部分"——POSIX 括号表达式把 \ 当字面量，因此 [^\n] 匹配"除反斜杠和字母 n 外的任意字符"，会在第一个 n 处截断每条 tsc 消息，使同文件/同错误码的不同故障坍缩为相同签名。— 故障场景：本轮 npm run build 因 src/x.ts(10,5): error TS2307: Cannot find module './foo' 失败，而基线分支因另一个不同的缺陷 src/x.ts(8,12): error TS2307: Cannot find module './bar' 失败。两者归约为完全相同的签名 src/x.ts: error TS2307: Ca（在 "Cannot" 的 n 处截断），comm -12 求交非空，gate 对本轮导致的失败输出 preexisting=true——本可修复该失败的 repair 阶段被跳过，评论还会误导去 "merge main"。已对真实脚本端到端探针复现；几乎所有真实 tsc 消息都在靠前位置含 n（"Cannot find name"、"is not assignable"、"does not exist on type"），新测试之所以通过仅因为桩消息 stub failure 不含 n。修复：改用 .*（grep 按行工作），并补充两条消息仅在第一个 n 之后不同的夹具用例。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| # The tree is no longer the one under verification and nothing after | ||
| # this point may trust it. | ||
| reject_fix 'could not restore the verification tree after the baseline check' |
There was a problem hiding this comment.
[Critical] R1-2: when the post-baseline restore fails, this rejects with retryable=true (reject_fix's second arg defaults to false) while the workspace stays detached at the baseline commit — a tree the comment above declares untrustworthy. Nothing downstream re-establishes the branch before acting on that verdict. — Failure scenario: the baseline leg recreates an untracked file the branch tracks (the exact shape the new restoreClash test constructs) or git fails transiently → retryable=true is written with HEAD detached at the pre-round SHA. The repair step is gated only on retryable == 'true' and performs no checkout; run-agent.mjs does no git ops — so the 18-minute repair agent works on the baseline tree that lacks the round's fix, its commit lands on detached HEAD and is orphaned by verify_repair's opening checkout, and the round is reported as if no repair ran. A persisting clash can even kill verify_repair verdict-less. Fix options (pick one): make this specific rejection non-retryable (the repair agent cannot act on an untrusted tree); or force-restore before rejecting (git checkout -f --quiet "${BRANCH}" and remove the clashing untracked file); or have the repair step assert/restore HEAD == ${BRANCH} before launching the agent.
中文说明
[Critical] R1-2：基线运行后恢复分支失败时，这里以 retryable=true 拒绝（reject_fix 第二参默认 false），但工作区仍停留在基线提交（detach）——正是上方注释宣称"不可再信任"的树。下游没有任何环节在依据该结论行动前恢复分支。— 故障场景：基线分支重新生成了分支所跟踪的未跟踪文件（新增 restoreClash 测试构造的正是此形态）或 git 瞬时失败 → HEAD 停留在轮前 SHA 的 detach 状态却写出 retryable=true。repair 步骤仅以 retryable == 'true' 为门控且不执行任何 checkout；run-agent.mjs 不做任何 git 操作——于是 18 分钟的 repair agent 在缺少本轮修复的基线树上工作，其提交落在 detached HEAD 上并被 verify_repair 开头的 checkout 抛弃，整轮被报告得如同从未 repair。若冲突持续存在，verify_repair 甚至会在没有结论的情况下被杀掉。修复（三选一）：将该特定拒绝改为非 retryable（repair agent 本就无法在不可信的树上行动）；或拒绝前强制恢复分支（git checkout -f 并移除冲突的未跟踪文件）；或让 repair 步骤在启动 agent 前断言/恢复 HEAD == ${BRANCH}。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| command -v docker > /dev/null || exit 0 | ||
| STALE="$(docker ps -aq --filter 'name=qwen-code-')" |
There was a problem hiding this comment.
[Critical] R1-3: the reap's premise — "a runner executes one job at a time, so any qwen-code-* container alive at job start is stale by definition" — holds per runner registration, but the filter queries the docker daemon, which is per host, and this workflow's own env comment (~lines 731-734) documents that this pool "carries many registrations sharing one OS". — Failure scenario: the concurrency groups serialize per-issue/per-PR only (max-parallel: 20 across different targets), so job B (a different issue/PR) can land on a second registration of the same host while job A's sandbox qwen-code-0.21.8-0 is running. Probe-verified on a live daemon: docker ps -aq --filter 'name=qwen-code-' is a substring match that lists the RUNNING container, and docker rm -f destroys running containers — job B's job-start reap force-removes job A's live sandbox mid-run, burning A's round and failing it with infrastructure noise (and symmetrically, another job's reaper can kill this job's container during its long pre-agent window). Suggested direction: make ownership unambiguous — label autofix containers at creation (--label qwen-autofix-run=<run_id>) and reap only containers whose owning job is verifiably dead, or serialize sandbox jobs per host; at minimum, correct the "alive ⇒ stale" comment and restrict the filter (e.g. --filter status=exited, pending maintainer judgement on which states must die). Applies to both copies (also in review-address).
中文说明
[Critical] R1-3：收割步骤的前提——"一台 runner 同时只跑一个 job，因此 job 启动时任何活着的 qwen-code-* 容器按定义都是残留"——对单个 runner 注册 成立，但过滤查询的是 docker 守护进程（按主机共享），而本 workflow 自己的 env 注释（约 731-734 行）明确记载该池"多个注册共享同一个 OS"。— 故障场景：并发组仅按 issue/PR 串行（不同目标间 max-parallel: 20），因此 job B（不同 issue/PR）可以落到同一主机的第二个注册上，而 job A 的沙箱 qwen-code-0.21.8-0 正在运行。已在真实守护进程上探针验证：docker ps -aq --filter 'name=qwen-code-' 是子串匹配且会列出运行中容器，docker rm -f 可以销毁运行中容器——job B 的启动收割会强制删除 job A 正在运行的沙箱，烧掉 A 的整轮并使其以基础设施噪声失败（对称地，本 job 的容器在 agent 启动前的漫长窗口里也可能被别的 job 的收割杀掉）。修复方向：让归属无歧义——创建容器时打标签（--label qwen-autofix-run=<run_id>），只收割能确认属主 job 已死的容器，或按主机串行沙箱 job；至少应修正 "alive ⇒ stale" 注释并收窄过滤（如 --filter status=exited，由维护者判断哪些状态必须杀）。两份拷贝（含 review-address）同样适用。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if [[ -z "${sig_head}" || -z "${sig_base}" ]] || | ||
| ! comm -12 <(printf '%s\n' "${sig_head}") <(printf '%s\n' "${sig_base}") \ | ||
| | grep -q .; then |
There was a problem hiding this comment.
[Critical] R1-4: under the script's set -eo pipefail, grep -q exits on the first match and SIGPIPEs comm once the common-signature output outruns the pipe buffer (exit 141) — pipefail then fails the pipeline and this condition takes the NO-MATCH branch for IDENTICAL failure signatures. This is the inverse of the [^\n] finding above: genuine pre-existing failures get charged to the round. — Failure scenario: both legs fail with a large shared tsc diagnostic set — exactly the monorepo-wide build break this function's own comment cites (run 31276008548). Probe-verified: ~200-300 shared unique diagnostics already flip the condition (grep -q never reads past the first match, so the threshold is far below one pipe buffer); 1,200 shared lines flip 5/5 trials. The gate then writes retryable=true without preexisting — the 18-minute repair is burned on a failure the repair agent is forbidden to touch, round after round: precisely the 13-round waste this feature was built to kill. The 2-line stub fixtures stay below the threshold, which is why every added test passes.
local common
common="$(comm -12 <(printf '%s\n' "${sig_head}") <(printf '%s\n' "${sig_base}") || true)"
if [[ -z "${sig_head}" || -z "${sig_base}" ]] || [[ -z "${common}" ]]; then(fix flip-verified: matches at 1,200 and 5,000 shared lines; disjoint large sets still correctly NO-MATCH.)
中文说明
[Critical] R1-4：在脚本 set -eo pipefail 下，grep -q 在首个匹配即退出，一旦共同签名输出超过管道缓冲区就会使 comm 收到 SIGPIPE（exit 141）——pipefail 使整个管道判为失败，条件因此对"完全相同"的失败签名走入"无匹配"分支。这是上面 [^\n] 问题的镜像：真正的预存失败被记到本轮头上。— 故障场景：两条腿因大量相同的 tsc 诊断失败——正是本函数注释引用的那种整仓构建崩溃（run 31276008548）。探针验证：约 200-300 条共同唯一诊断即可翻转条件（grep -q 从不读取首个匹配之后的内容，阈值远低于一个管道缓冲区）；1,200 条共同输出 5/5 次翻转。gate 随后写出 retryable=true 而无 preexisting——18 分钟 repair 被浪费在 repair agent 被禁止触碰的失败上，一轮接一轮：恰是本特性要消灭的"连续 13 轮浪费"。2 行桩夹具低于阈值，所以所有新增测试都通过。修复如上（先捕获 comm -12 结果再测 [[ -n ... ]]）；已验证修复翻转：1,200 与 5,000 条共同输出均正确匹配，大的不相交集合仍正确判为无匹配。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| idleTimer = setInterval(() => { | ||
| if (settled || idleTimedOut) return; |
There was a problem hiding this comment.
[Suggestion] R1-5: the idle guard checks settled || idleTimedOut but not timedOut — after the absolute budget fired first, a later idle tick can overwrite the classification, because the failure-detail chain checks idleTimedOut before timedOut. — Failure scenario (probe-verified): an agent streams output, then wedges on a pipe-holder outside the child's process group (the hang class this PR targets — processes a SIGKILL cannot immediately settle); the absolute budget fires first, the child never settles, and a later idle tick flips failure.md AND the agent-timeout sentinel to idle-timeout (... the sandbox likely hung at startup) — sending the operator to docker startup instead of a mid-run hang. Kill behavior is unaffected; only the label is wrong. Probe flip: with timedOut in the guard, the detail stays timeout (...).
| idleTimer = setInterval(() => { | |
| if (settled || idleTimedOut) return; | |
| idleTimer = setInterval(() => { | |
| if (settled || timedOut) return; |
中文说明
[Suggestion] R1-5：空闲守卫检查 settled || idleTimedOut 但不检查 timedOut——绝对预算先触发后，之后的空闲 tick 仍可能改写分类，因为失败详情链先查 idleTimedOut 后查 timedOut。— 故障场景（已探针验证）：agent 持续输出后卡在子进程进程组之外的管道持有者上（正是本 PR 针对的挂死形态——SIGKILL 无法立即结算的进程）；绝对预算先触发，子进程始终不结算，之后的空闲 tick 把 failure.md 与 agent-timeout 哨兵都改写为 idle-timeout (... the sandbox likely hung at startup)——把运维引向 docker 启动问题而非运行中挂死。杀进程行为不受影响，只是标签错误。探针翻转：守卫加入 timedOut 后详情保持 timeout (...)。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round — the two previous rounds exhausted the agent budget, so this round took only the Criticals). Finding is valid: the idle guard should also check timedOut so a late idle tick cannot reclassify a run after the absolute budget fired first. It is a one-token fix (settled || timedOut); queued first for the next round.
中文说明
暂缓（预算轮——前两轮均耗尽 agent 时间预算，本轮只处理 Critical）。该发现成立：空闲守卫应同时检查 timedOut，以免绝对预算先触发后，后续的空闲 tick 改写分类。这是一个单 token 修复（settled || timedOut），已列为下一轮首要事项。
| expect(r.status).not.toBe(0); | ||
| expect(r.failure).toContain('idle-timeout (no output for 1200ms'); |
There was a problem hiding this comment.
[Suggestion] R1-10: this test asserts the failure.md wording but never the agent-timeout sentinel — the one-line mutation deleting timedOut = true; from the idle branch of run-agent.mjs survives the entire 138-test suite (mutant-verified). — Failure scenario (probe-verified end to end): with that line gone, an idle kill still writes failure.md with the idle wording (every current assertion passes) but skips if (result.timedOut) → agent-timeout; the workflow's handoff (report step, ~line 5184) reads exactly that sentinel to route a budget-kill to a retry — the probe shows the watermark then ADVANCES and the feedback is stranded, the exact outcome the code comment forbids. With shipped code the same probe yields the retry sentinel. A future refactor dropping that one line ships green. Fix: return the workdir's agent-timeout content from runAgent (before its finally cleanup) and assert it exists and names the idle limit, mirroring the existing absolute-timeout test.
中文说明
[Suggestion] R1-10：该测试只断言 failure.md 措辞，从不断言 agent-timeout 哨兵——从 run-agent.mjs 空闲分支删掉 timedOut = true; 的单行变异可在全部 138 个测试下存活（已变异验证）。— 故障场景（端到端探针验证）：删掉该行后，空闲杀仍会写出带空闲措辞的 failure.md（当前所有断言通过），但跳过 if (result.timedOut) → agent-timeout；workflow 的交接（report 步骤约 5184 行）恰恰读取该哨兵把预算杀路由为重试——探针显示水位随之前进、反馈被丢下，正是代码注释明令禁止的结果。shipped 代码下同一探针产出重试哨兵。未来某次重构删掉这一行也能全绿通过。修复：让 runAgent 在 finally 清理前返回 workdir 中 agent-timeout 的内容，并断言其存在且标明空闲限制，与现有绝对预算测试对齐。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round). Valid mutant finding: the idle-kill test should also assert the agent-timeout sentinel (returning the workdir file from runAgent before its cleanup), mirroring the absolute-timeout test. Queued with the test-hardening batch.
中文说明
暂缓（预算轮）。变异测试发现成立：空闲杀测试还应断言 agent-timeout 哨兵（在 runAgent 清理前返回该 workdir 文件内容），与绝对预算测试对齐。已列入测试加固批次。
| expect(workflow).toContain('needs a base update (merge main)'); | ||
| // The YAML embeds the apostrophe via shell quoting, so match around it. | ||
| expect(workflow).toContain('own pre-round code needs attention'); |
There was a problem hiding this comment.
[Suggestion] R1-11: these assertions check that both GATE_CLAUSE strings and the CMP_R conditional exist in the workflow, but never the CORRESPONDENCE between condition and clause — swapping the two clause bodies survives all of them (mutant-verified: 138/138 pass with the swap applied). — Failure scenario: if that drift ever ships (an edit or a merge-conflict resolution), the failure report tells a behind/diverged branch its "own pre-round code needs attention" and an up-to-date branch to "merge main" — steering the human to the wrong remedy for the exact failure class this feature exists to classify. Fix: span the regex into the then-body and add a symmetric assertion, e.g.
expect(workflow).toMatch(
/== 'behind' \|\| "\$\{CMP_R:-\}" == 'diverged' \]\]; then\n\s*GATE_CLAUSE=[^\n]*base update \(merge main\)/,
);
expect(workflow).toMatch(
/else\n\s*GATE_CLAUSE=[^\n]*own pre-round code needs attention/,
);中文说明
[Suggestion] R1-11：这些断言检查两个 GATE_CLAUSE 字符串与 CMP_R 条件都存在于 workflow 中，但从不检查条件与子句的对应关系——交换两个子句体后所有断言仍通过（已变异验证：交换后 138/138 全过）。— 故障场景：若该漂移真的上线（手误编辑或合并冲突解决），失败报告会告诉落后/分叉的分支"自身轮前代码有问题"，告诉最新分支去 "merge main"——对本特性专门要分类的失败类别给出完全相反的处置指引。修复：把正则延伸进 then 分支并补对称断言（示例如上）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round). Valid: the GATE_CLAUSE assertions should extend into the then/else branches so a swapped-clause mutation fails. Queued with the test-hardening batch.
中文说明
暂缓（预算轮）。成立：GATE_CLAUSE 断言应延伸进 then/else 分支，使子句交换的变异失败。已列入测试加固批次。
| 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"'); |
There was a problem hiding this comment.
[Suggestion] R1-12: the evidence-window invariant (finished document ≤ 3900 bytes so the closing fence survives head -c 3900) is pinned only by string-existence checks — no executed fixture ever saturates tail -c "${tail_budget}" (every stub failure emits ~2 lines). — Failure scenario (probe-verified): the floor mutant (( tail_budget < 3500 )) && tail_budget=3500 keeps all 13 tests green, and a saturation harness with a 21 KB gate log renders a 4004-byte document whose head -c 3900 cuts the closing fence — the exact truncation malformation this PR's comments exist to prevent — while the original code produces 3312 bytes with the fence intact. Production build transcripts routinely exceed the budget, so saturation is the normal path. Fix: add a runGate scenario whose failing stub emits >4 KB so the tail saturates, and assert the resulting gate-rejection.md ends with the closing fence and/or stays under 3900 bytes.
中文说明
[Suggestion] R1-12：证据窗口不变量（成品文档 ≤ 3900 字节，收尾栅栏在 head -c 3900 下存活）仅由字符串存在性检查钉住——没有任何执行中的夹具使 tail -c "${tail_budget}" 饱和（每个桩失败只输出约 2 行）。— 故障场景（已探针验证）：把下限变异为 (( tail_budget < 3500 )) && tail_budget=3500 后全部 13 个测试仍绿；用 21 KB gate 日志做饱和测试时渲染出 4004 字节文档，head -c 3900 会切掉收尾栅栏——正是本 PR 注释要防止的截断畸形——而原代码在同一输入下产出 3312 字节、栅栏完好。生产构建日志经常超过预算，饱和才是常态路径。修复：新增一个失败桩输出 >4 KB 的 runGate 场景使尾部饱和，断言生成的 gate-rejection.md 以收尾栅栏结尾且/或总长小于 3900 字节。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round). Valid: no executed fixture saturates tail -c "${tail_budget}", so the ≤3900-byte closing-fence invariant is unpinned. Queued: one runGate scenario whose failing stub emits >4 KB, asserting the document ends with the closing fence and stays under 3900 bytes.
中文说明
暂缓（预算轮）。成立：没有任何执行中的夹具使 tail -c "${tail_budget}" 饱和，因此 ≤3900 字节的收尾栅栏不变量未被钉住。已列入计划：新增一个失败桩输出 >4 KB 的 runGate 场景，断言文档以收尾栅栏结尾且总长小于 3900 字节。
| // contexts, so twice that is the default. Distinct from QWEN_TIMEOUT_MS so | ||
| // the failure comment says which limit fired. |
There was a problem hiding this comment.
[Suggestion] R1-13: this design statement — "Distinct from QWEN_TIMEOUT_MS so the failure comment says which limit fired" — silently never applies to the repair leg: the 20-min idle default is above the repair pass's hardcoded 18-min absolute budget (QWEN_TIMEOUT_MS: '1080000'), so the absolute timer always settles a silent repair run first. And QWEN_IDLE_TIMEOUT_MS is forwarded NOWHERE in the workflow (grep: zero occurrences), so there is no operator knob to change that. — Failure scenario (probe-verified, flipped arms): a sandbox wedged at docker entry during a repair leg — the same docker sandbox surface the watchdog exists for — burns the full 18 min and reports timeout (1080000ms) in both failure.md and the sentinel feeding the PR comment, indistinguishable from budget exhaustion. The same dormancy hits the primary pass whenever an operator lowers QWEN_AUTOFIX_TIMEOUT_MS below 20 min (the clamp comment says it "can only LOWER the fallback", floor 60 s). Wall-clock behavior is unchanged (still bounded) — the cost is diagnostic precision plus tunability. Fix: forward a vars-backed knob to both agent steps the way the sibling is wired (QWEN_TIMEOUT_MS: '${{ vars.QWEN_AUTOFIX_TIMEOUT_MS || 7200000 }}'), set below 1080000 on the repair leg — or lower the default below the repair budget.
中文说明
[Suggestion] R1-13：这条设计声明——"与 QWEN_TIMEOUT_MS 区分，使失败评论说明是哪条限制触发"——在 repair 分支上悄悄失效：20 分钟的空闲默认值高于 repair 分支硬编码的 18 分钟绝对预算（QWEN_TIMEOUT_MS: '1080000'），因此绝对计时器总是先结算静默的 repair 运行。而且 QWEN_IDLE_TIMEOUT_MS 在 workflow 中没有任何地方转发（grep：零出现），运维没有任何旋钮可以改变这一点。— 故障场景（探针双臂翻转验证）：repair 分支中沙箱在 docker 入口处挂死——正是看门狗要覆盖的 docker 沙箱面——烧满 18 分钟后在 failure.md 与喂给 PR 评论的哨兵中都报告 timeout (1080000ms)，与预算耗尽无法区分。同样的休眠也会在运维把 QWEN_AUTOFIX_TIMEOUT_MS 调低于 20 分钟时发生在主分支（钳制注释写明它"只能调低回退值"，下限 60 秒）。墙上时钟行为不变（仍有界）——代价是诊断精度与可调节性。修复：像兄弟旋钮一样在两个 agent 步骤转发 vars 驱动的旋钮，repair 分支设低于 1080000；或把默认值降到低于 repair 预算。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round). Valid diagnostic-precision gap: on the repair leg the 20-min idle default sits above the hardcoded 18-min absolute budget, and the knob is forwarded nowhere. Fix is workflow wiring (a vars-backed knob on both agent steps, below 1080000 on the repair leg); queued as one unit.
中文说明
暂缓（预算轮）。诊断精度缺口成立：repair 分支上 20 分钟空闲默认值高于硬编码的 18 分钟绝对预算，且该旋钮无处转发。修复是 workflow 接线（两个 agent 步骤都转发 vars 驱动的旋钮，repair 分支设低于 1080000）；已作为整体列入计划。
| idleTimedOut = true; | ||
| timedOut = true; |
There was a problem hiding this comment.
[Suggestion] R1-14: idle timeouts set timedOut = true, so wedge rounds write the shared agent-timeout sentinel; the report headline 🤖 AutoFix ran out of time before finishing (idle-timeout ...) matches the cumulative-census needle verbatim (probe-verified), so idle wedges increment TIMEOUT_N toward TIMEOUT_WINDOW_CAP=3. — Failure scenario: a PR whose sandbox wedges at entry in 3 rounds of one counting window — a sick runner pool is precisely the recurrence this PR's reap rationale describes, and wedges interleaved with healthy rounds reset only CONSEC_FAIL — trips the cumulative breaker: MARK_ROUND is forced to MAX_ROUNDS and the PR is marked terminal with advice that cannot fix a docker wedge ("split or reduce the PR (or raise the agent time budget AND its step backstop together)") until a human re-arms — while the exact fact needed to say otherwise (idle-timeout … the sandbox likely hung at startup) is sitting in AGENT_TIMEOUT when the headline is composed. (Three zero-work rounds meriting human attention is defensible; the defect is the mislabel and the unactionable advice.) Fix: write a distinct agent-idle-timeout sentinel (or a census-excludable headline) so wedges stay bounded by the consecutive-failure cap, or adapt the terminal advice for the idle class to name runner health.
中文说明
[Suggestion] R1-14：空闲超时置 timedOut = true，因此挂死轮次写入共享的 agent-timeout 哨兵；报告标题 🤖 AutoFix ran out of time before finishing (idle-timeout ...) 与累计计数器的匹配串逐字相同（已探针验证），于是空闲挂死也计入 TIMEOUT_N，逼近 TIMEOUT_WINDOW_CAP=3。— 故障场景：某 PR 的沙箱在一个计数窗口内 3 轮都挂在入口——病态 runner 池正是本 PR 收割注释描述的复发形态，且挂死轮与健康轮交错时只重置 CONSEC_FAIL——累计断路器触发：MARK_ROUND 被强制为 MAX_ROUNDS，PR 被标记终止并给出无法修复 docker 挂死的建议（"拆分或缩小 PR（或同时上调 agent 时间预算与步骤上限）"），直到人工重置——而组成标题时 AGENT_TIMEOUT 里明明就有能说明真相的事实（idle-timeout … the sandbox likely hung at startup）。（三轮零产出确实值得关注，缺陷在于错误归类与不可执行的建议。）修复：写独立的 agent-idle-timeout 哨兵（或可被计数器排除的标题），使挂死仅受连续失败上限约束；或为空闲类别改写终止建议，指向 runner 健康。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round). Valid: idle wedges currently count toward the cumulative timeout census and trip the terminal breaker with budget advice that cannot cure a wedge. This is breaker/census behavior and overlaps the report-branching finding below — the two should land together as one idle-class disposition (distinct sentinel or census-exempt headline, plus adapted terminal advice).
中文说明
暂缓（预算轮）。成立：空闲挂死目前计入累计超时计数，并以无法修复挂死的预算建议触发终态熔断。这属于熔断器/计数行为，且与下条报告分支建议重叠——两者应作为一个空闲类别处置整体落地（独立哨兵或可被计数排除的标题，外加改写的终态建议）。
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
.github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted. [Critical] .github/scripts/run-autofix-review-verification.sh:69 SC2016 — Expressions don't expand in single quotes, use double quotes for that. [lint] [Critical] .github/scripts/run-autofix-review-verification.sh:69 SC2016 — Expressions don't expand in single quotes, use double quotes for that. [lint]
中文说明
.github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。 [Critical] .github/scripts/run-autofix-review-verification.sh:69 SC2016 — Expressions don't expand in single quotes, use double quotes for that. [lint] [Critical] .github/scripts/run-autofix-review-verification.sh:69 SC2016 — Expressions don't expand in single quotes, use double quotes for that. [lint]
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
| if [[ -z "${sig_head}" || -z "${sig_base}" ]] || | ||
| ! comm -12 <(printf '%s\n' "${sig_head}") <(printf '%s\n' "${sig_base}") \ | ||
| | grep -q .; then |
There was a problem hiding this comment.
[Critical] The pre-existing verdict fires on ANY one shared signature line: comm -12 intersection non-empty → preexisting=true, so a round-caused failure that shares a single normalized diagnostic (file + error code + message, position stripped) with the baseline is mislabeled pre-existing — the repair is skipped and the report names the wrong remedy. — Failure scenario: the round fixes a pre-existing error and introduces a different defect with an identical file+code+message at another position (or adds a second instance while the old one remains). sig_head ∩ sig_base ≠ ∅ → preexisting=true, retryable unset → the repair step is skipped and the round's commit is discarded, even though the head's actual failure is round-caused and repairable; the report tells the human "base update needed / pre-round code needs attention" — the wrong remedy. Probe (real script): head = baseline's 5 diagnostics + 5 round-caused TS8888 → preexisting=true, no retryable; a subset-rule patch flips it to retryable=true.
| if [[ -z "${sig_head}" || -z "${sig_base}" ]] || | |
| ! comm -12 <(printf '%s\n' "${sig_head}") <(printf '%s\n' "${sig_base}") \ | |
| | grep -q .; then | |
| # Pre-existing only when the round's failing set is a SUBSET of the | |
| # baseline's (the round added nothing new): | |
| if [[ -z "${sig_head}" || -z "${sig_base}" ]] || | |
| ! comm -23 <(printf '%s\n' "${sig_head}") <(printf '%s\n' "${sig_base}") \ | |
| | grep -q .; then |
中文说明
[Critical] “pre-existing” 判定只看任意一条共享签名：comm -12 交集非空即 preexisting=true。因此只要本轮引入的失败与基线共享一条归一化诊断（文件 + 错误码 + 消息，位置已被剥离），整轮失败就会被误判为 pre-existing——repair 被跳过，报告给出错误处置。— 故障场景：本轮修复了一个既有错误，又在另一位置引入一个 文件+错误码+消息 完全相同的不同缺陷（或旧错误未除又新增同款）。sig_head ∩ sig_base ≠ ∅ → preexisting=true、retryable 未置位 → 本可修复该缺陷的 repair 阶段被跳过，本轮提交被丢弃；报告还误导人类去 "merge main / 检查 round 前代码"。已用真实脚本探针复现：head = 基线 5 条诊断 + 5 条本轮新增 TS8888 → preexisting=true 且无 retryable；改为子集规则后翻转为 retryable=true。修复方向：仅当本轮失败集是基线失败集的子集（本轮未新增任何诊断）时才判 pre-existing。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
| command -v docker > /dev/null || exit 0 | ||
| STALE="$(docker ps -aq --filter 'name=qwen-code-')" |
There was a problem hiding this comment.
[Suggestion] The same 13-line "Remove stale sandbox containers" block is copied verbatim into both sandboxed jobs (issue-autofix and review-address), and the new test expect(workflow.split(step).length - 1).toBe(2) locks the duplication in — though the repo already has the reuse mechanism for exactly this (composite actions under .github/actions/). — Concrete cost: a future change (an extra name filter, a --filter status=exited refinement, a new image prefix) applied to one job only silently diverges the two sandboxed jobs — the precise class of leftover-container wedge this step exists to prevent, since one job's leak wedges the other's name counter.
| command -v docker > /dev/null || exit 0 | |
| STALE="$(docker ps -aq --filter 'name=qwen-code-')" | |
| - name: 'Remove stale sandbox containers' | |
| uses: './.github/actions/reap-stale-sandbox-containers' |
中文说明
[Suggestion] 这 13 行 "Remove stale sandbox containers" 步骤在两个沙箱 job（issue-autofix 与 review-address）中被逐字复制，新测试 expect(workflow.split(step).length - 1).toBe(2) 还把这份重复钉死——而本仓库已有现成复用机制（.github/actions/ 下的 composite action）。— 具体代价：未来任何改动（新增名称过滤、--filter status=exited 精化、新镜像前缀）只改一个 job 时，两个沙箱 job 会悄然分叉——这正是本步骤要防的“残留容器卡死命名计数器”那一类问题。建议抽成 composite action 后 uses: 引用。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round). Valid: the block is duplicated verbatim and the test pins the duplication. With the reap filter changed this round, extracting the composite action is more valuable, not less — queued so both jobs move together and the test's toBe(2) becomes a single-action assertion.
中文说明
暂缓（预算轮）。成立：该块被逐字复制，且测试把复制钉死。本轮收割过滤器刚改动，抽取 composite action 的价值只增不减——已列入计划，使两个 job 同步迁移，测试的 toBe(2) 改为对单一 action 的断言。
| if [ -n "${STALE}" ]; then | ||
| echo "removing stale sandbox containers:" | ||
| docker ps -a --filter 'name=qwen-code-' --format ' {{.Names}} ({{.Status}})' | ||
| printf '%s\n' "${STALE}" | xargs -r docker rm -f > /dev/null |
There was a problem hiding this comment.
[Suggestion] The reaper lives only at job start, but the leak is caused by the agent-kill path — and this PR's own idle watchdog is now the most frequent killer. The sandbox container is started with --rm, which fires only when the container's main process exits; killing the host-side docker client (exactly what killQwen does to the CLI's process group) orphans the container. — Concrete cost: on a wedged leg the idle watchdog kills the CLI at 20 min, and the container — holding the workspace bind-mount, the daemon, and the API-key env — keeps running on the persistent runner until the NEXT job's start (tens of minutes to hours later), during which the leak this PR describes is still happening. The name IS recoverable at kill time: the CLI prints ContainerName (regular): qwen-code-… to stderr, captured in agent.log/outputTail.
| printf '%s\n' "${STALE}" | xargs -r docker rm -f > /dev/null | |
| # At kill time in run-agent.mjs (idle/absolute/loop-guard paths): | |
| # extract the container name from the captured output and | |
| # execFileSync('docker', ['rm', '-f', name]) — best-effort, wrapped so it | |
| # can never mask the verdict. Keep the job-start reaper as belt-and- | |
| # suspenders for hard kills (job timeout) no process can catch. |
中文说明
[Suggestion] 收割步骤只在 job 开始时运行，但泄漏的制造者是 agent 杀进程路径——而本 PR 新增的空闲看门狗恰恰成了最频繁的杀手。沙箱容器以 --rm 启动，只有容器主进程退出时才会触发清理；杀掉宿主侧 docker 客户端（killQwen 对 CLI 进程组所做的正是这个）会让容器成为孤儿。— 具体代价：wedged leg 在 20 分钟被看门狗杀掉后，容器（持有工作区 bind-mount、daemon 与 API-key 环境变量）会一直运行到下一次 job 开始（数十分钟到数小时），本 PR 所描述的泄漏在此期间依旧存在。名字在杀进程时是可获取的：CLI 向 stderr 打印 ContainerName (regular): qwen-code-…，已进入 agent.log/outputTail。建议在 run-agent.mjs 的各杀进程路径按名 docker rm -f（best-effort，不得掩盖判定），job 开头收割保留作兜底。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round), now promoted: with this round's reap restricted to exited/dead containers (it must not touch running ones on the shared daemon), the kill paths in run-agent.mjs become the designated place to reclaim RUNNING orphans — best-effort docker rm -f on the container name parsed from the captured ContainerName (regular): … output, wrapped so it can never mask the verdict. Queued first with the run-agent batch.
中文说明
暂缓（预算轮），优先级提升：本轮把收割限制为仅 exited/dead 容器后（共享守护进程上不得触碰运行中容器），run-agent.mjs 的杀进程路径成为回收运行中孤儿容器的指定位置——从捕获的 ContainerName (regular): … 输出解析容器名做 best-effort docker rm -f，并包裹以确保永不掩盖判定。已与 run-agent 批次一起列为首要事项。
There was a problem hiding this comment.
[Critical] Revalidated on head 51e9034004bdf7c28652d4381ce36a2d66772b28: the latest safety fix makes this a blocking functional gap. The workflow and its new test both state that a budget/idle kill reaps only the host-side Docker client and that the sandbox container keeps running. The startup reaper now selects only status=exited and status=dead, so that exact running orphan does not match on the next job either; it remains indefinitely, still occupying a qwen-code-* name. In other words, the filter avoids killing a concurrent job safely, but it no longer implements the leak-reaping mitigation advertised by this PR. Please make ownership unambiguous and remove the container owned by this agent in the timeout kill path (or use an equivalent labeled-owner mechanism), while keeping unrelated running containers safe, and add a behavioral test for a running orphan. Re-expanding the startup filter to all running containers would reintroduce the shared-daemon race from the earlier Critical.
| if [[ -z "${sig_head}" || -z "${sig_base}" ]] || | ||
| ! comm -12 <(printf '%s\n' "${sig_head}") <(printf '%s\n' "${sig_base}") \ |
There was a problem hiding this comment.
[Suggestion] The empty-signature fail-closed branch is never exercised: the stub npm prints a tsc-style diagnostic on every staged failure, so sig_head/sig_base are non-empty in every A/B test — the documented "KNOWN LIMIT" (vite/esbuild failures → empty signature → deliberately charged to the round) has no test in either direction. — Concrete cost: a mutation that relaxes the signature regex, or makes empty signatures compare as "matching", would mislabel a round-caused vite failure as pre-existing — skipping the only repair that could fix it and posting a wrong "base update needed" comment; a stricter regex would recreate the 18-minute repair waste this PR exists to eliminate.
中文说明
[Suggestion] 空签名 fail-closed 分支从未被测试覆盖：桩 npm 在每次 staged 失败时都打印 tsc 风格诊断，因此所有 A/B 测试里 sig_head/sig_base 都非空——注释中记载的 "KNOWN LIMIT"（vite/esbuild 失败 → 空签名 → 故意归责本轮）两个方向都没有测试。— 具体代价：若未来放宽签名正则、或让空签名按“匹配”比较，本轮导致的 vite 失败会被误判 pre-existing——跳过唯一能修复它的 repair 并发布错误的 "base update needed" 评论；若收紧正则，则会把每次 pre-existing 失败都归责本轮，重新制造本 PR 要消灭的 18 分钟 repair 浪费。建议补一个只输出非 tsc 标记（无 error TS 行）的 runGate 变体，断言 retryable=true 且无 preexisting=true。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round). Valid: the empty-signature fail-closed branch (vite/esbuild shapes) has no test in either direction. Queued: a runGate variant whose stub emits no error TS line, asserting retryable=true without preexisting=true.
中文说明
暂缓（预算轮）。成立：空签名 fail-closed 分支（vite/esbuild 形态）两个方向都没有测试。已列入计划：新增桩不输出任何 error TS 行的 runGate 变体，断言 retryable=true 且无 preexisting=true。
| if [[ "${PREEXISTING}" == 'true' ]]; then | ||
| echo "preexisting=true" >> "${GITHUB_OUTPUT}" | ||
| fi |
There was a problem hiding this comment.
[Suggestion] The new workflow tests pin the env wiring and report clauses for the preexisting flag — but not the line that actually PRODUCES final_verify.outputs.preexisting (this guarded echo). Deleting this echo (or its guard) passes all 16 new tests. — Concrete cost: if it is dropped, every pre-existing rejection renders the generic "the verification gate rejected the attempt" clause instead of the honest "PRE-EXISTING failure … the branch needs a base update (merge main)" — the operator's only cue to merge main disappears, silently.
| if [[ "${PREEXISTING}" == 'true' ]]; then | |
| echo "preexisting=true" >> "${GITHUB_OUTPUT}" | |
| fi | |
| if [[ "${PREEXISTING}" == 'true' ]]; then | |
| echo "preexisting=true" >> "${GITHUB_OUTPUT}" | |
| fi |
中文说明
[Suggestion] 新加的 workflow 测试钉住了 preexisting 标志的 env 接线与报告分句，却唯独没钉住真正产出 final_verify.outputs.preexisting 的这行受保护的 echo。删除这行（或它的守卫）后全部 16 个新测试依然通过。— 具体代价：一旦被删，所有 pre-existing 拒绝都会退化为通用措辞 "the verification gate rejected the attempt"，而不会出现诚实的 "PRE-EXISTING failure … the branch needs a base update (merge main)"——运维者“去 merge main”的唯一提示会悄然消失。建议把现有 flag 选择正则测试延伸覆盖到这行 guard 与 echo。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round). Valid: the guarded echo that produces final_verify.outputs.preexisting is unpinned — deleting it passes all current tests and silently drops the "merge main" cue. Queued: extend the flag-selection regex test to cover the guard and the echo.
中文说明
暂缓（预算轮）。成立：产出 final_verify.outputs.preexisting 的受保护 echo 未被钉住——删除它后现有测试全部通过，"去 merge main" 的提示会悄然消失。已列入计划：将 flag 选择正则测试延伸覆盖该守卫与 echo。
| : result.timedOut | ||
| ? `timeout (${QWEN_TIMEOUT_MS}ms)` | ||
| : result.idleTimedOut | ||
| ? `idle-timeout (no output for ${QWEN_IDLE_TIMEOUT_MS}ms — the sandbox likely hung at startup)` |
There was a problem hiding this comment.
[Suggestion] The new idle-timeout marker flows into the failure report's timeout branch, which for the hang case advises raising the time budget — the wrong knob, contradicting the design goal stated in the code comment and test. CAUSE="ran out of time before finishing (…)" with LAST_FIX="…split the PR or raise the agent time budget…", and the TIMEOUT_WINDOW_CAP census counts the hang as "time-budget exhaustion" whose headline tells the operator to raise the budget. — Concrete cost: when the idle watchdog fires, the failure comment tells the operator to raise the time budget — for a wedged sandbox, raising the budget changes nothing (the hang burns whatever budget is set); the census can stop a PR early based on a diagnosis that misattributes hang cost to budget size.
中文说明
[Suggestion] 新的 idle-timeout 标记会流入失败报告的超时分支，而该分支针对挂起场景给出的建议是调大时间预算——旋钮拧错，与代码注释和测试声明的设计目标相悖。CAUSE="ran out of time before finishing (…)"、LAST_FIX="…split the PR or raise the agent time budget…"，TIMEOUT_WINDOW_CAP 普查还把挂起计为“time-budget exhaustion”，标题建议提高预算。— 具体代价：看门狗触发后，失败评论让运维去调大时间预算——对 wedged 沙箱而言调大预算毫无作用（挂起会烧掉任何预算）；普查还可能基于把挂起成本错算为预算不足的诊断提前叫停 PR。建议报告步骤按标记内容分支：AGENT_TIMEOUT 以 idle-timeout 开头时给出挂起专用处置并豁免预算建议。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round). Valid: the idle-timeout marker flows into the timeout branch whose advice (raise the budget) cannot cure a wedge. Queued together with the census finding above as one idle-class disposition: branch the report step on the marker prefix, with wedge-specific handling and no budget-raise advice.
中文说明
暂缓（预算轮）。成立：idle-timeout 标记流入超时分支，而该分支的建议（调大预算）无法修复挂死。已与上面的计数发现合并为一个空闲类别处置列入计划：报告步骤按标记前缀分支，给出挂死专用处置且不再建议调大预算。
| baseline_also_fails() { | ||
| # A deterministic rejection is only chargeable to this round if the same |
There was a problem hiding this comment.
[Suggestion] The PR is titled and described as "Two mitigations" for the sandbox hang, but roughly half the diff (~350 of +732 lines: baseline_also_fails, fail_signature, run_check_no_ab, the FIRST_PREEXISTING/REPAIR_PREEXISTING plumbing, and the report clause) is a rework of the review-verification gate that is absent from the description. — Concrete cost: the A/B rework changes observable autofix-loop behaviour — a pre-existing failure now emits preexisting=true and suppresses retryable=true, which changes whether the repair step runs and swaps the failure report's clause. A reviewer or merger reading the description cannot evaluate or even discover this gate behaviour change; the gate's retry semantics ship undisclosed under a sandbox-hang title.
中文说明
[Suggestion] PR 标题与描述只讲“两项缓解”沙箱挂起，但 diff 约一半（+732 行中的约 350 行：baseline_also_fails、fail_signature、run_check_no_ab、FIRST_PREEXISTING/REPAIR_PREEXISTING 接线与报告分句）是对评审验证门禁的重构，而描述对此只字未提。— 具体代价：A/B 重构改变了 autofix 循环可观察行为——pre-existing 失败现在输出 preexisting=true 并抑制 retryable=true，改变 repair 是否运行并切换失败报告措辞。只读描述的人无从评估甚至无从发现这次门禁行为变更；门禁重试语义在“沙箱挂起”标题下未披露地合入。建议在描述中增加 "What else this PR does" 一节，或拆成独立 PR。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred — not actionable from this round: the gate rework is real and should be disclosed in the PR description, but editing the PR body is a GitHub write this headless round cannot perform (the workflow owns PR creation/updates). Flagged here so a maintainer (or the workflow's PR-body path) adds a "What else this PR does" section covering the baseline A/B gate, the preexisting plumbing, and the changed retry/report semantics before merge.
中文说明
暂缓——本轮无法执行：门禁重构确实存在且应在 PR 描述中披露，但修改 PR 正文属于 GitHub 写操作，本无头轮次无权执行（PR 的创建/更新由 workflow 负责）。在此标记，请维护者（或 workflow 的 PR 正文路径）在合入前补充 "What else this PR does" 一节，说明基线 A/B 门禁、preexisting 接线以及变更后的重试/报告语义。
| 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 |
There was a problem hiding this comment.
[Suggestion] For a pre-existing verdict, the fixed-size evidence window (tail_budget = 3300 − preamble, ~2812 bytes) must hold BOTH the round's failing output AND this appended 1500-byte baseline tail — so a verbose failing-check log pushes the matched diagnostic entirely out of the rendered document, and the "with a matching failure signature" claim is backed by no visible diagnostic. — Failure scenario: any A/B-eligible check whose failure output is longer than ~1.3 KB with the tsc diagnostic not in its final ~1.2 KB (a monorepo tsc -b build where the matched error is in an early project), with a matching-signature baseline leg. Probe (real script, stub npm printing the diagnostic then ~11 KB noise on both legs): gate-rejection.md contained 0 occurrences of the matched diagnostic while the preamble asserted the match; rendering the comm -12 intersection instead flipped occurrences 0→1. The human and the next round's LAST_REJECTION must trust an unverifiable label; if the one matched line was itself a partial-overlap coincidence, the skipped repair is invisible in the evidence.
| tail -c 1500 "${ab_log}" >> "${GATE_LOG}" 2> /dev/null || true | |
| # Render the matched signature lines as evidence (the common | |
| # `file: error TS####: msg` lines) instead of the raw 1500-byte tail: | |
| comm -12 <(printf '%s\n' "${sig_head}") <(printf '%s\n' "${sig_base}") \ | |
| >> "${GATE_LOG}" 2> /dev/null || true |
中文说明
[Suggestion] 对 pre-existing 判定而言，固定大小的证据窗口（tail_budget = 3300 − preamble，约 2812 字节）必须同时容纳本轮失败输出和这里追加的 1500 字节基线尾部——因此冗长的失败日志会把被匹配的诊断整个挤出渲染文档，“with a matching failure signature”的说法背后没有任何可见诊断。— 故障场景：任一 A/B 候选检查失败输出超过约 1.3 KB 且 tsc 诊断不在其最后约 1.2 KB 内（monorepo tsc -b 中匹配错误出现在较早 project），且基线 leg 签名匹配。探针（真实脚本，桩 npm 打印诊断后跟约 11 KB 噪音）：gate-rejection.md 中匹配诊断出现 0 次，而 preamble 断言匹配存在；改为渲染 comm -12 交集后 0→1 翻转。人类读者与下一轮 LAST_REJECTION 只能信任无法核验的标签；若那条匹配行本身只是部分重叠巧合，被跳过的 repair 在证据中完全不可见。建议把匹配的签名行本身渲染进窗口，或补充冗长日志夹具。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round). Valid: on verbose failing logs the matched diagnostic can be pushed out of the fixed-size evidence window while the preamble still asserts the match. Queued: render the comm -12 intersection as the evidence for a pre-existing verdict (or add the verbose-log fixture), so the claim is always visible in the document.
中文说明
暂缓（预算轮）。成立：冗长失败日志下，被匹配的诊断可能被挤出固定大小的证据窗口，而 preamble 仍断言匹配存在。已列入计划：pre-existing 判定改为渲染 comm -12 交集作为证据（或补充冗长日志夹具），使该论断在文档中始终可见。
| if [[ "${PREEXISTING}" == 'true' ]]; then | ||
| if [[ "${CMP_R:-}" == 'behind' || "${CMP_R:-}" == 'diverged' ]]; then |
There was a problem hiding this comment.
[Suggestion] A pre-existing gate rejection is counted by the consecutive-failure circuit breaker (CONSEC_FAIL) even though it is the same "not the round's fault" class the breaker's own comment exempts — at the cap the breaker replaces the pre-existing headline with "PR is too large or conflicts with a fast-moving main; rebase, split, reduce" and stamps MARK_ROUND=MAX_ROUNDS, skipping the PR forever. — Failure scenario: a PR whose pre-round state is broken (the exact #8614 class this PR targets): each addressed item rejects pre-existing (watermark advances, item dropped, no repair), so several items in a row each burn a full agent round and increment CONSEC_FAIL (nothing in the preexisting path resets it). At the cap the breaker overwrites the pre-existing remedy ("merge main" / "pre-round code needs attention") with "rebase, split, or reduce" — which cannot cure a broken pre-round commit — and the terminal marker makes future scans skip the PR, contradicting the pre-existing headline's promise that "the loop stays engaged". Probe (verbatim breaker block, 5 pre-existing rejections, cap=5): MARK_ROUND=10, CONSEC_FAIL=5, headline replaced.
| if [[ "${PREEXISTING}" == 'true' ]]; then | |
| if [[ "${CMP_R:-}" == 'behind' || "${CMP_R:-}" == 'diverged' ]]; then | |
| # Exempt pre-existing rejections from the breaker — or have the | |
| # pre-existing branch declare the terminal state deliberately with its | |
| # own headline: | |
| [[ "${PREEXISTING:-}" != 'true' ]] && … |
中文说明
[Suggestion] pre-existing 门禁拒绝会被计入连续失败熔断器（CONSEC_FAIL），尽管它与熔断器注释中豁免的"非本轮过错"类别同质——达到上限后熔断器会用 "PR is too large or conflicts with a fast-moving main; rebase, split, reduce" 覆盖 pre-existing 头条并打上 MARK_ROUND=MAX_ROUNDS，从此永久跳过该 PR。— 故障场景：PR 的 round 前状态已坏（正是本 PR 针对的 #8614 类）：每处理一个 item 都 pre-existing 拒绝（watermark 前进、item 丢弃、无 repair），连续多轮各烧满一轮 agent 时间并累加 CONSEC_FAIL（pre-existing 路径没有任何重置）。到上限后熔断器把 pre-existing 处置（"merge main" / "pre-round code needs attention"）覆盖成 "rebase, split, or reduce"——对一个已坏的 pre-round 提交毫无作用——终态标记还让后续扫描跳过 PR，与 pre-existing 头条"loop stays engaged"的承诺相悖。探针（逐字熔断块，5 次 pre-existing，cap=5）：MARK_ROUND=10、CONSEC_FAIL=5、头条被替换。建议在熔断条件中豁免 PREEXISTING，或让 pre-existing 分支以自有头条显式宣告终态。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred (budget round). Valid: pre-existing rejections increment CONSEC_FAIL and at the cap get their headline replaced by rebase/split advice that cannot cure a broken pre-round state. Behavior change with a design edge — an exemption must not let a broken pre-round branch burn unbounded rounds — so it is queued with an explicit terminal state for the pre-existing class (own headline + deliberate MARK_ROUND handling) rather than a bare exemption.
中文说明
暂缓（预算轮）。成立：pre-existing 拒绝会计入 CONSEC_FAIL，达上限后其头条被 rebase/拆分建议覆盖，而后者无法修复已坏的轮前状态。这是带设计边界的行为变更——豁免不能让已坏的轮前分支无限烧轮——因此列入计划时为 pre-existing 类别设计显式终态（自有头条 + 明确的 MARK_ROUND 处理），而非简单豁免。
|
🤖 AutoFix ran out of time before finishing (timeout (7200000ms)) (attempt 1/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31327011711 🧠 Handled by Qwen Code · model/模型 |
|
🤖 AutoFix ran out of time before finishing (timeout (7200000ms)) (attempt 2/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31333022318 🧠 Handled by Qwen Code · model/模型 |
…ive kill
Budget-warning round — the five Criticals from both reviewers, no
suggestions (each deferred with a recorded reply).
fail_signature: `[^\n]*` in an ERE bracket expression does not mean
"rest of line" — in POSIX bracket expressions `\` is literal, so it
matched "neither backslash nor the letter n" and truncated every tsc
message at its first n. Nearly every real message has an early n
("Cannot find name", "is not assignable"), so distinct same-file
failures collapsed into identical signatures and a round-caused failure
could be labeled pre-existing, skipping the repair. grep is
line-oriented: `.*` is exactly the rest of the line. New fixture: two
messages differing only after their first n.
Pre-existing verdict: the intersection test mislabeled in both
directions. A round that ADDS a diagnostic sharing one normalized line
with the baseline was called pre-existing (repair skipped for a
round-caused, repairable failure); and `comm -12 | grep -q` under
`set -eo pipefail` SIGPIPEs comm (exit 141) once the shared output
outruns the pipe buffer, charging true pre-existing failures to the
round — the exact 18-minute repair waste the gate exists to kill.
Pre-existing now means the round's failing set is a SUBSET of the
baseline's, and the difference is captured before testing. New fixture:
a round adding a second diagnostic to a failing baseline.
Restore failure after the baseline leg: was retryable=true with HEAD
still detached at the baseline commit — the repair agent works in that
very checkout and does no git recovery, so its commit would land on the
baseline and be orphaned. Now rejected non-retryable (reject_fix grows
a third arg); the next round starts clean from the trusted checkout.
The restoreClash test pins the new semantics.
Stale-container reap: the premise "a runner runs one job at a time, so
any live qwen-code-* container is stale" holds per runner registration,
but the filter queries the docker daemon, which is per host — and this
pool runs several registrations on one OS. With per-issue/PR
serialization only, a concurrent job's sandbox is a substring match
away from `docker rm -f`. The reap now takes only provably-dead
containers (--filter status=exited/dead, both jobs) and the comment
says why a running one is left alone.
Preamble printf: the `\`` escapes sat inside a single-quoted format
where backslash is literal, so every pre-existing rejection rendered
raw backticks instead of code spans (shellcheck SC2016). Backticks
need no escaping there. Also syncs the side-log comment to the dynamic
tail_budget it actually renders.
Verified: scripts suite 140/140 (was 138; the two new fixtures and the
rewritten restoreClash test all fail against the pre-fix script),
npm run build / typecheck / lint pass, bash -n clean.
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： AutoFix review round 5 — summaryBudget warning in effect (the two previous rounds exhausted the agent time budget), so this round took the blocking Critical subset only, verified it, and committed it; every Suggestion is deferred with a recorded reply on its own thread ( Commit: Resolved (Criticals + one adjacent comment fix)
Deferred (Suggestions — replies posted on each thread)16 findings deferred under the budget warning: the run-agent batch (idle guard No VerificationCommands actually run on this runner, in order:
中文说明AutoFix 评审第 5 轮 — 总结预算警告生效中（前两轮均耗尽 agent 时间预算），因此本轮只处理阻断性 Critical 子集，验证后提交；每条 Suggestion 都在其各自线程中以记录在案的理由暂缓（见 提交： 已解决（Critical + 一处相邻注释修复）
暂缓（Suggestions — 已在各自线程回复）共 16 条发现依预算警告暂缓：run-agent 批次（空闲守卫补查 无 验证本 runner 上实际执行的命令（按顺序）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Round-6 verification at head 51e9034. First the good: all five prior Criticals are confirmed fixed at head (bracket-class regex with regression test, non-retryable restore-failure verdict, exited/dead-only reap filter, comm -23 with full capture, subset semantics with the added-diagnostic test), the watchdog semantics check out (lastOutputAt refreshed by both stdout and stderr, process-group SIGTERM→SIGKILL with ESRCH-safe fallback, all three timers cleared in finish(), no misreport race on normal completion, 20-min default is 2× the fleet's longest tolerated quiet and a false fire only costs a retryable round), and the A/B gate rework is fail-closed throughout with genuinely behavioral tests (+511 lines executing the real artifacts). The deferred suggestions are dispositioned with rationale and I verified the deferral claims.
One new P2 that blocks, not raised in prior rounds: the reap step can fail an entire round at setup. Under the default bash -eo pipefail, printf '%s\n' "${STALE}" | xargs -r docker rm -f exits nonzero when two jobs on different registrations of the same host list the same stale container and the second rm hits 'No such container' (or any transient daemon error like 'removal already in progress') — killing the step before any real work, which is the exact failure class this PR exists to eliminate; the window is widest at first deploy when dirty hosts still hold residue. Likewise STALE="$(docker ps -aq …)" aborts the step when the daemon is unreachable — the command -v docker || exit 0 guard only covers docker being absent. This is pure hygiene, one line per copy (qwen-autofix.yml:846 and :3513): printf '%s\n' "${STALE}" | xargs -r docker rm -f >/dev/null 2>&1 || true, and || true (or an explicit daemon check) on the listing. Minor adjacent note: the name filter is a substring match and also catches qwen-code-integration-test-* containers — harmless since those are exited+--rm, but worth knowing.
Also noted for the record (already dispositioned, not blocking): the reap deliberately excludes running containers so the client-killed orphans this PR's own watchdog creates still need the queued kill-time docker rm -f; and the QWEN_IDLE_TIMEOUT_MS parse admits negative/0 with zero live exposure today. One line per copy away from approve.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the complete 4-file diff at exact head 51e9034004bdf7c28652d4381ce36a2d66772b28, including existing review threads, workflow ownership and failure paths, all new preexisting and idle-timeout read/write sites, the sandbox launcher, and current checks.
The latest commit fixes the earlier signature parsing, pipefail, restore-state, subset-comparison, and cross-registration live-container deletion Criticals. One Critical remains: the stale-container mitigation now filters to exited/dead containers even though the failure mode leaves the sandbox running, so the target orphan is never reaped. Details and the required ownership-safe direction are in #8816 (comment).
Verification on this head: the focused review-gate/watchdog/reaper run passed 18 tests; node --check and bash -n passed. CI Test, Desktop Shell, and web-shell smoke checks are green. Requesting changes only for the blocking orphan-cleanup gap; previously recorded Suggestions remain non-blocking under the repository review-round policy.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
[Critical] .github/scripts/run-autofix-review-verification.sh:70 SC2016 — Expressions don't expand in single quotes, use double quotes for that. [lint]
[Critical] .github/scripts/run-autofix-review-verification.sh:70 SC2016 — Expressions don't expand in single quotes, use double quotes for that. [lint]
中文说明
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
[Critical] .github/scripts/run-autofix-review-verification.sh:70 SC2016 — Expressions don't expand in single quotes, use double quotes for that. [lint]
[Critical] .github/scripts/run-autofix-review-verification.sh:70 SC2016 — Expressions don't expand in single quotes, use double quotes for that. [lint]
— qwen3.8-max via Qwen Code /review (v0.21.8)
| # only provably-dead containers (exited/dead), before the sandbox | ||
| # picks a name (and before the leftovers can wedge the daemon). | ||
| command -v docker > /dev/null || exit 0 | ||
| STALE="$(docker ps -aq --filter 'name=qwen-code-' --filter 'status=exited' --filter 'status=dead')" |
There was a problem hiding this comment.
[Suggestion] R2-1: the "Remove stale sandbox containers" step runs fallible docker commands with no error tolerance under this workflow's bash -eo pipefail, so a docker failure at this best-effort hygiene step hard-fails the whole autofix round. The concurrent-reap race trigger was probe-refuted on docker 29.1.3 (docker rm -f on a vanished container exits 0); two triggers survive verification. — Failure scenario: (1) docker CLI present but daemon unreachable → STALE="$(docker ps -aq ...)" carries the nonzero status, bash -e aborts the step, and the round dies at cleanup with a raw docker error instead of at the purpose-built daemon check a few steps later — a transient blip kills a round that would have recovered; (2) a dead container whose removal fails (storage-driver error — the exact pathology this step's comment worries about) reappears in every listing, so every round on that host fails at this step until manual intervention, whereas before this PR the name counter simply worked around it. The sibling best-effort steps (Reset autofix workspace, Drop stale autofix branches) all end in || true; this is the only one without tolerance. Apply in both job copies.
STALE="$(docker ps -aq --filter 'name=qwen-code-' --filter 'status=exited' --filter 'status=dead' 2>/dev/null)" || STALE=''
# and make the removal line tolerant too:
printf '%s\n' "${STALE}" | xargs -r docker rm -f > /dev/null 2>&1 || true中文说明
[Suggestion] R2-1："Remove stale sandbox containers" 步骤在本 workflow 的 bash -eo pipefail 下运行可能失败的 docker 命令却没有任何容错，这个尽力而为的清理步骤一旦失败就会使整个 autofix 轮次硬失败。并发收割竞争触发已被 docker 29.1.3 实测探针否定（docker rm -f 对已消失的容器退出码为 0）；验证后存活的触发有两个。— 故障场景：(1) docker CLI 存在但守护进程不可达 → STALE="$(docker ps -aq ...)" 携带非零退出状态，bash -e 中止该步骤，轮次以裸 docker 错误死在清理步骤，而不是死在几步之后专门准备的守护进程检查上——一次瞬时抖动就会杀掉本可恢复的轮次；(2) 一个删除失败的 dead 容器（存储驱动错误——正是本步骤注释担心的病症）会出现在每次列举中，该主机上的每一轮都会在此步骤失败直到人工介入，而本 PR 之前命名计数器会直接绕过它。同类的尽力而为步骤（Reset autofix workspace、Drop stale autofix branches）都以 || true 收尾；唯本步骤没有容错。两个 job 副本都需修改。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const job = (id) => { | ||
| const start = workflow.indexOf(`\n ${id}:`); |
There was a problem hiding this comment.
[Suggestion] R2-2: this local job(id) slicer duplicates the established shared helper getWorkflowJob in scripts/tests/workflow-helpers.js — a probe against the real workflow shows the two return byte-identical slices for issue-autofix and review-address (the same two jobs that package-scripts.test.js already slices via the shared helper, ~23 uses across the suite). — Concrete cost: two job-slicing implementations now drift independently; if the shared helper is hardened (e.g., against a marker matching inside a comment), this test silently keeps the weaker behaviour, and future tests in this file face a fork over which slicer to use.
// import { getWorkflowJob } from './workflow-helpers.js'; (top of file)
const job = (id) => getWorkflowJob(workflow, id);中文说明
[Suggestion] R2-2：此处的局部 job(id) 切片器复制了已有的共享辅助函数 scripts/tests/workflow-helpers.js 中的 getWorkflowJob——对真实 workflow 的探针显示，两者对 issue-autofix 与 review-address 返回逐字节相同的切片（正是 package-scripts.test.js 已经通过共享辅助函数切片的同两个 job，整套测试约 23 处使用）。— 具体代价：两份 job 切片实现从此独立漂移；若共享辅助函数被加固（例如防止标记在注释内误匹配），本测试会静默保留较弱的行为，本文件后续测试也要面临选择哪个切片器的分叉。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| Math.min(30_000, Math.floor(QWEN_IDLE_TIMEOUT_MS / 4)), | ||
| ); | ||
| idleTimer = setInterval(() => { | ||
| if (settled || idleTimedOut) return; |
There was a problem hiding this comment.
[Suggestion] R1-5: the idle guard checks settled || idleTimedOut but not timedOut — after the absolute budget fired first, a later idle tick during the SIGTERM grace can overwrite the classification, because the failure-detail chain checks idleTimedOut before timedOut. Re-checked at this commit: still stands (the anchored code is unchanged since the round-1 comment; the recorded deferral reply has not landed). — Failure scenario: with QWEN_IDLE_TIMEOUT_MS configured at or just above QWEN_TIMEOUT_MS, the absolute timer fires first, a grace-period tick then sets idleTimedOut, and the failure comment names the wrong limit — the operator tunes the wrong knob. Fix: if (settled || timedOut || idleTimedOut) return;
中文说明
[Suggestion] R1-5：空闲守卫检查 settled || idleTimedOut 但不检查 timedOut——绝对预算先触发后，SIGTERM 宽限期内的后续空闲 tick 可能覆盖分类，因为失败详情链先检查 idleTimedOut 再检查 timedOut。在本提交复查：仍然成立（锚定代码自第 1 轮评论以来未变；记录在案的暂缓回复尚未落地）。— 故障场景：QWEN_IDLE_TIMEOUT_MS 配置为等于或略高于 QWEN_TIMEOUT_MS 时，绝对计时器先触发，宽限期内的 tick 随后置位 idleTimedOut，失败评论指向错误的限制——运维会调错旋钮。修复：if (settled || timedOut || idleTimedOut) return;
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // contexts, so twice that is the default. Distinct from QWEN_TIMEOUT_MS so | ||
| // the failure comment says which limit fired. | ||
| const QWEN_IDLE_TIMEOUT_MS = | ||
| Number(process.env.QWEN_IDLE_TIMEOUT_MS) || 20 * 60 * 1000; |
There was a problem hiding this comment.
[Suggestion] R1-6: the new knob's parse is unguarded — Number('-1') is -1 (truthy), so a NEGATIVE value passes the || default guard, and a MINUTES-shaped value like 20 passes too, becoming a 20 ms window. Re-checked at this commit: still stands (anchored code unchanged; deferral reply not landed). — Failure scenario: QWEN_IDLE_TIMEOUT_MS=-1 or =20 in the operator env → the watchdog window is negative or 20 ms and every agent is killed almost immediately as "idle". Fix: const parsed = Number(process.env.QWEN_IDLE_TIMEOUT_MS); const QWEN_IDLE_TIMEOUT_MS = Number.isFinite(parsed) && parsed > 0 ? parsed : 20 * 60 * 1000;
中文说明
[Suggestion] R1-6：新旋钮的解析无防护——Number('-1') 为 -1（真值），因此负值能通过 || default 守卫，20 这种"分钟形态"的值也能通过并变成 20 毫秒窗口。在本提交复查：仍然成立（锚定代码未变；暂缓回复尚未落地）。— 故障场景：运维环境中 QWEN_IDLE_TIMEOUT_MS=-1 或 =20 → 看门狗窗口为负或 20 毫秒，每个 agent 几乎立即被当作"空闲"杀掉。修复：const parsed = Number(process.env.QWEN_IDLE_TIMEOUT_MS); const QWEN_IDLE_TIMEOUT_MS = Number.isFinite(parsed) && parsed > 0 ? parsed : 20 * 60 * 1000;
— qwen3.8-max via Qwen Code /review (v0.21.8)
| killQwen(child, 'SIGTERM'); | ||
| killTimer = setTimeout(() => { | ||
| if (!settled) killQwen(child, 'SIGKILL'); | ||
| }, 10_000); |
There was a problem hiding this comment.
[Suggestion] R1-9: this inlines a verbatim copy of the SIGTERM→10s-grace→SIGKILL escalation that the absolute-timeout timer already contains ~18 lines above in the same runQwen function. Re-checked at this commit: still stands (anchored code unchanged; deferral reply not landed). — Concrete cost: the kill sequence is pinned in two places; a future change to the grace period or escalation must edit both, and editing only one leaves the two kill paths silently divergent. Fix: extract the escalation into one helper used by both timers.
中文说明
[Suggestion] R1-9：此处逐字内联了一份 SIGTERM→10 秒宽限→SIGKILL 升级序列，而同一 runQwen 函数上方约 18 行的绝对超时计时器已包含同样的序列。在本提交复查：仍然成立（锚定代码未变；暂缓回复尚未落地）。— 具体代价：杀进程序列被钉在两个位置；将来修改宽限时长或升级逻辑必须两处同改，只改一处会让两条杀进程路径静默分叉。修复：将升级序列抽成两个计时器共用的辅助函数。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| idleMs: 1200, | ||
| }); | ||
| expect(r.status).not.toBe(0); | ||
| expect(r.failure).toContain('idle-timeout (no output for 1200ms'); |
There was a problem hiding this comment.
[Suggestion] R1-10: this test asserts the failure.md wording but never the agent-timeout sentinel — the one-line mutation deleting timedOut = true; from the idle branch of run-agent.mjs survives the entire 140-test suite (mutant re-verified this round), and the report step's retry condition keys on that sentinel. The sibling absolute-timeout test pins exactly it. Re-checked at this commit: still stands. — Failure scenario: without the sentinel, a wedged-sandbox round writes failure.md but is routed as an evaluated verdict instead of a sentinel-ts retry — the watermark advances past the feedback and the PR is stranded, the exact outcome the watchdog was built to recover from. Fix: assert the agent-timeout file exists in the workdir (expose it from the runAgent helper), mirroring the absolute-timeout test.
中文说明
[Suggestion] R1-10：本测试断言了 failure.md 的措辞，却从未断言 agent-timeout 哨兵——从 run-agent.mjs 空闲分支删除 timedOut = true; 的单行变异能通过全部 140 条测试（本轮已重新做变异验证），而 report 步骤的重试条件正依赖该哨兵。同族的绝对超时测试恰好钉住了它。在本提交复查：仍然成立。— 故障场景：缺少哨兵时，沙箱卡死的轮次虽写出 failure.md，却被当作已评估的结论路由，而不是按哨兵时间戳重试——水位线越过反馈继续前进，PR 被搁浅，恰是看门狗要避免的结局。修复：断言 workdir 中存在 agent-timeout 文件（由 runAgent 辅助函数暴露），与绝对超时测试对齐。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // code — an up-to-date branch cannot be cured by merging main. | ||
| expect(workflow).toContain('needs a base update (merge main)'); | ||
| // The YAML embeds the apostrophe via shell quoting, so match around it. | ||
| expect(workflow).toContain('own pre-round code needs attention'); |
There was a problem hiding this comment.
[Suggestion] R1-11: these assertions check that both GATE_CLAUSE strings and the CMP_R conditional exist in the workflow, but never the CORRESPONDENCE between condition and clause — swapping the two clause bodies survives all of them. Re-checked at this commit: still stands. — Failure scenario: a swapped-clause mutation passes every test, and the posted comment advises "merge main" for an up-to-date branch and "check pre-round code" for a behind one — the wrong remedy in both directions. Fix: extend the assertions into the then/else branches so a swapped-clause mutation fails.
中文说明
[Suggestion] R1-11：这些断言只检查两个 GATE_CLAUSE 字符串与 CMP_R 条件存在于 workflow 中，从未检查条件与子句之间的对应关系——交换两个子句的主体仍能通过全部断言。在本提交复查：仍然成立。— 故障场景：交换子句的变异能通过所有测试，发布的评论会对已是最新的分支建议 "merge main"、对落后的分支建议"检查 round 前代码"——两个方向都给错处置。修复：将断言延伸到 then/else 分支，使交换子句的变异失败。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| 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"'); |
There was a problem hiding this comment.
[Suggestion] R1-12: the evidence-window invariant (finished document ≤ 3900 bytes so the closing fence survives head -c 3900) is pinned only by string-existence checks — no executed fixture ever saturates tail -c "${tail_budget}". Re-checked at this commit: still stands. — Failure scenario: a future change to the preamble size or the 3300/3900 constants can break the closing-fence invariant while every test stays green, malforming everything after the fence in the posted rejection comment. Fix: one runGate scenario whose failing stub emits >4 KB, asserting the rendered document ends with the closing fence.
中文说明
[Suggestion] R1-12：证据窗口不变量（成品文档 ≤ 3900 字节，使收尾围栏在 head -c 3900 下存活）仅由字符串存在性检查钉住——没有任何被执行过的夹具真正填满 tail -c "${tail_budget}"。在本提交复查：仍然成立。— 故障场景：将来改动 preamble 大小或 3300/3900 常量时，收尾围栏不变量可能在所有测试保持绿色的情况下被破坏，使发布的拒绝评论中围栏之后的内容全部畸形。修复：增加一个失败桩输出超过 4 KB 的 runGate 场景，断言渲染出的文档以收尾围栏结尾。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // 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. |
There was a problem hiding this comment.
[Suggestion] R1-13: this design statement — "Distinct from QWEN_TIMEOUT_MS so the failure comment says which limit fired" — silently never applies to the repair leg: the 20-min idle default sits ABOVE the repair pass's hardcoded 18-min absolute budget, and the knob is forwarded nowhere in the workflow. Re-checked at this commit: still stands (anchored code unchanged; deferral reply not landed). — Failure scenario: a wedged repair leg always dies as an absolute timeout at 18 min, never as an idle-timeout, so the diagnostic precision this knob was added for is unavailable exactly where the 18-minute budget is spent. Fix: wire the knob through a vars-backed env on both agent invocations, or scope the comment to the legs where the knob can fire.
中文说明
[Suggestion] R1-13：这条设计陈述——"与 QWEN_TIMEOUT_MS 区分，使失败评论说明是哪个限制触发"——对 repair 分支静默失效：20 分钟的空闲默认值高于 repair 阶段硬编码的 18 分钟绝对预算，且该旋钮在 workflow 中没有任何转发。在本提交复查：仍然成立（锚定代码未变；暂缓回复尚未落地）。— 故障场景：卡死的 repair 分支永远在 18 分钟以绝对超时死亡，从不以 idle-timeout 死亡——该旋钮所提供的诊断精度恰好在花费 18 分钟预算的地方不可用。修复：通过 vars 环境变量把旋钮接入两个 agent 调用，或将注释限定到旋钮能真正触发的分支。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (settled || idleTimedOut) return; | ||
| if (Date.now() - lastOutputAt >= QWEN_IDLE_TIMEOUT_MS) { | ||
| idleTimedOut = true; | ||
| timedOut = true; |
There was a problem hiding this comment.
[Suggestion] R1-14: idle timeouts set timedOut = true, so wedge rounds write the shared agent-timeout sentinel; the report headline matches the cumulative-timeout census needles, and the consecutive-failure breaker trips with raise-the-budget advice that cannot cure a wedge. Re-checked at this commit: still stands (anchored code unchanged; deferral reply not landed). — Failure scenario: repeated startup wedges count toward the cumulative timeout census and trip the terminal breaker with budget advice, even though the correct remedy is the idle knob or a sandbox fix. Fix: branch the report/census on idleTimedOut vs timedOut so idle wedges get their own classification.
中文说明
[Suggestion] R1-14：空闲超时会置 timedOut = true，因此卡死轮次写入共享的 agent-timeout 哨兵；报告标题会命中累计超时统计的匹配串，连续失败熔断器也会带着"提高预算"的建议触发——而该建议治不了卡死。在本提交复查：仍然成立（锚定代码未变；暂缓回复尚未落地）。— 故障场景：反复的启动卡死会计入累计超时统计并触发终端熔断器给出预算建议，尽管正确的处置是空闲旋钮或沙箱修复。修复：让报告/统计按 idleTimedOut 与 timedOut 分支，使空闲卡死拥有独立分类。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Autofix review round 6 — summaryHead after this round: Blocking findings — all fixed
Suggestions — disposition
Adjunct note from @yiliang114 acknowledged, no change: the Verification
中文说明Autofix 评审第 6 轮 — 摘要本轮后的 head： 阻断性发现 — 全部修复
建议 — 处置
@yiliang114 的附带说明确认知悉，不作修改： 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): Context: PR #8816 (QwenLM/qwen-code) — idle watchdog + st...: I did not execute the new vitest blocks (static analysis only; the worktree has no installed node_modules and the suite is 11.9k lines) — all conclusions above …. Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查。 建议见行内评论。 未探索到全部深度（达到工具调用预算）：Context: PR #8816 (QwenLM/qwen-code) — idle watchdog + st...：I did not execute the new vitest blocks (static analysis only; the worktree has no installed node_modules and the suite is 11.9k lines) — all conclusions above …。 未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| command -v docker > /dev/null || exit 0 | ||
| STALE="$(docker ps -aq --filter 'name=qwen-code-' --filter 'status=exited' --filter 'status=dead' 2>/dev/null)" || STALE='' |
There was a problem hiding this comment.
[Suggestion] R3-1: this hygiene step bounds every docker failure mode except a hang — none of its docker calls has a timeout wrapper and the step has no timeout-minutes, so an alive-but-wedged docker daemon blocks the job at setup until the job timeout. — Failure scenario: the daemon is alive but wedged — the exact condition this PR exists to eliminate; the step's own comment cites leftovers that "can wedge the daemon". docker ps -aq against an unresponsive daemon blocks indefinitely, and || STALE='' catches only a nonzero exit, not a hang. Probe against the verbatim step with a blocking docker shim: it hung until the outer cutoff, while a timeout-wrapped variant exited bounded. The round then sits in this step until the job timeout (issue-autofix 180 / review-address 300 min), which per this workflow's own comment cancels the always() reporters — a silent round, reintroduced ahead of the very idle watchdog this PR adds. Applies to both copies (also in review-address, ~line 3544). Suggested fix: wrap each docker invocation, e.g. STALE="$(timeout 30 docker ps -aq ... 2>/dev/null)" || STALE='' and printf '%s\n' "${STALE}" | xargs -r -I{} timeout 30 docker rm -f {} > /dev/null 2>&1 || true (GNU timeout is present on the ubuntu runners).
中文说明
[建议] R3-1：该清理步骤为每一种 docker 失败模式都做了兜底，唯独没有覆盖“挂起”——所有 docker 调用都没有 timeout 包装，步骤本身也没有 timeout-minutes，因此一个“活着但卡死”的 docker 守护进程会让 job 在 setup 阶段一直阻塞到 job 超时。— 故障场景：守护进程活着但卡死——这正是本 PR 要消灭的状态，步骤自己的注释也提到残留容器“可能卡死守护进程”。对无响应的守护进程执行 docker ps -aq 会无限阻塞，而 || STALE='' 只能兜住非零退出，兜不住挂起。已用阻塞型 docker 桩对原样步骤做探针：一直挂到外部截止时间；换成 timeout 包装后则有限退出。于是整轮卡在该步骤直到 job 超时（issue-autofix 180 / review-address 300 分钟），而按本 workflow 自己的注释，job 超时会取消 always() 报告步骤——一个静默轮次，恰好被重新引入在这个 PR 新增的看门狗之前。两份副本同样适用（review-address 约 3544 行）。修复建议：给每个 docker 调用加超时，例如 STALE="$(timeout 30 docker ps -aq ... 2>/dev/null)" || STALE='' 与 printf '%s\n' "${STALE}" | xargs -r -I{} timeout 30 docker rm -f {} > /dev/null 2>&1 || true（ubuntu runner 自带 GNU timeout）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| } | ||
| }); | ||
|
|
||
| it('a budget kill removes only the running sandbox its own agent launched', () => { |
There was a problem hiding this comment.
[Suggestion] R3-3: the test named 'a budget kill removes only the running sandbox its own agent launched' actually exercises only the idle-watchdog kill path, leaving the budget branch's container removal untested. — Failure scenario: the test sets QWEN_IDLE_TIMEOUT_MS=1200 and QWEN_TIMEOUT_MS=60000 with a stub that goes silent after one line, so the idle branch fires at ~1.2s (measured run time 1239ms — the idle timescale, not the 60s budget), and the helper's 30s spawnSync timeout is also below the budget, so the budget path is doubly unreachable. Both kill paths share escalateKill() today, so coverage is coincidental: if a future edit moves the docker rm out of escalateKill() into only one branch, or deletes the escalateKill() call from the absolute-timer callback, this test still passes while the budget-kill container leak silently returns — and the test's name tells readers the budget path is covered. Suggested fix: add a sibling case that triggers the absolute path (omit the idle window, set timeoutMs: 1200, same stub), or rename the test to say "an idle kill".
中文说明
[建议] R3-3：名为“budget kill 只移除自己启动的运行中沙箱”的测试实际只跑了 idle 看门狗这一条杀路径，预算分支的容器移除没有被测试覆盖。— 故障场景：该测试设置 QWEN_IDLE_TIMEOUT_MS=1200、QWEN_TIMEOUT_MS=60000，桩 agent 输出一行后永远沉默，因此 idle 分支约 1.2s 就触发（实测运行 1239ms——是 idle 时间尺度而非 60s 预算），且 helper 的 spawnSync 超时为 30s 也低于预算，预算路径双重不可达。目前两条杀路径共用 escalateKill()，覆盖只是巧合：若未来把 docker rm 移出 escalateKill() 只留在其中一个分支，或从绝对计时器回调中删掉 escalateKill() 调用，本测试仍会通过，而预算杀路径的容器泄漏会悄悄回归——测试名却让读者以为预算路径已被覆盖。修复建议：新增一个触发绝对路径的兄弟用例（不传 idle 窗口、设 timeoutMs: 1200、同样的桩），或把测试名改为“an idle kill”。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| expect(r.failure).toBe(''); | ||
| }); | ||
|
|
||
| it('ignores a non-positive QWEN_IDLE_TIMEOUT_MS instead of arming it', () => { |
There was a problem hiding this comment.
[Suggestion] R3-5: the idle-window parse guard (Number.isFinite && > 0 in run-agent.mjs) is tested only with idleMs: -1; the 0 and NaN rejection classes named in the code comment are unpinned. — Failure scenario: probe-verified, the mutation > 0 → >= 0 keeps the whole suite green. With that mutation shipped, an operator who sets QWEN_IDLE_TIMEOUT_MS=0 (intending "disable") arms a zero-length window: Date.now() - lastOutputAt >= 0 is true at the first tick, so every autofix agent leg is killed almost immediately — the probe showed a healthy talking agent killed at tick 1 with idle-timeout (no output for 0ms — the sandbox likely hung at startup) — burning rounds on instant idle-timeouts. Suggested fix: parameterize the guard test over the documented classes — for (const idleMs of [-1, 0, Number.NaN]) reusing the same talking stub, asserting status 0 and empty failure.
中文说明
[建议] R3-5：空闲窗口解析防护（run-agent.mjs 中的 Number.isFinite && > 0）只用 idleMs: -1 测过；代码注释中点名的 0 与 NaN 拒绝类别没有被钉住。— 故障场景：探针验证过，变异 > 0 → >= 0 后整套测试仍全绿。若带着该变异上线，运维把 QWEN_IDLE_TIMEOUT_MS=0（本意“禁用”）会装载一个零长度窗口：Date.now() - lastOutputAt >= 0 在第一个 tick 即为真，每条 autofix agent 分支几乎立即被杀——探针中一个持续输出的健康 agent 在 tick 1 就被以 idle-timeout (no output for 0ms — the sandbox likely hung at startup) 杀掉——整批轮次被即时空闲超时烧掉。修复建议：把防护测试参数化到注释声明的各类取值——for (const idleMs of [-1, 0, Number.NaN])，复用同一个持续输出的桩，断言 status 0 且 failure 为空。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (sandboxName) { | ||
| const rm = spawnSync('docker', ['rm', '-f', '--', sandboxName], { | ||
| stdio: 'ignore', | ||
| timeout: 30_000, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R3-6: escalateKill's synchronous spawnSync('docker rm -f', { timeout: 30_000 }) runs between the SIGTERM and the queued 10-second SIGKILL fallback, blocking the event loop for up to 30s — degrading the very backstop it sits next to, in exactly the wedged-daemon scenario the watchdog exists for. — Failure scenario: probe with a wedged (15s-sleep) docker stub: the SIGKILL backstop armed at ~1.2s could not fire through the spawnSync block — exit at 16.2s vs 11.2s with a healthy daemon — and the child's close, finish(), and log flushing all queue behind it; replacing spawnSync with an unref'd async spawn restores 11.2s while still issuing the rm. The inline comment promises "a daemon blip must not mask the kill", but a slow daemon delays the kill's backstop by up to the spawn timeout. Suggested fix: make the removal asynchronous — e.g. spawn('docker', ['rm', '-f', '--', sandboxName], { stdio: 'ignore', timeout: 30_000 }) with the warning emitted from the close/error handlers — or move the removal to after the child has settled.
中文说明
[建议] R3-6：escalateKill 中同步的 spawnSync('docker rm -f', { timeout: 30_000 }) 恰好运行在 SIGTERM 与排队中的 10 秒 SIGKILL 兜底之间，会把事件循环阻塞最多 30 秒——恰在看门狗针对的“守护进程卡死”场景下，拖慢了它紧挨着的那个兜底。— 故障场景：用卡死（sleep 15s）的 docker 桩做探针：约 1.2s 时挂起的 SIGKILL 兜底无法穿过 spawnSync 阻塞触发——退出时间 16.2s，而健康守护进程下是 11.2s——期间子进程的 close、finish() 与日志刷写全部排队等待；把 spawnSync 换成不 await 的异步 spawn 后恢复 11.2s，且 rm 照常发出。行内注释承诺“守护进程抖动不应掩盖 kill”，但慢守护进程会把 kill 兜底最多延迟一个 spawn 超时。修复建议：把容器移除改为异步——例如 spawn('docker', ['rm', '-f', '--', sandboxName], { stdio: 'ignore', timeout: 30_000 })，警告在 close/error 回调中输出——或把移除挪到子进程结束之后。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| 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' |
There was a problem hiding this comment.
[Suggestion] R3-7: this else-branch conflates "measured not-behind" with "compare never ran": CMP_R is only assigned when both gh api calls succeed (both swallow failures via || echo ''), so a transient API failure leaves CMP_R empty and this clause still asserts confidently that the failure lives in the branch's own pre-round code. — Failure scenario: probe executing the real report-step block with a stubbed gh: with PREEXISTING=true and the compare call failing, the headline renders "…the branch's own pre-round code needs attention" although the comparison never ran — the branch may actually be behind main and the cure a base update, so the maintainer audits pre-round commits and finds nothing. The comment above ("The compare already ran; use it") documents the assumption the API failure falsifies; the gate-rejection detail hedges both remedies, mitigating but not answering the confident headline. Suggested fix: split the empty case — when [[ -z "${CMP_R:-}" ]], emit a clause saying the base state could not be compared (e.g. "check whether merging main resolves it") instead of asserting the branch's own code is at fault.
中文说明
[建议] R3-7：该 else 分支把“测得未落后”与“比较从未执行”混为一谈：CMP_R 只有在两个 gh api 调用都成功时才赋值（两者都用 || echo '' 吞掉失败），因此一次瞬时的 API 失败会让 CMP_R 为空，而该子句仍然断言失败出在分支自己的轮前代码里。— 故障场景：用桩 gh 执行真实的报告步骤块做探针：PREEXISTING=true 且 compare 调用失败时，标题会渲染“……分支自己的轮前代码需要处理”，尽管比较根本没有执行——分支实际可能落后于 main，真正的解法是更新基线，维护者却去审计轮前提交而一无所获。上方注释（“The compare already ran; use it”）所记载的前提恰被 API 失败证伪；gate-rejection 详情对两种补救都有提及，有所缓解但无法纠正这句武断的标题。修复建议：把空值情形单列——当 [[ -z "${CMP_R:-}" ]] 时，输出“无法比较基线状态（可尝试合并 main 看是否解决）”之类的子句，而不是断言分支自身代码有问题。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| 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 |
There was a problem hiding this comment.
[Suggestion] R3-11: when the breaker window is entirely idle rounds (IDLE_N == TIMEOUT_N), the headline's closing actionable sentence still prescribes "split or reduce the PR (or raise the agent time budget AND its step backstop together)" — the exact remedy the just-inserted IDLE_CLAUSE declares cannot cure these rounds. — Failure scenario: probe reproduced the all-idle window (TIMEOUT_N=3, IDLE_N=3) against the real breaker block: the headline says "no budget increase can cure — investigate the sandbox image and runner docker daemon" and in the next sentence instructs raising the agent time budget. A wedged docker daemon — the incident class this PR exists for — produces consecutive silent-sandbox rounds; the maintainer follows the closing action sentence, spends the config change, and the next window fails identically. The round-level terminal path already splits advice by cause (the idle branch points at the sandbox, not the budget knobs); the breaker only appends the correction mid-paragraph. Suggested fix: mirror the round-level split — when [[ "${IDLE_N}" -ge "${TIMEOUT_N}" ]], replace the remedy sentence (interpolate a REMEDY variable set to "investigate the sandbox image and runner docker daemon") instead of only appending the clause.
中文说明
[建议] R3-11：当熔断窗口内全部是空闲超时轮次（IDLE_N == TIMEOUT_N）时，标题结尾的可执行句仍然建议“拆分/缩小 PR（或同时提高 agent 时间预算与其 step 兜底）”——而刚插入的 IDLE_CLAUSE 明确说这类轮次提高预算治不了。— 故障场景：对真实熔断块做探针复现全空闲窗口（TIMEOUT_N=3, IDLE_N=3）：标题先说“提高预算无药可治——应检查沙箱镜像与 runner docker 守护进程”，下一句却让提高 agent 时间预算。卡死的 docker 守护进程——本 PR 针对的事故类别——会产生连续的静默沙箱轮次；维护者照结尾那句执行，花掉一次配置变更，下一个窗口照样失败。轮级终端路径已经按原因拆分建议（idle 分支指向沙箱而非预算旋钮）；熔断器却只在段落中间追加一句纠正。修复建议：对齐轮级做法——当 [[ "${IDLE_N}" -ge "${TIMEOUT_N}" ]] 时改为替换补救句（插入一个 REMEDY 变量，取“检查沙箱镜像与 runner docker 守护进程”），而不是只追加子句。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| expect(r.timeoutSentinel).toContain('idle-timeout (no output for 1200ms'); | ||
| }); | ||
|
|
||
| it('never fires while the agent keeps talking, however slowly', () => { |
There was a problem hiding this comment.
[Suggestion] R3-12: the watchdog's "never kill a talking agent" contract is pinned for stdout only — every survival stub emits via echo (stdout), and both die-stubs die regardless of which stream counts, so the stderr half of the timing path has no liveness test. — Failure scenario: probe-verified, mutating record() so stderr chunks no longer refresh lastOutputAt (while stderr stays wired for the ContainerName capture the orphan test needs) survives the entire suite, yet a stderr-only-ticks probe (echo "tick $i" >&2 every 400ms, idleMs: 1500) kills the healthy agent under the mutation — idle-timeout (no output for 1500ms — the sandbox likely hung at startup) — and passes on pristine code. In production the sandbox launcher emits ContainerName via stderr, and a cold-runner image pull or docker/tool progress with quiet stdout is a realistic stderr-only span: a live run would be killed and mislabeled as a wedged sandbox — the exact false positive the watchdog's design comment exists to prevent. Suggested fix: add one survival case whose ticks go to stderr only, expecting status 0 and empty failure.
中文说明
[建议] R3-12：看门狗“持续输出的 agent 绝不能被杀”的契约只为 stdout 钉住——所有存活类桩都用 echo（stdout）输出，而两个必死类桩无论哪个流计数都会死，因此计时路径的 stderr 半边没有存活测试。— 故障场景：探针验证过，把 record() 变异成 stderr 数据块不再刷新 lastOutputAt（stderr 仍保留用于孤儿测试需要的 ContainerName 捕获）后整套测试依然全绿；而一个只往 stderr 打点的探针（每 400ms echo "tick $i" >&2、idleMs: 1500）在变异代码下会被杀——idle-timeout (no output for 1500ms — the sandbox likely hung at startup)——在未变异代码上则正常通过。生产环境中沙箱启动器通过 stderr 输出 ContainerName，冷 runner 拉镜像或 docker/工具进度输出而 stdout 安静是真实存在的“仅 stderr”时段：一个活着的运行会被杀掉并被误标为沙箱卡死——正是看门狗设计注释要杜绝的误报。修复建议：新增一个打点只走 stderr 的存活用例，断言 status 0 且 failure 为空。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // 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 |
There was a problem hiding this comment.
[Suggestion] R3-15: the strict-subset arm of the A/B decision table — baseline fails with a SUPERSET of the round's failing signatures, which per the documented subset semantics must yield preexisting=true — has no behavioral test, and the runGate fixture cannot stage it (its only asymmetry knob, extraRoundDiag, adds diagnostics to the HEAD leg). — Failure scenario: probe-verified, replacing the comm -23/new_in_round subset check with set equality ([[ "${sig_head}" != "${sig_base}" ]]) survives all 143 tests, while an always-charge control fails — the comparator is alive and the arm is genuinely unpinned. Production then flips strict-subset rounds from preexisting=true/no-retry to retryable=true: a branch already red at origin whose round fix incidentally cures one of two diagnostics still fails the gate with the remainder; the repair leg is dispatched on a failure the script's own comment declares outside its boundary, fails, and the verdict repeats each round — the repeated-repair-waste class (run 31276008548) this PR exists to eliminate. Suggested fix: add an extraBaselineDiag knob (the stub's baseline branch emits a second tsc-style diagnostic) and a mirror test asserting preexisting=true, no retryable=true, and headAfter restored.
中文说明
[建议] R3-15：A/B 决策表的严格子集分支——基线以本轮失败签名的超集失败，按文档化的子集语义应判 preexisting=true——没有行为测试，且 runGate 夹具无法构造它（唯一的非对称旋钮 extraRoundDiag 只能给 HEAD 一侧加诊断）。— 故障场景：探针验证过，把 comm -23/new_in_round 子集检查替换为集合相等（[[ "${sig_head}" != "${sig_base}" ]]）后全部 143 个测试仍通过，而一个“总是归责本轮”的对照则会失败——比较器是活的，该分支确实没被钉住。生产中严格子集轮次会因此从 preexisting=true/不重试翻转为 retryable=true：一个在 origin 上本来就红的分支，本轮修复顺带治好了两条诊断之一，仍会因剩下的那条失败；repair 分支被派去处理脚本注释明言超出其边界的失败，失败后每轮重复同样的结论——正是本 PR 要消灭的连续 repair 浪费（run 31276008548）。修复建议：增加 extraBaselineDiag 旋钮（桩的基线分支额外输出一条 tsc 风格诊断），并加镜像测试断言 preexisting=true、无 retryable=true、headAfter 已恢复。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| IDLE_N="$(grep -c 'idle-timeout' <<< "${PRIOR_HEADS}" || true)" | ||
| if [[ "${AGENT_TIMEOUT:-}" == 'idle-timeout'* ]]; then | ||
| IDLE_N=$(( IDLE_N + 1 )) | ||
| fi |
There was a problem hiding this comment.
[Suggestion] R3-18: the breaker's current-round idle increment (IDLE_N=$(( IDLE_N + 1 )) when AGENT_TIMEOUT starts with idle-timeout) is exercised by no test: idleMixed is the only positive idle case and its window already carries two idle priors, so the grep -c alone keeps IDLE_N > 0; the clause renders with or without the increment, and nothing asserts the count. The sibling TIMEOUT_N increment IS behaviorally pinned. — Failure scenario: probe-verified, deleting the increment lines still passes the breaker suite. Under that mutation every idle breaker window undercounts by one — "2 of those were silent-sandbox" against TIMEOUT_N=3, implying one genuine budget exhaustion and steering toward the budget remedy the clause exists to veto — and with cap−1 non-idle priors plus an idle current round (reachable at the shipped cap=3), IDLE_N stays 0 and IDLE_CLAUSE is suppressed entirely: pure budget advice for a failure class no budget increase cures. The probe also showed a case asserting "1 of those were silent-sandbox" flips red/green across the mutation boundary. Suggested fix: add that case (a window of cap−1 non-idle TIMEOUT_HEAD priors with the current round idle), or pin the increment lines literally alongside the grep pin.
中文说明
[建议] R3-18：熔断器对当前轮空闲的自增（AGENT_TIMEOUT 以 idle-timeout 开头时的 IDLE_N=$(( IDLE_N + 1 ))）没有任何测试覆盖：idleMixed 是唯一含空闲的正例，而它的窗口里已有两条空闲历史标题，单靠 grep -c 就能让 IDLE_N > 0；有无自增该子句都会渲染，也没有任何断言检查计数。而兄弟的 TIMEOUT_N 自增是有行为钉住的。— 故障场景：探针验证过，删掉自增行后熔断测试套件仍然通过。带着该变异，每个空闲熔断窗口都少计一次——TIMEOUT_N=3 时标题说“其中 2 次是静默沙箱超时”，暗示还存在一次真正的预算耗尽，把维护者引向该子句本要否决的“提高预算”补救——而当窗口是 cap−1 条非空闲历史 + 当前轮空闲（线上 cap=3 即可达成）时，IDLE_N 停留在 0，IDLE_CLAUSE 完全不出现：对一个提高预算治不了的故障类别给出纯预算建议。探针同时验证：断言“1 of those were silent-sandbox”的用例能在变异边界上红/绿翻转。修复建议：新增该用例（cap−1 条非空闲 TIMEOUT_HEAD 历史 + 当前轮空闲），或像 grep 钉住那样把自增行原文钉住。
— qwen3.8-max via Qwen Code /review (v0.21.8)
… unpinned arms - Wrap every docker call in the stale-container reap with timeout 30: an alive-but-wedged daemon blocks docker ps indefinitely, and the existing || guards only catch nonzero exits, not hangs (R3-1). - Make the kill-path container removal async in run-agent.mjs: the spawnSync blocked the event loop between SIGTERM and the 10s SIGKILL backstop for up to its 30s timeout — in exactly the wedged-daemon scenario the watchdog exists for. The main flow awaits the removal so the leak warning stays deterministic (R3-6). - Split the pre-existing gate clause for an empty CMP_R: a transient compare-API failure is "never measured", not "measured not-behind", and must not assert the branch's own code is at fault (R3-7). - Swap the timeout breaker's closing remedy to the sandbox investigation when every counted timeout was idle, mirroring the round-level split (R3-11). - Tests: pin the budget kill path separately from the idle kill path (R3-3), parameterize the idle-window parse guard over -1/0/NaN (R3-5), add a stderr-only liveness case (R3-12), pin the strict-subset A/B arm via a baseline-superset fixture knob (R3-15), and pin the breaker's current-round idle increment (R3-18).
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Addressed all nine round-3 suggestions; each fix is pinned by a new or updated test, and every pin was mutation-checked (the exact defect each finding names was injected and confirmed to flip its test red before reverting). No base-conflict merge was needed ( Implemented
Verification
中文说明已处理第三轮全部九条建议；每项修复都由新增或更新的测试钉住，且每个钉住都经过了变异检查（先注入各发现所指出的确切缺陷、确认对应测试变红后再还原）。无需基线冲突合并（ 已实施
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 334 passed · 0 failed · 334 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：334 通过 · 0 失败 · 334 总计 Verification reportPR 8816 Deep Verification ReportVerdict: 中文摘要
Scope selectionPR 8816 bundles three mechanisms (title names two; the first four commits add the third):
Out of scope by budget decision: the root cause inside the wedged container (declared out of scope by the PR itself), repo-wide gates, per-commit attribution (unreachable, see Not covered). A/B: idle watchdog (central claim)Harness
Sibling sweep on the container-name capture (same root cause, adjacent doors): launcher line split across two chunks → captured; The base arm's 30 s spawn-timeout on the silent cell during the head-tests-on-base run (see below) is the same flip at production scale: without the watchdog the shape simply runs out the whole budget. Reaper step (secondary A)Harness
Baseline A/B gate (secondary B)The suite's 14 Targeted gates
Mutation matrixRun at head with narrowed vitest filters; positive controls = the killed mutants (the suite demonstrably goes red). Witness
Corrections
FindingsAll four are nits; none block.
No injection attempts were observed in the PR text. Not covered
MethodologyOne paragraph: CI merge-ref checkout (HEAD = merge commit, Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
Code reviewIndependent baseline first: for silent sandbox hangs I would do exactly what this PR does — an output-activity watchdog in the runner wrapper with its own failure wording (so the operator tunes the right knob) routed to the existing retry sentinel, plus container cleanup split by ownership (kill path removes the container it launched; job-start reap removes only provably-dead containers). The PR matches that shape, and goes one refinement beyond the naive version: the docker daemon is per-host while several runner registrations share one OS, so a running What I verified in the diff at this head:
No critical findings. One standing caveat, not a blocker: the 20-minute default is an argument from fleet history (longest tolerated silence anywhere is 10 minutes) plus an env knob — it's only falsifiable by the next real hang. That's inherent to the problem, not a defect of this code. Test evidence — the PR's own CI (I do not run PR code in triage)
The ubuntu Test job ran the full profile (log: Not verified, and no sandboxed lane could verify it: whether the next real fleet hang dies at 20 minutes — that requires the wedge to recur on a self-hosted runner with its docker daemon state; neither 中文说明代码审查先说独立基线：针对静默挂起，我会做和本 PR 完全相同的事——runner 包装层加基于输出活动的看门狗、配独立失败措辞（让运维调对旋钮）、接入现有重试哨兵；容器清理按所有权拆分（kill 路径删自己启动的容器，job 启动收割只碰可证明已死的容器）。PR 与这个形态一致，还多了一层我没第一时间想到的细化：docker daemon 按宿主计，而一台宿主上有多个 runner 注册，运行中的 在当前 head 的 diff 中核实的内容：
无阻断发现。一个长期保留的非阻断 caveat：20 分钟默认值是基于舰队历史的论证（任何地方容忍的最长静默是 10 分钟）加一个环境变量旋钮——只有下次真实挂起才能证伪。这是问题本身的属性，不是代码缺陷。 测试证据——PR 自己的 CI（triage 不执行 PR 代码）CI 表格见上（英文部分，机器可读区由 finalize 工作流维护）。ubuntu Test job 跑的是 full profile（日志： 未验证、且任何沙箱通道都无法验证：下次真实舰队挂起是否会在 20 分钟死掉——那需要楔死在自托管 runner 上带着 daemon 状态复发， — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a careful, thoroughly-reviewed mitigation for an observed, expensive failure; CI green at this head and the mechanism pinned by end-to-end tests. The missing 1/5 is not code doubt: the fleet-level payoff only proves itself on the next real hang, and the branch still stacks on open PR #8765, which has moved one commit ahead since. Stepping back: my independent proposal and this PR's design agree, and the PR goes past the naive version of it — the per-host/per-registration container-ownership constraint (a running Remaining caveats, both named above and neither blocking: the 20-minute default is a fleet-history argument with an env escape hatch, and the merge order is the author's to manage — #8765 first, then a rebase of this branch (expect conflicts in the verification script and tests; this approval dismisses on that push, correctly). Approving, pinned to the reviewed commit. 中文说明信心：4/5 —— 针对已观测到的高代价故障的细致、充分审查过的缓解措施；当前 head CI 全绿，机制由端到端测试钉住。差的 1/5 不是对代码的怀疑：舰队层面的收益要等下次真实挂起才能证明，且分支仍堆叠在 open PR #8765 上，而后者又前进了一个提交。 退一步看：我的独立方案与本 PR 的设计一致，而且 PR 超过了朴素版本——按宿主/按注册的容器所有权约束（运行中的 其余两点 caveat 均已在上面点名、均非阻断：20 分钟默认值是基于舰队历史的论证并带环境变量逃生门；合并顺序由作者掌控——先 #8765，然后本分支 rebase（验证脚本与测试处预期有冲突；该推送会正确地把本次 approval 撤销）。 批准，钉在所审查的提交上。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅ (Triage: observed problem, sound two-part mitigation, end-to-end tests green at this head in full-profile CI; merge order vs stacked PR #8765 noted in the Stage 3 comment.)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查——无阻断问题。 建议见行内评论。 未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| idleTimer = setInterval(() => { | ||
| if (settled || timedOut || idleTimedOut) return; | ||
| if (Date.now() - lastOutputAt >= QWEN_IDLE_TIMEOUT_MS) { |
There was a problem hiding this comment.
[Suggestion] R4-1: the idle branch fires escalateKill() but never clears the absolute timer, and the absolute-timer callback has no guard — so escalateKill runs twice when an idle-killed child outlives SIGTERM+SIGKILL (uninterruptible/D state — the wedge family this PR targets). Only finish() clears timer, and it never runs for a child that never closes. — Failure scenario: production defaults (idle 20 min / absolute 50 min) leave a 30-minute double-fire window; the second docker rm -f on an already-removed container emits a false warning: leaked sandbox container …, and if the freed name has been reassigned by the per-host name counter (the reuse the reap-step comment documents — sandbox.ts picks the lowest free index), the second rm -f destroys a concurrent job's live sandbox.
| idleTimer = setInterval(() => { | |
| if (settled || timedOut || idleTimedOut) return; | |
| if (Date.now() - lastOutputAt >= QWEN_IDLE_TIMEOUT_MS) { | |
| idleTimer = setInterval(() => { | |
| if (settled || timedOut || idleTimedOut) return; | |
| if (Date.now() - lastOutputAt >= QWEN_IDLE_TIMEOUT_MS) { | |
| clearTimeout(timer); |
中文说明
[建议] R4-1：idle 分支触发 escalateKill() 但从未清除绝对计时器 timer，且绝对计时器回调没有任何守卫——因此当被 idle 杀死的子进程在 SIGTERM+SIGKILL 后仍存活（不可中断/D 状态——正是本 PR 针对的卡死家族）时，escalateKill 会执行两次。只有 finish() 会清除 timer，而永不清除的子进程永远不会触发它。— 故障场景：生产默认值（idle 20 分钟 / 绝对 50 分钟）留下 30 分钟的二次触发窗口；对已删除容器第二次执行 docker rm -f 会发出虚假的 warning: leaked sandbox container …，若该名字已被同主机命名计数器重新分配（reap 步骤注释记载的这种复用——sandbox.ts 选取最小空闲序号），第二次 rm -f 将摧毁另一个并发 job 正在运行的沙箱。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // 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 |
There was a problem hiding this comment.
[Suggestion] R4-2: the guard admits any positive value, so a minutes-shaped value arms a millisecond idle window — the sibling budget knob in this same workflow has an explicit BUDGET_FLOOR_MS=60000 ("120 means 120ms, not 120 minutes") for exactly this mistake class; the new knob does not mirror it. Probe-verified: with QWEN_IDLE_TIMEOUT_MS=20 a talking agent (output every 400ms) is idle-killed at the first 250ms tick with idle-timeout (no output for 20ms …); the same stub exits 0 under the default. Zero live exposure today (no workflow sets the knob), but its comment invites per-leg tuning. — Failure scenario: a maintainer wires QWEN_IDLE_TIMEOUT_MS: '20' (minutes-shaped) → every agent is idle-killed within ~250-500ms fleet-wide, every round fails with idle-timeout comments, and the breaker's new REMEDY points maintainers at the healthy sandbox image/docker daemon.
中文说明
[建议] R4-2：该防护接受任意正值，因此分钟形态的值会装载毫秒级空闲窗口——同一 workflow 中的姊妹预算旋钮专门为这类错误设了显式的 BUDGET_FLOOR_MS=60000（"120 意为 120ms，不是 120 分钟"），新旋钮没有镜像它。已探针验证：QWEN_IDLE_TIMEOUT_MS=20 时，持续输出（每 400ms）的 agent 在首个 250ms tick 即被以 idle-timeout (no output for 20ms …) 杀掉；同一桩在默认值下正常退出。当前零线上暴露（没有 workflow 设置该旋钮），但其注释鼓励按分支调参。— 故障场景：维护者接入 QWEN_IDLE_TIMEOUT_MS: '20'（分钟形态）→ 全集群每个 agent 在约 250-500ms 内被 idle 杀掉，每轮以 idle-timeout 失败，且熔断器新的 REMEDY 会把维护者引向健康的沙箱镜像/docker 守护进程。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| ' echo "stub build FAILED at $head"', | ||
| ' echo "src/f.ts${pos}: error TS${code}: ${msg}"', |
There was a problem hiding this comment.
[Suggestion] R4-3: the empty-signature fail-closed arm of baseline_also_fails ([[ -z "${sig_head}" || -z "${sig_base}" ]]) is exercised by no test, because the stub emits a tsc diagnostic on every staged failure. Probe-verified: deleting the guard keeps all 14 baseline-A/B tests green. — Failure scenario: an A/B-eligible check failing on both legs without error TS diagnostics (the vite/esbuild shapes fail_signature's own KNOWN-LIMIT comment names, or a baseline-leg infra hiccup) yields two empty signatures; comm -23 over empty sets is empty, and the verdict flips to preexisting=true — repair skipped, and the round stalls on wrong "base update" advice.
Suggested fix: add a noDiag stub knob (failing path emits no error TS line) and assert retryable=true, no preexisting=true for failAt: ['feature', 'origin/feature'].
中文说明
[建议] R4-3：baseline_also_fails 的空签名 fail-closed 分支（[[ -z "${sig_head}" || -z "${sig_base}" ]]）没有任何测试覆盖，因为桩在每次构造的失败中都会输出一条 tsc 诊断。已探针验证：删除该守卫后全部 14 个 baseline-A/B 测试仍为绿色。— 故障场景：某个 A/B eligible 检查在两腿上都失败但没有 error TS 诊断（fail_signature 自己的 KNOWN-LIMIT 注释点名的 vite/esbuild 形态，或基线腿的基础设施抖动）会产生两个空签名；对空集合做 comm -23 结果为空，判定会翻转为 preexisting=true——repair 被跳过，整轮卡在错误的 "base update" 建议上。
修复建议：给桩加一个 noDiag 旋钮（失败路径不输出 error TS 行），并对 failAt: ['feature', 'origin/feature'] 断言 retryable=true 且无 preexisting=true。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| expect(workflow).toContain( | ||
| "PREEXISTING: '${{ steps.final_verify.outputs.preexisting }}'", | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R4-4: the finalize re-emission of the preexisting flag (if [[ "${PREEXISTING}" == 'true' ]]; then echo "preexisting=true" >> "${GITHUB_OUTPUT}", workflow ~4654-4656, added in this PR) is pinned by no test — the consumption tests pin only the endpoints and env declarations. Probe-verified: deleting the finalize echo leaves all 146 tests green — steps.final_verify.outputs.preexisting becomes permanently empty, the report step's outer guard never fires, and this PR's entire preexisting-clause feature silently never renders. (The report-side outer guard itself IS pinned by the behavioral test 'retries a verification-gate crash instead of burying the fix'; only this link is unpinned.)
Suggested fix: expect(workflow).toMatch(/if \[\[ "\$\{PREEXISTING\}" == 'true' \]\]; then\n\s*echo "preexisting=true" >> "\$\{GITHUB_OUTPUT\}"/).
中文说明
[建议] R4-4：preexisting 标志在 finalize 阶段的重新发射（if [[ "${PREEXISTING}" == 'true' ]]; then echo "preexisting=true" >> "${GITHUB_OUTPUT}"，workflow 约 4654-4656 行，本 PR 新增）没有任何测试钉住——消费侧测试只钉了链路与 env 声明。已探针验证：删除该 finalize echo 后全部 146 个测试仍为绿色——steps.final_verify.outputs.preexisting 永久为空，报告步骤的外层守卫永不触发，本 PR 的整个 preexisting 子句功能会静默地永不渲染。（报告侧外层守卫本身由行为测试 'retries a verification-gate crash instead of burying the fix' 钉住；只有这一环没被钉。）
修复建议：expect(workflow).toMatch(/if \[\[ "\$\{PREEXISTING\}" == 'true' \]\]; then\n\s*echo "preexisting=true" >> "\$\{GITHUB_OUTPUT\}"/)。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| # 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='' |
There was a problem hiding this comment.
[Suggestion] R4-5: the reap probe's failure modes — daemon-down and daemon-wedged — collapse into STALE='' with zero log output, indistinguishable from the healthy "nothing stale" case; the step that exists to fight silent rounds emits no signal when its own probe fails. Probe-verified: both arms exit 0 with 0 bytes of output; a ::warning:: variant still exits 0, so it does not violate the must-not-kill-the-round rationale. Applies to both copies (~3553 too). — Failure scenario: a wedged docker daemon at setup: the round proceeds, sandbox creation hangs, the new idle watchdog kills the round ~20 min later as idle-timeout; every subsequent round on that host repeats the silent 30s probe + 20-minute burn until TIMEOUT_WINDOW_CAP stops the PR — with no setup-time breadcrumb, so operators debug the agent/model before the host.
| STALE="$(timeout 30 docker ps -aq --filter 'name=qwen-code-' --filter 'status=exited' --filter 'status=dead' 2>/dev/null)" || STALE='' | |
| STALE="$(timeout 30 docker ps -aq --filter 'name=qwen-code-' --filter 'status=exited' --filter 'status=dead' 2>/dev/null)" || { echo "::warning::docker ps failed or timed out; skipping stale-sandbox reap"; STALE=''; } |
(the contract test pins the exact || STALE='' string in both jobs — update those pins in lockstep)
中文说明
[建议] R4-5：reap 探测的失败模式——守护进程宕机与守护进程卡死——都静默坍缩为 STALE=''、零日志输出，与健康的"没有残留"情形无法区分；一个为消灭静默轮次而生的步骤，在自己的探测失败时却不发任何信号。已探针验证：两种情形都以 0 字节输出退出码 0；改用 ::warning:: 的变体仍以 0 退出，不违反"不得在 setup 杀掉整轮"的设计意图。两份副本同样适用（约 3553 行）。— 故障场景：setup 时 docker 守护进程卡死：本轮继续，沙箱创建挂起，约 20 分钟后新看门狗以 idle-timeout 杀掉本轮；该主机上后续每一轮都重复这静默的 30s 探测 + 20 分钟燃烧，直到 TIMEOUT_WINDOW_CAP 叫停 PR——没有任何 setup 阶段的线索，运维会先去排查 agent/模型而不是主机。
（契约测试在两个 job 中都钉了 || STALE='' 的确切字符串——需同步更新这些钉子。）
— qwen3.8-max via Qwen Code /review (v0.21.8)
| writeFileSync( | ||
| join(bin, 'docker'), | ||
| '#!/bin/bash\necho "$@" >> "${AGENT_WORKDIR}/docker-calls.txt"\nexit 0\n', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R4-10: the rm-failure branch (warnLeak on nonzero close / spawn error — the only signal of a leaked RUNNING container, which the startup reap deliberately cannot clean) is never exercised: both kill-path tests stub docker to exit 0. Probe-verified: a docker stub exiting 1 produces exactly one warning: leaked sandbox container qwen-code-9.9.9-9 on stderr — but deleting warnLeak keeps every test green. — Failure scenario: the wedged-daemon state escalateKill exists for is exactly where docker rm -f fails or hangs; with no test on that branch, a regression swallowing the warning ships, and a leaked running sandbox on the persistent runner becomes invisible — nothing else in this PR removes running containers.
Suggested fix: a runKillPath variant with a docker stub that exits 1, asserting stderr contains leaked sandbox container qwen-code-9.9.9-9.
中文说明
[建议] R4-10：rm 失败分支（非零 close / spawn 错误时的 warnLeak——泄漏的运行中容器的唯一信号，而启动 reap 有意不能清理运行中容器）从未被执行：两个 kill 路径测试的 docker 桩都以 0 退出。已探针验证：以 1 退出的 docker 桩恰好产生一条 warning: leaked sandbox container qwen-code-9.9.9-9——但删除 warnLeak 后所有测试仍为绿色。— 故障场景：escalateKill 为之而生的守护进程卡死状态，恰是 docker rm -f 失败或挂起的状态；该分支没有测试，吞掉警告的回归就能上线，持久 runner 上泄漏的运行中沙箱将不可见——本 PR 再无其他机制移除运行中容器。
修复建议：docker 桩以 1 退出的 runKillPath 变体，断言 stderr 含 leaked sandbox container qwen-code-9.9.9-9。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const QWEN_IDLE_TIMEOUT_MS = | ||
| Number.isFinite(parsedIdleTimeoutMs) && parsedIdleTimeoutMs > 0 | ||
| ? parsedIdleTimeoutMs | ||
| : 20 * 60 * 1000; |
There was a problem hiding this comment.
[Suggestion] R4-11: the 20-minute default is the production value (no workflow sets QWEN_IDLE_TIMEOUT_MS — grep-verified), yet every behavioral test supplies an explicit value, so the default is pinned nowhere. Probe-verified: changing the default to 5 minutes keeps all 7 watchdog/kill-path tests green. — Failure scenario: a too-small default would idle-kill healthy runs fleet-wide — long model thinking phases are silent by nature — burning rounds and posting bogus "silent sandbox" failure comments with no test objecting. The 20-minute choice (2× the fleet's documented 10-minute stream-idle tolerance) lives only in a comment.
Suggested fix: expect(readFileSync(autofixRunnerScriptPath, 'utf8')).toContain(': 20 * 60 * 1000;') alongside the existing runner-script pins, with a comment naming the 2× relationship.
中文说明
[建议] R4-11：20 分钟默认值就是生产值（没有 workflow 设置 QWEN_IDLE_TIMEOUT_MS——已 grep 验证），但所有行为测试都显式传值，因此默认值没有任何测试钉住。已探针验证：把默认值改成 5 分钟，全部 7 个看门狗/kill 路径测试仍为绿色。— 故障场景：过小的默认值会在全集群范围内把健康运行 idle 杀掉——模型长思考阶段天然是静默的——烧掉轮次并发布虚假的"静默沙箱"失败评论，而没有任何测试提出异议。20 分钟的取值（全队列文档化 10 分钟流空闲容忍的 2 倍）只存在于注释里。
修复建议：与现有 runner-script 钉子并列，expect(readFileSync(autofixRunnerScriptPath, 'utf8')).toContain(': 20 * 60 * 1000;')，注释注明 2 倍关系。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| expect(j, jobId).toContain( | ||
| 'xargs -r -I{} timeout 30 docker rm -f {} > /dev/null 2>&1 || true', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R4-12: the reap step is inlined byte-identically into issue-autofix and review-address, but these tests only toContain a few pinned substrings per job — there is no byte-identity pin between the two copies, unlike this file's existing precedent for the sanitize step (expect(sanitizeSteps[1]).toBe(sanitizeSteps[0])). Probe-verified: dropping the unpinned command -v docker > /dev/null || exit 0 line from the review-address copy only keeps ALL current assertions passing — the divergence is invisible. — Failure scenario: a fix later applied to one copy will not propagate, and no assertion notices, on a hardening step whose whole point is identical behavior on both registrations.
| expect(j, jobId).toContain( | |
| 'xargs -r -I{} timeout 30 docker rm -f {} > /dev/null 2>&1 || true', | |
| ); | |
| expect(j, jobId).toContain( | |
| 'xargs -r -I{} timeout 30 docker rm -f {} > /dev/null 2>&1 || true', | |
| ); | |
| } | |
| expect(getWorkflowStep(getWorkflowJob(workflow, 'review-address'), 'Remove stale sandbox containers')).toBe(getWorkflowStep(getWorkflowJob(workflow, 'issue-autofix'), 'Remove stale sandbox containers')); |
中文说明
[建议] R4-12：reap 步骤以逐字节相同的方式内联在 issue-autofix 与 review-address 两处，但这些测试只对每个 job toContain 若干钉住的子串——两份副本之间没有逐字节一致性钉子，不同于本文件对 sanitize 步骤的既有先例（expect(sanitizeSteps[1]).toBe(sanitizeSteps[0])）。已探针验证：仅从 review-address 副本删掉未钉住的 command -v docker > /dev/null || exit 0 行，现有全部断言仍通过——分歧不可见。— 故障场景：之后对某一份副本的修复不会传播到另一份，也没有任何断言能察觉，而这个强化步骤的全部意义就是两个注册上行为一致。
（suggestion 中追加一行逐字节一致性断言；getWorkflowStep/getWorkflowJob 已存在于 workflow-helpers.js。）
— qwen3.8-max via Qwen Code /review (v0.21.8)
| resolve('.qwen/skills/autofix/scripts/run-agent.mjs'), | ||
| '--mode', | ||
| 'address-review', |
There was a problem hiding this comment.
[Suggestion] R4-13: (pattern, 2 locations — this one and the runGate spawn at ~11454) runAgent hardcodes the runner-script path as a string literal while this file defines autofixRunnerScriptPath for exactly this path and every other call site — including runKillPath, added in the same diff ~180 lines below — uses the constant. — Failure scenario: if the skill script ever moves, updating the constant fixes runKillPath and the ~10 existing call sites, but the entire idle-watchdog suite keeps resolving the stale literal and fails with spawn errors that read as a watchdog regression rather than a path rename. Two sources of truth for one path created in the same commit.
| resolve('.qwen/skills/autofix/scripts/run-agent.mjs'), | |
| '--mode', | |
| 'address-review', | |
| resolve(autofixRunnerScriptPath), | |
| '--mode', | |
| 'address-review', |
中文说明
[建议] R4-13：（模式问题，共 2 处——此处与约 11454 行 runGate 的 spawn）runAgent 把 runner 脚本路径硬编码为字符串字面量，而本文件为同一路径定义了 autofixRunnerScriptPath，且其他所有调用点——包括同一 diff 中约 180 行下方新增的 runKillPath——都使用该常量。— 故障场景：若该 skill 脚本被移动，更新常量会修好 runKillPath 与约 10 个既有调用点，但整个 idle-watchdog 套件会继续解析陈旧的字面量，以 spawn 错误失败，看起来像看门狗回归而非路径改名。同一提交为一个路径制造了两个真源。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
|
||
| const res = spawnSync( | ||
| 'bash', | ||
| [resolve('.github/scripts/run-autofix-review-verification.sh')], |
There was a problem hiding this comment.
[Suggestion] R4-13: (pattern, 2 locations — this one and the runAgent helper at ~11754) runGate hardcodes the gate-script path as a string literal while this file defines reviewVerificationRunnerPath (pre-existing, line 30-31) for exactly this path and uses it for every other functional reference. — Failure scenario: on a script rename, updating the constant keeps the top-level source read working, but all 14 A/B behavioral tests keep resolving the stale literal; spawnSync returns {status: null, error: ENOENT} and the suite fails with spawn errors that read as an A/B-gate regression. Fix lands together with the sibling runAgent instance.
| [resolve('.github/scripts/run-autofix-review-verification.sh')], | |
| [resolve(reviewVerificationRunnerPath)], |
中文说明
[建议] R4-13：（模式问题，共 2 处——此处与约 11754 行的 runAgent 助手）runGate 把 gate 脚本路径硬编码为字符串字面量，而本文件为同一路径定义了（既有的，30-31 行）reviewVerificationRunnerPath，且所有其他功能性引用都使用它。— 故障场景：脚本改名时，更新常量可保证顶层源码读取正常，但全部 14 个 A/B 行为测试会继续解析陈旧字面量；spawnSync 返回 {status: null, error: ENOENT}，套件以 spawn 错误失败，看起来像 A/B gate 回归。与姊妹的 runAgent 实例一并修复。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no action takenThis round had no actionable feedback, so no code changes were made and no commit was created.
The PR head remains at the last verified commit ( 中文说明Autofix 评审轮次：未执行任何操作本轮没有可处理的反馈，因此未做任何代码改动，也未创建新的提交。
PR 的 head 仍停留在上一个经过验证的提交（ Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-review at head 74cd5e2 — my round-6 P2 is fixed and proven under live repro at both copies: the reap listing now survives a dead daemon (|| STALE=''), every rm is per-container with timeout 30 and a trailing || true, and I reproduced all three failure classes under bash -eo pipefail with a stub docker (concurrent-rm race, daemon unreachable, wedged daemon bounded at the timeout) — the step exits 0 in each. The hang-bounding goes beyond my suggested fix and closes the gap || true alone would have left. The kill-path orphan reap (the last open Critical) is in: ContainerName parsed from this run's own launcher output with a validated name pattern, removed on both budget and idle kills via the shared escalateKill, now async so the event loop is not blocked between SIGTERM and the SIGKILL backstop; the idle guard also checks timedOut and QWEN_IDLE_TIMEOUT_MS rejects -1/0/NaN, all pinned by tests. CI green and mergeable.
Non-blocking leftovers: the lineCarry name parser's 256-char cap can silently miss the ContainerName line after a very long single startup line (degrade-not-crash, extremely unlikely); escalateKill can run twice (idempotent signals/rm, harmless); and the two #8765 unique improvements (pre-detach empty-head-signature short-circuit, dist steering note) are not ported — per the coordination plan, land this first and port them as a follow-up, then close #8765 as subsumed. Ship it.
Main already carries this PR's A/B gate work through the stacked #8816 squash, which fixed the same round-5 gate defects with newer, stronger implementations (subset failure-identity comparator, non-retryable restore failure via reject_fix, unescaped printf backticks, three-arm CMP_R clause). Keep those, and re-apply this PR's additions that main lacks: the pre-detach head-signature decision, the dist-rebuilt warning on both retryable A/B exits, the side-log cleanup entries, the stale-base note on the embedded rejection, and the matching fixture knobs and scenarios.
…wenLM#8878) QwenLM#8816's branch accidentally carried QwenLM#8765's early commits, and the takeover loop evolved the gate further there (subset identity via comm -23, the retryable third arg, subset fixtures) — so QwenLM#8765 closes as subsumed, and this PR ports what main still lacks: the two improvements its reviewers named for porting, plus the open round-6/7 findings that survive on main's gate. - Pre-detach short-circuit: an empty head signature (vite/esbuild/ crash — the KNOWN LIMIT class) fails closed regardless of the baseline, so decide it BEFORE paying the detach + full baseline re-run + restore. - Build-dirt guard: the A/B'd build REWRITES a tracked file (the vscode companion settings schema), and the undiscarded rewrite makes either checkout refuse — degrading a real verdict into the crash path. `git restore -- .` before both checkouts; tracked-only, and the tree was asserted clean before the deterministic checks. - Restore-failure semantics: a plain outcome=failed is an EVALUATED rejection — the watermark advances and a transient git failure strands the item as a permanent human handoff. The gate now leaves outcome unset (the gate-crashed path retries next scan) and still writes the detail document so the crash comment explains itself. - The dist-rebuilt steering note seeds the repair feedback on both retryable A/B exits — the repair agent's only warning that dist/ holds baseline-built artifacts. - The stale-base retry handoff prefixes its embedded rejection with a the-base-has-moved note, so the retry agent is not steered toward no-action by framing written before the auto-update. - The two A/B side logs joined the repair step's cleanup list. - Tests: identity-less short-circuit, tracked-dirt survival, verdict-less restore crash, long-preamble render cap, PREEXISTING clause selection through the executable report harness, and the stale-framing note pin. Mutation-tested, 5 of 5 caught: short-circuit dropped, restore guards dropped, restore-failure reverted to the evaluated rejection, dist note dropped, stale-framing note dropped. Co-authored-by: verify <verify@local>
|
Released in v0.21.9. |
5 participants
What this PR does
Two mitigations for the silent 2-hour sandbox hangs that have been eating autofix rounds:
run-agent.mjs(QWEN_IDLE_TIMEOUT_MS, default 20 min): zero output for the window kills the agent with a distinctidle-timeout … the sandbox likely hung at startupdetail — a hung round costs 20 minutes instead of 120, and the failure comment names the right knob.Why it's needed
Four autofix rounds died identically (#8663 ×2, #8761 r3, #8763 r4): the last output is the sandbox wrapper's
ContainerName (regular): …line at docker container entry, then nothing — not one event — for the entire 2-hour budget:Four different runners (hk ×2, sg ×2), two image versions (0.21.7 / 0.21.8) — systemic, not a bad machine. Where the container wedges internally is still unknown (that needs docker state on a runner — flagged for whoever can shell in); what the logs prove is the shape: a wedged sandbox produces nothing, while a legitimate run is never silent for long — the fleet's longest tolerated quiet anywhere is the review pipeline's 10-minute stream-idle window for thinking phases on ~1M-token contexts. The watchdog default is twice that.
The container leak is observed directly, not inferred: the hung #8763 leg's container-name counter found
qwen-code-0.21.8-0already occupied and picked-1— a leftover from an earlier kill (sandbox.tsnames by countingdocker ps -a). The workflow had no cleanup anywhere. One job per runner at a time makes anyqwen-code-*container alive at job start stale by definition.Reviewer Test Plan
How to verify
Expected: 138/138 (3 new). Full scripts suite: 50 files, 1076 passed.
yamllint+node --checkclean.The new tests execute the real
run-agent.mjsend to end with stub agents:failure.mdnamesidle-timeout (no output for …)and not the absolute-budget wordingEvidence (Before & After)
Before is the four linked hangs (2 h × 4 ≈ 8 runner-hours for zero work). After: the same shape dies in 20 minutes with an honest failure detail, and each job start clears the leaked containers the previous kills left behind.
Mutation-tested — 3 of 3 caught:
Tested on
Risk & Scope
QWEN_IDLE_TIMEOUT_MSis the env knob. The reaper touches only containers namedqwen-code-*on a runner that, by GitHub's one-job-per-runner model, cannot have a live sibling.docker ps -a+docker logson a hung container, the diagnosis can go deeper.Linked Issues
Diagnosed across #8663, #8761 and #8763 (the comment that prompted this).
中文说明
What this PR does
针对一直在吞噬 autofix 轮次的"沙箱静默挂起 2 小时"问题的两项缓解:
run-agent.mjs增加无输出看门狗(QWEN_IDLE_TIMEOUT_MS,默认 20 分钟):窗口内零输出即杀,失败详情为独立的idle-timeout …(沙箱疑似启动时挂起)——挂起一轮的代价从 120 分钟降为 20 分钟,失败评论指向正确的旋钮。Why it's needed
四个 autofix 轮次以完全相同的方式死亡(#8663 ×2、#8761 r3、#8763 r4):最后一行输出是沙箱包装器在进入 docker 容器时打印的
ContainerName (regular): …,之后整整 2 小时预算内一个事件都没有:四台不同 runner(hk ×2、sg ×2)、两个镜像版本(0.21.7 / 0.21.8)——系统性问题,不是坏机器。容器内部究竟卡在哪仍未知(需要有人上 runner 看 docker 状态,已注明);日志能证明的是形态:挂死的沙箱什么都不产出,而合法运行不会长时间沉默——整个体系容忍的最长安静是 review 流水线为 ~1M token 上下文思考阶段设的 10 分钟流空闲窗口。看门狗默认取其两倍。
容器泄漏是直接观察到的而非推断:挂起的 #8763 leg 的容器命名计数器发现
qwen-code-0.21.8-0已被占用而选了-1——正是此前某次超时杀留下的残骸(sandbox.ts按docker ps -a计数命名)。workflow 此前没有任何清理。GitHub 一台 runner 同时只跑一个 job,因此 job 开始时还活着的任何qwen-code-*容器都必然是残留。Reviewer Test Plan
How to verify
预期 138/138(新增 3 条)。scripts 全量:50 个文件、1076 passed。
yamllint+node --check干净。新测试端到端执行真实的
run-agent.mjs配 stub agent:failure.md写明idle-timeout (no output for …)而不是绝对预算的措辞Evidence (Before & After)
Before 即链接的四次挂起(2 小时 × 4 ≈ 8 runner-小时零产出)。After:同样的形态 20 分钟内死亡并给出诚实的失败详情,且每次 job 启动都会清掉此前超时杀遗留的容器。
变异测试——3 个全部被捕获:
Tested on
Risk & Scope
QWEN_IDLE_TIMEOUT_MS就是旋钮。收割只触碰名为qwen-code-*的容器,而 GitHub 一 runner 一 job 的模型保证它不可能有活着的同胞。docker ps -a+docker logs,诊断可以更进一步。Linked Issues
跨 #8663、#8761、#8763 诊断(引发本 PR 的评论)。