From c404b415e4bdf715b92bc81222eee43fe58e56b5 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 6 Sep 2026 07:31:25 +0800 Subject: [PATCH 1/5] feat: delegate a subagent turn to an external agent over ACP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A subagent definition can now declare an `executor` block naming an external agent process. The turn runs there over ACP and is re-published as the same AgentEventEmitter events the in-process path emits, so the JSONL transcript writer, SubAgentTracker's nested-permission bridge, virtual subagent sessions and the Web Shell subagent panel all work unchanged. Claude Code is the first agent wired up. Delegating rather than exposing a foreign agent as a peer session backend keeps the parent Qwen session authoritative: no re-keying of workspace identity from cwd to (cwd, backend), and no change to the bridge's one-channel-per-runtime invariants. Agent definition files already mirror Claude Code's `.claude/agents/*.md` schema verbatim, so this extends that compatibility from the definition layer to the execution layer. Core side: a `SubagentExecutor` interface narrowed to the members production callers actually use, which `AgentHeadless` now implements so drift fails at compile time; `createAgentHeadless` returns the interface and dispatches on `config.executor`; `AgentCore.buildChatSystemPrompt` is extracted as `renderSubagentSystemPrompt` (a verbatim move, the private method delegates) so an executor that never builds an AgentCore produces a byte-identical prompt; `dispose?()` is composed into the returned dispose so the external process cannot outlive its subagent; and the host factory is injected through `Config.setExternalAgentExecutor`, following the existing `setSessionWorkflowEnabledProvider` inversion, so core gains no ACP dependency. The types a host needs are exposed on a narrow `./subagentRuntime` subpath rather than the package root. Because the key (`subagentRuntime`) does not mirror its source stem (`subagent-runtime.ts`), the wildcard cannot resolve it, so every resolution map that fronts core's exports carries an explicit entry: the cli `tsconfig` paths and vitest alias, `integration-tests/tsconfig.json` paths, and the skill-review-harness loader's named map. A cross-package contract test pins the export entry, the barrel's re-exports and the cli resolutions; the repo's own `integration-tsconfig-core-paths-sync` and `text-capture-core-loader-sync` guards pin the other two, so a future named export that forgets a mirror fails CI rather than silently falling through to a stale `dist`. Everything that could silently substitute a different agent, or grant authority nobody approved, fails loud instead. Three bypass paths are closed. Frontmatter rejects a malformed `executor` block outright rather than dropping it — dropping it leaves `config.executor` undefined, so no downstream check engages and the turn runs in-process under Qwen's model with nothing on stdout, stderr or the debug log — and the block is re-validated at the consumption point, because session-level subagents are injected as plain objects and bypass frontmatter parsing entirely, and at serialization, so a save/reload round trip cannot drop the backend. An executor-bearing file whose frontmatter YAML does not parse is rejected too, but only such a file: the validation reads the original document node and refuses when `parseDocument` reports an error AND the file claims a top-level `executor` — by AST key, by the sanitized frontmatter value, or by a raw-text probe anchored to column 0 with optional quotes, so a quoted `"executor":` is caught (the lenient parser keeps the quotes in the key and a repaired `parseDocument` nests it, so both miss it) while an indented `executor:` under `mcpServers:` or inside a `description: |` block scalar is not (it declares nothing and must not hard-refuse an unrelated definition). The error must also be able to reach the executor. The guard refuses when `parseDocument` — the real YAML parser — lost a text-claimed executor key (`!hasExecutor`): YAML errors are not line-local, so a compact-mapping error on an earlier line drops the whole remainder, and the lenient `parseSimple` fallback must not be trusted to rebuild it — it is a line-based heuristic that mangles block scalars and sequences, turning `command: |` plus an indented `npx` into `command: '|'` and so spawning an executable literally named `|` that the file never declared. It also refuses when `parseDocument` kept the node but an error sits at or after the executor's line, where a repair could rewrite it into a different command/args. Only an error strictly before the executor line that `parseDocument` survived (`hasExecutor` still true) leaves the subtree byte-faithful, so a valid external-agent definition — e.g. one carrying a duplicate `name:` from a bad merge — keeps loading instead of vanishing from `/agents` over an unrelated quirk. And because `parseDocument` tolerates an unresolved YAML alias (`command: *undef`) with an *empty* `document.errors` while `toJS()` throws when it resolves the node, the `toJS()` read is wrapped so that case is refused as an invalid executor block rather than escaping parseSubagentContent as a raw parse error the errors-based guard cannot see. For a real executor the lenient parser would otherwise repair a malformed document into a node that dispatches a different command/args than the file declares, or drops the key and runs in-process. Every other definition keeps loading through the shared parser's lenient `parseSimple` fallback — a description containing a colon, a tab-indented field, or a duplicate key from a bad merge must not silently vanish from `/agents`. The refusal message carries `invalid executor block` so `warnInvalidSubagentFile` surfaces it on stderr instead of the debug log. `convertToRuntimeConfig` rejects executor definitions, which is the path team and background spawning use. Workflow `agent()` rejects them before spawning, because token budgets, schema output and workflow tool restrictions cannot be enforced across the process boundary — that covers bounded and unbounded workflows alike. Cold resume is denied for a transcript carrying `executor: 'acp'` or a legacy `external-acp:*` model label; the label is deny-only and never selects an executor. With no factory registered, `create()` throws before warming providers or resolving a model, and an ordinary Error from an external factory propagates unwrapped instead of being relabelled as an AgentHeadless construction failure. This deliberately diverges from the lenient drop used for mcpServers and hooks: losing those degrades a capability, losing this one substitutes a different agent. The permission mode is the host's effective, already-clamped approval policy — not the definition's raw request and never the external agent's own config. The Agent tool resolves the definition's `approvalMode` against the parent session's mode and folder trust and stamps the result onto the runtime context, and the manager reads that resolved value back when it dispatches, so a definition cannot escalate the peer past the parent session's limit. The executor requires the resolved mode to be present in `session.modes.availableModes` and applies it with `session/set_mode` before the first prompt, because the agent's own `defaultMode` was measured to let a Write execute with no permission request emitted at all. When the agent offers no option matching the user's outcome the executor denies rather than falling back to the first offered option — approving "proceed once" against `[allow_always, reject_once]` must not answer `allow_always`. `optionKindForOutcome` is exhaustive over the enum with a `never` check and a runtime default that denies, so an unmapped outcome cannot become a grant. A host-policy denial (headless, permission avoidance, a display-only TOOL_WAITING_APPROVAL listener, or a routed interactive question) rejects the TOOL by selecting a `reject_once` option rather than answering ACP `cancelled`, because `cancelled` tells a conforming peer the whole turn is over and would abort the delegation on its first sensitive tool; it falls back to `cancelled` only when the peer offered no reject option, and the stream-json responder path still answers normally. An explicit user rejection of a single tool — the dialog's Cancel/Esc, which `selectPermissionOption` maps to undefined — likewise selects the peer's `reject_once` option instead of answering `cancelled`, so denying one action scopes to that tool and the turn continues, matching the button's "suggest changes" promise and the in-process sibling (which records "User did not allow tool call" and lets the model proceed) rather than abandoning the remaining work and billing a fresh prompt to re-delegate. Child-process handling: the child is spawned detached with `sanitizeChildEnv`, because `executor.command` comes from a project-level file, so a repository the user merely cloned picks the executable and must not receive the daemon bearer token. The handshake races a 10s deadline against both the `error` and `exit` events, since a command that spawns and stays silent would otherwise hang `create()` forever. Turn wall time defaults to 10 minutes when `max_time_minutes` is omitted, matching the workflow subagent default, and an invalid value throws before spawn. The child is owned by acp-bridge's ProcessRegistry with `ownsProcessTree`, so disposal escalates SIGTERM to SIGKILL and a SIGTERM-resistant descendant cannot survive; `dispose()` suppresses only the two shapes where the foreign root is already gone — the signal it was sent, and a root that exits on its own mid-snapshot (a Linux race where the prompt rejects on stdout EOF before the process `exit` event, invalidating the initial tree snapshot) — via the exported, unit-tested `isExpectedExternalAgentCleanupExit`. The second shape covers the peer's own exit status for ANY code/signal (`exited uncleanly during shutdown`), which acp-bridge raises only after it has driven every owned process group to empty, so it reports a foreign agent that exited — already reflected in `terminateMode` — not a cleanup failure; letting `dispose()` reject on it would replace the turn's declared terminal state (a user-CANCELLED turn would surface to the parent as `failed`). Every genuine cleanup-PROOF failure (a truncated snapshot, an absent or non-group-leader root, a failed snapshot/signal/inspect, or an exceeded exit deadline — i.e. a descendant that may have survived) still propagates. A post-handshake exit handler drains parked approvals; its ERROR emit is guarded by `rawListeners`, because AgentEventType.ERROR is Node's 'error' event and the background, resume and workflow emitters attach no listener, so an unguarded emit would turn a child crash into an uncaught exception. stderr goes through acp-bridge's `createStderrForwarder` for chunk buffering and credential redaction. `methodNotFound` returns a real `RequestError`, the only form the SDK preserves as -32601 rather than repackaging as -32603, and the SDK's `extMethod` hook answers the same way instead of hanging the agent. Token usage for an external turn is unknown rather than free: the adapter exposes only a context-window gauge, and feeding a level into accumulating statistics would inflate totals past the window. The Agent tool therefore suppresses execution stats for external subagents, skips live and completion stat refresh, reports no execution summary, and appends "[External executor token usage and cost are unavailable.]" to the result. That notice is a suffix appended after the model-visible text and its empty-text fallbacks on both the foreground and background completion paths, never baked into the text itself — baking it in would make a non-GOAL background run that produced no text publish the notice in place of the real failure reason (`Agent terminated with mode: TIMEOUT`), because the `finalText || ` fallback would see a non-empty string. Transcript metadata records `executor: 'acp'` and no `persistedCliFlags`, so the provenance is inspectable and the resume denial above has something to key on. Stop reasons are mapped narrowly — end_turn to GOAL, max_turn_requests to MAX_TURNS, cancelled to CANCELLED, and refusal, max_tokens and anything unknown to ERROR — so a truncated external turn cannot be reported as completed, and a peer that *refused* the task is not relabelled as a user cancel (CANCELLED renders "cancelled by the user", a false statement that also hides the refusal from telemetry). A wall-time expiry is classified by the timer's own `ExternalAgentTimeoutError` identity rather than re-reading `Date.now()` in the catch, because a second realtime clock can miss the monotonic deadline by under a millisecond and misreport a turn that merely reached its budget as ERROR instead of TIMEOUT. The continuation loop also re-checks that budget before dispatching each round: `remaining` clamps to 0 once the wall time is spent, and without a guard `connection.prompt(...)` is evaluated first — a new, billed model turn really reaches the peer — one tick before `wait`'s 0ms timer rejects with `ExternalAgentTimeoutError`, so the catch disposes (SIGTERM/SIGKILL) a peer that just started work and the message the round already drained and recorded as delivered is never processed. The loop now sets `TIMEOUT` and breaks before that dispatch when `remaining <= 0`, mirroring the in-process sibling, which checks the budget at the top of every round; it sets `terminateMode` directly rather than throwing, because the catch classifies wall-time expiry by error identity. And a continuation `execute()` turn — a resident external agent re-invoked per incoming user message — emits the task as a user-side `EXTERNAL_MESSAGE`, matching the in-process sibling, so the JSONL transcript does not lose every message after the first. When a turn ends with a tool call still open (cancel sets `cancelled` before the peer's terminal `tool_call_update` can arrive, and timeout or peer crash leave it mid-flight), `runTurn`'s finally flushes every entry left in the tool map as a failed `TOOL_RESULT` + `TOOL_RESPONSES_FINALIZED` before `FINISH`, so the inline frame and Web Shell row stop showing a tool executing forever, the JSONL `functionCall` is paired with a `tool_result`, and the `FINISH` totals sum — the in-process sibling closes the same gap via `onAllToolCallsComplete`. The session-update entry point drops any update once the turn is over for ANY reason — its guard now also checks `disposed` and the integrity-abort signal, not just `cancelled` — because on the timeout and crash paths the catch awaits `dispose()` before the finally clears `executing` and `cancelled` stays false, so a peer still writing would otherwise be appended to `finalText`, emitted as `STREAM_TEXT`/`ROUND_TEXT`, and counted into the `FINISH` totals for up to `EXIT_DEADLINE_MS` after termination, handing a partial result to the parent verbatim on the non-rethrowing TIMEOUT path. Safe mode also refuses an external executor at the dispatch gate, not only by filtering discovery: `loadSubagent` resolves a `subagent_type` from disk regardless of safe mode, and a cloned repo can ship `.qwen/agents/.md` carrying an `executor` that resolves before the built-in of the same name, so safe mode now throws rather than spawn a definition-supplied binary. The design doc now describes only the shipped product and these fail-loud rules; the rejected peer-backend alternative is recorded as rejected. Verification: core and cli `tsc` 0 errors, ESLint and Prettier clean, `git diff --check` clean, cli executor tests 48/48, core subagent/agent/runtime suites 1483/1483, cross-package contracts and the two core-export sync guards 17/17. Mutation proofs, each run in both directions: removing the frontmatter executor guard fails 8 tests, removing the workflow `agent()` rejection fails 6, replacing the deny fallback with the first offered option fails 3, returning a plain Error instead of `RequestError` fails the -32601 wire assertion, narrowing `isExpectedExternalAgentCleanupExit` back to the signal-only form fails the cleanup-exit classification test (which now also pins tolerating the peer's own non-zero exit code), answering a host-policy denial with `cancelled` instead of a `reject_once` selection fails 5, deriving the peer mode from the raw definition instead of the host-resolved policy fails its dispatch test, reading the sanitized frontmatter value instead of the original YAML node fails the null-args rejection, making the `document.errors` refusal unconditional (instead of executor-scoped) fails the non-executor lenient-load test, un-anchoring the executor-claim text probe (dropping the column-0 / optional-quote form) fails both the quoted-key refusal and the nested-key lenient-load tests, disabling the turn-end `flushOpenTools` fails the open-tool flush test, deleting the `loadCliConfig` executor registration fails the registration test, narrowing the session-update guard back to `cancelled`-only lets a post-timeout `LATE` chunk into `finalText` (failing the post-terminal-drop test), and disabling the safe-mode dispatch refusal lets an external executor run in a trusted safe-mode folder (failing the safe-mode refusal test), routing a user's Cancel back to ACP `cancelled` (dropping the `reject_once` fallback at the respond site) fails the user-cancel denial test, and reverting the executor-error guard to fire on any `document.errors` entry instead of scoping to errors that reach the executor line fails the unrelated-error-before-executor load test while the quoted-key and unterminated-quote refusals stay green, removing the continuation budget guard lets a 0-`remaining` round dispatch a second prompt (failing the no-dispatch-after-budget test, which then sees two prompts and GOAL instead of one and TIMEOUT), and reverting `astLostExecutor` to also require `frontmatter.executor === undefined` lets a `parseDocument`-dropped executor load through the `parseSimple` fallback as `command: '|'` (failing the dropped-node refusal test), and re-throwing the raw error from the `toJS()` wrapper instead of the graceful `SubagentError` fails the unresolved-alias refusal test (the rejection becomes the raw "Unresolved alias" YAML error, not /invalid executor block/). Mapping `refusal` back to CANCELLED fails the continuation test, and dropping the continuation `EXTERNAL_MESSAGE` emit fails the transcript test. The wall-time test was the flake the timeout-classification fix removes; it now passes 5/5 deterministic runs. The real-subprocess suite is gated `describe.skipIf(process.platform === 'win32')` like the repo's other real-process suites, because Windows tree-kill reports a numeric exit code that `dispose()` surfaces as unclean — the `test_windows` lane is merge-queue-only, so an ungated suite there would eject the entry. The executor tests drive a real ACP child process speaking the wire protocol, across init/session/mode/prompt hangs and exits, max_tokens and unknown stop reasons, duplicate permission callIds and optionIds, ask-user denial, descendant tree kill, env sanitization and unsupported-extension responses. A separate end-to-end run drove the real `@agentclientprotocol/claude-agent-acp` adapter and a real model through this executor: `session/set_mode` `default` landed before the prompt despite conflicting factory inputs, one permission request was emitted and answered, a `Cancel` produced no file while a `ProceedOnce` selected the adapter's `allow-once` (not its broader `allow-always`) and wrote exactly the requested content, and both turns reported zero tokens — which is why the Agent tool suppresses the summary rather than presenting zeros as free. Not done: the real-adapter run above is a direct-factory harness, not the full settings-loader → CLI → Web Shell path, and it does not exercise a browser approval dialog, a second permission request in one turn, or headless auto-denial against the real adapter; only macOS has been exercised, so the Windows `.cmd` spawn path is unimplemented and untested (R3-1, left open — `cross-spawn`, the repo's safe Windows launcher, is not a `packages/cli` dependency, and `shell: true` around a project-supplied command is an injection surface, so this needs a dependency decision and a Windows-testable design rather than a guess); the optional mid-turn external-message waiter is unimplemented (R3-6, left open — queued messages are still drained between turns, but an external subagent cannot park for a not-yet-arrived one); and the Web Shell approval dialog still uses the `info` confirmation variant, so it renders no file diff and has no screenshot here. `ROUND_START`/`ROUND_END`/`USAGE_METADATA` are still not emitted, but the one Critical consumer — the workflow stall watchdog, which arms only on those events — can no longer be reached by an external agent at all, because workflow `agent()` rejects external-executor definitions before spawn; `USAGE_METADATA` also has no honest source (the adapter exposes a context-window gauge, not per-turn token deltas). The round-4 through round-7 fixes in this commit were audited to convergence by mutation proof and direct full-diff review; the earlier rounds' agent-driven directionless audit did not converge because those agents died on infrastructure failures. --- docs/design/claude-code-web-shell-backend.md | 144 +++ .../skill-review-harness/text-capture.tsx | 1 + integration-tests/tsconfig.json | 3 + packages/cli/src/config/config.test.ts | 16 + packages/cli/src/config/config.ts | 8 + .../acp-subagent-executor.test.ts | 747 ++++++++++++++ .../external-agents/acp-subagent-executor.ts | 938 ++++++++++++++++++ packages/cli/tsconfig.json | 3 + packages/cli/vitest.config.ts | 4 + packages/core/package.json | 4 + packages/core/src/agents/agent-transcript.ts | 2 + .../agents/background-agent-resume.test.ts | 55 + .../src/agents/background-agent-resume.ts | 39 +- .../core/src/agents/runtime/agent-core.ts | 60 +- .../core/src/agents/runtime/agent-headless.ts | 3 +- .../src/agents/runtime/subagent-executor.ts | 74 ++ .../runtime/workflow-orchestrator.test.ts | 92 ++ .../agents/runtime/workflow-orchestrator.ts | 7 + packages/core/src/config/config.ts | 24 + packages/core/src/subagent-runtime.ts | 53 + .../agent-frontmatter-schema.test.ts | 97 ++ .../src/subagents/agent-frontmatter-schema.ts | 45 + .../src/subagents/subagent-manager.test.ts | 538 ++++++++++ .../core/src/subagents/subagent-manager.ts | 281 +++++- packages/core/src/subagents/types.ts | 36 + packages/core/src/tools/agent/agent.test.ts | 79 ++ packages/core/src/tools/agent/agent.ts | 170 ++-- scripts/tests/cross-package-contracts.test.js | 31 + 28 files changed, 3462 insertions(+), 92 deletions(-) create mode 100644 docs/design/claude-code-web-shell-backend.md create mode 100644 packages/cli/src/external-agents/acp-subagent-executor.test.ts create mode 100644 packages/cli/src/external-agents/acp-subagent-executor.ts create mode 100644 packages/core/src/agents/runtime/subagent-executor.ts create mode 100644 packages/core/src/subagent-runtime.ts diff --git a/docs/design/claude-code-web-shell-backend.md b/docs/design/claude-code-web-shell-backend.md new file mode 100644 index 00000000000..e660ea4992a --- /dev/null +++ b/docs/design/claude-code-web-shell-backend.md @@ -0,0 +1,144 @@ +# Claude Code 作为 Qwen 委派子 agent + +状态：选定产品设计；实现与验证仍在进行。 + +本文定义目标契约与验收条件，不是测试报告。接口、分派与执行器正在修订， +代码中存在某个字段或事件不代表端到端行为已经满足契约。 + +## 1. 产品范围 + +Qwen 保持父会话与任务编排者的身份，通过 Agent 工具把任务委派给 Claude Code。 +Claude 通过本机 ACP adapter 运行；父会话接收任务结果，用户观察子任务并可停止它。 +Web Shell 使用子 agent 展示入口，权限决策仍在父会话中完成。 + +本设计不增加可直接聊天的 Claude 对等后端，不增加独立 Claude workspace runtime， +不改变 workspace 的身份键或 HTTP 路由，也不替换 daemon 的 Qwen ACP 子进程。 +**既有 daemon external-tool guard 及其强制握手保持不变。** +Claude 子进程不因此自动获得 Qwen 内部工具策略的保护；两者的安全边界必须区分。 + +支持目标是普通前台、后台委派及同一存活执行器上的后续输入。 +不承诺 Claude 独立会话恢复、跨进程重启续跑、fork、Agent Team / teammate、 +其他厂商执行器或 `qwen/control/*` 扩展的替代实现。 +子任务面板不是独立聊天入口；查看已记录内容不等同于恢复外部执行器。 + +## 2. 定义与启动 + +agent 定义通过独立的 `executor` 字段选择执行器，不能把 `model` 当作后端选择器。 +示例放在 `.qwen/agents/claude-worker.md`；先由用户安装并固定所需 adapter 版本， +确认其可执行文件 `claude-agent-acp` 位于 PATH，并按 adapter 要求完成 Claude 认证： + +```markdown +--- +name: claude-worker +description: Delegate a bounded task to Claude Code +executor: + kind: acp + command: claude-agent-acp + args: [] +permissionMode: default +--- + +Complete the delegated task and report the result and any limitations. +``` + +`command` 是实际启动的 ACP adapter 可执行文件，不是假定存在的 `claude --acp`。 +运行时不得静默通过 `npx` 下载 latest，也不得替用户安装或升级 adapter。 +命令缺失、认证失败、握手失败或宿主未注入执行器都必须明确报错。 +无效的显式 `executor` 定义必须拒绝，不能丢弃该字段后转用 Qwen。 +同样不得在重试、后台续跑或其他分派路径中静默替换后端。 + +## 3. 分层与接口 + +实现边界对应以下文件；这些是代码位置，不是完成状态或测试证据： + +- `packages/core/src/agents/runtime/subagent-executor.ts`：执行器与工厂契约。 +- `packages/core/src/subagents/subagent-manager.ts`：定义解析、运行配置与分派入口。 +- `packages/cli/src/external-agents/acp-subagent-executor.ts`：子进程、ACP 与事件翻译。 + +core 定义 `SubagentExecutor`，CLI 注入 `ExternalAgentExecutor`；core 不承担 ACP transport。 +分派必须在创建 Qwen provider、工具注册表或执行定义 hooks 等副作用之前识别外部定义， +完成可信度与约束校验，再把派生的工作目录、提示、权限意图和运行配置传给 CLI。 +未声明外部执行器的定义继续使用 `AgentHeadless`。 + +执行器公共面包括 `execute`、`executeExternalInputs`、结果文本、终止原因、统计摘要、 +窄化的 `getCore` 视图、外部消息 provider，以及可选等待回调和资源释放方法。 +`getCore` 只提供事件 emitter、模型标签及可选运行视图，不伪造完整 `AgentCore`。 +模型标签必须标明外部执行器，不能把执行归到父 Qwen 模型。 +管理器返回的 `dispose` 必须组合执行器与宿主资源的清理责任。 + +提示模板由宿主渲染后与任务文本交给 adapter；通过 ACP 文本提交的指令 +不等同于 Claude 原生 system prompt。不能声称其优先级或上下文与 Qwen 完全一致。 +Qwen 的工具、skills、MCP、hooks 与模型配置也不会仅因传入接口就被 Claude 执行。 + +## 4. 信任与权限 + +外部定义会启动本机代码。项目来源的定义必须在工作区受信任后才能启动， +未受信任时不得先启动 adapter 再拒绝。子进程使用目标工作目录和过滤后的环境， +不继承 daemon bearer token 等 Qwen 内部秘密；环境过滤本身不是进程沙箱。 + +所有显式配置的限制必须可执行，不能静默忽略。工具白名单、禁用工具、 +执行限制、只读要求、MCP、hooks、模型选择及轮数限制等，若没有可靠映射或执行保障， +必须在启动前拒绝并指出不支持的约束；默认内部配置与用户显式要求须区分。 +仅在提示中要求“不要写文件”不能兑现只读策略。 + +权限模式必须由宿主的有效审批策略决定，子定义不能绕过父会话的限制。 +不能依赖 Claude 本地设置，也不能把 `session/new` 中的 `_meta.permissionMode` +视为已确认生效的协议契约。**计划修复是建会话后、首个 prompt 前调用原生 +`session/set_mode`，确认所需模式可用并设置成功；否则失败关闭。** +未知或不支持的显式模式必须拒绝，不能静默采用更宽松的本地默认值。 + +Claude 的 `session/request_permission` 转为宿主审批事件，经父 bridge session +路由到用户；不能把仅用于展示的 virtual subagent id 当成权限会话身份。 +响应必须选择 adapter 本次实际提供的 optionId；一次授权不得升级为永久授权。 +无监听者、无有效选项、未知审批结果、连接断开或取消时一律拒绝或取消， +并释放等待中的请求，不能挂起到无限期，也不能默认批准。 + +## 5. 生命周期与输入 + +每个外部执行器拥有自己的子进程、ACP 会话、待决权限与运行状态。 +启动、握手和模式设置须有期限，任一步失败都清理已创建资源。 +一次只允许一个受管理的活动 turn；并发输入必须序列化或明确拒绝。 + +已经取消的调用不得发起 prompt。活动取消先发送 ACP cancel， +若 adapter 不响应，须在有限宽限期后终止所拥有的进程及其子进程并等待回收。 +进程退出、连接损坏、超时和 dispose 都必须结束待决调用与权限等待。 +重复清理应安全，不能误杀其他会话；后台任务只在其管理器持有期间保留执行器。 + +后续输入必须使用相同的取消、统计、结果重置与终止事件路径。 +私有 steering 只有在能力明确支持时才可使用，不能让 adapter 自行启动不可跟踪的 turn。 +未支持 mid-turn 输入时不得假装消息已送达；空闲时的后续 prompt 仍须受生命周期管理。 +未知 stop reason、拒绝或截断不能无条件映射为成功完成。 + +## 6. 事件、记录与预算 + +ACP 更新翻译为 Qwen 的文本流、工具调用、结果、审批及终止事件， +供现有 transcript writer、virtual subagent session 和 Web Shell 消费。 +工具结果须保持 callId 与名称关联，重复终态不得重复计数；错误须可见且不崩溃父进程。 +每轮成功、失败和取消都必须产生一致终态，后续轮次不能混入上一轮的最终文本。 + +首期不承诺完整保真：文本和工具状态是基本目标，结构化工具输出、文件 diff、 +多模态内容及专用 edit/exec 审批展示只有经过映射与验收后才可宣称支持。 +只生成文本事件不代表已拥有完整 transcript；持久化、回放和 Stop 必须逐层验证。 + +`usage_update` 的上下文占用不是每轮生成 token 数，不能求和或当作 output tokens。 +未知 token 使用量必须表达为不可用，**不是免费执行或已知的零消耗**。 +若兼容统计结构暂存零，消费者仍须独立识别其不可计量状态。 +首期 workflow `agent()` 一律在启动前拒绝外部执行器定义：尚不能兑现 workflow 的 +预算、schema 和工具限制。普通 Agent 工具委派不受此门控影响。 +仅告警后继续运行不能保护预算；普通委派仍须遵守取消和时间限制。 + +## 7. 验收条件（待执行，不代表已通过） + +- 前台和后台委派确实启动指定 adapter，结果返回父会话，且不创建对等 runtime。 +- 无效定义、缺命令、缺注入、未知配置均明确失败；Qwen 替代执行次数为零。 +- 未受信任项目与不支持的显式约束在启动前失败，且无 hook、provider 或子进程副作用。 +- adapter 使用宽松本地默认时，仍在首个 prompt 前完成宿主要求的原生模式设置。 +- 模式不支持或设置失败时无 prompt；allow/deny、缺监听者与取消均按最小授权处理。 +- 正常完成、预取消、活动取消、不响应 cancel、子进程退出及重复 dispose 均能有限收敛。 +- 后续输入与并发调用不产生脱管 turn；状态、结果、权限等待和工具计数不跨轮污染。 +- Web Shell 可观察文本与工具状态、回放记录并停止任务，审批出现在父会话。 +- 有或没有 token 上限的 workflow 都在外部启动前拒绝；未知使用量不展示为已知零成本。 +- 既有 Qwen 委派、workspace 身份与 daemon guard 行为不变。 + +验收应结合针对性单测、可控制 ACP 对端的协议与失败路径测试，以及固定 adapter 版本的 +真实委派测试；保存可复查结果后再更新实现状态。本文不引用私人运行日志作为完成依据。 diff --git a/integration-tests/terminal-capture/skill-review-harness/text-capture.tsx b/integration-tests/terminal-capture/skill-review-harness/text-capture.tsx index a56c6ca13ab..e2692157d43 100644 --- a/integration-tests/terminal-capture/skill-review-harness/text-capture.tsx +++ b/integration-tests/terminal-capture/skill-review-harness/text-capture.tsx @@ -248,6 +248,7 @@ async function main() { ['subSessionConstants', 'tools/sub-session-constants.ts'], ['toolWriteOrigin', 'services/tool-write-origin.ts'], ['userPromptSubmitContext', 'hooks/user-prompt-submit-context.ts'], ['noFollowOpen', 'utils/no-follow-open.ts'], ['conversationsRuntimeMarker', 'utils/conversations-runtime-marker.ts'], + ['subagentRuntime', 'subagent-runtime.ts'], ['storage', 'config/storage.ts'], ['atomicFileWrite', 'utils/atomicFileWrite.ts'], ['debugLogger', 'utils/debugLogger.ts'], ]); diff --git a/integration-tests/tsconfig.json b/integration-tests/tsconfig.json index 45dfa216bda..482ab6e5c07 100644 --- a/integration-tests/tsconfig.json +++ b/integration-tests/tsconfig.json @@ -63,6 +63,9 @@ "@qwen-code/qwen-code-core/conversationsRuntimeMarker": [ "../packages/core/src/utils/conversations-runtime-marker.ts" ], + "@qwen-code/qwen-code-core/subagentRuntime": [ + "../packages/core/src/subagent-runtime.ts" + ], "@qwen-code/qwen-code-core/storage": [ "../packages/core/src/config/storage.ts" ], diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 71c0059c6fd..66e33acbf3e 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -1193,6 +1193,22 @@ describe('loadCliConfig', () => { ]); }); + it('registers the external agent executor factory so executor definitions dispatch (R1-7)', async () => { + process.argv = ['node', 'script.js']; + const argv = await parseArguments(); + + const config = await loadCliConfig({}, argv); + + // This single host-side registration is what the whole external-subagent + // feature dispatches through. Every dispatch test mocks + // getExternalAgentExecutor, so without this assertion deleting the + // injection would regress every valid executor definition to "registered no + // external agent executor" with the whole suite still green. + const factory = config.getExternalAgentExecutor(); + expect(factory).toBeDefined(); + expect(typeof factory?.create).toBe('function'); + }); + it('enables debug file logging for --debug when QWEN_DEBUG_LOG_FILE is unset', async () => { delete process.env['QWEN_DEBUG_LOG_FILE']; process.argv = ['node', 'script.js', '--debug']; diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index d1e65a738b4..88887cc1178 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -2461,6 +2461,14 @@ export async function loadCliConfig( const config = new Config(configParams); + // Load the ACP transport only when an external subagent is requested. + config.setExternalAgentExecutor({ + create: (params) => + import('../external-agents/acp-subagent-executor.js').then((module) => + module.acpExternalAgentExecutor.create(params), + ), + }); + if (lspEnabled) { try { const lspService = new NativeLspService( diff --git a/packages/cli/src/external-agents/acp-subagent-executor.test.ts b/packages/cli/src/external-agents/acp-subagent-executor.test.ts new file mode 100644 index 00000000000..7ce7bd9e3f5 --- /dev/null +++ b/packages/cli/src/external-agents/acp-subagent-executor.test.ts @@ -0,0 +1,747 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { + AgentEventEmitter, + AgentEventType, + AgentTerminateMode, + ContextState, + ToolConfirmationOutcome, + type ExternalAgentExecutorParams, + type SubagentExecutor, +} from '@qwen-code/qwen-code-core/subagentRuntime'; +import { Config, InputFormat } from '@qwen-code/qwen-code-core'; +import { + acpExternalAgentExecutor, + externalModelLabel, + isExpectedExternalAgentCleanupExit, + optionKindForOutcome, + resolvePermissionMode, + selectPermissionOption, + selectRejectOption, +} from './acp-subagent-executor.js'; + +const fixture = String.raw` +import { AgentSideConnection, ndJsonStream } from '@agentclientprotocol/sdk'; +import { Readable, Writable } from 'node:stream'; +import { spawn } from 'node:child_process'; +const scenario = process.argv[1]; +if (scenario === 'term-resistant' || scenario === 'late-after-timeout') + process.on('SIGTERM', () => {}); +let mode; +let prompts = 0; +let permissionCount = 0; +let connection; +const send = (update) => connection.sessionUpdate({sessionId:'fixture', update}); +connection = new AgentSideConnection(() => ({ + initialize: async () => { + if (scenario === 'init-exit') process.exit(3); + if (scenario === 'init-hang') return new Promise(() => {}); + return {protocolVersion:1, agentCapabilities:{}}; + }, + newSession: async () => { + if (scenario === 'session-exit') process.exit(4); + if (scenario === 'session-hang') return new Promise(() => {}); + return {sessionId:'fixture', modes:{currentModeId:'auto', availableModes:scenario === 'no-mode' ? [] : [{id:'default',name:'Ask'},{id:'plan',name:'Plan'}]}}; + }, + setSessionMode: async (params) => { + if (scenario === 'mode-error') throw new Error('cannot set mode'); + if (scenario === 'mode-hang') return new Promise(() => {}); + mode = params.modeId; + return {}; + }, + authenticate: async () => ({}), + cancel: async () => { await send({sessionUpdate:'agent_message_chunk',content:{type:'text',text:'CANCEL_RECEIVED'}}); }, + prompt: async (params) => { + prompts++; + if (mode !== 'default' && mode !== 'plan') throw new Error('PROMPT BEFORE SAFE MODE'); + if (scenario === 'prompt-exit') process.exit(5); + if (scenario === 'prompt-close') { process.stdout.end(); return new Promise(() => {}); } + if (scenario === 'prompt-hang') return new Promise(() => {}); + if (scenario === 'unsupported-extension') { + try { + await connection.extMethod('_fixture/unsupported', {sessionId:'fixture'}); + throw new Error('Unsupported method unexpectedly succeeded'); + } catch (error) { + await send({sessionUpdate:'agent_message_chunk',content:{type:'text',text:JSON.stringify({code:error.code,message:error.message})}}); + } + return {stopReason:'end_turn'}; + } + if (scenario === 'tree') { + const descendant = spawn(process.execPath, ['-e','process.on("SIGTERM",()=>{});process.send("ready");setInterval(()=>{},1000)'], {stdio:['ignore','ignore','ignore','ipc']}); + await new Promise(resolve => descendant.once('message', resolve)); + await send({sessionUpdate:'agent_message_chunk',content:{type:'text',text:String(descendant.pid)}}); + return {stopReason:'end_turn'}; + } + if (scenario === 'tool-open-hang') { + await send({sessionUpdate:'tool_call',toolCallId:'open-1',title:'Shell',kind:'execute',status:'in_progress',rawInput:{command:'sleep'}}); + return new Promise(() => {}); + } + if (scenario === 'late-after-timeout') { + // Outlive the wall-time budget, then emit after the turn was declared + // over (during dispose's SIGTERM grace). The widened onSessionUpdate + // guard must drop these (R8-1). + await new Promise((resolve) => setTimeout(resolve, 400)); + await send({sessionUpdate:'agent_message_chunk',content:{type:'text',text:'LATE_AFTER_TIMEOUT'}}); + await send({sessionUpdate:'tool_call',toolCallId:'late-1',title:'Shell',kind:'execute',status:'in_progress',rawInput:{}}); + await send({sessionUpdate:'tool_call_update',toolCallId:'late-1',status:'completed'}); + return {stopReason:'end_turn'}; + } + if (scenario.startsWith('permission')) { + const toolName = scenario === 'permission-question' ? 'AskUserQuestion' : scenario === 'permission-question-snake' ? 'ask_user_question' : 'Write'; + const request = () => connection.requestPermission({sessionId:'fixture',toolCall:{toolCallId:'same',title:'Write',kind:'edit',_meta:{claudeCode:{toolName}}},options:[{optionId:'once',name:'Once',kind:'allow_once'},{optionId:'always',name:'Always',kind:'allow_always'},{optionId:'no',name:'No',kind:'reject_once'}]}); + const result = scenario === 'permission-duplicate' ? await Promise.all([request(), request()]) : [await request()]; + await send({sessionUpdate:'agent_message_chunk',content:{type:'text',text:JSON.stringify(result)}}); + permissionCount++; + return {stopReason: result[0] && result[0].outcome.outcome === 'cancelled' ? 'cancelled' : 'end_turn'}; + } + if (scenario === 'env') { + await send({sessionUpdate:'agent_message_chunk',content:{type:'text',text:JSON.stringify(['QWEN_SERVER_TOKEN','QWEN_DAEMON_TOKEN','QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN','QWEN_CODE_PRIVATE_ACP_CAPABILITY'].map(k => process.env[k] ?? null))}}); + return {stopReason:'end_turn'}; + } + await send({sessionUpdate:'agent_thought_chunk',content:{type:'text',text:'thinking'}}); + await send({sessionUpdate:'agent_message_chunk',content:{type:'text',text:params.prompt.map(p=>p.text).join('|')}}); + await send({sessionUpdate:'tool_call',toolCallId:'tool-'+prompts,title:'Shell',kind:'execute',status:'in_progress',rawInput:{}}); + await send({sessionUpdate:'tool_call_update',toolCallId:'tool-'+prompts,rawInput:{command:'printf hello'},_meta:{claudeCode:{toolName:'Bash'}},content:[{type:'content',content:{type:'text',text:'hello'}}]}); + await send({sessionUpdate:'tool_call_update',toolCallId:'tool-'+prompts,status:'completed'}); + await send({sessionUpdate:'tool_call_update',toolCallId:'tool-'+prompts,status:'completed'}); + return {stopReason:scenario === 'continuation-refusal' && prompts > 1 ? 'refusal' : scenario === 'max-tokens' ? 'max_tokens' : scenario === 'unknown-stop' ? 'future_reason' : 'end_turn'}; + }, +}), ndJsonStream(Writable.toWeb(process.stdout),Readable.toWeb(process.stdin))); +setInterval(()=>{},1000); +`; + +const executors: SubagentExecutor[] = []; +afterEach(async () => { + await Promise.all( + executors.splice(0).map((executor) => executor.dispose?.()), + ); + vi.restoreAllMocks(); + vi.unstubAllEnvs(); +}); + +function params(scenario = 'normal'): ExternalAgentExecutorParams { + const runtimeContext = new Config({ + sessionId: 'fixture-test', + targetDir: process.cwd(), + cwd: process.cwd(), + model: 'unused', + debugMode: false, + }); + vi.spyOn(runtimeContext, 'isInteractive').mockReturnValue(true); + return { + spec: { + kind: 'acp', + command: process.execPath, + args: ['--input-type=module', '-e', fixture, scenario], + }, + name: 'fixture', + promptConfig: { systemPrompt: '' }, + modelConfig: { model: 'unused' }, + runConfig: {}, + runtimeContext, + eventEmitter: new AgentEventEmitter(), + }; +} + +async function create( + options: ExternalAgentExecutorParams, +): Promise { + const executor = await acpExternalAgentExecutor.create(options); + executors.push(executor); + return executor; +} + +function context(text = 'task'): ContextState { + const state = new ContextState(); + state.set('task_prompt', text); + return state; +} + +describe('permission mapping', () => { + it('honors loader approval precedence and fails unknown modes towards asking', () => { + expect(resolvePermissionMode('bypassPermissions', 'plan')).toBe('plan'); + expect(resolvePermissionMode('auto', 'default')).toBe('default'); + expect(resolvePermissionMode('auto', 'auto-edit')).toBe('acceptEdits'); + expect(resolvePermissionMode(undefined, undefined)).toBe('default'); + expect(resolvePermissionMode('unknown', undefined)).toBe('default'); + }); + it('never grants for non-approval outcomes or widens a single approval', () => { + expect(optionKindForOutcome(ToolConfirmationOutcome.RestorePrevious)).toBe( + 'reject_once', + ); + expect(optionKindForOutcome(ToolConfirmationOutcome.ModifyWithEditor)).toBe( + 'reject_once', + ); + const options = [ + { optionId: 'always', kind: 'allow_always' }, + { optionId: 'no', kind: 'reject_once' }, + ]; + expect( + selectPermissionOption(options, ToolConfirmationOutcome.ProceedOnce), + ).toBe('no'); + expect( + selectPermissionOption(options, ToolConfirmationOutcome.Cancel), + ).toBeUndefined(); + expect( + selectPermissionOption( + [{ optionId: 'once', kind: 'allow_once' }], + ToolConfirmationOutcome.ProceedAlways, + ), + ).toBe('once'); + expect( + selectPermissionOption( + [ + { optionId: 'same', kind: 'allow_once' }, + { optionId: 'same', kind: 'reject_once' }, + ], + ToolConfirmationOutcome.ProceedOnce, + ), + ).toBeUndefined(); + }); + it('uses an external model label', () => { + expect(externalModelLabel('/usr/bin/claude')).toBe('external-acp:claude'); + expect(externalModelLabel('C:\\tools\\claude')).toBe('external-acp:claude'); + }); + it('selects the narrowest reject option and refuses to guess', () => { + const full = [ + { optionId: 'once', kind: 'allow_once' }, + { optionId: 'always', kind: 'allow_always' }, + { optionId: 'no', kind: 'reject_once' }, + ]; + // Prefers reject_once over reject_always (deny this call, not the session). + expect(selectRejectOption(full)).toBe('no'); + expect( + selectRejectOption([ + { optionId: 'never', kind: 'reject_always' }, + { optionId: 'once', kind: 'allow_once' }, + ]), + ).toBe('never'); + // No reject option at all → undefined, so the caller falls back to cancel + // rather than selecting an allow. + expect( + selectRejectOption([ + { optionId: 'once', kind: 'allow_once' }, + { optionId: 'always', kind: 'allow_always' }, + ]), + ).toBeUndefined(); + expect(selectRejectOption([])).toBeUndefined(); + // Ambiguous duplicate IDs → refuse to guess. + expect( + selectRejectOption([ + { optionId: 'same', kind: 'reject_once' }, + { optionId: 'same', kind: 'allow_once' }, + ]), + ).toBeUndefined(); + }); +}); + +describe('cleanup-exit classification', () => { + it('treats a foreign root that died during disposal as expected', () => { + // The Linux race: the prompt rejects on stdout EOF, dispose() enters the + // registry's initial tree snapshot, and the root exits mid-snapshot. + expect( + isExpectedExternalAgentCleanupExit( + new Error( + 'ACP child pid=2628870 exited before its initial process-tree snapshot completed', + ), + ), + ).toBe(true); + // The peer's own exit status, for ANY code/signal: acp-bridge raises this + // only after the tree is proven gone, so it reports the foreign agent's + // exit, not a cleanup failure (R7-2). Letting dispose() reject on it would + // override the turn's classified terminal state. + for (const status of [ + 'code=none, signal=SIGTERM', + 'code=none, signal=SIGKILL', + 'code=5, signal=none', + 'code=1, signal=none', + 'code=none, signal=SIGHUP', + 'code=143, signal=none', + ]) { + expect( + isExpectedExternalAgentCleanupExit( + new Error( + `ACP child pid=4242 exited uncleanly during shutdown (${status})`, + ), + ), + ).toBe(true); + } + }); + it('propagates every genuine cleanup-proof failure', () => { + const mustThrow = [ + // These are raised when the tree could NOT be proven gone — a descendant + // we owned may have survived, so they must never be swallowed. + 'ACP child pid=4242 process-tree snapshot exceeded 512 processes or depth 8', + 'ACP child pid=4242 was absent from the initial process-tree snapshot', + 'ACP child pid=4242 was not an isolated process-group leader', + 'ACP child pid=4242 process-tree snapshot failed: ps exited 1', + 'ACP child pid=4242 could not send SIGKILL to pgid=4242: EPERM', + 'ACP child pid=4242 could not inspect pgid=999: EACCES', + 'ACP child pid=4242 did not exit within 5000ms', + ]; + for (const message of mustThrow) { + expect(isExpectedExternalAgentCleanupExit(new Error(message))).toBe( + false, + ); + } + }); + it('rejects non-Error and near-miss values', () => { + expect(isExpectedExternalAgentCleanupExit(undefined)).toBe(false); + expect(isExpectedExternalAgentCleanupExit(null)).toBe(false); + expect( + isExpectedExternalAgentCleanupExit( + 'ACP child pid=1 exited before its initial process-tree snapshot completed', + ), + ).toBe(false); + expect( + isExpectedExternalAgentCleanupExit( + new Error( + 'prefix ACP child pid=1 exited before its initial process-tree snapshot completed', + ), + ), + ).toBe(false); + expect( + isExpectedExternalAgentCleanupExit( + new Error( + 'ACP child pid=notanumber exited before its initial process-tree snapshot completed', + ), + ), + ).toBe(false); + }); +}); + +// Real subprocess + real ProcessRegistry tree-kill. On Windows the registry +// kills via taskkill/SIGKILL and Node reports a numeric exit code with +// signalCode null, which dispose() surfaces as an unclean-exit error — the same +// exposure the repo's other real-process suites gate (process-registry.process, +// hook-runner.process). The pure-function suites above run on every platform. +describe.skipIf(process.platform === 'win32')('real ACP subprocess', () => { + it('returns method-not-found -32601 across the real extension request wire', async () => { + const executor = await create(params('unsupported-extension')); + await executor.execute(context()); + expect(JSON.parse(executor.getFinalText())).toMatchObject({ code: -32601 }); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.GOAL); + }); + it('sets an advertised asking mode before prompt and preserves text/thought/tool transcript events', async () => { + const options = params(); + const text = vi.fn(); + const stream = vi.fn(); + const results = vi.fn(); + options.eventEmitter!.on(AgentEventType.ROUND_TEXT, text); + options.eventEmitter!.on(AgentEventType.STREAM_TEXT, stream); + options.eventEmitter!.on(AgentEventType.TOOL_RESPONSES_FINALIZED, results); + const executor = await create(options); + await executor.execute(context()); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.GOAL); + expect(text).toHaveBeenCalledWith( + expect.objectContaining({ text: 'task', thoughtText: 'thinking' }), + ); + expect(stream).toHaveBeenCalledWith( + expect.objectContaining({ text: 'thinking', thought: true }), + ); + expect(results).toHaveBeenCalledTimes(1); + expect(JSON.stringify(results.mock.calls)).toContain('hello'); + expect( + results.mock.calls[0]![0].responses[0].responseParts[0].functionResponse, + ).toMatchObject({ + name: 'execute', + response: { input: { command: 'printf hello' }, toolName: 'Bash' }, + }); + expect(executor.getExecutionSummary()).toMatchObject({ + totalToolCalls: 1, + successfulToolCalls: 1, + rounds: 1, + }); + const duration = executor.getExecutionSummary().totalDurationMs; + await new Promise((resolve) => setTimeout(resolve, 25)); + expect(executor.getExecutionSummary().totalDurationMs).toBe(duration); + }); + it.each(['no-mode', 'mode-error', 'init-exit', 'session-exit'])( + 'fails creation for %s', + async (scenario) => { + await expect(create(params(scenario))).rejects.toThrow(); + }, + ); + it.each(['init-hang', 'session-hang', 'mode-hang'])( + 'bounds and reaps a %s handshake', + async (scenario) => { + await expect(create(params(scenario))).rejects.toThrow('timed out'); + }, + 15000, + ); + it.each(['prompt-exit', 'prompt-close'])( + 'rejects %s without an error listener', + async (scenario) => { + const executor = await create(params(scenario)); + await expect(executor.execute(context())).rejects.toThrow(); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.ERROR); + }, + ); + it.each(['max-tokens', 'unknown-stop'])( + 'never certifies %s as success', + async (scenario) => { + const executor = await create(params(scenario)); + await executor.execute(context()); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.ERROR); + }, + ); + it('preserves both external-input variants and updates continuation lifecycle and stats', async () => { + const options = params('continuation-refusal'); + const finish = vi.fn(); + const external = vi.fn(); + options.eventEmitter!.on(AgentEventType.FINISH, finish); + options.eventEmitter!.on(AgentEventType.EXTERNAL_MESSAGE, external); + const executor = await create(options); + await executor.execute(context('first')); + await executor.executeExternalInputs( + ['second', { kind: 'notification', text: 'notice' }], + undefined, + { resetStats: false }, + ); + expect(executor.getFinalText()).toBe('second|notice'); + // The continuation peer returned ACP `refusal` — the agent declined, which + // is a failure the parent must see, not a user cancel (R6-2). + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.ERROR); + expect(executor.getExecutionSummary()).toMatchObject({ + rounds: 2, + totalToolCalls: 2, + }); + expect(finish).toHaveBeenCalledTimes(2); + expect(external).toHaveBeenCalledTimes(2); + await executor.execute(context('fresh')); + expect(executor.getExecutionSummary()).toMatchObject({ + rounds: 1, + totalToolCalls: 1, + }); + }); + it('records the user-side transcript entry on a continuation execute() turn', async () => { + const options = params(); + const external = vi.fn(); + options.eventEmitter!.on(AgentEventType.EXTERNAL_MESSAGE, external); + const executor = await create(options); + await executor.execute(context('first')); + // The first turn's task is seeded into the transcript as the initial user + // prompt by the dispatcher, so execute() must not double-record it. + expect(external).not.toHaveBeenCalled(); + await executor.execute(context('second')); + // A resident external agent re-invoked per incoming user message must + // record each continuation task, or the JSONL transcript loses every + // message after the first and round 2 shows an answer to an unasked + // question (R6-1). The in-process sibling emits the same event. + expect(external).toHaveBeenCalledTimes(1); + expect(external).toHaveBeenCalledWith( + expect.objectContaining({ kind: 'message', text: 'second' }), + ); + }); + it('flushes a tool call left open when the turn is cancelled', async () => { + const options = params('tool-open-hang'); + const results = vi.fn(); + const finalized = vi.fn(); + options.eventEmitter!.on(AgentEventType.TOOL_RESULT, results); + options.eventEmitter!.on( + AgentEventType.TOOL_RESPONSES_FINALIZED, + finalized, + ); + const abort = new AbortController(); + // Cancel once the peer's in-progress tool_call has been recorded, so the + // turn ends with that tool still open (R7-3). + options.eventEmitter!.on(AgentEventType.TOOL_CALL, () => abort.abort()); + const executor = await create(options); + await executor.execute(context(), abort.signal); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.CANCELLED); + // The open tool gets a terminal failed TOOL_RESULT + a finalized response, + // so the inline frame / Web Shell row stop showing it as executing, the + // JSONL functionCall is paired, and the FINISH totals sum. + expect(results).toHaveBeenCalledWith( + expect.objectContaining({ callId: 'open-1', success: false }), + ); + expect(finalized).toHaveBeenCalledWith( + expect.objectContaining({ + responses: [ + expect.objectContaining({ + callId: 'open-1', + responseParts: [ + expect.objectContaining({ + functionResponse: expect.objectContaining({ id: 'open-1' }), + }), + ], + }), + ], + }), + ); + const summary = executor.getExecutionSummary(); + expect(summary.totalToolCalls).toBe(1); + expect(summary.failedToolCalls).toBe(1); + expect(summary.successfulToolCalls).toBe(0); + }); + it('drains registered external messages before reporting completion', async () => { + const executor = await create(params()); + const provider = vi + .fn() + .mockReturnValueOnce(['queued']) + .mockReturnValue([]); + executor.setExternalMessageProvider(provider); + await executor.execute(context('first')); + expect(executor.getFinalText()).toContain('queued'); + expect(executor.getExecutionSummary().rounds).toBe(2); + }); + it('does not dispatch a continuation prompt once the wall-time budget is spent (R9-1)', async () => { + const executor = await create(params()); + // Stub the connection so prompt() is countable and resolves end_turn + // immediately; the first prompt also advances the mocked clock past the + // 10-minute budget, simulating a peer that consumed the whole wall time on + // round 1. A message is queued, so without a budget check the loop would + // dispatch round 2 with `remaining` clamped to 0 — billing a prompt it then + // abandons (wait's 0ms timer rejects with ExternalAgentTimeoutError) and + // killing the peer in dispose(), after emitInputs already recorded the + // queued message as delivered. The guard must stop before that dispatch. + const realNow = Date.now(); + let mockNow = realNow; + const nowSpy = vi.spyOn(Date, 'now').mockImplementation(() => mockNow); + const prompt = vi.fn().mockImplementation(() => { + mockNow = realNow + 11 * 60_000; + return Promise.resolve({ stopReason: 'end_turn' }); + }); + ( + executor as unknown as { + connection: { prompt: typeof prompt; cancel: () => Promise }; + } + ).connection = { prompt, cancel: async () => {} }; + executor.setExternalMessageProvider( + vi.fn().mockReturnValueOnce(['queued']).mockReturnValue([]), + ); + await executor.execute(context('first')); + // Removing the budget guard turns both red: round 2 dispatches (prompt is + // called twice) and resolves end_turn against the empty queue, so the turn + // ends GOAL instead of TIMEOUT. + expect(prompt).toHaveBeenCalledTimes(1); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.TIMEOUT); + nowSpy.mockRestore(); + }); + it('does not send a prompt for an already-aborted turn', async () => { + const executor = await create(params('prompt-exit')); + await executor.execute(context(), AbortSignal.abort()); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.CANCELLED); + expect(executor.getFinalText()).toBe(''); + }); + it('cancels and reaps an uncooperative continuation', async () => { + const executor = await create(params('prompt-hang')); + const abort = new AbortController(); + const running = executor.executeExternalInputs(['continue'], abort.signal); + setTimeout(() => abort.abort(), 50); + await running; + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.CANCELLED); + }); + it('enforces the configured wall time', async () => { + const options = params('prompt-hang'); + options.runConfig.max_time_minutes = 0.001; + const executor = await create(options); + await executor.execute(context()); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.TIMEOUT); + }); + it('drops session updates that arrive after a timeout declared the turn over', async () => { + const options = params('late-after-timeout'); + options.runConfig.max_time_minutes = 0.002; // 120ms budget + const executor = await create(options); + await executor.execute(context()); + // The turn times out at ~120ms; the SIGTERM-resistant peer then emits a + // chunk and a completing tool_call at ~400ms, during dispose's grace window + // while `executing` is still true and `cancelled` is false (the timeout path + // never sets it). The widened guard must drop them, so the late text never + // reaches finalText (the TIMEOUT path hands it to the parent verbatim) and + // the late tool never inflates the FINISH totals (R8-1). + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.TIMEOUT); + expect(executor.getFinalText()).not.toContain('LATE_AFTER_TIMEOUT'); + expect(executor.getExecutionSummary().totalToolCalls).toBe(0); + expect(executor.getExecutionSummary().successfulToolCalls).toBe(0); + }, 15000); + it('installs a ten-minute default deadline and clears it after the real prompt', async () => { + const executor = await create(params()); + const timers = vi.spyOn(globalThis, 'setTimeout'); + const cleared = vi.spyOn(globalThis, 'clearTimeout'); + await executor.execute(context()); + const index = timers.mock.calls.findIndex( + ([, delay]) => + typeof delay === 'number' && delay > 599_000 && delay <= 600_000, + ); + expect(index).toBeGreaterThanOrEqual(0); + expect(cleared).toHaveBeenCalledWith(timers.mock.results[index]!.value); + }); + it('refuses an unenforceable internal turn limit', async () => { + const options = params(); + options.runConfig.max_turns = 1; + await expect(create(options)).rejects.toThrow('max_turns'); + }); + it.each([NaN, Infinity, -1, 0, 100_000])( + 'rejects invalid timeout %s before spawning', + async (minutes) => { + const options = params(); + options.spec.command = '/nonexistent/external-agent'; + options.runConfig.max_time_minutes = minutes; + await expect(create(options)).rejects.toThrow('max_time_minutes'); + }, + ); + it('cleans up when a START subscriber throws', async () => { + const options = params(); + options.eventEmitter!.on(AgentEventType.START, () => { + throw new Error('start listener failed'); + }); + const executor = await create(options); + await expect(executor.execute(context())).rejects.toThrow( + 'start listener failed', + ); + await expect(executor.execute(context())).rejects.toThrow('closed'); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.ERROR); + }); + it.each(['permission-question', 'permission-question-snake'])( + 'denies routed interactive question %s', + async (scenario) => { + const options = params(scenario); + const approval = vi.fn(); + options.eventEmitter!.on(AgentEventType.TOOL_WAITING_APPROVAL, approval); + const executor = await create(options); + await executor.execute(context()); + // Denies the TOOL (selects reject_once), not the TURN (`cancelled`). + expect(JSON.parse(executor.getFinalText())).toEqual([ + { outcome: { outcome: 'selected', optionId: 'no' } }, + ]); + expect(approval).not.toHaveBeenCalled(); + }, + ); + it('denies when no approval listener exists', async () => { + const executor = await create(params('permission')); + await executor.execute(context()); + expect(JSON.parse(executor.getFinalText())).toEqual([ + { outcome: { outcome: 'selected', optionId: 'no' } }, + ]); + }); + it('denies the TOOL, not the TURN, when the user explicitly cancels (R8-3)', async () => { + const options = params('permission'); + options.eventEmitter!.on( + AgentEventType.TOOL_WAITING_APPROVAL, + async (event) => event.respond(ToolConfirmationOutcome.Cancel), + ); + const executor = await create(options); + await executor.execute(context()); + // An explicit user rejection of this one tool selects the peer's + // reject_once option, so the peer continues and ends the turn normally. + // Routing Cancel to ACP `cancelled` instead would abort the whole delegated + // turn (the peer answers stopReason `cancelled` → CANCELLED), abandoning the + // remaining work and billing a fresh prompt to re-delegate. + expect(JSON.parse(executor.getFinalText())).toEqual([ + { outcome: { outcome: 'selected', optionId: 'no' } }, + ]); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.GOAL); + }); + it('denies when the runtime disallows permission prompts even with a listener', async () => { + const options = params('permission'); + vi.spyOn( + options.runtimeContext, + 'getShouldAvoidPermissionPrompts', + ).mockReturnValue(true); + const approval = vi.fn(); + options.eventEmitter!.on(AgentEventType.TOOL_WAITING_APPROVAL, approval); + const executor = await create(options); + await executor.execute(context()); + expect(JSON.parse(executor.getFinalText())).toEqual([ + { outcome: { outcome: 'selected', optionId: 'no' } }, + ]); + expect(approval).not.toHaveBeenCalled(); + }); + it('denies plain headless requests despite a display-only approval listener', async () => { + const options = params('permission'); + vi.spyOn(options.runtimeContext, 'isInteractive').mockReturnValue(false); + vi.spyOn( + options.runtimeContext, + 'getExperimentalZedIntegration', + ).mockReturnValue(false); + vi.spyOn(options.runtimeContext, 'getInputFormat').mockReturnValue( + InputFormat.TEXT, + ); + vi.spyOn( + options.runtimeContext, + 'getShouldAvoidPermissionPrompts', + ).mockReturnValue(false); + const displayOnly = vi.fn(); + options.eventEmitter!.on(AgentEventType.TOOL_WAITING_APPROVAL, displayOnly); + const executor = await create(options); + await executor.execute(context()); + expect(JSON.parse(executor.getFinalText())).toEqual([ + { outcome: { outcome: 'selected', optionId: 'no' } }, + ]); + expect(displayOnly).not.toHaveBeenCalled(); + }); + it('allows stream-json approval responders', async () => { + const options = params('permission'); + vi.spyOn(options.runtimeContext, 'isInteractive').mockReturnValue(false); + vi.spyOn(options.runtimeContext, 'getInputFormat').mockReturnValue( + InputFormat.STREAM_JSON, + ); + options.eventEmitter!.on( + AgentEventType.TOOL_WAITING_APPROVAL, + async (event) => event.respond(ToolConfirmationOutcome.ProceedOnce), + ); + const executor = await create(options); + await executor.execute(context()); + expect(executor.getFinalText()).toContain('"optionId":"once"'); + }); + it('isolates duplicate outstanding tool IDs and ignores stale approval callbacks', async () => { + const options = params('permission-duplicate'); + const callbacks: Array<() => Promise> = []; + options.eventEmitter!.on(AgentEventType.TOOL_WAITING_APPROVAL, (event) => { + callbacks.push(() => event.respond(ToolConfirmationOutcome.ProceedOnce)); + setTimeout( + () => void event.respond(ToolConfirmationOutcome.ProceedOnce), + 30, + ); + }); + const executor = await create(options); + await executor.execute(context()); + expect(JSON.parse(executor.getFinalText())).toEqual([ + { outcome: { outcome: 'selected', optionId: 'once' } }, + { outcome: { outcome: 'cancelled' } }, + ]); + await callbacks[0]!(); + }); + it('releases pending permission requests on cancellation', async () => { + const options = params('permission'); + const abort = new AbortController(); + options.eventEmitter!.on(AgentEventType.TOOL_WAITING_APPROVAL, () => + abort.abort(), + ); + const executor = await create(options); + await executor.execute(context(), abort.signal); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.CANCELLED); + }); + it('scrubs all internal credentials in the real child environment', async () => { + for (const name of [ + 'QWEN_SERVER_TOKEN', + 'QWEN_DAEMON_TOKEN', + 'QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN', + 'QWEN_CODE_PRIVATE_ACP_CAPABILITY', + ]) + vi.stubEnv(name, 'secret'); + const executor = await create(params('env')); + await executor.execute(context()); + expect(JSON.parse(executor.getFinalText())).toEqual([ + null, + null, + null, + null, + ]); + }); + it('awaits verified disposal of a SIGTERM-resistant descendant', async () => { + const executor = await create(params('tree')); + await executor.execute(context()); + const pid = Number(executor.getFinalText()); + expect(pid).toBeGreaterThan(0); + process.kill(pid, 0); + await executor.dispose?.(); + expect(() => process.kill(pid, 0)).toThrow(); + }, 15000); +}); diff --git a/packages/cli/src/external-agents/acp-subagent-executor.ts b/packages/cli/src/external-agents/acp-subagent-executor.ts new file mode 100644 index 00000000000..54eb3febd37 --- /dev/null +++ b/packages/cli/src/external-agents/acp-subagent-executor.ts @@ -0,0 +1,938 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { spawn } from 'node:child_process'; +import { Readable, Writable } from 'node:stream'; +import { + ClientSideConnection, + ndJsonStream, + PROTOCOL_VERSION, + RequestError, + type Client, + type ContentBlock, + type RequestPermissionRequest, + type RequestPermissionResponse, + type SessionNotification, + type ToolCall, + type ToolCallUpdate, +} from '@agentclientprotocol/sdk'; +import { + AgentEventEmitter, + AgentEventType, + AgentTerminateMode, + ToolConfirmationOutcome, + renderSubagentSystemPrompt, + type AgentExternalInput, + type AgentStatsSummary, + type ContextState, + type ExternalAgentExecutor, + type ExternalAgentExecutorParams, + type SubagentExecutor, + type SubagentExecutorCore, +} from '@qwen-code/qwen-code-core/subagentRuntime'; +import { InputFormat, sanitizeChildEnv } from '@qwen-code/qwen-code-core'; +import { createStderrForwarder } from '@qwen-code/acp-bridge/spawnChannel'; +import { + ProcessRegistry, + type TrackedChildProcess, +} from '@qwen-code/acp-bridge/processRegistry'; + +const INIT_TIMEOUT_MS = 10_000; + +export function externalModelLabel(command: string): string { + return `external-acp:${command.split(/[\\/]/).pop() ?? command}`; +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +export function resolvePermissionMode( + permissionMode: string | undefined, + approvalMode: string | undefined, +): string { + switch ((approvalMode ?? permissionMode ?? '').trim().toLowerCase()) { + case 'auto': + case 'yolo': + return 'auto'; + case 'acceptedits': + case 'auto-edit': + return 'acceptEdits'; + case 'plan': + return 'plan'; + case 'bypasspermissions': + case 'bypass': + return 'bypassPermissions'; + default: + return 'default'; + } +} + +export function optionKindForOutcome( + outcome: ToolConfirmationOutcome, +): 'allow_once' | 'allow_always' | 'reject_once' { + switch (outcome) { + case ToolConfirmationOutcome.ProceedOnce: + case ToolConfirmationOutcome.ProceedOnceAndSwitchToDefault: + return 'allow_once'; + case ToolConfirmationOutcome.ProceedAlways: + case ToolConfirmationOutcome.ProceedAlwaysServer: + case ToolConfirmationOutcome.ProceedAlwaysTool: + case ToolConfirmationOutcome.ProceedAlwaysProject: + case ToolConfirmationOutcome.ProceedAlwaysUser: + return 'allow_always'; + case ToolConfirmationOutcome.ModifyWithEditor: + case ToolConfirmationOutcome.Cancel: + case ToolConfirmationOutcome.RestorePrevious: + return 'reject_once'; + default: { + const exhaustive: never = outcome; + void exhaustive; + return 'reject_once'; + } + } +} + +export function selectPermissionOption( + options: ReadonlyArray<{ optionId: string; kind: unknown }>, + outcome: ToolConfirmationOutcome, +): string | undefined { + if (outcome === ToolConfirmationOutcome.Cancel) return undefined; + // An ambiguous option ID could turn a selected rejection into a grant. + if ( + new Set(options.map((option) => option.optionId)).size !== options.length + ) { + return undefined; + } + const kind = optionKindForOutcome(outcome); + return ( + options.find((option) => option.kind === kind) ?? + (kind === 'allow_always' + ? options.find((option) => option.kind === 'allow_once') + : undefined) ?? + options.find((option) => option.kind === 'reject_once') ?? + options.find((option) => option.kind === 'reject_always') + )?.optionId; +} + +/** + * Picks the option that denies a single tool call without ending the turn. A + * host-policy denial (no responder, headless, permission-avoidance, a routed + * interactive question) must reject the TOOL, not cancel the TURN: ACP defines + * the `cancelled` outcome as "the prompt turn was cancelled", so answering a + * policy denial with it would abort the whole delegation on its first sensitive + * tool instead of letting the peer continue without that one action — which is + * what the in-process auto-deny path does. Prefers `reject_once` (deny this + * call) over `reject_always` (deny for the session) to keep the scope narrow, + * and returns undefined when no reject option is safely selectable so the caller + * falls back to `cancelled` rather than guessing. + */ +export function selectRejectOption( + options: ReadonlyArray<{ optionId: string; kind: unknown }>, +): string | undefined { + // An ambiguous option ID could turn a denial into the wrong selection. + if ( + new Set(options.map((option) => option.optionId)).size !== options.length + ) { + return undefined; + } + return ( + options.find((option) => option.kind === 'reject_once') ?? + options.find((option) => option.kind === 'reject_always') + )?.optionId; +} + +/** + * Errors the ACP process registry raises when disposing a foreign child whose + * process tree is already gone. acp-bridge raises both shapes only AFTER it has + * driven every owned process group to empty, so neither is a cleanup failure — + * they report that a foreign agent we do not own exited, which `terminateMode` + * has already classified. Two shapes are expected here: + * + * - the root exited on its own during the initial tree snapshot, invalidating + * the snapshot that would enumerate descendants (a Linux-only race where the + * prompt rejects on stdout EOF before the process `exit` event); and + * - `exited uncleanly during shutdown (code=…, signal=…)` for ANY code/signal — + * the peer's own exit status (a non-zero code from an uncaught exception, an + * OOM kill, a wrapper turning SIGTERM into 143, or the signal we sent). This + * is thrown only once `survivingGroups` is empty, i.e. cleanup succeeded. + * + * Tolerating the peer's exit status matters: `dispose()` is awaited inside + * `runTurn`'s catch between error classification and the return, so letting it + * reject here would replace the turn's declared terminal state — a turn the user + * CANCELLED would surface to the parent as `failed` with the registry's text. + * + * Every genuine cleanup-PROOF error still propagates — a truncated snapshot, a + * root that was absent or was not an isolated process-group leader, a failed + * snapshot/signal/inspect, or an exit deadline exceeded — so a descendant we were + * responsible for cannot survive disposal silently. + */ +export function isExpectedExternalAgentCleanupExit(error: unknown): boolean { + if (!(error instanceof Error)) return false; + return ( + /^ACP child pid=\d+ exited before its initial process-tree snapshot completed$/.test( + error.message, + ) || + /^ACP child pid=\d+ exited uncleanly during shutdown \(code=[^,]+, signal=[^)]+\)$/.test( + error.message, + ) + ); +} + +/** + * Thrown by `wait()` when an operation exceeds its deadline. A dedicated type + * lets `runTurn` classify a wall-time expiry by identity instead of re-deriving + * it from a second `Date.now()` read: the timer fires on libuv's monotonic clock + * against a `remaining` budget computed earlier, so comparing realtime elapsed + * with a 1ms-exact `>=` misclassifies a turn that merely reached its budget as + * ERROR whenever the timer wins by under a millisecond. + */ +class ExternalAgentTimeoutError extends Error { + constructor(timeoutMs: number) { + super(`External ACP operation timed out after ${timeoutMs}ms`); + this.name = 'ExternalAgentTimeoutError'; + } +} + +class AcpSubagentExecutor implements SubagentExecutor { + private connection!: ClientSideConnection; + private sessionId = ''; + private finalText = ''; + private thoughtText = ''; + private terminateMode = AgentTerminateMode.ERROR; + private round = 0; + private durationMs = 0; + private turnStartedAt = 0; + private toolCalls = 0; + private toolSucceeded = 0; + private toolFailed = 0; + private executing = false; + private started = false; + private disposed = false; + private cancelled = false; + private disposePromise?: Promise; + private provider?: () => AgentExternalInput[]; + private readonly pendingPermissions = new Map void>(); + private readonly tools = new Map(); + private readonly toolNames = new Map(); + private readonly finishedTools = new Set(); + private readonly failure = new AbortController(); + private readonly coreView: SubagentExecutorCore; + + private constructor( + private readonly params: ExternalAgentExecutorParams, + private readonly child: TrackedChildProcess, + private readonly emitter: AgentEventEmitter, + ) { + this.coreView = { + getEventEmitter: () => emitter, + modelConfig: { + ...params.modelConfig, + model: externalModelLabel(params.spec.command), + }, + }; + void child.exited.then( + () => this.fail(new Error(`external agent "${params.name}" exited`)), + (error: unknown) => this.fail(error), + ); + } + + static async create( + params: ExternalAgentExecutorParams, + ): Promise { + if (params.runConfig.max_turns !== undefined) { + throw new Error('External ACP agents cannot enforce max_turns.'); + } + const minutes = params.runConfig.max_time_minutes; + if ( + minutes !== undefined && + (!Number.isFinite(minutes) || + minutes <= 0 || + minutes * 60_000 > 2_147_483_647) + ) { + throw new Error( + 'External ACP max_time_minutes must be positive, finite, and within the Node timer range.', + ); + } + const child = spawn(params.spec.command, params.spec.args ?? [], { + cwd: params.runtimeContext.getTargetDir(), + stdio: ['pipe', 'pipe', 'pipe'], + detached: process.platform !== 'win32', + windowsHide: true, + env: sanitizeChildEnv(process.env), + }); + const tracked = new ProcessRegistry() + .reserve() + .attach(child, { ownsProcessTree: true }); + const executor = new AcpSubagentExecutor( + params, + tracked, + params.eventEmitter ?? new AgentEventEmitter(), + ); + child.on('error', (error) => executor.fail(error)); + child.stdin!.on('error', (error) => executor.fail(error)); + child.stdout!.on('error', (error) => executor.fail(error)); + const forwarder = createStderrForwarder({ + prefix: `[external-agent ${params.name}] `, + }); + child.stderr!.setEncoding('utf8'); + child.stderr!.on('data', forwarder.onData); + child.stderr!.on('end', forwarder.onEnd); + child.stderr!.on('error', () => {}); + try { + executor.connection = new ClientSideConnection( + () => executor.buildClient(), + ndJsonStream( + Writable.toWeb(child.stdin!) as WritableStream, + Readable.toWeb(child.stdout!) as ReadableStream, + ), + ); + void executor.connection.closed.then(() => + executor.fail(new Error('External ACP connection closed')), + ); + await executor.wait(executor.connect(), INIT_TIMEOUT_MS); + return executor; + } catch (error) { + await executor.dispose(); + throw error; + } + } + + private buildClient(): Client { + return { + sessionUpdate: async (params) => this.onSessionUpdate(params), + requestPermission: (params) => this.onRequestPermission(params), + readTextFile: async () => { + throw RequestError.methodNotFound('fs/read_text_file'); + }, + writeTextFile: async () => { + throw RequestError.methodNotFound('fs/write_text_file'); + }, + extMethod: async (method) => { + throw RequestError.methodNotFound(method); + }, + extNotification: async () => {}, + }; + } + + private async connect(): Promise { + const initialized = await this.connection.initialize({ + protocolVersion: PROTOCOL_VERSION, + clientCapabilities: { fs: { readTextFile: false, writeTextFile: false } }, + }); + if (initialized.protocolVersion !== PROTOCOL_VERSION) { + throw new Error('Unsupported external ACP protocol version'); + } + const session = await this.connection.newSession({ + cwd: this.params.runtimeContext.getTargetDir(), + mcpServers: [], + }); + if (!session.sessionId) + throw new Error('External ACP agent returned no sessionId'); + this.sessionId = session.sessionId; + const modeId = resolvePermissionMode( + this.params.permissionMode, + this.params.approvalMode, + ); + if ( + !session.modes?.availableModes.some( + (mode: { id: string }) => mode.id === modeId, + ) + ) { + throw new Error( + `External ACP agent does not advertise permission mode ${modeId}`, + ); + } + await this.connection.setSessionMode({ sessionId: this.sessionId, modeId }); + } + + private fail(error: unknown): void { + this.failure.abort(error); + this.drainPermissions(); + } + + private async wait( + operation: Promise, + timeoutMs?: number, + signal?: AbortSignal, + ): Promise { + let timer: ReturnType | undefined; + let onFailure = () => {}; + let onAbort = () => {}; + const stopped = new Promise((_resolve, reject) => { + onFailure = () => reject(this.failure.signal.reason); + onAbort = () => + reject(signal?.reason ?? new Error('External agent cancelled')); + this.failure.signal.addEventListener('abort', onFailure, { once: true }); + signal?.addEventListener('abort', onAbort, { once: true }); + if (this.failure.signal.aborted) onFailure(); + if (signal?.aborted) onAbort(); + if (timeoutMs !== undefined) { + timer = setTimeout( + () => reject(new ExternalAgentTimeoutError(timeoutMs)), + timeoutMs, + ); + } + }); + try { + return await Promise.race([operation, stopped]); + } finally { + clearTimeout(timer); + this.failure.signal.removeEventListener('abort', onFailure); + signal?.removeEventListener('abort', onAbort); + } + } + + async execute( + context: ContextState, + signal?: AbortSignal, + options: { resetStats?: boolean } = {}, + ): Promise { + if (context.get('initial_messages_override') !== undefined) { + throw new Error( + 'External ACP agents cannot import in-process conversation history', + ); + } + const system = renderSubagentSystemPrompt( + this.params.promptConfig, + context, + this.params.runtimeContext, + ); + const task = String(context.get('task_prompt') ?? 'Get Started!'); + // The first turn's task is seeded into the transcript as the initial user + // prompt by the dispatcher; a continuation turn (a resident external agent + // re-invoked per incoming user message) has no such seed, so emit the task + // as a user-side record here or the JSONL transcript loses every message + // after the first — the in-process sibling emits the same EXTERNAL_MESSAGE + // on continuation. + const continuation = this.started; + await this.runTurn( + [ + ...(system ? [{ type: 'text' as const, text: system }] : []), + { type: 'text', text: task }, + ], + signal, + options, + continuation ? [task] : undefined, + ); + } + + async executeExternalInputs( + inputs: AgentExternalInput[], + signal?: AbortSignal, + options: { resetStats?: boolean } = {}, + ): Promise { + if (inputs.length === 0) return; + await this.runTurn(this.inputPrompt(inputs), signal, options, inputs); + } + + private inputPrompt(inputs: AgentExternalInput[]): ContentBlock[] { + return inputs.map((input) => ({ + type: 'text', + text: typeof input === 'string' ? input : input.text, + })); + } + + private emitInputs(inputs: AgentExternalInput[]): void { + for (const input of inputs) { + this.emitter.emit(AgentEventType.EXTERNAL_MESSAGE, { + subagentId: this.id, + kind: typeof input === 'string' ? 'message' : input.kind, + text: typeof input === 'string' ? input : input.text, + timestamp: Date.now(), + }); + } + } + + private async runTurn( + prompt: ContentBlock[], + signal?: AbortSignal, + options: { resetStats?: boolean } = {}, + inputs?: AgentExternalInput[], + ): Promise { + if (this.executing) + throw new Error('External ACP agents do not support concurrent turns'); + if (this.disposed || this.failure.signal.aborted) + throw new Error('External ACP agent is closed'); + this.executing = true; + this.started = true; + this.cancelled = false; + this.finalText = ''; + this.thoughtText = ''; + this.terminateMode = AgentTerminateMode.ERROR; + if (options.resetStats !== false) { + this.round = + this.durationMs = + this.toolCalls = + this.toolSucceeded = + this.toolFailed = + 0; + } + this.tools.clear(); + this.toolNames.clear(); + this.finishedTools.clear(); + this.turnStartedAt = Date.now(); + // Match DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES without importing the workflow runtime. + const timeoutMs = (this.params.runConfig.max_time_minutes ?? 10) * 60_000; + const cancel = () => { + this.cancelled = true; + this.drainPermissions(); + void this.connection + .cancel({ sessionId: this.sessionId }) + .catch(() => {}); + }; + signal?.addEventListener('abort', cancel, { once: true }); + let timedOut = false; + try { + this.emitter.emit(AgentEventType.START, { + subagentId: this.id, + name: this.params.name, + model: this.coreView.modelConfig.model, + tools: [], + timestamp: Date.now(), + }); + if (signal?.aborted) { + this.terminateMode = AgentTerminateMode.CANCELLED; + return; + } + if (inputs) this.emitInputs(inputs); + let next = prompt; + do { + this.round++; + const remaining = + timeoutMs === undefined + ? undefined + : Math.max(0, timeoutMs - (Date.now() - this.turnStartedAt)); + // Stop before dispatching a round whose budget is already spent. Without + // this, `remaining` clamps to 0 and `connection.prompt(...)` is evaluated + // — a new, billed model turn really reaches the peer — one tick before + // `wait`'s 0ms timer rejects with ExternalAgentTimeoutError; the catch + // then disposes (SIGTERM/SIGKILL) a peer that just started work, and the + // message this round already drained and recorded as delivered is never + // processed. Mirrors the in-process sibling, which checks the budget at + // the top of every round and breaks with TIMEOUT before dispatching. Set + // terminateMode directly rather than throwing: the catch classifies + // wall-time expiry by ExternalAgentTimeoutError identity, so a plain + // Error here would be reclassified ERROR and rethrown to the parent. + if (remaining !== undefined && remaining <= 0) { + this.terminateMode = AgentTerminateMode.TIMEOUT; + break; + } + const result = await this.wait( + this.connection.prompt({ sessionId: this.sessionId, prompt: next }), + remaining, + signal, + ); + this.terminateMode = this.stopMode(result.stopReason); + if (this.terminateMode !== AgentTerminateMode.GOAL) break; + const queued = this.provider?.() ?? []; + if (queued.length === 0) break; + this.emitter.emit(AgentEventType.ROUND_TEXT, { + subagentId: this.id, + round: this.round, + text: this.finalText, + thoughtText: this.thoughtText, + timestamp: Date.now(), + }); + this.finalText = ''; + this.thoughtText = ''; + this.emitInputs(queued); + next = this.inputPrompt(queued); + } while (!signal?.aborted); + if (signal?.aborted) this.terminateMode = AgentTerminateMode.CANCELLED; + } catch (error) { + // Classify the wall-time expiry by the timer's own error identity, not by + // re-reading Date.now(): a second clock can miss the monotonic deadline by + // <1ms and misreport a budget-reaching turn as ERROR. See + // ExternalAgentTimeoutError. + timedOut = error instanceof ExternalAgentTimeoutError; + this.terminateMode = signal?.aborted + ? AgentTerminateMode.CANCELLED + : timedOut + ? AgentTerminateMode.TIMEOUT + : AgentTerminateMode.ERROR; + if ( + this.emitter.rawListeners(AgentEventType.ERROR).length > 0 && + !signal?.aborted + ) { + this.emitter.emit(AgentEventType.ERROR, { + subagentId: this.id, + error: error instanceof Error ? error.message : String(error), + timestamp: Date.now(), + }); + } + await this.dispose(); + if (!signal?.aborted && !timedOut) throw error; + } finally { + signal?.removeEventListener('abort', cancel); + this.durationMs += Date.now() - this.turnStartedAt; + this.executing = false; + this.drainPermissions(); + this.emitter.emit(AgentEventType.ROUND_TEXT, { + subagentId: this.id, + round: this.round, + text: this.finalText, + thoughtText: this.thoughtText, + timestamp: Date.now(), + }); + // Close out any tool call left open by cancel/timeout/crash before FINISH + // reports the totals, so the counts sum and no consumer is left showing a + // tool that never terminated (R7-3). + this.flushOpenTools(); + this.emitter.emit(AgentEventType.FINISH, { + subagentId: this.id, + terminateReason: this.terminateMode, + rounds: this.round, + totalDurationMs: this.durationMs, + totalToolCalls: this.toolCalls, + successfulToolCalls: this.toolSucceeded, + failedToolCalls: this.toolFailed, + timestamp: Date.now(), + }); + } + } + + private stopMode(reason: string): AgentTerminateMode { + switch (reason) { + case 'end_turn': + return AgentTerminateMode.GOAL; + case 'max_turn_requests': + return AgentTerminateMode.MAX_TURNS; + case 'cancelled': + return AgentTerminateMode.CANCELLED; + case 'refusal': + // A peer that refused the task neither achieved the goal nor was + // cancelled by the user. CANCELLED renders as "cancelled by the user" + // — a false statement about the user that also hides the refusal from + // telemetry — so report it as a failure the parent can react to. The + // ACP schema asks that a refusal be reflected rather than relabelled. + return AgentTerminateMode.ERROR; + default: + return AgentTerminateMode.ERROR; + } + } + + private get id(): string { + return this.params.subagentId ?? this.params.name; + } + getFinalText(): string { + return this.finalText; + } + getTerminateMode(): AgentTerminateMode { + return this.terminateMode; + } + getCore(): SubagentExecutorCore { + return this.coreView; + } + setExternalMessageProvider(provider: () => AgentExternalInput[]): void { + this.provider = provider; + } + getExecutionSummary(): AgentStatsSummary { + return { + rounds: this.round, + totalDurationMs: + this.durationMs + + (this.executing ? Date.now() - this.turnStartedAt : 0), + totalToolCalls: this.toolCalls, + successfulToolCalls: this.toolSucceeded, + failedToolCalls: this.toolFailed, + successRate: this.toolCalls + ? (this.toolSucceeded / this.toolCalls) * 100 + : 0, + inputTokens: 0, + outputTokens: 0, + thoughtTokens: 0, + cachedTokens: 0, + totalTokens: 0, + toolUsage: [], + }; + } + + dispose(): Promise { + this.disposed = true; + this.fail(new Error('External ACP agent disposed')); + return (this.disposePromise ??= this.child + .terminate() + .catch((error: unknown) => { + if (!isExpectedExternalAgentCleanupExit(error)) throw error; + })); + } + + private drainPermissions(): void { + for (const cancel of this.pendingPermissions.values()) cancel(); + this.pendingPermissions.clear(); + } + + private onSessionUpdate({ sessionId, update }: SessionNotification): void { + // Drop updates once the turn is over for ANY reason, not just user cancel. + // On timeout or peer crash the catch awaits dispose() before the finally + // clears `executing`, and `cancelled` stays false, so without the + // disposed/aborted checks the peer's still-flowing stdout would be appended + // to finalText, emitted as STREAM_TEXT/ROUND_TEXT and counted into the + // FINISH totals for up to EXIT_DEADLINE_MS after termination — a false + // transcript record and a partial result handed to the parent verbatim on + // the non-rethrowing TIMEOUT path (R8-1). + if ( + sessionId !== this.sessionId || + !this.executing || + this.cancelled || + this.disposed || + this.failure.signal.aborted + ) + return; + switch (update.sessionUpdate) { + case 'agent_message_chunk': + case 'agent_thought_chunk': { + const thought = update.sessionUpdate === 'agent_thought_chunk'; + const text = + update.content.type === 'text' + ? update.content.text + : JSON.stringify(update.content); + if (thought) this.thoughtText += text; + else this.finalText += text; + this.emitter.emit(AgentEventType.STREAM_TEXT, { + subagentId: this.id, + round: this.round, + text, + thought, + timestamp: Date.now(), + }); + break; + } + case 'tool_call': + this.updateTool(update); + break; + case 'tool_call_update': + this.updateTool(update); + break; + default: + break; + } + } + + private updateTool(update: ToolCall | ToolCallUpdate): void { + const callId = update.toolCallId; + if (this.finishedTools.has(callId)) return; + const previous = this.tools.get(callId); + const tool: ToolCall = { + toolCallId: callId, + title: previous?.title ?? 'External tool', + ...previous, + ...Object.fromEntries( + Object.entries(update).filter( + ([, value]) => value !== null && value !== undefined, + ), + ), + }; + this.tools.set(callId, tool); + const meta = tool._meta?.['claudeCode']; + const reportedName = + isRecord(meta) && typeof meta['toolName'] === 'string' + ? meta['toolName'] + : (tool.kind ?? 'external_tool'); + const name = this.toolNames.get(callId) ?? reportedName; + if (!previous) { + this.toolNames.set(callId, name); + this.toolCalls++; + this.emitter.emit(AgentEventType.TOOL_CALL, { + subagentId: this.id, + round: this.round, + callId, + name, + args: isRecord(tool.rawInput) + ? tool.rawInput + : { input: tool.rawInput }, + description: tool.title, + timestamp: Date.now(), + }); + } + if (tool.status !== 'completed' && tool.status !== 'failed') return; + this.finishedTools.add(callId); + this.tools.delete(callId); + const success = tool.status === 'completed'; + if (success) this.toolSucceeded++; + else this.toolFailed++; + this.toolNames.delete(callId); + const response = { + input: tool.rawInput ?? null, + toolName: reportedName, + output: tool.rawOutput ?? null, + content: tool.content ?? null, + status: tool.status, + }; + const responseParts = [ + { functionResponse: { id: callId, name, response } }, + ]; + this.emitter.emit(AgentEventType.TOOL_RESULT, { + subagentId: this.id, + round: this.round, + callId, + name, + success, + responseParts, + resultDisplay: JSON.stringify(tool.rawOutput ?? tool.content ?? null), + timestamp: Date.now(), + }); + this.emitter.emit(AgentEventType.TOOL_RESPONSES_FINALIZED, { + subagentId: this.id, + round: this.round, + responses: [{ callId, responseParts }], + timestamp: Date.now(), + }); + } + + /** + * Emit a terminal TOOL_RESULT + TOOL_RESPONSES_FINALIZED for every tool call + * still open when the turn ends. Cancel, timeout and peer crash all leave + * entries in `this.tools`: `cancel()` sets `this.cancelled` before the peer's + * own terminal `tool_call_update` can arrive (so `onSessionUpdate` drops it), + * and the next `runTurn` clears the map. Without this flush three consumers + * keep a false record — the inline frame and Web Shell row show the tool + * executing forever (TOOL_RESULT is the only writer of their status), the + * JSONL transcript holds a permanently unpaired `functionCall` + * (TOOL_RESPONSES_FINALIZED is the only writer of its `tool_result`), and + * FINISH's totals do not sum. The in-process sibling closes the same gap via + * `onAllToolCallsComplete`. + */ + private flushOpenTools(): void { + for (const [callId, tool] of this.tools) { + const meta = tool._meta?.['claudeCode']; + const reportedName = + isRecord(meta) && typeof meta['toolName'] === 'string' + ? meta['toolName'] + : (tool.kind ?? 'external_tool'); + const name = this.toolNames.get(callId) ?? reportedName; + this.toolFailed++; + const response = { + input: tool.rawInput ?? null, + toolName: reportedName, + output: null, + content: tool.content ?? null, + status: 'failed', + }; + const responseParts = [ + { functionResponse: { id: callId, name, response } }, + ]; + this.emitter.emit(AgentEventType.TOOL_RESULT, { + subagentId: this.id, + round: this.round, + callId, + name, + success: false, + responseParts, + resultDisplay: JSON.stringify(tool.content ?? null), + timestamp: Date.now(), + }); + this.emitter.emit(AgentEventType.TOOL_RESPONSES_FINALIZED, { + subagentId: this.id, + round: this.round, + responses: [{ callId, responseParts }], + timestamp: Date.now(), + }); + } + this.tools.clear(); + this.toolNames.clear(); + } + + private async onRequestPermission( + params: RequestPermissionRequest, + ): Promise { + const cancelled: RequestPermissionResponse = { + outcome: { outcome: 'cancelled' }, + }; + const runtime = this.params.runtimeContext; + const callId = params.toolCall.toolCallId; + const meta = params.toolCall._meta?.['claudeCode']; + const name = + isRecord(meta) && typeof meta['toolName'] === 'string' + ? meta['toolName'] + : (params.toolCall.kind ?? 'external_tool'); + // Turn-level and request-integrity states: the turn is already over, the + // request is not ours, or it cannot be answered safely (no options, or + // ambiguous duplicate option IDs). ACP `cancelled` is the right response — + // there is no single tool call to deny. + if ( + params.sessionId !== this.sessionId || + !this.executing || + this.cancelled || + this.disposed || + this.failure.signal.aborted || + this.pendingPermissions.has(callId) || + params.options.length === 0 || + new Set(params.options.map((option) => option.optionId)).size !== + params.options.length + ) + return cancelled; + // Host-policy denials: reject the TOOL, not the TURN, so the peer continues + // without this one action (see selectRejectOption). Falls back to + // `cancelled` only when the peer offered no reject option to select. + if ( + name.replaceAll('_', '').toLowerCase() === 'askuserquestion' || + runtime.getShouldAvoidPermissionPrompts() || + (!runtime.isInteractive() && + !runtime.getExperimentalZedIntegration() && + runtime.getInputFormat() !== InputFormat.STREAM_JSON) || + this.emitter.rawListeners(AgentEventType.TOOL_WAITING_APPROVAL).length === + 0 + ) { + const rejectOptionId = selectRejectOption(params.options); + return rejectOptionId + ? { outcome: { outcome: 'selected', optionId: rejectOptionId } } + : cancelled; + } + return new Promise((resolve) => { + const finish = (optionId?: string) => { + if (this.pendingPermissions.get(callId) !== deny) return; + this.pendingPermissions.delete(callId); + resolve( + optionId ? { outcome: { outcome: 'selected', optionId } } : cancelled, + ); + }; + const deny = () => finish(); + this.pendingPermissions.set(callId, deny); + try { + this.emitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, { + subagentId: this.id, + round: this.round, + callId, + name, + args: isRecord(params.toolCall.rawInput) + ? params.toolCall.rawInput + : {}, + description: params.toolCall.title ?? 'External action', + confirmationDetails: { + type: 'info', + title: params.toolCall.title ?? 'External action', + prompt: params.options.map((option) => option.name).join(' / '), + hideAlwaysAllow: true, + }, + respond: async (outcome) => + finish( + selectPermissionOption(params.options, outcome) ?? + // An explicit user rejection of THIS tool must deny the tool, + // not cancel the whole turn: `selectPermissionOption` maps + // Cancel to undefined, and ACP `cancelled` ends the prompt turn + // (abandoning the remaining work and billing a fresh prompt to + // re-delegate). Select the peer's `reject_once` option so the + // turn continues without this one action, matching the dialog's + // "suggest changes" promise and the in-process sibling. Fall + // back to `cancelled` only when no reject option is selectable. + (outcome === ToolConfirmationOutcome.Cancel + ? selectRejectOption(params.options) + : undefined), + ), + timestamp: Date.now(), + }); + } catch { + deny(); + } + }); + } +} + +export const acpExternalAgentExecutor: ExternalAgentExecutor = { + create: (params) => AcpSubagentExecutor.create(params), +}; diff --git a/packages/cli/tsconfig.json b/packages/cli/tsconfig.json index be0cb3f7934..371b8d34ef1 100644 --- a/packages/cli/tsconfig.json +++ b/packages/cli/tsconfig.json @@ -37,6 +37,9 @@ "@qwen-code/qwen-code-core/conversationsRuntimeMarker": [ "../core/src/utils/conversations-runtime-marker.ts" ], + "@qwen-code/qwen-code-core/subagentRuntime": [ + "../core/src/subagent-runtime.ts" + ], "@qwen-code/qwen-code-core/*": ["../core/src/*"], "@qwen-code/acp-bridge": ["../acp-bridge/src/index.ts"], "@qwen-code/acp-bridge/transcriptReplay": [ diff --git a/packages/cli/vitest.config.ts b/packages/cli/vitest.config.ts index f6c244ba088..73bd04d6b04 100644 --- a/packages/cli/vitest.config.ts +++ b/packages/cli/vitest.config.ts @@ -74,6 +74,10 @@ export default defineConfig({ __dirname, '../core/src/utils/conversations-runtime-marker.ts', ), + '@qwen-code/qwen-code-core/subagentRuntime': path.resolve( + __dirname, + '../core/src/subagent-runtime.ts', + ), }), // Mirrors `"@qwen-code/qwen-code-core/*": ["../core/src/*"]` from // packages/cli/tsconfig.json. esbuild reads that paths block when it diff --git a/packages/core/package.json b/packages/core/package.json index ad79cf1f54c..2eae3ee419b 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -13,6 +13,10 @@ "types": "./dist/index.d.ts", "import": "./dist/index.js" }, + "./subagentRuntime": { + "types": "./dist/src/subagent-runtime.d.ts", + "import": "./dist/src/subagent-runtime.js" + }, "./transcriptRecords": { "types": "./dist/src/utils/transcript-records.d.ts", "import": "./dist/src/utils/transcript-records.js" diff --git a/packages/core/src/agents/agent-transcript.ts b/packages/core/src/agents/agent-transcript.ts index 9e793b168e9..791c96af869 100644 --- a/packages/core/src/agents/agent-transcript.ts +++ b/packages/core/src/agents/agent-transcript.ts @@ -147,6 +147,8 @@ export interface AgentMeta { persistedCliFlags?: AgentPersistedCliFlags; /** Canonical subagent config name used to recreate this agent. */ subagentName?: string; + /** External launch provenance; transcript replay cannot restore its session. */ + executor?: 'acp'; /** UI hint preserved for resumed task rows. */ agentColor?: string; /** Number of explicit resume attempts performed so far. */ diff --git a/packages/core/src/agents/background-agent-resume.test.ts b/packages/core/src/agents/background-agent-resume.test.ts index b876f8ce5cd..b757511619c 100644 --- a/packages/core/src/agents/background-agent-resume.test.ts +++ b/packages/core/src/agents/background-agent-resume.test.ts @@ -459,6 +459,61 @@ describe('BackgroundAgentResumeService', () => { ).toBeUndefined(); }); + it.each(['persisted', 'definition', 'legacy-model', 'legacy-flags'] as const)( + 'blocks cold external resume using %s provenance', + async (provenance) => { + const sessionId = 'session-external'; + const agentId = 'external-agent'; + writeAgentMeta(getAgentMetaPath(tempDir, sessionId, agentId), { + agentId, + agentType: 'researcher', + description: 'External task', + parentSessionId: sessionId, + parentAgentId: null, + createdAt: new Date().toISOString(), + status: 'running', + ...(provenance === 'persisted' ? { executor: 'acp' as const } : {}), + ...(provenance === 'legacy-model' + ? { model: 'external-acp:claude' } + : {}), + ...(provenance === 'legacy-flags' + ? { persistedCliFlags: { model: 'external-acp:claude' } } + : {}), + }); + fs.writeFileSync( + getAgentJsonlPath(tempDir, sessionId, agentId), + JSON.stringify({ + uuid: 'u1', + sessionId, + type: 'user', + message: { role: 'user', parts: [{ text: 'External task' }] }, + }) + '\n', + ); + const { service, subagentManager } = createService(); + if (provenance === 'definition') { + subagentManager.loadSubagent.mockResolvedValue({ + name: 'researcher', + color: 'cyan', + model: undefined, + approvalMode: undefined, + executor: { kind: 'acp', command: 'claude' }, + } as Awaited>); + } + const recovered = await service.loadPausedBackgroundAgents(sessionId); + expect(recovered[0]?.resumeBlockedReason).toContain( + 'External subagent session cannot be restored', + ); + expect(await service.resumeBackgroundAgent(agentId)).toBeUndefined(); + // Recheck provenance at execution time, not only during discovery. + recovered[0]!.resumeBlockedReason = undefined; + expect(await service.resumeBackgroundAgent(agentId)).toBeUndefined(); + expect(registry.get(agentId)?.resumeBlockedReason).toContain( + 'External subagent session cannot be restored', + ); + expect(subagentManager.createAgentHeadless).not.toHaveBeenCalled(); + }, + ); + it('preserves model on recovered paused agents for per-model caps', async () => { const sessionId = 'session-model'; const agentId = 'agent-model'; diff --git a/packages/core/src/agents/background-agent-resume.ts b/packages/core/src/agents/background-agent-resume.ts index 22624bde8d4..e01ef87499c 100644 --- a/packages/core/src/agents/background-agent-resume.ts +++ b/packages/core/src/agents/background-agent-resume.ts @@ -17,6 +17,7 @@ import { } from './runtime/agent-events.js'; import { AgentTerminateMode } from './runtime/agent-types.js'; import { AgentHeadless, ContextState } from './runtime/agent-headless.js'; +import type { SubagentExecutor } from './runtime/subagent-executor.js'; import { buildAgentTranscriptAttach, getAgentJsonlPath, @@ -388,7 +389,7 @@ function recoverTranscript(records: ChatRecord[]): TranscriptRecovery { } function getCompletionStats( - subagent: AgentHeadless, + subagent: SubagentExecutor, liveToolCallCount: number, ): AgentCompletionStats { const summary = subagent.getExecutionSummary(); @@ -503,7 +504,12 @@ export class BackgroundAgentResumeService { if (registry.get(meta.agentId)) continue; const subagentName = meta.subagentName ?? meta.agentType; if (typeof subagentName !== 'string' || !subagentName) continue; - const target = await this.resolveResumeTarget(subagentName); + const target = await this.resolveResumeTarget( + subagentName, + meta.executor, + meta.model, + meta.persistedCliFlags?.model, + ); const outputFile = getAgentJsonlPath( projectDir, @@ -863,7 +869,12 @@ export class BackgroundAgentResumeService { try { const subagentName = meta.subagentName ?? meta.agentType; - const target = await this.resolveResumeTarget(subagentName); + const target = await this.resolveResumeTarget( + subagentName, + meta.executor, + meta.model, + meta.persistedCliFlags?.model, + ); if (!target.subagentConfig && !target.isFork) { const reason = target.unavailableReason || @@ -992,7 +1003,7 @@ export class BackgroundAgentResumeService { const bgEventEmitter = new AgentEventEmitter(); const launchModel = meta.model ?? meta.persistedCliFlags?.model; - let subagent: AgentHeadless; + let subagent: SubagentExecutor; if (target.isFork) { subagent = await this.createResumedForkSubagent( activeAgentConfig, @@ -1545,7 +1556,22 @@ export class BackgroundAgentResumeService { private async resolveResumeTarget( subagentName: string, + executor?: AgentMeta['executor'], + ...legacyModels: Array ): Promise { + // Older external runs wrote a synthetic model label instead of provenance. + // It can deny replay, but never authorizes selecting an executor. + if ( + executor !== undefined || + legacyModels.some((model) => model?.startsWith('external-acp:')) + ) { + return { + agentName: subagentName, + isFork: false, + unavailableReason: + 'External subagent session cannot be restored from a Qwen transcript. Start a new agent instead.', + }; + } if (subagentName === FORK_SUBAGENT_TYPE) { return { agentName: FORK_AGENT.name, @@ -1565,6 +1591,9 @@ export class BackgroundAgentResumeService { }; } + if (subagentConfig.executor !== undefined) { + return this.resolveResumeTarget(subagentName, 'acp'); + } return { agentName: subagentConfig.name, isFork: false, @@ -1784,7 +1813,7 @@ export class BackgroundAgentResumeService { } private async runSubagentStopHookLoop( - subagent: AgentHeadless, + subagent: SubagentExecutor, opts: { agentId: string; agentType: string; diff --git a/packages/core/src/agents/runtime/agent-core.ts b/packages/core/src/agents/runtime/agent-core.ts index 3aa9d45ce29..0e7b58c244a 100644 --- a/packages/core/src/agents/runtime/agent-core.ts +++ b/packages/core/src/agents/runtime/agent-core.ts @@ -391,6 +391,37 @@ export interface ExecutionStats { totalTokens?: number; } +export function renderSubagentSystemPrompt( + promptConfig: PromptConfig, + context: ContextState, + runtimeContext: Config, + interactive?: boolean, +): string { + if (!promptConfig.systemPrompt) { + return ''; + } + + let finalPrompt = templateString(promptConfig.systemPrompt, context); + + // Only add non-interactive instructions when NOT in interactive mode + if (!interactive) { + finalPrompt += ` + +Important Rules: + - You operate in non-interactive mode: do not ask the user questions; proceed with available context. + - Use tools only when necessary to obtain facts or make changes. + - When the task is complete, return the final result as a normal model response (not a tool call) and stop.`; + } + + // Context files (QWEN.md + output-language.md) keep the subagent aligned + // with project conventions; the volatile auto-memory section stays last. + return assembleSystemPrompt({ + base: finalPrompt, + contextFiles: runtimeContext.getUserMemory(), + autoMemory: runtimeContext.getAutoMemoryPrompt(), + }); +} + /** * AgentCore — shared execution engine for model reasoning and tool scheduling. * @@ -2623,29 +2654,12 @@ export class AgentCore { context: ContextState, options?: CreateChatOptions, ): string { - if (!this.promptConfig.systemPrompt) { - return ''; - } - - let finalPrompt = templateString(this.promptConfig.systemPrompt, context); - - // Only add non-interactive instructions when NOT in interactive mode - if (!options?.interactive) { - finalPrompt += ` - -Important Rules: - - You operate in non-interactive mode: do not ask the user questions; proceed with available context. - - Use tools only when necessary to obtain facts or make changes. - - When the task is complete, return the final result as a normal model response (not a tool call) and stop.`; - } - - // Context files (QWEN.md + output-language.md) keep the subagent aligned - // with project conventions; the volatile auto-memory section stays last. - return assembleSystemPrompt({ - base: finalPrompt, - contextFiles: this.runtimeContext.getUserMemory(), - autoMemory: this.runtimeContext.getAutoMemoryPrompt(), - }); + return renderSubagentSystemPrompt( + this.promptConfig, + context, + this.runtimeContext, + options?.interactive, + ); } /** diff --git a/packages/core/src/agents/runtime/agent-headless.ts b/packages/core/src/agents/runtime/agent-headless.ts index 08a00fc3382..2c567d705cb 100644 --- a/packages/core/src/agents/runtime/agent-headless.ts +++ b/packages/core/src/agents/runtime/agent-headless.ts @@ -29,6 +29,7 @@ import type { } from './agent-events.js'; import { AgentEventType } from './agent-events.js'; import type { AgentStatsSummary } from './agent-statistics.js'; +import type { SubagentExecutor } from './subagent-executor.js'; import type { PromptConfig, ModelConfig, @@ -135,7 +136,7 @@ export function templateString( * Each execute() call runs one task through AgentCore's reasoning loop. Calls * must be sequential; later calls reuse the same chat and prepared tools. */ -export class AgentHeadless { +export class AgentHeadless implements SubagentExecutor { private readonly core: AgentCore; private finalText: string = ''; private terminateMode: AgentTerminateMode = AgentTerminateMode.ERROR; diff --git a/packages/core/src/agents/runtime/subagent-executor.ts b/packages/core/src/agents/runtime/subagent-executor.ts new file mode 100644 index 00000000000..80633e4924b --- /dev/null +++ b/packages/core/src/agents/runtime/subagent-executor.ts @@ -0,0 +1,74 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { Config } from '../../config/config.js'; +import type { SubagentExecutorSpec } from '../../subagents/types.js'; +import type { RuntimeContentGeneratorView } from './agent-context.js'; +import type { AgentEventEmitter, AgentHooks } from './agent-events.js'; +import type { ContextState } from './agent-headless.js'; +import type { AgentStatsSummary } from './agent-statistics.js'; +import type { + AgentExternalInput, + AgentTerminateMode, + ModelConfig, + PromptConfig, + RunConfig, + ToolConfig, +} from './agent-types.js'; + +export interface SubagentExecutorCore { + getEventEmitter(): AgentEventEmitter; + readonly modelConfig: ModelConfig; + readonly runtimeView?: RuntimeContentGeneratorView; +} + +export interface SubagentExecutor { + execute( + context: ContextState, + externalSignal?: AbortSignal, + options?: { resetStats?: boolean }, + ): Promise; + + executeExternalInputs( + inputs: AgentExternalInput[], + externalSignal?: AbortSignal, + options?: { resetStats?: boolean }, + ): Promise; + + getFinalText(): string; + getTerminateMode(): AgentTerminateMode; + getExecutionSummary(): AgentStatsSummary; + getCore(): SubagentExecutorCore; + + setExternalMessageProvider(provider: () => AgentExternalInput[]): void; + setExternalMessageWaiter?( + waiter: (signal: AbortSignal) => Promise, + ): void; + setExternalMessageWaitPredicate?(predicate: () => boolean): void; + + /** Resolves after owned resources have been released. */ + dispose?(): void | Promise; +} + +export interface ExternalAgentExecutorParams { + spec: SubagentExecutorSpec; + name: string; + approvalMode?: string; + permissionMode?: string; + promptConfig: PromptConfig; + modelConfig: ModelConfig; + runConfig: RunConfig; + toolConfig?: ToolConfig; + eventEmitter?: AgentEventEmitter; + hooks?: AgentHooks; + taskName?: string; + subagentId?: string; + runtimeContext: Config; +} + +export interface ExternalAgentExecutor { + create(params: ExternalAgentExecutorParams): Promise; +} diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts index 9d693494ed3..3eaf228681d 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts @@ -3137,6 +3137,98 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( }; } + it.each([ + { label: 'plain', options: {}, tokenLimit: null }, + { label: 'budgeted', options: {}, tokenLimit: 100 }, + { + label: 'schema', + options: { schema: { type: 'object' } }, + tokenLimit: null, + }, + { + label: 'budgeted schema', + options: { schema: { type: 'object' } }, + tokenLimit: 100, + }, + { + label: 'worktree', + options: { isolation: 'worktree' as const }, + tokenLimit: null, + }, + { + label: 'pinned worktree', + options: { workingDir: '/fake/repo/worktree' }, + tokenLimit: null, + }, + ])( + 'rejects $label external workflow dispatch before agent creation', + async ({ options, tokenLimit }) => { + const { WorkflowBudgetImpl } = await import('./workflow-budget.js'); + const budget = new WorkflowBudgetImpl(tokenLimit); + const onTokens = vi.fn((tokens: number) => budget.recordSpent(tokens)); + const onCreate = vi.fn(async () => ({ + finalText: 'external output', + terminateMode: 'GOAL', + })); + const { config, calls } = fakeConfigWithMgr({ + findSubagentByName: async () => ({ + name: 'external-agent', + description: 'External agent', + systemPrompt: 'Complete the task.', + level: 'session', + executor: { kind: 'acp', command: 'external-agent' }, + }), + onCreate, + }); + const createRegistry = vi.spyOn(config, 'createToolRegistry'); + const dispatch = createProductionDispatch(config, undefined, onTokens); + + await expect( + dispatch('do work', { agentType: 'external-agent', ...options }), + ).rejects.toThrow( + 'Workflow agent() does not support external-executor agents: ' + + 'token budgets, schema output, and workflow tool restrictions ' + + 'cannot be enforced. Use an in-process agent definition instead.', + ); + + // The manager owns external factory/process creation; never enter it. + expect(calls).toEqual([]); + expect(onCreate).not.toHaveBeenCalled(); + expect(createRegistry).not.toHaveBeenCalled(); + expect(worktreeStubs.instances).toHaveLength(0); + expect(pinStub.seenLabels).toEqual([]); + expect(onTokens).not.toHaveBeenCalled(); + }, + ); + + it('continues to account tokens for an in-process workflow agent', async () => { + const { WorkflowBudgetImpl } = await import('./workflow-budget.js'); + const budget = new WorkflowBudgetImpl(100); + const { config, calls } = fakeConfigWithMgr({ + findSubagentByName: async () => ({ + name: 'ordinary-agent', + description: 'In-process agent', + systemPrompt: 'Complete the task.', + level: 'session', + }), + onCreate: async () => ({ finalText: 'done', terminateMode: 'GOAL' }), + }); + const previousTokens = nextOutputTokens.value; + nextOutputTokens.value = 25; + try { + const dispatch = createProductionDispatch(config, undefined, (tokens) => + budget.recordSpent(tokens), + ); + await expect( + dispatch('do work', { agentType: 'ordinary-agent' }), + ).resolves.toBe('done'); + expect(calls).toHaveLength(1); + expect(budget.remaining()).toBe(75); + } finally { + nextOutputTokens.value = previousTokens; + } + }); + it('agentType resolves SubagentConfig and routes through createAgentHeadless', async () => { const { config, calls } = fakeConfigWithMgr({ findSubagentByName: async () => ({ diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.ts b/packages/core/src/agents/runtime/workflow-orchestrator.ts index 7e31d59117e..380bd0e5424 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.ts @@ -511,6 +511,13 @@ export function createProductionDispatch( // agentType definition rides along so the override path reuses it // instead of re-scanning subagent files per attempt. const agentIdentity = await resolveWorkflowAgentIdentity(config, opts); + if (agentIdentity.resolvedAgentType?.executor !== undefined) { + throw new Error( + 'Workflow agent() does not support external-executor agents: ' + + 'token budgets, schema output, and workflow tool restrictions ' + + 'cannot be enforced. Use an in-process agent definition instead.', + ); + } let attempt = 0; return runStallResilient( async (attemptSignal, emitter) => { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 6362c624b46..a4344b9a27a 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -46,6 +46,7 @@ import { } from '../core/contentGenerator.js'; import { tokenLimit } from '../core/tokenLimits.js'; import { getRuntimeContentGenerator } from '../agents/runtime/agent-context.js'; +import type { ExternalAgentExecutor } from '../agents/runtime/subagent-executor.js'; import { isTieredEffortWireModel } from '../core/modalityDefaults.js'; import { DashScopeOpenAICompatibleProvider, @@ -2375,6 +2376,12 @@ export class Config { private readonly sessionWorkflowEnabled: boolean; private sessionWorkflowEnabledProvider?: () => boolean; private sessionWorkflowPlanRevision?: SessionWorkflowPlanRevision; + /** + * Host-supplied factory for subagents that declare an `executor` block. + * `packages/core` has no ACP dependency, so the implementation lives in the + * host package. See `ExternalAgentExecutor` and `setExternalAgentExecutor`. + */ + private externalAgentExecutor?: ExternalAgentExecutor; private readonly modelProposedGoals: ModelProposedGoalsMode; private readonly skipWorkflowUsageWarning: boolean = false; private readonly emitToolUseSummaries: boolean = true; @@ -7849,6 +7856,23 @@ export class Config { } } + /** + * Registers the host's factory for subagents that declare an `executor` + * block. `packages/core` has no ACP dependency, so the implementation lives + * in the host package; see `ExternalAgentExecutor`. + * + * A definition that declares an executor while none is registered fails + * loudly at spawn time rather than silently running in-process — silent + * substitution is the exact failure mode this seam exists to prevent. + */ + setExternalAgentExecutor(executor?: ExternalAgentExecutor): void { + this.externalAgentExecutor = executor; + } + + getExternalAgentExecutor(): ExternalAgentExecutor | undefined { + return this.externalAgentExecutor; + } + getSessionWorkflowPlanRevision(): SessionWorkflowPlanRevision | undefined { if (!this.isSessionWorkflowEnabled()) return undefined; return this.sessionWorkflowPlanRevision; diff --git a/packages/core/src/subagent-runtime.ts b/packages/core/src/subagent-runtime.ts new file mode 100644 index 00000000000..d78edb4b7b6 --- /dev/null +++ b/packages/core/src/subagent-runtime.ts @@ -0,0 +1,53 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +export type { + ExternalAgentExecutor, + ExternalAgentExecutorParams, + SubagentExecutor, + SubagentExecutorCore, +} from './agents/runtime/subagent-executor.js'; +export { renderSubagentSystemPrompt } from './agents/runtime/agent-core.js'; +export { ContextState } from './agents/runtime/agent-headless.js'; +export { AgentTerminateMode } from './agents/runtime/agent-types.js'; +export type { + AgentExternalInput, + ModelConfig, + PromptConfig, + RunConfig, + ToolConfig, +} from './agents/runtime/agent-types.js'; +export type { SubagentExecutorSpec } from './subagents/types.js'; +export { ToolConfirmationOutcome } from './tools/tools.js'; +export type { ToolCallConfirmationDetails } from './tools/tools.js'; +export type { + AgentStatsSummary, + ToolUsageStats, +} from './agents/runtime/agent-statistics.js'; +export { + AgentEventEmitter, + AgentEventType, +} from './agents/runtime/agent-events.js'; +export type { + AgentApprovalRequestEvent, + AgentConfirmationDetails, + AgentErrorEvent, + AgentEvent, + AgentEventMap, + AgentExternalMessageEvent, + AgentFinishEvent, + AgentHooks, + AgentRoundEvent, + AgentRoundTextEvent, + AgentStartEvent, + AgentStatusChangeEvent, + AgentStreamTextEvent, + AgentToolCallEvent, + AgentToolOutputUpdateEvent, + AgentToolResponsesFinalizedEvent, + AgentToolResultEvent, + AgentUsageEvent, +} from './agents/runtime/agent-events.js'; diff --git a/packages/core/src/subagents/agent-frontmatter-schema.test.ts b/packages/core/src/subagents/agent-frontmatter-schema.test.ts index 80c78ca763c..82eb5cc5705 100644 --- a/packages/core/src/subagents/agent-frontmatter-schema.test.ts +++ b/packages/core/src/subagents/agent-frontmatter-schema.test.ts @@ -9,6 +9,7 @@ import { PERMISSION_MODE_VALUES, COLOR_VALUES, claudePermissionModeToApprovalMode, + parseAgentExecutor, parseAgentHooks, parseAgentMcpServers, parseMaxTurns, @@ -244,4 +245,100 @@ describe('agent-frontmatter-schema', () => { expect(Object.hasOwn(result!, '__proto__')).toBe(true); }); }); + + describe('parseAgentExecutor — qwen-code extension (not mirrored from CC)', () => { + it('parses a minimal legal spec', () => { + expect(parseAgentExecutor({ kind: 'acp', command: 'npx' })).toEqual({ + kind: 'acp', + command: 'npx', + }); + }); + + it('preserves args and trims the command', () => { + expect( + parseAgentExecutor({ + kind: 'acp', + command: ' npx ', + args: ['-y', '@agentclientprotocol/claude-agent-acp'], + }), + ).toEqual({ + kind: 'acp', + command: 'npx', + args: ['-y', '@agentclientprotocol/claude-agent-acp'], + }); + }); + + it('keeps an explicitly empty args array', () => { + expect( + parseAgentExecutor({ kind: 'acp', command: 'x', args: [] }), + ).toEqual({ kind: 'acp', command: 'x', args: [] }); + }); + + it('drops the whole field for an unrecognized or missing kind', () => { + expect( + parseAgentExecutor({ kind: 'subprocess', command: 'npx' }), + ).toBeUndefined(); + expect(parseAgentExecutor({ command: 'npx' })).toBeUndefined(); + expect( + parseAgentExecutor({ kind: 'ACP', command: 'npx' }), + ).toBeUndefined(); + }); + + it('drops the whole field for a missing, non-string, or blank command', () => { + expect(parseAgentExecutor({ kind: 'acp' })).toBeUndefined(); + expect(parseAgentExecutor({ kind: 'acp', command: 42 })).toBeUndefined(); + expect( + parseAgentExecutor({ kind: 'acp', command: ['npx'] }), + ).toBeUndefined(); + expect( + parseAgentExecutor({ kind: 'acp', command: ' ' }), + ).toBeUndefined(); + }); + + it('drops the whole field when args is not an array of strings', () => { + expect( + parseAgentExecutor({ kind: 'acp', command: 'npx', args: '-y' }), + ).toBeUndefined(); + expect( + parseAgentExecutor({ kind: 'acp', command: 'npx', args: {} }), + ).toBeUndefined(); + }); + + it('drops the whole field rather than filtering a partially invalid args array', () => { + // Running with silently-truncated arguments is worse than not running: + // a dropped `-y` or a dropped package specifier changes what executes. + expect( + parseAgentExecutor({ + kind: 'acp', + command: 'npx', + args: ['-y', 42, '@agentclientprotocol/claude-agent-acp'], + }), + ).toBeUndefined(); + }); + + it('returns undefined for a non-object top level', () => { + expect(parseAgentExecutor(undefined)).toBeUndefined(); + expect(parseAgentExecutor(null)).toBeUndefined(); + expect(parseAgentExecutor('npx')).toBeUndefined(); + expect( + parseAgentExecutor([{ kind: 'acp', command: 'npx' }]), + ).toBeUndefined(); + expect(parseAgentExecutor(42)).toBeUndefined(); + }); + + it('does not pass through unrecognized keys', () => { + // The result is rebuilt from known fields only, so a definition cannot + // smuggle extra executor options past the parser into the spawn site. + const result = parseAgentExecutor({ + kind: 'acp', + command: 'npx', + args: ['-y'], + env: { MALICIOUS: '1' }, + cwd: '/etc', + shell: true, + }); + expect(result).toEqual({ kind: 'acp', command: 'npx', args: ['-y'] }); + expect(Object.keys(result!)).toEqual(['kind', 'command', 'args']); + }); + }); }); diff --git a/packages/core/src/subagents/agent-frontmatter-schema.ts b/packages/core/src/subagents/agent-frontmatter-schema.ts index 4305c3fb4f8..634b06e116c 100644 --- a/packages/core/src/subagents/agent-frontmatter-schema.ts +++ b/packages/core/src/subagents/agent-frontmatter-schema.ts @@ -20,6 +20,8 @@ * its semantics would break existing `.qwen/agents/*.md` files. */ +import type { SubagentExecutorSpec } from './types.js'; + /** Permission mode enum (DL7 `$E` / `kc` constant). */ export const PERMISSION_MODE_VALUES = [ 'acceptEdits', @@ -186,3 +188,46 @@ export function parseAgentHooks( } return Object.keys(result).length > 0 ? result : undefined; } + +/** + * Parse a frontmatter `executor` value into a `SubagentExecutorSpec`. + * + * Unlike `mcpServers` and `hooks`, this field is NOT part of the mirrored + * Claude Code schema — it is a qwen-code extension (see + * `docs/design/claude-code-web-shell-backend.md` §9.3). Because it names an + * external process, it is validated strictly enough that a typo cannot + * silently change what runs: + * + * - non-object / array / null → undefined (whole field dropped) + * - `kind` absent or not `'acp'` → undefined + * - `command` absent, non-string, or blank after trim → undefined + * - `args` present but not an array of strings → undefined. Dropped whole + * rather than filtered: running with silently-truncated arguments is + * worse than not running at all. + * + * The caller must reject a provided value when this returns undefined; it + * must never drop the executor and fall back to in-process execution. + */ +export function parseAgentExecutor( + value: unknown, +): SubagentExecutorSpec | undefined { + if (value === undefined || value === null) return undefined; + if (typeof value !== 'object' || Array.isArray(value)) return undefined; + const record = value as Record; + if (record['kind'] !== 'acp') return undefined; + const command = record['command']; + if (typeof command !== 'string') return undefined; + const trimmedCommand = command.trim(); + if (trimmedCommand === '') return undefined; + const rawArgs = record['args']; + if (rawArgs === undefined) { + return { kind: 'acp', command: trimmedCommand }; + } + if (!Array.isArray(rawArgs)) return undefined; + const args: string[] = []; + for (const arg of rawArgs) { + if (typeof arg !== 'string') return undefined; + args.push(arg); + } + return { kind: 'acp', command: trimmedCommand, args }; +} diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index c90033f180f..4b96aae4463 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -16,6 +16,7 @@ import { } from './types.js'; import type { ToolRegistry } from '../tools/tool-registry.js'; import type { Config } from '../config/config.js'; +import { ApprovalMode } from '../config/approval-mode.js'; import { makeFakeConfig } from '../test-utils/config.js'; import { AuthType } from '../core/contentGenerator.js'; import { ToolNames } from '../tools/tool-names.js'; @@ -380,6 +381,222 @@ You are a helpful assistant. `; describe('parseSubagentContent', () => { + it.each([ + 'null', + 'false', + '0', + '""', + '{ kind: ACP, command: npx }', + '{ kind: acp, command: " " }', + '{ kind: acp, command: npx, args: [null] }', + ])('rejects invalid executor frontmatter %s', async (executor) => { + const content = `---\nname: test-agent\ndescription: Test\nexecutor: ${executor}\n---\nPrompt`; + vi.mocked(fs.readFile).mockResolvedValue(content); + await expect( + manager.parseSubagentFile(validConfig.filePath!, 'project'), + ).rejects.toThrow(/invalid executor block/); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + }); + + it('rejects an executor whose null argument the shared parser would sanitize away', async () => { + // Pins the load-bearing guard that validates the ORIGINAL YAML node + // (parseDocument) rather than the shared parser's sanitized result. The + // shared parser strips null sequence items, turning `args: [null]` into + // `args: []` — which parseAgentExecutor ACCEPTS, launching the command + // with truncated arguments. Only the original node still carries [null] + // and is rejected. Drive the real shared parser so the stripping actually + // happens; with the guard reverted to the sanitized value this no longer + // throws and the test goes red. + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementationOnce(yaml.parse); + const content = + '---\nname: test-agent\ndescription: Test\nexecutor:\n kind: acp\n command: npx\n args:\n - null\n---\nPrompt'; + vi.mocked(fs.readFile).mockResolvedValue(content); + await expect( + manager.parseSubagentFile(validConfig.filePath!, 'project'), + ).rejects.toThrow(/invalid executor block/); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + }); + + it('rejects a definition whose frontmatter has a YAML syntax error rather than trusting a repaired executor node', async () => { + // parseDocument repairs invalid YAML instead of throwing; document.errors + // is the only signal. Without this guard an unterminated quote yields a + // silently repaired executor node that dispatches a different command than + // the file declares. + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + const content = + "---\nname: test-agent\ndescription: Test\nexecutor:\n kind: acp\n command: 'npx\n---\nPrompt"; + vi.mocked(fs.readFile).mockResolvedValue(content); + await expect( + manager.parseSubagentFile(validConfig.filePath!, 'project'), + ).rejects.toThrow(/invalid YAML frontmatter|invalid executor block/); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + }); + + it('still loads a non-executor definition whose frontmatter strict YAML rejects', async () => { + // R5-1: the document.errors refusal must be scoped to executor-bearing + // files. A description containing a colon makes strict YAML reject, but + // the shared parser's parseSimple fallback still loads it; with no + // executor block the definition must keep loading exactly as at the merge + // base, or the agent silently vanishes from /agents and subagent_type + // fails as agent-not-found. Drive the real shared parser so the lenient + // fallback is actually exercised; an unconditional document.errors guard + // makes this throw and the test go red. + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + const content = + '---\nname: reviewer\ndescription: Reviews code: fast and careful\n---\nPrompt'; + vi.mocked(fs.readFile).mockResolvedValue(content); + const config = await manager.parseSubagentFile( + validConfig.filePath!, + 'project', + ); + expect(config.name).toBe('reviewer'); + expect(config.description).toBe('Reviews code: fast and careful'); + expect(config.executor).toBeUndefined(); + }); + + it('refuses a quoted top-level "executor" key when the frontmatter YAML is malformed (R7-1 under-refusal leg)', async () => { + // A quoted `"executor":` plus a YAML error the lenient parser tolerates + // (an unquoted colon in description) is missed by parseSimple (keeps the + // quotes in the key) and by a repaired parseDocument (nests it). Only the + // column-0 raw-text probe catches it; without that the file loads with no + // executor and silently runs in-process — the exact substitution this PR + // exists to prevent. + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + const content = + '---\nname: worker\ndescription: Reviews code: fast\n"executor":\n kind: acp\n command: npx\n---\nPrompt'; + vi.mocked(fs.readFile).mockResolvedValue(content); + await expect( + manager.parseSubagentFile(validConfig.filePath!, 'project'), + ).rejects.toThrow(/invalid executor block/); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + }); + + it('does not refuse a definition whose only "executor" token is nested, when the YAML is malformed (R7-1 over-refusal leg)', async () => { + // An indented `executor:` under another key declares no top-level + // executor. With the column-0 anchor it is not matched, so a pre-existing + // definition that merely mentions executor in a nested position (and has + // an unrelated YAML quirk) still loads instead of being hard-refused. + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + const content = + '---\nname: reviewer\ndescription: Reviews code: fast\nmetadata:\n executor: legacy-note\n---\nPrompt'; + vi.mocked(fs.readFile).mockResolvedValue(content); + const config = await manager.parseSubagentFile( + validConfig.filePath!, + 'project', + ); + expect(config.name).toBe('reviewer'); + expect(config.executor).toBeUndefined(); + }); + + it('loads a valid executor whose frontmatter has an unrelated tolerated YAML error before it (R7-1 over-refusal, narrowed)', async () => { + // A duplicate `name:` key from a bad merge is a tolerated YAML error that + // sits BEFORE the executor line, so it cannot reach the executor subtree: + // `document.toJS().executor` stays byte-faithful. The old + // `claimsExecutor && document.errors.length > 0` guard hard-refused this, + // deleting a valid external-agent definition from /agents (and breaking + // `subagent_type:`) over an unrelated quirk that loads at the merge base. + // Removing the line-scoping must turn this red, while the quoted-key and + // unterminated-quote refusals (errors at/after the executor line) stay + // green. + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + const content = + '---\nname: a\nname: b\ndescription: Test\nexecutor:\n kind: acp\n command: claude-agent-acp\n---\nPrompt'; + vi.mocked(fs.readFile).mockResolvedValue(content); + const config = await manager.parseSubagentFile( + validConfig.filePath!, + 'project', + ); + expect(config.executor).toEqual({ + kind: 'acp', + command: 'claude-agent-acp', + }); + }); + + it('refuses an executor whose node parseDocument dropped via an earlier error, not trusting the parseSimple fallback (R9-2)', async () => { + // A compact-mapping error on an EARLIER line (the unquoted colon in + // description) makes parseDocument drop the whole remainder, so the + // executor node is ABSENT (has('executor') is false) rather than + // byte-faithful — YAML errors are not line-local, so an error before the + // executor line does not mean the executor survived. The lenient + // parseSimple fallback then rebuilds `command: |` plus an indented `npx` + // as `{command:'|', npx:''}` (it is a line-based heuristic, not a YAML + // parser), and parseAgentExecutor would accept `command:'|'`, spawning an + // executable literally named `|` that the file never declared. The refusal + // must key on parseDocument losing the node (!hasExecutor), not on + // parseSimple also missing it. Reverting that leg to + // `!hasExecutor && frontmatter.executor === undefined` turns this red, + // while the duplicate-key load test above (has('executor') true) stays + // green. + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + const content = + '---\nname: explore\ndescription: Reviews code: fast\nexecutor:\n kind: acp\n command: |\n npx\n---\nPrompt'; + vi.mocked(fs.readFile).mockResolvedValue(content); + await expect( + manager.parseSubagentFile(validConfig.filePath!, 'project'), + ).rejects.toThrow(/invalid executor block/); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + }); + + it('refuses an executor whose frontmatter has an unresolved YAML alias instead of throwing a raw parse error (R9 deferred :1968)', async () => { + // parseDocument tolerates an unresolved alias (`command: *undef`) with an + // EMPTY document.errors, so the errors-based executor guard cannot catch + // it, but document.toJS() throws when it resolves the node. Without the + // try/catch around toJS() the raw "Unresolved alias" YAML error escapes + // parseSubagentContent; with it the definition is refused as an invalid + // executor block. Removing the try/catch turns this red (the rejection + // message becomes the raw YAML error, not /invalid executor block/). + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + const content = + '---\nname: explore\ndescription: Test\nexecutor:\n kind: acp\n command: *undefined_anchor\n---\nPrompt'; + vi.mocked(fs.readFile).mockResolvedValue(content); + await expect( + manager.parseSubagentFile(validConfig.filePath!, 'project'), + ).rejects.toThrow(/invalid executor block/); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + }); + + it('visibly reports invalid executors while continuing discovery', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.mocked(fs.readdir).mockResolvedValue(['bad.md', 'good.md'] as never); + vi.mocked(fs.readFile).mockImplementation(async (file) => + String(file).endsWith('bad.md') + ? '---\nname: bad\ndescription: Test\nexecutor: { kind: acp, command: " " }\n---\nPrompt' + : validMarkdown, + ); + const agents = await manager.listSubagents({ + level: 'project', + force: true, + }); + expect(agents.map((agent) => agent.name)).toEqual(['test-agent']); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining('invalid executor block'), + ); + }); + it('should parse valid markdown content', () => { const config = manager.parseSubagentContent( validMarkdown, @@ -1029,6 +1246,48 @@ You are weird. }); describe('serializeSubagent', () => { + it('preserves the executor through save and reload', async () => { + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockStringifyYaml.mockImplementationOnce(yaml.stringify); + mockParseYaml.mockImplementationOnce(yaml.parse); + const executor = { + kind: 'acp' as const, + command: 'npx', + args: ['-y', 'adapter'], + }; + const serialized = manager.serializeSubagent({ + ...validConfig, + executor, + }); + expect( + manager.parseSubagentContent( + serialized, + validConfig.filePath!, + 'project', + ).executor, + ).toEqual(executor); + }); + + it.each([null, false, 0, '', { kind: 'acp', command: ' ' }])( + 'refuses to save invalid executor %j without writing', + async (executor) => { + vi.mocked(fs.access).mockRejectedValue(new Error('File not found')); + const config = { + ...validConfig, + executor, + } as unknown as SubagentConfig; + expect(() => manager.serializeSubagent(config)).toThrow( + /executor block failed validation/, + ); + await expect( + manager.createSubagent(config, { level: 'project' }), + ).rejects.toThrow(/executor block failed validation/); + expect(fs.writeFile).not.toHaveBeenCalled(); + }, + ); + it('should serialize basic configuration', () => { const serialized = manager.serializeSubagent(validConfig); @@ -2067,6 +2326,26 @@ bad`); describe('Runtime Configuration Methods', () => { describe('convertToRuntimeConfig', () => { + it.each([{ kind: 'acp', command: 'npx' }, null, false, 0, ''])( + 'refuses external executor %j before in-process conversion', + async (executor) => { + await expect( + manager.convertToRuntimeConfig({ + ...validConfig, + tools: ['read_file'], + executor, + } as unknown as SubagentConfig), + ).rejects.toMatchObject({ + code: SubagentErrorCode.INVALID_CONFIG, + message: expect.stringContaining( + 'cannot be converted to an in-process agent', + ), + }); + expect(mockToolRegistry.warmAll).not.toHaveBeenCalled(); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + }, + ); + it('should convert basic configuration', async () => { const runtimeConfig = await manager.convertToRuntimeConfig(validConfig); @@ -2283,6 +2562,265 @@ bad`); }); }); + describe('createAgentHeadless — external executor dispatch', () => { + const executorConfig: SubagentConfig = { + name: 'external-agent', + description: 'Runs somewhere else', + systemPrompt: 'You are external.', + level: 'session' as const, + executor: { kind: 'acp', command: 'npx', args: ['-y', 'some-acp'] }, + }; + + afterEach(() => { + mockAgentHeadlessCreate.mockReset(); + vi.restoreAllMocks(); + }); + + it('refuses to run in-process when no executor is registered', async () => { + vi.spyOn(mockConfig, 'getExternalAgentExecutor').mockReturnValue( + undefined, + ); + + await expect( + manager.createAgentHeadless(executorConfig, mockConfig), + ).rejects.toThrow(/registered no external agent executor/); + + // The load-bearing assertion: it must NOT silently substitute the + // in-process executor. A definition that asked for an external agent + // and got AgentHeadless would bill the wrong provider and report the + // wrong agent, with no signal either way. + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + expect(mockToolRegistry.warmAll).not.toHaveBeenCalled(); + expect(mockCreateContentGenerator).not.toHaveBeenCalled(); + }); + + it.each([null, false, 0, '', {}, { kind: 'ACP', command: 'npx' }])( + 'rejects injected executor %j without side effects', + async (executor) => { + const create = vi.fn(); + vi.spyOn(mockConfig, 'getExternalAgentExecutor').mockReturnValue({ + create, + }); + manager.loadSessionSubagents([ + { ...executorConfig, executor } as unknown as SubagentConfig, + ]); + const loaded = await manager.loadSubagent( + executorConfig.name, + 'session', + ); + await expect( + manager.createAgentHeadless(loaded!, mockConfig), + ).rejects.toThrow(/failed validation/); + expect(create).not.toHaveBeenCalled(); + expect(mockToolRegistry.warmAll).not.toHaveBeenCalled(); + expect(mockCreateContentGenerator).not.toHaveBeenCalled(); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + }, + ); + + it.each([ + { tools: [] }, + { tools: ['read_file'] }, + { disallowedTools: ['write_file'] }, + { mcpServers: { server: { command: 'node' } } }, + { hooks: { PreToolUse: [] } }, + { model: 'anthropic:claude' }, + { maxTurns: 3 }, + { runConfig: { max_turns: 3 } }, + ])( + 'rejects unsupported definition %j before factory or setup', + async (constraint) => { + const create = vi.fn(); + vi.spyOn(mockConfig, 'getExternalAgentExecutor').mockReturnValue({ + create, + }); + const hooks = vi.spyOn(mockConfig, 'getHookSystem'); + await expect( + manager.createAgentHeadless( + { ...executorConfig, ...constraint }, + mockConfig, + ), + ).rejects.toThrow(/does not support/); + expect(create).not.toHaveBeenCalled(); + expect(hooks).not.toHaveBeenCalled(); + expect(mockToolRegistry.warmAll).not.toHaveBeenCalled(); + expect(mockCreateContentGenerator).not.toHaveBeenCalled(); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + }, + ); + + it.each([ + { toolConfigOverride: { tools: ['structured_output'] } }, + { promptConfigOverrides: { initialMessages: [] } }, + { promptConfigOverrides: { renderedSystemPrompt: 'schema' } }, + { runtimeAuthOverrides: { authType: 'anthropic' } }, + { modelConfigOverrides: { model: 'claude' } }, + { + modelConfigOverrides: { temperature: 0.5 } as unknown as { + model?: string; + }, + }, + { hooks: { onStop: vi.fn() } }, + { runConfigOverrides: { max_turns: 3 } }, + ])('rejects unsupported options %j before factory', async (options) => { + const create = vi.fn(); + vi.spyOn(mockConfig, 'getExternalAgentExecutor').mockReturnValue({ + create, + }); + await expect( + manager.createAgentHeadless(executorConfig, mockConfig, options), + ).rejects.toThrow(/does not support/); + expect(create).not.toHaveBeenCalled(); + expect(mockCreateContentGenerator).not.toHaveBeenCalled(); + }); + + it('refuses project executables in an untrusted workspace', async () => { + const create = vi.fn(); + vi.spyOn(mockConfig, 'getExternalAgentExecutor').mockReturnValue({ + create, + }); + vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(false); + await expect( + manager.createAgentHeadless( + { ...executorConfig, level: 'project' }, + mockConfig, + ), + ).rejects.toThrow(/untrusted project/); + expect(create).not.toHaveBeenCalled(); + }); + + it('refuses an external executor in safe mode even in a trusted folder (R8-2)', async () => { + const create = vi.fn(); + vi.spyOn(mockConfig, 'getExternalAgentExecutor').mockReturnValue({ + create, + }); + vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(true); + vi.spyOn(mockConfig, 'isSafeMode').mockReturnValue(true); + // Safe mode promises only built-in subagents and no repo-supplied + // execution. Discovery filtering does not stop loadSubagent resolving a + // repo-shipped executor definition from disk, so the dispatch gate must + // refuse it — even at project level in a trusted folder. + await expect( + manager.createAgentHeadless( + { ...executorConfig, level: 'project' }, + mockConfig, + ), + ).rejects.toThrow(/safe mode/); + expect(create).not.toHaveBeenCalled(); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + }); + + it('preserves external factory errors without AgentHeadless labeling', async () => { + const error = new Error('spawn ENOENT'); + vi.spyOn(mockConfig, 'getExternalAgentExecutor').mockReturnValue({ + create: vi.fn().mockRejectedValue(error), + }); + await expect( + manager.createAgentHeadless(executorConfig, mockConfig), + ).rejects.toBe(error); + }); + + it('composes external disposal and propagates its error', async () => { + const error = new Error('dispose failed'); + const dispose = vi.fn().mockRejectedValue(error); + const create = vi.fn().mockResolvedValue({ dispose }); + vi.spyOn(mockConfig, 'getExternalAgentExecutor').mockReturnValue({ + create, + }); + const result = await manager.createAgentHeadless( + { ...executorConfig, model: 'inherit' }, + mockConfig, + { + modelConfigOverrides: {}, + runConfigOverrides: { max_time_minutes: 2 }, + }, + ); + expect(create.mock.calls[0][0]).toMatchObject({ + modelConfig: {}, + runConfig: { max_time_minutes: 2 }, + toolConfig: { disallowedTools: [ToolNames.ASK_USER_QUESTION] }, + }); + expect(mockCreateContentGenerator).not.toHaveBeenCalled(); + await expect(result.dispose()).rejects.toBe(error); + expect(dispose).toHaveBeenCalledOnce(); + }); + + it('derives the peer permission mode from the host-resolved approval policy, not the raw definition', async () => { + const create = vi.fn().mockResolvedValue({}); + vi.spyOn(mockConfig, 'getExternalAgentExecutor').mockReturnValue({ + create, + }); + // The definition asks for the most permissive mode; the host has + // clamped the effective policy to DEFAULT (resolveSubagentApprovalMode + // stamps that onto the runtimeContext the Agent tool hands in). + vi.spyOn(mockConfig, 'getApprovalMode').mockReturnValue( + ApprovalMode.DEFAULT, + ); + await manager.createAgentHeadless( + { ...executorConfig, approvalMode: 'yolo' }, + mockConfig, + ); + // The executor must receive the host's clamped policy, so a definition + // cannot escalate the external agent past the parent session's limit. + expect(create.mock.calls[0]![0]).toMatchObject({ + approvalMode: ApprovalMode.DEFAULT, + }); + }); + + it('re-validates the executor block at the consumption point', async () => { + // Session-level subagents are injected as plain objects and spread + // verbatim by loadSessionSubagents, bypassing frontmatter parsing — so + // an arbitrarily shaped executor can reach the dispatch. + const injected = { + ...executorConfig, + executor: { kind: 'acp', command: ' ' }, + } as unknown as SubagentConfig; + + await expect( + manager.createAgentHeadless(injected, mockConfig), + ).rejects.toThrow(/failed validation/); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + }); + + it('dispatches to the registered executor with the validated spec', async () => { + const externalSubagent = { execute: vi.fn() }; + const create = vi.fn().mockResolvedValue(externalSubagent as never); + vi.spyOn(mockConfig, 'getExternalAgentExecutor').mockReturnValue({ + create, + }); + + const result = await manager.createAgentHeadless( + executorConfig, + mockConfig, + ); + + expect(result.subagent).toBe(externalSubagent); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); + expect(create.mock.calls[0][0]).toMatchObject({ + spec: { kind: 'acp', command: 'npx', args: ['-y', 'some-acp'] }, + name: 'external-agent', + }); + }); + + it('leaves the in-process path untouched when no executor is declared', async () => { + mockAgentHeadlessCreate.mockResolvedValue({ + execute: vi.fn(), + } as never); + const create = vi.fn(); + vi.spyOn(mockConfig, 'getExternalAgentExecutor').mockReturnValue({ + create, + } as never); + + await manager.createAgentHeadless( + { ...executorConfig, executor: undefined }, + mockConfig, + ); + + expect(mockAgentHeadlessCreate).toHaveBeenCalled(); + expect(create).not.toHaveBeenCalled(); + }); + }); + describe('createAgentHeadless model override', () => { const agentConfig: SubagentConfig = { name: 'model-test-agent', diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index af2387d6737..87be92fa60b 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -8,6 +8,7 @@ import * as fs from 'fs/promises'; import * as path from 'path'; import * as os from 'os'; import { randomUUID } from 'crypto'; +import { parseDocument } from 'yaml'; import { parse as parseYaml, stringify as stringifyYaml, @@ -33,6 +34,7 @@ import { } from './types.js'; import { SubagentValidator } from './validation.js'; import { AgentHeadless } from '../agents/runtime/agent-headless.js'; +import type { SubagentExecutor } from '../agents/runtime/subagent-executor.js'; import type { AgentEventEmitter, AgentHooks, @@ -58,6 +60,7 @@ import { COLOR_VALUES, isColor, isPermissionMode, + parseAgentExecutor, parseAgentHooks, parseAgentMcpServers, parseMaxTurns, @@ -786,6 +789,18 @@ export class SubagentManager { frontmatter['hooks'] = config.hooks; } + if (config.executor !== undefined) { + const executor = parseAgentExecutor(config.executor); + if (!executor) { + throw new SubagentError( + `Subagent "${config.name}" executor block failed validation. Refusing to save it without its executor.`, + SubagentErrorCode.INVALID_CONFIG, + config.name, + ); + } + frontmatter['executor'] = executor; + } + // Serialize to YAML const yamlContent = stringifyYaml(frontmatter, { lineWidth: 0, // Disable line wrapping @@ -837,7 +852,7 @@ export class SubagentManager { /** Stable id used to keep one invocation grouped across resume. */ subagentId?: string; }, - ): Promise<{ subagent: AgentHeadless; dispose: () => Promise }> { + ): Promise<{ subagent: SubagentExecutor; dispose: () => Promise }> { // Track per-spawn cleanup callbacks declared outside the inner // `try/catch` so the catch can fire them on a constructor failure // before the caller ever receives the return value. The successful @@ -870,6 +885,124 @@ export class SubagentManager { }; try { + if (config.executor !== undefined) { + // Safe mode promises that only built-in subagents are available and + // that no repo-supplied execution surface runs. Discovery filtering is + // not enough: `loadSubagent` resolves a `subagent_type` from disk + // (project/user/extension) regardless of safe mode, and a cloned repo + // can ship `.qwen/agents/.md` carrying an `executor` that + // resolves before the built-in of the same name. Refuse at the dispatch + // gate so safe mode never spawns a definition-supplied binary. + if (runtimeContext.isSafeMode()) { + throw new SubagentError( + `Subagent "${config.name}" declares an external executor, which safe mode forbids: safe mode restricts subagents to the built-in set and never spawns a definition-supplied binary.`, + SubagentErrorCode.INVALID_CONFIG, + config.name, + ); + } + const spec = parseAgentExecutor(config.executor); + if (!spec) { + throw new SubagentError( + `Subagent "${config.name}" executor block failed validation. Refusing to run it in-process.`, + SubagentErrorCode.INVALID_CONFIG, + config.name, + ); + } + if (config.level === 'project' && !runtimeContext.isTrustedFolder()) { + throw new SubagentError( + `Cannot start external agent "${config.name}" from an untrusted project.`, + SubagentErrorCode.INVALID_CONFIG, + config.name, + ); + } + const unsupported = [ + ['tools', config.tools], + ['disallowedTools', config.disallowedTools], + ['mcpServers', config.mcpServers], + ['hooks', config.hooks], + ['maxTurns', config.maxTurns], + ['runConfig.max_turns', config.runConfig?.max_turns], + [ + 'runConfigOverrides.max_turns', + options?.runConfigOverrides?.max_turns, + ], + [ + 'model', + config.model && config.model !== 'inherit' + ? config.model + : undefined, + ], + [ + 'modelConfigOverrides', + Object.keys(options?.modelConfigOverrides ?? {}).length > 0 + ? options?.modelConfigOverrides + : undefined, + ], + ['runtimeAuthOverrides', options?.runtimeAuthOverrides], + ['toolConfigOverride', options?.toolConfigOverride], + [ + 'promptConfigOverrides.renderedSystemPrompt', + options?.promptConfigOverrides?.renderedSystemPrompt, + ], + [ + 'promptConfigOverrides.initialMessages', + options?.promptConfigOverrides?.initialMessages, + ], + ['hooks callbacks', options?.hooks], + ].filter(([, value]) => value !== undefined); + if (unsupported.length > 0) { + throw new SubagentError( + `External agent "${config.name}" does not support ${unsupported.map(([key]) => key).join(', ')}.`, + SubagentErrorCode.INVALID_CONFIG, + config.name, + ); + } + const externalExecutor = runtimeContext.getExternalAgentExecutor(); + if (!externalExecutor) { + throw new SubagentError( + `Subagent "${config.name}" declares an executor but this host registered no external agent executor. Refusing to run it in-process.`, + SubagentErrorCode.INVALID_CONFIG, + config.name, + ); + } + const subagent = await externalExecutor.create({ + spec, + name: config.name, + // The peer permission mode must be the host's effective, already + // clamped approval policy — not the definition's raw request. The + // Agent tool resolves the definition's approvalMode against the + // parent session's mode and folder trust (resolveSubagentApprovalMode) + // and stamps the result onto this runtimeContext, so reading it back + // here is what stops a definition from escalating the external agent + // past the parent session's limit. config.permissionMode is already + // bridged into that resolved value at parse time. + approvalMode: runtimeContext.getApprovalMode(), + permissionMode: config.permissionMode, + promptConfig: { + systemPrompt: config.systemPrompt, + ...options?.promptConfigOverrides, + }, + modelConfig: {}, + runConfig: { + ...config.runConfig, + ...options?.runConfigOverrides, + }, + toolConfig: { + tools: ['*'], + disallowedTools: [ToolNames.ASK_USER_QUESTION], + }, + eventEmitter: options?.eventEmitter, + taskName: options?.taskName, + subagentId: options?.subagentId, + runtimeContext: deriveConfig(runtimeContext), + }); + return { + subagent, + dispose: async () => { + await subagent.dispose?.(); + }, + }; + } const runtimeConfig = await this.convertToRuntimeConfig( config, runtimeContext, @@ -978,6 +1111,12 @@ export class SubagentManager { throw innerError; } } catch (error) { + // Already-classified errors carry an accurate message; re-wrapping them + // under "Failed to create AgentHeadless" would misreport the executor + // path, which never constructs an AgentHeadless at all. + if (error instanceof SubagentError || config.executor !== undefined) { + throw error; + } if (error instanceof Error) { throw new SubagentError( `Failed to create AgentHeadless: ${error.message}`, @@ -1261,6 +1400,13 @@ export class SubagentManager { config: SubagentConfig, runtimeContext?: Config, ): Promise { + if (config.executor !== undefined) { + throw new SubagentError( + `Subagent "${config.name}" declares an external executor and cannot be converted to an in-process agent.`, + SubagentErrorCode.INVALID_CONFIG, + config.name, + ); + } const promptConfig: PromptConfig = { systemPrompt: config.systemPrompt, }; @@ -1734,6 +1880,130 @@ function parseSubagentContent( ); } + // executor: qwen-code extension (not part of the mirrored CC schema). + // Strictly validated because it names an external process to run; see + // `parseAgentExecutor`. Availability of the named command is NOT checked + // here — that is the injected executor's job at spawn time. + // + // A malformed block is a hard error, NOT a lenient drop. Dropping it would + // leave `config.executor` undefined, so the definition would run + // in-process: the task completes under Qwen's model, billed to the Qwen + // provider, with nothing on stdout or stderr — precisely the silent + // substitution this feature exists to prevent, and the consumption-point + // re-validation can never catch it because the field is already gone. The + // warn-only path was invisible in practice too, since `debugLogger.warn` + // is a no-op unless QWEN_DEBUG_LOG_FILE is set. This deliberately diverges + // from the lenient posture used for `mcpServers` / `hooks`: losing those + // degrades a capability, whereas losing this one substitutes a different + // agent for the one the definition asked for. + // The shared parser strips nulls, including null arguments. Validate the + // original YAML node so sanitization cannot change the executable request. + const document = parseDocument(frontmatterYaml); + const hasExecutor = document.has('executor'); + // parseDocument repairs a syntactically invalid document instead of + // throwing, and `document.errors` is the only signal that it did so. For an + // executor-bearing definition that is unsafe: the repaired node can dispatch + // a different command/args than the file declares, or drop the key and run + // in-process. Refuse it and surface the real YAML error (with line/column). + // + // Scope this to executor-bearing files only. Every OTHER definition must + // keep loading through the shared parser's lenient `parseSimple` fallback — + // a description containing a colon, a tab-indented field, or a key + // duplicated by a bad merge loads at the merge base, and refusing it here + // would silently delete an agent from `/agents` (and break + // `subagent_type:`) with nothing on stdout or stderr. Detect the claim from + // the raw text too, so a YAML error that corrupts the `executor` key out of + // the AST still cannot slip through to a silent in-process run. The text + // probe is anchored to a TOP-LEVEL key (column 0, optional quotes) on + // purpose: an indented `executor:` nested under `mcpServers:` or written + // inside a `description: |` block scalar declares no executor, and matching + // it would hard-refuse an unrelated pre-existing definition; while a quoted + // top-level `"executor":` IS a declaration that `parseSimple` (keeps the + // quotes in the key) and a repaired `parseDocument` (nests it) both miss, so + // it must be caught here or the file loads executor-less and runs in-process. + const executorClaimMatch = /^["']?executor["']?[ \t]*:/m.exec( + frontmatterYaml, + ); + const claimsExecutor = + hasExecutor || + frontmatter['executor'] !== undefined || + executorClaimMatch !== null; + // Refuse only when an error can actually corrupt the executor node, not on + // every tolerated YAML error in the file. Two distinct dangers: (1) the real + // YAML parser (parseDocument) lost the executor node (!hasExecutor) although + // the file claims one; do NOT fall back to the lenient parseSimple value, a + // line-based heuristic that mangles block scalars and sequences (command: | + // plus an indented npx becomes command '|', spawning an executable the file + // never declared), and YAML errors are not line-local, so an earlier + // compact-mapping error can drop the whole remainder including executor; + // (2) the AST + // has the executor but an error sits at or after its line, so parseDocument + // may have repaired the node into a different command/args than the file + // declares. An error strictly BEFORE the executor line — a duplicate `name:` + // from a bad merge, say — cannot have reached the executor subtree that + // parseDocument kept (hasExecutor still true), which stays byte-faithful, so + // refusing there would delete a valid external-agent + // definition over an unrelated quirk (it still loads at the merge base via + // the lenient `parseSimple` fallback). When the claim cannot be localized to + // a line, fail closed and treat every error as reaching it. + const executorLine = + executorClaimMatch === null + ? undefined + : frontmatterYaml.slice(0, executorClaimMatch.index).split('\n').length; + if (claimsExecutor && document.errors.length > 0) { + const astLostExecutor = !hasExecutor; + const reachingError = document.errors.find( + (error) => + executorLine === undefined || + error.linePos === undefined || + error.linePos[0].line >= executorLine, + ); + if (astLostExecutor || reachingError !== undefined) { + throw new SubagentError( + `Agent file ${filePath} has an invalid executor block: its frontmatter ` + + `YAML does not parse (${(reachingError ?? document.errors[0]).message}). ` + + `Refusing to load the definition rather than dispatch a silently ` + + `repaired executor.`, + SubagentErrorCode.INVALID_CONFIG, + name, + ); + } + } + // parseDocument tolerates an unresolved YAML alias (e.g. `command: *undef`) + // without recording a `document.errors` entry, but `toJS()` throws when it + // resolves the node — so the errors-based guard above cannot catch it. Refuse + // gracefully rather than let a raw YAML error escape parseSubagentContent. + let executorRaw: unknown; + if (hasExecutor) { + let documentJs: unknown; + try { + documentJs = document.toJS(); + } catch (error) { + throw new SubagentError( + `Agent file ${filePath} has an invalid executor block: its frontmatter ` + + `YAML does not resolve (${error instanceof Error ? error.message : String(error)}). ` + + `Refusing to load the definition rather than dispatch an executor ` + + `parsed from YAML that will not resolve to a document.`, + SubagentErrorCode.INVALID_CONFIG, + name, + ); + } + executorRaw = (documentJs as Record)['executor']; + } else { + executorRaw = frontmatter['executor']; + } + const executor = parseAgentExecutor(executorRaw); + if ((hasExecutor || executorRaw !== undefined) && executor === undefined) { + throw new SubagentError( + `Agent file ${filePath} has an invalid executor block (expected ` + + `{ kind: 'acp', command: string, args?: string[] }). Refusing to load ` + + `the definition: dropping the block would silently run it in-process ` + + `instead of in the external agent it asked for.`, + SubagentErrorCode.INVALID_CONFIG, + name, + ); + } + const config: SubagentConfig = { name, description, @@ -1751,6 +2021,7 @@ function parseSubagentContent( ...(maxTurns !== undefined ? { maxTurns } : {}), ...(mcpServers !== undefined ? { mcpServers } : {}), ...(hooks !== undefined ? { hooks } : {}), + ...(executor !== undefined ? { executor } : {}), }; // Validate the parsed configuration @@ -1776,5 +2047,13 @@ function parseSubagentContent( */ function warnInvalidSubagentFile(filePath: string, error: unknown): void { const message = error instanceof Error ? error.message : String(error); + if ( + error instanceof SubagentError && + message.includes('invalid executor block') + ) { + // eslint-disable-next-line no-console -- executor rejection must be visible without debug logging + console.warn(`Skipped invalid file ${filePath}: ${message}`); + return; + } debugLogger.debug(`Skipped invalid file ${filePath}: ${message}`); } diff --git a/packages/core/src/subagents/types.ts b/packages/core/src/subagents/types.ts index ea4c6bf9a30..6dd79e29b11 100644 --- a/packages/core/src/subagents/types.ts +++ b/packages/core/src/subagents/types.ts @@ -43,6 +43,26 @@ export type SubagentLevel = | 'extension' | 'builtin'; +/** + * Declares that a subagent's turn is executed by an external agent process + * speaking ACP, rather than by the in-process reasoning loop. + * + * `parseAgentExecutor` validates this strictly: an unrecognized `kind`, a + * missing/blank `command`, or a malformed `args` yields `undefined`. The + * frontmatter loader then **rejects the whole definition** rather than dropping + * the field — a dropped field would leave the definition running in-process, + * silently substituting a different agent for the one it asked for. This is a + * deliberate divergence from the lenient drop used for `mcpServers` and `hooks`. + */ +export interface SubagentExecutorSpec { + /** Executor kind. `acp` is the only supported value today. */ + kind: 'acp'; + /** Executable to run, resolved on PATH. */ + command: string; + /** Arguments passed to `command`. Omitted when the definition declares none. */ + args?: string[]; +} + /** * Core configuration for a subagent as stored in Markdown files. * This interface represents the file-based configuration that gets @@ -161,6 +181,22 @@ export interface SubagentConfig { */ hooks?: Record; + /** + * Optional external executor. When present, the subagent's turn is run by an + * external agent process instead of the in-process `AgentCore` reasoning + * loop; `SubagentManager.createAgentHeadless` dispatches to the executor + * injected via `Config.setExternalAgentExecutor`. + * + * Absent (the default) keeps the in-process executor. A definition that + * declares an executor while none is injected fails loudly rather than + * silently falling back to in-process. + * + * `command` is resolved on PATH. Project-level executors require a trusted + * workspace. Declared host tool restrictions, MCP servers, hooks and + * model/provider overrides are unsupported and rejected before spawning. + */ + executor?: SubagentExecutorSpec; + /** * Indicates whether this is a built-in agent. * Built-in agents cannot be modified or deleted. diff --git a/packages/core/src/tools/agent/agent.test.ts b/packages/core/src/tools/agent/agent.test.ts index 731cedd4180..e1dc0db129f 100644 --- a/packages/core/src/tools/agent/agent.test.ts +++ b/packages/core/src/tools/agent/agent.test.ts @@ -6073,6 +6073,85 @@ describe('AgentTool', () => { }); }); + it.each([true, false])( + 'omits parent attribution and unknown usage for external agents (background=%s)', + async (background) => { + const writeMetaSpy = vi.spyOn(transcript, 'writeAgentMeta'); + vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue({ + ...bgSubagent, + executor: { kind: 'acp', command: 'claude' }, + }); + const invocation = ( + agentTool as AgentToolWithProtectedMethods + ).createInvocation({ + description: 'External task', + prompt: 'Do the task', + subagent_type: 'monitor', + run_in_background: background, + }); + const result = await invocation.execute(); + expect(partToString(result.llmContent)).toContain( + background ? 'Background agent launched' : 'Monitor done', + ); + const meta = writeMetaSpy.mock.calls.at(-1)?.[1]; + expect(meta?.executor).toBe('acp'); + expect(meta?.persistedCliFlags).toBeUndefined(); + expect(meta?.model).toBeUndefined(); + expect( + (result.returnDisplay as AgentResultDisplay).executionSummary, + ).toBeUndefined(); + if (background) { + await vi.waitFor(() => + expect(mockRegistry.complete).toHaveBeenCalled(), + ); + expect(mockRegistry.complete.mock.calls[0]?.[2]).toBeUndefined(); + expect(mockRegistry.complete.mock.calls[0]?.[1]).toContain( + 'token usage and cost are unavailable', + ); + expect(mockRegistry.tryReserveBackgroundSlot).toHaveBeenCalledWith( + undefined, + null, + ); + const resident = mockRegistry.registerResidentAgent.mock + .calls[0]?.[1] as { + continue: (message: string) => boolean; + }; + expect(resident.continue('Continue externally')).toBe(true); + await vi.waitFor(() => + expect(mockAgent.execute).toHaveBeenCalledTimes(2), + ); + expect(mockSubagentManager.createAgentHeadless).toHaveBeenCalledTimes( + 1, + ); + } + }, + ); + + it('publishes the real failure reason, not just the usage notice, for a background external agent that produced no text', async () => { + vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue({ + ...bgSubagent, + executor: { kind: 'acp', command: 'claude' }, + }); + vi.mocked(mockAgent.getTerminateMode).mockReturnValue( + AgentTerminateMode.TIMEOUT, + ); + vi.mocked(mockAgent.getFinalText).mockReturnValue(''); + const invocation = ( + agentTool as AgentToolWithProtectedMethods + ).createInvocation({ + description: 'External task', + prompt: 'Do the task', + subagent_type: 'monitor', + run_in_background: true, + }); + await invocation.execute(); + await vi.waitFor(() => expect(mockRegistry.fail).toHaveBeenCalled()); + const failureMessage = mockRegistry.fail.mock.calls[0]?.[1] as string; + // The notice must be appended AFTER the fallback, never in place of it. + expect(failureMessage).toContain('Agent terminated with mode: TIMEOUT'); + expect(failureMessage).toContain('token usage and cost are unavailable'); + }); + it('should run in background when agent definition has background: true', async () => { const writeMetaSpy = vi.spyOn(transcript, 'writeAgentMeta'); const attachSpy = vi.spyOn(transcript, 'attachJsonlTranscriptWriter'); diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index 493acc255f2..8cc7c0bad44 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -34,6 +34,7 @@ import { AgentHeadless, ContextState, } from '../../agents/runtime/agent-headless.js'; +import type { SubagentExecutor } from '../../agents/runtime/subagent-executor.js'; import type { AgentExternalInput } from '../../agents/runtime/agent-types.js'; import type { Content } from '@google/genai'; import { @@ -133,6 +134,9 @@ import type { import { buildModelIdContext, resolveModelId } from '../../utils/modelId.js'; import type { AuthOverrides } from '../../models/content-generator-config.js'; +const EXTERNAL_USAGE_NOTICE = + '\n\n[External executor token usage and cost are unavailable.]'; + function persistBackgroundCancellation( metaPath: string, persistedStatus: 'running' | 'cancelled', @@ -1680,7 +1684,7 @@ class AgentToolInvocation extends BaseToolInvocation { eventEmitter: AgentEventEmitter = this.eventEmitter, subagentId?: string, ): Promise<{ - subagent: AgentHeadless; + subagent: SubagentExecutor; initialMessages?: Content[]; taskPrompt: string; toolConfig: ToolConfig; @@ -1869,7 +1873,7 @@ class AgentToolInvocation extends BaseToolInvocation { // the reason back and re-executes until the configured cap prevents a // misconfigured hook from looping forever. private async runSubagentStopHookLoop( - subagent: AgentHeadless, + subagent: SubagentExecutor, opts: { agentId: string; agentType: string; @@ -2091,12 +2095,13 @@ class AgentToolInvocation extends BaseToolInvocation { * as execution progresses. */ private async runSubagentWithHooks( - subagent: AgentHeadless, + subagent: SubagentExecutor, contextState: ContextState, opts: { agentId: string; agentType: string; resolvedMode: PermissionMode; + externalExecutor?: boolean; signal?: AbortSignal; updateOutput?: (output: ToolResultDisplay) => void; /** @@ -2158,7 +2163,9 @@ class AgentToolInvocation extends BaseToolInvocation { stopHookWarning, ); const success = terminateMode === AgentTerminateMode.GOAL; - const executionSummary = subagent.getExecutionSummary(); + const executionSummary = opts.externalExecutor + ? undefined + : subagent.getExecutionSummary(); // Publish span outcome BEFORE side-effectful UI/registry calls — if // updateDisplay throws, the subagent's real terminal state must @@ -2719,31 +2726,33 @@ class AgentToolInvocation extends BaseToolInvocation { } if (shouldRunInBackground) { - // Resolve the concrete model the sub-agent (or fork) will run with so the - // registry can apply a per-model cap. `subagentConfig.model` is a - // selector (omitted/"inherit"/"fast"/modelId/authType:modelId); - // resolveModelId maps it to the actual model ID, falling back to the - // parent's current model when the sub-agent inherits (forks always - // inherit, since FORK_AGENT has no model selector). - const resolvedSubagentModel = resolveModelId( - subagentConfig.model, - buildModelIdContext(this.config), - ); - subagentModelId = resolvedSubagentModel?.modelId; - subagentModelId ??= this.config.getModel(); - const parentContentGeneratorConfig = - this.config.getContentGeneratorConfig(); - const authType = - resolvedSubagentModel?.authType ?? - parentContentGeneratorConfig.authType; - subagentRuntimeAuthOverrides = authType - ? { - authType, - ...(authType === parentContentGeneratorConfig.authType - ? { baseUrl: parentContentGeneratorConfig.baseUrl } - : {}), - } - : undefined; + if (subagentConfig.executor === undefined) { + // Resolve the concrete model the sub-agent (or fork) will run with so the + // registry can apply a per-model cap. `subagentConfig.model` is a + // selector (omitted/"inherit"/"fast"/modelId/authType:modelId); + // resolveModelId maps it to the actual model ID, falling back to the + // parent's current model when the sub-agent inherits (forks always + // inherit, since FORK_AGENT has no model selector). + const resolvedSubagentModel = resolveModelId( + subagentConfig.model, + buildModelIdContext(this.config), + ); + subagentModelId = resolvedSubagentModel?.modelId; + subagentModelId ??= this.config.getModel(); + const parentContentGeneratorConfig = + this.config.getContentGeneratorConfig(); + const authType = + resolvedSubagentModel?.authType ?? + parentContentGeneratorConfig.authType; + subagentRuntimeAuthOverrides = authType + ? { + authType, + ...(authType === parentContentGeneratorConfig.authType + ? { baseUrl: parentContentGeneratorConfig.baseUrl } + : {}), + } + : undefined; + } const registry = this.config.getBackgroundTaskRegistry(); backgroundSlotReservation = registry.tryReserveBackgroundSlot( subagentModelId, @@ -2996,6 +3005,7 @@ class AgentToolInvocation extends BaseToolInvocation { // rows, and the meta sidecar all read this field. agentType: subagentConfig.name, resolvedMode, + externalExecutor: subagentConfig.executor !== undefined, signal, updateOutput, }; @@ -3020,7 +3030,7 @@ class AgentToolInvocation extends BaseToolInvocation { // Create the subagent. Fork bypasses SubagentManager because its runtime // configs are synthesized from the parent's cache-safe params. - let subagent: AgentHeadless; + let subagent: SubagentExecutor; let taskPrompt: string; let initialMessages: Content[] | undefined; let toolConfig: ToolConfig | undefined; @@ -3295,13 +3305,17 @@ class AgentToolInvocation extends BaseToolInvocation { executionAllowedTools: [...bgToolConfig.executionAllowedTools], } : {}), - persistedCliFlags: capturePersistedCliFlags( - this.config, - resolvedApprovalMode, - bgSubagent.getCore().modelConfig.model, - bgSubagent.getCore().runtimeView?.contentGeneratorConfig ?? - subagentRuntimeAuthOverrides, - ), + executor: subagentConfig.executor?.kind, + persistedCliFlags: + subagentConfig.executor !== undefined + ? undefined + : capturePersistedCliFlags( + this.config, + resolvedApprovalMode, + bgSubagent.getCore().modelConfig.model, + bgSubagent.getCore().runtimeView?.contentGeneratorConfig ?? + subagentRuntimeAuthOverrides, + ), subagentName: subagentConfig.name, agentColor: subagentConfig.color, resumeCount: 0, @@ -3326,7 +3340,12 @@ class AgentToolInvocation extends BaseToolInvocation { let liveToolCallCount = 0; const refreshLiveStats = () => { const entry = registry.get(hookOpts.agentId); - if (!entry || entry.status !== 'running') return; + if ( + !entry || + entry.status !== 'running' || + subagentConfig.executor !== undefined + ) + return; const summary = bgSubagent.getExecutionSummary(); entry.stats = { totalTokens: summary.totalTokens, @@ -3379,6 +3398,9 @@ class AgentToolInvocation extends BaseToolInvocation { ); const getCompletionStats = () => { + // The shared summary requires known token counts. Keep external + // usage absent rather than describing an unmetered run as free. + if (subagentConfig.executor !== undefined) return undefined; const summary = bgSubagent.getExecutionSummary(); return { totalTokens: summary.totalTokens, @@ -3556,18 +3578,27 @@ class AgentToolInvocation extends BaseToolInvocation { const wtSuffix = formatWorktreeSuffix( hadWorktreeIsolation ? await cleanupWorktreeIsolation() : {}, ); + // The usage notice is a suffix, not part of the model-visible + // text: baking it into finalText would make the `finalText || + // ` fallbacks below see a non-empty string and publish + // the notice in place of the real failure reason for any + // non-GOAL external run that produced no text. The foreground + // path already appends it after its fallbacks; mirror that. + const externalSuffix = + subagentConfig.executor !== undefined + ? EXTERNAL_USAGE_NOTICE + : ''; const modelVisibleText = toModelVisibleSubagentResult( subagentRawText, terminateMode, ); - const finalText = - appendStopHookBlockingCapWarning( - terminateMode === AgentTerminateMode.GOAL - ? modelVisibleText || - '(subagent produced no model-visible output)' - : modelVisibleText, - stopHookWarning, - ) + wtSuffix; + const finalText = appendStopHookBlockingCapWarning( + terminateMode === AgentTerminateMode.GOAL + ? modelVisibleText || + '(subagent produced no model-visible output)' + : modelVisibleText, + stopHookWarning, + ); const completionStats = getCompletionStats(); if ( terminateMode === AgentTerminateMode.GOAL && @@ -3610,7 +3641,11 @@ class AgentToolInvocation extends BaseToolInvocation { ) : {}), }); - registry.complete(hookOpts.agentId, finalText, completionStats); + registry.complete( + hookOpts.agentId, + finalText + wtSuffix + externalSuffix, + completionStats, + ); } else if ( terminateMode === AgentTerminateMode.CANCELLED || terminateMode === AgentTerminateMode.SHUTDOWN @@ -3622,7 +3657,7 @@ class AgentToolInvocation extends BaseToolInvocation { // wenshao @ #4410. registry.finalizeCancelled( hookOpts.agentId, - finalText, + finalText + wtSuffix + externalSuffix, completionStats, ); persistBackgroundCancellation( @@ -3634,16 +3669,16 @@ class AgentToolInvocation extends BaseToolInvocation { registry.get(hookOpts.agentId)?.recentActivities, ); } else { - registry.fail( - hookOpts.agentId, - finalText || `Agent terminated with mode: ${terminateMode}`, - completionStats, - ); + const failureText = + (finalText || + `Agent terminated with mode: ${terminateMode}`) + + wtSuffix + + externalSuffix; + registry.fail(hookOpts.agentId, failureText, completionStats); patchAgentMeta(metaPath, { status: 'failed', lastUpdatedAt: new Date().toISOString(), - lastError: - finalText || `Agent terminated with mode: ${terminateMode}`, + lastError: failureText, ...(sessionWorkflowAgent ? getAgentMetaTerminalSummary( completionStats, @@ -4041,7 +4076,12 @@ class AgentToolInvocation extends BaseToolInvocation { let fgLiveToolCallCount = 0; const refreshFgLiveStats = () => { const entry = registry.get(hookOpts.agentId); - if (!entry || entry.status !== 'running') return; + if ( + !entry || + entry.status !== 'running' || + subagentConfig.executor !== undefined + ) + return; const summary = subagent.getExecutionSummary(); entry.stats = { totalTokens: summary.totalTokens, @@ -4144,11 +4184,15 @@ class AgentToolInvocation extends BaseToolInvocation { executionAllowedTools: [...toolConfig.executionAllowedTools], } : {}), - persistedCliFlags: capturePersistedCliFlags( - this.config, - resolvedApprovalMode, - subagent.getCore().modelConfig.model, - ), + executor: subagentConfig.executor?.kind, + persistedCliFlags: + subagentConfig.executor !== undefined + ? undefined + : capturePersistedCliFlags( + this.config, + resolvedApprovalMode, + subagent.getCore().modelConfig.model, + ), subagentName: subagentConfig.name, agentColor: subagentConfig.color, resumeCount: 0, @@ -4163,7 +4207,9 @@ class AgentToolInvocation extends BaseToolInvocation { toModelVisibleSubagentResult(subagent.getFinalText(), terminateMode), stopHookWarning, ); - const wtSuffix = formatWorktreeSuffix(await cleanupWorktreeIsolation()); + const wtSuffix = + formatWorktreeSuffix(await cleanupWorktreeIsolation()) + + (subagentConfig.executor !== undefined ? EXTERNAL_USAGE_NOTICE : ''); if (terminateMode === AgentTerminateMode.ERROR) { return { llmContent: (finalText || 'Subagent execution failed.') + wtSuffix, diff --git a/scripts/tests/cross-package-contracts.test.js b/scripts/tests/cross-package-contracts.test.js index ce3afc86f8a..a04114c61be 100644 --- a/scripts/tests/cross-package-contracts.test.js +++ b/scripts/tests/cross-package-contracts.test.js @@ -112,3 +112,34 @@ it('distinguishes locale resolution from prompt sanitization', () => { definitionFiles('^export function getSanitizedExtensionDisplayName[(]'), ).toEqual(['packages/cli/src/utils/extension-mention.ts']); }); + +it('publishes the external subagent runtime through its declared subpath', () => { + const pkg = JSON.parse( + readFileSync(join(root, 'packages/core/package.json'), 'utf8'), + ); + expect(pkg.exports['./subagentRuntime']).toEqual({ + types: './dist/src/subagent-runtime.d.ts', + import: './dist/src/subagent-runtime.js', + }); + const barrel = readFileSync( + join(root, 'packages/core/src/subagent-runtime.ts'), + 'utf8', + ); + expect(barrel).toContain("from './agents/runtime/subagent-executor.js'"); + expect(barrel).toContain('ExternalAgentExecutor'); + expect(barrel).toContain('AgentEventEmitter'); + const aliases = readFileSync( + join(root, 'packages/cli/vitest.config.ts'), + 'utf8', + ); + expect(aliases).toMatch( + /'@qwen-code\/qwen-code-core\/subagentRuntime'\s*:\s*path\.resolve\(\s*__dirname,\s*'\.\.\/core\/src\/subagent-runtime\.ts',?\s*\)/, + ); + const tsconfig = readFileSync( + join(root, 'packages/cli/tsconfig.json'), + 'utf8', + ); + expect(tsconfig).toMatch( + /"@qwen-code\/qwen-code-core\/subagentRuntime"\s*:\s*\[\s*"\.\.\/core\/src\/subagent-runtime\.ts"\s*\]/, + ); +}); From c6138f163d7a52ea8530cd00af9c16d040c781b0 Mon Sep 17 00:00:00 2001 From: probe Date: Wed, 9 Sep 2026 16:51:17 +0800 Subject: [PATCH 2/5] fix(subagent): close the round-10 review Criticals on the external executor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five Criticals from the round-10 review of the external-agent executor and its definition loader. Each is mutation-verified (reverting the fix turns its test red) and the surrounding guarantees are pinned. R10-1 — the executor-frontmatter parser derived both the claim and the value from a column-0 regex plus the lenient `parseSimple` fallback instead of the real YAML AST, which got the verdict wrong in both directions. Under-detection: a TAB-indented top-level `executor:` is invalid YAML the AST drops, and a column-0 probe missed it, so the definition loaded executor-less and ran in-process under a Qwen model with nothing on any channel. Invented dispatch: `parseSimple` hoists an `executor:` line sitting inside a `description: |` block scalar into a top-level key, so a file declaring no executor loaded as external and dispatched a command that exists only as prose. Both legs now come from the AST: the claim probe is indentation-tolerant (`^[ \t]*`), `claimsExecutor` no longer reads `frontmatter['executor']`, and `executorRaw` is `undefined` (never the `parseSimple` value) when `parseDocument` has no top-level executor node. The accepted cost — an `executor:` token nested under another key in an otherwise-malformed file is now refused — is a visible, user-fixable over-refusal, which beats an invisible substitution or an invented dispatch. R10-2 — the load-time executor refusals were file *skips*, so `loadSubagent`'s session>project>user>extension>builtin fall-through substituted a lower-precedence in-process definition (or a case-insensitive builtin) of the same name and the delegated turn ran under a Qwen model — the exact substitution this feature prevents, with only a discovery-time `console.warn`. Executor-block refusals are now recorded by declared name during the level scan (the `parseSubagentContent` re-wrap preserves the `SubagentError` and its `subagentName`), and the by-name fall-through path throws the recorded refusal before descending or reaching `getBuiltinAgent`. Scoped to executor refusals (not parse failures generally) so an arbitrary malformed repo file cannot disable an unrelated builtin, and scoped to the no-level fall-through path (the explicit -level path does not fall through, so it cannot substitute) to keep the management callers' contract unchanged; `isNameAvailable` treats a refusal as "name taken". R10-3 — `isExpectedExternalAgentCleanupExit` swallowed acp-bridge's `exited before its initial process-tree snapshot completed`, which `mergeAsynchronousSnapshot` records and returns BEFORE `collectOwnership`, so a detached (`setsid`) descendant was never enumerated, signalled or reaped — the tree was not proven gone, yet `dispose()` resolved silently and the parent saw a clean teardown. That shape is now `isUnprovenExternalAgentTreeExit`: `dispose()` reports it (`debugLogger.warn` + an `AgentEventType.ERROR`) and resolves rather than rethrowing (it is awaited between terminal-state classification and return, so rethrowing would convert a classified TIMEOUT/CANCELLED into a thrown ERROR). `isExpectedExternalAgentCleanupExit` is narrowed to the peer's own unclean exit (raised only once the tree is proven gone); every other cleanup-proof failure still propagates. R10-4 — `runTurn` destructively drained the external-input queue and recorded the messages as delivered before the round-top budget check, so a wall-time expiry (or a bottom-of-loop abort) discarded user messages already emitted as delivered — the transcript certified delivery of a message never sent, and the registry had already spliced it out. The drain is now gated on the budget and the abort signal first, mirroring the in-process sibling (`agent-core` checks the budget before `drainExternalInputs`): when the budget is spent or the signal aborted, the loop breaks before draining, so the message stays queued for a future turn. R10-5 — an absent `max_time_minutes` silently meant "10 minutes then terminate" for an external agent, while meaning "no cap" everywhere else, so the same definition behaved differently based only on whether it declared an executor, and a TIMEOUT turn (handed to the parent as the answer with no truncation marker) was reachable by default. The cited rationale was void: `DEFAULT_WORKFLOW_SUBAGENT_ MAX_TIME_MINUTES` is read only on the workflow path, which hard-rejects external executors upstream, so it never reaches this executor. An absent value is now passed through as `undefined` (no timer), matching `agent-core`; a caller that wants a cap supplies `max_time_minutes` (validated positive, finite, ≤ the Node timer range). Verification: core and cli `tsc` 0 errors (after a full `npm run build` to refresh the workspace `dist` the merge had left stale); ESLint and Prettier clean; `git diff --check` clean; cli executor suite 50/50; core subagent/agent/runtime suites 2510 passed (6 skipped); cross-package contracts and the two core-export sync guards 17/17. Mutation proofs, each run in both directions: reinstating `?? 10` fails the no-default-cap test; removing the pre-drain budget/abort gate fails the queue-preserved test (the message is drained, recorded delivered, and lost); making `dispose()` swallow the snapshot-race error fails the unproven-tree-report test; reverting the claim probe to column-0 anchoring fails the nested-token refusal test; restoring the `parseSimple` fallback for `executorRaw` fails the block-scalar-prose test (it loads `command:'npx'` from prose); and no-oping the recorded-refusal throw fails the by-name dispatch test (it resolves the builtin instead of rejecting). Reverse-audit converged after one fix (scoping the R10-2 throw to the fall-through path so the explicit-level management callers keep their null contract), then clean passes over the production and test diffs. --- .../acp-subagent-executor.test.ts | 167 +++++++++++++----- .../external-agents/acp-subagent-executor.ts | 129 +++++++++++--- .../src/subagents/subagent-manager.test.ts | 74 ++++++-- .../core/src/subagents/subagent-manager.ts | 102 +++++++++-- 4 files changed, 378 insertions(+), 94 deletions(-) diff --git a/packages/cli/src/external-agents/acp-subagent-executor.test.ts b/packages/cli/src/external-agents/acp-subagent-executor.test.ts index 7ce7bd9e3f5..d39d1db22c5 100644 --- a/packages/cli/src/external-agents/acp-subagent-executor.test.ts +++ b/packages/cli/src/external-agents/acp-subagent-executor.test.ts @@ -19,6 +19,7 @@ import { acpExternalAgentExecutor, externalModelLabel, isExpectedExternalAgentCleanupExit, + isUnprovenExternalAgentTreeExit, optionKindForOutcome, resolvePermissionMode, selectPermissionOption, @@ -241,20 +242,11 @@ describe('permission mapping', () => { }); describe('cleanup-exit classification', () => { - it('treats a foreign root that died during disposal as expected', () => { - // The Linux race: the prompt rejects on stdout EOF, dispose() enters the - // registry's initial tree snapshot, and the root exits mid-snapshot. - expect( - isExpectedExternalAgentCleanupExit( - new Error( - 'ACP child pid=2628870 exited before its initial process-tree snapshot completed', - ), - ), - ).toBe(true); + it("tolerates the peer's own unclean exit but not the unproven-tree race", () => { // The peer's own exit status, for ANY code/signal: acp-bridge raises this - // only after the tree is proven gone, so it reports the foreign agent's - // exit, not a cleanup failure (R7-2). Letting dispose() reject on it would - // override the turn's classified terminal state. + // only after the tree is proven gone, so dispose() tolerates it silently + // (R7-2). Letting dispose() reject on it would override the turn's + // classified terminal state. for (const status of [ 'code=none, signal=SIGTERM', 'code=none, signal=SIGKILL', @@ -263,14 +255,22 @@ describe('cleanup-exit classification', () => { 'code=none, signal=SIGHUP', 'code=143, signal=none', ]) { - expect( - isExpectedExternalAgentCleanupExit( - new Error( - `ACP child pid=4242 exited uncleanly during shutdown (${status})`, - ), - ), - ).toBe(true); + const error = new Error( + `ACP child pid=4242 exited uncleanly during shutdown (${status})`, + ); + expect(isExpectedExternalAgentCleanupExit(error)).toBe(true); + expect(isUnprovenExternalAgentTreeExit(error)).toBe(false); } + // The snapshot-race shape means the tree was NEVER enumerated (a detached + // descendant may survive), so it is reported, not silently swallowed + // (R10-3). isExpectedExternalAgentCleanupExit must be false so dispose() + // does not treat it as a clean exit; isUnprovenExternalAgentTreeExit routes + // it to the report-and-resolve path. + const snapshot = new Error( + 'ACP child pid=2628870 exited before its initial process-tree snapshot completed', + ); + expect(isExpectedExternalAgentCleanupExit(snapshot)).toBe(false); + expect(isUnprovenExternalAgentTreeExit(snapshot)).toBe(true); }); it('propagates every genuine cleanup-proof failure', () => { const mustThrow = [ @@ -285,31 +285,35 @@ describe('cleanup-exit classification', () => { 'ACP child pid=4242 did not exit within 5000ms', ]; for (const message of mustThrow) { + // Neither silently tolerated nor report-and-resolved: these must rethrow. expect(isExpectedExternalAgentCleanupExit(new Error(message))).toBe( false, ); + expect(isUnprovenExternalAgentTreeExit(new Error(message))).toBe(false); } }); it('rejects non-Error and near-miss values', () => { - expect(isExpectedExternalAgentCleanupExit(undefined)).toBe(false); - expect(isExpectedExternalAgentCleanupExit(null)).toBe(false); - expect( - isExpectedExternalAgentCleanupExit( - 'ACP child pid=1 exited before its initial process-tree snapshot completed', - ), - ).toBe(false); + for (const predicate of [ + isExpectedExternalAgentCleanupExit, + isUnprovenExternalAgentTreeExit, + ]) { + expect(predicate(undefined)).toBe(false); + expect(predicate(null)).toBe(false); + } + // The snapshot shape's anchoring and pid validation now live in + // isUnprovenExternalAgentTreeExit (R10-3). + const tail = 'exited before its initial process-tree snapshot completed'; + expect(isUnprovenExternalAgentTreeExit(`ACP child pid=1 ${tail}`)).toBe( + false, + ); expect( - isExpectedExternalAgentCleanupExit( - new Error( - 'prefix ACP child pid=1 exited before its initial process-tree snapshot completed', - ), + isUnprovenExternalAgentTreeExit( + new Error(`prefix ACP child pid=1 ${tail}`), ), ).toBe(false); expect( - isExpectedExternalAgentCleanupExit( - new Error( - 'ACP child pid=notanumber exited before its initial process-tree snapshot completed', - ), + isUnprovenExternalAgentTreeExit( + new Error(`ACP child pid=notanumber ${tail}`), ), ).toBe(false); }); @@ -491,7 +495,11 @@ describe.skipIf(process.platform === 'win32')('real ACP subprocess', () => { expect(executor.getExecutionSummary().rounds).toBe(2); }); it('does not dispatch a continuation prompt once the wall-time budget is spent (R9-1)', async () => { - const executor = await create(params()); + const options = params(); + // An explicit cap: an absent max_time_minutes now means "no cap" (R10-5), so + // the budget logic this test exercises needs a real value. + options.runConfig.max_time_minutes = 10; + const executor = await create(options); // Stub the connection so prompt() is countable and resolves end_turn // immediately; the first prompt also advances the mocked clock past the // 10-minute budget, simulating a peer that consumed the whole wall time on @@ -523,6 +531,45 @@ describe.skipIf(process.platform === 'win32')('real ACP subprocess', () => { expect(executor.getTerminateMode()).toBe(AgentTerminateMode.TIMEOUT); nowSpy.mockRestore(); }); + it('does not drain or record a queued message it has no budget to dispatch (R10-4)', async () => { + const options = params(); + options.runConfig.max_time_minutes = 10; + const delivered: unknown[] = []; + options.eventEmitter!.on(AgentEventType.EXTERNAL_MESSAGE, (event) => + delivered.push(event), + ); + const executor = await create(options); + // Spend the budget during round 1 (the R9-1 construction) and back the + // provider with a mutable queue that drains destructively, like + // registry.drainMessages. Round 1 returns end_turn with a message waiting. + const realNow = Date.now(); + let mockNow = realNow; + const nowSpy = vi.spyOn(Date, 'now').mockImplementation(() => mockNow); + const prompt = vi.fn().mockImplementation(() => { + mockNow = realNow + 11 * 60_000; + return Promise.resolve({ stopReason: 'end_turn' }); + }); + ( + executor as unknown as { + connection: { prompt: typeof prompt; cancel: () => Promise }; + } + ).connection = { prompt, cancel: async () => {} }; + const queue = ['USER MESSAGE FROM PARENT']; + const provider = vi.fn().mockImplementation(() => queue.splice(0)); + executor.setExternalMessageProvider(provider); + await executor.execute(context('first')); + // The budget is spent before round 2, so the loop must break BEFORE the + // destructive drain: the queue still holds the message, the provider was + // never called, and no EXTERNAL_MESSAGE certified a delivery that never + // reached the peer. Removing the pre-drain budget/abort check turns these + // red — the queue empties, the provider runs, and the message is recorded as + // delivered while `next` is dropped at the round-top guard. + expect(queue).toEqual(['USER MESSAGE FROM PARENT']); + expect(provider).not.toHaveBeenCalled(); + expect(delivered).toEqual([]); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.TIMEOUT); + nowSpy.mockRestore(); + }); it('does not send a prompt for an already-aborted turn', async () => { const executor = await create(params('prompt-exit')); await executor.execute(context(), AbortSignal.abort()); @@ -560,17 +607,55 @@ describe.skipIf(process.platform === 'win32')('real ACP subprocess', () => { expect(executor.getExecutionSummary().totalToolCalls).toBe(0); expect(executor.getExecutionSummary().successfulToolCalls).toBe(0); }, 15000); - it('installs a ten-minute default deadline and clears it after the real prompt', async () => { + it('installs no wall-time deadline when max_time_minutes is absent (R10-5)', async () => { + // An absent max_time_minutes means "no cap" everywhere else in the codebase + // (agent-core installs no timer when maxTimeMinutes is falsy), so the + // external executor must not borrow a 10-minute default: that would make the + // same definition behave differently based only on whether it declares an + // executor, and would make a TIMEOUT turn — handed to the parent as the + // answer with no truncation marker — reachable by default. wait() already + // treats an undefined budget as "no timer". Reinstating `?? 10` turns this + // red (a ~600000ms timer is installed). const executor = await create(params()); const timers = vi.spyOn(globalThis, 'setTimeout'); - const cleared = vi.spyOn(globalThis, 'clearTimeout'); await executor.execute(context()); - const index = timers.mock.calls.findIndex( + const tenMinute = timers.mock.calls.find( ([, delay]) => typeof delay === 'number' && delay > 599_000 && delay <= 600_000, ); - expect(index).toBeGreaterThanOrEqual(0); - expect(cleared).toHaveBeenCalledWith(timers.mock.results[index]!.value); + expect(tenMinute).toBeUndefined(); + }); + it('reports an unproven process tree on disposal instead of swallowing it (R10-3)', async () => { + const options = params(); + const errors: string[] = []; + options.eventEmitter!.on(AgentEventType.ERROR, (event) => + errors.push(String((event as { error?: unknown }).error)), + ); + const executor = await create(options); + // Stub the tracked child so terminate() reaps the real peer first (no leaked + // subprocess) and then rejects with the snapshot-race proof error — the shape + // acp-bridge raises when the root exited before the tree was enumerated, so a + // detached descendant may survive. dispose() must report it (not swallow it + // silently) and must NOT rethrow: it is awaited between the turn's + // terminal-state classification and the return, so rethrowing would replace a + // classified TIMEOUT/CANCELLED with a thrown ERROR. + const real = ( + executor as unknown as { child: { terminate: () => Promise } } + ).child; + ( + executor as unknown as { child: { terminate: () => Promise } } + ).child = { + terminate: async () => { + await real.terminate().catch(() => {}); + throw new Error( + 'ACP child pid=4242 exited before its initial process-tree snapshot completed', + ); + }, + }; + await expect(executor.dispose!()).resolves.toBeUndefined(); + expect(errors.some((message) => message.includes('not proven gone'))).toBe( + true, + ); }); it('refuses an unenforceable internal turn limit', async () => { const options = params(); diff --git a/packages/cli/src/external-agents/acp-subagent-executor.ts b/packages/cli/src/external-agents/acp-subagent-executor.ts index 54eb3febd37..5799ca051ed 100644 --- a/packages/cli/src/external-agents/acp-subagent-executor.ts +++ b/packages/cli/src/external-agents/acp-subagent-executor.ts @@ -39,8 +39,10 @@ import { ProcessRegistry, type TrackedChildProcess, } from '@qwen-code/acp-bridge/processRegistry'; +import { createDebugLogger } from '@qwen-code/qwen-code-core/utils/debugLogger.js'; const INIT_TIMEOUT_MS = 10_000; +const debugLogger = createDebugLogger('EXTERNAL_AGENT'); export function externalModelLabel(command: string): string { return `external-acp:${command.split(/[\\/]/).pop() ?? command}`; @@ -146,42 +148,62 @@ export function selectRejectOption( } /** - * Errors the ACP process registry raises when disposing a foreign child whose - * process tree is already gone. acp-bridge raises both shapes only AFTER it has - * driven every owned process group to empty, so neither is a cleanup failure — - * they report that a foreign agent we do not own exited, which `terminateMode` - * has already classified. Two shapes are expected here: + * The peer's own exit status, raised by acp-bridge as `exited uncleanly during + * shutdown (code=…, signal=…)` ONLY after it has driven every owned process + * group to empty (`survivingGroups` empty). So it reports that a foreign agent + * we do not own exited — a non-zero code from an uncaught exception, an OOM + * kill, a wrapper turning SIGTERM into 143, or the signal we sent — which + * `terminateMode` has already classified, not a cleanup failure. `dispose()` + * tolerates it silently for any code/signal. * - * - the root exited on its own during the initial tree snapshot, invalidating - * the snapshot that would enumerate descendants (a Linux-only race where the - * prompt rejects on stdout EOF before the process `exit` event); and - * - `exited uncleanly during shutdown (code=…, signal=…)` for ANY code/signal — - * the peer's own exit status (a non-zero code from an uncaught exception, an - * OOM kill, a wrapper turning SIGTERM into 143, or the signal we sent). This - * is thrown only once `survivingGroups` is empty, i.e. cleanup succeeded. + * Tolerating it matters: `dispose()` is awaited inside `runTurn`'s catch between + * error classification and the return, so letting it reject here would replace + * the turn's declared terminal state — a turn the user CANCELLED would surface + * to the parent as `failed` with the registry's text. * - * Tolerating the peer's exit status matters: `dispose()` is awaited inside - * `runTurn`'s catch between error classification and the return, so letting it - * reject here would replace the turn's declared terminal state — a turn the user - * CANCELLED would surface to the parent as `failed` with the registry's text. - * - * Every genuine cleanup-PROOF error still propagates — a truncated snapshot, a - * root that was absent or was not an isolated process-group leader, a failed - * snapshot/signal/inspect, or an exit deadline exceeded — so a descendant we were - * responsible for cannot survive disposal silently. + * This deliberately does NOT cover the snapshot-race shape (see + * `isUnprovenExternalAgentTreeExit`): that one means the tree was never + * enumerated, so it is reported rather than silently swallowed. Every other + * genuine cleanup-PROOF error still propagates from `dispose()` — a truncated + * snapshot, a root that was absent or was not an isolated process-group leader, + * a failed snapshot/signal/inspect, or an exit deadline exceeded — so a + * descendant we were responsible for cannot survive disposal silently. */ export function isExpectedExternalAgentCleanupExit(error: unknown): boolean { - if (!(error instanceof Error)) return false; return ( - /^ACP child pid=\d+ exited before its initial process-tree snapshot completed$/.test( - error.message, - ) || + error instanceof Error && /^ACP child pid=\d+ exited uncleanly during shutdown \(code=[^,]+, signal=[^)]+\)$/.test( error.message, ) ); } +/** + * acp-bridge raises `exited before its initial process-tree snapshot completed` + * when the peer's root exited while the initial snapshot was in flight: + * `mergeAsynchronousSnapshot` records this proof error and returns BEFORE + * `collectOwnership`, so `knownGroups` holds only the root pgid and a detached + * (`setsid`) descendant the peer started — an MCP or dev server in its own + * process group — was never enumerated, signalled or reaped. The tree was + * therefore NOT proven gone, and acp-bridge ranks this proof error above the + * unclean-exit (process-registry.ts:416 before :417). + * + * It is not a hard cleanup failure (the root and its own group are settled), so + * `dispose()` must not rethrow it — rethrowing would convert an already + * classified TIMEOUT/CANCELLED turn into a thrown ERROR — but it must not be + * swallowed silently either, or the parent is told a clean teardown happened + * while a descendant may survive. `dispose()` reports it (debug log + an ERROR + * event) and resolves. + */ +export function isUnprovenExternalAgentTreeExit(error: unknown): boolean { + return ( + error instanceof Error && + /^ACP child pid=\d+ exited before its initial process-tree snapshot completed$/.test( + error.message, + ) + ); +} + /** * Thrown by `wait()` when an operation exceeds its deadline. A dedicated type * lets `runTurn` classify a wall-time expiry by identity instead of re-deriving @@ -475,8 +497,20 @@ class AcpSubagentExecutor implements SubagentExecutor { this.toolNames.clear(); this.finishedTools.clear(); this.turnStartedAt = Date.now(); - // Match DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES without importing the workflow runtime. - const timeoutMs = (this.params.runConfig.max_time_minutes ?? 10) * 60_000; + // An absent max_time_minutes means "no wall-time cap", matching the + // in-process sibling (agent-core installs no timer when maxTimeMinutes is + // falsy) and the rest of the codebase. Do NOT borrow a default from + // DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES: workflow agent() hard-rejects + // external-executor definitions upstream, so that constant provably never + // reaches this path — the only route here is the Agent tool, which sets no + // default. A borrowed 10-minute cap would make the same definition behave + // differently based only on whether it declares an executor, and would make + // a TIMEOUT turn (presented to the parent as the answer) reachable by + // default. `wait` already treats an undefined budget as "no timer". + const timeoutMs = + this.params.runConfig.max_time_minutes === undefined + ? undefined + : this.params.runConfig.max_time_minutes * 60_000; const cancel = () => { this.cancelled = true; this.drainPermissions(); @@ -528,6 +562,28 @@ class AcpSubagentExecutor implements SubagentExecutor { ); this.terminateMode = this.stopMode(result.stopReason); if (this.terminateMode !== AgentTerminateMode.GOAL) break; + // Gate the destructive drain on the budget and the abort signal BEFORE + // recording anything as delivered — mirroring the in-process sibling, + // which checks the wall-time budget before drainExternalInputs. The + // provider splices the queued messages out of the registry and + // emitInputs writes them to the transcript as delivered; if the budget + // is already spent (the round-top guard would break the next round) or + // the signal aborted (the loop-bottom condition), `next` is discarded + // without ever reaching connection.prompt — the transcript would + // certify delivery of a message that was never sent, and the registry + // has already lost it. Only drain once committed to dispatching. + if (signal?.aborted) { + this.terminateMode = AgentTerminateMode.CANCELLED; + break; + } + const remainingNext = + timeoutMs === undefined + ? undefined + : Math.max(0, timeoutMs - (Date.now() - this.turnStartedAt)); + if (remainingNext !== undefined && remainingNext <= 0) { + this.terminateMode = AgentTerminateMode.TIMEOUT; + break; + } const queued = this.provider?.() ?? []; if (queued.length === 0) break; this.emitter.emit(AgentEventType.ROUND_TEXT, { @@ -657,6 +713,25 @@ class AcpSubagentExecutor implements SubagentExecutor { return (this.disposePromise ??= this.child .terminate() .catch((error: unknown) => { + if (isUnprovenExternalAgentTreeExit(error)) { + // The peer's root exited before its process tree was enumerated, so a + // detached descendant may survive. Report it — do NOT rethrow: + // dispose() is awaited between the turn's terminal-state classification + // and its return, so rethrowing would replace a classified + // TIMEOUT/CANCELLED with a thrown ERROR. + const detail = error instanceof Error ? error.message : String(error); + debugLogger.warn( + `External ACP agent process tree was not proven gone after disposal: ${detail}`, + ); + if (this.emitter.rawListeners(AgentEventType.ERROR).length > 0) { + this.emitter.emit(AgentEventType.ERROR, { + subagentId: this.id, + error: `External agent process tree not proven gone after disposal: ${detail}`, + timestamp: Date.now(), + }); + } + return; + } if (!isExpectedExternalAgentCleanupExit(error)) throw error; })); } diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index 4b96aae4463..15dbd47de51 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -483,11 +483,16 @@ You are a helpful assistant. expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); }); - it('does not refuse a definition whose only "executor" token is nested, when the YAML is malformed (R7-1 over-refusal leg)', async () => { - // An indented `executor:` under another key declares no top-level - // executor. With the column-0 anchor it is not matched, so a pre-existing - // definition that merely mentions executor in a nested position (and has - // an unrelated YAML quirk) still loads instead of being hard-refused. + it('refuses a malformed file whose only "executor" token is nested, fail-closed (R10-1 re-scopes the R7-1 over-refusal leg)', async () => { + // R10-1 widened the claim probe to indentation-tolerant so an executor key + // the real AST dropped cannot slip through to a silent in-process run (a + // TAB- or space-indented top-level `executor:` that the column-0 anchor + // missed). The accepted cost: an `executor:` token nested under another + // key, in a file that ALSO has a YAML error, is now treated as a claim the + // AST lost and refused — a visible, user-fixable over-refusal that beats an + // invisible substitution. Reverting the probe to column-0 anchoring turns + // this red (the definition loads again). A nested executor token in a file + // with NO YAML error still loads: the guard is gated on errors.length > 0. const yaml = await vi.importActual< typeof import('../utils/yaml-parser.js') >('../utils/yaml-parser.js'); @@ -495,12 +500,10 @@ You are a helpful assistant. const content = '---\nname: reviewer\ndescription: Reviews code: fast\nmetadata:\n executor: legacy-note\n---\nPrompt'; vi.mocked(fs.readFile).mockResolvedValue(content); - const config = await manager.parseSubagentFile( - validConfig.filePath!, - 'project', - ); - expect(config.name).toBe('reviewer'); - expect(config.executor).toBeUndefined(); + await expect( + manager.parseSubagentFile(validConfig.filePath!, 'project'), + ).rejects.toThrow(/invalid executor block/); + expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); }); it('loads a valid executor whose frontmatter has an unrelated tolerated YAML error before it (R7-1 over-refusal, narrowed)', async () => { @@ -579,6 +582,31 @@ You are a helpful assistant. expect(mockAgentHeadlessCreate).not.toHaveBeenCalled(); }); + it('loads a block-scalar prose "executor:" as no executor, not a hoisted command (R10-1)', async () => { + // A `description: |` block scalar whose prose contains an `executor:` line, + // plus an unresolved alias elsewhere so the strict parse fails and the + // lenient parseSimple fallback runs. parseSimple hoists the prose + // `executor:` into a top-level key ({kind:acp, command:npx}); the old code + // used that value as executorRaw and loaded the definition as EXTERNAL, + // dispatching `npx` — a command that exists only as prose. parseDocument + // correctly sees no top-level executor (hasExecutor false) and, with no + // document error, the guard does not fire, so the fix loads with executor + // undefined. Restoring the parseSimple fallback for executorRaw turns this + // red (config.executor becomes {kind:'acp', command:'npx'}). + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + const content = + '---\nname: x\ndescription: |\n executor:\n kind: acp\n command: npx\nother: *undefined_anchor\n---\nPrompt'; + vi.mocked(fs.readFile).mockResolvedValue(content); + const config = await manager.parseSubagentFile( + validConfig.filePath!, + 'project', + ); + expect(config.executor).toBeUndefined(); + }); + it('visibly reports invalid executors while continuing discovery', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); vi.mocked(fs.readdir).mockResolvedValue(['bad.md', 'good.md'] as never); @@ -1818,6 +1846,30 @@ You are a helpful assistant.`; path.normalize('/test/project/.qwen/agents/misnamed-file.md'), ); }); + + it('refuses a by-name dispatch matching a skipped invalid-executor file instead of falling through to a builtin (R10-2)', async () => { + // A project file declares the builtin name 'Explore' with an invalid + // executor (typo'd `kind: ACP`), so discovery skips it with a warning. + // Without R10-2, loadSubagent('Explore') falls through + // session>project>user>extension>builtin and resolves the BUILTIN Explore + // — an in-process agent under a Qwen model — silently substituting it for + // the external agent the file asked for, with only a console.warn as the + // trace. loadSubagent must instead refuse with the recorded executor error. + // Reverting to skip-and-continue (no recorded refusal) turns this red: the + // call resolves the builtin instead of rejecting. + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.mocked(fs.readdir).mockResolvedValue(['explore.md'] as any); + vi.mocked(fs.readFile).mockResolvedValue( + '---\nname: Explore\ndescription: Test\nexecutor:\n kind: ACP\n command: npx\n---\nPrompt', + ); + await expect(manager.loadSubagent('Explore')).rejects.toThrow( + /invalid executor block/, + ); + }); }); describe('updateSubagent', () => { diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index 87be92fa60b..f372218f696 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -106,6 +106,14 @@ function subagentApprovalModesLabel(): string { export class SubagentManager { private readonly validator: SubagentValidator; private subagentsCache: Map | null = null; + // R10-2: executor-block refusals recorded during a level scan, keyed by level + // then lowercased declared name. loadSubagent consults this so a by-name + // dispatch REFUSES instead of falling through to a lower-precedence in-process + // definition of the same name (the silent substitution this feature prevents). + private readonly executorRefusals = new Map< + SubagentLevel, + Map + >(); private readonly changeListeners: Set<() => void> = new Set(); constructor(private readonly config: Config) { @@ -308,6 +316,10 @@ export class SubagentManager { ); } + // The explicit-level path does not fall through to a lower-precedence + // definition or a builtin, so it cannot silently substitute an in-process + // agent — return the level's match (or null) without the R10-2 refusal. + // Only the by-name fall-through path below (used by dispatch) refuses. return this.findSubagentByNameAtLevel(name, level); } @@ -325,12 +337,14 @@ export class SubagentManager { if (projectConfig) { return projectConfig; } + this.throwRecordedExecutorRefusal('project', lowerName); // Try user level const userConfig = await this.findSubagentByNameAtLevel(name, 'user'); if (userConfig) { return userConfig; } + this.throwRecordedExecutorRefusal('user', lowerName); // Try extension level const extensionConfig = await this.findSubagentByNameAtLevel( @@ -340,11 +354,27 @@ export class SubagentManager { if (extensionConfig) { return extensionConfig; } + this.throwRecordedExecutorRefusal('extension', lowerName); // Try built-in agents as fallback return this.getBuiltinAgent(name); } + // R10-2: refuse a by-name dispatch when this level skipped a file that + // declared `name` but had an invalid executor block, instead of falling + // through to a lower-precedence in-process definition (or a builtin) of the + // same name — which would silently substitute a Qwen-model agent for the + // external one the file asked for, with only a discovery-time console.warn. + // Scoped to executor refusals recorded during the level scan, so an arbitrary + // malformed file cannot disable an unrelated builtin. + private throwRecordedExecutorRefusal( + level: SubagentLevel, + lowerName: string, + ): void { + const refusal = this.executorRefusals.get(level)?.get(lowerName); + if (refusal) throw refusal; + } + /** * Updates an existing subagent configuration. * @@ -1599,6 +1629,7 @@ export class SubagentManager { const files = await fs.readdir(baseDir); const subagents: SubagentConfig[] = []; + const refusals = new Map(); for (const file of files) { if (!file.endsWith('.md')) continue; @@ -1613,10 +1644,23 @@ export class SubagentManager { // mistyped frontmatter or used a reserved name had no way to see // why their agent wasn't loading. warnInvalidSubagentFile(filePath, error); + // R10-2: record an executor-block refusal by declared name so a + // by-name dispatch can REFUSE instead of falling through to a + // lower-precedence in-process definition of the same name. Scoped to + // executor refusals (not parse failures generally) so an arbitrary + // malformed repo file cannot disable an unrelated builtin at dispatch. + if ( + error instanceof SubagentError && + error.subagentName !== undefined && + error.message.includes('invalid executor block') + ) { + refusals.set(error.subagentName.toLowerCase(), error); + } continue; } } + this.executorRefusals.set(level, refusals); return subagents; } catch (_error) { // Directory doesn't exist or can't be read @@ -1656,7 +1700,21 @@ export class SubagentManager { * @returns True if name is available */ async isNameAvailable(name: string, level?: SubagentLevel): Promise { - const existing = await this.loadSubagent(name, level); + let existing: SubagentConfig | null; + try { + existing = await this.loadSubagent(name, level); + } catch (error) { + // R10-2: loadSubagent now refuses a name claimed by an invalid-executor + // file. For availability that name IS taken (the file exists), so report + // it unavailable rather than propagating the dispatch-time refusal. + if ( + error instanceof SubagentError && + error.message.includes('invalid executor block') + ) { + return false; + } + throw error; + } if (!existing) { return true; // Name is available @@ -1913,21 +1971,23 @@ function parseSubagentContent( // would silently delete an agent from `/agents` (and break // `subagent_type:`) with nothing on stdout or stderr. Detect the claim from // the raw text too, so a YAML error that corrupts the `executor` key out of - // the AST still cannot slip through to a silent in-process run. The text - // probe is anchored to a TOP-LEVEL key (column 0, optional quotes) on - // purpose: an indented `executor:` nested under `mcpServers:` or written - // inside a `description: |` block scalar declares no executor, and matching - // it would hard-refuse an unrelated pre-existing definition; while a quoted - // top-level `"executor":` IS a declaration that `parseSimple` (keeps the - // quotes in the key) and a repaired `parseDocument` (nests it) both miss, so - // it must be caught here or the file loads executor-less and runs in-process. - const executorClaimMatch = /^["']?executor["']?[ \t]*:/m.exec( + // the AST still cannot slip through to a silent in-process run. The probe is + // indentation-TOLERANT (`^[ \t]*`, optional quotes) on purpose: a TAB before + // a top-level `executor:` is invalid YAML indentation, so parseDocument + // errors and drops the key, and a column-0-anchored probe would miss it — the + // definition would then load executor-less and run in-process under a Qwen + // model with nothing on any channel, the exact silent substitution this + // feature exists to prevent. The accepted cost is that 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 or an invented dispatch. The + // claim is NOT read from the lenient `parseSimple` value: that line-based + // heuristic hoists an `executor:` line inside a `description: |` block scalar + // into a top-level key, which would dispatch a command existing only as prose. + const executorClaimMatch = /^[ \t]*["']?executor["']?[ \t]*:/m.exec( frontmatterYaml, ); - const claimsExecutor = - hasExecutor || - frontmatter['executor'] !== undefined || - executorClaimMatch !== null; + const claimsExecutor = hasExecutor || executorClaimMatch !== null; // Refuse only when an error can actually corrupt the executor node, not on // every tolerated YAML error in the file. Two distinct dangers: (1) the real // YAML parser (parseDocument) lost the executor node (!hasExecutor) although @@ -1990,7 +2050,14 @@ function parseSubagentContent( } executorRaw = (documentJs as Record)['executor']; } else { - executorRaw = frontmatter['executor']; + // Do NOT fall back to the lenient parseSimple value: when parseDocument + // (the real YAML parser) has no top-level executor node, any `executor:` + // token the text probe matched is nested or block-scalar prose, and + // parseSimple's reconstruction is untrustworthy (it invents `command: '|'`, + // hoists prose into a key). A claim the AST lost under a YAML error is + // already refused above; with no error there is genuinely no top-level + // executor, so load without one rather than dispatch an unconfirmed value. + executorRaw = undefined; } const executor = parseAgentExecutor(executorRaw); if ((hasExecutor || executorRaw !== undefined) && executor === undefined) { @@ -2032,6 +2099,11 @@ function parseSubagentContent( return config; } catch (error) { + // Preserve a SubagentError as-is: an executor-block refusal carries the + // declared agent name (subagentName), which loadSubagent needs to refuse a + // by-name dispatch instead of falling through to a lower-precedence + // in-process definition (R10-2). Re-wrapping would strip the name and code. + if (error instanceof SubagentError) throw error; throw new SubagentError( `Failed to parse subagent file: ${error instanceof Error ? error.message : 'Unknown error'}`, SubagentErrorCode.INVALID_CONFIG, From 32069379fbb2a86a333c11b3592cff1c282e963b Mon Sep 17 00:00:00 2001 From: probe Date: Wed, 9 Sep 2026 23:52:41 +0800 Subject: [PATCH 3/5] fix(subagent): close the round-11 review Criticals on the external executor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six Criticals from the round-11 review: five on the executor and its definition loader, one a regression the round-10 R10-2 fix itself introduced. Each is mutation-verified (reverting the fix turns its test red). R11-1 — a file that CLAIMS an executor but fails an EARLIER validation (a missing description, a bad approvalMode) was skipped with nothing recorded, so loadSubagent's fall-through silently substituted the builtin of the same name. The executor claim and the trusted declared name are now computed at function scope, right after the frontmatter is parsed (BEFORE any validation can throw), so the catch converts ANY load failure of an executor-claiming file into a named executor refusal the fall-through throws. (A file whose own name is unparseable stays undefined-keyed and falls through to the generic wrap — it cannot be matched by name anyway.) R11-2 — the wall-time budget was computed per-turn (timeoutMs - elapsed this turn), so a resident agent re-invoked per incoming message (resetStats:false, which preserves durationMs) got a FRESH max_time_minutes cap each continuation and could overrun the whole-delegation budget indefinitely. Both remaining computations now subtract the accumulated durationMs, matching the in-process sibling's preserveStats base: max_time_minutes caps the whole delegation. Both production continuation callers (background-agent-resume, agent) pass resetStats:false, so the cumulative cap reaches the real path. R11-3 — the host approval mode was used AS the peer's mode id, but the vocabularies differ (host/qwen: auto-edit/auto/yolo; Claude: acceptEdits/ bypassPermissions), so a host auto-edit could select a peer mode that does not exist (or, worse, a qwen peer's broader auto). resolvePermissionMode now maps the host policy to a peer-vocabulary-independent TOKEN (default/plan/acceptEdits/ bypass) and connect() picks the first id the peer actually advertises via a canonical-first alias table, refusing (naming the policy) when none is advertised. The mapping never WIDENS: acceptEdits prefers the peer's narrowest edit-only alias. R11-4 — the executor refusal was keyed by the lenient parseSimple name, which strips only double quotes, so a single-quoted `name: 'Explore'` was recorded under "'explore'" and missed the 'explore' dispatch lookup, falling through to the builtin. The refusal is now keyed by the real YAML AST's name (parseDocument strips both quote styles), with the lenient value only as a fallback when the AST read throws. R11-5 — the approval confirmation's prompt rendered the OPTION LABELS (which the dialog already shows as buttons) and dropped the action's arguments, so the user approved "Write" without seeing the `rm -rf ./build` it would run. The prompt is now describeExternalAction(toolCall): the title plus the rawInput, bounded to 300 chars and stripped of C0/C1/DEL control chars (foreign-process data, never rendered as markup). R10-2 (fix-induced) — extension agents load via loadSubagentFromDir, which skips + warns on a refusal, so the R10-2 level-scan recording never ran for them and the extension leg of the fall-through read an empty map. loadSubagentFromDir now takes an optional refusal collector; the runtime loadExtension records each extension's executor refusals onto extension.agentExecutorRefusals, and listSubagentsAtLevel('extension') merges them into the 'extension' refusal bucket before findSubagentByNameAtLevel returns, so the by-name fall-through refuses them too. (The install/update consent path also calls loadSubagentFromDir, but only for display — it does not feed the dispatch fall-through, so it needs no collector.) Verification: core and cli tsc 0 errors; ESLint and Prettier clean; cli executor suite 57/57; core subagent-manager suite 199/199; core subagent/agent/runtime regression net 2513 passed (6 skipped). Mutation proofs: dropping the cumulative durationMs subtraction fails R11-2 (turn 2 ends GOAL, not TIMEOUT); removing the peer-canonical acceptEdits alias fails R11-3 (the auto-edit dispatch finds no advertised mode); reverting the approval prompt to the title fails R11-5 (no `rm -rf ./build`); neutralizing the catch conversion fails R11-1 (loadSubagent resolves the builtin); keying by the lenient name fails R11-4 (single-quoted name resolves the builtin); and no-oping the extension-refusal merge fails the R10-2 extension leg. Reverse-audit converged after one fix (the R11-2 test burned the whole budget in turn 1, tripping the R10-4 pre-drain gate's GOAL-to-TIMEOUT reclassification; redesigned so turn 1 ends GOAL and turn 2 hits the cumulative cap), then clean passes over the production and test diffs. --- .../acp-subagent-executor.test.ts | 141 +++++++++++++++++- .../external-agents/acp-subagent-executor.ts | 116 ++++++++++++-- .../core/src/extension/extensionManager.ts | 9 +- .../src/subagents/subagent-manager.test.ts | 74 +++++++++ .../core/src/subagents/subagent-manager.ts | 99 ++++++++++-- 5 files changed, 408 insertions(+), 31 deletions(-) diff --git a/packages/cli/src/external-agents/acp-subagent-executor.test.ts b/packages/cli/src/external-agents/acp-subagent-executor.test.ts index d39d1db22c5..d8ac7552393 100644 --- a/packages/cli/src/external-agents/acp-subagent-executor.test.ts +++ b/packages/cli/src/external-agents/acp-subagent-executor.test.ts @@ -22,6 +22,7 @@ import { isUnprovenExternalAgentTreeExit, optionKindForOutcome, resolvePermissionMode, + selectPeerModeId, selectPermissionOption, selectRejectOption, } from './acp-subagent-executor.js'; @@ -47,7 +48,7 @@ connection = new AgentSideConnection(() => ({ newSession: async () => { if (scenario === 'session-exit') process.exit(4); if (scenario === 'session-hang') return new Promise(() => {}); - return {sessionId:'fixture', modes:{currentModeId:'auto', availableModes:scenario === 'no-mode' ? [] : [{id:'default',name:'Ask'},{id:'plan',name:'Plan'}]}}; + return {sessionId:'fixture', modes:{currentModeId:'auto', availableModes:scenario === 'no-mode' ? [] : scenario === 'claude-modes' ? [{id:'default',name:'Ask'},{id:'acceptEdits',name:'AcceptEdits'},{id:'bypassPermissions',name:'Bypass'}] : scenario === 'qwen-modes' ? [{id:'plan'},{id:'default'},{id:'auto-edit'},{id:'auto'},{id:'yolo'}] : [{id:'default',name:'Ask'},{id:'plan',name:'Plan'}]}}; }, setSessionMode: async (params) => { if (scenario === 'mode-error') throw new Error('cannot set mode'); @@ -59,7 +60,9 @@ connection = new AgentSideConnection(() => ({ cancel: async () => { await send({sessionUpdate:'agent_message_chunk',content:{type:'text',text:'CANCEL_RECEIVED'}}); }, prompt: async (params) => { prompts++; - if (mode !== 'default' && mode !== 'plan') throw new Error('PROMPT BEFORE SAFE MODE'); + const safeModes = scenario === 'claude-modes' ? ['default','acceptEdits','bypassPermissions'] : scenario === 'qwen-modes' ? ['plan','default','auto-edit','auto','yolo'] : ['default','plan']; + if (!safeModes.includes(mode)) throw new Error('PROMPT BEFORE SAFE MODE'); + if (scenario === 'claude-modes' || scenario === 'qwen-modes') { await send({sessionUpdate:'agent_message_chunk',content:{type:'text',text:'MODE:'+mode}}); return {stopReason:'end_turn'}; } if (scenario === 'prompt-exit') process.exit(5); if (scenario === 'prompt-close') { process.stdout.end(); return new Promise(() => {}); } if (scenario === 'prompt-hang') return new Promise(() => {}); @@ -94,7 +97,7 @@ connection = new AgentSideConnection(() => ({ } if (scenario.startsWith('permission')) { const toolName = scenario === 'permission-question' ? 'AskUserQuestion' : scenario === 'permission-question-snake' ? 'ask_user_question' : 'Write'; - const request = () => connection.requestPermission({sessionId:'fixture',toolCall:{toolCallId:'same',title:'Write',kind:'edit',_meta:{claudeCode:{toolName}}},options:[{optionId:'once',name:'Once',kind:'allow_once'},{optionId:'always',name:'Always',kind:'allow_always'},{optionId:'no',name:'No',kind:'reject_once'}]}); + const request = () => connection.requestPermission({sessionId:'fixture',toolCall:{toolCallId:'same',title:'Write',kind:'edit',rawInput:{command:'rm -rf ./build'},_meta:{claudeCode:{toolName}}},options:[{optionId:'once',name:'Once',kind:'allow_once'},{optionId:'always',name:'Always',kind:'allow_always'},{optionId:'no',name:'No',kind:'reject_once'}]}); const result = scenario === 'permission-duplicate' ? await Promise.all([request(), request()]) : [await request()]; await send({sessionUpdate:'agent_message_chunk',content:{type:'text',text:JSON.stringify(result)}}); permissionCount++; @@ -171,6 +174,35 @@ describe('permission mapping', () => { expect(resolvePermissionMode(undefined, undefined)).toBe('default'); expect(resolvePermissionMode('unknown', undefined)).toBe('default'); }); + it('maps host approval vocabulary to policy tokens, never peer mode ids (R11-3)', () => { + expect(resolvePermissionMode('yolo', undefined)).toBe('bypass'); + expect(resolvePermissionMode('bypassPermissions', undefined)).toBe( + 'bypass', + ); + expect(resolvePermissionMode('auto', undefined)).toBe('acceptEdits'); + expect(resolvePermissionMode('auto-edit', undefined)).toBe('acceptEdits'); + }); + it('selects the peer-advertised alias for a policy, never a host token (R11-3)', () => { + // Claude vocabulary: host auto-edit maps to the peer's acceptEdits, never + // the qwen-only 'auto' (which a Claude peer does not even advertise). + expect( + selectPeerModeId('acceptEdits', [ + 'default', + 'acceptEdits', + 'bypassPermissions', + ]), + ).toBe('acceptEdits'); + // qwen vocabulary: host auto-edit maps to the peer's auto-edit. + expect( + selectPeerModeId('acceptEdits', ['auto-edit', 'auto', 'default']), + ).toBe('auto-edit'); + expect(selectPeerModeId('bypass', ['bypassPermissions'])).toBe( + 'bypassPermissions', + ); + expect(selectPeerModeId('bypass', ['yolo'])).toBe('yolo'); + // No advertised alias → undefined so the caller refuses, not guesses. + expect(selectPeerModeId('bypass', ['default', 'plan'])).toBeUndefined(); + }); it('never grants for non-approval outcomes or widens a single approval', () => { expect(optionKindForOutcome(ToolConfirmationOutcome.RestorePrevious)).toBe( 'reject_once', @@ -371,6 +403,56 @@ describe.skipIf(process.platform === 'win32')('real ACP subprocess', () => { await expect(create(params(scenario))).rejects.toThrow(); }, ); + it('drives the peer-advertised acceptEdits mode for a host auto-edit approval (R11-3)', async () => { + const options = params('claude-modes'); + options.approvalMode = 'auto-edit'; + const executor = await create(options); + await executor.execute(context()); + // The peer is told 'acceptEdits' (its own vocabulary), never the qwen-only + // 'auto'/'auto-edit' token — the fixture echoes the mode it received. + expect(executor.getFinalText()).toContain('MODE:acceptEdits'); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.GOAL); + }); + it('drives the peer-advertised bypassPermissions mode for a host yolo approval (R11-3)', async () => { + const options = params('claude-modes'); + options.approvalMode = 'yolo'; + const executor = await create(options); + await executor.execute(context()); + expect(executor.getFinalText()).toContain('MODE:bypassPermissions'); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.GOAL); + }); + it('refuses creation when the peer advertises no mode for the host bypass policy (R11-3)', async () => { + // The default fixture advertises only default+plan; a host yolo approval + // maps to the bypass policy, which has no advertised alias → connect must + // fail loudly (naming the policy), never silently weaken the mode. + const options = params(); + options.approvalMode = 'yolo'; + await expect(create(options)).rejects.toThrow(/bypass/); + }); + it('selects the qwen peer auto-edit mode for a host auto-edit approval (R11-3)', async () => { + // A qwen-vocabulary peer advertises auto-edit (NOT the Claude-only + // acceptEdits), so the old hard-coded 'acceptEdits' id made connect() throw + // and killed the delegation. The policy token + alias selection must find + // the peer's own auto-edit id and connect must succeed. + const options = params('qwen-modes'); + options.approvalMode = 'auto-edit'; + const executor = await create(options); + await executor.execute(context()); + expect(executor.getFinalText()).toContain('MODE:auto-edit'); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.GOAL); + }); + it('selects the qwen peer yolo mode for a host yolo approval, never a weaker auto (R11-3)', async () => { + // The old mapping sent host yolo → 'auto', which a qwen peer accepted — + // silently running a YOLO delegation in the strictly weaker AUTO mode (and + // against a Claude peer, 'auto' is not advertised, so it threw). The bypass + // policy must select the peer's yolo id. + const options = params('qwen-modes'); + options.approvalMode = 'yolo'; + const executor = await create(options); + await executor.execute(context()); + expect(executor.getFinalText()).toContain('MODE:yolo'); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.GOAL); + }); it.each(['init-hang', 'session-hang', 'mode-hang'])( 'bounds and reaps a %s handshake', async (scenario) => { @@ -570,6 +652,39 @@ describe.skipIf(process.platform === 'win32')('real ACP subprocess', () => { expect(executor.getTerminateMode()).toBe(AgentTerminateMode.TIMEOUT); nowSpy.mockRestore(); }); + it('enforces the wall-time budget cumulatively across resetStats:false continuations (R11-2)', async () => { + const options = params(); + options.runConfig.max_time_minutes = 10; + const executor = await create(options); + const realNow = Date.now(); + let mockNow = realNow; + const nowSpy = vi.spyOn(Date, 'now').mockImplementation(() => mockNow); + // Each prompt burns 7 of the 10 budget minutes (mocked clock), resolving + // end_turn immediately so wait() never times out the in-flight round. + const prompt = vi.fn().mockImplementation(() => { + mockNow += 7 * 60_000; + return Promise.resolve({ stopReason: 'end_turn' }); + }); + ( + executor as unknown as { + connection: { prompt: typeof prompt; cancel: () => Promise }; + } + ).connection = { prompt, cancel: async () => {} }; + // Turn 1 (fresh, resetStats default): burns 7min < 10min cap, ends GOAL. + await executor.execute(context('first')); + expect(prompt).toHaveBeenCalledTimes(1); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.GOAL); + // Turn 2 (continuation, stats preserved): the cumulative clock starts at + // 7min, leaving only 3min. Its prompt burns 7min more, pushing the + // cumulative elapsed (14min) past the 10min cap, so the turn is cut off + // TIMEOUT. Without subtracting this.durationMs, turn 2 would get a fresh + // per-turn 10min budget, its 7min prompt would fit, and it would end GOAL — + // a continued agent overrunning the whole-delegation cap. + await executor.execute(context('second'), undefined, { resetStats: false }); + expect(prompt).toHaveBeenCalledTimes(2); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.TIMEOUT); + nowSpy.mockRestore(); + }); it('does not send a prompt for an already-aborted turn', async () => { const executor = await create(params('prompt-exit')); await executor.execute(context(), AbortSignal.abort()); @@ -775,6 +890,26 @@ describe.skipIf(process.platform === 'win32')('real ACP subprocess', () => { await executor.execute(context()); expect(executor.getFinalText()).toContain('"optionId":"once"'); }); + it('carries the action arguments into the approval confirmation (R11-5)', async () => { + const options = params('permission'); + let seenPrompt: unknown; + options.eventEmitter!.on( + AgentEventType.TOOL_WAITING_APPROVAL, + async (event) => { + seenPrompt = (event as { confirmationDetails?: { prompt?: unknown } }) + .confirmationDetails?.prompt; + await event.respond(ToolConfirmationOutcome.ProceedOnce); + }, + ); + const executor = await create(options); + await executor.execute(context()); + // The approval dialog must show WHAT the user is authorizing — the action's + // arguments — not just the tool title and the option labels the dialog + // already renders as buttons. The fixture peer asks permission for a Write + // whose rawInput carries {command: 'rm -rf ./build'}. Reverting the prompt + // back to the option-names join turns this red (no command string). + expect(String(seenPrompt)).toContain('rm -rf ./build'); + }); it('isolates duplicate outstanding tool IDs and ignores stale approval callbacks', async () => { const options = params('permission-duplicate'); const callbacks: Array<() => Promise> = []; diff --git a/packages/cli/src/external-agents/acp-subagent-executor.ts b/packages/cli/src/external-agents/acp-subagent-executor.ts index 5799ca051ed..8863e9adc35 100644 --- a/packages/cli/src/external-agents/acp-subagent-executor.ts +++ b/packages/cli/src/external-agents/acp-subagent-executor.ts @@ -52,14 +52,66 @@ function isRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value); } +const EXTERNAL_ACTION_PROMPT_MAX = 300; + +function stripControlChars(value: string): string { + // Strip C0/C1 control characters and DEL; the toolCall payload is foreign-process + // data that must never be rendered as markup or terminal escapes. Done by code + // point rather than a control-char regex so the source carries no literal control + // bytes. + let out = ''; + for (const ch of value) { + const code = ch.codePointAt(0) ?? 0; + out += + code >= 0x20 && code !== 0x7f && !(code >= 0x80 && code <= 0x9f) + ? ch + : ' '; + } + return out; +} + +/** + * Renders an external action's arguments into the approval prompt so the user + * sees WHAT they are authorizing, not just the option labels. `rawInput` is an + * unvalidated payload from a foreign process, so the rendering is bounded in + * length and control characters are stripped — never forwarded as markup (R11-5). + */ +export function describeExternalAction( + toolCall: RequestPermissionRequest['toolCall'], +): string { + const title = stripControlChars( + toolCall.title ?? toolCall.kind ?? 'External action', + ); + const raw = toolCall.rawInput; + if (!isRecord(raw)) return title; + const detail = stripControlChars(JSON.stringify(raw)); + const rendered = `${title}: ${detail}`; + return rendered.length > EXTERNAL_ACTION_PROMPT_MAX + ? `${rendered.slice(0, EXTERNAL_ACTION_PROMPT_MAX)}…` + : rendered; +} + +/** + * The host approval policy as a peer-vocabulary-independent token. Peer ACP mode + * ids differ per peer (Claude: `acceptEdits` / `bypassPermissions`; qwen: + * `auto-edit` / `auto` / `yolo`), so resolvePermissionMode maps the host policy + * to a TOKEN and connect() picks the id the peer actually advertises via + * `POLICY_MODE_ALIASES` (R11-3) — a new peer vocabulary needs no new case here. + * The mapping never WIDENS the host policy: a weaker host mode must not select a + * stronger peer mode. + */ +export type ExternalPermissionPolicy = + | 'default' + | 'plan' + | 'acceptEdits' + | 'bypass'; + export function resolvePermissionMode( permissionMode: string | undefined, approvalMode: string | undefined, -): string { +): ExternalPermissionPolicy { switch ((approvalMode ?? permissionMode ?? '').trim().toLowerCase()) { case 'auto': - case 'yolo': - return 'auto'; case 'acceptedits': case 'auto-edit': return 'acceptEdits'; @@ -67,12 +119,33 @@ export function resolvePermissionMode( return 'plan'; case 'bypasspermissions': case 'bypass': - return 'bypassPermissions'; + case 'yolo': + return 'bypass'; default: return 'default'; } } +// The ranked peer-mode-id aliases per host policy token, canonical-first. +// connect() selects the first alias the peer advertises and refuses if none is, +// so a host policy that has no peer counterpart fails loudly (naming the policy) +// instead of silently running the peer in a weaker mode. +const POLICY_MODE_ALIASES: Record = { + bypass: ['bypassPermissions', 'yolo'], + acceptEdits: ['acceptEdits', 'auto-edit', 'auto'], + plan: ['plan'], + default: ['default'], +}; + +export function selectPeerModeId( + policy: ExternalPermissionPolicy, + availableModeIds: readonly string[], +): string | undefined { + return POLICY_MODE_ALIASES[policy].find((id) => + availableModeIds.includes(id), + ); +} + export function optionKindForOutcome( outcome: ToolConfirmationOutcome, ): 'allow_once' | 'allow_always' | 'reject_once' { @@ -355,17 +428,17 @@ class AcpSubagentExecutor implements SubagentExecutor { if (!session.sessionId) throw new Error('External ACP agent returned no sessionId'); this.sessionId = session.sessionId; - const modeId = resolvePermissionMode( + const policy = resolvePermissionMode( this.params.permissionMode, this.params.approvalMode, ); - if ( - !session.modes?.availableModes.some( - (mode: { id: string }) => mode.id === modeId, - ) - ) { + const availableIds = + session.modes?.availableModes.map((mode: { id: string }) => mode.id) ?? + []; + const modeId = selectPeerModeId(policy, availableIds); + if (!modeId) { throw new Error( - `External ACP agent does not advertise permission mode ${modeId}`, + `External ACP agent does not advertise any mode for the host's ${policy} approval policy (advertised: ${availableIds.join(', ') || 'none'})`, ); } await this.connection.setSessionMode({ sessionId: this.sessionId, modeId }); @@ -536,10 +609,19 @@ class AcpSubagentExecutor implements SubagentExecutor { let next = prompt; do { this.round++; + // The wall-time budget is CUMULATIVE across a resident agent's + // continuation turns, matching the in-process sibling's preserveStats + // base: `durationMs` holds prior turns' elapsed time (reset only when + // `resetStats` is not false), so subtracting it makes max_time_minutes a + // cap on the whole delegation rather than a fresh per-turn budget that a + // continued agent could overrun. (R11-2) const remaining = timeoutMs === undefined ? undefined - : Math.max(0, timeoutMs - (Date.now() - this.turnStartedAt)); + : Math.max( + 0, + timeoutMs - this.durationMs - (Date.now() - this.turnStartedAt), + ); // Stop before dispatching a round whose budget is already spent. Without // this, `remaining` clamps to 0 and `connection.prompt(...)` is evaluated // — a new, billed model turn really reaches the peer — one tick before @@ -579,7 +661,10 @@ class AcpSubagentExecutor implements SubagentExecutor { const remainingNext = timeoutMs === undefined ? undefined - : Math.max(0, timeoutMs - (Date.now() - this.turnStartedAt)); + : Math.max( + 0, + timeoutMs - this.durationMs - (Date.now() - this.turnStartedAt), + ); if (remainingNext !== undefined && remainingNext <= 0) { this.terminateMode = AgentTerminateMode.TIMEOUT; break; @@ -981,7 +1066,10 @@ class AcpSubagentExecutor implements SubagentExecutor { confirmationDetails: { type: 'info', title: params.toolCall.title ?? 'External action', - prompt: params.options.map((option) => option.name).join(' / '), + // Carry the action's arguments so the user sees WHAT they are + // authorizing — not just the option labels the dialog already + // renders as buttons (R11-5). Bounded and control-stripped. + prompt: describeExternalAction(params.toolCall), hideAlwaysAllow: true, }, respond: async (outcome) => diff --git a/packages/core/src/extension/extensionManager.ts b/packages/core/src/extension/extensionManager.ts index 1c4dfa6b739..e1844b9369f 100644 --- a/packages/core/src/extension/extensionManager.ts +++ b/packages/core/src/extension/extensionManager.ts @@ -10,7 +10,7 @@ import type { } from '../config/config.js'; import { Config } from '../config/config.js'; import { validateSkillName, type SkillConfig } from '../skills/types.js'; -import type { SubagentConfig } from '../subagents/types.js'; +import type { SubagentConfig, SubagentError } from '../subagents/types.js'; import type { ClaudeMarketplaceConfig } from './claude-converter.js'; import type { HookEventName, HookDefinition } from '../hooks/types.js'; import { Storage } from '../config/storage.js'; @@ -178,6 +178,10 @@ export interface Extension { commands?: string[]; skills?: SkillConfig[]; agents?: SubagentConfig[]; + // R10-2: executor-block refusals for this extension's agent files, keyed by + // lowercased declared name, recorded at load so a by-name dispatch can refuse + // instead of falling through to a builtin of the same name. + agentExecutorRefusals?: Map; hooks?: { [K in HookEventName]?: HookDefinition[] }; channels?: Record; } @@ -1711,9 +1715,12 @@ export class ExtensionManager { extension.skills = await loadSkillsFromDir( `${effectiveExtensionPath}/skills`, ); + const agentExecutorRefusals = new Map(); extension.agents = await loadSubagentFromDir( `${effectiveExtensionPath}/agents`, + agentExecutorRefusals, ); + extension.agentExecutorRefusals = agentExecutorRefusals; } if ( diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index 15dbd47de51..bcc0c5f807c 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -1870,6 +1870,80 @@ You are a helpful assistant.`; /invalid executor block/, ); }); + + it('refuses a by-name dispatch for an executor-claiming file that fails an earlier validation (R11-1)', async () => { + // A project file declares an executor but OMITS the description, so it + // fails the earlier required-field validation BEFORE the executor block is + // reached. Without hoisting the claim probe + declared name above those + // validations, the file is skipped with nothing recorded and loadSubagent + // falls through to the builtin Explore — the substitution R10-2 prevents. + // The catch now converts any load failure of an executor-claiming file into + // a named executor refusal. Reverting the catch conversion turns this red + // (loadSubagent resolves the builtin instead of rejecting). + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.mocked(fs.readdir).mockResolvedValue(['explore.md'] as any); + vi.mocked(fs.readFile).mockResolvedValue( + '---\nname: Explore\nexecutor:\n kind: acp\n command: npx\n---\nPrompt', + ); + await expect(manager.loadSubagent('Explore')).rejects.toThrow( + /invalid executor block/, + ); + }); + + it('keys the executor refusal by the AST-parsed name, so a quoted name still refuses the dispatch (R11-4)', async () => { + // parseSimple's parseValue strips only double quotes, so `name: 'Explore'` + // (single-quoted) would otherwise be recorded under "'explore'" and miss + // the 'explore' dispatch lookup. The refusal must be keyed by the name the + // real YAML AST sees. The colon in description forces the astLostExecutor + // refusal path; reverting `declaredName ?? name` to `name` turns this red. + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.mocked(fs.readdir).mockResolvedValue(['explore.md'] as any); + vi.mocked(fs.readFile).mockResolvedValue( + "---\nname: 'Explore'\ndescription: Reviews code: fast and careful\nexecutor:\n kind: ACP\n command: npx\n---\nPrompt", + ); + await expect(manager.loadSubagent('Explore')).rejects.toThrow( + /invalid executor block/, + ); + }); + + it('refuses a by-name dispatch for an extension-level executor refusal instead of falling through to a builtin (R10-2 extension leg)', async () => { + // Extension agents load via loadSubagentFromDir, which skips + warns on a + // refusal — so the directory-scan recording never ran for them and the + // R10-2 extension leg read an empty map (a no-op). The refusals are now + // carried on the loaded extension and merged into the 'extension' bucket, + // so the fall-through refuses. Reverting the merge (dropping the + // executorRefusals.set('extension', ...)) turns this red: loadSubagent + // resolves the builtin instead of rejecting. + vi.spyOn(mockConfig, 'getActiveExtensions').mockReturnValue([ + { + agents: [], + agentExecutorRefusals: new Map([ + [ + 'explore', + new SubagentError( + 'Agent file /ext/agents/explore.md has an invalid executor block: it declares an executor but failed to load.', + SubagentErrorCode.INVALID_CONFIG, + 'Explore', + ), + ], + ]), + } as never, + ]); + // No project/user file declares 'Explore' — only the extension refusal. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.mocked(fs.readdir).mockResolvedValue([] as any); + await expect(manager.loadSubagent('Explore')).rejects.toThrow( + /invalid executor block/, + ); + }); }); describe('updateSubagent', () => { diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index f372218f696..3c882e1f90d 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -1607,6 +1607,18 @@ export class SubagentManager { if (level === 'extension') { const extensions = this.config.getActiveExtensions(); + // R10-2: extension agents load via loadSubagentFromDir, which skips and + // warns on a refusal — so the directory-scan branch below never runs for + // them and nothing would be recorded. Merge each active extension's + // recorded executor-block refusals so the by-name fall-through refuses + // them too instead of resolving a builtin of the same name. + const merged = new Map(); + for (const extension of extensions) { + for (const [name, error] of extension.agentExecutorRefusals ?? []) { + merged.set(name, error); + } + } + this.executorRefusals.set('extension', merged); return extensions.flatMap((extension) => extension.agents || []); } @@ -1730,6 +1742,11 @@ export class SubagentManager { export async function loadSubagentFromDir( baseDir: string, + // R10-2: when provided, executor-block refusals (an agent file that declares + // an executor but failed to load) are recorded keyed by lowercased declared + // name, so a by-name dispatch can refuse instead of falling through to a + // builtin of the same name. + refusals?: Map, ): Promise { try { const files = await fs.readdir(baseDir); @@ -1751,6 +1768,14 @@ export async function loadSubagentFromDir( subagents.push(config); } catch (error) { warnInvalidSubagentFile(filePath, error); + if ( + refusals && + error instanceof SubagentError && + error.subagentName !== undefined && + error.message.includes('invalid executor block') + ) { + refusals.set(error.subagentName.toLowerCase(), error); + } continue; } } @@ -1768,6 +1793,19 @@ function parseSubagentContent( level: SubagentLevel, validator: SubagentValidator, ): SubagentConfig { + // R11-1/R11-4: the executor claim and the trusted declared name are computed + // at FUNCTION scope so the catch can convert ANY load failure of an + // executor-claiming file into a named executor refusal (recorded by + // listSubagentsAtLevel, refused by loadSubagent's fall-through) — otherwise a + // file that fails an EARLIER validation (missing description, bad approvalMode) + // is skipped with nothing recorded, and a by-name dispatch silently falls + // through to a lower-precedence in-process definition or the builtin. The name + // comes from the real AST (parseDocument strips quotes), NOT the lenient value + // — the lenient parser strips only double quotes, so a single-quoted + // `name: 'Explore'` would otherwise be keyed "'explore'" and miss the + // 'explore' dispatch lookup. + let claimsExecutor = false; + let declaredName: string | undefined; try { const normalizedContent = normalizeContent(content); @@ -1784,6 +1822,29 @@ function parseSubagentContent( // Parse YAML frontmatter const frontmatter = parseYaml(frontmatterYaml) as Record; + // Real-AST executor claim + trusted declared name, computed BEFORE any + // validation can throw so the catch can route an executor-claiming file's + // failure to a named refusal. The executor block below reuses `document`, + // `hasExecutor` and `executorClaimMatch`. + const document = parseDocument(frontmatterYaml); + const hasExecutor = document.has('executor'); + const executorClaimMatch = /^[ \t]*["']?executor["']?[ \t]*:/m.exec( + frontmatterYaml, + ); + claimsExecutor = hasExecutor || executorClaimMatch !== null; + try { + const nodeName = document.has('name') ? document.get('name') : undefined; + if (typeof nodeName === 'string' && nodeName !== '') + declaredName = nodeName; + } catch { + // toJS/node reads can throw on an unresolved alias; keep the lenient name. + } + if (declaredName === undefined) { + const lenientName = frontmatter['name']; + if (typeof lenientName === 'string' && lenientName !== '') + declaredName = lenientName; + } + // Extract required fields and convert to strings const nameRaw = frontmatter['name']; const descriptionRaw = frontmatter['description']; @@ -1956,8 +2017,9 @@ function parseSubagentContent( // agent for the one the definition asked for. // The shared parser strips nulls, including null arguments. Validate the // original YAML node so sanitization cannot change the executable request. - const document = parseDocument(frontmatterYaml); - const hasExecutor = document.has('executor'); + // (`document`, `hasExecutor`, `executorClaimMatch` and `claimsExecutor` are + // computed earlier, right after the frontmatter is parsed, so the catch can + // route an executor-claiming file's failure to a named refusal.) // parseDocument repairs a syntactically invalid document instead of // throwing, and `document.errors` is the only signal that it did so. For an // executor-bearing definition that is unsafe: the repaired node can dispatch @@ -1984,10 +2046,6 @@ function parseSubagentContent( // claim is NOT read from the lenient `parseSimple` value: that line-based // heuristic hoists an `executor:` line inside a `description: |` block scalar // into a top-level key, which would dispatch a command existing only as prose. - const executorClaimMatch = /^[ \t]*["']?executor["']?[ \t]*:/m.exec( - frontmatterYaml, - ); - const claimsExecutor = hasExecutor || executorClaimMatch !== null; // Refuse only when an error can actually corrupt the executor node, not on // every tolerated YAML error in the file. Two distinct dangers: (1) the real // YAML parser (parseDocument) lost the executor node (!hasExecutor) although @@ -2025,7 +2083,7 @@ function parseSubagentContent( `Refusing to load the definition rather than dispatch a silently ` + `repaired executor.`, SubagentErrorCode.INVALID_CONFIG, - name, + declaredName ?? name, ); } } @@ -2045,7 +2103,7 @@ function parseSubagentContent( `Refusing to load the definition rather than dispatch an executor ` + `parsed from YAML that will not resolve to a document.`, SubagentErrorCode.INVALID_CONFIG, - name, + declaredName ?? name, ); } executorRaw = (documentJs as Record)['executor']; @@ -2067,7 +2125,7 @@ function parseSubagentContent( `the definition: dropping the block would silently run it in-process ` + `instead of in the external agent it asked for.`, SubagentErrorCode.INVALID_CONFIG, - name, + declaredName ?? name, ); } @@ -2099,11 +2157,26 @@ function parseSubagentContent( return config; } catch (error) { - // Preserve a SubagentError as-is: an executor-block refusal carries the - // declared agent name (subagentName), which loadSubagent needs to refuse a - // by-name dispatch instead of falling through to a lower-precedence - // in-process definition (R10-2). Re-wrapping would strip the name and code. + // Preserve a SubagentError as-is: an executor-block refusal already carries + // the declared agent name (subagentName), which loadSubagent needs to refuse + // a by-name dispatch instead of falling through (R10-2). Re-wrapping would + // strip the name and code. if (error instanceof SubagentError) throw error; + // R11-1: a file that CLAIMS an executor but failed to load for ANY reason — + // a missing description, an invalid approvalMode, any validation before or + // after the executor block — must surface as a named executor refusal, so the + // dispatch refuses rather than falling through to an in-process substitute. + // (A file whose own name is unparseable stays undefined-keyed and falls + // through to the generic wrap; it cannot be matched by name anyway.) + if (claimsExecutor && declaredName) { + throw new SubagentError( + `Agent file ${filePath} has an invalid executor block: it declares an ` + + `executor but failed to load (${error instanceof Error ? error.message : 'Unknown error'}). ` + + `Refusing to fall through to a lower-precedence in-process substitute.`, + SubagentErrorCode.INVALID_CONFIG, + declaredName, + ); + } throw new SubagentError( `Failed to parse subagent file: ${error instanceof Error ? error.message : 'Unknown error'}`, SubagentErrorCode.INVALID_CONFIG, From 5e1cd46eb30fc0ae515d12ae9596799b0dd933f0 Mon Sep 17 00:00:00 2001 From: probe Date: Thu, 10 Sep 2026 07:23:16 +0800 Subject: [PATCH 4/5] fix(subagent): close the round-12 review Criticals on the external executor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six Criticals from the round-12 review — three on the executor, one a regression the R11-5 fix itself introduced, and two on the definition loader's refusal bookkeeping. Each is mutation-verified (reverting the fix turns its test red). R12-1 — execute() re-rendered and re-sent the entire system-prompt bundle on every continuation turn, although the live ACP session has held it since turn 1 (NewSessionRequest has no system-prompt channel, so the prompt content block is the bundle's only channel and the FIRST turn must still send it). A resident external agent re-invoked per incoming message re-sent the definition's systemPrompt + the appended rules + the whole memory hierarchy (~16.5 KB) every turn, billed by the peer and pushing its context toward compaction, while the transcript recorded only the task. The bundle is now gated on the first turn: `const continuation = this.started` is hoisted above the render and continuations send only the task. R12-2 — the turn-entry `emitInputs` recorded the caller's messages as delivered BEFORE the round-top wall-time guard, so an over-budget continuation wrote EXTERNAL_MESSAGE user records for a prompt that was never dispatched (the guard breaks TIMEOUT first). The entry emit moved to the loop's commit point — after the budget guard, before the prompt — keyed on the ENTRY round (`entryRound`, not `this.round === 1`, which is broken under resetStats:false because the round counter is preserved across continuations). Emitting before the dispatch keeps user-before-assistant order in the JSONL transcript. R11-5 (fix-induced) — the R11-5 confirmation carried the peer's arguments but rendered them as MARKDOWN (the `confirmationDetails` omitted `renderPromptAsPlainText`), so a glob `**` was eaten and a `[label](url)` could mis-render, misrepresenting the action being approved. The info variant now sets `renderPromptAsPlainText: true` (the bound and control-strip stay — the flag is not a substitute for them). Consolidating the local control-char stripper into the centralized `stripAnsiAndControl` was considered but requires a new cross-package core export; the flag is what neutralizes the rendering, and the local stripper already kills the ESC byte so no escape sequence survives. R12-3 — `resolveResumeTarget` reaches the `loadSubagent` that R10-2/R11 made throwing, unguarded, so background-agent discovery swallowed the throw into a debug-only per-sidecar warning and the recovered row VANISHED from /tasks instead of listing with a `resumeBlockedReason`. The `loadSubagent` call is now wrapped and the throw converted into the existing `unavailableReason` shape (both call sites already fold it into `resumeBlockedReason`). The executor-provenance early return stays ahead of the call, so the four cold-external-resume provenance legs still refuse before any load. R12-4 — `executorRefusals` was only rewritten when a level scan reached the end of its `try`, so a scan that could not read the directory left the previous scan's refusals in place and `loadSubagent` kept throwing for a file that no longer exists. Both non-scan paths now reset the level's map: the `readdir` catch and the `project == home` early return. R12-5 — the indentation-tolerant raw-text probe made `claimsExecutor` true for frontmatter that only MENTIONS `executor:` as prose inside a `description: |`/`>` block scalar, so `astLostExecutor` hard-refused an in-process definition that carried an unrelated tolerated YAML quirk (duplicate key, tab indent), and keyed the refusal by its name — blocking every lower-precedence definition and builtin of that name. The probe match is now excluded when its offset lies inside a block scalar (walked via the real AST's BLOCK_LITERAL/BLOCK_FOLDED scalar ranges, fail-closed if the walk throws). A genuine top-level — even TAB-misindented or nested — `executor:` key is not inside a block scalar, so the R10-1/R9-2/R7-1 refusals all still hold. Verification: core and cli tsc 0 errors; ESLint and Prettier clean; cli executor suite 61/61; core subagent-manager 202/202 and background-agent-resume 56/56; core subagent/agent/runtime regression net 2517 passed (6 skipped). Mutation proofs: re-sending the bundle on continuation fails R12-1 (turn 2 carries 2 blocks); moving the entry emit back above the guard fails R12-2 (`delivered` becomes `['second']`); dropping `renderPromptAsPlainText` fails the R11-5 case; rethrowing in `resolveResumeTarget` fails R12-3 (the row vanishes); deleting the catch reset fails R12-4 (stale refusal persists); dropping the block-scalar exclusion fails both R12-5 variants (they reject). Reverse-audit converged to clean passes over the production and test diffs. --- .../acp-subagent-executor.test.ts | 85 ++++++++++++++++++- .../external-agents/acp-subagent-executor.ts | 38 +++++++-- .../agents/background-agent-resume.test.ts | 50 +++++++++++ .../src/agents/background-agent-resume.ts | 23 ++++- .../src/subagents/subagent-manager.test.ts | 68 +++++++++++++++ .../core/src/subagents/subagent-manager.ts | 55 +++++++++++- 6 files changed, 302 insertions(+), 17 deletions(-) diff --git a/packages/cli/src/external-agents/acp-subagent-executor.test.ts b/packages/cli/src/external-agents/acp-subagent-executor.test.ts index d8ac7552393..58b82936783 100644 --- a/packages/cli/src/external-agents/acp-subagent-executor.test.ts +++ b/packages/cli/src/external-agents/acp-subagent-executor.test.ts @@ -685,6 +685,70 @@ describe.skipIf(process.platform === 'win32')('real ACP subprocess', () => { expect(executor.getTerminateMode()).toBe(AgentTerminateMode.TIMEOUT); nowSpy.mockRestore(); }); + it('sends the system-prompt bundle only on the first turn, not continuations (R12-1)', async () => { + const options = params(); + options.promptConfig.systemPrompt = 'SYSTEM PROMPT SENTINEL'; + const executor = await create(options); + const payloads: string[][] = []; + const prompt = vi + .fn() + .mockImplementation((p: { prompt: Array<{ text: string }> }) => { + payloads.push(p.prompt.map((block) => block.text)); + return Promise.resolve({ stopReason: 'end_turn' }); + }); + ( + executor as unknown as { + connection: { prompt: typeof prompt; cancel: () => Promise }; + } + ).connection = { prompt, cancel: async () => {} }; + // Turn 1: the bundle (system + task) is the only channel the instruction has. + await executor.execute(context('first')); + // Turn 2 (continuation on the same live session): only the task is sent — + // the session already holds the bundle. Re-sending it would duplicate the + // whole bundle per turn. Removing the gate turns this red (turn 2 gets 2 + // blocks again). + await executor.execute(context('second')); + expect(payloads.length).toBe(2); + expect(payloads[0].length).toBe(2); + expect(payloads[0][0]).toContain('SYSTEM PROMPT SENTINEL'); + expect(payloads[0][1]).toBe('first'); + expect(payloads[1]).toEqual(['second']); + }); + it('does not record entry inputs as delivered when the budget guard breaks a continuation (R12-2)', async () => { + const options = params(); + options.runConfig.max_time_minutes = 10; + const executor = await create(options); + const realNow = Date.now(); + let mockNow = realNow; + const nowSpy = vi.spyOn(Date, 'now').mockImplementation(() => mockNow); + // Turn 1's single prompt burns 11 minutes — past the 10-minute cap. + const prompt = vi.fn().mockImplementation(() => { + mockNow += 11 * 60_000; + return Promise.resolve({ stopReason: 'end_turn' }); + }); + ( + executor as unknown as { + connection: { prompt: typeof prompt; cancel: () => Promise }; + } + ).connection = { prompt, cancel: async () => {} }; + const delivered: string[] = []; + options.eventEmitter!.on(AgentEventType.EXTERNAL_MESSAGE, (event) => { + delivered.push(String((event as { text?: unknown }).text)); + }); + // Turn 1 ends via the round-bottom budget guard (TIMEOUT); durationMs=11min. + await executor.execute(context('first')); + expect(prompt).toHaveBeenCalledTimes(1); + // Turn 2 (continuation, resetStats:false): the cumulative budget is already + // spent, so the round-top guard breaks TIMEOUT before any dispatch. The entry + // input ('second') must NOT be recorded as delivered — the peer never gets + // it. Moving the emit back above the guard turns this red (delivered becomes + // ['second']). + await executor.execute(context('second'), undefined, { resetStats: false }); + expect(prompt).toHaveBeenCalledTimes(1); + expect(delivered).toEqual([]); + expect(executor.getTerminateMode()).toBe(AgentTerminateMode.TIMEOUT); + nowSpy.mockRestore(); + }); it('does not send a prompt for an already-aborted turn', async () => { const executor = await create(params('prompt-exit')); await executor.execute(context(), AbortSignal.abort()); @@ -892,12 +956,20 @@ describe.skipIf(process.platform === 'win32')('real ACP subprocess', () => { }); it('carries the action arguments into the approval confirmation (R11-5)', async () => { const options = params('permission'); - let seenPrompt: unknown; + let seenDetails: + | { prompt?: unknown; renderPromptAsPlainText?: unknown } + | undefined; options.eventEmitter!.on( AgentEventType.TOOL_WAITING_APPROVAL, async (event) => { - seenPrompt = (event as { confirmationDetails?: { prompt?: unknown } }) - .confirmationDetails?.prompt; + seenDetails = ( + event as { + confirmationDetails?: { + prompt?: unknown; + renderPromptAsPlainText?: unknown; + }; + } + ).confirmationDetails; await event.respond(ToolConfirmationOutcome.ProceedOnce); }, ); @@ -908,7 +980,12 @@ describe.skipIf(process.platform === 'win32')('real ACP subprocess', () => { // already renders as buttons. The fixture peer asks permission for a Write // whose rawInput carries {command: 'rm -rf ./build'}. Reverting the prompt // back to the option-names join turns this red (no command string). - expect(String(seenPrompt)).toContain('rm -rf ./build'); + expect(String(seenDetails?.prompt)).toContain('rm -rf ./build'); + // The payload is foreign-process text, so the dialog must render it as plain + // text — otherwise markdown in it (a glob `**`, a `[label](url)`) is eaten + // or mis-rendered, misrepresenting what the user approves. Removing the flag + // turns this red. (R11-5 fix) + expect(seenDetails?.renderPromptAsPlainText).toBe(true); }); it('isolates duplicate outstanding tool IDs and ignores stale approval callbacks', async () => { const options = params('permission-duplicate'); diff --git a/packages/cli/src/external-agents/acp-subagent-executor.ts b/packages/cli/src/external-agents/acp-subagent-executor.ts index 8863e9adc35..53c28d38c64 100644 --- a/packages/cli/src/external-agents/acp-subagent-executor.ts +++ b/packages/cli/src/external-agents/acp-subagent-executor.ts @@ -491,11 +491,21 @@ class AcpSubagentExecutor implements SubagentExecutor { 'External ACP agents cannot import in-process conversation history', ); } - const system = renderSubagentSystemPrompt( - this.params.promptConfig, - context, - this.params.runtimeContext, - ); + // A continuation turn re-enters on the same live ACP session, which has held + // the system-prompt bundle since turn 1 (NewSessionRequest carries only cwd/ + // mcpServers — the prompt content block is the bundle's only channel, so the + // FIRST turn must still send it). Re-rendering and re-sending it on every + // continuation would duplicate the whole bundle (system prompt + rules + + // memory hierarchy) per turn, billed by the peer and pushing its context + // toward compaction. Continuations send only the task. (R12-1) + const continuation = this.started; + const system = continuation + ? '' + : renderSubagentSystemPrompt( + this.params.promptConfig, + context, + this.params.runtimeContext, + ); const task = String(context.get('task_prompt') ?? 'Get Started!'); // The first turn's task is seeded into the transcript as the initial user // prompt by the dispatcher; a continuation turn (a resident external agent @@ -503,7 +513,6 @@ class AcpSubagentExecutor implements SubagentExecutor { // as a user-side record here or the JSONL transcript loses every message // after the first — the in-process sibling emits the same EXTERNAL_MESSAGE // on continuation. - const continuation = this.started; await this.runTurn( [ ...(system ? [{ type: 'text' as const, text: system }] : []), @@ -605,7 +614,11 @@ class AcpSubagentExecutor implements SubagentExecutor { this.terminateMode = AgentTerminateMode.CANCELLED; return; } - if (inputs) this.emitInputs(inputs); + // R12-2: the entry inputs are emitted at the loop's commit point (after + // the round-top budget guard), not here — emitting now would record them + // as delivered even when the guard breaks an over-budget continuation + // without ever dispatching the prompt. + const entryRound = this.round; let next = prompt; do { this.round++; @@ -637,6 +650,13 @@ class AcpSubagentExecutor implements SubagentExecutor { this.terminateMode = AgentTerminateMode.TIMEOUT; break; } + // Record the caller's entry messages as delivered only now that the + // budget guard has committed to dispatching this round, and only on the + // entry round. Keyed on entryRound, NOT this.round === 1, because + // resetStats:false preserves this.round across continuations, so a + // continuation's first round is >= 2. Emitting before the prompt keeps + // user-before-assistant order in the JSONL transcript. (R12-2) + if (inputs && this.round === entryRound + 1) this.emitInputs(inputs); const result = await this.wait( this.connection.prompt({ sessionId: this.sessionId, prompt: next }), remaining, @@ -1070,6 +1090,10 @@ class AcpSubagentExecutor implements SubagentExecutor { // authorizing — not just the option labels the dialog already // renders as buttons (R11-5). Bounded and control-stripped. prompt: describeExternalAction(params.toolCall), + // The prompt is a foreign-process payload; render it as plain text, + // never markdown — otherwise the dialog would eat glob `**` and + // render links, misrepresenting what the user approves. (R11-5 fix) + renderPromptAsPlainText: true, hideAlwaysAllow: true, }, respond: async (outcome) => diff --git a/packages/core/src/agents/background-agent-resume.test.ts b/packages/core/src/agents/background-agent-resume.test.ts index b757511619c..f666c4ba8fe 100644 --- a/packages/core/src/agents/background-agent-resume.test.ts +++ b/packages/core/src/agents/background-agent-resume.test.ts @@ -23,6 +23,7 @@ import { } from './agent-transcript.js'; import { ToolNames } from '../tools/tool-names.js'; import { AgentTerminateMode } from './runtime/agent-types.js'; +import { SubagentError, SubagentErrorCode } from '../subagents/types.js'; import { AgentEventEmitter } from './runtime/agent-events.js'; import { getCurrentAgentDepth } from './runtime/agent-context.js'; import { AgentHeadless } from './runtime/agent-headless.js'; @@ -686,6 +687,55 @@ describe('BackgroundAgentResumeService', () => { expect(subagentManager.loadSubagent).toHaveBeenCalledWith('deleted-agent'); }); + it('keeps a paused agent listed when its same-named definition now fails the executor guard (R12-3)', async () => { + const sessionId = 'session-executor-refusal'; + const agentId = 'agent-executor-refusal'; + writeAgentMeta(getAgentMetaPath(tempDir, sessionId, agentId), { + agentId, + agentType: 'researcher', + description: + 'Background task whose same-named definition now fails to load', + parentSessionId: sessionId, + parentAgentId: null, + createdAt: '2026-04-20T00:00:00.000Z', + status: 'running', + subagentName: 'researcher', + resolvedApprovalMode: 'default', + }); + fs.writeFileSync( + getAgentJsonlPath(tempDir, sessionId, agentId), + JSON.stringify({ + uuid: 'u1', + parentUuid: null, + sessionId, + timestamp: '2026-04-20T00:00:00.000Z', + type: 'user', + message: { role: 'user', parts: [{ text: 'task' }] }, + }) + '\n', + 'utf8', + ); + + const { service, subagentManager } = createService(); + // The R10-2/R11 executor refusal makes loadSubagent THROW for a same-named + // file that failed to load. resolveResumeTarget must convert that throw into + // the existing "unavailable" shape so discovery keeps the row listed with a + // resumeBlockedReason — otherwise the per-sidecar catch swallows it into a + // debug-only warning and the row vanishes from /tasks. Removing the try/catch + // turns this red (recovered is empty). + subagentManager.loadSubagent.mockRejectedValue( + new SubagentError( + 'Agent file /test/project/.qwen/agents/researcher.md has an invalid executor block: it declares an executor but failed to load.', + SubagentErrorCode.INVALID_CONFIG, + 'researcher', + ), + ); + const recovered = await service.loadPausedBackgroundAgents(sessionId); + expect(recovered).toHaveLength(1); + expect(recovered[0]?.resumeBlockedReason).toContain( + 'invalid executor block', + ); + }); + it('keeps paused tasks resumable when they only carry a stale lastError', async () => { const sessionId = 'session-stale-error'; const agentId = 'agent-stale-error'; diff --git a/packages/core/src/agents/background-agent-resume.ts b/packages/core/src/agents/background-agent-resume.ts index e01ef87499c..6c817df971b 100644 --- a/packages/core/src/agents/background-agent-resume.ts +++ b/packages/core/src/agents/background-agent-resume.ts @@ -1580,9 +1580,26 @@ export class BackgroundAgentResumeService { }; } - const subagentConfig = await this.config - .getSubagentManager() - .loadSubagent(subagentName); + let subagentConfig: SubagentConfig | null; + try { + subagentConfig = await this.config + .getSubagentManager() + .loadSubagent(subagentName); + } catch (error) { + // loadSubagent throws a recorded executor-block refusal (R10-2/R11) when a + // same-named definition failed to load. This is resume *discovery*, not a + // dispatch, so surface it as the existing "unavailable" shape — the row + // stays listed with a resumeBlockedReason — instead of letting the throw + // escape into the per-sidecar catch, which would drop the row entirely. + return { + agentName: subagentName, + isFork: false, + unavailableReason: + error instanceof Error + ? error.message + : `Subagent "${subagentName}" is no longer available.`, + }; + } if (!subagentConfig) { return { agentName: subagentName, diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index bcc0c5f807c..1962eefbf1a 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -1871,6 +1871,74 @@ You are a helpful assistant.`; ); }); + it('clears a stale executor refusal when the directory later becomes unreadable (R12-4)', async () => { + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + // First scan: a malformed-executor file records a refusal for 'explore'. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.mocked(fs.readdir).mockResolvedValue(['explore.md'] as any); + vi.mocked(fs.readFile).mockResolvedValue( + '---\nname: Explore\ndescription: Test\nexecutor:\n kind: ACP\n command: npx\n---\nPrompt', + ); + await expect(manager.loadSubagent('Explore')).rejects.toThrow( + /invalid executor block/, + ); + // The directory then disappears / becomes unreadable (git checkout of a + // branch with no agents dir, rm -rf, an unreadable dir). Without resetting + // the level's refusals on the scan-failure path, loadSubagent keeps + // throwing the stale refusal for a file that no longer exists, leaving the + // builtin permanently unreachable. With the reset, the scan returns [] and + // the dispatch falls through to the builtin again. Deleting the catch reset + // turns this red (the second call rejects with the stale error). + vi.mocked(fs.readdir).mockRejectedValue( + new Error('ENOENT: no such directory'), + ); + const resolved = await manager.loadSubagent('Explore'); + expect(resolved?.isBuiltin).toBe(true); + }); + + it('does not refuse an in-process definition whose block-scalar prose mentions executor: (R12-5)', async () => { + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.mocked(fs.readdir).mockResolvedValue(['foo.md'] as any); + // A `description: |` block scalar documents the executor syntax as prose; a + // duplicate `name:` key is a tolerated YAML quirk (control: it loads + // alone). The raw-text probe matches the prose `executor:` line, so without + // the block-scalar exclusion `claimsExecutor` is true and astLostExecutor + // refuses (the duplicate key makes has('executor') false). With the + // exclusion the prose match is not a claim, no refusal is recorded, and the + // in-process definition loads — reverting the exclusion turns this red + // (loadSubagent rejects /invalid executor block/). + vi.mocked(fs.readFile).mockResolvedValue( + '---\nname: foo\nname: foo\ndescription: |\n Reviews code. To run externally use:\n executor: acp\n for details.\n---\nPrompt', + ); + const config = await manager.loadSubagent('foo'); + expect(config).not.toBeNull(); + expect(config!.name).toBe('foo'); + expect(config!.executor).toBeUndefined(); + }); + + it('does not refuse an in-process definition whose folded-block-scalar prose mentions executor: (R12-5)', async () => { + const yaml = await vi.importActual< + typeof import('../utils/yaml-parser.js') + >('../utils/yaml-parser.js'); + mockParseYaml.mockImplementation(yaml.parse); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.mocked(fs.readdir).mockResolvedValue(['foo.md'] as any); + // Same as above but with a folded block scalar (`description: >`). + vi.mocked(fs.readFile).mockResolvedValue( + '---\nname: foo\nname: foo\ndescription: >\n Reviews code. To run externally use:\n executor: acp\n for details.\n---\nPrompt', + ); + const config = await manager.loadSubagent('foo'); + expect(config).not.toBeNull(); + expect(config!.executor).toBeUndefined(); + }); + it('refuses a by-name dispatch for an executor-claiming file that fails an earlier validation (R11-1)', async () => { // A project file declares an executor but OMITS the description, so it // fails the earlier required-field validation BEFORE the executor block is diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index 3c882e1f90d..286ef5a85db 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -8,7 +8,7 @@ import * as fs from 'fs/promises'; import * as path from 'path'; import * as os from 'os'; import { randomUUID } from 'crypto'; -import { parseDocument } from 'yaml'; +import { isScalar, parseDocument, visit } from 'yaml'; import { parse as parseYaml, stringify as stringifyYaml, @@ -1629,6 +1629,10 @@ export class SubagentManager { // If project level is requested but project root is same as home directory, // return empty array to avoid conflicts between project and global agents if (level === 'project' && isHomeDirectory) { + // No fresh scan happens on this path, so clear any refusal a prior scan + // recorded for the level — otherwise loadSubagent keeps throwing a stale + // refusal for a file this path no longer reads. (R12-4) + this.executorRefusals.set(level, new Map()); return []; } @@ -1675,7 +1679,10 @@ export class SubagentManager { this.executorRefusals.set(level, refusals); return subagents; } catch (_error) { - // Directory doesn't exist or can't be read + // Directory doesn't exist or can't be read. Clear any refusal a prior + // scan recorded for this level so a stale entry cannot keep refusing a + // by-name dispatch for a file that no longer exists or is unreadable. (R12-4) + this.executorRefusals.set(level, new Map()); return []; } } @@ -1787,6 +1794,39 @@ export async function loadSubagentFromDir( } } +/** + * R12-5: returns true when a raw-text `executor:` probe match at `matchIndex` + * lies inside a block scalar (`description: |` or `>`), i.e. the match is prose + * documenting the executor syntax, not a real claim. The R10-1 probe is + * indentation-tolerant, so without this a `description: |` line reading + * `executor: acp` would hoist into a claim and hard-refuse an in-process + * definition that merely carries an unrelated tolerated YAML quirk. Fail-closed: + * if the AST walk itself throws, return false so the match stays a claim. + */ +function probeMatchInsideBlockScalar( + document: ReturnType, + matchIndex: number, +): boolean { + try { + let inside = false; + visit(document, (_key, node) => { + if ( + isScalar(node) && + (node.type === 'BLOCK_LITERAL' || node.type === 'BLOCK_FOLDED') && + node.range && + node.range[0] <= matchIndex && + matchIndex < node.range[1] + ) { + inside = true; + } + }); + return inside; + } catch { + // A failed walk must not drop a real claim; leave the match as a claim. + return false; + } +} + function parseSubagentContent( content: string, filePath: string, @@ -1831,7 +1871,16 @@ function parseSubagentContent( const executorClaimMatch = /^[ \t]*["']?executor["']?[ \t]*:/m.exec( frontmatterYaml, ); - claimsExecutor = hasExecutor || executorClaimMatch !== null; + // R12-5: a probe match lying inside a block scalar is prose (a + // `description: |`/`>` documenting the executor syntax), not a claim — + // exclude it so the refusal guards below do not fire on an in-process + // definition that only mentions `executor:` in prose. A genuine top-level + // (even TAB-misindented) executor key is not inside a block scalar, so it + // stays a claim and the R10-1/R9-2 refusals still hold. + const probeIsClaim = + executorClaimMatch !== null && + !probeMatchInsideBlockScalar(document, executorClaimMatch.index); + claimsExecutor = hasExecutor || probeIsClaim; try { const nodeName = document.has('name') ? document.get('name') : undefined; if (typeof nodeName === 'string' && nodeName !== '') From 9aa279a4a4466effe3bc80dcf0d6fd50c27091fa Mon Sep 17 00:00:00 2001 From: probe Date: Thu, 10 Sep 2026 09:55:17 +0800 Subject: [PATCH 5/5] fix(subagent): resolve the two carried Criticals via maintainer decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the two Criticals standing since round 3, per the maintainer's decision on the round-12 review: both are resolved by an explicit, surfaced limitation rather than an untestable/billing-risky implementation. Each is mutation-verified (reverting the fix turns its test red). R3-1/R3-22 (Windows spawn) — decision: POSIX-only for this release. On Windows an npm-installed adapter resolves to a `.cmd` launcher that libuv's PATH search never finds (bare name + `.exe` only) and Node >= 18.20.2 refuses to spawn without a shell; the process-tree reaping is likewise POSIX-specific. The executor previously failed at dispatch with a misleading `spawn ENOENT` (reporting an installed adapter as missing). A new `assertExternalAgentSpawnPlatformSupported` guard now fails closed at the top of `create()` with a clear, actionable "POSIX-only in this release" error, before any spawn. A cross-spawn-style PATHEXT resolution + quoted `cmd.exe` arm is a tracked follow-up (it changes the security-critical detached-spawn path and is untestable without a Windows lane). R3-6/R3-51 (mid-turn waiter) — decision: explicitly decline + surface the limitation. ACP v1 has no mid-turn injection primitive (no `session/steer`), so the executor deliberately does NOT implement `setExternalMessageWaiter` / `setExternalMessageWaitPredicate` (documented on the class); input arriving mid-prompt is delivered at the next turn boundary via the provider. To stop the background loop presenting a queued steer as if delivered mid-turn, the delegation result for an external executor now appends `EXTERNAL_MID_TURN_INPUT_NOTICE` (alongside the existing usage notice) stating queued input is delivered between turns, not mid-turn. True mid-turn steering (cancel + re-prompt) re-bills the in-flight turn — a protocol/billing decision, tracked as a follow-up. Verification: cli + core tsc 0 errors; ESLint + Prettier clean; cli executor suite 63/63 (incl. a win32-stubbed `create()` rejection that never reaches spawn); core agent suite 288/288; core subagent/agent/runtime regression net 2518 passed (6 skipped). Mutation proofs: removing the win32 throw fails both R3-1 tests; dropping the mid-turn notice fails the R3-6 result assertion. Reverse-audit converged to clean passes over the production and test diffs. --- .../acp-subagent-executor.test.ts | 28 ++++++++++++++ .../external-agents/acp-subagent-executor.ts | 38 +++++++++++++++++++ packages/core/src/tools/agent/agent.test.ts | 24 ++++++++++++ packages/core/src/tools/agent/agent.ts | 9 ++++- 4 files changed, 98 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/external-agents/acp-subagent-executor.test.ts b/packages/cli/src/external-agents/acp-subagent-executor.test.ts index 58b82936783..62aa5e4f9a4 100644 --- a/packages/cli/src/external-agents/acp-subagent-executor.test.ts +++ b/packages/cli/src/external-agents/acp-subagent-executor.test.ts @@ -17,6 +17,7 @@ import { import { Config, InputFormat } from '@qwen-code/qwen-code-core'; import { acpExternalAgentExecutor, + assertExternalAgentSpawnPlatformSupported, externalModelLabel, isExpectedExternalAgentCleanupExit, isUnprovenExternalAgentTreeExit, @@ -356,6 +357,33 @@ describe('cleanup-exit classification', () => { // signalCode null, which dispose() surfaces as an unclean-exit error — the same // exposure the repo's other real-process suites gate (process-registry.process, // hook-runner.process). The pure-function suites above run on every platform. +describe('spawn platform guard (R3-1)', () => { + it('refuses with a clear POSIX-only error on win32, allows POSIX', () => { + // The guard throws BEFORE any spawn, so on win32 a user gets a clear + // "POSIX-only" refusal instead of a misleading `spawn ENOENT` (an + // npm-installed launcher resolving to a `.cmd` libuv never finds). + // Removing the win32 throw turns the first assertion red. + expect(() => assertExternalAgentSpawnPlatformSupported('win32')).toThrow( + /POSIX-only/, + ); + expect(() => + assertExternalAgentSpawnPlatformSupported('darwin'), + ).not.toThrow(); + expect(() => + assertExternalAgentSpawnPlatformSupported('linux'), + ).not.toThrow(); + }); + + it('create() rejects on win32 without spawning', async () => { + const realPlatform = process.platform; + Object.defineProperty(process, 'platform', { value: 'win32' }); + try { + await expect(create(params())).rejects.toThrow(/POSIX-only/); + } finally { + Object.defineProperty(process, 'platform', { value: realPlatform }); + } + }); +}); describe.skipIf(process.platform === 'win32')('real ACP subprocess', () => { it('returns method-not-found -32601 across the real extension request wire', async () => { const executor = await create(params('unsupported-extension')); diff --git a/packages/cli/src/external-agents/acp-subagent-executor.ts b/packages/cli/src/external-agents/acp-subagent-executor.ts index 53c28d38c64..4aed6b106e7 100644 --- a/packages/cli/src/external-agents/acp-subagent-executor.ts +++ b/packages/cli/src/external-agents/acp-subagent-executor.ts @@ -91,6 +91,29 @@ export function describeExternalAction( : rendered; } +/** + * R3-1: the executor spawn is POSIX-only in this release. On Windows an + * npm-installed launcher resolves to a `.cmd`/`.bat`, which libuv's PATH search + * never finds (bare name + `.exe` only) and which Node >= 18.20.2 refuses to + * spawn without a shell — and the process-tree reaping (detached process-group + * SIGTERM/SIGKILL) is POSIX-specific. Rather than let the spawn fail with a + * misleading `spawn ENOENT` (reporting an installed adapter as missing), + * fail closed here with a clear, actionable message. Windows support (a + * cross-spawn-style PATHEXT resolution + quoted `cmd.exe` arm) is a tracked + * follow-up. + */ +export function assertExternalAgentSpawnPlatformSupported( + platform: NodeJS.Platform = process.platform, +): void { + if (platform === 'win32') { + throw new Error( + 'External ACP agents are not supported on Windows in this release: the executor spawn is POSIX-only. ' + + 'An npm-installed adapter resolves to a `.cmd` launcher that cannot be spawned without a shell, and the process-tree reaping is POSIX-specific. ' + + 'Run the external agent on macOS/Linux (or WSL). Windows support is a tracked follow-up.', + ); + } +} + /** * The host approval policy as a peer-vocabulary-independent token. Peer ACP mode * ids differ per peer (Claude: `acceptEdits` / `bypassPermissions`; qwen: @@ -338,6 +361,12 @@ class AcpSubagentExecutor implements SubagentExecutor { static async create( params: ExternalAgentExecutorParams, ): Promise { + // Fail closed with a clear message on Windows rather than letting the spawn + // below fail with a misleading `spawn ENOENT` (an npm-installed + // launcher resolves to a `.cmd`, which libuv's PATH search never finds and + // Node refuses to spawn without a shell). The executor's spawn and its + // process-tree reaping are POSIX-only in this release. (R3-1) + assertExternalAgentSpawnPlatformSupported(); if (params.runConfig.max_turns !== undefined) { throw new Error('External ACP agents cannot enforce max_turns.'); } @@ -791,6 +820,15 @@ class AcpSubagentExecutor implements SubagentExecutor { setExternalMessageProvider(provider: () => AgentExternalInput[]): void { this.provider = provider; } + // This executor deliberately does NOT implement `setExternalMessageWaiter` / + // `setExternalMessageWaitPredicate`: ACP v1 has no mid-turn injection + // primitive (no `session/steer`), so input that arrives while a prompt is in + // flight cannot be delivered until that prompt resolves. Queued input is + // drained between prompts via the provider above (next-turn-boundary + // delivery). Implementing true mid-turn steering would require cancel + + // re-prompt — re-billing the whole in-flight turn — which is a protocol/billing + // decision, not a safe default. The delegation result surfaces this + // limitation (see agent.ts EXTERNAL_MID_TURN_INPUT_NOTICE). (R3-6) getExecutionSummary(): AgentStatsSummary { return { rounds: this.round, diff --git a/packages/core/src/tools/agent/agent.test.ts b/packages/core/src/tools/agent/agent.test.ts index 17a29971613..c060cbd4449 100644 --- a/packages/core/src/tools/agent/agent.test.ts +++ b/packages/core/src/tools/agent/agent.test.ts @@ -6168,6 +6168,30 @@ describe('AgentTool', () => { expect(failureMessage).toContain('token usage and cost are unavailable'); }); + it('surfaces that mid-turn input is unavailable for a background external agent (R3-6)', async () => { + vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue({ + ...bgSubagent, + executor: { kind: 'acp', command: 'claude' }, + }); + const invocation = ( + agentTool as AgentToolWithProtectedMethods + ).createInvocation({ + description: 'External task', + prompt: 'Do the task', + subagent_type: 'monitor', + run_in_background: true, + }); + await invocation.execute(); + await vi.waitFor(() => expect(mockRegistry.complete).toHaveBeenCalled()); + const completionText = mockRegistry.complete.mock.calls[0]?.[1] as string; + // ACP v1 has no mid-turn injection primitive, so a steer sent while a turn + // is running reaches the peer only at the next turn boundary. The result + // must say so, not present a queued steer as delivered mid-turn. Dropping + // EXTERNAL_MID_TURN_INPUT_NOTICE turns the second assertion red. + expect(completionText).toContain('token usage and cost are unavailable'); + expect(completionText).toContain('next turn boundary'); + }); + it('should run in background when agent definition has background: true', async () => { const writeMetaSpy = vi.spyOn(transcript, 'writeAgentMeta'); const attachSpy = vi.spyOn(transcript, 'attachJsonlTranscriptWriter'); diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index 0857c657e5e..6e2199922f8 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -137,6 +137,13 @@ import type { AuthOverrides } from '../../models/content-generator-config.js'; const EXTERNAL_USAGE_NOTICE = '\n\n[External executor token usage and cost are unavailable.]'; +// ACP v1 has no mid-turn injection primitive, so an external agent cannot be +// steered while a prompt is in flight — queued input is delivered at the next +// turn boundary. Surface that on the result so a caller does not read a queued +// steer as having been delivered mid-turn. (R3-6) +const EXTERNAL_MID_TURN_INPUT_NOTICE = + '\n\n[External agents receive queued input only between turns; a message sent while a turn is running is delivered at the next turn boundary, not mid-turn.]'; + function persistBackgroundCancellation( metaPath: string, persistedStatus: 'running' | 'cancelled', @@ -3593,7 +3600,7 @@ class AgentToolInvocation extends BaseToolInvocation { // path already appends it after its fallbacks; mirror that. const externalSuffix = subagentConfig.executor !== undefined - ? EXTERNAL_USAGE_NOTICE + ? EXTERNAL_USAGE_NOTICE + EXTERNAL_MID_TURN_INPUT_NOTICE : ''; const modelVisibleText = toModelVisibleSubagentResult( subagentRawText,