Fix subagent in-process result scoping (Codex review round 1)
Two merge-blocking bugs in the shared in-process run driver, both rooted in `readResult` scanning the whole child session and deriving the stop reason only from `turn/end`: - A pre-turn `cancel()` cleared the queued prompt before any `turn/end` was logged, so the run settled `error` instead of `aborted`, violating the `SubagentRun.cancel()` contract. The driver now tracks that a cancel was requested and maps the no-turn case to `aborted`. - A fork child whose own turn produced no `assistant/message` returned the SEEDED parent's last message as a `completed` success. `readResult` now scopes to the child's OWN events (after the seed prefix), so a message-less child yields empty output. Both fixes carry a regression test proven to go red on the pre-fix driver. Also: correct the `SubagentRun.id` / event-payload docs (it is the child AGENT id, not a session id — the backend mints distinct tokens); refresh the stale `coding-agent` welcome string (subagent is now a tool); and replace the stale `TODO(sub-agents)` "deferred" prose in the Agent interface, core.md, and architecture.md with an accurate pointer to the realized seam.
This commit is contained in:
@@ -121,10 +121,6 @@ export class AgentLoop extends Service implements AgentFactory {
|
||||
* deliberate resume-or-create policy (resume the prior session if one exists,
|
||||
* else start fresh) or an explicit caller-chosen session id — revisit when the
|
||||
* UI/ACP path owns session selection.
|
||||
*
|
||||
* TODO(sub-agents): spawn/fork land here — accept a parent agent reference;
|
||||
* fork seeds the new Session with the parent's event log, spawn starts
|
||||
* fresh; the child is returned as a regular Agent handle.
|
||||
*/
|
||||
create(id: AgentId, options: AgentOptions = {}): ReactLoopAgent {
|
||||
this.assertAgentIdFree(id)
|
||||
|
||||
@@ -118,11 +118,12 @@ export interface Agent {
|
||||
*/
|
||||
whenIdle(): Promise<void>
|
||||
|
||||
// TODO(sub-agents): spawn/fork seams — semantics deliberately deferred.
|
||||
// The intended shape: a creation option referencing a parent agent
|
||||
// (fork = seed the child Session with the parent's event log; spawn =
|
||||
// fresh Session), with the child returned as an Agent handle so steer()
|
||||
// and event subscription work uniformly. See docs/architecture.md.
|
||||
// Subagent delegation is realized on top of this interface by the
|
||||
// `@deepseek-ai/dsh-subagent` seam, not by a method here: a backend creates
|
||||
// the child through `ctx.agents.create` (fork seeds the child Session with a
|
||||
// balanced prefix of the parent's log via `CreateAgentOptions.seed`; spawn
|
||||
// starts fresh) and drives it as an ordinary Agent handle, so steer() and
|
||||
// event subscription work uniformly. See docs/core-data-structures/subagent.md.
|
||||
}
|
||||
|
||||
declare module 'cordis' {
|
||||
|
||||
@@ -10,11 +10,16 @@ import AgentLoop from '@deepseek-ai/dsh-agent-loop'
|
||||
import * as Invariants from '@deepseek-ai/dsh-invariants'
|
||||
import SubagentService from '@deepseek-ai/dsh-subagent'
|
||||
import { MockAdapter, textResponse } from '../../../core/agent-loop/tests/mock-adapter.ts'
|
||||
import type { StreamChunk } from '@deepseek-ai/dsh-llm'
|
||||
import * as fork from '../src/index.ts'
|
||||
import { completedTurnPrefix } from '../src/index.ts'
|
||||
|
||||
type Script = ConstructorParameters<typeof MockAdapter>[0]
|
||||
|
||||
/** A bare `stop` finish that streams no content → the turn ends `completed`
|
||||
* with NO `assistant/message` of its own. */
|
||||
const emptyStop: StreamChunk[] = [{ type: 'finish', reason: { kind: 'stop' } }]
|
||||
|
||||
/**
|
||||
* Drives the REAL fork backend with a real loop + scripted mock MODEL + the
|
||||
* real dsh-invariants plugin. The invariants plugin re-replays a seeded child
|
||||
@@ -132,6 +137,26 @@ describe('dsh-subagent-fork', () => {
|
||||
await run.dispose()
|
||||
})
|
||||
|
||||
it('does NOT return the seeded parent output when the child produces no message of its own', async () => {
|
||||
// Regression: readResult must scope to the child's OWN events (after the
|
||||
// seed). The parent completes a turn with a distinctive assistant message,
|
||||
// then the fork child's own turn finishes with a bare `stop` and NO
|
||||
// assistant/message. Scanning the whole (seeded) log would return the
|
||||
// parent's "parent stale" message with stopReason 'completed'; scoped to the
|
||||
// child's own events the output is empty.
|
||||
const { ctx, parent } = await setup([textResponse('parent stale'), emptyStop])
|
||||
parent.send([{ type: 'text', text: 'parent question' }])
|
||||
await parent.whenIdle()
|
||||
|
||||
const run = ctx.subagents.start('fork', { prompt: [{ type: 'text', text: 'child question' }], parent })
|
||||
const result = await run.result
|
||||
// The child completed its own (empty) turn — completed, but with NO output
|
||||
// borrowed from the seeded parent prefix.
|
||||
expect(result.stopReason).toBe('completed')
|
||||
expect(result.output).toEqual([])
|
||||
await run.dispose()
|
||||
})
|
||||
|
||||
it('advertises depthLimit but not outputSchema/toolFilter', async () => {
|
||||
const { ctx } = await setup([])
|
||||
expect(ctx.subagents.getProvider('fork')!.capabilities).toEqual({ outputSchema: false, depthLimit: true, toolFilter: false })
|
||||
|
||||
@@ -99,6 +99,11 @@ export function startInProcessRun(
|
||||
}
|
||||
|
||||
const childId = AgentId(randomUUID())
|
||||
// The child's OWN events begin after the seed (fork seeds the parent's
|
||||
// completed-turn prefix; spawn seeds nothing). `readResult` scopes to this
|
||||
// boundary so a child that produces no message of its own never returns the
|
||||
// SEEDED parent's last assistant message as its result.
|
||||
const seedLength = options.seed?.length ?? 0
|
||||
const parentHeader = request.parent.session.header
|
||||
// Inherit the parent's model by default (a child with no model cannot run);
|
||||
// an explicit `request.agentOptions.model` overrides it. The parent's
|
||||
@@ -124,14 +129,23 @@ export function startInProcessRun(
|
||||
|
||||
// Bridge the request's abort signal to the child (the consumer also bridges
|
||||
// its own exec.signal, but a backend-level bridge keeps the contract local).
|
||||
const onAbort = (): void => { child.cancel('subagent cancelled') }
|
||||
// `cancelled` records that a cancel was requested at all, so the pre-turn
|
||||
// cancel window — where the child clears the queued prompt before any
|
||||
// `turn/end` is logged — settles as `aborted` (honoring the cancel contract)
|
||||
// rather than falling through to the no-turn `error` mapping.
|
||||
let cancelled = false
|
||||
const requestCancel = (reason: string): void => {
|
||||
cancelled = true
|
||||
child.cancel(reason)
|
||||
}
|
||||
const onAbort = (): void => { requestCancel('subagent cancelled') }
|
||||
request.signal?.addEventListener('abort', onAbort, { once: true })
|
||||
|
||||
const result: Promise<SubagentResult> = (async () => {
|
||||
try {
|
||||
child.send(request.prompt)
|
||||
await child.whenIdle()
|
||||
return readResult(child)
|
||||
return readResult(child, seedLength, cancelled)
|
||||
} finally {
|
||||
request.signal?.removeEventListener('abort', onAbort)
|
||||
}
|
||||
@@ -141,7 +155,7 @@ export function startInProcessRun(
|
||||
id: childId,
|
||||
result,
|
||||
cancel(reason?: string): void {
|
||||
child.cancel(reason ?? 'subagent cancelled')
|
||||
requestCancel(reason ?? 'subagent cancelled')
|
||||
},
|
||||
async dispose(): Promise<void> {
|
||||
request.signal?.removeEventListener('abort', onAbort)
|
||||
@@ -151,14 +165,22 @@ export function startInProcessRun(
|
||||
}
|
||||
|
||||
/**
|
||||
* Read a settled child's terminal result from its session log: the last
|
||||
* `assistant/message` content (deep-cloned — the log is frozen) and the last
|
||||
* `turn/end` reason mapped to a {@link SubagentStopReason}.
|
||||
* Read a settled child's terminal result from its session log, scoped to the
|
||||
* child's OWN events (everything at or after `seedLength` — fork seeds the
|
||||
* parent's completed-turn prefix, so a child that produced no message of its
|
||||
* own must NOT return the seeded parent's last assistant message). The output
|
||||
* is the child's last `assistant/message` content (deep-cloned — the log is
|
||||
* frozen); the stop reason is the child's last `turn/end` reason mapped to a
|
||||
* {@link SubagentStopReason}. When `cancelled` is set but no `turn/end` was
|
||||
* logged (a cancel landed in the pre-turn window, before any turn ran), the
|
||||
* run settles `aborted` per the {@link SubagentRun.cancel} contract rather than
|
||||
* the generic no-turn `error`.
|
||||
*/
|
||||
function readResult(child: Agent): SubagentResult {
|
||||
const events = child.session.events
|
||||
const lastMessage = events.findLast((e): e is SessionEvent<'assistant/message'> => e.type === 'assistant/message')
|
||||
const lastEnd = events.findLast((e): e is SessionEvent<'turn/end'> => e.type === 'turn/end')
|
||||
function readResult(child: Agent, seedLength: number, cancelled: boolean): SubagentResult {
|
||||
const own = child.session.events.slice(seedLength)
|
||||
const lastMessage = own.findLast((e): e is SessionEvent<'assistant/message'> => e.type === 'assistant/message')
|
||||
const lastEnd = own.findLast((e): e is SessionEvent<'turn/end'> => e.type === 'turn/end')
|
||||
const output: ContentBlock[] = lastMessage ? structuredClone(lastMessage.data.content) : []
|
||||
if (lastEnd === undefined && cancelled) return { output, stopReason: 'aborted' }
|
||||
return { output, stopReason: toStopReason(lastEnd?.data.reason) }
|
||||
}
|
||||
|
||||
@@ -128,6 +128,22 @@ describe('dsh-subagent-spawn', () => {
|
||||
await run.dispose()
|
||||
})
|
||||
|
||||
it('cancelling BEFORE the child turn starts settles aborted, not error', async () => {
|
||||
// Regression: a cancel landing in the pre-turn window clears the queued
|
||||
// prompt before any `turn/end` is logged. Deriving the stop reason from
|
||||
// `turn/end` alone then mis-maps the no-turn case to `error`; the run must
|
||||
// honor the cancel contract and settle `aborted`. The cancel is synchronous
|
||||
// (same tick as start, before the loop's queued-wait continuation runs), so
|
||||
// the turn is dropped and the empty script is never consumed.
|
||||
const { ctx, parent } = await setup([])
|
||||
const run = ctx.subagents.start('spawn', { prompt: [{ type: 'text', text: 'p' }], parent })
|
||||
run.cancel('early')
|
||||
const result = await run.result
|
||||
expect(result.stopReason).toBe('aborted')
|
||||
expect(result.output).toEqual([])
|
||||
await run.dispose()
|
||||
})
|
||||
|
||||
it('cancelling a running child settles the run as aborted (the abort bridge + cancel())', async () => {
|
||||
// 'hang' makes the child's model stream one chunk then wait until aborted.
|
||||
const controller = new AbortController()
|
||||
|
||||
@@ -70,7 +70,7 @@ declare module 'cordis' {
|
||||
export interface SubagentRunInfo {
|
||||
/** The provider that started the run. */
|
||||
provider: string
|
||||
/** The child agent/session id. */
|
||||
/** The child agent's id. */
|
||||
id: AgentId
|
||||
}
|
||||
|
||||
@@ -78,7 +78,7 @@ export interface SubagentRunInfo {
|
||||
export interface SubagentRunEndInfo {
|
||||
/** The provider that ran it. */
|
||||
provider: string
|
||||
/** The child agent/session id. */
|
||||
/** The child agent's id. */
|
||||
id: AgentId
|
||||
/** The terminal stop reason. */
|
||||
stopReason: SubagentResult['stopReason']
|
||||
|
||||
@@ -122,7 +122,7 @@ export interface SubagentResult {
|
||||
* presence of the method IS the capability — narrow before calling.
|
||||
*/
|
||||
export interface SubagentRun {
|
||||
/** The child agent's id (also its session id token, for correlation). */
|
||||
/** The child agent's id (use `ctx.agents.get(id)` to reach the live child). */
|
||||
readonly id: AgentId
|
||||
/**
|
||||
* Resolves with the child's terminal {@link SubagentResult} when the run
|
||||
|
||||
Reference in New Issue
Block a user