feat: delegate a subagent turn to an external agent over ACP (Claude Code first) - #11003
Conversation
E2E report — re-verified against the committed codeThe before/after table in the PR description was measured on a build that predates the last fix in Fixture:
Baseline column is the shipped CLI 0.23.0 run of the identical fixture, recorded during the pre-implementation dry run. Still outstanding
|
|
Thanks @wenshao — re-running the gate at Template looks good ✓ — every required heading is present and filled in, the OS table is honest about what was and wasn't tested (macOS ✅, Windows/Linux Problem: observed, not theoretical. The before/after table is a measurement on the shipped 0.23.0 CLI — file created, Direction: aligned, and I checked rather than assumed. Size: 4,609 changed lines break down as 2,217 production / 2,248 test / 144 docs, with no generated or schema files. That's over both the 500-line and the 1,000-line thresholds, and it lands heavily in core — Approach: the scope feels earned, and I went looking for churn that could be cut. I didn't find unrelated edits or drive-by refactors: the Risk: Stage 1e matched none of the high-risk paths from the revert-history analysis, so no elevated risk signal there. What I would flag instead is contract surface, which is a different axis: this adds a core module reachable through the published export map, widens Moving on to code review. 🔍 中文说明感谢 @wenshao —— 在 模板完整 ✓ —— 所有必需标题都在且填写完整，操作系统表格如实标注了测试范围（macOS ✅，Windows/Linux 问题： 是已观测到的，不是理论性的。before/after 表格是在已发布的 0.23.0 CLI 上实测的结果 —— 文件创建成功、 方向： 是对齐的，而且我是查证过的，不是假设。base 分支的 规模： 4,609 行改动的构成是 生产代码 2,217 行 / 测试 2,248 行 / 文档 144 行，没有生成文件或 schema 文件。这超过了 500 行和 1,000 行两个阈值，而且集中在 core —— 方案： 范围是合理的，我也专门找过可以砍掉的改动。没有发现无关改动或顺手重构： 风险： Stage 1e 没有命中回滚历史分析里的任何高风险路径，所以那里没有升级风险信号。我要指出的是另一个维度 —— 契约面：本 PR 新增了一个可通过已发布 export map 访问的 core 模块，把 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code review at No Critical findings. Everything below is a Suggestion or a nit, and per this repo's five-round rule on a branch that is now at thirteen, I'm recording them as follow-ups rather than asking for another round. What I tried to break and couldn'tThe in-process dispatch and execution path is untouched, which was my main regression worry on a diff this size. The The fail-closed story holds up under tracing. The frontmatter schema is strict, rejects unknown keys inside the executor block, validates The interface widening is sound. Production Two details I'd single out as good work. Reuse is real, not decorative: The system-prompt extraction is equivalent, which the description claims and I verified rather than accepted: One field I checked specifically because it looked dangerous and isn't: Suggestions — follow-ups, not blockersTwo behaviour changes reach consumers outside the diff. These are the findings I'd most want a maintainer to see, because neither is visible from the diff alone. First, Second, the new try/catch around Dead fields in the new executor contract. Executor refusals are detected by error-message text. Three sites ( A question about the trust gate's scope, not a defect I can prove. Init timeout versus the documented invocation. Design documentation. Nits: How the pieces talksequenceDiagram
participant P1 as Agent tool
participant P2 as SubagentManager
participant P3 as AcpSubagentExecutor
participant P4 as External ACP peer process
participant P5 as AgentEventEmitter
P1->>P2: createAgentHeadless(config, runtimeContext)
P2->>P2: refuse if safe mode, untrusted folder, or unsupported constraint
P2->>P3: externalExecutor.create(params)
P3->>P3: assert POSIX platform, reject max_turns
P3->>P4: spawn with sanitized env, no shell
P3->>P4: initialize then newSession then setSessionMode
P4-->>P3: advertised modes and sessionId
P1->>P3: execute(context, signal)
P3->>P3: check wall-time budget before dispatching
P3->>P4: prompt with system bundle then task
P4-->>P5: session updates republished as agent events
P4->>P3: requestPermission for a sensitive tool
P3->>P5: TOOL_WAITING_APPROVAL, plain text, bounded
P5-->>P3: user outcome mapped to a peer option id
P3-->>P4: selected option, or reject once, never a guess
P4-->>P3: stopReason
P3->>P3: flush open tools, then dispose and reap the process tree
Files changed (29 of 29 shown)
Test evidence — the PR's own CI, read via the APIThis is an unattended CI run (
No check is red, so there is no failing job log to quote. Pending PR CI workflow runs for this head: 0 — CI is settled, and the only thing still in flight is this triage run's own orchestration job. Two of these green checks carry more weight than they look, and I confirmed it at step level rather than assuming. Inside What CI does not prove: no job in Sandboxed verification would settle the part that matters most and that I could not reach: To be explicit about attribution: the before/after table in the PR description — the external model label in 中文说明
在 没有 Critical 级别的问题。 下面全部是 Suggestion 或细节问题；按照本仓库"约 5 轮之后只落 Critical 修复"的规则，而这个分支已经到第 13 轮，我把它们记为后续跟进项。 进程内的派发与执行路径确实没有被改动，这是我在这么大的 diff 上最担心的回归点。 fail-closed 的设计经得起追踪。frontmatter schema 是严格的：拒绝 executor 块里的未知键、大小写敏感地校验 接口放宽是可靠的。base 中 有两处细节值得单独点出。 复用是真实的： 系统提示词的抽取是等价的 —— 这是描述里的声明，我做了核对： 有一个字段我专门查了，因为它看起来危险但实际不是： Suggestion —— 后续跟进，非阻塞： 有两处行为变化触及了 diff 之外的消费方。 这是我最希望维护者看到的两项，因为单看 diff 都看不出来。 第一， 第二， 新 executor 契约里的死字段。 executor 拒绝是靠错误消息文本识别的。 三处（ 一个关于信任门禁范围的疑问，不是我能证实的缺陷。 初始化超时与文档中的调用方式不一致。 设计文档。 细节： 测试证据 —— 来自 PR 自己的 CI，通过 API 读取。 这是一次无人值守的 CI 运行（ 没有红色 check，因此没有失败日志可引用。该 head 上待完成的 PR CI workflow run 数为 0。 其中两项绿色 check 的分量比看上去重，而且我是在 step 级别确认的。 CI 没有证明的部分： 沙箱验证可以解决最关键、而我在本路径上无法触及的部分： 关于归属需要明确说明：PR 描述里的 before/after 表格是作者本人在 macOS 上的测量。我没有重跑，在此路径上也无法重跑。那是一个声明，不是我验证过的证据。未验证项：任何平台上的真实适配器行为、任何 Linux 行为，以及我用静态追踪得出但未曾实际执行的 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — solid, and I'd merge it. Not 5/5 because of the named follow-ups, and because the real-adapter behaviour is the one thing this review path structurally cannot execute.
The state changed while this run was in progress. Earlier in this same run I posted a defer to @qqqys, on the grounds that the direction and contract-surface call needed a human maintainer and that qqqys had asked for an end-to-end read before approving. Both grounds are now gone. Three maintainers approved this exact commit while I was still reading it — @yiliang114 at 03:00:41Z, @qqqys at 03:13:00Z and @doudouOUC at 03:13:57Z, all against Going back to what I'd have written before reading the diff: my instinct was to reuse the ACP plumbing the repo already has, keep the parent Qwen session authoritative so workspace identity doesn't need re-keying, republish peer events as the existing agent events so the transcript and permission layers don't change, and fail closed everywhere the host can't honour the definition. That is what this PR does. I didn't find a materially simpler design I'd defend, and the 80%-scope question doesn't have a good answer here — the frontmatter validation machinery is the only part I'd try to shrink, and it's load-bearing for the guarantee the whole PR argues for. So the approach question settles in the PR's favour, and it did so before the approvals landed; I'm not revising upward because other people voted. On the six-months question, which is the one I actually struggled with: this commits the repo to a second execution path for subagents, and every future What I'd ask for as follow-ups, none of which should gate this merge under the five-round rule:
The one thing I'd still flag for whoever merges: the real-adapter path is unverified by any completed CI job, because the 45 executor tests run against an injected fake peer and the CLI integration lane was skipped at this head — the same lane that would have touched the 中文说明Confidence: 4/5 —— 扎实，我会合并它。没有给到 5/5，是因为下面点名的后续跟进项，以及真实适配器行为是这条审查路径在结构上无法执行的那一部分。
本次运行进行期间状态发生了变化。 在同一次运行的早些时候，我发过一条转交 @qqqys 的 defer，理由是方向与契约面的判断需要人类维护者，以及 qqqys 曾要求在批准前做一次端到端通读。这两个理由现在都不成立了。就在我还在读代码的时候，三位维护者批准了这个确切的 commit —— @yiliang114 于 03:00:41Z、@qqqys 于 03:13:00Z、@doudouOUC 于 03:13:57Z，全部针对 回到我在读 diff 之前会写下的方案：我的直觉是复用仓库已有的 ACP 设施、让父 Qwen 会话保持权威从而不需要重新定义 workspace identity、把对端事件重新发布为既有的 agent 事件从而不改动 transcript 与权限层，并在宿主无法遵守定义的每一处 fail closed。这正是本 PR 所做的。我没有找到一个我自己愿意为之辩护的、明显更简单的设计；"砍掉 80% 范围"这个问题在这里也没有好答案 —— 我唯一会尝试缩减的是 frontmatter 校验那套机制，而它正是整个 PR 所要论证的那个保证的承重结构。所以方案层面的结论有利于本 PR，而且这个结论在那三票之前就已成立；我不是因为别人投了票才上调判断。 关于"六个月后维护"这个问题 —— 这是我真正纠结的一点：本 PR 让仓库承诺了 subagent 的第二条执行路径，未来每一个新的 我希望作为后续跟进项处理的内容，按五轮规则都不应阻塞本次合并：
我仍然要向执行合并的人指出一点：真实适配器路径没有任何已完成的 CI job 验证过，因为 45 个 executor 测试跑在注入的假对端上，而该 head 上 CLI 集成通道被跳过 —— 也正是那条通道本会触及上面的 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Requesting changes on three Critical findings in the new ACP executor — full detail, with the in-repo precedent for each, is in the Stage 2 comment above.
-
spawn(..., { env: process.env })hands the child Qwen-internal secrets (QWEN_SERVER_TOKEN,QWEN_DAEMON_TOKEN,QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN,QWEN_CODE_PRIVATE_ACP_CAPABILITY).sanitize-child-env.tsstates these must never reach a child the agent launches on the user's behalf (issue #6601) and that the private ACP capability is never handed to an ACP child;mcp-client.ts:2564does the equivalent spawn withsanitizeChildEnv. The command comes from a project-level agent file, so a cloned repository can name the executable that receives them. Fix:env: sanitizeChildEnv(process.env). -
The handshake has no deadline and no
exitracer.spawnFailurerejects only on theerrorevent, so a command that spawns, stays alive and never speaks ACP leavescreate()pending forever — an Agent tool call that never returns and never errors.qwen-live/src/adaptor/acp-adaptor.ts:574-582already racesinitializeagainst bothhandshakeDeadline()and anexitPromiserejecting onerrorandexit. Both races here (initialize,newSession) need it, and no post-handshakeexithandler exists either, so a mid-turn crash leaves parked permissions attached to a dead process. -
options.find(o => o.kind === wantKind) ?? options[0]can answer a permission request with a more permissive option than the user approved — "proceed once" against[allow_always, reject_once]sendsallow_always. It contradicts the fail-safe posture the same file argues for inresolvePermissionModeandparseAgentExecutor, and it is untested. Fall back to a rejection, or to the least-permissive offered option.
Suggestions 4-6 (reuse createStderrForwarder for redaction and cross-chunk line buffering; executeExternalInputs diverging from AgentHeadless on finalText and resetStats; the lazy-import comment describing behaviour the code does not have) are non-blocking — ride along or defer as you prefer.
None of this is an argument against the feature or the design. The core seam is careful and I verified the interface-widening claim independently: six getCore() call sites, exactly the three members SubagentExecutorCore declares, no instanceof AgentHeadless anywhere. I want this to land — the three fixes are small and all three are already precedented in-tree.
CI was still running at cbd124f3aca41166cf33fcbc06f96f75864062f5 when I reviewed, so I have not read a green suite on this commit.
中文说明
就新增 ACP executor 里的三条严重问题请求修改 —— 完整细节与每条对应的仓库内先例见上方的 Stage 2 评论。
-
spawn(..., { env: process.env })把 Qwen 内部密钥（QWEN_SERVER_TOKEN、QWEN_DAEMON_TOKEN、QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN、QWEN_CODE_PRIVATE_ACP_CAPABILITY）交给了子进程。sanitize-child-env.ts写明这些变量绝不能到达 agent 代表用户启动的子进程（issue #6601），并说明私有 ACP capability 绝不交给 ACP 子进程；mcp-client.ts:2564的同类 spawn 用的是sanitizeChildEnv。命令来自项目级 agent 文件，因此一个被 clone 下来的仓库就能指名那个接收它们的可执行文件。修法：env: sanitizeChildEnv(process.env)。 -
握手既无超时也无
exit竞争者。spawnFailure只在error事件上 reject，所以一个启动成功、保持存活、却从不说 ACP 的命令会让create()永久挂起 —— 一个永不返回也永不报错的 Agent 工具调用。qwen-live/src/adaptor/acp-adaptor.ts:574-582已经让initialize同时与handshakeDeadline()和一个在error和exit上都 reject 的exitPromise竞争。这里两处竞争（initialize、newSession）都需要，而且握手之后也没有 exit 处理器，因此回合中途崩溃会把停放的权限请求留在一个已死的进程上。 -
options.find(o => o.kind === wantKind) ?? options[0]可能用一个比用户批准范围更宽松的选项去应答权限请求 —— 对着[allow_always, reject_once]选择"仅此次"，回传的是allow_always。这与同一文件在resolvePermissionMode和parseAgentExecutor中主张的失败即安全姿态相矛盾，且没有测试。应回退到拒绝，或回退到所提供选项中最不宽松的那个。
建议 4-6（复用 createStderrForwarder 以获得脱敏与跨 chunk 的行缓冲；executeExternalInputs 在 finalText 与 resetStats 上偏离 AgentHeadless；懒加载注释描述了代码并不具备的行为）不阻塞合并 —— 一起改或后续处理都可以。
这些都不是在反对这个能力或这个设计。core 接缝很用心，而且接口放宽这个主张我是独立核过的：六处 getCore() 调用点，正好是 SubagentExecutorCore 声明的那三个成员，整棵树里没有任何 instanceof AgentHeadless。我希望它合入 —— 三处修改都很小，而且三条在树内都已有先例。
我审查时 CI 仍在 cbd124f3aca41166cf33fcbc06f96f75864062f5 上运行，所以我没有读到这个 commit 的绿色套件。
— Qwen Code · qwen3.8-max-2026-09-02
Reviewed at cbd124f3aca41166cf33fcbc06f96f75864062f5 · re-run with @qwen-code /triage
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-12 unconditional await import of the ACP executor in loadCliConfig with a false lazy-import comment — already reported (triage review 5109679716, Suggestion 6)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.23.0)
Review fixes pushed —
|
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Evidence screenshots addedThese are terminal captures, not Web Shell browser screenshots. The browser route was attempted first and is not available in this environment: driving a desktop browser here requires the The Web Shell surface for a delegated subagent is What was runA Result
Full scrollback
The run was verified independently of the screenshotsCaptured at 39.9s, 4 frames. Checked against the filesystem and the subagent metadata rather than taken on trust from the image:
Images are hosted on the Not yet capturedThe Web Shell subagent panel rendering this delegation, and the approval dialog for an external agent's tool call. Both need a browser session; the approval dialog additionally needs the permission E2E (test-plan groups 4 and 5), which is still outstanding and is the security-relevant gap noted in the previous comment. The capture scenario is not committed: it hardcodes an absolute repo path, so it is not portable as written. It should be parameterised before it lands. |
Headless Playwright capture of the Web Shell driving the same delegation: a session prompt asks for the claude-worker subagent, the turn runs in the external Claude Code process, and the requested file is produced. Verified independently of the images: ws-shots.txt contains WS_OK, and the subagent metadata for that session reports model external-acp:node, agentType claude-worker, status completed, with Bash x3 in its transcript and zero qwen write_file calls. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/external-agents/acp-subagent-executor.ts:436 — [review] System prompt re-sent as user text on every execute() — stop-hook continuations inject the entire rendered prompt N extra times (probe-verified); code unchanged since …packages/core/src/subagents/subagent-manager.ts:1053 — [review] The new SubagentError pass-through guard has no witnessing test — guard-deletion mutant ships green (mutation probe); code unchanged since round 1 (code-age rule)docs/design/claude-code-web-shell-backend.md:5 — [review] All '实测/已核实' evidence points at a git-ignored spike file absent from the tree — Decision 4's security premise ships unfalsifiabledocs/design/claude-code-web-shell-backend.md:298 — [review] Q4 boundary-sync obligation (multi-agent-coordination.md update + acceptance) not fulfilled — the feature doc still routes cross-vendor coordination to Herdrdocs/design/claude-code-web-shell-backend.md:376 — [review] §8.5-2 'only 4 ClientSideConnection sites' enumeration falsified by this commit's fifth (sweep: 5 production sites)packages/cli/src/external-agents/acp-subagent-executor.ts:493 — [review] executeExternalInputs never emits EXTERNAL_MESSAGE — injected send_message turns absent from the JSONL transcript (probe-verified); code unchanged since round 1 (code-…docs/design/claude-code-web-shell-backend.md:24 — [review] Unpinned npx -y adapter spawn vs the certified v0.73.0 envelope — registry latest is 0.74.0 (measured), proven to ignore the delivered permission mode; any R1-4 fix can be silently …packages/cli/src/external-agents/acp-subagent-executor.ts:599 — [review] terminateModeForStopReason fails open — unknown/absent stopReason maps to GOAL, certifying incomplete work as finished (probe flip); code unchanged since round 1 (code…packages/cli/src/external-agents/acp-subagent-executor.ts:496 — [review] A steering extMethod rejection propagates and tears down the whole subagent instead of falling back to prompt() (probe flip); code unchanged since round 1 (code-age ru…packages/core/src/subagents/subagent-manager.ts:1015 — [review] Composed dispose propagates executor dispose() failures — workflow-orchestrator.ts:1223 awaits it bare in finally, converting success into error or masking the real failure (pr…packages/core/src/agents/runtime/subagent-executor.ts:142 — [review] taskName populated by every dispatch caller, read by no implementation — dead field on the new public subagentRuntime surface, sixth member of R1-3's unconsumed-config cla…packages/core/src/subagents/subagent-manager.ts:1010 — [review] Dispatch test never pins the runtimeContext identity — the one-word shadow mutant compiles and passes all four tests (mutation probe); code unchanged since round 1 (code-age ru…
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 12 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
8 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-6 the report-0 token decision's mandated honesty diagnostic is unimplemented — already reported (comments 3935344376, 3938129399)
- R1-7 the executor's load-bearing behaviours are pinned by no test — already reported (comments 3935344365, 3938129403)
- R1-8 the design doc's file.ts:line citations are systematically wrong at this commit — already reported (comments 3935344342, 3938129407)
- R1-9 the committed design doc records states and decisions the same PR's code contradicts — already reported (comments 3935344353, 3938129414)
- R1-24 the SubagentError pass-through guard is vacuously covered, and rescues only SubagentError while every real executor failure is a plain Error — already reported (comment 3935344403)
- R1-44 the contract's options.resetStats is a dead switch on the external executor — already reported (comment 3935344421)
- R1-52 the new subagentRuntime subpath has no packages/cli tsconfig paths mapping — already reported (comment 3935344433)
- R1-55 workflow schema mode can dispatch an executor-based definition with no fail-fast guard — already reported (comment 3935344439)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally, so the Windows-only spawn failure (R3-22) is unexercised by CI.
Not reviewed: test-efficacy — the mutation/hunk probe harness could not be validated (harnessValidated: null; the positive control ran and died on the vitest global-setup prerequisite guard), so whether the diff's new tests gate its new behaviour is unmeasured in either direction.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": §9.4's hard constraint that a permission request must be emitted under the parent bridge session id — I read SubAgentTracker.createToolCallHandler but never t…; "agent reverse-audit (round 3)": §8.4 rows 3/4/6 existence-and-line claims ( cli/src/serve/virtual-subagent-sessions.ts id synthesis/polling and the 5-route whitelist, web-shell/.../SubagentD…; "agent reverse-audit (round 3)": §8.6 hard-constraint 2's server-side claim ("服务端也没有任何接受 virtual id 的 prompt 路由", DaemonClient.ts:3228,3240 ) — not traced through cli/src/serve/routes .; "agent reverse-audit (round 3)": §8.8's comparison table and §8.9/决策记录 rationale — read but not independently falsifiable (judgement rows, no code claim to check); no verification performed.; chunk 10: did not execute npm run typecheck or the packages/core subagent vitest files; every claim above is from reading code at HEAD (3463e13974), not from a run., and 2 more.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/external-agents/acp-subagent-executor.ts:137 — [review] A schema-valid permissionMode: dontAsk is silently…packages/core/src/subagents/subagent-manager.ts:1011 — [review] The executor branch sits after the in-process-only…packages/core/src/subagents/subagent-manager.ts:961 — [review] The executor branch inherits the frontmatter hook…packages/core/src/subagents/agent-frontmatter-schema.ts:208 — [review] parseAgentExecutor's contract comment states a caller…packages/cli/src/external-agents/acp-subagent-executor.ts:559 — [review] The steering outcome check fails open: any outcome other…packages/cli/src/external-agents/acp-subagent-executor.ts:496 — [review] The full rendered system prompt is prepended on every…docs/design/claude-code-web-shell-backend.md:880 — [review] §10's status section (and §9.8's 待确认) records the state of…packages/cli/src/external-agents/acp-subagent-executor.ts:599 — [review] terminateModeForStopReason fails open: an unknown or…packages/core/src/agents/runtime/subagent-executor.ts:142 — [review] taskName is populated by every dispatch caller and read by…packages/cli/src/external-agents/acp-subagent-executor.ts:89 — [review] externalModelLabel's docstring premise — the label is only…packages/core/src/subagent-runtime.ts:13 — [review] The rationale that justifies this new subpath is false for…packages/core/src/subagents/types.ts:193 — [review] The trust rationale this new JSDoc records for…packages/core/src/subagents/agent-frontmatter-schema.ts:228 — [review] parseAgentExecutor's 'args must be an array of strings'…docs/design/claude-code-web-shell-backend.md:73 — [review] 决策1 — the doc's self-declared 核心改动 — invents a composite…docs/design/claude-code-web-shell-backend.md:541 — [review] §Q11's blast-radius argument — the stated justification…docs/design/claude-code-web-shell-backend.md:553 — [review] §Q11 lists toolUsage among the fields the ACP executor CAN…packages/cli/src/acp-integration/session/SubAgentTracker.ts:228 — [review] The human-readable payload the executor builds for an…docs/design/claude-code-web-shell-backend.md:893 — [review] The design doc's load-bearing rule for the new…packages/cli/src/external-agents/acp-subagent-executor.ts:89 — [review] externalModelLabel labels from the command BASENAME, which…packages/cli/src/external-agents/acp-subagent-executor.ts:652 — [review] An agent REFUSAL is mapped to the same terminate mode as a…- …and 7 more (see the run report)
Convergence: round 3 posted 26 inline comment(s), 12 of them reported for the first time; the previous round posted 28 (4 new). Findings keep coming back to the same files: packages/cli/src/external-agents/acp-subagent-executor.ts (findings in rounds 1, 2; 11 more now); packages/core/src/subagents/subagent-manager.ts (findings in rounds 1, 2; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 8 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally, so the Windows-only spawn failure (R3-22) is unexercised by CI.
未审查（原文为英文）：test-efficacy — the mutation/hunk probe harness could not be validated (harnessValidated: null; the positive control ran and died on the vitest global-setup prerequisite guard), so whether the diff's new tests gate its new behaviour is unmeasured in either direction.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：§9.4's hard constraint that a permission request must be emitted under the parent bridge session id — I read SubAgentTracker.createToolCallHandler but never t…；"agent reverse-audit (round 3)"：§8.4 rows 3/4/6 existence-and-line claims ( cli/src/serve/virtual-subagent-sessions.ts id synthesis/polling and the 5-route whitelist, web-shell/.../SubagentD…；"agent reverse-audit (round 3)"：§8.6 hard-constraint 2's server-side claim ("服务端也没有任何接受 virtual id 的 prompt 路由", DaemonClient.ts:3228,3240 ) — not traced through cli/src/serve/routes .；"agent reverse-audit (round 3)"：§8.8's comparison table and §8.9/决策记录 rationale — read but not independently falsifiable (judgement rows, no code claim to check); no verification performed.；chunk 10：did not execute npm run typecheck or the packages/core subagent vitest files; every claim above is from reading code at HEAD (3463e13974), not from a run.，另有 2 条。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 27 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 26 条行内评论，其中 12 条是首次提出；上一轮发布了 28 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/cli/src/external-agents/acp-subagent-executor.ts（第 1、2 轮已出过发现，本轮又有 11 条）；packages/core/src/subagents/subagent-manager.ts（第 1、2 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
| case ToolConfirmationOutcome.ProceedAlways: | ||
| case ToolConfirmationOutcome.ProceedAlwaysServer: | ||
| case ToolConfirmationOutcome.ProceedAlwaysTool: | ||
| case ToolConfirmationOutcome.ProceedAlwaysProject: | ||
| case ToolConfirmationOutcome.ProceedAlwaysUser: | ||
| return 'allow_always'; |
There was a problem hiding this comment.
[Suggestion] R3-29: All four durable/scope-bearing outcomes are collapsed to a session-scoped ACP allow_always, and the info confirmationDetails this executor emits never sets hideAlwaysAllow, so both approval surfaces offer 'Always Allow in project'/'for this user' grants this path cannot honour — no qwen permission rule is ever written
A trusted-folder user (TUI: ToolConfirmationMessage.tsx:545 pushes ProceedAlwaysProject/ProceedAlwaysUser for type === 'info' whenever isTrustedFolder && !hideAlwaysAllow; ACP/Web Shell: permissionUtils.ts:310-330 builds the same two options for the 'info' case, filtered only by the persistence policy SubAgentTracker passes in) clicks 'Always allow in this project' on an external subagent's tool call. SubAgentTracker.createApprovalHandler (SubAgentTracker.ts:263-268) forwards the outcome straight to event.respond and persists nothing itself — persistence is the responder's job on every other path (coreToolScheduler.ts:3996 persistPermissionOutcome for the in-process subagent, Session.ts:12757 for the primary ACP path). This executor's respond only translates the outcome to an option id, so the click writes nothing: the grant the label promised exists nowhere, the identical call is re-prompted on the next turn and in every later session, and .qwen/settings.json shows no rule for a permission the user believes they recorded. The same click on an in-process subagent DOES persist, so behaviour silently differs based on whether the definition has an executor block.
Witness:
[probe] through the REAL builder toPermissionOptions — 'ARM A (as shipped, no hideAlwaysAllow) -> 4 options: proceed_always_project[allow_always]"Always Allow in project", proceed_always_user[allow_always]"Always Allow for user", proceed_once[allow_once]"Allow", cancel[reject_once]"Reject"' / 'ARM B (hideAlwaysAllow: true) -> 2 options: proceed_once[allow_once]"Allow", cancel[reject_once]"Reject"'. Persistence sweep: persistPermissionOutcome has exactly 2 production call sites (coreToolScheduler.ts:4001, Session.ts:12760) — 0 on the nested/external-executor path. TUI half is quoted, not driven (ToolConfirmationMessage.tsx:547 'if (isTrustedFolder && !confirmationDetails.hideAlwaysAllow)'); the ACP/Web Shell half is the measured one.
Fix: Set hideAlwaysAllow: true on the confirmationDetails emitted with TOOL_WAITING_APPROVAL (acp-subagent-executor.ts:790-794), so both surfaces offer only allow-once/cancel — which allow_once/cancelled can faithfully deliver. Do NOT fix it by persisting: a rule derived from the external agent's tool name (claudeCode.toolName, e.g. Bash) would be consulted by qwen's own scheduler and would over-grant qwen's own tool of the same name.
The fix must not violate: packages/core/src/tools/tools.ts:920-923 states when the flag is mandatory — 'When true, the UI should not show "Always allow" options (ProceedAlwaysProject/User). Set when an explicit interaction or PM ask rule cannot be replaced by a persisted allow rule.' — and the existing precedent for an approval path that cannot persist is restrictWorkflowConfirmationDetails (packages/core/src/agents/workflow-run-registry.ts:1679-1686), which sets hideAlwaysAllow: true on the 'info' variant. Setting the flag makes optionKindForOutcome's allow_always branch unreachable from this path, so that branch and …
Please add the test that pins this — acp-subagent-executor.test.ts: drive onRequestPermission (or capture the emitter's TOOL_WAITING_APPROVAL payload) and assert confirmationDetails.hideAlwaysAllow === true; deleting the flag makes it red. permissionUtils already drops every kind === 'allow_always' option when the flag is set (filterAlwaysAllowOptions, permissionUtils.ts:64-77), so asserting the offered option set contains no allow_always entry also pins it. Remove the fix and confirm that test goes red.
中文说明
四个带持久语义/作用域的确认结果全部被折叠成会话级的 ACP allow_always，而本执行器发出的 info 型 confirmationDetails 从不设置 hideAlwaysAllow，因此两个审批界面都会提供“在项目中始终允许”/“对该用户始终允许”这类本路径无法兑现的授权——不会写入任何 Qwen 权限规则。实测（经真实构造器 toPermissionOptions）：ARM A（现状，无 hideAlwaysAllow）→ 4 个选项，含 proceed_always_project[allow_always]"Always Allow in project" 与 proceed_always_user；ARM B（hideAlwaysAllow: true）→ 2 个选项（proceed_once、cancel）。持久化清扫：persistPermissionOutcome 只有 2 个生产调用点（coreToolScheduler.ts:4001、Session.ts:12760），嵌套/外部执行器路径上为 0。触发：受信任文件夹下的用户（TUI：ToolConfirmationMessage.tsx:547 在 isTrustedFolder && !hideAlwaysAllow 时为 type === 'info' 推出这两个选项；ACP/Web Shell：permissionUtils.ts:309-330 的 case 'info' 构造同样两项，只被一个在 requiresManagedConversationBinding 之外恒为 undefined 的策略过滤）在外部子 agent 的工具调用上点“在项目中始终允许”。SubAgentTracker.createApprovalHandler（:263-268）只把结果转发给 event.respond，自身不持久化——在其他每条路径上持久化都是响应者的职责。本执行器的 respond 只把结果翻译成一个 option id，所以这次点击什么都不写：标签承诺的授权不存在，同一调用在下一回合以及此后每个会话都会再次询问，而 .qwen/settings.json 里看不到用户以为已记录的规则。同一点击落在进程内子 agent 上则会持久化，因此行为会随定义是否带 executor 块而静默不同。修法：在 TOOL_WAITING_APPROVAL 携带的 confirmationDetails 上设置 hideAlwaysAllow: true（:790-794），让两个界面只提供 allow-once/cancel——这是 allow_once/cancelled 能忠实兑现的。不要靠持久化来“修”：由外部 agent 工具名（claudeCode.toolName，例如 Bash）派生的规则会被 Qwen 自己的调度器读取，从而对同名的 Qwen 工具过度授权。tools.ts:920-923 写明了该标志何时是必须的，既有先例是 workflow-run-registry.ts:1679-1686 的 restrictWorkflowConfirmationDetails；设置该标志会使 optionKindForOutcome 的 allow_always 分支从本路径不可达，所以那个分支及其测试（:78-86）必须保留。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Deliberate; leaving open for a maintainer decision. allow_always is narrowed to allow_once when the agent offers both, because an ACP allow_always outlives the subagent turn and this PR has no scope in which to honour a durable grant. The narrowing under-grants relative to what the user clicked, which is the intended failure direction.
There was a problem hiding this comment.
Deliberate fail-closed under-grant, unchanged at 19121e91ae. allow_always is narrowed to allow_once when the peer offers both, because an ACP allow_always outlives the subagent turn and this PR has no scope in which to honour a durable, cross-session grant — so it under-grants relative to what the user clicked, which is the intended failure direction (the opposite, over-granting, is the R1-15 class this PR prevents). This is a Suggestion; left open for a maintainer to accept the posture or specify the scope in which a durable grant should be honoured.
| const match = options.find((option) => option.kind === wantKind); | ||
| if (match) return match.optionId; | ||
| const deny = options.find((option) => |
There was a problem hiding this comment.
[Suggestion] R3-28: selectPermissionOption only ever matches the EXACT requested kind, so an affirmative allow_always answer is thrown away — and turned into a rejection — when the agent offered allow_once but not allow_always, although granting strictly less than the user authorized is available and safe
The rule as written is symmetric, but only one direction is safe to deny. An external agent offers [{kind:'allow_once'},{kind:'reject_once'}] — the author's own test fixture at acp-subagent-executor.test.ts:161-164 is the mirror image of this set, so a partial offer set is already conceded as reachable, and the file header states the executor must tolerate drifting adapters. The user clicks 'Allow always' (ProceedAlways*); wantKind is allow_always, match is undefined, the deny fallback fires, and the executor answers the agent with its REJECT option. The user just approved and the tool call is refused: the agent aborts or retries the step, the parent turn reports a denial, and nothing tells the user their approval was discarded. In-process the same click executes the call (coreToolScheduler.ts:3996 treats ProceedAlways* as proceed and additionally persists a rule).
Witness:
[probe] tsx, scratch tree, real module — intact PR: 'offered=[allow_once, reject_once] outcome=proceed_always -> "no"' (same for proceed_always_project, _user, _server, _tool); with the implied one-directional narrowing patched in: '-> "once"' for all five, while the escalation control stayed 'offered=[allow_always, reject_once] outcome=proceed_once -> "no"' in BOTH arms (the existing pin at acp-subagent-executor.test.ts:185 is not broken). Reverted and re-ran intact: back to '"no"'. Residual uncertainty is only whether a given adapter ever omits allow_always — the author's own fixture (test.ts:161-164) is the mirror-image partial set, so partial offers are conceded reachable.
Fix: Before denying, fall back only in the narrowing direction — when wantKind === 'allow_always', take an offered allow_once if present — and deny only when the peer offered no allow-kind option at all.
The fix must not violate: The narrowing must stay one-directional — the existing pin it('denies rather than widening a one-time approval to the session', …) at acp-subagent-executor.test.ts:185 requires ProceedOnce against [allow_always, reject_once] to keep returning the reject optionId, so allow_once must never fall back to allow_always.
Please add the test that pins this — acp-subagent-executor.test.ts, describe('selectPermissionOption — never escalates (R1-15 / R2-2)') at :160: add it('narrows an always-grant to allow_once when that is all the agent offered') asserting selectPermissionOption([{optionId:'once',kind:'allow_once'},{optionId:'no',kind:'reject_once'}], ToolConfirmationOutcome.ProceedAlwaysProject) === 'once'; it goes red while the deny fallback runs first. Remove the fix and confirm that test goes red.
中文说明
selectPermissionOption 只匹配完全相同的 kind，因此当 agent 只提供了 allow_once 而没有 allow_always 时，一次肯定性的“始终允许”会被丢弃——并被转成拒绝，尽管授予严格少于用户授权范围的选项是可用且安全的。规则本身是对称的，但只有拒绝这一侧是不安全的。实测（真实模块）：未修改的 PR 下 offered=[allow_once, reject_once] outcome=proceed_always -> "no"（proceed_always_project/_user/_server/_tool 同样），加入只朝收窄方向的回退后五种全部 -> "once"，而升级方向的对照在两个 arm 中都保持 offered=[allow_always, reject_once] outcome=proceed_once -> "no"（既有钉住 acp-subagent-executor.test.ts:185 未被破坏）；还原后重跑回到 "no"。触发：外部 agent 提供 [{kind:'allow_once'},{kind:'reject_once'}]——作者自己的测试夹具（:161-164）正是这个集合的镜像，说明部分提供集已被承认可达，且文件头声明执行器必须容忍漂移的 adapter。用户点“始终允许”，wantKind 为 allow_always，match 为 undefined，拒绝回退触发，执行器用 agent 的拒绝选项作答：用户刚刚批准，工具调用却被拒，agent 中止或重试该步骤，父回合报告一次拒绝，而没有任何东西告诉用户他的批准被丢弃。进程内同一次点击会执行该调用（coreToolScheduler.ts:3996 把 ProceedAlways* 视为 proceed 并额外持久化规则）。剩余不确定性仅在于某个 adapter 是否真的会省略 allow_always。修法：在拒绝之前只朝收窄方向回退——wantKind === 'allow_always' 时若存在 allow_once 就取它——只有对端完全没提供任何 allow 类选项时才拒绝。收窄必须单向：既有钉住要求 ProceedOnce 面对 [allow_always, reject_once] 继续返回拒绝选项，所以 allow_once 绝不能回退到 allow_always。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Not fixed; leaving open, same direction as R3-29. Matching is exact-kind, then the allow_always → allow_once narrowing, then reject_once/reject_always, then deny — so an affirmative option worded differently is denied rather than guessed at.
There was a problem hiding this comment.
Deliberate fail-closed under-grant, unchanged at 19121e91ae. selectPermissionOption matches the exact requested kind, then narrows allow_always → allow_once, then falls back to reject_once/reject_always, then denies — so an affirmative option worded differently from the requested kind is denied rather than guessed at. Guessing a differently-worded affirmative risks granting more than the user approved (the R1-15 escalation this PR exists to prevent); denying is the safe direction. This is a Suggestion; left open for a maintainer to accept or reject the posture rather than silently widening the match.
3463e13 to
8c39916
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
Finding status at head
|
| ID | Status |
|---|---|
R2-1 malformed frontmatter executor dropped → silent in-process run |
Fixed. Loader now throws SubagentError(INVALID_CONFIG); the SubagentExecutorSpec doc asserting the opposite lenient intent is corrected in the same change. Consumption-point re-validation retained for the session-injection path. Test still missing. |
R2-5 methodNotFound serialized as -32603 |
Fixed. Returns RequestError.methodNotFound; extMethod threads the real method name. Wire test still missing. |
R2-14 onChildExit emitted Node's 'error' with no listener |
Fixed. Guarded by rawListeners(ERROR).length > 0; terminateMode is what surfaces the failure. Test still missing. |
| R2-2 deny fallback untested; mutant shipped green | Fixed. Rule extracted to exported selectPermissionOption, 8 tests added. Mutation-verified: replacing the deny fallback with options[0] fails 3 of them, including the exact escalation R1-15 described. |
Round 1
| ID | Status |
|---|---|
| R1-15 permission fallback could grant more than approved | Fixed (round 2), now test-covered and mutation-verified. |
R1-13 optionKindForOutcome defaulted to allow_once — fails open |
Fixed. Exhaustive over the enum with a never assignment so a new member fails to compile, and a runtime default that denies. RestorePrevious was the live case. Test added asserting no non-proceeding outcome maps to an allow kind. |
| R1-3 dispatch forwards declared config the executor never consumes | NOT ADDRESSED. Still Critical. The executor receives toolConfig and ignores it, so a definition's tools / disallowedTools — including the read-only pinning /coordinate relies on — does not constrain the external agent. |
R1-26 executeExternalInputs does not implement the turn contract |
NOT ADDRESSED. Still Critical. Documented as a known limit in the executor header and the commit message, not fixed: finalText accumulates across turns and resetStats is ignored. |
R1-49 onRequestPermission parks every request with no deny-fallback |
NOT ADDRESSED. Still Critical. If no human answers, the request stays parked; onChildExit now drains on child death, but there is no timeout. |
| R1-6 report-0 tokens conditioned on a diagnostic the diff never implements | NOT ADDRESSED. The design doc still conditions the decision on an honesty compensation that does not exist in code. |
| R1-7 load-bearing behaviours pinned by no test | Partly addressed. selectPermissionOption and optionKindForOutcome now are; spawn, handshake, translation and disposal still are not. |
R1-16 pendingPermissions keyed by callId, .set() overwrites |
NOT ADDRESSED. |
| R1-19 listener on an already-aborted signal never fires | NOT ADDRESSED. |
R1-24 re-throw passes only SubagentError; plain executor Errors get mislabelled "Failed to create AgentHeadless" |
NOT ADDRESSED. |
R1-38 dispose sends one SIGTERM, no exit verification, no SIGKILL escalation |
NOT ADDRESSED. The earlier "child reaped" claim rests on one pgrep after a clean run, not on a test. |
R1-44 resetStats honoured only by the in-process executor |
NOT ADDRESSED (same root as R1-26). |
R1-52 no test pinning the ./subagentRuntime subpath |
NOT ADDRESSED. |
| R1-55 workflow schema mode can dispatch an executor definition that cannot satisfy it | NOT ADDRESSED. |
| R1-8 / R1-9 design doc line citations systematically wrong; doc records states the code contradicts | NOT ADDRESSED. The doc is committed as-is. Given R1-6 above, at least one contradiction is now known and unfixed. |
| Stage-1 framing note (silent substitution is documented lenient-parser behaviour, not a shipped defect) | Accepted, not applied. The body still leans on the measurement more than it should. |
Verification at this head
Repo typecheck 0 errors, ESLint clean, Prettier clean, cli executor 19/19, core subagent and agent-runtime 357/357, git diff --name-only origin/main = exactly the 18 owned files.
Not run: the multi-pass directionless audit this repo's workflow requires before commit. The commit was made without it, which is a deviation from the project's own pre-commit rule and is stated in the commit message.
Summary
Three round-1 Criticals (R1-3, R1-26, R1-49) remain open, and none of the three round-2 fixes has the test its review asked for. This PR should not merge on this head.
a76d160 to
3e01fb2
Compare
Finding status at head
|
| ID | Status |
|---|---|
R1-39 resolvePermissionMode preferred raw permissionMode over approvalMode |
Fixed. Precedence inverted to approvalMode ?? permissionMode, normalized and lower-cased, default resolves to the mode that asks. Unit-tested. |
R1-38 no detached, killChild signalled only the direct child |
Fixed. Child is spawned detached (POSIX) and owned by acp-bridge's ProcessRegistry with ownsProcessTree, so disposal escalates SIGTERM to SIGKILL across the tree. Test drives a real child that ignores SIGTERM and has a grandchild; both are reaped. dispose() suppresses only the registry's own clean-shutdown signal error, so a cleanup-proof failure still throws. |
R1-4 permission mode delivered only as session/new _meta |
Fixed. The resolved mode must appear in session.modes.availableModes and is applied with session/set_mode before the first prompt; a missing or rejected mode fails create(). _meta.permissionMode is gone. Tests cover no-mode, mode-error and mode-hang. |
| R1-19 already-aborted signal never fired the listener | Fixed. wait() checks signal?.aborted at wiring time, and runTurn returns CANCELLED without prompting when the signal is already aborted. |
| R1-18 mid-turn child death had no settle path | Fixed. child.exited and connection.closed both abort a shared failure controller that races every awaited ACP operation, so a dead agent settles instead of hanging. Tests: prompt-exit, prompt-close, prompt-hang. |
R3-3 execute()'s catch emitted AgentEventType.ERROR unguarded |
Fixed. Same rawListeners guard as onChildExit; terminateMode is what surfaces the failure. |
R3-4 inbound JSON-RPC errors stringified to [object Object] |
Fixed. The SDK's RequestError extends Error (verified in @agentclientprotocol/sdk dist), so the instanceof Error ? error.message branch carries the code and message. Outbound methodNotFound is a real RequestError — mutation-verified: a plain Error with a code property fails the -32601 wire assertion. |
R1-26 / R3-5 executeExternalInputs skipped the turn contract and read only the object member |
Fixed. Both entry points funnel into one runTurn: stats reset honours resetStats, finalText/thoughtText are cleared per turn, START/ROUND_TEXT/FINISH are emitted, duration accumulates, and the wall-time cap applies. String and object inputs are both accepted and both emit EXTERNAL_MESSAGE with the right kind. |
R3-6 setExternalMessageProvider discarded the provider |
Partially fixed. The provider is stored and drained between completed prompts, with the queued inputs emitted as a continuation round. The optional setExternalMessageWaiter / setExternalMessageWaitPredicate are still not implemented, so unlike AgentHeadless an external executor cannot park waiting for a message that has not arrived yet. Core's background dispatch wires those two by optional call for external subagents too; I have not traced what the missing park does to SendMessage delivery between rounds, so I am not claiming it is benign. |
R3-7 max_tokens mapped to GOAL |
Fixed. end_turn to GOAL, max_turn_requests to MAX_TURNS, cancelled/refusal to CANCELLED, everything else — including max_tokens and unknown values — to ERROR. A truncated turn can no longer be certified as complete. Tested. |
R1-42 no ROUND_TEXT / TOOL_RESPONSES_FINALIZED |
Fixed. ROUND_TEXT carries text and thought per round and in finally; TOOL_RESPONSES_FINALIZED is emitted with the functionResponse parts alongside each TOOL_RESULT. |
R3-8 agent_thought_chunk dropped |
Fixed. Handled in the same case as agent_message_chunk, accumulated into thoughtText and streamed with thought: true. |
| R1-49 every permission request parked unconditionally | Fixed for the unconditional part. Requests are now denied outright when the host cannot answer: non-interactive without the Zed integration or STREAM_JSON, getShouldAvoidPermissionPrompts(), no TOOL_WAITING_APPROVAL listener (a display-only listener is not a responder), empty options, duplicate callId, duplicate optionIds, and AskUserQuestion / ask_user_question spellings. Parked requests are drained on cancel, failure and disposal. Still no parking timeout: an interactive host whose user never answers still parks until the turn's wall-time cap or child death. |
| R1-29 resume could receive the ACP executor | Fixed. Cold resume is denied for AgentMeta.executor === 'acp' and for legacy external-acp:* model labels, at discovery and again at execution. The label is deny-only and never selects an executor. 4 tests, createAgentHeadless never called. |
| R1-1 executor branch spawned a project-supplied command with no trust gate | Fixed. A level === 'project' definition is refused when isTrustedFolder() is false, before spawn, alongside the other consumption-point checks. User-level definitions are not gated, since those are not repo-supplied. |
R1-2 raw approvalMode / permissionMode forwarded |
Fixed at both ends. The manager forwards the declared values without inventing a mode, and the executor resolves and enforces one before any prompt (see R1-39, R1-4), so the external agent's own config can never be the authority. |
R1-3 runConfig / toolConfig / hooks forwarded but ignored |
Fixed by rejection instead of enforcement. A definition or dispatch that declares tools, disallowedTools, mcpServers, hooks, maxTurns, runConfig.max_turns, runConfigOverrides.max_turns, a non-inherit model, modelConfigOverrides, runtimeAuthOverrides, toolConfigOverride, a rendered system prompt, injected initial messages or hook callbacks is refused with the unsupported keys named. The executor itself consumes only max_time_minutes and rejects max_turns. Residual: the toolConfig, hooks and taskName members remain on ExternalAgentExecutorParams while the executor reads none of them. |
| R2-1 the loader's malformed-executor throw was swallowed | Fixed. warnInvalidSubagentFile routes executor rejections to console.warn — visible without debug logging — while other parse failures stay on the debug channel, and discovery continues past the bad file. Test asserts the visible warning and that a sibling valid file still loads. |
R1-35 executor missing from serializeSubagent |
Fixed. The serializer re-validates with parseAgentExecutor and throws before writing rather than persisting a definition that would lose its backend, then writes the block. Round-trip test over real YAML. |
| R1-34 the JSDoc "never silently run in-process" guarantee held only for some paths | Fixed. The guarantee now holds at every dispatch path I could find: the manager's external branch, convertToRuntimeConfig (the team/background conversion path, which previously allowed a silent in-process substitution), and workflow agent(). |
R3-1 no Windows spawn path (npx.cmd) |
NOT FIXED. detached is gated off win32 and windowsHide is set, but a .cmd launcher still needs a shell on Windows and none is used. No Windows run has been attempted. |
R3-2 no ROUND_START / ROUND_END / USAGE_METADATA |
NOT FIXED. Only START, STREAM_TEXT, ROUND_TEXT, TOOL_CALL, TOOL_RESULT, TOOL_RESPONSES_FINALIZED, EXTERNAL_MESSAGE, FINISH and guarded ERROR are emitted. USAGE_METADATA has no honest source: the adapter exposes a context-window gauge, not per-turn token deltas. |
| R3-9 durable outcomes collapsed to a session-scoped allow | Deliberate, not fixed. allow_always is narrowed to allow_once when the agent offers both, because an ACP allow_always outlives the subagent turn and this PR has no scope in which to honour a durable grant. The narrowing is fail-closed — it under-grants relative to what the user clicked. |
| R3-10 exact-kind match only; an affirmative option with different wording is denied | NOT FIXED. Still exact-kind, then the allow_always→allow_once narrowing, then reject_once/reject_always, then deny. Under-granting is the intended failure direction. |
R3-11 PendingPermission.options written and never read |
Fixed by removal. The pending map now stores only the deny callback; the response closure holds the options. |
Round 2
| ID | Status |
|---|---|
R2-1 malformed frontmatter executor dropped |
Fixed, now tested. Parse-side rejection of the original YAML node, consumption-point re-validation, serialization re-validation, and the visible warning above. Mutation-verified: removing the frontmatter guard fails 8 tests. |
R2-2 methodNotFound serialized as -32603 |
Fixed, now wire-tested. The fixture agent asserts -32601 on the wire; the plain-Error mutant fails it. |
R2-3 / R2-14 onChildExit emitted Node's 'error' with no listener |
Fixed. rawListeners guard; terminateMode carries the failure. |
| R2-4 deny fallback untested | Fixed. selectPermissionOption exported, 8 tests, mutation-verified: options[0] fails 3, including the exact ProceedOnce → allow_always escalation. |
Round 1
| ID | Status |
|---|---|
R1-13 optionKindForOutcome failed open |
Fixed. Exhaustive with a never assignment and a runtime default that denies; RestorePrevious was the live case. |
| R1-15 permission fallback could grant more than approved | Fixed (round 2), test-covered and mutation-verified. |
R1-16 pendingPermissions.set() overwrote a parked request |
Fixed. A duplicate callId is denied instead of overwriting, and finish checks that the stored callback is still its own before resolving. |
| R1-24 plain executor Errors mislabelled "Failed to create AgentHeadless" | Fixed. The outer catch passes through SubagentError and anything raised on an executor path; test asserts the original error object identity. |
R1-44 resetStats honoured only in-process |
Fixed. runTurn resets round, duration and tool counters unless resetStats === false; execute and executeExternalInputs both accept the option. |
R1-52 no tsconfig paths mapping / no test pinning the subpath |
Fixed. packages/cli/tsconfig.json maps @qwen-code/qwen-code-core/subagentRuntime to source ahead of the wildcard (verified with tsc --traceResolution), and a cross-package contract test pins the package.json export entry, the barrel's re-exports, the vitest alias and the tsconfig mapping. 13/13. |
| R1-55 workflow schema mode could dispatch an executor definition | Fixed, and widened. Workflow agent() now rejects every external-executor definition before spawn — bounded and unbounded, schema or not — because token budgets, schema output and workflow tool restrictions cannot be enforced across the process boundary. 6 tests plus an in-process positive control; mutation-verified: removing the guard fails all 6. |
| R1-6 report-0 tokens conditioned on a diagnostic the diff never implements | Addressed differently. No diagnostic was added. Honesty is now enforced where the number is presented: the Agent tool suppresses the execution summary and completion stats for external subagents, skips live-stat refresh, and appends "[External executor token usage and cost are unavailable.]" to the result; transcript metadata records executor: 'acp'. The rewritten design doc no longer conditions the decision on a diagnostic. The executor's own getExecutionSummary() still returns zeros. |
| R1-7 load-bearing behaviours pinned by no test | Substantially addressed. 39 executor tests now drive a real ACP child process over the wire: handshake hangs and exits, mode negotiation, prompt hangs/exits/close, max_tokens and unknown stop reasons, permission denial paths and duplicate ids, descendant tree kill, env sanitization, unsupported-extension -32601. Gap: nothing pins the CLI's lazy setExternalAgentExecutor registration in loadCliConfig itself. |
| R1-8 / R1-9 design doc citations wrong; doc recorded states the code contradicts | Fixed by rewrite. The doc is 144 lines, describes only the shipped product and its fail-loud rules, records the peer-backend alternative as rejected, states session/set_mode as the mechanism rather than the disproved _meta claim, and carries no file.ts:line citations to go stale. Acceptance criteria are marked pending rather than claimed. |
| R1-56 child inherited the full parent environment | Fixed. sanitizeChildEnv(process.env), because executor.command comes from a project-level file and must not receive the daemon bearer token. Test asserts the spawned child's env. |
R1-12 unconditional await import with a false lazy-import comment |
Fixed. The factory is registered as a thunk that imports the ACP module only when an external subagent is actually requested, and the comment now says what the code does. |
| R1-35, R1-34, R1-29, R1-26, R1-19, R1-18, R1-16, R1-15, R1-13, R1-4, R1-3, R1-2, R1-1 | See the round-3 rows above; all were re-checked at this head rather than carried forward. |
Verification at this head
Re-run after the rebase onto 1b604721b0, not carried over: packages/core and packages/cli tsc --noEmit 0 errors; ESLint and Prettier clean over the changed files; git diff --check clean; cli executor 39/39; core targeted suites 807/807 (src/subagents, background-agent-resume, workflow-orchestrator, tools/agent); cross-package contracts 13/13; git diff --name-only origin/main = exactly the 25 owned files.
Four mutation proofs, each run in both directions: frontmatter executor guard (8 tests red), workflow agent() rejection (6 red), options[0] permission fallback (3 red), plain Error instead of RequestError (-32601 wire assertion red).
Still open, stated plainly
- R3-1 Windows spawn. Not implemented, not run. A
.cmdlauncher will fail today. - R3-6 waiter/predicate. An external executor cannot park for a not-yet-arrived external message; I have not traced the consequence for background
SendMessagedelivery and am not claiming it is safe. - R1-49 parking timeout. Interactive host, user never answers: the request parks until the wall-time cap or child death.
- R3-2 round events.
ROUND_START/ROUND_END/USAGE_METADATAare not emitted. - Real Claude Code end to end. The earlier macOS run (delegated file produced, metadata
external-acp:node,Bashin the transcript, zero qwenwrite_filecalls) predates this round's executor rewrite. Since the rewrite the permission and mode behaviour is proven against the fixture ACP agent, not the shipped@agentclientprotocol/claude-agent-acpadapter. A re-run is outstanding. - Audit convergence. The multi-pass directionless audit this repo requires did not converge: the audit agents died on infrastructure failures, so what actually happened was doc/contract-scope passes plus direct full-diff review by me, and the four mutation proofs above. That is weaker than the rule asks for and is recorded in the commit message.
- R3-10 / R3-9 remain deliberate under-grants, not oversights.
My own read: items 1, 2, 5 and 6 are the ones that should block a merge decision. Items 3, 4 and 7 are honest limitations that could ship documented, and 2 is the one I would fix next because I cannot currently bound its blast radius.
Follow-up at the same head
|
| Case | Elapsed | Wire msgs | session/set_mode |
Wire outcome | Filesystem |
|---|---|---|---|---|---|
Deny (Cancel) |
9961 ms | 26 | default, before prompt |
{"outcome":"cancelled"} |
target absent, Write failed |
Allow (ProceedOnce) |
8705 ms | 26 | default, before prompt |
{"outcome":"selected","optionId":"allow-once"} |
target exactly PR11003_ALLOW\n, Write succeeded |
What this proves against a real adapter, not just the fixture:
- R1-4 / R1-39: the factory was handed conflicting inputs (
permissionMode: "auto",approvalMode: "default") and the wire shows exactly onesession/set_modewithmodeId: "default"before any prompt. Precedence and mode-before-prompt hold end to end. - R1-15 / R2-2: the real adapter offered
allow-once(allow_once),allow-with-updates(allow_always),reject(reject_once).ProceedOnceansweredallow-once— it did not take the broader session-wide grant that was on offer. The narrowing rule is now confirmed against real option data. - R1-49: exactly one
TOOL_WAITING_APPROVALper run, the responder's answer reached the wire, andCancelproduced no file. - Token honesty: both runs ended
GOALwithgetExecutionSummary()reporting zero tokens, which is precisely why the Agent tool suppresses the summary and appends the unavailability notice instead of presenting zeros as free.
Evidence: .qwen/e2e-tests/pr11003-permission-proof/ (report.md, proof.mjs, results.json, deny-wire.jsonl, allow-wire.jsonl; the pre-rebase run kept as *.prev). The 25 PR files are byte-identical between 24801d6362 and 3e01fb25dd, so the earlier run exercised the same executor source; this re-run ties it to the pushed head.
Still not covered by this proof: the settings loader forwarding a user's permissionMode into a definition (the harness supplied both values directly), the Web Shell approval dialog in a browser, more than one permission request in a turn, headless auto-denial against the real adapter, and Windows.
Item 2 (R3-6 waiter gap) — I traced it, and my previous wording was too alarmist
I wrote that I "cannot currently bound its blast radius". I have now read the path, so here is the bounded version.
The background loop in the Agent tool already drains the registry after every GOAL turn and re-invokes executeExternalInputs(pending, signal, { resetStats: false }), and when the queue is empty it calls beginFinishing, which makes a racing send_message be rejected rather than silently orphaned. So queued messages are not lost for an external background subagent — that part is fine, and resetStats: false is now honoured by the executor, which is what makes the continuation stats accumulate correctly.
The real difference is narrower: without setExternalMessageWaiter / setExternalMessageWaitPredicate, an external subagent cannot park inside a turn waiting for a message that has not arrived yet. The in-process AgentCore parks when the predicate says a Monitor is still running for that owner, which keeps the agent alive to receive a later notification. An external one finishes at its own end_turn and the queue closes.
One honest caveat: an external subagent runs the foreign agent's own tools, so it is not obvious that it can ever own a Qwen Monitor — in which case the predicate would be false anyway and this gap is unreachable. I have not established that either way, and I have not measured a Monitor run.
Two ways to close it, and I would rather ask than pick silently:
- Reject Monitor ownership for external subagents — small, fail-loud, consistent with how workflow
agent()and the unsupported-constraint list already handle things this PR cannot enforce. - Implement the waiter/predicate on the ACP executor — park between prompts within the remaining wall time. Larger: it needs a nudge-prompt decision when the wait yields nothing (AgentCore sends "Please provide the final result now and stop calling tools."), and for an external agent that is another real, billable prompt call whose semantics I would not want to invent unasked.
Unchanged from the previous comment
Item 1 (Windows .cmd spawn) and item 3 (no parking timeout for an interactive host whose user never answers) are still open. Item 6 still stands: the multi-pass directionless audit did not converge to consecutive clean passes because the audit agents died on infrastructure failures; what ran was doc/contract-scope passes, direct full-diff review by me, and the four mutation proofs. Items 4, 7 and the rest of the R3 rows are as previously stated.
CI at the time of writing: Lint & Static, Test (ubuntu), Integration Tests (no-AK) and review-pr still pending; everything else that ran passed, with windows/macos/CLI-integration lanes skipped by the workflow.
3e01fb2 to
7526d58
Compare
Thread resolution, and one defect I found in my own pushed headResolved conversationsEvery inline thread whose finding is fixed at this head now has a reply stating what changed and which test or mutation proof pins it, and is marked resolved: 73 of 80 threads. The 7 left open are the ones I am not claiming:
ID note: the inline comment IDs and the IDs in the ledger trailers disagree for round 3 (inline Two status refinements rather than restatements, both visible in the thread replies:
A defect in my own pushed head, found after pushing, now fixedThe head I pushed an hour ago ( What exposed it: CI's Fixed at Verification after the fix, all re-run rather than carried over: core and cli The one remaining local failure is The push was |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review at head 9aa279a4. The two Criticals this branch has been carrying since round 3 are now closed in the code, and I verified both. This is still a comment rather than an approval because the current-code gate for a diff of this size is not something I completed in this run's budget, and the unit/lint/integration jobs had not finished at this head when I wrote this.
Carried Criticals — resolved at this head
R3-1 / R3-22 (the peer spawn had no Windows path). The remedy is an explicit, fail-closed platform scope rather than a Windows spawn: assertExternalAgentSpawnPlatformSupported() is exported from acp-subagent-executor.ts:94-116 and is the first statement of AcpSubagentExecutor.create() (:369), ahead of the max_turns/max_time_minutes validation and well ahead of the spawn at :384. On win32 it throws a message that names the actual mechanism (an npm-installed launcher resolves to .cmd, which libuv's PATH search does not find and which Node refuses to spawn without a shell; the process-group reaping is POSIX-only) and the workaround (macOS/Linux/WSL), with Windows support recorded as a tracked follow-up. So the failure is no longer a misleading spawn <cmd> ENOENT against an adapter that is installed, and it happens before any process is created.
I also checked how that throw travels: subagent-manager.ts:998 awaits externalExecutor.create(…) inside a try whose catch rethrows unchanged when config.executor !== undefined (:1143-1149), with the inner catch running runCleanup() first (:1135-1142), so the platform error reaches the dispatcher verbatim — no re-wrap under "Failed to create AgentHeadless", no unhandled rejection, no leaked hook or registry entries.
R3-6 / R3-51 (the optional mid-turn input waiters were absent). This is now a recorded scope decision with a user-visible consequence rather than a silent gap: acp-subagent-executor.ts:823-831 documents why setExternalMessageWaiter / setExternalMessageWaitPredicate are deliberately not implemented (ACP v1 has no mid-turn injection primitive, so true steering would mean cancel-and-re-prompt and re-billing an in-flight turn), and states the delivery contract that replaces it — queued input is drained between prompts through the provider. agent.ts:140-146 adds EXTERNAL_MID_TURN_INPUT_NOTICE (constant at :144) and :3603 appends it to externalSuffix whenever subagentConfig.executor !== undefined, so the model-visible result tells the caller that a message sent during a running turn lands at the next turn boundary, not mid-turn. Both changes carry tests (acp-subagent-executor.test.ts +28, agent.test.ts +24).
The rest of the branch's blocking history was closed in the two preceding commits and is unchanged by this one: the round-12 items (system-prompt re-send gated on continuation, entry emitInputs moved below the budget guard and keyed on entryRound, renderPromptAsPlainText on the approval info variant) and the round-10/11 items on refusal recording, the permission-mode vocabulary and the wall-time budget. The only threads still open are the two Suggestion-level ones on permission-option selection (R3-28/R3-29), which the author has recorded twice as a deliberate fail-closed under-grant pending a maintainer decision — non-blocking.
One residual worth a line, not a blocker: the design doc this PR adds still walks a reader through configuring an npm-installed adapter without stating the Windows scope the new guard establishes, so the platform limitation currently lives in the runtime error and the code comment.
Gate not completed in this budget
What I did not re-read at this head, and the reason this is not an approval: the executor's turn loop, stop-mode mapping and teardown (~1,100 lines), the agent.ts restructuring beyond the new notice (+108/-62), background-agent-resume.ts, the workflow-orchestrator.ts external-executor refusal, agent-frontmatter-schema.ts, and the extensionManager.ts wiring. Two runs on this PR have closed the reported Criticals one round at a time; the remaining surface needs a single pass by someone with the budget to read it end to end, which is also what this repository's rule for a large feat change under packages/core/src/** asks for.
CI at this head
Install on ubuntu, macOS and windows, precheck-pr, Classify PR and the TUI parity gate are green; Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox) and the OpenTUI no-flicker gate were still running when I wrote this, and five route checks are cancelled. I did not wait on them, and nothing in the rollup is evidence of a defect here — but the new guard and notice have not yet been executed by a completed unit-test job at this commit.
Next step
Let the pending jobs finish, then have a maintainer read the remaining surface above in one pass. Nothing in this review asks for a code change: the two carried Criticals are closed as of this commit.
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 173 passed · 0 failed · 173 total Flakiness gate: ✅ 8 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：173 通过 · 0 失败 · 173 总计 抖动门：✅ 8 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11003 deep verification —
|
| # | previous finding | severity | status at head 9aa279a4 |
evidence |
|---|---|---|---|---|
| F1 | Reviewer Test Plan's registered no external agent executor oracle is unreachable in the shipped CLI |
Suggestion | fixed (stands fixed) | Mutant M9-consume-no-executor killed by createAgentHeadless — external executor dispatch refuses to run in-process when no executor is registered; the single unconditional registration is still at packages/cli/src/config/config.ts:2483 |
| F2 | The original-YAML-node guard is load-bearing but unpinned (suite's vi.mock never reproduced null-stripping) |
Suggestion | fixed (stands fixed) | Mutant M1-original-yaml-node killed by rejects an executor whose null argument the shared parser would sanitize away — the real-shared-parser fixture the previous round recommended |
| F3 | The executorLine === undefined || fail-closed clause is unpinned but load-bearing |
Suggestion | stands — not addressed | Mutant M7-executorLine-failclosed SURVIVED again (1498 collected, 0 new failures, 0 lost baselines). No test in the suite mentions ? executor, explicit-key, or executorLine. Clause still present at subagent-manager.ts:2124. Load-bearing re-proven below. |
| H4 | Mutation matrix partial (5 of 18 rows) | — | superseded | 9 rows run this round, scoped to the delta plus the rows that decide F1/F2/F3; see Mutation matrix. The 9 rows I did not re-run are listed in Not covered. |
| G1 | 3 subagent-manager.test.ts failures from the container's real home ≠ mocked /home/user |
note | stands (unchanged, still pre-existing) | Head: 1489 passed / 3 failed / 6 pending, 1498 collected. The 3 names are the same 3; all 3 messages are home-path mismatches (expected '/home/user/.qwen/agents/…', received '/test/project/…' / '/__w/_temp/verify-agent-home/…'). None touches the executor path. |
F3 stands, and it is still load-bearing — measured, not inferred. Witness: 03-mutation-matrix-and-m7-load-bearing.png. The A/B corpus fixture C5-explicit-key uses YAML explicit-key syntax, the one shape where claimsExecutor is true while the text probe does not match:
? executor
: {kind: acp, command: npx}
name: charlie-five
name: charlie-five-b
description: dFeeding that frontmatter to the same yaml library the shipped probe uses:
| guard input | value |
|---|---|
hasExecutor |
true |
| text probe matched | false → executorLine = undefined |
document.errors |
1 — Map keys must be unique at line 4 |
reachingError with the clause |
true → refuses |
reachingError without the clause |
false (4 >= undefined is false) and astLostExecutor is false → loads |
The head arm of the sweep measured the refusal behaviourally: C5 → refused, "its frontmatter YAML does not parse (Map keys must be unique at line 4…)". The M7 arm is computed from the guard's own inputs rather than from a rebuilt dist (budget) — labelled as such. Net effect of deleting the clause: a definition whose frontmatter does not parse dispatches a definition-supplied binary. Classification unchanged: coverage gap, not dead code and not redundant defence. Per the repo's own review rule this is a Suggestion — the shipped behaviour is correct and fail-closed; nothing asserts it. The fixture the previous round proposed would still pin it, and M1/M9/M14 prove the core command collects tests that exercise this exact file (1, 1 and 11 new reds respectively), so the survivor is interpretable without a further control.
Central claim and A/B proof
Central claim: a subagent definition that declares an executor is either honoured (a valid spec is produced) or refused loudly — it can never load executor-less and silently run in-process under a Qwen model.
Secondary claim tested: the parser rebuilds the spec from known fields only, so unrecognized keys cannot reach the spawn site.
A/B sibling sweep — 126 assertions, 0 failures
Witness: 01-ab-sibling-sweep-base-vs-head.png. Both arms drive their own tree's real compiled dist/ through the real public SubagentManager.parseSubagentContent. No mocks. Base is a scratch git worktree at HEAD^1 with only packages/core rebuilt. The corpus was rebuilt from the mechanism (the previous round's is unreachable) at 39 fixtures, sweeping the siblings of the one root cause — the YAML AST and the raw text disagreeing about whether an executor was declared:
| group | fixtures | what it probes | base | head |
|---|---|---|---|---|
| A — well-formed | 9 | block mapping, no-args, "executor"/'executor' quoted keys, flow mapping, tab before colon, block scalar above, command needing trim, empty args |
9 × null |
9 × spec |
| B — malformed | 9 | kind: ACP, blank command, null, scalar, array, args:[null], args:[1,2], no command, unknown kind |
9 × null |
9 × refused |
| C — AST/text disagreement | 5 | quoted scalar above the block, tab error before executor, duplicate name: before, duplicate name: after, explicit-key ? executor |
5 × null |
3 × spec, 2 × refused |
| D — over-refusal controls | 8 | no executor, nested executor: in a well-formed file, # executor: comment, executorKind:, block-scalar prose, CRLF, sequence item - executor:, executorExtra: |
7 × null, 1 × refused |
identical — no over-refusal |
| E — accepted-cost boundary | 7 | nested token ± tolerated error, deep nesting, sequence/comment/block-scalar siblings, CRLF + executor | 6 × null, 1 × refused |
2 × refused, 5 unchanged |
| F — hostile keys | 1 | cwd, shell, detached, uid, env.INJECTED alongside a valid block |
null |
spec |
Flip: 24/24 of the flip-eligible executor-declaring fixtures changed from silently-in-process on base to spec-or-refused on head. The 25th declaring fixture (E2, CRLF) is an A/A control, not a flip: base and head refuse it with a byte-identical message (Failed to parse subagent file: Validation failed: System prompt must be at least 10 characters long), so CRLF agent files are broken on both arms for a pre-existing reason unrelated to this PR. D6 (CRLF, no executor) is the same control without an executor and refuses identically — that is what makes the attribution safe.
The adjudicator does not merely compare outcomes; it exposes the guard's own four decision inputs per fixture (hasExecutor / probe-matched / executorLine / error count / reachingError) and requires that any spec outcome on a file with YAML errors be justified by reachingError === undefined && hasExecutor === true. That check is what makes C1–C3 evidence rather than a tuned expectation: each has 1 error strictly before the executor line, so parseDocument's kept subtree is byte-faithful and the narrowing rule says load. C4 has its error at/after the executor line and is refused.
Secondary claim (hostile keys) re-confirmed. F1 declares cwd: /tmp/evil-cwd, shell: true, detached: false, uid: 0, env.INJECTED and executor.cwd / executor.shell alongside a valid block. The parsed spec's keys are exactly ["args","command","kind"] and its value exactly {kind:'acp', command:'npx', args:['-y','some-acp']} — nothing else survives to the spawn site.
Measured boundary of the author's documented accepted over-refusal
The guard's comment names an accepted cost: "an executor: token nested under another key (e.g. metadata:) in an otherwise-malformed file is also treated as a claim and refused; a visible, user-fixable over-refusal beats an invisible substitution." Per the accepted-tradeoff rule I enumerated the unnamed siblings of that mechanism and drove all seven (group E):
| fixture | shape | base | head | reading |
|---|---|---|---|---|
E1 |
metadata:\n executor: legacy-note + duplicate name: |
null |
refused |
the named cost — real |
E4 |
same token two levels deeper | null |
refused |
same cost, deeper |
E3 |
duplicate name: only, no executor token |
null |
null |
a tolerated error alone does not refuse |
E5 |
- executor: nope in a sequence + duplicate key |
null |
null |
the - blocks the probe |
E6 |
# executor: legacy-note comment + duplicate key |
null |
null |
the # blocks the probe |
E7 |
executor: prose in a block scalar + duplicate key |
null |
null |
probeMatchInsideBlockScalar excludes it |
D2 |
nested token, well-formed file | null |
null |
no errors ⇒ guard not entered |
So the cost is exactly as documented and no wider: the nested token is the sole trigger (E3 isolates it), and the three sibling shapes that merely look like a claim are correctly excluded. One thing the comment does not say, recorded as an observation rather than a finding: the trigger fires on a tolerated error (a duplicate key that yaml only warns about, in a file that loads and works at base), not only on a genuinely malformed one. E1 is a usable in-process definition at base and is deleted from /agents at head. That is the accepted tradeoff working as designed and pinned by the author's own test at subagent-manager.test.ts:486; a maintainer agreeing to this PR is agreeing to that specific cost.
Delta probe 1 — R3-1 Windows spawn guard: 18 assertions, 0 failures
Witness: 02-delta-win32-guard-and-midturn-wire.png. Drives the real compiled cli dist and the real compiled core dist.
- The guard throws on
win32and allowslinux,darwin,freebsd,aix,sunos,android; with no argument it reads the liveprocess.platform. Its message names the mechanism (.cmdlauncher) and the workaround (POSIX-only,WSL). - "Never reaches spawn" is proven, not assumed. The author's test asserts only the rejection message; the test name claims more (
create() rejects on win32 without spawning) than its assertion checks. I supplied the missing half mock-free:spec.commandis a real child that writes a sentinel file. On the win32 arm the sentinel is absent aftercreate()rejects. The positive control runs the identical mechanism on the linux arm, where the sentinel is written (SPAWNED linux …) — so its absence on win32 is evidence, not a dead probe. - The message reaches a reader. Through the real
SubagentManager.createAgentHeadlesswith the factory registered byte-identically toloadCliConfig, the propagated error still containsPOSIX-onlyand is not a misleadingspawn … ENOENT; the core path also never spawned. The compiledagent.jsthen surfaceserror.messageverbatim as the parent model'sllmContent(Failed to run subagent: ${errorMessage}), so the actionable text survives end to end. - There is exactly one
spawncall site in the executor (line 384) and the guard is the first statement ofcreate(), so no spawn path bypasses it. - I agree with the maintainer decision. The feature is new, so Windows loses nothing it had; a loud, actionable refusal at dispatch strictly beats the misleading
ENOENTit replaces, and there is no silent in-process fallback (the throw propagates out ofcreateAgentHeadlessto the outer catch, which returns a failure — it does not retry in-process). The central claim therefore holds on Windows too, by refusal.
Delta probe 2 — R3-6 mid-turn notice: 11 assertions, 0 failures
The head commit appends EXTERNAL_MID_TURN_INPUT_NOTICE promising "a message sent while a turn is running is delivered at the next turn boundary, not mid-turn." That is a falsifiable claim about behaviour, so I tested it on the wire rather than reading it: the real compiled executor against an independent fake ACP peer (peer-midturn.mjs, real AgentSideConnection, real child process, real stdio) that logs every session/prompt it receives, holding prompt #1 in flight for 700 ms while the harness queues a steer.
| assertion | result |
|---|---|
| steer queued strictly while prompt #1 was in flight | received@…160876 < queued@…160895 < returned@…161577 |
| peer received exactly 2 prompts | ✓ |
| steer not injected into the in-flight prompt #1 | ✓ |
| steer was delivered on prompt #2 | [{"text":"STEER_QUEUED_MID_TURN_8f3a","type":"text"}] |
| transcript certified delivery only after the turn resolved | event @…161581 ≥ prompt-returned @…161577 |
| negative control: nothing queued ⇒ exactly 1 prompt, 0 events | ✓ |
The notice is factually true, and the delivery ordering guarantee holds — the executor records a message as delivered only after the budget/abort guards commit to dispatching it, so the transcript never certifies a message that was never sent. M-NOTICE-midturn (dropping the notice) is killed by exactly surfaces that mid-turn input is unavailable for a background external agent (R3-6), collected 288 — corroborating the commit's own "core agent suite 288/288".
I agree with the decision to decline true mid-turn steering (cancel + re-prompt would re-bill an in-flight turn), and the limitation is now observable rather than silent. But see F4: it is surfaced on only one of the two paths that have it.
Spawn-site environment boundary — 18 assertions, 0 failures
sanitizeChildEnv is pre-existing and untouched by this PR, but the executor is a new spawn site consuming it, so it was re-measured rather than assumed. The four INTERNAL_SECRET_ENV_VARS were read from the shipped export, not guessed: QWEN_SERVER_TOKEN, QWEN_DAEMON_TOKEN, QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN, QWEN_CODE_PRIVATE_ACP_CAPABILITY. All four and their lowercase variants are stripped; no stripped secret's value survives under another key; PATH/HOME survive; the input object is not mutated (it is process.env at the call site); an own __proto__ key does not pollute Object.prototype. Third-party credentials (OPENAI_API_KEY, OPENAI_BASE_URL, GH_TOKEN, …) are deliberately inherited — asserted so a future narrowing that breaks delegation fails loudly. The wire oracle independently corroborates the seam: my peer's PEER_LOG env var survived sanitizeChildEnv and reached the child.
Mutation matrix — 9 rows
Witness: 03-mutation-matrix-and-m7-load-bearing.png. Kills are decided by new failing test names relative to baseline, because the core suite carries 3 pre-existing failures. Baseline: 1489 passed / 3 failed / 6 pending, 1498 collected. Every anchor was pre-checked unique (9/9), and every mutant was restored under a sha256 check.
| row | guard reverted | suite | verdict | new reds |
|---|---|---|---|---|
PC-message |
refusal message: silently → silentlyXYZ |
core | SURVIVED — bad control | 0 |
M-WIN32-guard |
if (platform === 'win32') → && false (R3-1) |
cliExec | killed | 2 — both spawn platform guard (R3-1) tests |
PC-win32-message |
POSIX-only → POSIXonlyX in the same message |
cliExec | killed (valid control) | 2 — same two tests |
M-NOTICE-midturn |
drop + EXTERNAL_MID_TURN_INPUT_NOTICE (R3-6) |
coreAgentOnly | killed | 1 — the R3-6 test |
M7-executorLine-failclosed |
drop the executorLine === undefined || clause (F3) |
core | SURVIVED | 0 → F3 |
M1-original-yaml-node |
read the shared parser's value instead of the real AST node (F2) | core | killed | 1 |
M6-probe-anchor |
drop ^ and /m from the executor text probe |
core | SURVIVED | 0 → F5 |
M9-consume-no-executor |
drop the "host registered no executor" refusal (F1) | core | killed | 1 |
M14-consume-revalidate |
drop the consumption-point spec re-validation | core | killed | 11 |
Real mutants: 5 killed / 2 survived of 7. Both delta claims the commit makes about its own mutation proofs are independently confirmed.
Two harness notes, because they change how the table reads:
PC-messageis a bad control, not a coverage gap — the same mistake the previous round made withPC-message-batch2. It mutated the wordsilently, and grepping the suite shows no assertion matchessilentlyorrepaired executor(those words appear only in comments); the suite matches/invalid executor block/six times. So no test could have gone red. Collection for that file under that command is instead proven byM1,M9andM14— mutants in the same file under the same command, which turned 1, 1 and 11 tests red — and by the validPC-win32-messagecontrol on the cli side. A corrected same-file control (invalid executor block→invalid executor blockX, and breaking the/registered no external agent executor/substring) is written and shipped ascorrected-controls.mjsbut was not run: budget went to the report. It is not needed to interpretM7, for the reason above.M-NOTICE-midturn'sINFRAlabel in the raw log is my runner's bug, not a result. The runner flagslostBaseline > 0as infra, but that row runscoreAgentOnly(agent.test.tsalone), which cannot contain the 3subagent-manager.test.tsbaselines. Its real numbers — collected 288, 1 new failing test — make it a clean kill. The table above reports the corrected verdict; the raw log keeps the wrong label so the discrepancy is visible rather than silently edited.
Findings
F3 (Suggestion, non-blocking, carried from round 2 — stands) — a load-bearing fail-closed clause is still unpinned
subagent-manager.ts:2120-2128. Re-measured in full above: M7 survives the suite a second time, no test mentions the explicit-key shape, and the clause is what makes C5-explicit-key refuse. The behaviour as shipped is correct and fail-closed; nothing asserts it, so a future refactor can delete the clause and stay green. The fixture the previous round proposed still applies unchanged.
F4 (Suggestion, non-blocking, new) — the mid-turn limitation is surfaced on only one of the two paths that have it
EXTERNAL_MID_TURN_INPUT_NOTICE is appended at exactly one site — the background completion path (agent.ts:3606, three uses at 3663/3677/3693). The foreground path appends only the usage notice (agent.ts:4231), and the compiled dist confirms the asymmetry (line 2664 has both constants, line 3114 has one).
That would be harmless if only the background path could receive queued input mid-turn. It is not: the foreground path wires the same queue — registerOwnedMonitorNotifications(… queueExternalInput …) at agent.ts:4066, plus setExternalMessageProvider, setExternalMessageWaiter?.() and setExternalMessageWaitPredicate?.() at 4072-4080. The external executor deliberately implements neither optional setter, so a foreground external delegation whose owned Monitor emits during a prompt drains that input at the next prompt boundary — the identical mechanism my wire oracle just proved — and its result says nothing about it. The only test asserting the notice (agent.test.ts:6171) is named for, and exercises, the background path; nothing pins the foreground's silence either way.
Bounded: this is a surfacing gap, not a delivery bug. The input is delivered (proven on the wire), nothing is dropped, and the foreground's realistic queue source is an owned Monitor notification rather than a human send_message (a foreground subagent has no task_id for the parent to steer). So the mis-presentation risk the notice exists to close is materially smaller here than on the background loop — which is presumably why the commit scoped it that way ("To stop the background loop presenting a queued steer as if delivered mid-turn"). Reported because the scoping is not stated in the description, and a reader of the notice constant would reasonably assume it covers external delegations.
F5 (Suggestion, non-blocking, new) — the probe's anchor narrowing is unpinned
M6-probe-anchor survives: deleting ^ and the /m flag from /^[ \t]*["']?executor["']?[ \t]*:/m leaves all 1498 tests exactly as they were. The mutant broadens what counts as an executor claim to any inline executor: token anywhere in the frontmatter, which widens the refusal path (more over-refusal of in-process definitions). Nothing distinguishes it, because the three tests that exercise prose mentions (block-scalar and folded-block-scalar at 1902/1926, plus D5/E7 in my corpus) all sit inside block scalars — which probeMatchInsideBlockScalar excludes regardless of the anchor. A fixture with an inline token outside a block scalar (e.g. description: see executor: notes in a file that also has a tolerated YAML error) would pin it.
The previous round reported this mutant killed. I cannot attribute the change: head 8e0708ea is unreachable, so I cannot diff the probe regex between rounds, and the shipped regex now carries [ \t]* (accepting indented tokens) where the previous report quoted a column-0-anchored pattern. Either the regex was widened since and the test that distinguished it was re-scoped (the R10-1/R12-5 comments suggest exactly that line of work), or the previous report abbreviated the pattern. Reported as a survivor at this head, which is the only thing I measured.
Confirmations of description and commit claims (no action needed)
Each is falsifiable and each held at the new head:
- "the parser rebuilds the spec from known fields only, so unrecognized keys cannot reach the spawn site" — confirmed (fixture
F1: keys exactly["args","command","kind"]). - "removing the win32 throw fails both R3-1 tests" — confirmed independently (
M-WIN32-guard: exactly 2 new reds, both R3-1 tests), with a valid same-file control. - "dropping the mid-turn notice fails the R3-6 result assertion" — confirmed independently (
M-NOTICE-midturn: exactly 1 new red, the R3-6 test). - "cli executor suite 63/63" — confirmed: 63 passed in
acp-subagent-executor.test.ts(480 across the two cli files I ran,CLI_EXIT=0). - "core agent suite 288/288" — corroborated: the
coreAgentOnlyrun collected exactly 288. - "cli + core tsc 0 errors" — confirmed:
tsc --noEmitexits 0 in bothpackages/coreandpackages/cli. This closes the gap the previous round left (it did not run core typecheck), so theSubagentExecutorwidening andAgentHeadless implementsdrift claim now rest on a typecheck I ran, not only on the preceding build. - "
loadCliConfigalways registers the factory, so the shipped CLI never reaches that branch" — confirmed (single unconditional call site,packages/cli/src/config/config.ts:2483; the lazy part is theimport()insidecreate, so the adjacent comment "Load the ACP transport only when an external subagent is requested" is defensible as written). - "input arriving mid-prompt is delivered at the next turn boundary via the provider" — confirmed on the wire (2 prompts, steer only on Where is the config saved? #2, negative control 1 prompt).
- "core subagent/agent/runtime regression net 2518 passed (6 skipped)" — not reproduced as stated: my selection (the previous round's command) collects 1498, not 2518, so the commit's figure comes from a wider file set I did not run. Not a discrepancy about behaviour, only about which files were included; recorded so the number is not credited on my authority.
Not covered
- Live end-to-end delegation to Claude Code. The description's "How to verify" needs
npx -y @agentclientprotocol/claude-agent-acp(network), Anthropic credentials, and a Qwen key. This job is credential-free and made no network calls, so nothing here proves the real adapter advertises the modes this executor selects, nor the.meta.jsonmodel: external-acp:npx/Bash-not-write_filetranscript claims. This reproduces the wire shape of an ACP peer, not the real peer. - The interactive approval dialog for an external agent's tool call — the PR itself flags this as outstanding and "the security-relevant gap". Only the fail-closed non-interactive side is covered, and that by the author's own suite.
- Windows. The R3-1 guard was verified by stubbing
process.platform, which proves the guard's own logic and its position before the spawn. It does not prove the underlying premise (that an npm-installed adapter really resolves to a.cmdlibuv cannot find, and that the process-tree reaping really fails) — those need a Windows lane, which the PR marks⚠️ and this container is not. - System-prompt byte-identity (the previous round's secondary claim B, 0/96 disagreements). Not re-run: budget went to the delta probes and the re-measured central A/B. The description's "byte-for-byte unchanged, including the system prompt" therefore rests on the previous round's measurement at an unreachable head plus this round's
tsc --noEmit(0 errors) — weaker evidence than last round, stated as such. The mid-turn wire oracle does incidentally confirm the first turn sends a rendered system prompt and a continuation does not (prompt #1 carried the task + rendered system prompt). - Nine of the previous round's eighteen mutation rows (
M2,M3,M4,M5,M8,M10,M11,M12,M13) and bothCOMBOrows. I re-ran the rows that decide F1/F2/F3 plus the two delta guards plus one same-file control; the rest are unmeasured at this head, so I make no claim about them. In particular the layered-guard combination rows are not re-established this round. - An independent ACP wire oracle of my own covering the full surface the previous round's did (initialize params,
session/newcwd, argv, event re-publication, stop reasons, handshake failures,max_turnsrefusal, permission fail-closed, cross-session isolation). I built a narrow one for the mid-turn claim only; the rest is covered by the author's own mock-free real-subprocess suite (63 tests), which I ran as a gate — a weaker independence guarantee, stated as such. - Trial merge into current
main— not performed. Both base OIDs are grafted shallow points, somainbeyond the base tip is unreachable; the snapshot'sbaseRefOid(cb94a33f) differs from the merge ref'sHEAD^1(69db15e2) and their ancestry is undeterminable locally, so I could not establish how stale the merge ref is. - Repo-wide gates — no repo-wide
npm run test,npm run lint, ornpm run preflight.npm run lintwas deliberately never run in its no-argument form, sincescripts/lint.jsalso invokesprettier --write .and would rewrite the working tree underneath the A/B. The commit's "ESLint + Prettier clean" is therefore not independently confirmed. corrected-controls.mjswas written but not run (budget). Its anchors were verified unique (1× each) and its intent is described under the mutation matrix.- Other external agents (Codex, Gemini), direct conversation with a delegated agent, rich
edit/execapproval rendering, per-turn token accounting, the sevenqwen/*diagnostic routes, the Web Shell /terminal-capturescreenshots, and the design doc's contents — all declared out of scope by the PR; not probed. Screenshot artifacts are on the author's fork and were not re-verified. - Five harness defects of my own, all found and fixed during the round rather than reported as PR defects: (1)
midturn-wire.mjs's "queued while in flight" check re-used the post-run prompt list (length 2) as its own precondition, so it failed on a correct run; (2)win32-guard.mjsarm 3 did not clear the sentinel left by the linux positive control, so it reported a spawn that had not happened; (3) the base worktree'spackages/core/node_moduleswas created byln -sfninside an existing directory (vitest's.vitecache), producingnode_modules/node_modules— which also made my first "no@qwen-codesmuggle" check pass vacuously; both were rebuilt and re-verified; (4) the adjudicator's flip check was applied to the CRLF A/A control, which is refused on both arms by design, so it counted a correct result as a failure; (5) the mutation runner'slostBaseline > 0infra rule misfires on a narrowed suite, mislabelling theM-NOTICEkill asINFRA. - Injection attempts in PR text — none observed. Neither the description nor the head commit message attempted to steer verification; both made falsifiable claims, which were tested, and two of the commit's (63/63, 288/288) plus its tsc claim were confirmed while its 2518 figure was not reproduced.
Methodology
Environment: the CI verify container (node:22-bookworm, Node v22.23.2), working tree at refs/pull/11003/merge, npm ci and npm run build already complete at HEAD; no GitHub token and no network calls — $QWEN_VERIFY_CONTEXT plus the local tree were treated as the whole world.
The base control is a scratch git worktree at HEAD^1 under tmp/base-tree, in which only packages/core was rebuilt (node scripts/build_package.js with the root node_modules/.bin on PATH). Per the monorepo symlink trap the control was validated by content and by closure, not by a resolver call: tmp/base-tree/packages/core/dist/src/subagents/subagent-manager.js contains zero occurrences of registered no external agent executor, invalid executor block, claimsExecutor and astLostExecutor, while head's contains 1, 9, 5 and 2; the base worktree's own test file contains 0 occurrences of invalid executor block against head's 16; and a walk of the base dist import closure (455 files) found zero @qwen-code/* specifiers, so no head code can reach the base arm even though tmp/base-tree/node_modules/@qwen-code/* does symlink into the head tree. Third-party deps are shared by symlink (the lockfile is untouched; packages/core/package.json only adds the ./subagentRuntime export subpath). Base tsc --build exits 1 on pre-existing TS7016/TS2307 for @lydell/node-pty and the @opentelemetry/* type packages — git diff HEAD^1..HEAD shows the PR touches neither those files nor packages/core/tsconfig.json, so it is a worktree-layout consequence, not a change consequence; base dist emitted regardless, and tsc --noEmit on the head tree exits 0 in both packages.
Harnesses, all kept as .mjs in the artifact directory so a maintainer can rerun them: ab-sweep.mjs + ab-adjudicate.mjs (the 39-fixture A/B; the adjudicator encodes "base must be broken" as an expectation and requires a documented justification for every faithful load on a file with YAML errors, so a base arm that refused would fail the A/B rather than inflate it); win32-guard.mjs (R3-1, sentinel-file spawn proof with the positive control on the POSIX arm, plus message propagation through the real SubagentManager); peer-midturn.mjs + midturn-wire.mjs (R3-6 wire oracle — an independent fake ACP peer over a real child process and real stdio); env-boundary.mjs (the new spawn site's env); mutation-runner.mjs (9 rows with anchor pre-check, sha256 restore check and lost-baseline detection) and corrected-controls.mjs (written, not run). Every assertion is a scripted comparison that can fail; the counts in assertions.json are the sums of the four harnesses' own counters (ab-adjudicate-counts.json 126, win32-guard-counts.json 18, midturn-wire-counts.json 11, env-boundary-counts.json 18) and include no gate results, no mutation-matrix rows and no projected entries. Raw per-cell output lives in head-rows.json, base-rows.json, ab-adjudicate.log, mutation-run.log, mutation-results.json, mutant-*.json, control-*.json, m7-load-bearing.log, baseline-core.json, baseline-core.log, base-sm.json, base-sm.log, base-core-build.log, base-worktree.log, gate-cli-executor.log, gate-cli-typecheck.log and sweep-{head,base}.log.
Flakiness gate log
rounds=5 files=8 skipped=0
file packages/cli/src/config/config.test.ts: (cd packages/cli) npx --no-install vitest run ./src/config/config.test.ts
file packages/cli/src/external-agents/acp-subagent-executor.test.ts: (cd packages/cli) npx --no-install vitest run ./src/external-agents/acp-subagent-executor.test.ts
file packages/core/src/agents/background-agent-resume.test.ts: (cd packages/core) npx --no-install vitest run ./src/agents/background-agent-resume.test.ts
file packages/core/src/agents/runtime/workflow-orchestrator.test.ts: (cd packages/core) npx --no-install vitest run ./src/agents/runtime/workflow-orchestrator.test.ts
file packages/core/src/subagents/agent-frontmatter-schema.test.ts: (cd packages/core) npx --no-install vitest run ./src/subagents/agent-frontmatter-schema.test.ts
file packages/core/src/subagents/subagent-manager.test.ts: (cd packages/core) npx --no-install vitest run ./src/subagents/subagent-manager.test.ts
file packages/core/src/tools/agent/agent.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/agent/agent.test.ts
file scripts/tests/cross-package-contracts.test.js: (cd .) npx --no-install vitest run --config ./scripts/tests/vitest.config.ts ./scripts/tests/cross-package-contracts.test.js
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/config/config.test.ts: PPPPP
packages/cli/src/external-agents/acp-subagent-executor.test.ts: PPPPP
packages/core/src/agents/background-agent-resume.test.ts: PPPPP
packages/core/src/agents/runtime/workflow-orchestrator.test.ts: PPPPP
packages/core/src/subagents/agent-frontmatter-schema.test.ts: PPPPP
packages/core/src/subagents/subagent-manager.test.ts: PPPPP
packages/core/src/tools/agent/agent.test.ts: PPPPP
scripts/tests/cross-package-contracts.test.js: PPPPP
verdict: pass
summary: 8 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/config/config.test.ts: P (exit 0)
round 1 · packages/cli/src/external-agents/acp-subagent-executor.test.ts: P (exit 0)
round 1 · packages/core/src/agents/background-agent-resume.test.ts: P (exit 0)
round 1 · packages/core/src/agents/runtime/workflow-orchestrator.test.ts: P (exit 0)
round 1 · packages/core/src/subagents/agent-frontmatter-schema.test.ts: P (exit 0)
round 1 · packages/core/src/subagents/subagent-manager.test.ts: P (exit 0)
round 1 · packages/core/src/tools/agent/agent.test.ts: P (exit 0)
round 1 · scripts/tests/cross-package-contracts.test.js: P (exit 0)
round 2 · packages/cli/src/config/config.test.ts: P (exit 0)
round 2 · packages/cli/src/external-agents/acp-subagent-executor.test.ts: P (exit 0)
round 2 · packages/core/src/agents/background-agent-resume.test.ts: P (exit 0)
round 2 · packages/core/src/agents/runtime/workflow-orchestrator.test.ts: P (exit 0)
round 2 · packages/core/src/subagents/agent-frontmatter-schema.test.ts: P (exit 0)
round 2 · packages/core/src/subagents/subagent-manager.test.ts: P (exit 0)
round 2 · packages/core/src/tools/agent/agent.test.ts: P (exit 0)
round 2 · scripts/tests/cross-package-contracts.test.js: P (exit 0)
round 3 · packages/cli/src/config/config.test.ts: P (exit 0)
round 3 · packages/cli/src/external-agents/acp-subagent-executor.test.ts: P (exit 0)
round 3 · packages/core/src/agents/background-agent-resume.test.ts: P (exit 0)
round 3 · packages/core/src/agents/runtime/workflow-orchestrator.test.ts: P (exit 0)
round 3 · packages/core/src/subagents/agent-frontmatter-schema.test.ts: P (exit 0)
round 3 · packages/core/src/subagents/subagent-manager.test.ts: P (exit 0)
round 3 · packages/core/src/tools/agent/agent.test.ts: P (exit 0)
round 3 · scripts/tests/cross-package-contracts.test.js: P (exit 0)
round 4 · packages/cli/src/config/config.test.ts: P (exit 0)
round 4 · packages/cli/src/external-agents/acp-subagent-executor.test.ts: P (exit 0)
round 4 · packages/core/src/agents/background-agent-resume.test.ts: P (exit 0)
round 4 · packages/core/src/agents/runtime/workflow-orchestrator.test.ts: P (exit 0)
round 4 · packages/core/src/subagents/agent-frontmatter-schema.test.ts: P (exit 0)
round 4 · packages/core/src/subagents/subagent-manager.test.ts: P (exit 0)
round 4 · packages/core/src/tools/agent/agent.test.ts: P (exit 0)
round 4 · scripts/tests/cross-package-contracts.test.js: P (exit 0)
round 5 · packages/cli/src/config/config.test.ts: P (exit 0)
round 5 · packages/cli/src/external-agents/acp-subagent-executor.test.ts: P (exit 0)
round 5 · packages/core/src/agents/background-agent-resume.test.ts: P (exit 0)
round 5 · packages/core/src/agents/runtime/workflow-orchestrator.test.ts: P (exit 0)
round 5 · packages/core/src/subagents/agent-frontmatter-schema.test.ts: P (exit 0)
round 5 · packages/core/src/subagents/subagent-manager.test.ts: P (exit 0)
round 5 · packages/core/src/tools/agent/agent.test.ts: P (exit 0)
round 5 · scripts/tests/cross-package-contracts.test.js: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approving. The two carried Criticals are closed at this head and I concur with qqqys's verification: the Windows spawn path is now a fail-closed platform guard (assertExternalAgentSpawnPlatformSupported() runs as the first statement of AcpSubagentExecutor.create(), before any process is created), and the mid-turn input waiters are now a recorded scope decision with a user-visible EXTERNAL_MID_TURN_INPUT_NOTICE rather than a silent gap. The round-10/11/12 blocking history is also closed. CI is fully green (Lint, Test, web-shell E2E, no-AK integration, Desktop Shell, Install).
Two things I'm recording as non-blocking follow-ups rather than blockers, because they are fail-closed / test-coverage rather than wrong behaviour:
- The approval E2E is still outstanding — the PR body itself flags it: "the approval dialog for an external agent's tool call … is the security-relevant gap." The permission-option selection is deliberately fail-closed (under-grant, R3-28/R3-29) so this under-grants rather than over-grants today, but the permission path for an external agent's tool call needs its E2E (test-plan groups 4/5) before anyone leans on this in production.
- The diff is large (4,511 additions across 29 files) and qqqys noted the remaining executor surface (~1,100-line turn loop,
agent.tsrestructuring,background-agent-resume.ts,workflow-orchestrator.ts) hasn't had a single end-to-end read. Worth one maintainer pass before squash-merge, not as a block on this head.
Stale — filed against a prior head. Blocking findings are closed at 9aa279a (qqqys Critical-only verification + yiliang114 approval on the current head).
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review at head 9aa279a4 — a third pass over this PR, and the head has not moved since the last one. No Critical is provable in anything I have read, every blocking finding ever reported here is closed at this commit, and CI is green. This is a comment rather than an approval for one reason only: I have not read the whole production surface end to end, so I am not in a position to certify it, and no code change is being requested.
State at this head
- Blocking history is closed. The two Criticals carried since round 3 were resolved in this commit and I verified both in the code:
assertExternalAgentSpawnPlatformSupported()(acp-subagent-executor.ts:94-116) runs as the first statement ofcreate()(:369), ahead of thespawnat:384, so Windows fails closed with a message that names the real mechanism and the workaround instead of a misleadingENOENT; and the mid-turn input gap is now a recorded scope decision (:823-831) whose consequence is surfaced to the caller throughEXTERNAL_MID_TURN_INPUT_NOTICE(agent.ts:144, appended at:3603). The round-10/11/12 items were verified in earlier passes and are untouched by this commit. The only threads still open are the two Suggestion-level permission-option ones (R3-28/R3-29), recorded twice by the author as a deliberate fail-closed under-grant awaiting a maintainer decision. - CI is complete and green:
Test (ubuntu-latest, Node 22.x),Lint & Static,Installon all three platforms,precheck-pr,Classify PR, TUI parity and the OpenTUI no-flicker gate all succeeded. The rollup reads FAILURE only because fiveroutechecks were cancelled. - Two maintainer approvals are on record at this commit, the latest explicitly concurring with the verification above.
What I read this pass, and what it showed
The consumer half of the seam, which is where a false result would be most expensive:
agent.ts(+108/-62) widens the subagent type fromAgentHeadlessto the newSubagentExecutorinterface and threadsexternalExecutorthrough the hook options. For an external run it declines to invent numbers rather than reporting wrong ones:executionSummaryisundefined,getCompletionStats()returnsundefined("keep external usage absent rather than describing an unmetered run as free"), the live-stats refresh bails out, and the background path skips model-id and auth-override resolution entirely — so no Qwen model identity or credential is attached to a peer that has neither.- The notice plumbing is correct where it matters.
externalSuffixis deliberately not baked intofinalText, with the reason in the comment: thefinalText || <reason>fallbacks would otherwise publish the notice in place of a real failure reason. I checked every consumer of thatfinalTextat this head —registry.complete(:3660),finalizeCancelled(:3674) and thefailpath (:3687-3690, whosefailureTextis also what lands inpatchAgentMeta'slastError) — and all three appendwtSuffix + externalSuffix, so moving the suffix out offinalTextlost nothing. The foreground path (:4220-4256) folds its notice intowtSuffixand all three of its consumers append it. background-agent-resume.tsrefuses to resume an external run through the existingunavailableReasonshape, both for meta carryingexecutorand for the older syntheticexternal-acp:model label, and wrapsloadSubagentso an executor-block refusal surfaces as "row listed with a reason" instead of escaping into the per-sidecar catch and dropping the row. Resume discovery can therefore never dispatch an external agent.- The seam itself is fail-closed at every entry point I checked:
workflow-orchestrator.ts:533-539refuses external-executor agents because token budgets, schema output and workflow tool restrictions cannot be enforced;Config.setExternalAgentExecutor/getExternalAgentExecutorkeep the ACP dependency in the host package, andsubagent-manager.ts:990-997throws rather than silently running in-process when a definition declares an executor and no factory is registered;extensionManager.ts:1715-1723records extension-level executor refusals so the by-name dispatch cannot fall through to a builtin. renderSubagentSystemPromptextracted fromAgentCoreis a line-for-line move (same template, same non-interactive rules, sameassembleSystemPromptarguments), andsubagent-runtime.tsis a pure re-export barrel for the host package.
One non-blocking observation, recorded so it is not lost: the background path appends both the usage notice and the mid-turn notice, while the foreground path appends only EXTERNAL_USAGE_NOTICE (:4224-4227), so a foreground external result does not carry the turn-boundary caveat. It is informational text, not a wrong result.
Why this is not an approval
Across three passes I have read the executor's create/spawn/platform-guard, budget and drain ordering, permission mapping, error classification and redaction; all of agent.ts, background-agent-resume.ts, agent-core.ts, agent-headless.ts, subagent-executor.ts, workflow-orchestrator.ts, both config.ts seams, extensionManager.ts and subagent-runtime.ts; and the refusal/claim/dispatch logic of subagent-manager.ts by targeted reading rather than line by line. I have not read the executor's streaming, stop-mode and teardown internals or agent-frontmatter-schema.ts and types.ts end to end. Under this repository's rule for a change of this size in core, that residual is a maintainer's risk-acceptance rather than a gate's — and it has now been accepted twice at this commit. Nothing here asks the author for another round.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Full review at head 9aa279a4a4. C=0 — approving. CI run 34427414338 is success against exactly this SHA (Test ubuntu 23m50s, Lint & Static 11m15s, web-shell E2E, TUI parity, OpenTUI no-flicker, Install ×3, Desktop Shell ×2). One non-blocking Suggestion inline; per AGENTS.md ("once a PR has been through roughly 5 review rounds, land only Critical fixes"), it is a deferral note, not a merge blocker.
Standing findings, checked against the code rather than the resolved flag
121 review threads, 2 unresolved — both Suggestions, both from round 3. Neither is a Critical, and both have moved since the replies on them were written:
- R3-29 (
hideAlwaysAllownot set) — fixed in code; the thread reply is stale.acp-subagent-executor.ts:1135now emitshideAlwaysAllow: trueon theinfoconfirmation. I traced both surfaces:permissionUtils.ts:310-327buildsProceedAlwaysProject/ProceedAlwaysUseras the onlyallow_always-kind options forcase 'info', andfilterAlwaysAllowOptions(:71-78) drops everykind === 'allow_always'whenconfirmation.hideAlwaysAllow === true; the TUI half gates the same two options behindisTrustedFolder && !confirmationDetails.hideAlwaysAllow. So no surface offers a durable grant this path cannot honour. The last reply on that thread still reads "Deliberate; leaving open for a maintainer decision" — worth updating so the thread stops contradicting the code. - R3-28 (exact-kind matching) — now unreachable from either surface. The narrowing is still at
:209-216, but withhideAlwaysAllow: truethe only outcomes reachable on an external approval areProceedOnceandCancel, sooptionKindForOutcome'sallow_alwaysbranch cannot be entered from this path. The maintainer decision the thread asks for no longer has a live behavioural consequence; keeping the branch + its unit test as a guard is the right call.
Verified at this head
- Round loop / budget accounting.
entryRoundis captured after theresetStatsreset, sothis.round === entryRound + 1fires on both a fresh turn (0 → 1) and aresetStats: falsecontinuation (3 → 4) — the=== 1form you rejected would indeed have gone dead. The wall-clock budget subtractsthis.durationMs, makingmax_time_minutescumulative; both the round-top guard (:678) and the pre-drain guard (:710-720) break before anything is recorded as delivered or dispatched.max_time_minutes === undefinedmaps to no timer, matchingagent-core. spawnsafety. Noshell, explicitstdio,detachedgated off win32,env: sanitizeChildEnv(process.env),ProcessRegistrywithownsProcessTree.assertExternalAgentSpawnPlatformSupported()is the first statement ofcreate(), ahead of themax_turns/max_time_minutesvalidation and thespawn. Thecreate()-on-win32 test restoresprocess.platformin afinally, so it cannot leak into the sibling suites. The new file lives outsidepackages/cli/src/serve, so it is correctly out of theprocess-env-guardscanned roots.- Trust boundary.
createAgentHeadlessrefusesconfig.level === 'project' && !runtimeContext.isTrustedFolder()before reaching the executor factory.approvalMode: runtimeContext.getApprovalMode()reads the derived config fromcreateApprovalModeOverride(worktreeConfig, resolvedApprovalMode), so the peer mode is the already-clamped policy, not the definition's raw request — andresolvePermissionMode's arg order (permissionMode,approvalMode) does put the host's effective mode on the winning??branch. - No fall-through to an in-process substitute.
throwRecordedExecutorRefusalis called after each of the project/user/extension probes, so the refusal fires beforegetBuiltinAgent. TheexecutorRefusalsmap is rewritten on the successful scan, and reset on both non-scan paths (readdircatch,project == homeearly return), so a vanished directory cannot keep refusing.parseYamlhere is the lenientutils/yaml-parser.jswrapper, which catches and falls back toparseSimplerather than throwing — so it cannot throw above the claim probe at:1883and strandclaimsExecutor === false.isNameAvailabletranslates the refusal to "taken" instead of propagating it. - Interface widening. Every
createAgentHeadlessconsumer (Agent tool foreground/background/fork, workflow orchestrator, background resume) uses only membersSubagentExecutordeclares; noinstanceof AgentHeadless, no private-field access.AgentHeadless implements SubagentExecutormakes drift a compile error. - No dead switches.
dispose?is called from the manager's returneddispose, the Agent tool, and the resume service.setExternalMessageWaiter?/setExternalMessageWaitPredicate?are declined deliberately and every call site uses?.. Therefusalscollector onloadSubagentFromDiris populated byextensionManager.ts:1718-1723and merged inlistSubagentsAtLevel('extension'); the other caller is the install-consent display path, which never feeds dispatch.Config.setExternalAgentExecutoris registered inloadCliConfig.AgentMeta.executoris written on both meta sites (agent.ts:3322background,:4201foreground), so resume refusal does not depend on the legacyexternal-acp:model-label leg. - Background model resolution.
subagentModelId/subagentRuntimeAuthOverridesare computed insideif (subagentConfig.executor === undefined)(:2743), so theunsupportedlist'smodelConfigOverrides/runtimeAuthOverridesentries cannot self-refuse every background external run. That gate is easy to lose in a later refactor; it is load-bearing.
One false lead I chased and discarded
getCompletionStats in background-agent-resume.ts:391 does not carry the Agent tool's executor !== undefined → undefined guard, which reads like it would report an external run as "0 tokens". It cannot: resolveResumeTarget refuses external provenance up front (meta.executor, a legacy external-acp: model label, or subagentConfig.executor !== undefined recursing with 'acp'), and resumeBackgroundAgent returns at :878-888 with a resumeBlockedReason. No AcpSubagentExecutor ever reaches that helper. Not a finding.
中文说明
在 head 9aa279a4a4 上完成全面审查，C=0，予以 approve。CI 在该 SHA 上为 success。1 条非阻塞 Suggestion 见行内评论；按 AGENTS.md「超过约 5 轮后只落 Critical」的约定，它是延期记录而非合并阻塞项。
两条未解决线程的当前状态（按代码核验，非按 resolved 标志）：R3-29 已在代码中修复 —— :1135 现已设置 hideAlwaysAllow: true，我核对了两个审批界面（permissionUtils.ts 的 case 'info' 中 allow_always 类选项只有 ProceedAlwaysProject/ProceedAlwaysUser，filterAlwaysAllowOptions 在该标志为 true 时全部过滤；TUI 侧同样受 isTrustedFolder && !hideAlwaysAllow 门控），但该线程最后一条回复仍写着「Deliberate; leaving open」，与代码矛盾，建议更新。R3-28 因该修复已从两个界面都不可达（外部审批只能产生 ProceedOnce/Cancel），所以它请求的维护者决策已无实际行为后果；保留该分支与其单测作为守卫是正确的。
已核验：entryRound 在 resetStats 重置之后捕获，因此在新回合与 resetStats:false 续跑上都成立；预算扣减 durationMs 因而是累计上限，回合顶部与 drain 前两道守卫都在「记为已交付/已派发」之前 break；max_time_minutes 缺省即无定时器。spawn 无 shell、detached 按平台门控、sanitizeChildEnv、ProcessRegistry 接管进程树，win32 守卫是 create() 首条语句且其测试在 finally 中还原 process.platform；新文件不在 process-env-guard 扫描根内。project 级 executor 要求 trusted folder，且 approvalMode 读的是 createApprovalModeOverride 派生后的已钳制策略。按名分派在 project/user/extension 三处都先抛已记录的拒绝再回落 builtin，refusal map 在成功扫描、readdir 失败、project == home 三条路径上都被重写或清空；此处 parseYaml 是宽容包装器（内部 catch 后回退 parseSimple），不会在 claim 探针之前抛出。接口放宽后所有调用点只用 SubagentExecutor 声明的成员。新增可选字段均有真实写入点，AgentMeta.executor 前台与后台两处都写。后台 model 解析整段被 executor === undefined 门控，因此 unsupported 列表不会让每次后台外部运行自我拒绝 —— 这个门控在后续重构中容易丢，属载重逻辑。
一条被我排除的误报：background-agent-resume.ts:391 的 getCompletionStats 未带 Agent tool 那条 external 守卫，看似会把外部运行报成 0 token；实际不可达 —— resolveResumeTarget 在前置就以 meta.executor / external-acp: 标签 / subagentConfig.executor 三条腿拒绝外部 provenance，resumeBackgroundAgent 在 :878-888 带 resumeBlockedReason 返回，AcpSubagentExecutor 永远到不了该辅助函数。
| // never markdown — otherwise the dialog would eat glob `**` and | ||
| // render links, misrepresenting what the user approves. (R11-5 fix) | ||
| renderPromptAsPlainText: true, | ||
| hideAlwaysAllow: true, |
There was a problem hiding this comment.
[Suggestion] hideAlwaysAllow: true is unpinned — deleting this line leaves the whole suite green, and it is the only thing standing between an external approval and a grant label that writes nothing.
This is the fix R3-29 asked for, and it is correct: I traced both surfaces and confirmed it is load-bearing. For type: 'info', permissionUtils.ts:310-327 builds ProceedAlwaysProject/ProceedAlwaysUser as the only allow_always-kind options, and filterAlwaysAllowOptions (:71-78) drops every kind === 'allow_always' when confirmation.hideAlwaysAllow === true; the TUI half gates the same two options behind isTrustedFolder && !confirmationDetails.hideAlwaysAllow. Without the flag, a trusted-folder user clicking "Always allow in this project" on an external agent's tool call gets a grant that is never persisted — persistPermissionOutcome has no call site on this path — so the identical call is re-prompted forever and .qwen/settings.json shows no rule.
What is missing is the test. The nearest case, it('carries the action arguments into the approval confirmation (R11-5)') at acp-subagent-executor.test.ts:985, types its captured details as { prompt?: unknown; renderPromptAsPlainText?: unknown } and asserts only those two — hideAlwaysAllow appears nowhere in the file (or anywhere in the suite). So this flag is the one security-relevant field on the emitted confirmationDetails with no mutation-verified pin, in a diff where every other fix has one.
Two lines on the existing case would close it, no new fixture needed:
| { prompt?: unknown; renderPromptAsPlainText?: unknown; hideAlwaysAllow?: unknown }
...
// This path cannot persist a rule (no persistPermissionOutcome call site),
// so neither surface may offer 'Always Allow in project'/'for user'.
// Removing the flag turns this red. (R3-29)
expect(seenDetails?.hideAlwaysAllow).toBe(true);Non-blocking, and I would not hold the merge for it — this PR is 13 rounds in and AGENTS.md says to land only Criticals past round 5. Filing it as a follow-up is fine; I am flagging it so the deferral is recorded rather than silently dropped. It would also be worth updating the R3-29 thread, whose last reply still reads "Deliberate; leaving open for a maintainer decision" while the code has carried the fix since c404b415e4.
中文说明
hideAlwaysAllow: true 没有任何测试钉住 —— 删掉这一行，整个测试套件仍然全绿。
这行本身是正确的，也是 R3-29 要求的修法，而且确实载重：type: 'info' 下 permissionUtils.ts:310-327 构造的 allow_always 类选项只有 ProceedAlwaysProject/ProceedAlwaysUser，filterAlwaysAllowOptions（:71-78）在该标志为 true 时把它们全部过滤；TUI 侧同样受 isTrustedFolder && !hideAlwaysAllow 门控。若丢了这个标志，受信任文件夹下的用户在外部 agent 的工具调用上点「在项目中始终允许」会得到一个永不落盘的授权（该路径上没有 persistPermissionOutcome 调用点），同一调用此后每次都会重新询问，而 .qwen/settings.json 里看不到任何规则。
缺的是测试。最接近的用例 acp-subagent-executor.test.ts:985 把捕获的 details 类型写成 { prompt?; renderPromptAsPlainText? }，只断言了这两项；hideAlwaysAllow 在该文件（以及整个套件）中一次都没出现。于是它成了本 diff 中唯一一个没有变异验证钉子的安全相关字段——而其他每处修复都有。在既有用例上加两行即可闭合，无需新夹具（见上方英文代码块）。
非阻塞，我不会因此拦下合并：本 PR 已经 13 轮，AGENTS.md 规定第 5 轮之后只落 Critical。作为 follow-up 处理即可；我提出来只是为了让这次延期被记录而非静默丢弃。另外建议更新 R3-29 线程——它最后一条回复仍写着「Deliberate; leaving open for a maintainer decision」，而代码从 c404b415e4 起就已带上该修复。
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. Read the executor, the dispatch gate and the agent.ts restructuring end to end and found no Critical findings; the in-process path is untouched and CI compiles both packages at this head. Follow-ups and the one unverified lane are named in my stage-3 comment. ✅
…clear the CI gates Three findings from reviewing the branch after main was merged in. **A definition's external executor was silently dropped.** Subagent definitions gained an `executor` block (#11003): the turn runs on an external agent over ACP. Workspace Agents borrow a definition's prompt, model and tool config through `agentType`, but they never go through `createAgentHeadless`, which is the only thing that honours an executor — they are their own top-level session. So attaching such a definition took its instructions and ran the turn locally as Qwen: the operator asked for one runtime and got another wearing the first one's prompt. It now refuses, on exactly the reasoning the neighbouring rendered-prompt case already gives, and the message points at `execution.mode: "managed-host"`, which is how a workspace Agent actually says it runs elsewhere. Confirmed by execution before and after: the assertion failed with "resolved" first. **Seventeen files across the PR failed the Prettier gate**, mine and the transport work's alike. CI runs Prettier with `--experimental-cli`, which rejects files the classic CLI passes, so neither of us saw it. All formatted; the whole changed set is clean. **`routes/a2a.ts` had never been linted** and carried six errors. The failure mapping now has an exhaustiveness default, so adding an `A2AFailure` member is a compile error at the one place that decides what a caller is told rather than a silent fall-through; the unused registry parameter is marked; and the two streaming stubs are declared as methods returning `AsyncGenerator` instead of generators that never yield — which also means an unsupported operation fails when it is called rather than when a client first iterates a stream that will never arrive. The transport audit still reports streaming refused. Also checked and found sound, so recorded rather than changed: unknown tools still classify as `deny`, so an upstream tool landing without a classification fails closed; both lockfiles carry `@a2a-js/sdk` and its transitive `jose`; and the new upstream `SessionSourceService` is about attached content sources, not `sourceType: agent`, so it does not touch the server-binding check. 376 / 34 / 21 / 15 across the four audits, all green after the merge. Refs #11206
|
Released in v0.23.3. |
5 participants
What this PR does
Lets a subagent definition delegate its turn to an external coding agent instead of running it in-process. A definition declares an
executorblock naming a command; the turn is then driven over ACP in that external process, and everything that happens is re-published as the same agent events the in-process path emits, so the existing transcript writer, permission bridge, virtual subagent sessions and Web Shell subagent panel all work unchanged. Claude Code is the first and only external agent wired up.A definition that declares an executor now fails loudly when the host cannot honour it. Previously there was no such concept at all, and the measured behaviour of an unrecognized definition field was to run the work in-process anyway — successfully, silently, and billed to the wrong provider.
Why it's needed
Running a second vendor's agent from Qwen Code was only possible by handing the whole session to a different backend, which needs workspace identity to be re-keyed from
cwdto(cwd, backend)in both the daemon registry and the Web Shell session catalog. Delegating instead keeps the parent Qwen session authoritative: no change to workspace identity, no change to the bridge's one-channel-per-runtime invariants, and permissions, todos and artifacts stay owned by the parent.Agent definition files in this repo already mirror Claude Code's
.claude/agents/<name>.mdschema verbatim so a user can drop one into.qwen/agents/. This extends that compatibility from the definition layer to the execution layer.The silent-substitution problem is concrete and was measured on the shipped CLI, not theorised: given a definition carrying an unknown
executorfield,qwen -pcompleted the task, created the requested file, reportedsubtype: "success", and emitted nothing on stdout, stderr or the debug log to indicate the field had been ignored. The subagent's metadata reportedmodel: qwen3.8-max-2026-09-02and its transcript showedread_file,write_file,run_shell_commandandglob— all Qwen tools — with 118,209 tokens billed to the Qwen provider. A user who asked for Claude got Qwen and had no way to tell.Reviewer Test Plan
How to verify
Build, then delegate to a definition that names the Claude Code ACP adapter:
Confirm three things.
impl.txtcontainsIMPL_OK. The subagent's.meta.jsonunder$QWEN_RUNTIME_DIR/projects/<cwd>/subagents/<sessionId>/reportsmodel: external-acp:npxrather than a Qwen model id. The subagent's.jsonltranscript contains Claude tool names and zero occurrences ofwrite_file.Then confirm the fail-closed refusals, which are observable without editing the shipped CLI:
executorblock (wrong-casekind: ACP, a blankcommand, a non-stringargsentry) is refused at load: the definition is skipped with a visibleSkipped invalid file …: invalid executor blockwarning, and it never runs in-process under a Qwen model.executordefinition that also declares a constraint the external process cannot honour (e.g.tools: [read_file],mcpServers,maxTurns) is refused at dispatch withdoes not support …, rather than silently ignoring the constraint.The distinct "host registered no executor at all" refusal (
registered no external agent executor) is a unit-test criterion only —loadCliConfigalways registers the factory, so the shipped CLI never reaches that branch.subagent-manager.test.tsasserts both the refusal and thatAgentHeadless.createis never called when no executor is registered, which is the load-bearing guarantee that a definition asking for an external agent never silently becomes an in-process one.Unit tests:
cd packages/cli && npx vitest run src/external-agents/andcd packages/core && npx vitest run src/subagents/ src/agents/runtime/agent-headless.test.ts src/agents/runtime/agent-core.test.ts.Evidence (Before & After)
Same fixture, same prompt, same machine. Before is the shipped CLI 0.23.0; after is this branch's local build.
subtype: "success", file createdsubtype: "success", file created[external-agent claude-worker] [session/query] … apiType=native.meta.jsonmodelqwen3.8-max-2026-09-02external-acp:noderead_file×2,write_file×2,run_shell_command×2,glob×2Bash;write_filecount 0executorwas ignoredRaw before/after artifacts are in the branch author's spike notes; the after-run transcript and
.meta.jsonare reproducible with the command above.Web Shell
Headless Playwright capture of the Web Shell served by
qwen serveover a workspace whose.qwen/agents/claude-worker.mdcarries theexecutorblock. The prompt asks for theclaude-workersubagent; the turn runs in the external Claude Code process.Web Shell final state
That run was verified independently of the images rather than taken on trust from them:
ws-shots.txtcontains exactlyWS_OK, and the subagent metadata for the session reportspersistedCliFlags.model: "external-acp:node",agentType: "claude-worker",status: "completed", withBashthree times in its transcript and zero qwenwrite_filecalls.external-acp:nodeis the label this PR's executor reports throughgetCore().modelConfig.model; a run that had silently fallen back to in-process would report a Qwen model id there instead.A pre-existing environment caveat, visible in the browser console during capture and unrelated to this PR:
GET /standalone/sessionsreturned 503The Conversations runtime is owned by another daemon. It affects the standalone-session list, not the session flow shown above.Terminal
The same delegation driven through the interactive CLI, captured with the repo's own
terminal-captureharness (node-pty → xterm.js → Playwright headless). Included because it shows the tool-level output more legibly than the Web Shell transcript view.Full scrollback
Its run was verified the same way:
shots.txtcontains exactlySHOTS_OKand the subagent metadata reportsmodel: "external-acp:node",status: "completed".Still not captured: the approval dialog for an external agent's tool call. It needs the permission E2E (test-plan groups 4 and 5), which is outstanding and is the security-relevant gap.
Images are hosted on the
assets-pr11003branch of the author's fork underpr11003/.Tested on
Environment
Local build via
npm run build/npm run bundle, run asnode packages/cli/dist/index.js. Claude Code 2.1.259 and@agentclientprotocol/claude-agent-acp0.73.0.~/.claude/settings.jsonon the test machine setspermissions.defaultMode: "auto", which is the configuration the permission-mode derivation is designed to override.Risk & Scope
qwen/*extension methods are unimplemented by any external agent, so the seven session-scoped diagnostic routes return 500 for one. That does not apply to this PR's delegation path — a delegated subagent is a task inside a Qwen session, not a foreign session — but it does bound what a future peer-backend design could reuse.SubagentExecutorinterface widenscreateAgentHeadless's return type from a concrete class to an interface. All seven production call sites were updated;AgentHeadlessdeclaresimplements, so drift fails at compile time.usage_update {size, used}), which is a level, not a per-turn delta; feeding a level into the accumulating statistics would inflate totals past the window size and trip the workflow budget gate early. The consequence is that an external subagent does not advanceQWEN_CODE_MAX_TOKENS_PER_WORKFLOW. That gate is opt-in and defaults to unbounded, and the agent-count andmax_turns/max_time_minutesbounds still apply.infoconfirmation variant, so the Web Shell shows the tool title and the option names but not a rendered file diff. Theedit/execvariants are a follow-up.executor.commandnames an executable from a project-level file. This is the same trust model the existingmcpServersandhookscommand fields already carry, so it introduces no new boundary; the parser rebuilds the spec from known fields only, so unrecognized keys cannot reach the spawn site.edit/execapproval rendering; per-turn token accounting.executorblock the in-process path is byte-for-byte unchanged, including the system prompt —AgentCore.buildChatSystemPromptnow delegates to an extractedrenderSubagentSystemPromptwith identical behaviour.Linked Issues
None. Design rationale, the measured ACP behaviour this is built against, and the rejected peer-backend alternative are in
docs/design/claude-code-web-shell-backend.md, added here.中文说明
这个 PR 做了什么
让 subagent 定义可以把一轮任务委派给外部 coding agent，而不在进程内执行。定义里声明一个
executor块指名命令；该轮任务就通过 ACP 在那个外部进程里驱动，发生的一切再以与进程内路径相同的 agent 事件重新发布，因此既有的 transcript writer、权限桥、virtual subagent session 与 Web Shell 的 subagent 面板全部无需改动即可工作。Claude Code 是第一个也是目前唯一接入的外部 agent。声明了 executor 的定义，在宿主无法满足它时会显式报错。此前根本没有这个概念，而未识别的定义字段的实测行为是照样在进程内把活干完 —— 成功、静默、并且记账到错误的 provider。
为什么需要
要从 Qwen Code 里跑另一家厂商的 agent，过去只能把整个会话交给另一个后端，而那需要把 workspace 身份从
cwd重新键为(cwd, backend)，且 daemon 注册表与 Web Shell 会话目录两层都要改。改为委派则让父 Qwen 会话保持权威：不动 workspace 身份，不动 bridge 的 one-channel-per-runtime 不变量，权限、todos、artifacts 仍归父会话所有。本仓库的 agent 定义文件已经逐字镜像 Claude Code 的
.claude/agents/<name>.mdschema，用户可以直接把文件丢进.qwen/agents/。这个 PR 把该兼容性从定义层延伸到执行层。静默替换问题是具体的、且在已发布 CLI 上实测过，不是推测：给一个带未知
executor字段的定义，qwen -p会完成任务、创建所要求的文件、报告subtype: "success"，并且在 stdout、stderr、debug log 上都不发出任何"该字段被忽略了"的信号。subagent 的元数据报告model: qwen3.8-max-2026-09-02，transcript 里是read_file、write_file、run_shell_command、glob—— 全是 Qwen 的工具 —— 118,209 个 token 记在 Qwen provider 账上。要 Claude 的用户拿到的是 Qwen，而且无从察觉。评审测试计划
如何验证
构建后，委派给一个指名 Claude Code ACP adapter 的定义（命令见英文正文）。确认三件事：
impl.txt内容为IMPL_OK；$QWEN_RUNTIME_DIR/projects/<cwd>/subagents/<sessionId>/下的.meta.json报告model: external-acp:npx而非 Qwen 的 model id；subagent 的.jsonltranscript 含 Claude 工具名且write_file出现 0 次。再确认 fail-closed 拒绝路径（无需改动已发布 CLI 即可观察）：
executor块（大小写错误的kind: ACP、空白command、非字符串的args项）在加载时被拒绝：该定义被跳过并在 stderr 打出可见的Skipped invalid file …: invalid executor block警告，绝不会以 Qwen 模型静默跑在进程内。tools: [read_file]、mcpServers、maxTurns）的executor定义，在分派时被以does not support …拒绝，而不是静默忽略该约束。另一种"宿主完全未注册 executor"的拒绝（
registered no external agent executor）只是单测判据——loadCliConfig永远会注册该工厂，所以已发布 CLI 根本到不了那个分支。subagent-manager.test.ts同时断言了该拒绝、以及在未注册 executor 时AgentHeadless.create从不被调用——后者正是"请求外部 agent 的定义绝不会静默变成进程内 agent"这一核心保证。证据（前后对比）
同一 fixture、同一 prompt、同一台机器。Before 是已发布的 CLI 0.23.0，After 是本分支的本地构建。对照表见英文正文：任务结果两边都成功；外部 agent 进程 Before 无、After 有（adapter 自己的 stderr 被转发为
[external-agent claude-worker] [session/query] … apiType=native）；.meta.json的 model 从qwen3.8-max-2026-09-02变为external-acp:node；transcript 工具名从四个 Qwen 工具变为Bash且write_file计数为 0；token 记账从 118,209 全在 Qwen provider 变为 subagent 推理不在 Qwen provider 上；"executor 被忽略"的信号从"任何地方都没有"变为不适用（它被履行了）。Web Shell：用无头 Playwright 截取，daemon 为
qwen serve，工作区的.qwen/agents/claude-worker.md带executor块。prompt 要求使用claude-workersubagent，该轮在外部 Claude Code 进程中执行。Web Shell 最终状态
该次运行经过独立于图片的核验：
ws-shots.txt内容确为WS_OK，该会话的 subagent 元数据报persistedCliFlags.model: "external-acp:node"、agentType: "claude-worker"、status: "completed"，transcript 中Bash三次、qwenwrite_file零次。external-acp:node正是本 PR 执行器通过getCore().modelConfig.model上报的标签；若静默回退到进程内，那里会是 Qwen 的 model id。一处既有的环境干扰（与本 PR 无关，截取时在浏览器控制台可见）：
GET /standalone/sessions返回 503The Conversations runtime is owned by another daemon。它影响 standalone 会话列表，不影响上面展示的会话流程。终端：同一次委派经交互式 CLI 驱动，用仓库自带的
terminal-capture（node-pty → xterm.js → Playwright headless）截取。附上是因为它比 Web Shell 的 transcript 视图更清晰地展示工具级输出。完整回滚缓冲
其运行同样经过核验：
shots.txt内容确为SHOTS_OK，subagent 元数据报model: "external-acp:node"、status: "completed"。仍未截取：外部 agent 工具调用的审批对话框。它依赖权限 E2E（测试计划组 4 与组 5），该项尚未完成，是本 PR 安全攸关的缺口。
图片托管在作者 fork 的
assets-pr11003分支pr11003/下。测试环境
本地构建（
npm run build/npm run bundle），以node packages/cli/dist/index.js运行。Claude Code 2.1.259、@agentclientprotocol/claude-agent-acp0.73.0。测试机的~/.claude/settings.json设了permissions.defaultMode: "auto"，而这正是权限模式推导要覆盖掉的配置。风险与范围
qwen/*扩展方法，因此七条 session 级诊断路由对它会返回 500。这不适用于本 PR 的委派路径 —— 被委派的 subagent 是 Qwen 会话内的一个任务，不是一个外部会话 —— 但它界定了将来"对等后端"设计能复用的范围。SubagentExecutor接口把createAgentHeadless的返回类型从具体类放宽为接口。七处生产调用点全部更新；AgentHeadless声明了implements，因此漂移会在编译期失败。usage_update {size, used}），那是水位而非每轮增量；把水位喂进累加式统计会让总量膨胀到超过窗口大小并过早触发 workflow 预算闸门。后果是外部 subagent 不推进QWEN_CODE_MAX_TOKENS_PER_WORKFLOW。该闸门是 opt-in 且默认无上界，且 agent 数量上限与max_turns/max_time_minutes仍然生效。info确认变体，因此 Web Shell 显示工具标题与选项名，但不渲染文件 diff。edit/exec变体留作后续。executor.command指名一个来自项目级文件的可执行文件。这与既有mcpServers、hooks的 command 字段是同一套信任模型，因此没有引入新边界；解析器只从已知字段重建 spec，未识别的键到不了 spawn 点。edit/exec审批渲染；每轮 token 记账。executor块时进程内路径逐字节不变，包括 system prompt ——AgentCore.buildChatSystemPrompt现在委托给抽出的renderSubagentSystemPrompt，行为一致。关联 Issue
无。设计依据、本 PR 所依据的 ACP 实测行为、以及被否决的"对等后端"替代方案都在本 PR 新增的
docs/design/claude-code-web-shell-backend.md里。