Fix review findings: lifecycle containment, configurable tool name, coverage, type catalog
Address four findings from the first Codex review round: - Contain subagent/start|end listener throws (emitContainedStart/End): a thrown lifecycle listener could escape SubagentService.start() before the caller received the live run to dispose it (a leaked child), and a thrown subagent/end listener could surface as an unhandled rejection on the detached result-settle hook. Both emits now log-and-contain, mirroring the agent registry's agent/created|disposed containment. - Make the model-facing tool name configurable (Config.toolName, default subagent). The docs say to load dsh-tool-subagent once per provider to expose multiple transports, but the hardcoded name made the second load throw a duplicate-tool-name error; a distinct toolName per load is now required and documented. - Reach the per-file 100% coverage gate: tests for the subagent/end error branch, lifecycle-listener containment, every stopReasonError arm + the merge-extensible default, the multi-provider toolName path, agentOptions forwarding, and the direct-apply schema-bypass fallbacks. - Document the seam vocabulary in docs/core-data-structures/subagent.md with verbatim type-equiv blocks + manifest entries, and link it from core.md (a brand-new core/seam type the doc-sync gate cannot detect on its own).
This commit is contained in:
@@ -4,11 +4,12 @@ The model-facing `subagent` tool: delegate a self-contained task to a child agen
|
||||
|
||||
## Provider selection is config, not model-facing
|
||||
|
||||
This plugin binds to **exactly one** provider (`Config.provider`). The model sees only `{ description, prompt }` — there is no provider/type parameter in the schema. To expose more than one transport, load the plugin more than once, each bound to a different provider. Keeping selection in config (not the schema) is the deliberate split: the *service* holds a multi-provider registry; the *tool* picks one.
|
||||
This plugin binds to **exactly one** provider (`Config.provider`). The model sees only `{ description, prompt }` — there is no provider/type parameter in the schema. To expose more than one transport, load the plugin more than once, each bound to a different provider **and a distinct `toolName`** (the tool registry rejects a duplicate name, so a second load that kept the default `subagent` name would throw). Keeping selection in config (not the schema) is the deliberate split: the *service* holds a multi-provider registry; the *tool* picks one.
|
||||
|
||||
| Config key | Meaning |
|
||||
|---|---|
|
||||
| `provider` (required) | The `ctx.subagents` provider name to start runs on (`spawn`, `fork`, `acp`, …). |
|
||||
| `toolName` | The model-facing tool name to register (default `subagent`). Set a distinct value per load when exposing multiple providers, e.g. `subagent` + `subagent_acp`. |
|
||||
| `agentOptions` | Default per-child `{ model?, systemPrompt? }` applied to every spawned child. |
|
||||
|
||||
## Lifecycle (synchronous collect)
|
||||
|
||||
@@ -35,6 +35,14 @@ export const inject = ['tools', 'subagents']
|
||||
export interface Config {
|
||||
/** The `ctx.subagents` provider name to start runs on (e.g. `spawn`, `acp`). */
|
||||
provider: string
|
||||
/**
|
||||
* The model-facing tool name to register (default `subagent`). To expose more
|
||||
* than one transport, load this plugin once per provider — each load MUST set
|
||||
* a distinct `toolName` (the tool registry rejects a duplicate name), e.g.
|
||||
* `{ provider: 'spawn', toolName: 'subagent' }` and
|
||||
* `{ provider: 'acp', toolName: 'subagent_acp' }`.
|
||||
*/
|
||||
toolName?: string
|
||||
/**
|
||||
* Default per-child agent options (model, system prompt) applied to every
|
||||
* spawned child. Omitted fields fall back to the child loop's own defaults.
|
||||
@@ -44,6 +52,7 @@ export interface Config {
|
||||
|
||||
export const Config: z<Config> = z.object({
|
||||
provider: z.string().required(),
|
||||
toolName: z.string().default('subagent'),
|
||||
agentOptions: z.object({
|
||||
model: z.string(),
|
||||
systemPrompt: z.string(),
|
||||
@@ -85,7 +94,7 @@ function stopReasonError(result: SubagentResult): string | undefined {
|
||||
|
||||
export function apply(ctx: Context, config: Config): void {
|
||||
ctx.tools.register(defineTool({
|
||||
name: 'subagent',
|
||||
name: config.toolName ?? 'subagent',
|
||||
description:
|
||||
'Delegate a self-contained task to a subagent (a separate agent that works in its own context) '
|
||||
+ 'and return its final result. Use this to offload focused, independent work — research, a scoped '
|
||||
|
||||
@@ -68,11 +68,121 @@ describe('dsh-tool-subagent', () => {
|
||||
expect(Object.keys(props).sort()).toEqual(['description', 'prompt'])
|
||||
})
|
||||
|
||||
it('maps a non-completed stop reason to an isError result (not partial success)', async () => {
|
||||
const ctx = await setup({ provider: 'mock' }, { stopReason: 'refusal' })
|
||||
it.each([
|
||||
{ stopReason: 'aborted' as const, fragment: 'cancelled' },
|
||||
{ stopReason: 'error' as const, fragment: 'failed' },
|
||||
{ stopReason: 'max-tokens' as const, fragment: 'token limit' },
|
||||
{ stopReason: 'refusal' as const, fragment: 'declined' },
|
||||
])('maps stop reason $stopReason to an isError result (not partial success)', async ({ stopReason, fragment }) => {
|
||||
const ctx = await setup({ provider: 'mock' }, { stopReason })
|
||||
const result = await callSubagent(ctx, { description: 'd', prompt: 'p' })
|
||||
expect(result.isError).toBe(true)
|
||||
expect(text(result)).toContain('declined')
|
||||
expect(text(result)).toContain(fragment)
|
||||
})
|
||||
|
||||
it('registers under a configurable toolName so multiple providers can coexist', async () => {
|
||||
// The defining multi-provider use case: two loads, two distinct tool names,
|
||||
// each bound to a different provider — the tool registry rejects duplicate
|
||||
// names, so a configurable name is what makes this work.
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SystemPrompt)
|
||||
await ctx.plugin(ToolRegistry)
|
||||
await ctx.plugin(SubagentService)
|
||||
await ctx.plugin(mock, { name: 'spawn', reply: 'from spawn' })
|
||||
await ctx.plugin(mock, { name: 'acp', reply: 'from acp' })
|
||||
await ctx.plugin(tool, { provider: 'spawn', toolName: 'subagent' })
|
||||
await ctx.plugin(tool, { provider: 'acp', toolName: 'subagent_acp' })
|
||||
|
||||
const names = ctx.tools.schemas().map(s => s.name).filter(n => n.startsWith('subagent')).sort()
|
||||
expect(names).toEqual(['subagent', 'subagent_acp'])
|
||||
|
||||
const viaSpawn = await ctx.tools.execute({ callId: CallId('c-spawn'), name: 'subagent', arguments: { description: 'd', prompt: 'p' }, agent: fakeAgent() })
|
||||
const viaAcp = await ctx.tools.execute({ callId: CallId('c-acp'), name: 'subagent_acp', arguments: { description: 'd', prompt: 'p' }, agent: fakeAgent() })
|
||||
expect(text(viaSpawn)).toBe('from spawn')
|
||||
expect(text(viaAcp)).toBe('from acp')
|
||||
})
|
||||
|
||||
it('treats an unknown (plugin-added) stop reason as an isError result', async () => {
|
||||
// SubagentStopReason is merge-extensible; the tool's stopReasonError default
|
||||
// arm must treat an unrecognized terminal reason as a failure, not success.
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SystemPrompt)
|
||||
await ctx.plugin(ToolRegistry)
|
||||
await ctx.plugin(SubagentService)
|
||||
ctx.subagents.registerProvider({
|
||||
name: 'weird',
|
||||
capabilities: { outputSchema: false, depthLimit: false, toolFilter: false },
|
||||
start: () => ({
|
||||
id: AgentId('weird-child'),
|
||||
result: Promise.resolve({ output: [{ type: 'text', text: 'partial' }], stopReason: 'frobnicated' as never }),
|
||||
cancel() {},
|
||||
dispose: async () => {},
|
||||
}),
|
||||
})
|
||||
await ctx.plugin(tool, { provider: 'weird' })
|
||||
|
||||
const result = await callSubagent(ctx, { description: 'd', prompt: 'p' })
|
||||
expect(result.isError).toBe(true)
|
||||
expect(text(result)).toContain('abnormally')
|
||||
})
|
||||
|
||||
it('forwards configured agentOptions into the start request', async () => {
|
||||
// Cover the `config.agentOptions ? … : {}` spread: a provider that captures
|
||||
// the request lets us assert the agentOptions reached it.
|
||||
let seen: { agentOptions?: { model?: string } } | undefined
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SystemPrompt)
|
||||
await ctx.plugin(ToolRegistry)
|
||||
await ctx.plugin(SubagentService)
|
||||
ctx.subagents.registerProvider({
|
||||
name: 'capture',
|
||||
capabilities: { outputSchema: false, depthLimit: false, toolFilter: false },
|
||||
start: (request) => {
|
||||
seen = request
|
||||
return {
|
||||
id: AgentId('capture-child'),
|
||||
result: Promise.resolve({ output: [{ type: 'text', text: 'ok' }], stopReason: 'completed' as const }),
|
||||
cancel() {},
|
||||
dispose: async () => {},
|
||||
}
|
||||
},
|
||||
})
|
||||
await ctx.plugin(tool, { provider: 'capture', agentOptions: { model: 'child-model', systemPrompt: 'be terse' } })
|
||||
|
||||
await callSubagent(ctx, { description: 'd', prompt: 'p' })
|
||||
expect(seen?.agentOptions).toEqual({ model: 'child-model', systemPrompt: 'be terse' })
|
||||
})
|
||||
|
||||
it('defaults toolName and omits agentOptions when apply() is called directly (schema bypass)', async () => {
|
||||
// `ctx.plugin` validates+defaults config first (toolName→'subagent', the
|
||||
// agentOptions object→{}), so the runtime `?? 'subagent'` fallback and the
|
||||
// no-agentOptions branch are only reachable via a direct apply() that
|
||||
// bypasses schemastery — the same pattern acp-agent uses for its defaults.
|
||||
let seen: { agentOptions?: unknown } | undefined
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SystemPrompt)
|
||||
await ctx.plugin(ToolRegistry)
|
||||
await ctx.plugin(SubagentService)
|
||||
ctx.subagents.registerProvider({
|
||||
name: 'bare',
|
||||
capabilities: { outputSchema: false, depthLimit: false, toolFilter: false },
|
||||
start: (request) => {
|
||||
seen = request
|
||||
return {
|
||||
id: AgentId('bare-child'),
|
||||
result: Promise.resolve({ output: [{ type: 'text', text: 'ok' }], stopReason: 'completed' as const }),
|
||||
cancel() {},
|
||||
dispose: async () => {},
|
||||
}
|
||||
},
|
||||
})
|
||||
// Direct apply with only `provider` — no toolName, no agentOptions.
|
||||
tool.apply(ctx, { provider: 'bare' })
|
||||
await new Promise(r => setTimeout(r, 10))
|
||||
|
||||
expect(ctx.tools.schemas().some(s => s.name === 'subagent')).toBe(true)
|
||||
await callSubagent(ctx, { description: 'd', prompt: 'p' })
|
||||
expect(seen?.agentOptions).toBeUndefined()
|
||||
})
|
||||
|
||||
it('fails loud when invoked without a calling agent', async () => {
|
||||
|
||||
Reference in New Issue
Block a user