From a520965f09a9721ccfec1dbaf2515fd4674ac872 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 7 Jul 2026 00:11:55 +0800 Subject: [PATCH] fix review findings: post-capture tool calls denied; schema snapshotted at start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bot findings on the structured runtime: - Terminal means terminal WITHIN the step: a model response listing structured_output before further tool calls executed those calls after the final answer was accepted (the turn-continuation veto only fires at step end). A third runtime listener now denies every call for a captured agent at the tools/pre-execute gate — dispatch skipped, isError result naming the contract. Calls preceding the capture in the same response are untouched. - The output schema is structuredClone'd before the subset assertion: the caller keeps its reference, so asserting and attaching the original let a post-start() mutation drift the enforced schema away from the asserted one. The clone pins assertion, model-visible parameters, and validation to one value. --- .../subagent/subagent-inprocess/src/index.ts | 11 ++- .../subagent-inprocess/src/structured.ts | 31 ++++++- .../tests/structured.spec.ts | 86 ++++++++++++++++++- 3 files changed, 122 insertions(+), 6 deletions(-) diff --git a/packages/subagent/subagent-inprocess/src/index.ts b/packages/subagent/subagent-inprocess/src/index.ts index 381f94acba..1309556f3f 100644 --- a/packages/subagent/subagent-inprocess/src/index.ts +++ b/packages/subagent/subagent-inprocess/src/index.ts @@ -119,9 +119,14 @@ export function startInProcessRun( if (request.maxDepth !== undefined && childDepth > request.maxDepth) { throw new SubagentDepthError(childDepth, request.maxDepth) } - // Assert the schema subset BEFORE any child exists (the service has already - // capability-gated; this rejects a schema outside the enforced subset loud). - const schema = request.outputSchema + // Snapshot, then assert, the schema subset BEFORE any child exists (the + // service has already capability-gated; this rejects a schema outside the + // enforced subset loud). The snapshot is load-bearing: the caller keeps its + // reference, so validating and attaching the ORIGINAL would let a + // post-start() mutation drift the enforced schema away from the asserted + // one — the clone pins assertion, the model-visible parameters, and + // validateStructuredValue to the same isolation-immutable value. + const schema = request.outputSchema === undefined ? undefined : structuredClone(request.outputSchema) if (schema !== undefined) assertSupportedOutputSchema(schema) const childId = AgentId(randomUUID()) diff --git a/packages/subagent/subagent-inprocess/src/structured.ts b/packages/subagent/subagent-inprocess/src/structured.ts index 69e4ea4fd6..e866e5e699 100644 --- a/packages/subagent/subagent-inprocess/src/structured.ts +++ b/packages/subagent/subagent-inprocess/src/structured.ts @@ -25,7 +25,11 @@ * output is captured — without it, the loop's default "had tool calls ⇒ * continue" buys a wasted extra model step per structured child. It is also * `prepend: true`: the veto must run before any earlier-registered listener - * that could short-circuit the chain into a forced continue. + * that could short-circuit the chain into a forced continue. A third listener + * closes the within-step window the continuation veto cannot: a + * `tools/pre-execute` deny for any call arriving after the agent's capture, so + * a response that lists `structured_output` before further tool calls cannot + * run side effects after the final answer was accepted. * * Lifetime is refcounted with two kinds of holder: each backend acquires for * its plugin lifetime (so the tool exists before any run), and each structured @@ -42,7 +46,7 @@ import type { Agent } from '@deepseek-ai/dsh-agent' import type { ContentBlock, ToolSchema } from '@deepseek-ai/dsh-llm' import type { ContinuationDecision } from '@deepseek-ai/dsh-agent' import type { AssembleContext, PromptAssembly } from '@deepseek-ai/dsh-system-prompt' -import type { ToolExecution } from '@deepseek-ai/dsh-tools' +import type { PreToolDecision, ToolExecution } from '@deepseek-ai/dsh-tools' import { ToolArgsError, validateStructuredValue, type StructuredOutputSchema } from '@deepseek-ai/dsh-tools' /** The model-facing tool name a structured child must call to finish. */ @@ -236,4 +240,27 @@ function registerRuntime(root: Context, runtime: StructuredRuntime): void { if (runtime.states.get(agent)?.captured) return Promise.resolve({ action: 'stop' }) return next() }, { prepend: true })) + + // Terminal means terminal WITHIN the step, not only at its end: the + // turn-continuation veto above runs after every call in the current model + // response has executed, so a response that puts `structured_output` before + // further tool calls would still perform those side effects after the final + // answer was accepted. Deny every later call for a captured agent at the + // allow/deny gate — dispatch is skipped and the model sees an `isError` + // result naming the contract. Calls that PRECEDE the capture in the same + // response ran before `captured` was set and are untouched; a second + // `structured_output` is denied like any other call. `prepend: true` for the + // same reason as the continuation veto: no earlier-registered allow may + // short-circuit past the terminal contract. + runtime.disposers.push(root.on('tools/pre-execute', function ( + this: unknown, exec: ToolExecution, next: () => Promise, + ): Promise { + if (exec.agent && runtime.states.get(exec.agent)?.captured) { + return Promise.resolve({ + kind: 'deny', + reason: `structured output already recorded: the run is complete, so \`${exec.name}\` is not executed`, + }) + } + return next() + }, { prepend: true })) } diff --git a/packages/subagent/subagent-inprocess/tests/structured.spec.ts b/packages/subagent/subagent-inprocess/tests/structured.spec.ts index 6982f7cde5..006fcd9d8a 100644 --- a/packages/subagent/subagent-inprocess/tests/structured.spec.ts +++ b/packages/subagent/subagent-inprocess/tests/structured.spec.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest' import { Context } from 'cordis' -import LlmService, { type GenerateOptions } from '@deepseek-ai/dsh-llm' +import LlmService, { CallId, type ContentBlock, type GenerateOptions } from '@deepseek-ai/dsh-llm' import SessionStore from '@deepseek-ai/dsh-session' import SystemPrompt from '@deepseek-ai/dsh-system-prompt' import ToolRegistry from '@deepseek-ai/dsh-tools' @@ -86,6 +86,90 @@ describe('in-process structured output', () => { await run.dispose() }) + it('denies tool calls that FOLLOW the capture in the same response — terminal means terminal', async () => { + // One model response carrying structured_output FIRST and a side-effecting + // call after it: the continuation veto only fires at step end, so without + // the pre-execute deny the trailing call would still run after the final + // answer was accepted. + const response = [ + ...toolCallResponse('c1', STRUCTURED_OUTPUT_TOOL, { answer: 5 }).slice(0, -2), + { type: 'block-start', index: 1, blockType: 'tool-call' }, + { type: 'block-end', index: 1, block: { type: 'tool-call', id: CallId('c2'), name: 'side_effect', arguments: '{}' } }, + { type: 'usage', usage: { inputTokens: 10, outputTokens: 5 } }, + { type: 'finish', reason: { kind: 'tool-calls' } }, + ] as Script[number] + const { ctx, parent } = await setup([response]) + let sideEffectRan = false + ctx.tools.register({ + name: 'side_effect', + description: 'probe', + parameters: { type: 'object', properties: {} }, + execute(): Promise { + sideEffectRan = true + return Promise.resolve([{ type: 'text', text: 'ran' }]) + }, + }) + const run = ctx.subagents.start('spawn', structuredRequest(parent)) + const result = await run.result + expect(result.stopReason).toBe('completed') + expect(result.structured).toEqual({ answer: 5 }) + // The deny skipped dispatch entirely: the probe body never ran. + expect(sideEffectRan).toBe(false) + await run.dispose() + }) + + it('leaves tool calls that PRECEDE the capture in the same response untouched', async () => { + const response = [ + { type: 'block-start', index: 0, blockType: 'tool-call' }, + { type: 'block-end', index: 0, block: { type: 'tool-call', id: CallId('c1'), name: 'side_effect', arguments: '{}' } }, + ...toolCallResponse('c2', STRUCTURED_OUTPUT_TOOL, { answer: 6 }).map(chunk => + 'index' in chunk ? { ...chunk, index: 1 } : chunk), + ] as Script[number] + const { ctx, parent } = await setup([response]) + let sideEffectRan = false + ctx.tools.register({ + name: 'side_effect', + description: 'probe', + parameters: { type: 'object', properties: {} }, + execute(): Promise { + sideEffectRan = true + return Promise.resolve([{ type: 'text', text: 'ran' }]) + }, + }) + const run = ctx.subagents.start('spawn', structuredRequest(parent)) + const result = await run.result + // The call ran BEFORE captured was set: the deny gate only guards the + // window after the terminal answer landed. + expect(sideEffectRan).toBe(true) + expect(result.structured).toEqual({ answer: 6 }) + await run.dispose() + }) + + it('snapshots the schema at start(): caller mutation after start cannot drift enforcement', async () => { + const mutable: StructuredOutputSchema = { + type: 'object', + properties: { answer: { type: 'number' } }, + required: ['answer'], + additionalProperties: false, + } + const pristine = structuredClone(mutable) + const { ctx, parent, adapter } = await setup([ + toolCallResponse('c1', STRUCTURED_OUTPUT_TOOL, { answer: 3 }), + ]) + const run = ctx.subagents.start('spawn', structuredRequest(parent, { outputSchema: mutable })) + // Mutate the caller's object AFTER start() returned but before the child's + // first request assembles: with a live reference this would reach both the + // model-visible parameters and validateStructuredValue. + ;(mutable.properties as Record).answer = { type: 'string' } + const result = await run.result + expect(result.structured).toEqual({ answer: 3 }) + // The child's request carried the PRISTINE schema, not the mutated one. + const childRequest = adapter.requests.at(-1) + const captureTool = (childRequest?.tools ?? []).find(tool => tool.name === STRUCTURED_OUTPUT_TOOL) + expect(captureTool?.parameters).toEqual(pristine) + await run.dispose() + }) + it('the captured-turn veto is prepend: an EARLIER force-continue listener cannot short-circuit it', async () => { const ctx = new Context() await ctx.plugin(SystemPrompt)