Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 152 additions & 10 deletions .github/scripts/run-autofix-review-verification.sh
Original file line number Diff line number Diff line change
Expand Up @@ -43,25 +43,154 @@ git checkout "${BRANCH}"
GATE_LOG="${WORKDIR}/gate-output.log"
: > "${GATE_LOG}"
reject_fix() {
echo "❌ ${1}"
local label="${1}"
local preexisting="${2:-false}"
echo "❌ ${label}"
# Declare the verdict before writing its detail. An empty outcome on a failed
# step means the gate itself crashed, so losing the detail file must not turn
# a deterministic rejection into an infrastructure retry.
echo "outcome=failed" >> "${GITHUB_OUTPUT}"
echo "retryable=true" >> "${GITHUB_OUTPUT}"
if [[ "${preexisting}" == 'true' ]]; then
# NOT retryable: the repair agent is only allowed to amend this round's
# fix, and a failure that exists without the fix is outside that boundary
# by definition — the 18-minute repair budget cannot reach it. The remedy
# is a base update (merge main into the branch), not a repair.
echo "preexisting=true" >> "${GITHUB_OUTPUT}"
else
echo "retryable=true" >> "${GITHUB_OUTPUT}"
fi
# The evidence tail flexes so the WHOLE document stays under the report
# step's head -c 3900 render cap: truncating the finished document from
# the outside cuts the closing fence and malforms everything after it in
# the posted comment. Budget = 3300 minus the preamble, floored at 500.
local preamble tail_budget
preamble="**${label}**"
if [[ "${preexisting}" == 'true' ]]; then
preamble+="$(printf '\n\nMeasured fact: the same check also fails at \`origin/%s\` (the branch as pushed, before this round) in this environment, with a matching failure signature. The repair pass may only amend the round'"'"'s own fix, so it cannot reach this failure. If the branch is behind \`main\`, a base update (merge main) is the usual cure; otherwise the failure lives in the branch'"'"'s own pre-round commits.' "${BRANCH}")"
fi
tail_budget=$(( 3300 - ${#preamble} ))
(( tail_budget < 500 )) && tail_budget=500
{
echo "**${1}**"
printf '%s\n' "${preamble}"
echo
# Captured output can contain triple-backtick fences.
echo '````'
tail -c 3000 "${GATE_LOG}" 2> /dev/null
tail -c "${tail_budget}" "${GATE_LOG}" 2> /dev/null
echo '````'
} > "${WORKDIR}/gate-rejection.md" ||
echo "::warning::could not write the gate rejection detail; the verdict stands."
exit 1
}
baseline_also_fails() {
# A deterministic rejection is only chargeable to this round if the same
Comment on lines +86 to +87

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 接线以及变更后的重试/报告语义。

# check passes WITHOUT the round's commits. Measured counterexample, run
# 31276008548: PR #8614's branch predated #8693's tsconfig guard while
# node_modules came from the post-#8693 trusted base, so `npm run build`
# was just as red at origin/<branch> — 63 minutes of accepted agent work
# were discarded and an 18-minute repair burned on a failure the repair
# agent is forbidden to touch, thirteen rounds in a row.
# Returns 0 (pre-existing) only when the SAME command demonstrably fails
# at the pre-round ref; any A/B infrastructure problem returns 1 so the
# rejection keeps today's semantics (fail closed toward "charge the fix").
local current baseline rc
current="$(git rev-parse HEAD)" || return 1
baseline="$(git rev-parse --quiet --verify "origin/${BRANCH}^{commit}")" ||
return 1
# No round commit (the core-rebuild check runs before the commit gate and
# is A/B-eligible) — the baseline IS the tree under test; nothing to
# compare.
[[ "${baseline}" != "${current}" ]] || return 1
echo "🔁 Baseline A/B: re-running the failed check at origin/${BRANCH}" \
"(${baseline})" | tee -a "${GATE_LOG}"
git checkout --quiet --detach "${baseline}" 2>> "${GATE_LOG}" || return 1
# The baseline transcript goes to a SIDE log: gate-rejection.md renders
# `tail -c 3000` of GATE_LOG as the evidence window, and on a green
# baseline a chatty success transcript would fill it and push the actual
# failure text out — misdirecting the repair agent, the PR comment, and
# the next round's LAST_REJECTION block all at once.
local ab_log="${GATE_LOG}.baseline"
: > "${ab_log}"
rc=0
if ! "$@" >> "${ab_log}" 2>&1; then
rc=1
fi
if ! git checkout --quiet "${BRANCH}" 2>> "${GATE_LOG}"; then
# The tree is no longer the one under verification and nothing after
# this point may trust it.
reject_fix 'could not restore the verification tree after the baseline check'
fi
if [[ "${rc}" -ne 1 ]]; then
echo "🔁 baseline is green — the failure belongs to this round" \
| tee -a "${GATE_LOG}"
return 1
fi
# A nonzero baseline is NOT enough: the branch can fail there for reason A
# while the round fails for reason B, and an infrastructure hiccup in the
# baseline leg is a nonzero exit too. Pre-existing requires the SAME
# failure identity on both legs — compiler diagnostics normalized to
# file + error code (line/column shift with the round's edits). No
# diagnostics on either side means identity cannot be established, and
# the rejection stays charged to the round (fail closed).
local sig_head sig_base
# `|| true`: grep exits 1 on the NORMAL no-match case, and these
# assignments only survive `set -e` today because this function is called
# from an `if` condition (which suspends errexit). A future unconditional
# call site would otherwise turn the documented fail-closed path into a
# verdict-less gate crash.
sig_head="$(fail_signature "${GATE_LOG}.check")" || true
sig_base="$(fail_signature "${ab_log}")" || true
if [[ -z "${sig_head}" || -z "${sig_base}" ]] ||
! comm -12 <(printf '%s\n' "${sig_head}") <(printf '%s\n' "${sig_base}") \

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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。

| grep -q .; then

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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.

Suggested change
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)

echo "🔁 baseline fails for a DIFFERENT reason — charged to the round" \
| tee -a "${GATE_LOG}"
return 1
fi
# Only a FAILING baseline transcript with a matching signature is
# evidence — merge its tail into the window, where it backs the label.
tail -c 1500 "${ab_log}" >> "${GATE_LOG}" 2> /dev/null || true

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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.

Suggested change
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 交集作为证据(或补充冗长日志夹具),使该论断在文档中始终可见。

return 0
}
fail_signature() {
# Stable identity of a failed check: tsc-style diagnostics with the
# position stripped but the MESSAGE kept ("src/a.ts: error TS2504: …").
# Position strips because line/column shift with the round's edits; the
# message stays because file + code alone collide — two unrelated defects
# in one file sharing a common code (TS2339 is everywhere) would compare
# as "the same failure" and skip a repair that could have worked. A
# message naming a round-renamed identifier then under-matches — the
# fail-closed direction. Sorted unique so two transcripts compare with
# comm(1). KNOWN LIMIT: only tsc diagnostics carry identity; vite/esbuild
# failures yield an empty signature and deliberately fail closed (charged
# to the round) — widening needs their position formats normalized first.
grep -oE "[^ '\"]+\([0-9]+,[0-9]+\): error TS[0-9]+[^\n]*" "${1}" 2> /dev/null \
| sed -E 's/\([0-9]+,[0-9]+\)//' | sort -u
}
run_check() {
# pipefail makes the pipeline carry the command's status, not tee's.
# pipefail makes the pipeline carry the command's status, not tee's. The
# side copy holds THIS check's transcript alone — the identity comparison
# must not match diagnostics an earlier check left in the shared log.
local label="${1}"
shift
: > "${GATE_LOG}.check"
if ! "$@" 2>&1 | tee -a "${GATE_LOG}" "${GATE_LOG}.check"; then
if baseline_also_fails "$@"; then
reject_fix "${label} (pre-existing: also fails without this round's commit)" 'true'
fi
reject_fix "${label}"
fi
}
run_check_no_ab() {
# A/B-exempt: for checks whose baseline re-run would compare a DIFFERENT
# computation than the one that failed, so a baseline verdict proves
# nothing. The contracts check consumes its file list from stdin, which
# the first run drains — the baseline leg would re-check an empty list
# and pass vacuously. The schema check reads packages/core/dist, which
# the core-rebuild guard built from the ROUND's sources and which,
# being gitignored, survives the detach and confounds the baseline. Their
# rejections stay charged to the round — which is also where the repair
# agent can actually act on them (generate:settings-schema is in its
# allowlist).
local label="${1}"
shift
if ! "$@" 2>&1 | tee -a "${GATE_LOG}"; then
Expand Down Expand Up @@ -112,10 +241,10 @@ fi
# that predates the script does not contain it (bash would exit 127
# and kill the gate with no outcome), and the gate logic must come
# from the trusted base, not the branch under verification.
run_check 'settings schema is stale on the agent-committed fix' \
run_check_no_ab 'settings schema is stale on the agent-committed fix' \
bash "${RUNNER_TEMP}/check-settings-schema.sh"
CHANGED_FILES="$(git diff --name-only "origin/main...${BRANCH}")"
run_check 'cross-package contract verification failed' \
run_check_no_ab 'cross-package contract verification failed' \
bash "${RUNNER_TEMP}/check-autofix-contracts.sh" <<< "${CHANGED_FILES}"
assert_verification_tree

Expand All @@ -140,8 +269,14 @@ fi

echo '🔬 Re-running deterministic checks (independent of the agent)...'
run_check 'build failed on the agent-committed fix' npm run build
run_check 'typecheck failed on the agent-committed fix' npm run typecheck
run_check 'lint failed on the agent-committed fix' npm run lint
# Typecheck consumes core's dist (sdk-typescript resolves
# @qwen-code/qwen-code-core through the package exports to ./dist/*.d.ts),
# and dist is gitignored — it survives the baseline detach carrying the
# ROUND's build, so a baseline typecheck would run reverted sources against
# round-built declarations. Probe-verified three-arm flip on this tree. Same
# class as the schema check: A/B-exempt.
run_check_no_ab 'typecheck failed on the agent-committed fix' npm run typecheck
run_check_no_ab 'lint failed on the agent-committed fix' npm run lint

# Test changed/related files for the packages this PR touches.
# --changed follows the import graph so transitive breakage is caught.
Expand Down Expand Up @@ -172,7 +307,14 @@ else
continue
fi
echo "🧪 Testing ${p} (changed files only)..."
run_check "tests failed in ${p}" \
# A/B-exempt: package tests resolve sibling workspaces through their
# dist exports (channels/github -> @qwen-code/channel-base/dist), and
# dist survives the baseline detach carrying the ROUND's build — a
# baseline leg would test reverted sources against round-built
# dependencies. (A round-ADDED workspace also has no baseline at all:
# npm exits 1 there with "No workspaces found".) Their rejections stay
# charged to the round, where the repair agent can act.
run_check_no_ab "tests failed in ${p}" \
npm run test --workspace "${p}" --if-present -- --changed origin/main --passWithNoTests
done
fi
Expand Down
76 changes: 73 additions & 3 deletions .github/workflows/qwen-autofix.yml
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,24 @@ jobs:
fetch-depth: 0
persist-credentials: false

- name: 'Remove stale sandbox containers'
run: |-
# A budget kill (SIGTERM/SIGKILL from run-agent.mjs or the job
# timeout) reaps the HOST-side docker client, not the container:
# every killed sandbox keeps running on this persistent runner.
# Observed directly — a hung leg's container name counter found
# qwen-code-0.21.8-0 already occupied and picked -1. A runner
# executes one job at a time, so any qwen-code-* container alive
# at job start is stale by definition; reap it 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-')"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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.

Suggested change
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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.

Suggested change
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 批次一起列为首要事项。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

fi

- name: 'Reset autofix workspace'
run: |-
rm -rf "${WORKDIR}"
Expand Down Expand Up @@ -3471,6 +3489,24 @@ jobs:
fetch-depth: 0
persist-credentials: false

- name: 'Remove stale sandbox containers'
run: |-
# A budget kill (SIGTERM/SIGKILL from run-agent.mjs or the job
# timeout) reaps the HOST-side docker client, not the container:
# every killed sandbox keeps running on this persistent runner.
# Observed directly — a hung leg's container name counter found
# qwen-code-0.21.8-0 already occupied and picked -1. A runner
# executes one job at a time, so any qwen-code-* container alive
# at job start is stale by definition; reap it 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-')"
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
fi

- name: 'Reset autofix workspace'
run: |-
rm -rf "${WORKDIR}"
Expand Down Expand Up @@ -4549,10 +4585,25 @@ jobs:
REPAIR_OUTCOME: '${{ steps.verify_repair.outputs.outcome }}'
REPAIR_COMMITTED: '${{ steps.verify_repair.outputs.committed }}'
REPAIR_VERIFIED_HEAD: '${{ steps.verify_repair.outputs.verified_head }}'
FIRST_PREEXISTING: '${{ steps.verify.outputs.preexisting }}'
REPAIR_PREEXISTING: '${{ steps.verify_repair.outputs.preexisting }}'
run: |-
OUTCOME="${FIRST_OUTCOME}"
COMMITTED="${FIRST_COMMITTED}"
VERIFIED_HEAD="${FIRST_VERIFIED_HEAD}"
# The flag travels WITH the attempt whose outcome is selected: the
# first pass can fail a round-caused check, the repair fixes it, and
# the repair verification can then hit a pre-existing failure — that
# final classification is the one the report must render. Forwarded
# so the failure report can say "base update needed" instead of the
# generic gate-rejection clause.
PREEXISTING="${FIRST_PREEXISTING}"
if [[ "${REPAIR_ATTEMPTED}" == 'true' ]]; then
PREEXISTING="${REPAIR_PREEXISTING}"
fi
if [[ "${PREEXISTING}" == 'true' ]]; then
echo "preexisting=true" >> "${GITHUB_OUTPUT}"
fi
Comment on lines +4654 to +4656

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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.

Suggested change
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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。

if [[ "${REPAIR_ATTEMPTED}" == 'true' ]]; then
OUTCOME="${REPAIR_OUTCOME}"
COMMITTED="${REPAIR_COMMITTED:-${FIRST_COMMITTED}}"
Expand Down Expand Up @@ -5009,6 +5060,7 @@ jobs:
env:
OUTCOME: '${{ steps.final_verify.outputs.outcome }}'
COMMITTED: '${{ steps.final_verify.outputs.committed }}'
PREEXISTING: '${{ steps.final_verify.outputs.preexisting }}'
CONFLICT: '${{ steps.prepare.outputs.conflict }}'
DRY_RUN: '${{ needs.route.outputs.dry_run }}'
GITHUB_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}'
Expand Down Expand Up @@ -5280,6 +5332,20 @@ jobs:
# wording-doesn't-match-behaviour bug this PR fixes.
GATE_CLAUSE=''
[[ -s "${WORKDIR}/gate-rejection.md" ]] && GATE_CLAUSE=' — the verification gate rejected the attempt'
# Pre-existing failures get the honest clause: the rejection
# is not the agent's and the repair was deliberately skipped.
# The remedy depends on WHY it pre-exists, and this branch
# only renders when the stale-base auto-update above did NOT
# fire — which includes a branch current with main whose own
# pre-round commits carry the failure, where "merge main"
# changes nothing. The compare already ran; use it.
if [[ "${PREEXISTING}" == 'true' ]]; then
if [[ "${CMP_R:-}" == 'behind' || "${CMP_R:-}" == 'diverged' ]]; then
Comment on lines +5404 to +5405

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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.

Suggested change
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 处理),而非简单豁免。

GATE_CLAUSE=' — the verification gate hit a PRE-EXISTING failure (present without this round'"'"'s commit); the repair pass may only amend the round'"'"'s own fix, so it cannot reach it — the branch needs a base update (merge main)'
else
GATE_CLAUSE=' — the verification gate hit a PRE-EXISTING failure (present without this round'"'"'s commit); the repair pass may only amend the round'"'"'s own fix, so it cannot reach it — the branch'"'"'s own pre-round code needs attention'
fi
fi
HEADLINE="🤖 Could not produce a passing fix for this feedback (round ${MARK_ROUND}/${MAX_ROUNDS})${GATE_CLAUSE}. This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own."
fi
fi
Expand Down Expand Up @@ -5455,9 +5521,13 @@ jobs:
echo '<!-- autofix-gate-rejection-start -->'
echo "**Why it was not pushed:**"
echo
# Must stay >= reject_fix's tail -c 3000 + label + two four-backtick
# fences (~3.1 KB), or the closing fence is silently truncated.
head -c 3500 "${WORKDIR}/gate-rejection.md" | iconv -f utf-8 -t utf-8 -c | sed 's/<!--/<!\\-\\-/g' || true
# reject_fix sizes its own document: the evidence tail is
# dynamic (budget 3300 minus the preamble, floored at 500),
# so the finished file tops out ≈3.3 KB. 3900 is headroom,
# not arithmetic — retune it only together with the
# script's tail_budget formula, or the closing fence gets
# silently truncated again.
head -c 3900 "${WORKDIR}/gate-rejection.md" | iconv -f utf-8 -t utf-8 -c | sed 's/<!--/<!\\-\\-/g' || true
echo '<!-- autofix-gate-rejection-end -->'
fi
echo
Expand Down
Loading
Loading