fix(integration-tests): ack daemon tool-guard handshake in the mock ACP child (#9159) - #9161
Conversation
…CP child (#9159) The cross-worktree Git guard made the daemon's built-in tool guard unconditional, so the bridge now refuses any ACP child that does not acknowledge the required guard handshake during initialize. The mock ACP child used by the live-journal recovery E2E tests never acked, failing every session it served with "ACP child did not acknowledge the required external tool guard" on all E2E platforms and sandbox legs. Mirror the production child contract in the mock: consume the private guard marker and return the ready acknowledgment in the initialize response when the daemon requires the guard.
E2E Failure Analysis — Issue #9159 (E2E Tests on
|
github-actions
Bot
added
the
review/self-reported
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. The daemon's built-in tool guard is now unconditional — Direction: aligned — this repairs Size: not applicable — one file under Approach: the minimal fix, and the right one. The fixture imports the handshake constants from Risk: no elevated risk signals — no high-risk paths from the revert-history analysis are touched. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 已观测到的问题，而非理论性担忧。daemon 内置工具守卫现在是无条件启用的—— 方向： 对齐——这是修复 规模： 不适用—— 方案： 最小且正确的修复。夹具从 风险： 无升级风险信号——未触及 revert 历史分析中的任何高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review — independent proposal first: the daemon enforces the guard handshake on every child it spawns, so the only correct fix is to make the mock child perform the same handshake the production child does — consume the private marker env var, delete it, and ack via initialize What I verified against the code at the reviewed commit:
No blockers found. No AGENTS.md violations — the one added comment explains the non-obvious contract (why), which is the convention. Testing — unattended CI run: I do not execute PR code; this section quotes the PR's own CI on the reviewed commit. One structural fact matters here: the E2E workflow ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the remaining gap: Not verified: the E2E outcome on this branch — no PR-side CI lane exists for it (see above); the 中文说明代码审查 —— 先说独立方案：daemon 对每个派生的子进程强制守卫握手，因此唯一正确的修复就是让 mock 子进程执行与生产子进程相同的握手——消费私有标记环境变量、删除它、并通过 initialize 的 在审查提交上逐项核实：
未发现阻塞项。无 AGENTS.md 违规——新增的唯一注释解释了非显而易见的契约（why），符合约定。 测试 —— 无人值守 CI 运行：我不执行 PR 代码；本节引用该 PR 在审查提交上的自身 CI。一个结构性事实很重要：E2E 工作流（ 表格区域（机器可读，CI 稳定后由 finalize 工作流原地更新）：Linux 单元测试进行中；macOS/Windows 测试与集成任务按设计仅合并队列运行故跳过；Classify PR、依赖 CVE 审计、Desktop Shell、密钥扫描均已通过。 沙箱验证可以补足剩余缺口： 未验证：本分支上的 E2E 结果——PR 侧不存在对应 CI 通道（见上）； — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. The independent proposal for this problem — read the marker, delete it, ack with the shared constants — is exactly what the diff does, and the alternatives all make things worse: exempting the mock from the guard defeats its fail-closed purpose, skipping the tests hides the signal, and hardcoding the values invites drift. Every link in the handshake chain checks out in code at the reviewed commit: the daemon injects the marker unconditionally, the bridge enforces the ack with the exact error from the On the skepticism check: the problem is objectively real (red Approval deferred until CI lands green on 中文说明置信度：5/5 —— 每个阶段都干净；会毫不犹豫地合入。 针对此问题的独立方案——读取标记、删除标记、用共享常量确认——与 diff 完全一致，而所有替代方案都更糟：豁免 mock 会破坏失败即关闭的设计，跳过测试会掩盖信号，硬编码取值会引入漂移。在审查提交上，握手链条的每一环都经得起核对：daemon 无条件注入标记、桥接以 关于审慎性检查：问题客观存在（ 审批将推迟到 CI 在审查提交 — Qwen Code · qwen3.8-max Reviewed at |
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. |
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 — CI landed green after the review. ✅
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): "This PR fixes broken daemon E2E tests by making the mock…": none — all checks I started were completed (~10 of ~32 calls).; "This PR fixes broken daemon E2E tests by making the mock…": none — all planned checks completed within budget.; "This PR fixes broken daemon E2E tests by making the mock…": none — all checks I started were completed within budget..
Test Plan (not a blocker): cli/qwen-serve-streaming.test.ts — no such file or directory; cli/qwen-serve-client-mcp.test.ts — no such file or directory; cli/mock-acp-typecheck.test.ts — no such file or directory.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："This PR fixes broken daemon E2E tests by making the mock…"：none — all checks I started were completed (~10 of ~32 calls).；"This PR fixes broken daemon E2E tests by making the mock…"：none — all planned checks completed within budget.；"This PR fixes broken daemon E2E tests by making the mock…"：none — all checks I started were completed within budget.。
Test Plan（非阻断）：cli/qwen-serve-streaming.test.ts — no such file or directory; cli/qwen-serve-client-mcp.test.ts — no such file or directory; cli/mock-acp-typecheck.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const externalToolGuardRequired = | ||
| process.env[PRIVATE_EXTERNAL_TOOL_GUARD_ENV] === | ||
| EXTERNAL_TOOL_GUARD_REQUIRED_VALUE; |
There was a problem hiding this comment.
[Suggestion] R1-1: The handshake fix is correct, but the mock still lacks a handler for the qwen/control/session/close ext method — Failure scenario: run QWEN_LOADTEST_ENABLED=1 npx vitest run --config integration-tests/vitest.loadtest.config.ts (measured at this PR's HEAD): rapid lifecycle, SSE slow consumer eviction, and Last-Event-ID reconnect each fail with DaemonHttpError: DELETE /session/:id: "Method not found": qwen/control/session/close, and burst fails expected 10 to be +0 because all 10 sessions hit the same missing handler via closeSession. Pre-existing (before this PR these same tests failed earlier, at session create) and not CI-gated — non-blocking. Suggested follow-up: register a no-op handler in the mock for the qwen/control/session/close ext method returning {}, mirroring the production child (the SERVE_CONTROL_EXT_METHODS.sessionClose case in packages/cli/src/acp-integration/acpAgent.ts). Note (verified by probe): adding this handler is necessary but not sufficient for a fully green loadtest — with the handler in place, SSE slow consumer triggers eviction through HTTP then fails one layer deeper (expect(evicted).toBe(true) received false), previously masked because closeSession threw first.
中文说明
握手修复本身是正确的，但 mock 仍然缺少 qwen/control/session/close ext method 的处理器 —— 失败场景：运行 QWEN_LOADTEST_ENABLED=1 npx vitest run --config integration-tests/vitest.loadtest.config.ts（在本 PR HEAD 上实测）：rapid lifecycle、SSE slow consumer eviction、Last-Event-ID reconnect 均以 DaemonHttpError: DELETE /session/:id: "Method not found": qwen/control/session/close 失败，burst 因 10 个会话全部经由 closeSession 命中同一缺失处理器而以 expected 10 to be +0 失败。此为既有问题（本 PR 之前这些测试在更早的会话创建阶段即失败），且不受 CI 门控 —— 不阻塞合并。建议后续跟进：在 mock 中为 qwen/control/session/close ext method 注册一个返回 {} 的 no-op 处理器，镜像生产子进程（packages/cli/src/acp-integration/acpAgent.ts 中的 SERVE_CONTROL_EXT_METHODS.sessionClose 分支）。注意（经探针验证）：添加该处理器是必要但不充分条件——有了该处理器后，SSE slow consumer triggers eviction through HTTP 会在更深一层失败（expect(evicted).toBe(true) 收到 false），此前该失败被 closeSession 先行抛出所掩盖。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
🤖 Addressed the latest review feedback (round 1/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/10 轮）。改动内容与我反驳保留之处如下： Autofix review-feedback round — PR #9161 (issue #9159)Feedback points and dispositions[Suggestion] R1-1 ( Reproduced first, then fixed:
Known remaining failure (documented by the finding itself, out of scope for this PR): with the handler in place, Review-body note "Test Plan (not a blocker)" — file-not-found for Conflict: none ( Changed files
Committed as Note on environment repair: at round start several package build outputs were missing ( VerificationCommands actually run this round, in order:
中文说明Autofix 审查反馈轮次 — PR #9161（issue #9159）反馈点及处理[Suggestion] R1-1（ 先复现、后修复：
已知遗留失败（finding 本身已记录，超出本 PR 范围）： 处理器就位后， 审查正文中的 "Test Plan (not a blocker)" —— 冲突： 无（ 变更文件
已提交为 环境修复说明：本轮开始时若干包的构建产物缺失（ 验证本轮实际运行的命令（按顺序）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 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/模型 |
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): "本 PR 仅改动测试夹具 integration-tests/fixtures/mock-acp-child/agen…": node_modules is not installed in this review worktree, so I could not execute the three consuming integration tests, nor read @agentclientprotocol/sdk@0.14 's …; "本 PR 仅改动测试夹具 integration-tests/fixtures/mock-acp-child/agen…": did not run the integration tests ( integration-tests recommended) — no node_modules in the worktree and install+build+bundle exceeds this review's budget; v…; "本 PR 仅改动测试夹具 integration-tests/fixtures/mock-acp-child/agen…": could not read @agentclientprotocol/sdk source to confirm unknown-method dispatch to agent.extMethod (no node_modules); relied on the production child's ide….
Test Plan (not a blocker): cli/qwen-serve-streaming.test.ts — no such file or directory; cli/qwen-serve-client-mcp.test.ts — no such file or directory; cli/mock-acp-typecheck.test.ts — no such file or directory.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："本 PR 仅改动测试夹具 integration-tests/fixtures/mock-acp-child/agen…"：node_modules is not installed in this review worktree, so I could not execute the three consuming integration tests, nor read @agentclientprotocol/sdk@0.14 's …；"本 PR 仅改动测试夹具 integration-tests/fixtures/mock-acp-child/agen…"：did not run the integration tests ( integration-tests recommended) — no node_modules in the worktree and install+build+bundle exceeds this review's budget; v…；"本 PR 仅改动测试夹具 integration-tests/fixtures/mock-acp-child/agen…"：could not read @agentclientprotocol/sdk source to confirm unknown-method dispatch to agent.extMethod (no node_modules); relied on the production child's ide…。
Test Plan（非阻断）：cli/qwen-serve-streaming.test.ts — no such file or directory; cli/qwen-serve-client-mcp.test.ts — no such file or directory; cli/mock-acp-typecheck.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| PRIVATE_EXTERNAL_TOOL_GUARD_ENV, | ||
| PRIVATE_EXTERNAL_TOOL_GUARD_PROVIDER_ENV, | ||
| } from '@qwen-code/acp-bridge/externalToolGuard'; | ||
| import { SERVE_CONTROL_EXT_METHODS } from '@qwen-code/acp-bridge/status'; |
There was a problem hiding this comment.
[Suggestion] The status subpath import pulls the entire @qwen-code/qwen-code-core barrel into this fixture — Concrete cost: packages/acp-bridge/src/status.ts keeps a runtime value import (import { SkillError } from '@qwen-code/qwen-code-core') in the compiled dist, and core's main entry is a barrel of eager re-exports, so every spawned mock child now evaluates core's whole module graph while only SERVE_CONTROL_EXT_METHODS.sessionClose is used. Probe-measured on this PR's tree (flips both ways): the pre-diff import set loads 80 modules (~47 ms boot, ~70 MB RSS); with the status import it loads 1444 modules, 644 from packages/core (~490 ms boot, ~165 MB RSS) — a ~10x startup cost paid per channel spawn of a fixture whose header documents it as lightweight, and hardcoding the method string restores 81 modules / ~47 ms / ~70 MB. The loadtest's p99 assertions still pass, but the fixture's boot profile — the stress baseline it exists to provide — shifts for a reason unrelated to the daemon, and the fixture now hard-depends on core-barrel import-time health: a future core import-time regression would surface here as an opaque ACP channel init failure, misdirecting investigation at the fixture/daemon handshake. (The other new import, externalToolGuard, is dependency-free.) Suggested fix (either): hardcode the method string here with a pointer comment — drift still fails loudly via the daemon's close error — or move SERVE_CONTROL_EXT_METHODS into a dependency-free leaf subpath of acp-bridge (as externalToolGuard.ts already is) and import it from there:
// drop: import { SERVE_CONTROL_EXT_METHODS } from '@qwen-code/acp-bridge/status';
// SERVE_CONTROL_EXT_METHODS.sessionClose, hardcoded to keep this fixture
// lightweight; drift fails loudly via the daemon's close error.
const SESSION_CLOSE_EXT_METHOD = 'qwen/control/session/close';
// ...and use it in extMethod below.中文说明
[Suggestion] status 子路径导入会把整个 @qwen-code/qwen-code-core 桶（barrel）拉进这个夹具 —— 具体代价：packages/acp-bridge/src/status.ts 中有一个运行时值导入（import { SkillError } from '@qwen-code/qwen-code-core'）保留在编译产物 dist 中，而 core 的主入口是一个急切再导出的桶，因此每个派生的 mock 子进程现在都会求值 core 的整个模块图，而实际只用到 SERVE_CONTROL_EXT_METHODS.sessionClose。在本 PR 代码树上经探针实测（双向翻转验证）：改动前的导入集合加载 80 个模块（启动约 47 ms、RSS 约 70 MB）；加入 status 导入后加载 1444 个模块，其中 644 个来自 packages/core（启动约 490 ms、RSS 约 165 MB）——对一个头部注释自述为轻量级的夹具，每次通道派生都要付出约 10 倍的启动成本；若将方法字符串硬编码，则恢复为 81 个模块 / 约 47 ms / 约 70 MB。loadtest 的 p99 断言仍会通过，但该夹具的启动画像——它存在所要提供的压测基线——会因与 daemon 无关的原因发生漂移，且夹具从此硬依赖 core 桶的导入期健康：未来 core 桶的任何导入期回归都会在这里表现为不透明的 ACP 通道初始化失败，把排查引向夹具/daemon 握手。（另一个新导入 externalToolGuard 是零依赖的。）建议修复（二选一）：在此处硬编码方法字符串并加一条指向 SERVE_CONTROL_EXT_METHODS.sessionClose 的注释——常量漂移仍会通过 daemon 的 close 错误响亮地失败；或者把 SERVE_CONTROL_EXT_METHODS 移到 acp-bridge 中一个零依赖的叶子子路径（externalToolGuard.ts 已是这种形态）并从那里导入。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| async extMethod(method, params) { | ||
| if (method === SERVE_CONTROL_EXT_METHODS.sessionClose) { |
There was a problem hiding this comment.
[Suggestion] The session-close ack ships with no test gating it in default CI — Concrete cost: reverting only this hunk leaves every suite default CI collects green (measured: the three consuming suites still pass 3/3), because the journal-recovery tests tear down via SIGTERM and never reach DELETE /session/:id; the only consumer of this path is qwen-daemon-loadtest.test.ts, gated behind QWEN_LOADTEST_ENABLED=1 and skipped in normal shards. Failure scenario: if the daemon's close contract drifts (method name, or the { sessionId, closed: true } success shape read by notifyAgentSessionClose), the fixture silently rots and only the opt-in loadtest — run manually, out of CI — would catch it. Suggested fix: add one assertion path collected by default CI, e.g. in one of the journal-recovery tests call client.closeSession(created.sessionId) after the turn and expect a successful close, so this ack is gated the same way the handshake hunks are.
中文说明
[Suggestion] 这个会话关闭确认（session-close ack）在默认 CI 中没有任何测试门控 —— 具体代价：仅回退本 hunk，默认 CI 收集的所有套件仍然全绿（实测：三个消费方套件依旧 3/3 通过），因为 journal-recovery 测试通过 SIGTERM 终止 daemon，从不会走到 DELETE /session/:id；该路径的唯一消费方是 qwen-daemon-loadtest.test.ts，它受 QWEN_LOADTEST_ENABLED=1 门控，在常规分片中跳过。失败场景：如果 daemon 的关闭契约发生漂移（方法名，或 notifyAgentSessionClose 读取的 { sessionId, closed: true } 成功形状），该夹具会悄无声息地腐化，只有手动启用的 loadtest（CI 之外）才能发现。建议修复：增加一条默认 CI 会收集的断言路径，例如在其中一个 journal-recovery 测试里，于回合结束后调用 client.closeSession(created.sessionId) 并断言关闭成功，使该确认与握手相关 hunk 一样被测试门控。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
🤖 Addressed the latest review feedback (round 2/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/10 轮）。改动内容与我反驳保留之处如下： Round address summary — PR #9161 (issue #9159)Feedback points and dispositions1. [Suggestion] The Reproduced the claim with a module-load probe on this branch's build: adding Applied the reviewer's first option: hardcoded The reviewer's second option (moving 2. [Suggestion] The session-close ack ships with no test gating it in default CI ( Confirmed: the only default-CI suites consuming the fixture are the two journal-recovery suites (both tear down via SIGTERM and never reach Implemented the suggested gate as-is: in the first test of Mutation proof: pointing the fixture's close-method constant at 3. Review-body note "Test Plan (not a blocker)" — no action needed The three files reported as Changes
No conflicts ( VerificationCommands actually run on this checkout (runner had already done
中文说明本轮处理总结 — PR #9161（issue #9159）反馈点及处理结论1. [Suggestion] 在本分支的构建产物上用模块加载探针复现了该结论：在夹具改动前的导入集合之上再导入 采用了审稿人的第一个方案：硬编码 未采用审稿人的第二个方案（把 2. [Suggestion] 会话关闭确认（session-close ack）在默认 CI 中没有任何测试门控（ 已确认：默认 CI 中消费该夹具的套件只有两个 journal-recovery 套件（两者都通过 SIGTERM 终止，从不会走到 按建议原样实现了门控：在 变异验证：把夹具的关闭方法常量改成 3. 审查正文中的 “Test Plan（非阻断）” 备注 — 无需处理 被报告为 变更内容
无冲突（ 验证在本检出上实际执行的命令（runner 已完成
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 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/模型 |
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.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // Gate the fixture's session-close ack in default CI: the only other | ||
| // consumer of this path is the opt-in loadtest, so without this call a | ||
| // drifted close method name or ack shape would rot silently. |
There was a problem hiding this comment.
[Suggestion] R3-1: The gate catches a drifted close method name (and a missing/throwing handler) but not a drifted ack shape, while the comment claims both — Failure scenario: if the fixture's close ack drifts from { sessionId, closed: true } to { sessionId } or { closed: false }, notifyAgentSessionClose evaluates response['closed'] === true → false without throwing (bridge.ts:5003 — throwOnFailure: true only rethrows RPC errors), closeSessionImpl takes the cancel fallback and resolves normally (bridge.ts:6938-6951), the route returns 204, and closeSession resolves undefined — this assertion stays green while every daemon close of a mock session silently degrades to the cancel-fallback path. Probe-measured on this tree: ack → { sessionId } and ack → { sessionId, closed: false } both leave the suite 2/2 green; drifting the method constant fails exactly at this assertion.
| // Gate the fixture's session-close ack in default CI: the only other | |
| // consumer of this path is the opt-in loadtest, so without this call a | |
| // drifted close method name or ack shape would rot silently. | |
| // Gate the fixture's session-close ack in default CI: the only other | |
| // consumer of this path is the opt-in loadtest, so without this call a | |
| // drifted close method name or missing close handler would rot silently | |
| // (ack-shape drift is absorbed by the daemon's cancel fallback and is | |
| // not observable from the client). |
中文说明
[Suggestion] 该门能捕获 close 方法名漂移（以及处理器缺失/抛错），但无法捕获 ack 形状漂移，而注释声称两者都能捕获。失败场景：夹具的 close ack 从 { sessionId, closed: true } 漂移为 { sessionId } 或 { closed: false } 时，notifyAgentSessionClose 求值 response['closed'] === true → false 且不抛错（bridge.ts:5003 —— throwOnFailure: true 只重抛 RPC 错误），closeSessionImpl 走 cancel 回退路径并正常完成（bridge.ts:6938-6951），路由返回 204，closeSession 解析为 undefined —— 断言保持绿色，而每次对 mock 会话的 daemon 关闭都静默降级为 cancel 回退路径。已在本树上经探针验证：将 ack 变为 { sessionId } 或 { sessionId, closed: false } 后套件仍 2/2 通过；漂移方法常量后恰好在此断言处失败。建议修复：将注释收窄到该调用实际门控的范围（ack 形状在 REST close 路径上对客户端无可观测影响——两种情况都返回 204），或在 bridge 单元测试中直接钉住 closed === true 契约（响应在那里可直接观测）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
# Conflicts: # integration-tests/fixtures/mock-acp-child/agent.mjs
|
🤖 Addressed the latest review feedback (round 3/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/10 轮）。改动内容与我反驳保留之处如下： Autofix review round summary — PR #9161 (issue #9159)Feedback points and decisions
Base-conflict resolution (
|
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): "agent 6a": could not read the installed @agentclientprotocol/sdk dist (no node_modules present) to confirm RequestError.methodNotFound and the no- extMethod default d…; "agent 6c": could not read @agentclientprotocol/sdk source to confirm the pre-PR wire behavior for ext methods when the agent provides no extMethod handler at all — no…; "agent 6c": did not execute the consuming integration tests (no installed dependencies / bundle in the review worktree); verification was static, end-to-end through all fiv….
Test Plan (not a blocker): cli/qwen-serve-streaming.test.ts — no such file or directory; cli/qwen-serve-client-mcp.test.ts — no such file or directory; cli/mock-acp-typecheck.test.ts — no such file or directory.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 6a"：could not read the installed @agentclientprotocol/sdk dist (no node_modules present) to confirm RequestError.methodNotFound and the no- extMethod default d…；"agent 6c"：could not read @agentclientprotocol/sdk source to confirm the pre-PR wire behavior for ext methods when the agent provides no extMethod handler at all — no…；"agent 6c"：did not execute the consuming integration tests (no installed dependencies / bundle in the review worktree); verification was static, end-to-end through all fiv…。
Test Plan（非阻断）：cli/qwen-serve-streaming.test.ts — no such file or directory; cli/qwen-serve-client-mcp.test.ts — no such file or directory; cli/mock-acp-typecheck.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| await expect( | ||
| activeDaemon.client.closeSession(created.sessionId), | ||
| ).resolves.toBeUndefined(); |
There was a problem hiding this comment.
[Suggestion] The gate is proven effective (removing the fixture's close handler fails this assertion with DaemonHttpError: DELETE /session/:id: "Method not found": qwen/control/session/close), but no PR-gated CI job collects this test file, so it only executes in the post-merge/nightly E2E Tests workflow. Verified at this commit: e2e.yml has no pull_request trigger (its own comment says E2E "is NOT in the merge queue"; it runs post-merge on main, nightly, and on dispatch); the PR-gated no-AK integration gate (test:integration:no-ak:sandbox:none in the root package.json) is a fixed file list that omits this file; and ci.yml's integration_cli job — whose test:integration:cli:sandbox:none would collect it — is gated to merge_group and never triggers while no merge queue is configured on main (the active main ruleset has no merge_queue rule). The comment above claims this gates the ack "in default CI". Concrete cost: a future change that drifts SESSION_CLOSE_EXT_METHOD or removes the fixture's close handler leaves every PR check green, merges, and surfaces only at the next post-merge main E2E or nightly run — the silent-rot window this call is meant to close, displaced to after merge. (Drift still fails loudly there — that is how the original breakage this PR fixes was caught — so this is a placement improvement, not an open hole.) Suggested fix: add ./cli/qwen-serve-live-journal-recovery.test.ts to the test:integration:no-ak:sandbox:none file list — the test needs no API key (it drives the mock ACP child), which is the class that gate exists to run at PR time; alternatively narrow the comment to name the post-merge E2E workflow.
中文说明
该门控已被证实有效（移除夹具的 close 处理器会使此断言以 DaemonHttpError: DELETE /session/:id: "Method not found": qwen/control/session/close 失败），但没有任何 PR 门控的 CI 任务收集该测试文件，因此它只会在合并后/每晚的 E2E Tests 工作流中执行。已在本提交上核实：e2e.yml 没有 pull_request 触发器（其自身注释写明 E2E "不在合并队列中"；仅在 main 合并后、每晚定时和手动触发时运行）；PR 门控的 no-AK 集成门（根 package.json 中的 test:integration:no-ak:sandbox:none）是一份固定文件列表，其中不含该文件；ci.yml 中的 integration_cli 任务——其 test:integration:cli:sandbox:none 本会收集它——仅在 merge_group 事件下运行，而 main 当前生效的 ruleset 没有 merge_queue 规则，故该任务从不触发。上方注释声称此调用在"默认 CI"中门控该 ack。具体代价：未来若 SESSION_CLOSE_EXT_METHOD 漂移或夹具的 close 处理器被移除，所有 PR 检查仍将保持绿色并照常合并，问题只会在下一次 main 合并后 E2E 或每晚运行中浮现——这正是此调用意图关闭的静默腐化窗口，只是被推迟到了合并之后。（漂移在那里仍会响亮地失败——本 PR 所修复的最初破损正是这样被捕获的——因此这是门控位置的改进，而非敞开的漏洞。）建议修复：将 ./cli/qwen-serve-live-journal-recovery.test.ts 加入 test:integration:no-ak:sandbox:none 的文件列表——该测试不需要 API key（它驱动 mock ACP 子进程），正是该门存在以便在 PR 阶段运行的类别；或者将注释收窄，改为指明合并后 E2E 工作流。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
🤖 Addressed the latest review feedback (round 4/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/10 轮）。改动内容与我反驳保留之处如下： Autofix review round summary — PR #9161 (issue #9159)Feedback points and decisions1. [Suggestion] Close-gate comment claims the ack is gated "in default CI" (rc:3785883472)Finding: The gate is proven effective, but no PR-gated CI job collects Verification of the claim (reproduced against this commit): Confirmed on all three points —
Decision: Implemented the finding's alternative — narrowed the comment. Adding the file to the no-AK list requires editing the root Change: Rewrote the comment above the 2. Review body notes (informational — no action)
Conflict notes
Verification
Commit: 中文说明Autofix 审查轮次总结 — PR #9161（issue #9159）反馈点与决定1. [Suggestion] close 门控注释声称该 ack 在"默认 CI"中被门控（rc:3785883472）发现内容： 该门控已被证实有效，但没有任何 PR 门控的 CI 任务收集 对声明的核实（在本提交上复现）： 三点全部确认——
决定：实施该发现提供的替代方案——收窄注释。 将文件加入 no-AK 列表需要编辑根 变更： 重写了 2. 审查正文中的说明（信息性——无需处理）
冲突说明
验证
提交： Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Test Plan (not a blocker): cli/qwen-serve-streaming.test.ts — no such file or directory; cli/qwen-serve-client-mcp.test.ts — no such file or directory; cli/mock-acp-typecheck.test.ts — no such file or directory.
中文说明
未发现问题。LGTM！✅
Test Plan（非阻断）：cli/qwen-serve-streaming.test.ts — no such file or directory; cli/qwen-serve-client-mcp.test.ts — no such file or directory; cli/mock-acp-typecheck.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
Released in v0.21.13. |
4 participants
What this PR does
The mock ACP child fixture used by the daemon live-journal recovery E2E tests now honors the tool-guard handshake that
qwen serverequires of every child it spawns: it consumes the private guard marker the daemon passes and returns the guard-ready acknowledgment in itsinitializeresponse, exactly as the production child does. The acknowledgment constants are imported from the sharedacp-bridgemodule, so the fixture cannot drift from the values the daemon checks.Why it's needed
The cross-worktree Git guard feature made the daemon's built-in tool guard unconditional, and the bridge now refuses to create a session through any ACP child that does not acknowledge the required guard handshake — a deliberate fail-closed posture. The mock child predates that contract and never acknowledged it, so every session it served failed with
ACP child did not acknowledge the required external tool guard, which broke themainE2E workflow on every platform and sandbox leg that schedules the two live-journal recovery tests (issue #9159, runs 4028 and 4030; the run immediately before the guard commit was green).Reviewer Test Plan
How to verify
Run the two affected E2E files on this branch — all three tests should pass:
npm run build && npm run bundle npx cross-env QWEN_SANDBOX=false vitest run --root ./integration-tests cli/qwen-serve-live-journal-recovery.test.ts cli/qwen-serve-webui-live-journal-recovery.test.tsBefore the fix, all three tests fail with
DaemonHttpError: POST /session: ACP child did not acknowledge the required external tool guard (received: undefined).; after the fix they pass. Also confirm the other daemon-spawned E2E areas stay green (cli/qwen-serve-streaming.test.ts,cli/qwen-serve-client-mcp.test.ts,cli/mock-acp-typecheck.test.ts) and thatpackages/acp-bridgeunit tests pass unchanged. No production code changed — the daemon's fail-closed handshake requirement is untouched.Evidence (Before & After)
N/A (test-fixture change, no user-visible behavior)
Tested on
Environment (optional)
Local self-hosted Linux runner, Node v22.23.2,
QWEN_SANDBOX=falseintegration runs against the bundled CLI. Thesandbox:dockerleg could not be run locally (no Docker daemon); the failure and fix live in the host-level daemon↔child ACP handshake, which is identical across sandbox legs.Risk & Scope
sandbox:dockerE2E leg (no Docker on this runner) and the excluded daemon loadtest, which shares the fixture and benefits from the same fix. The guard policy itself and all production guard code are untouched and out of scope.Linked Issues
Fixes #9159
中文说明
本 PR 做了什么
daemon 实时日志（live-journal）恢复 E2E 测试所使用的 mock ACP 子进程夹具，现在会履行
qwen serve对其派生的每个子进程所要求的工具守卫握手：它消费 daemon 传递的私有守卫标记，并在其initialize响应中返回守卫就绪确认，与生产子进程的行为完全一致。确认所用的常量直接从共享的acp-bridge模块导入，因此夹具不会与 daemon 校验的取值产生漂移。为什么需要
跨 worktree Git 守卫功能使 daemon 内置的工具守卫变为无条件启用，桥接现在会拒绝通过任何未确认必需守卫握手的 ACP 子进程创建会话——这是有意的失败即关闭（fail-closed）姿态。该 mock 子进程早于此契约引入，从未进行过确认，因此经由它服务的每个会话都会以
ACP child did not acknowledge the required external tool guard失败，导致main分支 E2E 工作流在所有排到这两个实时日志恢复测试的平台与 sandbox 分支上全部失败（issue #9159，运行 4028 与 4030；守卫提交之前的那次运行是绿色的）。审阅者测试计划
如何验证
在本分支上运行受影响的两个 E2E 文件——三个测试应全部通过：
npm run build && npm run bundle npx cross-env QWEN_SANDBOX=false vitest run --root ./integration-tests cli/qwen-serve-live-journal-recovery.test.ts cli/qwen-serve-webui-live-journal-recovery.test.ts修复前，三个测试全部失败，报错为
DaemonHttpError: POST /session: ACP child did not acknowledge the required external tool guard (received: undefined).；修复后通过。同时确认其他由 daemon 派生的 E2E 场景保持绿色（cli/qwen-serve-streaming.test.ts、cli/qwen-serve-client-mcp.test.ts、cli/mock-acp-typecheck.test.ts），且packages/acp-bridge单元测试不受影响地通过。未改动任何生产代码——daemon 失败即关闭的握手要求保持原样。证据（修复前后）
N/A（测试夹具改动，无用户可见行为）
测试环境
环境（可选）
本地自托管 Linux runner，Node v22.23.2，针对打包后的 CLI 以
QWEN_SANDBOX=false运行集成测试。sandbox:docker分支无法在本地运行（没有 Docker daemon）；失败与修复都位于宿主机层的 daemon↔子进程 ACP 握手，在各 sandbox 分支中完全一致。风险与范围
sandbox:dockerE2E 分支（本 runner 没有 Docker），以及共享该夹具并同样受益的、被排除在外的 daemon loadtest。守卫策略本身与所有生产守卫代码均未改动，超出本 PR 范围。关联 Issue
Fixes #9159