Repository navigation
test(e2e): isolate protocol checks and ACP test state - #11286
Conversation
The prompt-latency probe in qwen-serve-baseline sends 20 real prompts through the shared OpenAI-compatible gateway and asserts p99 < 60s. One gateway-queued prompt in twenty fails it, and each vitest retry re-issues all 20 prompts into the same degraded window, so a single slow window fails every attempt and turns the shard red. Since the pool skip (#11004) the probe runs on exactly one E2E leg — macOS shard 2/2, the only shard holding the file — and that leg failed twice within six hours on unrelated commits (runs 34070970091 and 34088422718, both full-duration with retries consumed) while every Linux leg, macOS shard 1/2, and the OpenTUI leg stayed green. The measured quantity is gateway weather, not the daemon; the same argument the pool skip made for host contention applies to the shared gateway. Skip the probe whenever CI is set (any populated CI marker, matching the repo's other CI checks), keeping the self-hosted disjunct so a pool-shaped shell outside CI keeps its specific skip reason. The probe still runs off CI on a credential, and QWEN_BASELINE_ENABLE_PROMPT_LATENCY=1 force-runs it anywhere. The skip reason distinguishes CI gateway contention from pool host contention.
E2E Report — Issue #11271 (Main CI failed: E2E Tests on 36f2c14)SummaryThe tracked run (34088422718, commit
DiagnosisThe commit cannot be the cause. The failing leg narrows it to one test. Both failures were full-duration runs (9.1–9.4m vs 7.8–10.9m for greens), so a real test failed all three vitest attempts (
Without access to the job log (GitHub returns 403 for unauthenticated log downloads) the failing test cannot be named from a log line; this is the evidence chain instead. The residual alternative in the same file — the 1s attach-latency bound — was weighed and rejected: the Linux pool legs run it under far worse host contention (~30 jobs per host, single fork) and pass. The second leg is not actionable. FixOne commit:
This extends the accepted rationale of #11004 ("N real model round-trips would measure host contention, not the daemon") to the variable the project controls even less: the shared gateway. Note the open PR #11134 covers a different failure class (all-green shard death) at the workflow level; the two are complementary, not overlapping. Verification
Remaining riskIf macOS shard 2/2 reds continue after this lands, the failing test is something else in the 37-file shard and needs the job log to identify; the attach-latency bound in the same baseline file is the next candidate to inspect (argued against above, but not excludable without logs). The 中文说明E2E 报告 — Issue #11271（main 分支 E2E Tests 在 36f2c14 上失败）摘要被跟踪的运行（34088422718，提交
诊断该提交不可能是原因。 失败的腿把范围缩小到一个测试。 两次失败都是完整时长的运行（9.1–9.4 分钟，而绿色运行为 7.8–10.9 分钟），说明某个真实测试在 vitest 的三次尝试（
由于无法获取作业日志（GitHub 对未认证的日志下载返回 403），无法从日志行指认失败的测试；以上为证据链。同一文件中的剩余备选——1 秒的 attach 延迟上限——经过权衡后被排除：Linux 池腿在恶劣得多的主机竞争下（每台主机约 30 个作业、单 fork）运行它并且通过。 第二条腿不可操作。 修复一个提交：
这把 #11004 已接受的论据（"N 次真实模型往返测量的是主机竞争，而不是守护进程"）推广到了项目更无法控制的那个变量：共享网关。注意处于打开状态的 PR #11134 覆盖的是另一种失败类别（全绿分片死亡），在工作流层面；两者互补而不重叠。 验证
剩余风险如果合入后 macOS shard 2/2 继续变红，失败的测试就是该 37 文件分片中的其他测试，需要作业日志来确认；同一基线文件中的 attach 延迟上限是下一个候选（上文已论证排除，但没有日志无法彻底排除）。ECS 池上的 🧠 Handled by Qwen Code · model/模型 |
github-actions
Bot
added
the
review/self-reported
…de case (#11271) The CI-wide prompt-latency skip rested on a misattribution: in both cited runs the latency probe ran and passed, and the macOS leg went red on cli/acp-integration.test.ts > blocks write tools in plan mode — killed by a transient model-serving -32603 returned over ACP, with the retry attempt then tripping ENOTEMPTY on the previous attempt's leftover .qwen-home. Revert the CI skip, re-issue the plan-mode session/prompt on the observed gateway -32603 shape (bounded at 3 attempts, 2s apart, inside the existing per-request timeout), and let the TestRig.setup directory reset retry through the mid-delete refill race the way globalSetup's teardown already does.
|
🤖 Addressed the latest review feedback (round 1/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/10 轮）。改动内容与我反驳保留之处如下： Autofix round — PR #11286 (issue #11271)Outcome: the CI-wide prompt-latency skip is reverted, and the fix is retargeted at the test that actually failed twice — The reviews established from the cited runs' own job logs that the PR's premise did not hold: in both runs (34070970091, 34088422718) the prompt-latency probe ran and passed, while the leg went red on the acp plan-mode case dying on a transient model-serving Per-finding dispositionsR1-1 (rc:3948529249, Critical) — Resolved in code
R1-2 (rc:3948529257) — Resolved in codeThe comment recording the refuted causal claim (probe passed in both cited runs) is deleted by the revert; the restored R1-3 (rc:3948529263) — Resolved in codeThe unconditional CI fallthrough in R1-4 (rc:3948529277) — Resolved in codeConditional on keeping a CI-wide skip; the skip is not kept. The measurement never leaves CI — the macOS legs keep executing and enforcing R1-6 (rc:3948529282) — Resolved in codeThe Review rv:5129647277 — process note acknowledgedThe note that failing job logs are retrievable via Deleted tests (gate advisory context)The three deleted cases ( Conflict notesNo base-branch conflict ( Verification
中文说明Autofix 本轮处理 — PR #11286(issue #11271)结果:CI 全量跳过 prompt 延迟探针的改动已被回退,修复改投到真正失败了两次的测试上—— 两轮 review 依据所引用运行的一手作业日志证明:本 PR 的前提不成立。在这两次运行(34070970091、34088422718)中,prompt 延迟探针都真实运行并通过;把这条腿跑红的是 acp plan-mode 用例——它死于一次通过 ACP 返回的瞬时模型服务 逐项处理结论R1-1(rc:3948529249,Critical)——已在代码中解决
R1-2(rc:3948529257)——已在代码中解决记录被否证因果论断的注释(所引两次运行中探针均通过)随回退删除;恢复后的 R1-3(rc:3948529263)——已在代码中解决
R1-4(rc:3948529277)——已在代码中解决该建议以保留 CI 全量跳过为前提;跳过没有保留。测量从未离开 CI——macOS 腿继续执行并强制 R1-6(rc:3948529282)——已在代码中解决需要固定真值边界的 Review rv:5129647277 —— 流程提示已知悉"失败作业日志可用持有 删除的测试(供门禁 advisory 对照)被删除的三个用例( 冲突说明无基线分支冲突( 验证
🦷 Gate advisory — this round resolves a Critical/Request-changes finding with test-only changes (machine-measured): the bite check cannot verify a test-side fix, so the resolution rests on the round summary alone. · 本轮以纯测试改动解决 Critical/Request-changes 反馈（门自动测量）：bite 检查无法验证测试侧修复，该解决仅以轮次摘要为凭。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 5 selected review thread(s). · 已关闭全部选中的 5 条评审线程。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
yiliang114
changed the title
yiliang114
changed the title
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Approval blockers: none.
Tier: Standard — E2E test isolation fix; two test files modified.
Checked:
- Import contract (cross-file):
fakeToolCall,FakeOpenAIServer,startFakeOpenAIServerexist and are exported fromfake-openai-server.ts(lines 101, 75, 116).fakeServerHostOptions,applyContainerSandboxNoProxyexist intest-helper.ts(lines 149, 157). No missing-export risk. - ACP wire shape
_meta.toolName: Confirmed bybridgeClient.ts:510comment ("ToolCallEmitter stamps_meta.toolName") andtool-call-emitter.ts:180. Old test read top-leveltoolUpdate.toolNamewhich was never populated. status: 'failed': Confirmed intranscript-replay.ts:362(options.success ? 'completed' : 'failed'). Old test checked'error', which is not a valid enum value — the old assertion was always-false when write_file was blocked.content[].content.textshape: Confirmed inbuildToolResultContent(transcript-replay.ts:1502): errorMessage becomes{ type: 'content', content: { type: 'text', text: errorMessage } }.emitProgressUpdateuses the same format. Test'smap(({ content }) => content?.text)correctly extracts the error text.- Unconditional assertion: Old test guarded by
if (writeFileEvents.length > 0)— always passed when LLM never called write_file. New test injects write_file deterministically via fake server, making the assertion unconditional and non-vacuous. - Timing:
await sendRequest('session/prompt')resolves after full agent turn; allsession/updatenotifications already delivered on same stdout stream. Removingawait delay(2000)is correct. - Fake server lifecycle:
fakeServer.close()infinallyblock (acp test) andafterEach(json-output test). Container sandbox proxy env properly restored. - 4th json-output test (auth mismatch): correctly omits
fakeModelArgs()— fails before reaching model service. - Previous reviews R1-1/R2-x: All apply to intermediate commits (sha
180f503,a25ab8c) that touched_prompt-latency-policy.tsand_acp-model-serving-retry.ts. At current head (aa79c7c), net PR diff is two files only; none of the previously criticized changes are present. All prior findings are stale.
Not covered: No production code changed; Linux/Windows execution not validated by author but changes are environment-agnostic (fake server on localhost, no platform-specific paths).
Reviewed with AI assistance.
|
🕐 Review received — an automatic review of the current head is still running, so this round is held until it lands (a push now would cancel it and discard its work, #8888). Your feedback stays queued for the next eligible round. 中文说明🕐 已收到评审 —— 当前 head 上仍有一轮自动 review 在运行，本轮暂缓（现在推送会取消该 review 并丢弃其工作，#8888）。反馈保持排队，等待下一次可运行的轮次处理。 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Review at head aa79c7c1e3 (merge-base a1cbe75cf1). No blocking findings. 3 inline: 1 P2, 2 P3.
The central change checks out, and it is stronger than the description claims. I traced the rewritten plan-mode assertion into production rather than trusting the diff:
isPlanModeBlocked→earlyErrorResponse(packages/cli/src/acp-integration/session/Session.ts:12525-12536) carrying the literalPlan mode is active. The tool "..." cannot be executed(:12527) →ToolCallEmitter.emitError(session/emitters/tool-call-emitter.ts:258-282) →createTranscriptToolCallResultUpdate(packages/acp-bridge/src/transcript-replay.ts:354-381). That builder setsstatus: 'failed'whensuccessis false (:362), placestoolNamein_metathroughbuildUpdateMeta's top-level...options.extraspread (:239 with :368), and carries the message ascontent[0].content.textthroughbuildToolResultContent'serrorMessagebranch (:1502-1509). Every field the new collector reads matches production exactly.- The same read shows the update object has no top-level
toolName, which means the pre-diff test was vacuous rather than merely weak:toolUpdate.toolNamewas alwaysundefined,toolCallEventsstayed empty, and the removedif (writeFileEvents.length > 0)guard meant not one assertion in that block ever executed. Reading_meta.toolNameand dropping the guard turns a dead test into a live one. Worth stating plainly, because it is the strongest argument for this PR and the description frames it only as "a weak assertion". - The strengthened form is non-tautological in both directions: with
permissionHandlerreturningproceed_oncefor non-switch_moderequests (:1003), broken enforcement yields no'failed'write_file event and creates the file, so :1066 and :1074 both go red; a tool call that never arrives also goes red at :1066 instead of passing silently as before. - Dropping
await delay(2000)is safe, not a new race:sessionUpdatesis appended synchronously in the single readline handler and JSON-RPC notifications precede thesession/promptresponse on the same stdout stream, so the blocking update is already collected whensendRequestresolves. setupAcpTestmerges the injected env as{ ...process.env, ...options?.env, QWEN_HOME: qwenHome }(:136), so per-agentQWEN_HOMEisolation cannot be clobbered.joinis imported (:9);rig.testDiris assigned inTestRig.setupbefore the handler closure evaluates; and the trailing absence check at :1072 uses the same path the fake tool call requests at :974.json-output.test.tsis fully isolated, not partially: exactly four tests, three take...fakeModelArgs()(:58, :93, :169), and the fourth (:269) fails pre-flight on enforced auth mismatch before any model request.
Verification gap, disclosed. I executed nothing: this head has no node_modules locally and the machine's root filesystem is at 100% (765 MB free), so no install, build, or test run was possible. CI at this head is 14 SUCCESS / 0 failed / 25 SKIPPED with only review-pr in progress — but per the P2 inline, neither changed file runs on any pull_request lane, so green CI is not evidence these assertions pass. The only execution evidence is the author-reported local macOS run in the PR body.
Stale review state, for whoever lands this. reviewDecision reads CHANGES_REQUESTED from two round-1 bot reviews submitted at head 180f5037bc. Their Critical (ledger R1-1, _prompt-latency-policy.ts:51) targeted the earlier CI-wide latency-skip approach, which the rework abandoned: that file is byte-identical to base in this diff, so the finding is superseded rather than fixed. Those reviews need dismissing, or a fresh bot pass at this head, for the state to clear — chiga0 already approved at aa79c7c1e3.
…#11271) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 2/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/10 轮）。改动内容与我反驳保留之处如下： Autofix round — PR #11286 (issue #11271)Context this roundThe six round-2 bot findings (R2-1, R2-2, R2-3, R2-4, R2-6, R2-7) were posted against the retry-based approach of FindingsResolved in code
Not resolved (replied on the thread, left open)
Non-actionable items
Verification
中文说明Autofix 本轮处理 — PR #11286(issue #11271)本轮背景第 2 轮机器人审查的六条发现(R2-1、R2-2、R2-3、R2-4、R2-6、R2-7)针对的是 发现逐条处理已在代码中解决
未解决(已在对应讨论串回复,保持开启)
无法行动的事项
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 6 selected review thread(s). · 已关闭全部选中的 6 条评审线程。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Local verification report — PR #11286Verified as maintainer against head Verdict: the PR's claims hold, and the case for it is stronger than the description says. One PR-introduced issue found (medium, low CI impact but it makes the reviewer's own check flaky), plus one pre-existing gap the PR could close in the same pass. 1. The assertion this PR replaces could never have firedThe description calls main's assertion "weak … allowed the plan-mode case to pass when the model never attempted the write." It is stronger than that: on I captured the real
Three independent field mismatches — the tool name is at 2. The new assertion is load-bearing; main's is notI mutated product source and re-bundled. Both arms use the PR's deterministic driver, so the model always attempts the write — the most favourable possible conditions for main's assertion.
M2 and M3 are what an ACP client actually feels: the write is refused, but the editor can no longer say why, or is told the call succeeded. And the decisive case — ship a build where plan mode blocks nothing (M1), with a healthy model that answers with a plan instead of calling
3. The CI outage reproduces, and the isolation is measured — not assumedA sentinel stands in for the shared gateway, answers every request
Repeatability with the gateway configured but down — ACP plan mode 15/15, json-output 10/10, 0 gateway requests across all 25 runs. 4. The Linux/container half (marked
|
| Arm | Runs | Assertions | ENOTEMPTY at teardown |
|---|---|---|---|
| PR as submitted | 45 | 45/45 pass | 11 (24%) |
PR + await delay(2000) restored (only difference) |
20 | 20/20 pass | 0 |
main's assertions + PR driver + delay(2000) |
20 | 20/20 pass | 0 |
whole acp-integration.test.ts file, PR arm |
6 | 11/11 ×6 | 0 |
Mechanism, measured from cleanup() returning: +283 ms projects/<slug>/meta.json, +274 ms projects/<slug>/memory/MEMORY.md, +290 ms usage_record.jsonl — then nothing for the next 10 s. setupAcpTest puts the agent's QWEN_HOME inside rig.testDir; cleanup() awaits the agent's exit, but ~300 ms of writes still land there afterwards. On main the 2 s wait — plus a multi-second real-model turn — always covered that window. The PR's turn now finishes in 1.3 s with no wait, so global teardown's recursive rm can start inside it. retry: 2 does not retry teardown errors.
Impact. Low for CI as configured: e2e.yml shards whole files and two more tests follow this one, so the window closes — 0/6 in full-file runs. Real for a reviewer: the isolated -t run, which is the natural way to check this PR, is red about one time in four on macOS.
Suggestion — please don't just put the sleep back. Either create the agent's QWEN_HOME outside rig.testDir (mkdtempSync(join(tmpdir(), 'acp-home-')), cleaned in cleanup()), or have cleanup() wait for the directory to stop changing. Both keep the determinism this PR is buying; a blind 2 s sleep re-adds the thing the PR set out to remove.
F2 — low, pre-existing — case 4 of json-output.test.ts is still coupled to the CI secrets
should return a JSON error for enforced auth mismatch before running never calls the model, but it needs all three of OPENAI_API_KEY + OPENAI_BASE_URL + OPENAI_MODEL to exist so the CLI resolves the current auth type as openai. With any one missing it fails on both arms:
AssertionError: expected 'No auth type is selected. Please conf…' to contain 'configured auth type is qwen-oauth'
Not a regression — but it is the one case in this file the PR's stated goal does not reach, and it closes with the same ...fakeModelArgs() the other three now use (or by setting the three vars explicitly). Worth doing here so the file becomes hermetic as a whole.
F3 / F4 — nits
- F3. The non-streaming branch (
{"selected_memories":[]}) is never exercised in either file. Instrumented ledgers: the ACP case sees exactly 2 requests, both streaming (#0 stream=true msgs=2 tools=30,#1 stream=true msgs=4);json-output.test.tssees 6, all streaming. Harmless as a safety net — just don't read it as covering a live path. - F4.
streamingRequestIndex++ === 0encodes "the first streaming request is the user's turn". True today (measured above). If a streaming side query is ever issued ahead of the turn, the scriptedwrite_filelands on the wrong request and the failure will be confusing. A predicate on the request body (e.g. the last message being the user prompt) would survive that.
Recommendation
Approve after F1. The core of this PR is right and the evidence is stronger than the description claims — it does not merely stabilise a flaky test, it replaces one that could not fail. F1 is the only thing I would hold on, and it is a few lines in setupAcpTest. F2 is a natural companion fix; F3/F4 are optional.
Reproduction
# tree
git worktree add --detach wt11286 3af26895e7 && cd wt11286
npm install && npm run build && npm run bundle
# headline: PR passes with the shared gateway configured but down, and never calls it
OPENAI_API_KEY=k OPENAI_BASE_URL=http://127.0.0.1:<dead>/v1 OPENAI_MODEL=m \
QWEN_HOME=$(mktemp -d) QWEN_SANDBOX=false \
npx vitest run --root ./integration-tests cli/json-output.test.ts --retry=0
QWEN_HOME=$(mktemp -d) QWEN_SANDBOX=false \
npx vitest run --root ./integration-tests cli/acp-integration.test.ts --retry=0 \
-t 'blocks write tools in plan mode'
# F1: repeat the isolated ACP run ~20x with a fresh QWEN_HOME each time and
# grep the output for ENOTEMPTY — ~1 in 4 exits non-zero with every test green.
# mutations (each: edit source, `npm run bundle`, re-run)
# M1 Session.ts:12515 prefix the plan-mode guard with `false &&`
# M2 Session.ts:12527 replace the "Plan mode is active…" message
# M3 transcript-replay.ts:362 status: options.success ? 'completed' : 'failed' -> 'completed'中文说明
PR #11286 本地验证报告
以维护者身份针对 head 3af26895e7 验证。使用独立 worktree、真实打包 CLI（npm run build && npm run bundle → dist/cli.js）、真实 qwen --acp 进程、真实 Docker，以及一个会记录全部请求的共享模型网关替身。被测产品不打桩；每个变异都改产品源码并重新 bundle（每次核对 bundle chunk 哈希，还原后逐字节回到 acpAgent-XBVLQOC3.js）。
结论：PR 的论点成立，而且实际理由比描述里写的更强。 发现 1 个本 PR 引入的问题（中等；对 CI 影响低，但会让评审者自己的验证命令变得不稳定），另有 1 个既有缺口建议在本次一并收口。
1. 被替换掉的那条断言根本不可能触发
描述说 main 的断言"过弱……模型没尝试写入时也能通过"。实际更严重：在 main 上该断言块结构性不可达，对任何模型行为都是如此。
我从真实 ACP agent 抓下 session/update 帧，用同一份 payload 分别跑两个提取器（图 1）：工具名在 _meta.toolName（不在顶层），状态是 "failed"（不是 "error"），原因在 content[0].content.text（不在 error.message）。三处字段全不匹配，所以 writeFileEvents 恒为空，if (writeFileEvents.length > 0) 永不执行，唯一还起作用的只剩 expect(fileExists).toBe(false) —— 而模型什么都没做时它同样通过。
2. 新断言是承重件，main 的不是
改产品源码 + 重新 bundle。两臂都用 PR 的确定性驱动，即模型必然尝试写入 —— 这已经是对 main 断言最有利的条件（图 2）：
| # | 变异 | main 的断言 | PR |
|---|---|---|---|
| M1 | 删除 plan mode 拦截（Session.ts:12515） |
FAIL（仅靠 fileExists） |
FAIL（直接指出缺陷） |
| M2 | 仍然拦截，但拒绝原因被降级（Session.ts:12527） |
PASS —— 看不见 | FAIL —— 抓到 |
| M3 | 仍然拦截，但向客户端报告为 completed（transcript-replay.ts:362） |
PASS —— 看不见 | FAIL —— 抓到 |
M2、M3 正是 ACP 客户端真正会感知到的回归：写入被拒了，但编辑器再也说不出为什么，或者被告知调用成功了。
决定性的一组（图 3）：产品端 plan mode 完全不拦截（M1），模型像平常那样只回一份计划、不调用 write_file —— main 的 blocks write tools in plan mode (issue #1806) 报绿。PR 版本报红并指出缺陷。
3. CI 故障可复现，隔离性是量出来的，不是假设的
替身网关对所有请求返回 500 {"type":"internal_error","code":"model_serving_error"}，并记录每一个到达的请求。两臂的 OPENAI_API_KEY / OPENAI_BASE_URL / OPENAI_MODEL 都指向它，和 CI secrets 一样（图 4）：
| 套件 | 臂 | 结果 | 打到共享网关的请求数 | 耗时 |
|---|---|---|---|---|
| ACP plan mode | main | FAIL —— Request 5 (session/prompt) timed out |
24 | 耗尽 60 s 预算 |
| ACP plan mode | PR | PASS | 0 | 1.3 s |
| json-output（4 例） | main | 3 失败 1 通过 | 84 | 92.2 / 97.3 / 95.4 s |
| json-output（4 例） | PR | 4 通过 | 0 | 1.40 / 1.33 / 1.33 / 1.05 s |
网关配置存在但宕机时的重复性：ACP plan mode 15/15，json-output 10/10，25 次运行累计 0 次网关请求。
4. PR 标 ⚠️ 的 Linux/容器那一半可用
本机 Docker 缺 buildx，仓库自带 sandbox 镜像构建不出来，因此我直接验证同一条可达路径：把本次构建的 dist/cli.js 放进 Linux 容器，用 PR 的同款参数经 host.docker.internal 打宿主机上的 fake server（图 6）。三个 JSON 用例断言的字段全部产生：type:"result"、is_error:false、result:"Paris"、stats。宿主机账本显示预期的两次请求（主轮 + managed-auto-memory-extractor）。
PR 臂另外通过：tsc -p integration-tests/tsconfig.json、eslint、prettier --check。
发现
F1 —— 中等 —— 去掉 await delay(2000) 打开了一个约 300 ms 的 teardown 竞态（图 5）
所有断言都通过，运行仍以非零码退出：
Test Files 1 passed (1)
Tests 1 passed | 10 skipped (11)
⎯⎯⎯⎯⎯⎯⎯ Startup Error ⎯⎯⎯⎯⎯⎯⎯⎯
[Error: ENOTEMPTY: directory not empty, rmdir '.../acp-plan-mode-enforcement/.qwen-home']
归因 —— 各臂只差这个 settle 窗口：
| 臂 | 运行次数 | 断言 | teardown ENOTEMPTY |
|---|---|---|---|
| PR 原样 | 45 | 45/45 通过 | 11（24%） |
PR + 恢复 await delay(2000)（唯一差异） |
20 | 20/20 通过 | 0 |
main 的断言 + PR 驱动 + delay(2000) |
20 | 20/20 通过 | 0 |
整份 acp-integration.test.ts，PR 臂 |
6 | 11/11 ×6 | 0 |
机理（从 cleanup() 返回开始计时）：+283 ms projects/<slug>/meta.json、+274 ms projects/<slug>/memory/MEMORY.md、+290 ms usage_record.jsonl，之后 10 s 内再无新文件。setupAcpTest 把 agent 的 QWEN_HOME 放在 rig.testDir 里面；cleanup() 虽然等到了 agent 退出，但之后仍有约 300 ms 的写入落在该目录。main 上那 2 秒等待（加上真实模型多秒的一轮）始终盖住了这个窗口；PR 这一轮 1.3 s 结束且不再等待，全局 teardown 的递归 rm 就可能落进窗口内。retry: 2 不会重试 teardown 错误。
影响：对现状的 CI 影响低 —— e2e.yml 按整文件分片，本用例后面还有两个用例，窗口会被盖住，整文件运行 0/6。但对评审者是实打实的：单独 -t 跑这个用例（也就是验证本 PR 最自然的方式）在 macOS 上约每四次红一次。
建议 —— 请不要简单把 sleep 加回去。 要么把 agent 的 QWEN_HOME 建在 rig.testDir 之外（mkdtempSync(join(tmpdir(), 'acp-home-'))，在 cleanup() 里清理），要么让 cleanup() 等目录不再变化。两种做法都保住本 PR 换来的确定性；盲等 2 秒等于把 PR 想去掉的东西又加回来。
F2 —— 低，既有问题 —— json-output.test.ts 第 4 个用例仍绑定 CI secrets
should return a JSON error for enforced auth mismatch before running 从不调用模型，但仍然需要 OPENAI_API_KEY + OPENAI_BASE_URL + OPENAI_MODEL 三者都存在，CLI 才会把当前 auth 类型解析成 openai。少任何一个，两臂都失败：
AssertionError: expected 'No auth type is selected. Please conf…' to contain 'configured auth type is qwen-oauth'
不是回归 —— 但这是本文件里 PR 目标唯一没覆盖到的用例，用其他三例已经在用的 ...fakeModelArgs()（或显式设置这三个变量）即可收口。建议一并做掉，让整个文件真正自洽。
F3 / F4 —— nit
- F3：两个文件里非流式分支（
{"selected_memories":[]}）都从未被触发。插桩账本：ACP 用例只有 2 个请求且都是流式（#0 stream=true msgs=2 tools=30、#1 stream=true msgs=4）；json-output.test.ts共 6 个，全部流式。作为兜底无害，只是别把它当成覆盖了真实路径。 - F4：
streamingRequestIndex++ === 0等于把"第一个流式请求就是用户这一轮"写死了。今天成立（上面已实测）。若将来在该轮之前插入一个流式 side query，脚本化的write_file会落到错误的请求上，失败现象会很难懂。改成对请求体做判断（例如最后一条消息是用户 prompt）会更耐改。
建议
修掉 F1 后可以合入。 这个 PR 的核心是对的，而且证据比描述写得更强 —— 它不只是稳住了一个 flaky 测试，而是替换掉了一个根本不会失败的测试。我唯一想卡的只有 F1，改动量是 setupAcpTest 里的几行。F2 建议顺手一起做；F3/F4 可选。
- Move per-agent QWEN_HOME out of rig.testDir: the agent keeps writing there ~300ms after exit, racing global teardown's recursive rm (ENOTEMPTY). cleanup() now removes it with retries. - Drop the plan-mode test's permissionHandler: the plan-mode guard short-circuits before any permission request, so it could never run and its comment claimed coverage the test does not have. - Name the collected tool-call events in the plan-mode assertion so a wire-shape drift is distinguishable from a plan-mode let-through. - Stub loopback NO_PROXY in json-output tests: an inherited HTTP(S)_PROXY otherwise tunnels the fake-server POST and the cases time out. Two-chunk fixture restores multi-delta accumulator coverage. - Make the auth-mismatch case hermetic via fakeModelArgs instead of the ambient OPENAI_* secrets. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 3/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/10 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #11286Commit: Feedback dispositionsInline review findings (round 3)
Maintainer verification report ([ic:5575224215], @wenshao)
Deferred (re-recorded for the follow-up queue)
VerificationEvery command actually run, with its result (all from the repo root unless noted; integration runs used a fresh
Mutation probes (guard deleted/broken → focused tests must fail → restored → green):
中文说明Autofix 本轮总结 — PR #11286提交： 反馈处置行内评审发现（第 3 轮）
维护者验证报告（[ic:5575224215]，@wenshao）
延期（重新登记进后续队列）
验证实际执行过的每条命令及其结果（除注明外均在仓库根目录运行；集成测试均使用全新的
变异探针（删除/破坏守卫 → 聚焦测试必须失败 → 还原 → 复绿）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 4 selected review thread(s). · 已关闭全部选中的 4 条评审线程。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 4/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/10 轮）。改动内容与我反驳保留之处如下： Address review — round 4 (commit d791032)Both round-4 findings were Suggestion-level, verified against the code, and implemented. No conflicts with Feedback points and decisions[rc:3953160954] R4-1 — ACP homes orphaned outside the tmpdir sweeper — IMPLEMENTEDVerified: Fix, staying inside the sweeper's own guards (24h age floor, exact-path exclusion of the live hermetic home,
Mutation probe (required red check): temporarily reverting [rc:3953160964] R4-2 — drifted duplicate of the warn-and-swallow retried
|
Superseded by the current test-only rework; the criticized latency-policy files are no longer in the PR diff.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no code changeOutcome: no action. All feedback in this round is deferred by maintainer decision; the working tree is unchanged and nothing was committed. What this round containedThe round-5 automated review posted four Suggestion-level inline findings. No Critical finding is open, no check is failing, and there is no base conflict. Maintainer @yiliang114 replied to each finding in its own thread, explicitly deferring all four under the repository review-round cap (after roughly five review rounds, land only Critical fixes and move remaining Suggestions to a follow-up — AGENTS.md). Disposition of each findingAll four findings were verified against the code before deferral — they are real, so they are deferred (recorded for the follow-up queue), not declined:
Follow-up handlingEach finding is recorded in VerificationNo code was changed this round, so no build/typecheck/lint/test run was applicable. Verification performed: each finding's claims were checked against the current source at 中文说明Autofix 评审轮次 — 无代码变更结果:无需处理。 本轮所有反馈均已由维护者决定暂缓;工作树未改动,也未提交任何内容。 本轮内容第 5 轮自动评审发布了四条 Suggestion 级别的行内发现。没有未决的 Critical 发现,没有失败的检查,也不存在与基线的冲突。维护者 @yiliang114 已在每条发现所在的线程中逐一回复,依据仓库的评审轮次上限规则(约五轮评审后只合入 Critical 修复,其余 Suggestion 转入后续处理 —— 见 AGENTS.md)明确暂缓了全部四条。 各发现的处置四条发现在暂缓前均已对照代码核实 —— 它们属实,因此处置为延期(记录进后续处理队列),而非拒绝:
后续处理每条发现都已记录进 验证本轮没有代码变更,因此不适用任何构建/类型检查/lint/测试运行。已执行的验证:在 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Approval blockers: none.
Tier: Standard — test-only change; E2E test isolation and determinism fix.
Scope: integration-tests/ only; no production code changed.
Checked:
- ACP_HOME_PREFIX nesting:
qwen-e2e-home-acp-starts withHERMETIC_HOME_PREFIX(qwen-e2e-home-) atglobalSetup.ts:57→sweepLeakedQwenHomesreclaims leaked ACP homes.globalSetup.test.tspins this withTMPDIRredirect soos.tmpdir()returnstmpRootwhen the module is freshly imported. ✓ - wenshao F1 (teardown race):
setupAcpTestnow places the agent QWEN_HOME intmpdir()viamkdtempSync(join(tmpdir(), ACP_HOME_PREFIX))and callsremoveScratchDir(qwenHome)incleanup()— moves it outsiderig.testDirso the ~300 ms post-exit write window no longer races global teardown. ✓ - wenshao F2 (auth mismatch test):
fakeModelArgs()now passed to the auth-mismatch case, providing all three ofOPENAI_API_KEY,OPENAI_BASE_URL,OPENAI_MODEL— test no longer depends on CI secrets. ✓ envoption threading:setupAcpTestspreadsoptions?.envinto spawn at line 141; plan-mode test env vars reach the agent process. ✓- Assertion correctness (
_meta.toolName,status: "failed",content[0].content.text): shape confirmed against wenshao's live capture and corroborated by transcript-replay.ts. Oldif (writeFileEvents.length > 0)guard was structurally unreachable; new unconditional assertion with deterministicwrite_fileinjection is load-bearing. ✓ - Timing:
sendRequestresolves after the full agent turn;session/updatenotifications arrive on the same stdout readline stream ahead of the response. Removingdelay(2000)is correct here (delay's job was to cover the post-exit write race, not notification ordering). ✓ fakeServer?.close(): optional chaining inafterEachhandles the case wherestartFakeOpenAIServerrejects. ✓removeScratchDirdeduplication: extracted toscratch-dir.ts, used in bothglobalSetup.tsandacp-integration.test.ts.scratch-dir.test.tsverifies the non-fatal catch path with a deterministicENOTDIR. ✓- NO_PROXY:
vi.stubEnv/vi.unstubAllEnvspattern is safe; replaces the previousapplyContainerSandboxNoProxymanual restore. ✓ - Cross-check vs yiliang114 P2/P3: P2 (integration lane not running this file) is informational; P3 (positional coupling on
streamingRequestIndex) and P3 (fakeServer?.close()) — the latter is fixed; the former is acknowledged and deferred under the project's review-round cap. No items require re-opening.
Not covered: production code unchanged; Linux/Windows execution not validated locally, but the change uses mkdtempSync and join(tmpdir(), ...) which are platform-agnostic, and wenshao's verification ran the container path successfully.
Reviewed with AI assistance.
Local verification report — PR #11286, round 2 (delta since
|
| Arm | Result |
|---|---|
3af26895e7 |
no test ever reported; I killed it at my own 600 s timeout |
d791032ba5 |
Tests 4 passed (4), 9.99 s |
Before this round the file called applyContainerSandboxNoProxy(), which is a no-op on every non-container lane, so an inherited proxy took the whole file down. Worth recording, because the reviewer thread treated this as a tidy-up.
3. F1 is closed structurally — and closing it showed what was actually wrong
The home now comes from mkdtempSync(join(tmpdir(), ACP_HOME_PREFIX)) instead of join(rig.testDir, '.qwen-home'), exactly the fix I asked for. Measured with KEEP_OUTPUT=true:
3af26895e7:.integration-tests/<ts>/acp-plan-mode-enforcement/.qwen-home/present and populated — inside the tree teardown recursively removes.d791032ba5: no.qwen-homeanywhere underrunDir.
Honest caveat: the ENOTEMPTY rate I reported last round (11/45) did not reproduce on my box today — 0/25 unloaded and 0/20 pinned to one core, on the pre-fix arm. The structural argument stands regardless, and what follows is a stronger version of the same evidence.
N1 — non-blocking — the per-agent home is re-created right after removeScratchDir() removes it, so nearly every run leaks one into the OS temp dir
The spawned node dist/cli.js --acp is a wrapper: packages/cli/src/llm.tsx:729-731 relaunches the app in a child so it can be restarted internally. waitForExit() resolves on the wrapper's exit; the child is reparented to ppid=1 and keeps running. A 50 ms process sampler over one run:
PID 122654 ppid=122569 (vitest worker) 755.191 s → 757.359 s ← spawn() target; waitForExit() resolves here
PID 122738 ppid=122654 → 1 755.718 s → 757.418 s + ← the real agent, still alive
A 5 ms watcher on TMPDIR over the same run:
4554 ms CREATE qwen-e2e-home-acp-7VDO1X
6767 ms DELETE qwen-e2e-home-acp-7VDO1X ← removeScratchDir succeeds; no warning is printed
6778 ms CREATE qwen-e2e-home-acp-7VDO1X ← +11 ms, re-created by the ppid=1 survivor
The /proc census taken at that second CREATE names the survivor directly, and the re-created home holds usage_record.jsonl. So the retries in removeScratchDir never fire — the rm works, the directory just comes back.
| Arm | CPU | Runs | Assertions | homes left in TMPDIR |
|---|---|---|---|---|
3af26895e7 |
16 cores / 1 core | 25 / 20 | all pass | n/a — home lives in runDir |
| PR as submitted | 16 cores | 25 | 25/25 pass | 25 |
| PR as submitted | 1 core | 20 | 20/20 pass | 10 |
PR + QWEN_CODE_NO_RELAUNCH: '1' |
16 cores | 10 | 10/10 pass | 0 |
PR + QWEN_CODE_NO_RELAUNCH: '1' |
1 core | 12 | 12/12 pass | 0 |
Whole-file runs accumulate 2 per run here (8 failed | 3 passed — see N3; the eight that die at authenticate never complete a turn, so on a green CI lane the count would be higher). Negative control: deleting await removeScratchDir(qwenHome); gives exactly one leak per run, monotone 1→2→3 — the call is load-bearing, it just loses the race with the survivor.
Why this is not blocking. Each home is 36 KB, mode drwx------, and holds no credentials (settings.json is only {"security":{"auth":{"selectedType":"openai"}}}). Nothing turns red. And round 4's own sweepLeakedQwenHomes() reclaims them — I verified the nesting and the age floor in §4.
Why it is still worth a line. The round-4 rationale describes the leak source as "a worker torn down mid-test never runs the cleanup". That is not the dominant case: the ordinary all-green path leaks too — 35/35 unloaded runs here, and 10/20 when pinned to a single core. So the 24 h sweeper is now carrying the normal path rather than acting as a safety net, and cleanup() returning does not mean the agent is gone.
Fix I validated: adding QWEN_CODE_NO_RELAUNCH: '1' to setupAcpTest's agent env — the escape hatch the CLI already honours (llm.tsx:461) — makes the spawned process be the agent, so waitForExit() means what the test assumes. 0 leaks across 22 runs on two load profiles, and the whole-file failure set is byte-identical (8 failed | 3 passed, same names). The tradeoff to weigh: it stops these cases exercising the relaunch wrapper, which is a real production path for an ACP client. If you would rather keep that covered, the other shape is to poll the home until it stops changing before removing it.
4. Round 4's sweeper checks out
- The prefix nesting is genuinely pinned. Mutating
ACP_HOME_PREFIXtoqwen-acp-home-(outsideHERMETIC_HOME_PREFIX) turns the newglobalSetup.test.tscase red; as submitted it is green. The witness is not decorative. - The 24 h floor cannot reap a run in flight. The PR only tests the deletion direction, so I probed the other one against the real
setup(): a home 30 minutes old survives, one 23 h 59 m old survives, one 25 h old is swept. A concurrent E2E run on the same host is safe. - What is not pinned (already reported as
rc:3953916454and deferred under the round cap — this just confirms the deferral is factually right):maxRetries: 5 → 0inscratch-dir.tsleavesscratch-dir.test.tsgreen, because its only case pins the swallow-and-warn contract through a non-retryableENOTDIR.
Also clean on the PR arm: tsc -p integration-tests/tsconfig.json exit 0, eslint --max-warnings 0 on all six changed files exit 0, prettier --check clean, globalSetup.test.ts 5/5, scratch-dir.test.ts 1/1, json-output.test.ts 4/4.
N2 — nit — the multi-delta fixture buys real coverage that nothing would miss
Round 3's { content: 'Paris' } → { contentChunks: ['Par', 'is'] } is a genuine improvement, measured on the wire through the real CLI: 1 content_block_delta (["Paris"]) becomes 2 (["Par","is"]). But json-output.test.ts:238 is expect(eventTypes).toContain('content_block_delta'), which one delta satisfies — reverting the fixture leaves all four cases green. One line closes it:
expect(eventTypes.filter((t) => t === 'content_block_delta').length).toBeGreaterThan(1);N3 — context, not a defect — 8 of the 11 ACP cases still depend on the shared gateway
Running the whole file with no model credentials gives 8 failed | 3 passed — identically on both arms, same test names, so this is not a regression. The eight die at authenticate({methodId:'openai'}) → Internal error. Worth stating plainly because the PR title is "isolate protocol checks from model service": it isolates the one case that was breaking main, and the rest of the file is still exposed to exactly the outage this PR was opened for. That is a reasonable scope, not a gap in the change — just don't read the file as isolated.
Also checked
- R3-1 (dropping
permissionHandler) is safe. I instrumented the collector and ran the plan-mode case: zerosession/request_permissionframes. The plan-mode guard short-circuits before the permission round-trip, exactly as the round-3 report claims, so the defaultproceed_oncehandler is never consulted. - The Linux/container half.
packages/cli/src/serve/sandbox.tsexplicitly volume-mounts both the resolvedQWEN_HOME(:503-505) andos.tmpdir()at the same path (:536), so moving the home into the temp dir does not put it out of the sandbox's reach. I could not finish building the repo's sandbox image inside this session's budget, so that half is a code trace, not an execution.
Recommendation
Merge. Both blocking-ish items from my first pass are fixed and the fixes hold up under measurement rather than inspection. N1 is real and I would take the one-line version of it, but it leaks 36 KB of non-sensitive scratch that this PR's own sweeper already reclaims — that is follow-up material, not a merge gate, especially with the review round cap in play. N2/N3 are notes.
Reproduction
git worktree add --detach wt11286 d791032ba5 && cd wt11286
# product source is identical to 3af26895e7, so one bundle serves both arms
npm install && npm run build && npm run bundle
# F2: no OPENAI_* set anywhere
HOME=$(mktemp -d) QWEN_SANDBOX=false \
npx vitest run --root ./integration-tests cli/json-output.test.ts --retry=0
# d791032ba5 -> 4 passed ; 3af26895e7 -> 1 failed (enforced auth mismatch)
# NO_PROXY: dead proxy that accepts and never answers
python3 -c "import socket;s=socket.socket();s.bind(('127.0.0.1',39997));s.listen(50);
[s.accept() for _ in iter(int,1)]" &
HTTP_PROXY=http://127.0.0.1:39997 HTTPS_PROXY=http://127.0.0.1:39997 \
HOME=$(mktemp -d) QWEN_SANDBOX=false timeout 600 \
npx vitest run --root ./integration-tests cli/json-output.test.ts --retry=0
# N1: leak count over N runs with a private TMPDIR
T=$(mktemp -d); for i in $(seq 1 10); do
TMPDIR=$T HOME=$(mktemp -d) QWEN_SANDBOX=false \
npx vitest run --root ./integration-tests cli/acp-integration.test.ts \
--retry=0 -t 'blocks write tools in plan mode' >/dev/null 2>&1
done; ls -d $T/qwen-e2e-home-acp-* | wc -l # 10 as submitted, 0 with QWEN_CODE_NO_RELAUNCH=1
# sweeper mutation
sed -i "s/'qwen-e2e-home-acp-'/'qwen-acp-home-'/" integration-tests/scratch-dir.ts
npx vitest run --root ./integration-tests globalSetup.test.ts -t 'sweeps a stale per-agent ACP home'中文说明
PR #11286 本地验证报告 —— 第 2 轮（3af26895e7 之后的增量）
第二次以维护者身份验证。第一轮报告覆盖的是 3af26895e7 上的 base↔PR 对比；本轮只看之后新增的 4a3c61b270（round 3）和 d791032ba5（round 4），并排查这两轮是否引入新问题。环境与上轮相同：独立 worktree、真实打包 CLI（npm run build && npm run bundle → dist/cli.js）、真实 qwen --acp 进程，被测产品不打任何桩。
有一点让 A/B 变得非常干净：git diff 3af26895e7 d791032ba5 -- packages/ scripts/ package.json 为空，因此一份 bundle 可同时服务两臂，下文测出的每一处差异都只来自测试代码。
结论：我上一轮提出的两个问题都已修复，而且都能实测出来。可以合入。 新发现 1 个非阻塞问题——它与其说是 F1 修复引入的，不如说是被它暴露出来的：底层是同一个缺陷，只是从"跑红"变成了"临时目录泄漏"。另有 2 条较小的说明。
1. F2 已收口 —— json-output.test.ts 现在是自洽的
在环境里完全没有 OPENAI_API_KEY / OPENAI_BASE_URL / OPENAI_MODEL 的前提下，第四个用例（enforced auth mismatch）在 3af26895e7 上失败、在 PR head 上通过。同一台机器、同一份 bundle、同样全新的 HOME/TMPDIR。这原本是本文件里 PR 目标唯一没覆盖到的用例，现在覆盖到了。
2. R3-3 的 NO_PROXY 改动价值高于评审线程里的定位
同见上图。我给两臂都配了一个死代理——接受连接但永不应答，正是 CI 或企业出口代理卡死时的形态——其余完全一致：
| 臂 | 结果 |
|---|---|
3af26895e7 |
一个用例都没有报告结果；被我自己的 600s 超时杀掉 |
d791032ba5 |
Tests 4 passed (4)，9.99 秒 |
本轮之前该文件调用的是 applyContainerSandboxNoProxy()，它在所有非容器 lane 上是空操作，所以一个继承来的代理就能把整个文件拖死。值得记一笔，因为评审线程把它当成了顺手整理。
3. F1 已从结构上收口 —— 而且收口过程暴露了真正的问题
home 现在由 mkdtempSync(join(tmpdir(), ACP_HOME_PREFIX)) 创建，不再是 join(rig.testDir, '.qwen-home')，正是我上轮建议的做法。用 KEEP_OUTPUT=true 实测：
3af26895e7：.integration-tests/<ts>/acp-plan-mode-enforcement/.qwen-home/存在且有内容 —— 就在 teardown 递归删除的那棵树里。d791032ba5：runDir下不存在任何.qwen-home。
需要如实说明： 我上轮报告的 ENOTEMPTY 概率（11/45）今天在我这台机器上没有复现 —— 修复前那一臂无负载 0/25、单核绑定 0/20。结构性论证不受影响，而且下面的证据比概率统计更强。
N1 —— 非阻塞 —— removeScratchDir() 删掉 home 之后它立刻被重建，因此几乎每次运行都会往系统临时目录泄漏一个
测试 spawn 出来的 node dist/cli.js --acp 只是个外壳：packages/cli/src/llm.tsx:729-731 会把 app 重新拉起到子进程里，以便内部重启。waitForExit() 等到的是外壳的退出；子进程被 reparent 到 ppid=1 后继续存活。一次运行的 50ms 进程采样：
PID 122654 ppid=122569 (vitest worker) 755.191s → 757.359s ← spawn() 的目标；waitForExit() 在此返回
PID 122738 ppid=122654 → 1 755.718s → 757.418s + ← 真正的 agent，仍然活着
同一次运行、对 TMPDIR 的 5ms 监视：
4554 ms CREATE qwen-e2e-home-acp-7VDO1X
6767 ms DELETE qwen-e2e-home-acp-7VDO1X ← removeScratchDir 删除成功，没有打印任何告警
6778 ms CREATE qwen-e2e-home-acp-7VDO1X ← +11ms，被 ppid=1 的幸存进程重建
在第二次 CREATE 时刻做的 /proc 普查直接点出了这个幸存进程，重建出来的 home 里放着 usage_record.jsonl。也就是说 removeScratchDir 的重试根本没被触发 —— rm 是成功的，只是目录又回来了。
| 臂 | CPU | 运行次数 | 断言 | TMPDIR 中残留 home |
|---|---|---|---|---|
3af26895e7 |
16 核 / 单核 | 25 / 20 | 全部通过 | 不适用 —— home 在 runDir 里 |
| PR 原样 | 16 核 | 25 | 25/25 通过 | 25 |
| PR 原样 | 单核 | 20 | 20/20 通过 | 10 |
PR + QWEN_CODE_NO_RELAUNCH: '1' |
16 核 | 10 | 10/10 通过 | 0 |
PR + QWEN_CODE_NO_RELAUNCH: '1' |
单核 | 12 | 12/12 通过 | 0 |
整文件运行在我这里每次累积 2 个（8 failed | 3 passed，见 N3；那 8 个在 authenticate 阶段就挂了，从未完成一轮对话，所以在 CI 全绿的 lane 上这个数字会更高）。负对照：删掉 await removeScratchDir(qwenHome); 后每次运行恰好泄漏 1 个，单调 1→2→3 —— 这行是承重的，只是它跑不赢那个幸存进程。
为什么不阻塞。 每个 home 36KB，权限 drwx------，不含任何凭证（settings.json 只有 {"security":{"auth":{"selectedType":"openai"}}}）。不会让任何用例变红。而且 round 4 自己加的 sweepLeakedQwenHomes() 会回收它们 —— 嵌套前缀和年龄下限我在第 4 节都验过了。
但仍然值得写一行。 round 4 的说明把泄漏来源描述为"worker 在测试中途被拆掉，cleanup 没跑"。这并不是主要情形：全绿的正常路径同样会泄漏，在我这里无负载 35/35 次运行全部泄漏，单核绑定时 10/20。也就是说那个 24 小时清扫器现在承担的是常规路径，而不是兜底；并且 cleanup() 返回并不代表 agent 已经消失。
我实测通过的修法： 在 setupAcpTest 的 agent env 里加 QWEN_CODE_NO_RELAUNCH: '1' —— CLI 本来就认这个开关（llm.tsx:461）—— 这样 spawn 出来的进程就是 agent 本身，waitForExit() 才是测试以为的那个语义。两种负载下共 22 次运行零泄漏，整文件失败集合逐条一致（8 failed | 3 passed，同名）。需要权衡的代价：这些用例将不再覆盖 relaunch 外壳，而对 ACP 客户端来说那是真实的生产路径。如果希望保留这层覆盖，另一种形态是在删除前轮询 home 直到它不再变化。
4. round 4 的清扫器没问题
- 前缀嵌套是真的被钉住了。 把
ACP_HOME_PREFIX变异成qwen-acp-home-（跑出HERMETIC_HOME_PREFIX之外）会让新增的globalSetup.test.ts用例变红；原样则绿。这个见证不是摆设。 - 24 小时下限不会误删在跑的运行。 PR 只测了删除方向，我对着真实的
setup()补测了另一个方向：30 分钟前的 home 存活、23 小时 59 分的存活、25 小时的被清扫。同宿主机上并发的 E2E 运行是安全的。 - 没有被钉住的部分（已在
rc:3953916454报告过并按轮次上限延后处理 —— 这里只是确认该延后判断在事实层面成立）：把scratch-dir.ts的maxRetries: 5改成0，scratch-dir.test.ts依然全绿，因为它唯一的用例是用不可重试的ENOTDIR去钉"吞掉异常并告警"这个契约。
PR 臂另外通过：tsc -p integration-tests/tsconfig.json 退出 0、六个改动文件的 eslint --max-warnings 0 退出 0、prettier --check 干净、globalSetup.test.ts 5/5、scratch-dir.test.ts 1/1、json-output.test.ts 4/4。
N2 —— nit —— 多 delta 夹具带来的覆盖是真的，但没有任何断言会发现它消失
round 3 把 { content: 'Paris' } 改成 { contentChunks: ['Par', 'is'] } 确实是改进，我在真实 CLI 上从线上量了：content_block_delta 从 1 个（["Paris"]）变成 2 个（["Par","is"]）。但 json-output.test.ts:238 是 expect(eventTypes).toContain('content_block_delta')，1 个就满足 —— 把夹具改回去，四个用例照样全绿。一行即可收口：
expect(eventTypes.filter((t) => t === 'content_block_delta').length).toBeGreaterThan(1);N3 —— 是背景说明而非缺陷 —— 11 个 ACP 用例里仍有 8 个依赖共享网关
不带任何模型凭证跑整文件得到 8 failed | 3 passed —— 两臂完全一致、用例名相同，所以不是回归。这 8 个都挂在 authenticate({methodId:'openai'}) → Internal error。之所以要写明，是因为 PR 标题是"isolate protocol checks from model service"：它隔离的是那个把 main 弄红的用例，文件里其余部分仍然暴露在本 PR 想解决的那类故障之下。这个范围是合理的，不算改动缺口 —— 只是别把整个文件当成已隔离。
另外核对过的
- R3-1（删掉
permissionHandler）是安全的。 我给收集器插了桩再跑 plan mode 用例：session/request_permission帧数为 0。plan mode 守卫在权限往返之前就短路了，与 round 3 报告的说法一致，所以默认的proceed_once处理器根本不会被用到。 - Linux / 容器那一半。
packages/cli/src/serve/sandbox.ts会显式把解析后的QWEN_HOME（:503-505）和os.tmpdir()（:536，映射到同一路径）挂进容器，所以把 home 挪到临时目录不会让它在 sandbox 里不可达。本次会话预算内我没能把仓库自带的 sandbox 镜像构建完，因此这一半是代码追踪，不是实跑。
建议
可以合入。 我上一轮提的两个问题都已修复，而且是实测站得住、不是看代码推断出来的。N1 是真问题，我个人会采纳那个一行版本的修法，但它泄漏的是 36KB 非敏感草稿数据，而且本 PR 自带的清扫器已经会回收 —— 属于后续跟进，不构成合入门槛，尤其是在评审轮次上限已经生效的情况下。N2/N3 是提示性说明。
🤖 Generated with Claude Code — Claude Opus 5 (1M context)
yiliang114
changed the title
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The current head keeps the change test-only, covers both deterministic protocol checks and ACP scratch-home lifecycle, has no unresolved review threads, and the required CI checks pass.
4 participants
What this PR does
Makes the ACP plan-mode authorization and JSON output formatting E2E coverage deterministic by serving fixed OpenAI-compatible responses locally. The plan-mode case now always requests a file write, verifies that ACP rejects it because plan mode is active, and verifies that no file is created. The JSON and streaming JSON cases continue to exercise the real CLI output pipeline without contacting the shared model service.
Each spawned ACP agent also receives an independent QWEN_HOME under the OS temporary directory. Cleanup retries tolerate late post-exit writes, and global setup reclaims abandoned test homes older than 24 hours.
Why it's needed
Main E2E run 34123176225 failed because the shared model service returned
-32603/ model-serving errors. These cases verify local permission enforcement and output serialization rather than model quality, so a gateway outage should not fail them.The retry also exposed an
ENOTEMPTYteardown race: the ACP agent can keep writing to its home briefly after exit. Keeping that home outside the per-test workspace prevents late writes from racing workspace cleanup, while bounded best-effort cleanup avoids turning an otherwise green run red.Reviewer Test Plan
How to verify
Confirm that the plan-mode scenario deterministically attempts a write, receives a failed tool update containing the plan-mode rejection, and leaves the target file absent. Confirm that JSON and streaming JSON output remain parseable, report success, and contain the fixed
Parisresponse without contacting the shared model service. Confirm that ACP agents receive distinct temporary homes, cleanup failures warn without failing the run, and only abandoned homes older than 24 hours are swept.Evidence (Before & After)
Before: run 34123176225 failed the ACP and JSON-output cases when the shared model service returned internal errors; the ACP retry could then fail teardown with
ENOTEMPTY.After: the focused ACP and JSON-output cases pass against the local fake service, and the scratch-home cleanup behavior is covered by focused tests. The current head passes the repository CI checks.
Tested on
Environment (optional)
macOS, no sandbox, local OpenAI-compatible fake server.
Risk & Scope
Linked Issues
References #11271
Fixes #11307
中文说明
这个 PR 做了什么
使用固定的本地 OpenAI 兼容响应，让 ACP plan mode 权限校验和 JSON 输出格式 E2E 测试变得确定。Plan mode 用例现在会必然请求写入文件，验证 ACP 因 plan mode 生效而拒绝该工具调用，并验证文件没有被创建。JSON 和流式 JSON 用例继续覆盖真实的 CLI 输出链路，但不再访问共享模型服务。
每个启动的 ACP agent 也会获得位于系统临时目录下的独立 QWEN_HOME。清理过程使用有限重试来容忍退出后的延迟写入，全局 setup 会回收超过 24 小时的遗留测试 home。
为什么需要
Main E2E run 34123176225 因共享模型服务返回
-32603/ model-serving 错误而失败。这些用例验证的是本地权限约束和输出序列化，而不是模型质量，因此不应因网关故障失败。重试还暴露了一个
ENOTEMPTY清理竞态：ACP agent 退出后仍可能短暂写入自己的 home。将该 home 放在单个测试 workspace 之外，可以避免延迟写入与 workspace 清理竞争；有限的 best-effort 清理也不会把原本绿色的测试变红。Reviewer 测试计划
如何验证
确认 plan mode 场景会确定地尝试写入，收到包含 plan mode 拒绝信息的失败工具更新，并且目标文件不存在。确认 JSON 和流式 JSON 输出仍然可解析、报告成功并包含固定的
Paris响应，同时不访问共享模型服务。确认 ACP agent 使用不同的临时 home，清理失败只告警而不会让测试失败，并且只有超过 24 小时的遗留 home 会被回收。证据（修改前后）
修改前：run 34123176225 在共享模型服务返回内部错误时，ACP 和 JSON-output 用例失败；ACP 重试随后还可能因
ENOTEMPTY导致清理失败。修改后：定向 ACP 和 JSON-output 用例通过本地假服务运行，临时 home 的清理行为也有定向测试覆盖。当前 head 的仓库 CI 检查通过。
测试平台
环境（可选）
macOS，无 sandbox，本地 OpenAI 兼容假服务。
风险与范围
关联 Issue
References #11271
Fixes #11307