From 4bd98407a98d2c570508e3b1de588a8cb661383d Mon Sep 17 00:00:00 2001 From: Dudu-0223 Date: Sun, 26 Jul 2026 23:00:47 +0800 Subject: [PATCH] fix: address codex review round 1 - listChildren() takes an optional AbortSignal and rechecks it after every un-signalled session-query await (the cold-resume cooperative-cancellation boundary); list_agents forwards exec.signal so the registry's drain of started tool bodies cannot block on a slow or large catalog. - The list_agents description now presents running/complete as a stored-record snapshot and defers deliverability to send_message, matching the ownership- conflict semantics the service tests pin. --- docs/cordis-catalog/services.md | 8 ++- docs/tool-catalog.md | 2 +- .../system-prompt.expected.md | 2 +- .../tool-schemas.expected.json | 2 +- .../both-mode-turn/tool-schemas.expected.json | 2 +- .../code-mode-turn/system-prompt.expected.md | 2 +- .../lsp-definition/tool-schemas.expected.json | 2 +- .../pty-tools/tool-schemas.expected.json | 2 +- .../tool-schemas.expected.json | 2 +- .../text-turn/tool-schemas.expected.json | 2 +- .../web-fetch/tool-schemas.expected.json | 2 +- .../cordis/tool-cordis/src/api-catalog.ts | 4 +- packages/subagent/subagent/src/index.ts | 21 ++++++-- .../subagent/tests/list-children.spec.ts | 54 +++++++++++++++++++ .../tool-subagent-control/src/list-agents.ts | 14 +++-- 15 files changed, 99 insertions(+), 22 deletions(-) diff --git a/docs/cordis-catalog/services.md b/docs/cordis-catalog/services.md index e7b6fedc6e..0a415f40be 100644 --- a/docs/cordis-catalog/services.md +++ b/docs/cordis-catalog/services.md @@ -1997,11 +1997,15 @@ async drainContinuableDescendants(parents: readonly Agent[]): Promise * live-preferred corpus without loading or resuming an Agent. The lineage * trace supplies stable candidate order and live status; each candidate is * then inspected independently for exactly one supported descriptor in its - * own suffix. + * own suffix. Session-query reads take no signal, so cancellation is + * cooperative: the scan rechecks `signal` after every un-signalled await and + * stops between candidates instead of draining a slow or large catalog after + * the caller has gone. * @param parentSessionId - parent whose direct children are listed. + * @param signal - caller-owned cancellation observed between query awaits. * @returns child and diagnostic entries in lineage-trace order. */ -async listChildren(parentSessionId: SessionId): Promise +async listChildren(parentSessionId: SessionId, signal?: AbortSignal): Promise /** * Register a provider under its name. Registration is effect-scoped and HMR diff --git a/docs/tool-catalog.md b/docs/tool-catalog.md index e680c5eb8a..bec172be24 100644 --- a/docs/tool-catalog.md +++ b/docs/tool-catalog.md @@ -1151,7 +1151,7 @@ The registered tool name is the load-time `toolName` config (default `subagent`) ### `list_agents` -List your background subagents: every subagent you started that can receive `send_message`, whether it is still working (running) or has finished its current turn (complete — a follow-up message starts a new turn on the same conversation). Children that could not be read are reported as diagnostics instead of being silently dropped. +List your background subagents by durable id and label. Status is a snapshot of the stored record: running means the subagent session is currently live in this process, complete means it exists only in storage and a `send_message` starts a new turn on the same conversation. The snapshot is not a delivery promise — `send_message` performs the authoritative check and may still fail. Children that could not be read are reported as diagnostics instead of being silently dropped. ```json { diff --git a/examples/acp-agent/tests/snapshots/advanced-toolchain/system-prompt.expected.md b/examples/acp-agent/tests/snapshots/advanced-toolchain/system-prompt.expected.md index acd0320e39..bbb1f53cc0 100644 --- a/examples/acp-agent/tests/snapshots/advanced-toolchain/system-prompt.expected.md +++ b/examples/acp-agent/tests/snapshots/advanced-toolchain/system-prompt.expected.md @@ -94,7 +94,7 @@ interface ToolArgsMap { } & Record; /** Read the current same-session goal, including its exact id/revision, objective, phase, completed continuation rounds, round limit, blocker reason when present, and whether another continuation is armed. Call this before updating a goal. */ get_goal: Record; - /** List your background subagents: every subagent you started that can receive `send_message`, whether it is still working (running) or has finished its current turn (complete — a follow-up message starts a new turn on the same conversation). Children that could not be read are reported as diagnostics instead of being silently dropped. */ + /** List your background subagents by durable id and label. Status is a snapshot of the stored record: running means the subagent session is currently live in this process, complete means it exists only in storage and a `send_message` starts a new turn on the same conversation. The snapshot is not a delivery promise — `send_message` performs the authoritative check and may still fail. Children that could not be read are reported as diagnostics instead of being silently dropped. */ list_agents: Record; /** Run a foreground fresh-agent Ralph loop toward one immutable objective. Use only when the direct human explicitly asks for Ralph or fresh-agent iteration. Each round opens a new child with no parent conversation or prior child session; the shared workspace is long-term memory, and only a bounded structured report crosses rounds. The call returns when a worker reports completion or a concrete blocker, or at the round limit. Ordinary long-running same-session work belongs to goal tools. */ ralph: { diff --git a/examples/acp-agent/tests/snapshots/advanced-toolchain/tool-schemas.expected.json b/examples/acp-agent/tests/snapshots/advanced-toolchain/tool-schemas.expected.json index ab7baeb774..82ca0d436a 100644 --- a/examples/acp-agent/tests/snapshots/advanced-toolchain/tool-schemas.expected.json +++ b/examples/acp-agent/tests/snapshots/advanced-toolchain/tool-schemas.expected.json @@ -174,7 +174,7 @@ }, { "name": "list_agents", - "description": "List your background subagents: every subagent you started that can receive `send_message`, whether it is still working (running) or has finished its current turn (complete — a follow-up message starts a new turn on the same conversation). Children that could not be read are reported as diagnostics instead of being silently dropped.", + "description": "List your background subagents by durable id and label. Status is a snapshot of the stored record: running means the subagent session is currently live in this process, complete means it exists only in storage and a `send_message` starts a new turn on the same conversation. The snapshot is not a delivery promise — `send_message` performs the authoritative check and may still fail. Children that could not be read are reported as diagnostics instead of being silently dropped.", "parameters": { "type": "object", "properties": {} diff --git a/examples/acp-agent/tests/snapshots/both-mode-turn/tool-schemas.expected.json b/examples/acp-agent/tests/snapshots/both-mode-turn/tool-schemas.expected.json index a26ac45b89..3f00f54f18 100644 --- a/examples/acp-agent/tests/snapshots/both-mode-turn/tool-schemas.expected.json +++ b/examples/acp-agent/tests/snapshots/both-mode-turn/tool-schemas.expected.json @@ -117,7 +117,7 @@ }, { "name": "list_agents", - "description": "List your background subagents: every subagent you started that can receive `send_message`, whether it is still working (running) or has finished its current turn (complete — a follow-up message starts a new turn on the same conversation). Children that could not be read are reported as diagnostics instead of being silently dropped.", + "description": "List your background subagents by durable id and label. Status is a snapshot of the stored record: running means the subagent session is currently live in this process, complete means it exists only in storage and a `send_message` starts a new turn on the same conversation. The snapshot is not a delivery promise — `send_message` performs the authoritative check and may still fail. Children that could not be read are reported as diagnostics instead of being silently dropped.", "parameters": { "type": "object", "properties": {} diff --git a/examples/acp-agent/tests/snapshots/code-mode-turn/system-prompt.expected.md b/examples/acp-agent/tests/snapshots/code-mode-turn/system-prompt.expected.md index 2435bfc0b3..c0db704008 100644 --- a/examples/acp-agent/tests/snapshots/code-mode-turn/system-prompt.expected.md +++ b/examples/acp-agent/tests/snapshots/code-mode-turn/system-prompt.expected.md @@ -77,7 +77,7 @@ interface ToolArgsMap { } & Record; /** Read the current same-session goal, including its exact id/revision, objective, phase, completed continuation rounds, round limit, blocker reason when present, and whether another continuation is armed. Call this before updating a goal. */ get_goal: Record; - /** List your background subagents: every subagent you started that can receive `send_message`, whether it is still working (running) or has finished its current turn (complete — a follow-up message starts a new turn on the same conversation). Children that could not be read are reported as diagnostics instead of being silently dropped. */ + /** List your background subagents by durable id and label. Status is a snapshot of the stored record: running means the subagent session is currently live in this process, complete means it exists only in storage and a `send_message` starts a new turn on the same conversation. The snapshot is not a delivery promise — `send_message` performs the authoritative check and may still fail. Children that could not be read are reported as diagnostics instead of being silently dropped. */ list_agents: Record; /** Run a foreground fresh-agent Ralph loop toward one immutable objective. Use only when the direct human explicitly asks for Ralph or fresh-agent iteration. Each round opens a new child with no parent conversation or prior child session; the shared workspace is long-term memory, and only a bounded structured report crosses rounds. The call returns when a worker reports completion or a concrete blocker, or at the round limit. Ordinary long-running same-session work belongs to goal tools. */ ralph: { diff --git a/examples/acp-agent/tests/snapshots/lsp-definition/tool-schemas.expected.json b/examples/acp-agent/tests/snapshots/lsp-definition/tool-schemas.expected.json index f19643e8a1..0eb5f8c6b7 100644 --- a/examples/acp-agent/tests/snapshots/lsp-definition/tool-schemas.expected.json +++ b/examples/acp-agent/tests/snapshots/lsp-definition/tool-schemas.expected.json @@ -117,7 +117,7 @@ }, { "name": "list_agents", - "description": "List your background subagents: every subagent you started that can receive `send_message`, whether it is still working (running) or has finished its current turn (complete — a follow-up message starts a new turn on the same conversation). Children that could not be read are reported as diagnostics instead of being silently dropped.", + "description": "List your background subagents by durable id and label. Status is a snapshot of the stored record: running means the subagent session is currently live in this process, complete means it exists only in storage and a `send_message` starts a new turn on the same conversation. The snapshot is not a delivery promise — `send_message` performs the authoritative check and may still fail. Children that could not be read are reported as diagnostics instead of being silently dropped.", "parameters": { "type": "object", "properties": {} diff --git a/examples/acp-agent/tests/snapshots/pty-tools/tool-schemas.expected.json b/examples/acp-agent/tests/snapshots/pty-tools/tool-schemas.expected.json index 638c8b9bfc..68c7366b4d 100644 --- a/examples/acp-agent/tests/snapshots/pty-tools/tool-schemas.expected.json +++ b/examples/acp-agent/tests/snapshots/pty-tools/tool-schemas.expected.json @@ -117,7 +117,7 @@ }, { "name": "list_agents", - "description": "List your background subagents: every subagent you started that can receive `send_message`, whether it is still working (running) or has finished its current turn (complete — a follow-up message starts a new turn on the same conversation). Children that could not be read are reported as diagnostics instead of being silently dropped.", + "description": "List your background subagents by durable id and label. Status is a snapshot of the stored record: running means the subagent session is currently live in this process, complete means it exists only in storage and a `send_message` starts a new turn on the same conversation. The snapshot is not a delivery promise — `send_message` performs the authoritative check and may still fail. Children that could not be read are reported as diagnostics instead of being silently dropped.", "parameters": { "type": "object", "properties": {} diff --git a/examples/acp-agent/tests/snapshots/session-query-spill/tool-schemas.expected.json b/examples/acp-agent/tests/snapshots/session-query-spill/tool-schemas.expected.json index 82d263326e..7dc29ffd3b 100644 --- a/examples/acp-agent/tests/snapshots/session-query-spill/tool-schemas.expected.json +++ b/examples/acp-agent/tests/snapshots/session-query-spill/tool-schemas.expected.json @@ -117,7 +117,7 @@ }, { "name": "list_agents", - "description": "List your background subagents: every subagent you started that can receive `send_message`, whether it is still working (running) or has finished its current turn (complete — a follow-up message starts a new turn on the same conversation). Children that could not be read are reported as diagnostics instead of being silently dropped.", + "description": "List your background subagents by durable id and label. Status is a snapshot of the stored record: running means the subagent session is currently live in this process, complete means it exists only in storage and a `send_message` starts a new turn on the same conversation. The snapshot is not a delivery promise — `send_message` performs the authoritative check and may still fail. Children that could not be read are reported as diagnostics instead of being silently dropped.", "parameters": { "type": "object", "properties": {} diff --git a/examples/acp-agent/tests/snapshots/text-turn/tool-schemas.expected.json b/examples/acp-agent/tests/snapshots/text-turn/tool-schemas.expected.json index 27b4d6e801..b61f56dc8d 100644 --- a/examples/acp-agent/tests/snapshots/text-turn/tool-schemas.expected.json +++ b/examples/acp-agent/tests/snapshots/text-turn/tool-schemas.expected.json @@ -117,7 +117,7 @@ }, { "name": "list_agents", - "description": "List your background subagents: every subagent you started that can receive `send_message`, whether it is still working (running) or has finished its current turn (complete — a follow-up message starts a new turn on the same conversation). Children that could not be read are reported as diagnostics instead of being silently dropped.", + "description": "List your background subagents by durable id and label. Status is a snapshot of the stored record: running means the subagent session is currently live in this process, complete means it exists only in storage and a `send_message` starts a new turn on the same conversation. The snapshot is not a delivery promise — `send_message` performs the authoritative check and may still fail. Children that could not be read are reported as diagnostics instead of being silently dropped.", "parameters": { "type": "object", "properties": {} diff --git a/examples/acp-agent/tests/snapshots/web-fetch/tool-schemas.expected.json b/examples/acp-agent/tests/snapshots/web-fetch/tool-schemas.expected.json index e06f643462..dc75c376af 100644 --- a/examples/acp-agent/tests/snapshots/web-fetch/tool-schemas.expected.json +++ b/examples/acp-agent/tests/snapshots/web-fetch/tool-schemas.expected.json @@ -117,7 +117,7 @@ }, { "name": "list_agents", - "description": "List your background subagents: every subagent you started that can receive `send_message`, whether it is still working (running) or has finished its current turn (complete — a follow-up message starts a new turn on the same conversation). Children that could not be read are reported as diagnostics instead of being silently dropped.", + "description": "List your background subagents by durable id and label. Status is a snapshot of the stored record: running means the subagent session is currently live in this process, complete means it exists only in storage and a `send_message` starts a new turn on the same conversation. The snapshot is not a delivery promise — `send_message` performs the authoritative check and may still fail. Children that could not be read are reported as diagnostics instead of being silently dropped.", "parameters": { "type": "object", "properties": {} diff --git a/packages/cordis/tool-cordis/src/api-catalog.ts b/packages/cordis/tool-cordis/src/api-catalog.ts index d77d9673cc..623a60c557 100644 --- a/packages/cordis/tool-cordis/src/api-catalog.ts +++ b/packages/cordis/tool-cordis/src/api-catalog.ts @@ -897,8 +897,8 @@ export const SERVICE_API: readonly ServiceApiEntry[] = [ jsDoc: '/**\n * Close continuable admission below exact live parent Agents, stop only their\n * visible descendant Activations synchronously, then await admitted scoped\n * materializations and release those forests child-first. The scoped cutoff\n * lasts until each exact parent leaves the registry; unrelated parent trees\n * remain live.\n * @param parents - exact host-owned parent Agents entering teardown.\n * @returns once every retained descendant Activation released its `AgentHandle`.\n * @throws an aggregate error after all scoped branches settle when any failed.\n */', }, { - signature: 'async listChildren(parentSessionId: SessionId): Promise', - jsDoc: '/**\n * Enumerate one session\'s direct continuable children from the durable,\n * live-preferred corpus without loading or resuming an Agent. The lineage\n * trace supplies stable candidate order and live status; each candidate is\n * then inspected independently for exactly one supported descriptor in its\n * own suffix.\n * @param parentSessionId - parent whose direct children are listed.\n * @returns child and diagnostic entries in lineage-trace order.\n */', + signature: 'async listChildren(parentSessionId: SessionId, signal?: AbortSignal): Promise', + jsDoc: '/**\n * Enumerate one session\'s direct continuable children from the durable,\n * live-preferred corpus without loading or resuming an Agent. The lineage\n * trace supplies stable candidate order and live status; each candidate is\n * then inspected independently for exactly one supported descriptor in its\n * own suffix. Session-query reads take no signal, so cancellation is\n * cooperative: the scan rechecks `signal` after every un-signalled await and\n * stops between candidates instead of draining a slow or large catalog after\n * the caller has gone.\n * @param parentSessionId - parent whose direct children are listed.\n * @param signal - caller-owned cancellation observed between query awaits.\n * @returns child and diagnostic entries in lineage-trace order.\n */', }, { signature: 'registerProvider(provider: SubagentProvider): () => void', diff --git a/packages/subagent/subagent/src/index.ts b/packages/subagent/subagent/src/index.ts index 3bc73d0e5d..6fdadba115 100644 --- a/packages/subagent/subagent/src/index.ts +++ b/packages/subagent/subagent/src/index.ts @@ -251,11 +251,15 @@ export class SubagentService extends Service { * live-preferred corpus without loading or resuming an Agent. The lineage * trace supplies stable candidate order and live status; each candidate is * then inspected independently for exactly one supported descriptor in its - * own suffix. + * own suffix. Session-query reads take no signal, so cancellation is + * cooperative: the scan rechecks `signal` after every un-signalled await and + * stops between candidates instead of draining a slow or large catalog after + * the caller has gone. * @param parentSessionId - parent whose direct children are listed. + * @param signal - caller-owned cancellation observed between query awaits. * @returns child and diagnostic entries in lineage-trace order. */ - async listChildren(parentSessionId: SessionId): Promise { + async listChildren(parentSessionId: SessionId, signal?: AbortSignal): Promise { const query = this.ctx.get('sessionQuery') if (query === undefined) { throw new SubagentError( @@ -266,7 +270,8 @@ export class SubagentService extends Service { const trace = await query.traceSession(parentSessionId) const entries: SubagentListEntry[] = [] for (const node of trace.descendants) { - const entry = await this.inspectChild(query, parentSessionId, node.session) + assertListingNotCancelled(signal) + const entry = await this.inspectChild(query, parentSessionId, node.session, signal) if (entry !== undefined) entries.push(entry) } return entries @@ -277,10 +282,12 @@ export class SubagentService extends Service { query: SessionQueryService, parentSessionId: SessionId, candidate: SessionRecord, + signal?: AbortSignal, ): Promise { const childId = candidate.header.id try { const records = await query.listEvents(childId) + assertListingNotCancelled(signal) // Fork seeds replay ancestor events, so only this child's suffix owns its descriptor. const seedLength = candidate.header.seedLength ?? 0 const descriptorSeqs = records @@ -294,6 +301,7 @@ export class SubagentService extends Service { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const seq = descriptorSeqs[0]! const window = await query.readEvent({ sessionId: childId, seq }) + assertListingNotCancelled(signal) assertSessionHeadersCompatible(window.session, candidate.header) if (window.session.parentSession !== parentSessionId || window.target.type !== 'subagent/descriptor') { return { kind: 'diagnostic', id: childId, reason: 'corrupt' } @@ -452,6 +460,13 @@ export class SubagentService extends Service { export default SubagentService +/** Stop a cooperative listing scan at its next cancellation checkpoint. */ +function assertListingNotCancelled(signal: AbortSignal | undefined): void { + if (signal?.aborted) { + throw new SubagentError('subagent listing was cancelled', 'CANCELLED') + } +} + /** Map isolated session-query failures to the fixed child diagnostic taxonomy. */ function perChildDiagnosticReason(error: unknown): 'corrupt' | 'unavailable' | undefined { if (!(error instanceof SessionQueryError)) return undefined diff --git a/packages/subagent/subagent/tests/list-children.spec.ts b/packages/subagent/subagent/tests/list-children.spec.ts index 182981f26d..f7d9d5cb8f 100644 --- a/packages/subagent/subagent/tests/list-children.spec.ts +++ b/packages/subagent/subagent/tests/list-children.spec.ts @@ -387,6 +387,60 @@ describe('SubagentService.listChildren', () => { ]) }) + it('stops the scan at the between-candidates checkpoint when the signal aborts', async () => { + const { ctx, parent } = await setup([textResponse('one'), textResponse('two')]) + await startChild(ctx, parent, 'first child') + await startChild(ctx, parent, 'second child') + const controller = new AbortController() + const query = ctx.get('sessionQuery')! + const originalListEvents = query.listEvents.bind(query) + let inspected = 0 + query.listEvents = (sessionId) => { + inspected += 1 + // Cancel while the first candidate's read is in flight: the loop's next + // between-candidates checkpoint must stop before the second read. + controller.abort() + return originalListEvents(sessionId) + } + await expect(ctx.subagents.listChildren(parent.id, controller.signal)).rejects.toThrow( + expect.objectContaining({ code: 'CANCELLED' }) as Error, + ) + expect(inspected).toBe(1) + }) + + it('stops after a per-child read when the signal aborts mid-inspection', async () => { + const { ctx, parent } = await setup([textResponse('done')]) + await startChild(ctx, parent, 'cancelled mid-read') + const controller = new AbortController() + const query = ctx.get('sessionQuery')! + const originalReadEvent = query.readEvent.bind(query) + let exactReads = 0 + query.readEvent = async (request) => { + exactReads += 1 + const window = await originalReadEvent(request) + controller.abort() + return window + } + // The post-read checkpoint throws a subagent error, which is not a + // session-query failure and therefore propagates instead of becoming a + // per-child diagnostic. + await expect(ctx.subagents.listChildren(parent.id, controller.signal)) + .rejects.toThrow(expect.objectContaining({ code: 'CANCELLED' }) as Error) + expect(exactReads).toBe(1) + }) + + it('a pre-aborted signal stops before any candidate read', async () => { + const { ctx, parent } = await setup([textResponse('done')]) + await startChild(ctx, parent, 'never read') + const controller = new AbortController() + controller.abort() + const query = ctx.get('sessionQuery')! + query.listEvents = () => Promise.reject(new Error('must not be called')) + await expect(ctx.subagents.listChildren(parent.id, controller.signal)).rejects.toThrow( + expect.objectContaining({ code: 'CANCELLED' }) as Error, + ) + }) + it('returns an empty array for a parent with no children', async () => { const { ctx, parent } = await setup([]) await ctx.sessions.flush(parent.session) diff --git a/packages/subagent/tool-subagent-control/src/list-agents.ts b/packages/subagent/tool-subagent-control/src/list-agents.ts index 04b2fb3bdd..a467c398c9 100644 --- a/packages/subagent/tool-subagent-control/src/list-agents.ts +++ b/packages/subagent/tool-subagent-control/src/list-agents.ts @@ -23,10 +23,12 @@ export function apply(ctx: Context): void { ctx.tools.register(defineTool({ name: 'list_agents', description: - 'List your background subagents: every subagent you started that can receive `send_message`, ' - + 'whether it is still working (running) or has finished its current turn (complete — a follow-up ' - + 'message starts a new turn on the same conversation). Children that could not be read are ' - + 'reported as diagnostics instead of being silently dropped.', + 'List your background subagents by durable id and label. Status is a snapshot of the stored ' + + 'record: running means the subagent session is currently live in this process, complete means ' + + 'it exists only in storage and a `send_message` starts a new turn on the same conversation. ' + + 'The snapshot is not a delivery promise — `send_message` performs the authoritative check and ' + + 'may still fail. Children that could not be read are reported as diagnostics instead of being ' + + 'silently dropped.', parameters: {}, output: { schema: { @@ -70,7 +72,9 @@ export function apply(ctx: Context): void { // Non-agent callers have no session whose children could be listed. throw new Error('list_agents requires a calling agent (exec.agent was undefined)') } - return await ctx.subagents.listChildren(parent.id) + // The registry drains started tool bodies, so the scan must observe the + // call's signal rather than finish a slow catalog after cancellation. + return await ctx.subagents.listChildren(parent.id, exec.signal) }, })) }