From 93106b87b4cbed9dcbf7905404620c3ca96f264b Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 30 Jun 2026 21:52:16 +0800 Subject: [PATCH] fix(subagent): deep-clone lastAssistantMessage onto subagent/end (observe-only) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review caught an observe-only violation: the subagent/end emit fires from a detached `.then` registered BEFORE start() returns — so before the caller's own `await run.result` continuation runs. Carrying `result.output` by reference let a mutating subagent/end listener corrupt the SubagentResult.output the caller/tool then consumes. structuredClone() makes the event a read-only snapshot. Added a regression test that mutates the event's array and asserts the caller's result is untouched; proven to fail red without the clone. Updated the RFC + READMEs to note the clone is load-bearing for the observe-only guarantee. --- .../2026-06-30-subagent-observe-enrich.md | 2 +- packages/subagent/subagent/README.md | 2 +- packages/subagent/subagent/src/index.ts | 10 ++++++- .../subagent/subagent/tests/service.spec.ts | 28 +++++++++++++++++++ 4 files changed, 39 insertions(+), 3 deletions(-) diff --git a/docs/rfc/implemented/feature/2026-06-30-subagent-observe-enrich.md b/docs/rfc/implemented/feature/2026-06-30-subagent-observe-enrich.md index 46cf403666..a1893ed99c 100644 --- a/docs/rfc/implemented/feature/2026-06-30-subagent-observe-enrich.md +++ b/docs/rfc/implemented/feature/2026-06-30-subagent-observe-enrich.md @@ -16,7 +16,7 @@ Add two pieces of information to the subagent lifecycle surface: 1. **`agentType` — a caller-supplied subagent-kind label**, the harness analogue of CC's `subagent_type`. It is optional on `SubagentStartRequest`, carried VERBATIM onto both `subagent/start` (`SubagentRunInfo`) and `subagent/end` (`SubagentRunEndInfo`). The seam never interprets it. The model-facing `dsh-tool-subagent` tool threads it from a new optional `Config.agentType`, so a deployment that exposes multiple subagent kinds (one tool load per kind) labels each. Absent when the caller does not distinguish kinds (the spread omits the key — `exactOptionalPropertyTypes`-correct). -2. **`lastAssistantMessage` — the child's final output**, added to `SubagentRunEndInfo`. On the settle path it is `SubagentResult.output` (so an observer sees WHAT the subagent produced without holding the run). On the REJECT path (an infrastructure fault where no `SubagentResult` was produced — the seam only knows `stopReason: 'error'`) it is absent. +2. **`lastAssistantMessage` — the child's final output**, added to `SubagentRunEndInfo`. On the settle path it is a DEEP CLONE of `SubagentResult.output` (so an observer sees WHAT the subagent produced without holding the run). On the REJECT path (an infrastructure fault where no `SubagentResult` was produced — the seam only knows `stopReason: 'error'`) it is absent. The clone is load-bearing for observe-only: the `subagent/end` emit fires from a detached `.then` registered *before* `start()` returns, i.e. before the caller's own `await run.result` continuation — handing listeners the same array reference would let a mutating listener corrupt the caller's `SubagentResult.output`. `structuredClone` makes the event a read-only view (a regression test mutates the event's array and asserts the caller's result is untouched). Both events stay plain **`emit`s**. `subagent/end` fires from a detached `.then` on `run.result` and awaits no listener, so it is genuinely observe-only by construction — a `subagent/start` listener can still reach the live child via `ctx.agents.get(info.id)` and `inject()` into it; a `subagent/end` listener can only observe (the run has settled). Per-listener containment (already in place) keeps one bad subscriber from stranding a live run or surfacing as an unhandled rejection on the detached settle hook. diff --git a/packages/subagent/subagent/README.md b/packages/subagent/subagent/README.md index 7d6c58af3c..6db26272c7 100644 --- a/packages/subagent/subagent/README.md +++ b/packages/subagent/subagent/README.md @@ -32,7 +32,7 @@ Unlike the bash seam (one executor per context, second load throws), **multiple `provider.start(request)` returns a `SubagentRun`: a handle with a `result` promise, `cancel()`, `dispose()`, and the optional runtime methods. `result` resolves with a `SubagentResult` (`output`, optional `structured`, `stopReason`) — it does **not** reject on a child-level failure (a model/transport failure resolves with `stopReason: 'error'`), so the consumer maps a non-`completed` reason to an `isError` tool result. The consumer MUST `dispose()` on every path (success, error, abort) to reach child quiescence and avoid leaking an idle child / session. -The service emits `subagent/start` (payload `SubagentRunInfo`) and `subagent/end` (payload `SubagentRunEndInfo`) around the run — both **observe-only** (plain `emit`s; `subagent/end` fires from a detached `.then` and awaits no listener). Both payloads carry the request's optional `agentType` label (Claude Code's `subagent_type`, verbatim — the seam never interprets it); `subagent/end` additionally carries `lastAssistantMessage` (the child's final `output`) on the settle path, absent when the run rejected at the infrastructure level. A `subagent/start` listener can still reach the live child via `ctx.agents.get(info.id)`; a `subagent/end` listener can only observe (the run has settled). Any run-affecting decision (continuation, injection that changes the run) is out of scope for this observe-only surface. +The service emits `subagent/start` (payload `SubagentRunInfo`) and `subagent/end` (payload `SubagentRunEndInfo`) around the run — both **observe-only** (plain `emit`s; `subagent/end` fires from a detached `.then` and awaits no listener). Both payloads carry the request's optional `agentType` label (Claude Code's `subagent_type`, verbatim — the seam never interprets it); `subagent/end` additionally carries `lastAssistantMessage` (a deep clone of the child's final `output`) on the settle path, absent when the run rejected at the infrastructure level. The clone keeps the surface observe-only: the end emit fires from a detached `.then` before the caller's `await run.result` resumes, so a shared reference would let a mutating listener corrupt the caller's result. A `subagent/start` listener can still reach the live child via `ctx.agents.get(info.id)`; a `subagent/end` listener can only observe (the run has settled). Any run-affecting decision (continuation, injection that changes the run) is out of scope for this observe-only surface. ## Scope (first cut) diff --git a/packages/subagent/subagent/src/index.ts b/packages/subagent/subagent/src/index.ts index 916c53e759..ec66118ec6 100644 --- a/packages/subagent/subagent/src/index.ts +++ b/packages/subagent/subagent/src/index.ts @@ -201,7 +201,15 @@ export class SubagentService extends Service { // Per-listener containment also keeps a thrown `subagent/end` listener from // becoming an unhandled rejection on this detached `.then`. void run.result.then( - (result) => { this.emitLifecycle('subagent/end', { provider: name, id: run.id, ...agentType, stopReason: result.stopReason, lastAssistantMessage: result.output }) }, + (result) => { + // Deep-clone the output onto the event: this detached `.then` runs BEFORE + // the caller's own `await run.result` continuation, so handing listeners + // the SAME array reference the caller consumes would let a mutating + // `subagent/end` listener corrupt the caller's SubagentResult.output — + // breaking the observe-only contract. A snapshot makes the event a + // read-only view, not a shared handle. + this.emitLifecycle('subagent/end', { provider: name, id: run.id, ...agentType, stopReason: result.stopReason, lastAssistantMessage: structuredClone(result.output) }) + }, () => { this.emitLifecycle('subagent/end', { provider: name, id: run.id, ...agentType, stopReason: 'error' }) }, ) return run diff --git a/packages/subagent/subagent/tests/service.spec.ts b/packages/subagent/subagent/tests/service.spec.ts index e23ae4ff3b..2e5a3ef8a7 100644 --- a/packages/subagent/subagent/tests/service.spec.ts +++ b/packages/subagent/subagent/tests/service.spec.ts @@ -223,6 +223,34 @@ describe('SubagentService', () => { expect(endInfo.lastAssistantMessage).toEqual([{ type: 'text', text: 'ok' }]) }) + it('observe-only: a subagent/end listener mutating lastAssistantMessage cannot corrupt the caller\'s result', async () => { + // The subagent/end emit fires from a detached `.then` registered before + // start() returns — i.e. BEFORE the caller's own `await run.result` + // continuation. If the event shared the result.output reference, a mutating + // listener would change the SubagentResult the caller consumes. The service + // deep-clones output onto the event, so the listener mutates only its copy. + const ctx = new Context() + await ctx.plugin(SubagentService) + ctx.subagents.registerProvider(new StubProvider( + 'clone', + ALL_CAPS, + { output: [{ type: 'text', text: 'original' }], stopReason: 'completed' }, + )) + + ctx.on('subagent/end', (info) => { + // A hostile/buggy listener reaches in and mutates the event's array. + const blocks = info.lastAssistantMessage + if (blocks?.[0]?.type === 'text') blocks[0].text = 'HIJACKED' + blocks?.push({ type: 'text', text: 'injected' }) + }) + + const run = ctx.subagents.start('clone', baseRequest()) + const result = await run.result + await Promise.resolve() // let the detached settle hook (and its listener) run + // The caller's result.output is untouched by the listener's mutation. + expect(result.output).toEqual([{ type: 'text', text: 'original' }]) + }) + it('omits lastAssistantMessage on the reject path (no SubagentResult was produced)', async () => { const ctx = new Context() await ctx.plugin(SubagentService)