From e7595e2fa9daa9c8539006c0145087b6ea61a8b9 Mon Sep 17 00:00:00 2001 From: "agentfront[bot]" Date: Mon, 2 Nov 2026 06:31:13 +0000 Subject: [PATCH 1/2] Cherry-pick: fix(sdk): run Finally hooks on every stage outcome and close spans for refused answers Cherry-picked from #980 (merged to release/1.10.x) Original commit: 4a445a6f18020a04573c038673101150288b66b4 Co-Authored-By: frontegg-david <69419539+frontegg-david@users.noreply.github.com> --- .../e2e/dts-consumer.e2e.spec.ts | 2 +- .../fixture/hooks-must-compile.ts | 14 + .../e2e/finally-hooks.e2e.spec.ts | 81 ++++ .../src/apps/audit/data/finally-log.ts | 26 ++ .../demo-e2e-hooks/src/apps/audit/index.ts | 17 +- .../audit/plugins/finally-recorder.plugin.ts | 44 +++ .../apps/audit/tools/clear-audit-log.tool.ts | 4 +- .../apps/audit/tools/failing-execute.tool.ts | 18 + .../apps/audit/tools/get-finally-log.tool.ts | 28 ++ .../apps/audit/tools/refused-answer.tool.ts | 18 + .../observability-refused-answer.e2e.spec.ts | 33 ++ .../src/apps/observability-probe/index.ts | 16 + .../observability-probe/telemetry-records.ts | 42 ++ .../tools/get-telemetry.tool.ts | 28 ++ .../tools/refused-answer.tool.ts | 18 + .../src/main-observability.ts | 8 +- .../__tests__/observability-plugin.spec.ts | 48 ++- .../src/__tests__/server-telemetry.spec.ts | 32 +- .../src/plugin/observability.plugin.ts | 48 +-- .../src/common/decorators/hook.decorator.ts | 17 +- .../common/interfaces/internal/flow.utils.ts | 3 + libs/sdk/src/common/metadata/hook.metadata.ts | 3 +- .../flows/__tests__/flow-error-state.spec.ts | 116 +++++- .../__tests__/flow-stage-hook-lists.spec.ts | 16 +- .../flows/__tests__/flow-typed-keys.spec.ts | 26 ++ libs/sdk/src/flows/flow.instance.ts | 102 ++++- libs/sdk/src/flows/flow.stages.ts | 8 +- .../__tests__/flow-hooks.behaviour.spec.ts | 19 + .../__tests__/flow-hooks.finally.spec.ts | 359 ++++++++++++++++++ .../job/__tests__/execute-job.flow.spec.ts | 46 ++- .../__tests__/call-tool.output-schema.spec.ts | 31 +- libs/sdk/src/tool/flows/call-tool.flow.ts | 2 + .../catalog/frontmcp-development/SKILL.md | 2 +- .../references/create-plugin-hooks.md | 98 +++-- .../references/decorators-guide.md | 112 +++--- .../telemetry-api/plugin-telemetry.md | 4 +- libs/skills/catalog/skills-manifest.json | 4 +- 37 files changed, 1314 insertions(+), 179 deletions(-) create mode 100644 apps/e2e/demo-e2e-hooks/e2e/finally-hooks.e2e.spec.ts create mode 100644 apps/e2e/demo-e2e-hooks/src/apps/audit/data/finally-log.ts create mode 100644 apps/e2e/demo-e2e-hooks/src/apps/audit/plugins/finally-recorder.plugin.ts create mode 100644 apps/e2e/demo-e2e-hooks/src/apps/audit/tools/failing-execute.tool.ts create mode 100644 apps/e2e/demo-e2e-hooks/src/apps/audit/tools/get-finally-log.tool.ts create mode 100644 apps/e2e/demo-e2e-hooks/src/apps/audit/tools/refused-answer.tool.ts create mode 100644 apps/e2e/demo-e2e-standalone/e2e/observability-refused-answer.e2e.spec.ts create mode 100644 apps/e2e/demo-e2e-standalone/src/apps/observability-probe/index.ts create mode 100644 apps/e2e/demo-e2e-standalone/src/apps/observability-probe/telemetry-records.ts create mode 100644 apps/e2e/demo-e2e-standalone/src/apps/observability-probe/tools/get-telemetry.tool.ts create mode 100644 apps/e2e/demo-e2e-standalone/src/apps/observability-probe/tools/refused-answer.tool.ts create mode 100644 libs/sdk/src/flows/__tests__/flow-typed-keys.spec.ts create mode 100644 libs/sdk/src/hooks/__tests__/flow-hooks.finally.spec.ts diff --git a/apps/e2e/demo-e2e-dts-consumer/e2e/dts-consumer.e2e.spec.ts b/apps/e2e/demo-e2e-dts-consumer/e2e/dts-consumer.e2e.spec.ts index 4183ee0a8..e5bbbe45e 100644 --- a/apps/e2e/demo-e2e-dts-consumer/e2e/dts-consumer.e2e.spec.ts +++ b/apps/e2e/demo-e2e-dts-consumer/e2e/dts-consumer.e2e.spec.ts @@ -90,7 +90,7 @@ describe('@frontmcp/sdk declarations in a strict consumer project', () => { expect(describeEach(inFixtureFile('prompts-must-compile.ts'))).toEqual([]); }); - it('types the prompt and completion hooks, the scope channel accessors and server-level adapters', () => { + it('types the prompt, completion and Finally hooks, the scope channel accessors and server-level adapters', () => { expect(describeEach(inFixtureFile('hooks-must-compile.ts'))).toEqual([]); }); diff --git a/apps/e2e/demo-e2e-dts-consumer/fixture/hooks-must-compile.ts b/apps/e2e/demo-e2e-dts-consumer/fixture/hooks-must-compile.ts index 626ba3a03..c98ffdb31 100644 --- a/apps/e2e/demo-e2e-dts-consumer/fixture/hooks-must-compile.ts +++ b/apps/e2e/demo-e2e-dts-consumer/fixture/hooks-must-compile.ts @@ -7,6 +7,7 @@ import { FrontMcp, ListPromptsHook, PromptHook, + ToolHook, type FlowCtxOf, type FrontMcpAdapterResponse, type ScopeEntry, @@ -37,6 +38,19 @@ export class PromptAudit { @CompletionHook.Did('complete') afterComplete(): void {} + + @GetPrompt.Finally('execute') + afterPromptWhateverItsOutcome(ctx: FlowCtxOf<'prompts:get-prompt'>): void { + void ctx.state.flowError?.message; + } +} + +// Finally hooks run after a stage whatever its outcome, and read the failure from state.flowError. +export class CallAudit { + @ToolHook.Finally('finalize') + afterCall(ctx: FlowCtxOf<'tools:call-tool'>): void { + void ctx.state.flowError?.message; + } } // ScopeEntry declares the channel accessors, so no cast is needed. diff --git a/apps/e2e/demo-e2e-hooks/e2e/finally-hooks.e2e.spec.ts b/apps/e2e/demo-e2e-hooks/e2e/finally-hooks.e2e.spec.ts new file mode 100644 index 000000000..e7c026e08 --- /dev/null +++ b/apps/e2e/demo-e2e-hooks/e2e/finally-hooks.e2e.spec.ts @@ -0,0 +1,81 @@ +/** + * E2E: Finally hooks run after a stage whatever its outcome, and never change what the client gets. + * + * The finally-recorder plugin records how the `execute` and `finalize` stages of a call ended. Before its + * recorders run, two other Finally hooks of `finalize` try to answer in place of the call and to fail it: both + * are ignored. `refused-answer` answers with a value its output schema refuses (a `finalize` failure, which used + * to skip every hook after the stage), and `failing-execute` fails in `execute`. + */ +import { expect, test, type ServerFixture } from '@frontmcp/testing'; + +interface FinallyLog { + entries: Array<{ stage: string; outcome: string }>; +} + +/** The server's logs once they contain `text`, or as they are after `timeoutMs`: its output reaches us after the reply. */ +async function logsOnceTheyContain(server: ServerFixture, text: string, timeoutMs = 5000): Promise { + const deadline = Date.now() + timeoutMs; + while (!server.getLogs().join('\n').includes(text) && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 50)); + } + return server.getLogs().join('\n'); +} + +test.describe('Finally hooks E2E', () => { + test.use({ + server: 'apps/e2e/demo-e2e-hooks/src/main.ts', + project: 'demo-e2e-hooks', + publicMode: true, + }); + + test('see an answer the output schema refuses, and the client still gets INVALID_OUTPUT', async ({ mcp }) => { + await mcp.tools.call('clear-audit-log', {}); + + const result = await mcp.tools.call('refused-answer', {}); + + expect(result).toBeError(); + expect(result.raw._meta?.['code']).toBe('INVALID_OUTPUT'); + expect(result).not.toHaveTextContent('replaced by a Finally hook'); + const log = await mcp.tools.call('get-finally-log', { toolName: 'refused-answer' }); + expect(log.json().entries).toEqual([ + { stage: 'execute', outcome: 'ok' }, + { stage: 'finalize', outcome: 'InvalidOutputError' }, + ]); + }); + + test('see an execute that failed, and the client still gets its error', async ({ mcp }) => { + await mcp.tools.call('clear-audit-log', {}); + + const result = await mcp.tools.call('failing-execute', {}); + + expect(result).toBeError(); + expect(result.text()).toBe('The ledger is unreachable'); + const log = await mcp.tools.call('get-finally-log', { toolName: 'failing-execute' }); + expect(log.json().entries).toEqual([ + { stage: 'execute', outcome: 'PublicMcpError' }, + { stage: 'finalize', outcome: 'PublicMcpError' }, + ]); + }); + + test('see a call that succeeded, and its answer is left as it was', async ({ mcp }) => { + await mcp.tools.call('clear-audit-log', {}); + + const result = await mcp.tools.call('audited-tool', { message: 'finally' }); + + expect(result).toBeSuccessful(); + expect(result.json<{ message: string }>().message).toBe('Processed: finally'); + const log = await mcp.tools.call('get-finally-log', { toolName: 'audited-tool' }); + expect(log.json().entries).toEqual([ + { stage: 'execute', outcome: 'ok' }, + { stage: 'finalize', outcome: 'ok' }, + ]); + }); + + test('log what an ignored Finally hook threw', async ({ mcp, server }) => { + server.clearLogs(); + + await mcp.tools.call('audited-tool', { message: 'logged' }); + + expect(await logsOnceTheyContain(server, 'the recorder store is down')).toContain('the recorder store is down'); + }); +}); diff --git a/apps/e2e/demo-e2e-hooks/src/apps/audit/data/finally-log.ts b/apps/e2e/demo-e2e-hooks/src/apps/audit/data/finally-log.ts new file mode 100644 index 000000000..f592ae1af --- /dev/null +++ b/apps/e2e/demo-e2e-hooks/src/apps/audit/data/finally-log.ts @@ -0,0 +1,26 @@ +/** + * What the Finally hooks of the finally-recorder plugin saw, per tool call stage. + */ +export interface FinallyEntry { + toolName: string; + stage: string; + outcome: string; +} + +class FinallyLogStore { + private entries: FinallyEntry[] = []; + + add(entry: FinallyEntry): void { + this.entries.push(entry); + } + + entriesFor(toolName: string): FinallyEntry[] { + return this.entries.filter((entry) => entry.toolName === toolName); + } + + clear(): void { + this.entries = []; + } +} + +export const finallyLog = new FinallyLogStore(); diff --git a/apps/e2e/demo-e2e-hooks/src/apps/audit/index.ts b/apps/e2e/demo-e2e-hooks/src/apps/audit/index.ts index 9ddb5a292..a112818ce 100644 --- a/apps/e2e/demo-e2e-hooks/src/apps/audit/index.ts +++ b/apps/e2e/demo-e2e-hooks/src/apps/audit/index.ts @@ -1,17 +1,30 @@ import { App } from '@frontmcp/sdk'; import AuditPlugin from './plugins/audit.plugin'; +import FinallyRecorderPlugin from './plugins/finally-recorder.plugin'; import AuditSummaryPrompt from './prompts/audit-summary.prompt'; import AuditLogResource from './resources/audit-log.resource'; import AuditedTool from './tools/audited.tool'; import ClearAuditLogTool from './tools/clear-audit-log.tool'; +import FailingExecuteTool from './tools/failing-execute.tool'; import GetAuditLogTool from './tools/get-audit-log.tool'; +import GetFinallyLogTool from './tools/get-finally-log.tool'; +import RefusedAnswerTool from './tools/refused-answer.tool'; import { PlainEchoTool, StaticGuardedEchoTool } from './tools/static-guarded.tool'; @App({ name: 'audit', - plugins: [AuditPlugin], - tools: [AuditedTool, GetAuditLogTool, ClearAuditLogTool, StaticGuardedEchoTool, PlainEchoTool], + plugins: [AuditPlugin, FinallyRecorderPlugin], + tools: [ + AuditedTool, + GetAuditLogTool, + ClearAuditLogTool, + StaticGuardedEchoTool, + PlainEchoTool, + RefusedAnswerTool, + FailingExecuteTool, + GetFinallyLogTool, + ], resources: [AuditLogResource], prompts: [AuditSummaryPrompt], }) diff --git a/apps/e2e/demo-e2e-hooks/src/apps/audit/plugins/finally-recorder.plugin.ts b/apps/e2e/demo-e2e-hooks/src/apps/audit/plugins/finally-recorder.plugin.ts new file mode 100644 index 000000000..52c80881f --- /dev/null +++ b/apps/e2e/demo-e2e-hooks/src/apps/audit/plugins/finally-recorder.plugin.ts @@ -0,0 +1,44 @@ +import { Plugin, ToolHook, type FlowCtxOf } from '@frontmcp/sdk'; + +import { finallyLog } from '../data/finally-log'; + +type CallToolFlow = FlowCtxOf<'tools:call-tool'>; + +const RECORDED_TOOLS = new Set(['audited-tool', 'refused-answer', 'failing-execute']); + +const isRecorded = (flowCtx: CallToolFlow) => RECORDED_TOOLS.has(String(flowCtx.state.input?.name)); + +function record(flowCtx: CallToolFlow, stage: string): void { + finallyLog.add({ + toolName: String(flowCtx.state.input?.name), + stage, + outcome: flowCtx.state.flowError?.name ?? 'ok', + }); +} + +/** The first two Finally hooks try to answer and to fail the call: both are ignored, and the recorders still run. */ +@Plugin({ + name: 'finally-recorder', + description: 'Records how the execute and finalize stages of a tool call ended', +}) +export default class FinallyRecorderPlugin { + @ToolHook.Finally('finalize', { priority: -10, filter: isRecorded }) + replaceAnswer(flowCtx: CallToolFlow) { + flowCtx.respond({ content: [{ type: 'text', text: 'replaced by a Finally hook' }] }); + } + + @ToolHook.Finally('finalize', { priority: -5, filter: isRecorded }) + failRecorder() { + throw new Error('the recorder store is down'); + } + + @ToolHook.Finally('execute', { filter: isRecorded }) + recordExecute(flowCtx: CallToolFlow) { + record(flowCtx, 'execute'); + } + + @ToolHook.Finally('finalize', { filter: isRecorded }) + recordFinalize(flowCtx: CallToolFlow) { + record(flowCtx, 'finalize'); + } +} diff --git a/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/clear-audit-log.tool.ts b/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/clear-audit-log.tool.ts index 1cc9b55f2..1aadacd9f 100644 --- a/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/clear-audit-log.tool.ts +++ b/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/clear-audit-log.tool.ts @@ -2,6 +2,7 @@ import { z } from '@frontmcp/lazy-zod'; import { Tool, ToolContext } from '@frontmcp/sdk'; import { auditLog } from '../data/audit-log'; +import { finallyLog } from '../data/finally-log'; const inputSchema = {}; @@ -15,13 +16,14 @@ type Output = z.infer; @Tool({ name: 'clear-audit-log', - description: 'Clear all audit log entries', + description: 'Clear all audit log and Finally log entries', inputSchema, outputSchema, }) export default class ClearAuditLogTool extends ToolContext { async execute(_input: Input): Promise { auditLog.clear(); + finallyLog.clear(); return { success: true, diff --git a/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/failing-execute.tool.ts b/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/failing-execute.tool.ts new file mode 100644 index 000000000..c46afb1ee --- /dev/null +++ b/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/failing-execute.tool.ts @@ -0,0 +1,18 @@ +import { z } from '@frontmcp/lazy-zod'; +import { PublicMcpError, Tool, ToolContext } from '@frontmcp/sdk'; + +const outputSchema = z.object({ posted: z.boolean() }); + +type Output = z.infer; + +@Tool({ + name: 'failing-execute', + description: 'Fails while it runs', + inputSchema: {}, + outputSchema, +}) +export default class FailingExecuteTool extends ToolContext { + async execute(): Promise { + throw new PublicMcpError('The ledger is unreachable'); + } +} diff --git a/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/get-finally-log.tool.ts b/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/get-finally-log.tool.ts new file mode 100644 index 000000000..ddb9c56e4 --- /dev/null +++ b/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/get-finally-log.tool.ts @@ -0,0 +1,28 @@ +import { z } from '@frontmcp/lazy-zod'; +import { Tool, ToolContext } from '@frontmcp/sdk'; + +import { finallyLog } from '../data/finally-log'; + +const inputSchema = { + toolName: z.string().describe('The tool whose calls to list'), +}; + +const outputSchema = z.object({ + entries: z.array(z.object({ stage: z.string(), outcome: z.string() })), +}); + +type Input = z.infer>; +type Output = z.infer; + +@Tool({ + name: 'get-finally-log', + description: 'How the execute and finalize stages of the calls of one tool ended, as its Finally hooks saw them', + inputSchema, + outputSchema, +}) +export default class GetFinallyLogTool extends ToolContext { + async execute(input: Input): Promise { + const entries = finallyLog.entriesFor(input.toolName).map(({ stage, outcome }) => ({ stage, outcome })); + return { entries }; + } +} diff --git a/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/refused-answer.tool.ts b/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/refused-answer.tool.ts new file mode 100644 index 000000000..b187bf3bf --- /dev/null +++ b/apps/e2e/demo-e2e-hooks/src/apps/audit/tools/refused-answer.tool.ts @@ -0,0 +1,18 @@ +import { z } from '@frontmcp/lazy-zod'; +import { Tool, ToolContext } from '@frontmcp/sdk'; + +const outputSchema = z.object({ total: z.number() }); + +type Output = z.infer; + +@Tool({ + name: 'refused-answer', + description: 'Answers with a total its own output schema refuses', + inputSchema: {}, + outputSchema, +}) +export default class RefusedAnswerTool extends ToolContext { + async execute(): Promise { + return { total: 'twelve' } as unknown as Output; + } +} diff --git a/apps/e2e/demo-e2e-standalone/e2e/observability-refused-answer.e2e.spec.ts b/apps/e2e/demo-e2e-standalone/e2e/observability-refused-answer.e2e.spec.ts new file mode 100644 index 000000000..c845d3548 --- /dev/null +++ b/apps/e2e/demo-e2e-standalone/e2e/observability-refused-answer.e2e.spec.ts @@ -0,0 +1,33 @@ +/** + * E2E: `@frontmcp/observability` closes the `tools/call` span and records the failure on the request log when + * a tool's output schema refuses its answer. The refusal fails the `finalize` stage, which skips that stage's + * Did hooks, so the span stayed open and the request log recorded no error. Observability closes them in a + * Finally hook now. + */ +import { expect, test } from '@frontmcp/testing'; + +interface Telemetry { + spans: Array<{ tool: string; call: string }>; + requestLogs: Array<{ status: string; errorCode?: string }>; +} + +test.describe('Observability of an answer the output schema refuses', () => { + test.use({ + server: 'apps/e2e/demo-e2e-standalone/src/main-observability.ts', + project: 'demo-e2e-standalone', + publicMode: true, + }); + + test('closes the tools/call span as failed and records INVALID_OUTPUT on the request log', async ({ mcp }) => { + const result = await mcp.tools.call('refused-answer', {}); + expect(result).toBeError(); + expect(result.raw._meta?.['code']).toBe('INVALID_OUTPUT'); + + const telemetry = await mcp.tools.call('get-telemetry', { toolName: 'refused-answer' }); + + expect(telemetry.json()).toEqual({ + spans: [{ tool: 'ok', call: 'error' }], + requestLogs: [{ status: 'error', errorCode: 'INVALID_OUTPUT' }], + }); + }); +}); diff --git a/apps/e2e/demo-e2e-standalone/src/apps/observability-probe/index.ts b/apps/e2e/demo-e2e-standalone/src/apps/observability-probe/index.ts new file mode 100644 index 000000000..ca7062f44 --- /dev/null +++ b/apps/e2e/demo-e2e-standalone/src/apps/observability-probe/index.ts @@ -0,0 +1,16 @@ +import { App } from '@frontmcp/sdk'; + +import GetTelemetryTool from './tools/get-telemetry.tool'; +import RefusedAnswerTool from './tools/refused-answer.tool'; + +export { recordRequestLog } from './telemetry-records'; + +/** + * Reports what `@frontmcp/observability` recorded for a call whose answer its output schema refuses. + */ +@App({ + name: 'observability-probe', + description: 'A tool whose answer its output schema refuses, and what observability recorded for it', + tools: [RefusedAnswerTool, GetTelemetryTool], +}) +export class ObservabilityProbeApp {} diff --git a/apps/e2e/demo-e2e-standalone/src/apps/observability-probe/telemetry-records.ts b/apps/e2e/demo-e2e-standalone/src/apps/observability-probe/telemetry-records.ts new file mode 100644 index 000000000..6b52c47bd --- /dev/null +++ b/apps/e2e/demo-e2e-standalone/src/apps/observability-probe/telemetry-records.ts @@ -0,0 +1,42 @@ +import { SpanStatusCode, trace } from '@opentelemetry/api'; +import { + BasicTracerProvider, + InMemorySpanExporter, + SimpleSpanProcessor, + type ReadableSpan, +} from '@opentelemetry/sdk-trace-base'; + +/** Registered when this module loads, before the server starts, so observability records into this exporter. */ +const exporter = new InMemorySpanExporter(); +trace.setGlobalTracerProvider(new BasicTracerProvider({ spanProcessors: [new SimpleSpanProcessor(exporter)] })); + +interface RecordedRequestLog { + tool_name?: string; + status?: string; + error?: { code?: string }; +} + +const requestLogs: RecordedRequestLog[] = []; + +export function recordRequestLog(log: unknown): void { + requestLogs.push(log as RecordedRequestLog); +} + +const statusOf = (span: ReadableSpan) => (span.status.code === SpanStatusCode.ERROR ? 'error' : 'ok'); + +/** For each finished span of the tool, its status and the status of its `tools/call` span, or 'open'. */ +export function callSpanStatuses(toolName: string): Array<{ tool: string; call: string }> { + const finished = exporter.getFinishedSpans(); + return finished + .filter((span) => span.name === `tool ${toolName}`) + .map((toolSpan) => { + const callSpan = finished.find((span) => span.spanContext().spanId === toolSpan.parentSpanContext?.spanId); + return { tool: statusOf(toolSpan), call: callSpan ? statusOf(callSpan) : 'open' }; + }); +} + +export function toolRequestLogs(toolName: string): Array<{ status: string; errorCode?: string }> { + return requestLogs + .filter((log) => log.tool_name === toolName) + .map((log) => ({ status: String(log.status), errorCode: log.error?.code })); +} diff --git a/apps/e2e/demo-e2e-standalone/src/apps/observability-probe/tools/get-telemetry.tool.ts b/apps/e2e/demo-e2e-standalone/src/apps/observability-probe/tools/get-telemetry.tool.ts new file mode 100644 index 000000000..0f28e4628 --- /dev/null +++ b/apps/e2e/demo-e2e-standalone/src/apps/observability-probe/tools/get-telemetry.tool.ts @@ -0,0 +1,28 @@ +import { z } from '@frontmcp/lazy-zod'; +import { Tool, ToolContext } from '@frontmcp/sdk'; + +import { callSpanStatuses, toolRequestLogs } from '../telemetry-records'; + +const inputSchema = { + toolName: z.string().describe('The tool whose calls to report'), +}; + +const outputSchema = z.object({ + spans: z.array(z.object({ tool: z.string(), call: z.string() })), + requestLogs: z.array(z.object({ status: z.string(), errorCode: z.string().optional() })), +}); + +type Input = z.infer>; +type Output = z.infer; + +@Tool({ + name: 'get-telemetry', + description: 'The span statuses and the request logs of the calls of one tool', + inputSchema, + outputSchema, +}) +export default class GetTelemetryTool extends ToolContext { + async execute(input: Input): Promise { + return { spans: callSpanStatuses(input.toolName), requestLogs: toolRequestLogs(input.toolName) }; + } +} diff --git a/apps/e2e/demo-e2e-standalone/src/apps/observability-probe/tools/refused-answer.tool.ts b/apps/e2e/demo-e2e-standalone/src/apps/observability-probe/tools/refused-answer.tool.ts new file mode 100644 index 000000000..b187bf3bf --- /dev/null +++ b/apps/e2e/demo-e2e-standalone/src/apps/observability-probe/tools/refused-answer.tool.ts @@ -0,0 +1,18 @@ +import { z } from '@frontmcp/lazy-zod'; +import { Tool, ToolContext } from '@frontmcp/sdk'; + +const outputSchema = z.object({ total: z.number() }); + +type Output = z.infer; + +@Tool({ + name: 'refused-answer', + description: 'Answers with a total its own output schema refuses', + inputSchema: {}, + outputSchema, +}) +export default class RefusedAnswerTool extends ToolContext { + async execute(): Promise { + return { total: 'twelve' } as unknown as Output; + } +} diff --git a/apps/e2e/demo-e2e-standalone/src/main-observability.ts b/apps/e2e/demo-e2e-standalone/src/main-observability.ts index 71abf57f6..3f2c9e944 100644 --- a/apps/e2e/demo-e2e-standalone/src/main-observability.ts +++ b/apps/e2e/demo-e2e-standalone/src/main-observability.ts @@ -1,5 +1,6 @@ import { FrontMcp, LogLevel } from '@frontmcp/sdk'; +import { ObservabilityProbeApp, recordRequestLog } from './apps/observability-probe'; import { ParentApp } from './apps/parent'; const port = parseInt(process.env['PORT'] ?? '3104', 10); @@ -14,11 +15,12 @@ const port = parseInt(process.env['PORT'] ?? '3104', 10); * Telemetry must activate (or, if the module genuinely fails to load, the SDK * must say so accurately rather than claim it isn't installed). * - * Uses the issue's exact config shape. + * Uses the issue's exact config shape, plus a request-log callback and a probe app that reports the spans and + * request logs of a call whose answer its output schema refuses. */ @FrontMcp({ info: { name: 'Demo E2E Observability', version: '0.1.0' }, - apps: [ParentApp], + apps: [ParentApp, ObservabilityProbeApp], logging: { level: LogLevel.Info }, http: { port }, auth: { mode: 'public', sessionTtl: 3600, anonymousScopes: ['anonymous'] }, @@ -26,7 +28,7 @@ const port = parseInt(process.env['PORT'] ?? '3104', 10); observability: { tracing: true, logging: { sinks: [{ type: 'stdout' }] }, - requestLogs: true, + requestLogs: { onRequestComplete: recordRequestLog }, }, }) export default class Server {} diff --git a/libs/observability/src/__tests__/observability-plugin.spec.ts b/libs/observability/src/__tests__/observability-plugin.spec.ts index c2a1f45ee..e58c91433 100644 --- a/libs/observability/src/__tests__/observability-plugin.spec.ts +++ b/libs/observability/src/__tests__/observability-plugin.spec.ts @@ -1,7 +1,9 @@ +import { FrontMcpFlowHookTokens, type HookMetadata } from '@frontmcp/sdk'; + +import { StructuredLogTransport } from '../logging/structured-log-transport'; +import { OTEL_CONFIG, OTEL_TRACER } from '../otel/otel.tokens'; import ObservabilityPlugin from '../plugin/observability.plugin'; -import { OTEL_TRACER, OTEL_CONFIG } from '../otel/otel.tokens'; import { REQUEST_LOG_COLLECTOR } from '../request-log/request-log.tokens'; -import { StructuredLogTransport } from '../logging/structured-log-transport'; describe('ObservabilityPlugin', () => { describe('constructor', () => { @@ -69,7 +71,7 @@ describe('ObservabilityPlugin', () => { it('_httpDidAuth', () => plugin._httpDidAuth(emptyCtx)); it('_httpWillRoute', () => plugin._httpWillRoute(emptyCtx)); it('_httpDidRoute', () => plugin._httpDidRoute(emptyCtx)); - it('_httpDidFinalize', () => plugin._httpDidFinalize(emptyCtx)); + it('_httpFinallyFinalize', () => plugin._httpFinallyFinalize(emptyCtx)); // Tool hooks it('_toolWillParse', () => plugin._toolWillParse(emptyCtx)); @@ -81,28 +83,28 @@ describe('ObservabilityPlugin', () => { it('_toolDidExecute', () => plugin._toolDidExecute(emptyCtx)); it('_toolWillValidateOut', () => plugin._toolWillValidateOut(emptyCtx)); it('_toolWillApplyUI', () => plugin._toolWillApplyUI(emptyCtx)); - it('_toolDidFinalize', () => plugin._toolDidFinalize(emptyCtx)); + it('_toolFinallyFinalize', () => plugin._toolFinallyFinalize(emptyCtx)); // Resource hooks it('_resourceWillParse', () => plugin._resourceWillParse(emptyCtx)); it('_resourceWillFind', () => plugin._resourceWillFind(emptyCtx)); it('_resourceWillExecute', () => plugin._resourceWillExecute(emptyCtx)); it('_resourceDidExecute', () => plugin._resourceDidExecute(emptyCtx)); - it('_resourceDidFinalize', () => plugin._resourceDidFinalize(emptyCtx)); + it('_resourceFinallyFinalize', () => plugin._resourceFinallyFinalize(emptyCtx)); // Prompt hooks it('_promptWillParse', () => plugin._promptWillParse(emptyCtx)); it('_promptWillFind', () => plugin._promptWillFind(emptyCtx)); it('_promptWillExecute', () => plugin._promptWillExecute(emptyCtx)); it('_promptDidExecute', () => plugin._promptDidExecute(emptyCtx)); - it('_promptDidFinalize', () => plugin._promptDidFinalize(emptyCtx)); + it('_promptFinallyFinalize', () => plugin._promptFinallyFinalize(emptyCtx)); // Agent hooks it('_agentWillParse', () => plugin._agentWillParse(emptyCtx)); it('_agentWillFind', () => plugin._agentWillFind(emptyCtx)); it('_agentWillExecute', () => plugin._agentWillExecute(emptyCtx)); it('_agentDidExecute', () => plugin._agentDidExecute(emptyCtx)); - it('_agentDidFinalize', () => plugin._agentDidFinalize(emptyCtx)); + it('_agentFinallyFinalize', () => plugin._agentFinallyFinalize(emptyCtx)); // List hooks it('_listToolsWill', () => plugin._listToolsWill(emptyCtx)); @@ -132,7 +134,7 @@ describe('ObservabilityPlugin', () => { it('_sseDidInit', () => plugin._sseDidInit(emptyCtx)); it('_sseDidMsg', () => plugin._sseDidMsg(emptyCtx)); it('_sseDidElicit', () => plugin._sseDidElicit(emptyCtx)); - it('_sseDidCleanup', () => plugin._sseDidCleanup(emptyCtx)); + it('_sseFinallyCleanup', () => plugin._sseFinallyCleanup(emptyCtx)); // Transport: Streamable HTTP it('_shttpWillParse', () => plugin._shttpWillParse(emptyCtx)); @@ -142,12 +144,12 @@ describe('ObservabilityPlugin', () => { it('_shttpDidElicit', () => plugin._shttpDidElicit(emptyCtx)); it('_shttpDidSse', () => plugin._shttpDidSse(emptyCtx)); it('_shttpDidExtApps', () => plugin._shttpDidExtApps(emptyCtx)); - it('_shttpDidCleanup', () => plugin._shttpDidCleanup(emptyCtx)); + it('_shttpFinallyCleanup', () => plugin._shttpFinallyCleanup(emptyCtx)); // Transport: Stateless HTTP it('_slhttpWillParse', () => plugin._slhttpWillParse(emptyCtx)); it('_slhttpDidHandle', () => plugin._slhttpDidHandle(emptyCtx)); - it('_slhttpDidCleanup', () => plugin._slhttpDidCleanup(emptyCtx)); + it('_slhttpFinallyCleanup', () => plugin._slhttpFinallyCleanup(emptyCtx)); // Auth it('_authVerifyWill', () => plugin._authVerifyWill(emptyCtx)); @@ -232,7 +234,7 @@ describe('ObservabilityPlugin', () => { plugin._httpDidAuth(noop); plugin._httpWillRoute(noop); plugin._httpDidRoute(noop); - plugin._httpDidFinalize(noop); + plugin._httpFinallyFinalize(noop); // Tool plugin._toolWillParse(noop); @@ -244,28 +246,28 @@ describe('ObservabilityPlugin', () => { plugin._toolDidExecute(noop); plugin._toolWillValidateOut(noop); plugin._toolWillApplyUI(noop); - plugin._toolDidFinalize(noop); + plugin._toolFinallyFinalize(noop); // Resource plugin._resourceWillParse(noop); plugin._resourceWillFind(noop); plugin._resourceWillExecute(noop); plugin._resourceDidExecute(noop); - plugin._resourceDidFinalize(noop); + plugin._resourceFinallyFinalize(noop); // Prompt plugin._promptWillParse(noop); plugin._promptWillFind(noop); plugin._promptWillExecute(noop); plugin._promptDidExecute(noop); - plugin._promptDidFinalize(noop); + plugin._promptFinallyFinalize(noop); // Agent plugin._agentWillParse(noop); plugin._agentWillFind(noop); plugin._agentWillExecute(noop); plugin._agentDidExecute(noop); - plugin._agentDidFinalize(noop); + plugin._agentFinallyFinalize(noop); // Lists plugin._listToolsWill(noop); @@ -295,7 +297,7 @@ describe('ObservabilityPlugin', () => { plugin._sseDidInit(noop); plugin._sseDidMsg(noop); plugin._sseDidElicit(noop); - plugin._sseDidCleanup(noop); + plugin._sseFinallyCleanup(noop); // Transport: Streamable HTTP plugin._shttpWillParse(noop); @@ -305,12 +307,12 @@ describe('ObservabilityPlugin', () => { plugin._shttpDidElicit(noop); plugin._shttpDidSse(noop); plugin._shttpDidExtApps(noop); - plugin._shttpDidCleanup(noop); + plugin._shttpFinallyCleanup(noop); // Transport: Stateless HTTP plugin._slhttpWillParse(noop); plugin._slhttpDidHandle(noop); - plugin._slhttpDidCleanup(noop); + plugin._slhttpFinallyCleanup(noop); // Auth plugin._authVerifyWill(noop); @@ -415,4 +417,14 @@ describe('ObservabilityPlugin', () => { expect(providers).toHaveLength(0); }); }); + + describe('hooks on the finalize stages', () => { + it('are Finally hooks, which still run when a stage failed, so the spans they end never stay open', () => { + const hooks = Reflect.getMetadata(FrontMcpFlowHookTokens.hooks, ObservabilityPlugin) as HookMetadata[]; + const finalizeStageHooks = hooks.filter((hook) => hook.stage === 'finalize' || hook.stage === 'cleanup'); + + expect(finalizeStageHooks.length).toBeGreaterThan(0); + expect(finalizeStageHooks.filter((hook) => hook.type !== 'finally').map((hook) => hook.method)).toEqual([]); + }); + }); }); diff --git a/libs/observability/src/__tests__/server-telemetry.spec.ts b/libs/observability/src/__tests__/server-telemetry.spec.ts index a4ad5539d..11b860290 100644 --- a/libs/observability/src/__tests__/server-telemetry.spec.ts +++ b/libs/observability/src/__tests__/server-telemetry.spec.ts @@ -22,6 +22,7 @@ import { Tool, ToolContext, ToolHook, + z, type FlowCtxOf, } from '@frontmcp/sdk'; @@ -59,6 +60,13 @@ class ExportReport extends ToolContext { } } +@Tool({ name: 'total_invoice', inputSchema: {}, outputSchema: { total: z.number() } }) +class TotalInvoice extends ToolContext { + async execute() { + return { total: 'twelve' } as unknown as { total: number }; + } +} + @Plugin({ name: 'audit' }) class AuditPlugin { @ToolHook.Did('execute') @@ -67,7 +75,7 @@ class AuditPlugin { } } -@App({ id: 'desk', name: 'Desk', tools: [CloseTicket, Ping, ExportReport] }) +@App({ id: 'desk', name: 'Desk', tools: [CloseTicket, Ping, ExportReport, TotalInvoice] }) class DeskApp {} function toolCall(name: string): Request { @@ -163,6 +171,26 @@ describe('a call that a hook failed with a plain Error', () => { }); }); +describe('a call whose answer the output schema refuses', () => { + it('ends the tools/call span and logs the refusal with the error ID the client gets', async () => { + const logs: RequestLog[] = []; + const handler = await serve({}, logs); + + const { result } = await (await handler(toolCall('total_invoice'))).json(); + await settle(); + + const { code, errorId } = result._meta; + expect(code).toBe('INVALID_OUTPUT'); + expect(span('tools/call').status.code).toBe(SpanStatusCode.ERROR); + expect(span('tools/call').events.find((event) => event.name === 'exception')?.attributes).toEqual( + expect.objectContaining({ 'exception.type': 'INVALID_OUTPUT' }), + ); + expect(logs[0].error).toEqual( + expect.objectContaining({ type: 'InvalidOutputError', code: 'INVALID_OUTPUT', error_id: errorId }), + ); + }); +}); + describe('the spans of one request', () => { it('nests the tools/call span in the POST / span, and this.fetch() sends the GET span as the parent', async () => { const handler = await serve(); @@ -222,7 +250,7 @@ describe('startupReport', () => { await serve(); expect(span('frontmcp.startup').attributes).toEqual( - expect.objectContaining({ 'frontmcp.startup.tools_count': 3, 'frontmcp.startup.plugins_count': 2 }), + expect.objectContaining({ 'frontmcp.startup.tools_count': 4, 'frontmcp.startup.plugins_count': 2 }), ); }); diff --git a/libs/observability/src/plugin/observability.plugin.ts b/libs/observability/src/plugin/observability.plugin.ts index 5cef9a1bf..4caf07a88 100644 --- a/libs/observability/src/plugin/observability.plugin.ts +++ b/libs/observability/src/plugin/observability.plugin.ts @@ -296,8 +296,8 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onHttpDidRoute(ctx); } - @HttpHook.Did('finalize', { priority: 1000 }) - _httpDidFinalize(ctx: unknown): void { + @HttpHook.Finally('finalize', { priority: 1000 }) + _httpFinallyFinalize(ctx: unknown): void { if (this.tracingEnabled) onHttpDidFinalize(ctx); // Delivered off the response path: a slow onRequestComplete must not hold the response if (this.requestLogsEnabled) void completeRequestLog(ctx as never); @@ -367,8 +367,8 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onToolWillApplyUI(ctx); } - @ToolHook.Did('finalize', { priority: 1000 }) - _toolDidFinalize(ctx: unknown): void { + @ToolHook.Finally('finalize', { priority: 1000 }) + _toolFinallyFinalize(ctx: unknown): void { if (this.tracingEnabled) onToolDidFinalize(ctx); if (this.requestLogsEnabled) recordRequestLogFailure(ctx as never); } @@ -409,8 +409,8 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onResourceDidExecute(ctx); } - @ResourceHook.Did('finalize', { priority: 1000 }) - _resourceDidFinalize(ctx: unknown): void { + @ResourceHook.Finally('finalize', { priority: 1000 }) + _resourceFinallyFinalize(ctx: unknown): void { if (this.tracingEnabled) onResourceDidFinalize(ctx); if (this.requestLogsEnabled) recordRequestLogFailure(ctx as never); } @@ -451,8 +451,8 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onPromptDidExecute(ctx); } - @PromptHook.Did('finalize', { priority: 1000 }) - _promptDidFinalize(ctx: unknown): void { + @PromptHook.Finally('finalize', { priority: 1000 }) + _promptFinallyFinalize(ctx: unknown): void { if (this.tracingEnabled) onPromptDidFinalize(ctx); if (this.requestLogsEnabled) recordRequestLogFailure(ctx as never); } @@ -492,8 +492,8 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onAgentDidExecute(ctx); } - @AgentCallHook.Did('finalize', { priority: 1000 }) - _agentDidFinalize(ctx: unknown): void { + @AgentCallHook.Finally('finalize', { priority: 1000 }) + _agentFinallyFinalize(ctx: unknown): void { if (this.tracingEnabled) onAgentDidFinalize(ctx); } @@ -575,7 +575,7 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onGenericFlowWillStart('skills/search', this.tracingOpts, ctx); } - @SkillSearchHook.Did('finalize', { priority: 1000 }) + @SkillSearchHook.Finally('finalize', { priority: 1000 }) _skillSearchDone(ctx: unknown): void { if (this.tracingEnabled) onGenericFlowDidFinalize(ctx); } @@ -590,7 +590,7 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onGenericFlowStage('loadSkills', ctx); } - @SkillLoadHook.Did('finalize', { priority: 1000 }) + @SkillLoadHook.Finally('finalize', { priority: 1000 }) _skillLoadDone(ctx: unknown): void { if (this.tracingEnabled) onGenericFlowDidFinalize(ctx); } @@ -604,7 +604,7 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onGenericFlowWillStart('completion/complete', this.tracingOpts, ctx); } - @CompletionHook.Did('finalize', { priority: 1000 }) + @CompletionHook.Finally('finalize', { priority: 1000 }) _completionDone(ctx: unknown): void { if (this.tracingEnabled) onGenericFlowDidFinalize(ctx); } @@ -638,8 +638,8 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onTransportStage('onElicitResult', ctx); } - @LegacySseHook.Did('cleanup', { priority: 1000 }) - _sseDidCleanup(ctx: unknown): void { + @LegacySseHook.Finally('cleanup', { priority: 1000 }) + _sseFinallyCleanup(ctx: unknown): void { if (this.tracingEnabled) onTransportDidFinalize(ctx); } @@ -682,8 +682,8 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onTransportStage('onExtApps', ctx); } - @StreamableHttpHook.Did('cleanup', { priority: 1000 }) - _shttpDidCleanup(ctx: unknown): void { + @StreamableHttpHook.Finally('cleanup', { priority: 1000 }) + _shttpFinallyCleanup(ctx: unknown): void { if (this.tracingEnabled) onTransportDidFinalize(ctx); } @@ -701,8 +701,8 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onTransportStage('handleRequest', ctx); } - @StatelessHttpHook.Did('cleanup', { priority: 1000 }) - _slhttpDidCleanup(ctx: unknown): void { + @StatelessHttpHook.Finally('cleanup', { priority: 1000 }) + _slhttpFinallyCleanup(ctx: unknown): void { if (this.tracingEnabled) onTransportDidFinalize(ctx); } @@ -812,7 +812,7 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.traces('elicitationSpans')) onGenericFlowWillStart('elicitation/request', this.tracingOpts, ctx); } - @ElicitRequestHook.Did('finalize', { priority: 1000 }) + @ElicitRequestHook.Finally('finalize', { priority: 1000 }) _elicitReqDone(ctx: unknown): void { if (this.traces('elicitationSpans')) onGenericFlowDidFinalize(ctx); } @@ -822,7 +822,7 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.traces('elicitationSpans')) onGenericFlowWillStart('elicitation/result', this.tracingOpts, ctx); } - @ElicitResultHook.Did('finalize', { priority: 1000 }) + @ElicitResultHook.Finally('finalize', { priority: 1000 }) _elicitResDone(ctx: unknown): void { if (this.traces('elicitationSpans')) onGenericFlowDidFinalize(ctx); } @@ -836,7 +836,7 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onGenericFlowWillStart('resources/subscribe', this.tracingOpts, ctx); } - @SubscribeResourceHook.Did('finalize', { priority: 1000 }) + @SubscribeResourceHook.Finally('finalize', { priority: 1000 }) _subscribeDone(ctx: unknown): void { if (this.tracingEnabled) onGenericFlowDidFinalize(ctx); } @@ -846,7 +846,7 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onGenericFlowWillStart('resources/unsubscribe', this.tracingOpts, ctx); } - @UnsubscribeResourceHook.Did('finalize', { priority: 1000 }) + @UnsubscribeResourceHook.Finally('finalize', { priority: 1000 }) _unsubscribeDone(ctx: unknown): void { if (this.tracingEnabled) onGenericFlowDidFinalize(ctx); } @@ -928,7 +928,7 @@ export default class ObservabilityPlugin extends DynamicPlugin< if (this.tracingEnabled) onGenericFlowWillStart('logging/set-level', this.tracingOpts, ctx); } - @LoggingSetLevelHook.Did('finalize', { priority: 1000 }) + @LoggingSetLevelHook.Finally('finalize', { priority: 1000 }) _setLevelDone(ctx: unknown): void { if (this.tracingEnabled) onGenericFlowDidFinalize(ctx); } diff --git a/libs/sdk/src/common/decorators/hook.decorator.ts b/libs/sdk/src/common/decorators/hook.decorator.ts index 5766dd238..0b529cdea 100644 --- a/libs/sdk/src/common/decorators/hook.decorator.ts +++ b/libs/sdk/src/common/decorators/hook.decorator.ts @@ -151,7 +151,7 @@ export function WillHookOf(flow: Name) { } /** - * Creates a typed Did hook decorator (runs after stage) + * Creates a typed Did hook decorator (runs after the stage succeeds, and not when it fails) */ export function DidHookOf(flow: Name) { type T = ExtendFlows[Name]['stage']; @@ -174,11 +174,23 @@ export function AroundHookOf(name: Name) { }; } +/** + * Creates a typed Finally hook decorator (runs after the stage whatever its outcome; cannot change it) + */ +export function FinallyHookOf(name: Name) { + type T = ExtendFlows[Name]['stage']; + type Ctx = ExtendFlows[Name]['ctx']; + const base = make(name, 'finally'); + return function (stage: T, opts: HookOptions = {}) { + return base(stage, opts); + }; +} + /** * Creates all hook decorators for a specific flow * @example * ```typescript - * const { Stage, Will, Did, Around } = FlowHooksOf('http:request'); + * const { Stage, Will, Did, Around, Finally } = FlowHooksOf('http:request'); * * class HttpRequestFlow { * @Stage('checkAuthorization') @@ -195,5 +207,6 @@ export function FlowHooksOf(name: Name) { Will: WillHookOf(name), Did: DidHookOf(name), Around: AroundHookOf(name), + Finally: FinallyHookOf(name), }; } diff --git a/libs/sdk/src/common/interfaces/internal/flow.utils.ts b/libs/sdk/src/common/interfaces/internal/flow.utils.ts index 037ada969..9df6e5a40 100644 --- a/libs/sdk/src/common/interfaces/internal/flow.utils.ts +++ b/libs/sdk/src/common/interfaces/internal/flow.utils.ts @@ -82,6 +82,9 @@ export class FlowState { (data as any)[prop] = value; return true; }, + deleteProperty(_t, prop: PropertyKey) { + return Reflect.deleteProperty(data as object, prop); + }, has(_t, prop) { return prop === 'required' || methodKeys.has(prop) || prop in (data as object); }, diff --git a/libs/sdk/src/common/metadata/hook.metadata.ts b/libs/sdk/src/common/metadata/hook.metadata.ts index 48517755c..c4af96371 100644 --- a/libs/sdk/src/common/metadata/hook.metadata.ts +++ b/libs/sdk/src/common/metadata/hook.metadata.ts @@ -4,7 +4,8 @@ import { type EntryOwnerRef } from '../entries/base.entry'; import { type ProviderRegistryInterface } from '../interfaces/internal'; import { type FlowName } from './flow.metadata'; -export type HookStageType = 'stage' | 'will' | 'did' | 'around'; +/** A `did` hook runs only when its stage succeeds; a `finally` hook runs on every outcome and cannot change it. */ +export type HookStageType = 'stage' | 'will' | 'did' | 'around' | 'finally'; export type HookPriority = number; /** diff --git a/libs/sdk/src/flows/__tests__/flow-error-state.spec.ts b/libs/sdk/src/flows/__tests__/flow-error-state.spec.ts index b2ff7c5ca..e4624ce94 100644 --- a/libs/sdk/src/flows/__tests__/flow-error-state.spec.ts +++ b/libs/sdk/src/flows/__tests__/flow-error-state.spec.ts @@ -1,15 +1,21 @@ /** * A flow that fails records why in `state.flowError` before its error and finalize stages run, so a * finalize hook can tell a failed call from a successful one (#766: the observability plugin's - * finalize hook ended a failed tool's span as a success and left its execution span open). + * finalize hook ended a failed tool's span as a success and left its execution span open). A failure + * of the finalize stage itself, such as an output schema refusing the answer, is recorded too, for + * the Finally hooks of that stage. */ import 'reflect-metadata'; +import { z } from '@frontmcp/lazy-zod'; + import { App, FlowHooksOf, Plugin, Tool, ToolContext } from '../../common'; +import { InvalidOutputError } from '../../errors'; import { FrontMcpInstance } from '../../front-mcp/front-mcp'; const ToolHook = FlowHooksOf('tools:call-tool'); const seen: unknown[] = []; +const seenFinally: unknown[] = []; @Plugin({ name: 'flow-error-probe' }) class FlowErrorProbe { @@ -17,6 +23,11 @@ class FlowErrorProbe { record(flowCtx: { state: { flowError?: unknown } }) { seen.push(flowCtx.state.flowError); } + + @ToolHook.Finally('finalize') + recordAlways(flowCtx: { state: { flowError?: unknown } }) { + seenFinally.push(flowCtx.state.flowError); + } } @Tool({ name: 'crash', inputSchema: {} }) @@ -33,7 +44,14 @@ class OkTool extends ToolContext { } } -@App({ id: 'desk', name: 'Desk', tools: [CrashTool, OkTool], plugins: [FlowErrorProbe] }) +@Tool({ name: 'refused', inputSchema: {}, outputSchema: { total: z.number() } }) +class RefusedTool extends ToolContext { + async execute() { + return { total: 'twelve' } as unknown as { total: number }; + } +} + +@App({ id: 'desk', name: 'Desk', tools: [CrashTool, OkTool, RefusedTool], plugins: [FlowErrorProbe] }) class DeskApp {} @Plugin({ name: 'zero-thrower' }) @@ -47,7 +65,53 @@ class ZeroThrower { @App({ id: 'zero', name: 'Zero', tools: [OkTool], plugins: [ZeroThrower, FlowErrorProbe] }) class ZeroApp {} +@Plugin({ name: 'unprintable-thrower' }) +class UnprintableThrower { + @ToolHook.Will('execute') + throwUnprintable() { + throw Object.create(null); + } +} + +@App({ id: 'unprintable', name: 'Unprintable', tools: [OkTool], plugins: [UnprintableThrower, FlowErrorProbe] }) +class UnprintableApp {} + +const probeSeen: boolean[] = []; + +@Plugin({ name: 'state-probe' }) +class StateProbe { + @ToolHook.Finally('finalize', { priority: -1 }) + markThenThrow(flowCtx: { state: { set(key: never, value: never): unknown } }) { + flowCtx.state.set('probe' as never, true as never); + throw new Error('the probe store is down'); + } + + @ToolHook.Finally('finalize', { priority: 1 }) + lookForMark(flowCtx: { state: object }) { + probeSeen.push('probe' in flowCtx.state); + } +} + +@App({ id: 'probed', name: 'Probed', tools: [OkTool], plugins: [StateProbe] }) +class ProbedApp {} + +@Plugin({ name: 'finalize-breaker' }) +class FinalizeBreaker { + @ToolHook.Will('finalize') + breakFinalize() { + throw new Error('finalize broke'); + } +} + +@App({ id: 'broken', name: 'Broken', tools: [CrashTool], plugins: [FinalizeBreaker, FlowErrorProbe] }) +class BrokenFinalizeApp {} + describe('state.flowError', () => { + beforeEach(() => { + seen.length = 0; + seenFinally.length = 0; + }); + it('is the error a failed call threw, and unset for a successful one', async () => { const server = await FrontMcpInstance.createDirect({ info: { name: 'desk', version: '1.0.0' }, apps: [DeskApp] }); @@ -61,7 +125,6 @@ describe('state.flowError', () => { }); it('is an Error even when a stage threw a falsy value', async () => { - seen.length = 0; const zeroServer = await FrontMcpInstance.createDirect({ info: { name: 'zero', version: '1.0.0' }, apps: [ZeroApp], @@ -72,4 +135,51 @@ describe('state.flowError', () => { expect(seen).toHaveLength(1); expect(seen[0]).toBeInstanceOf(Error); }); + + it('is an Error, and the Finally hooks still run, when a stage threw a value String() cannot convert', async () => { + const server = await FrontMcpInstance.createDirect({ + info: { name: 'unprintable', version: '1.0.0' }, + apps: [UnprintableApp], + }); + await server.callTool('ok', {}).catch(() => undefined); + await server.dispose(); + + expect(seenFinally).toHaveLength(1); + expect(String(seenFinally[0])).toContain('[object Object]'); + }); + + it('loses a key a Finally hook added before a throw the runner dropped', async () => { + const server = await FrontMcpInstance.createDirect({ + info: { name: 'probed', version: '1.0.0' }, + apps: [ProbedApp], + }); + await server.callTool('ok', {}); + await server.dispose(); + + expect(probeSeen).toEqual([false]); + }); + + it("is the output schema's refusal of the answer for the Finally hooks of finalize, whose Did hooks skip it", async () => { + const server = await FrontMcpInstance.createDirect({ info: { name: 'desk', version: '1.0.0' }, apps: [DeskApp] }); + + await server.callTool('refused', {}).catch(() => undefined); + await server.dispose(); + + expect(seen).toEqual([]); + expect(seenFinally).toEqual([expect.any(InvalidOutputError)]); + }); + + it('keeps the first failure, the one the client gets, when the finalize stage fails after it', async () => { + const server = await FrontMcpInstance.createDirect({ + info: { name: 'broken', version: '1.0.0' }, + apps: [BrokenFinalizeApp], + }); + + const failure = await server.callTool('crash', {}).catch((error: unknown) => error); + await server.dispose(); + + expect(String(failure)).toContain('tool crashed'); + expect(seenFinally).toHaveLength(1); + expect(String(seenFinally[0])).toContain('tool crashed'); + }); }); diff --git a/libs/sdk/src/flows/__tests__/flow-stage-hook-lists.spec.ts b/libs/sdk/src/flows/__tests__/flow-stage-hook-lists.spec.ts index 1e665d80e..cde4ffb04 100644 --- a/libs/sdk/src/flows/__tests__/flow-stage-hook-lists.spec.ts +++ b/libs/sdk/src/flows/__tests__/flow-stage-hook-lists.spec.ts @@ -1,6 +1,6 @@ /** - * A stage runs its will and did hooks only when it has some, and reads each list when the stage starts, so hooks a - * run adds (the tool class's own, joined at `createToolCallContext`) still run on stages that had none before. + * A stage runs its will, did and finally hooks only when it has some, and reads each list when the stage starts, so + * hooks a run adds (the tool class's own, joined at `createToolCallContext`) still run on stages that had none before. */ import 'reflect-metadata'; @@ -25,6 +25,11 @@ class StageTracePlugin { await next(); trace.push('plugin around validateInput: after'); } + + @ToolHook.Finally('validateInput') + finallyValidateInput() { + trace.push('plugin finally validateInput'); + } } @Tool({ name: 'echo', inputSchema: {} }) @@ -44,6 +49,11 @@ class EchoTool extends ToolContext { trace.push('tool did validateOutput'); } + @ToolHook.Finally('validateOutput') + finallyValidateOutput() { + trace.push('tool finally validateOutput'); + } + @ToolHook.Will('finalize') willFinalize() { trace.push('tool will finalize'); @@ -81,8 +91,10 @@ describe('flow stage hook lists', () => { 'tool will checkToolCredentials', 'plugin around validateInput: before', 'plugin around validateInput: after', + 'plugin finally validateInput', 'execute', 'tool did validateOutput', + 'tool finally validateOutput', 'tool will finalize', ]); } diff --git a/libs/sdk/src/flows/__tests__/flow-typed-keys.spec.ts b/libs/sdk/src/flows/__tests__/flow-typed-keys.spec.ts new file mode 100644 index 000000000..06faacc93 --- /dev/null +++ b/libs/sdk/src/flows/__tests__/flow-typed-keys.spec.ts @@ -0,0 +1,26 @@ +import { parseTypedKey, stageBasesOf } from '../flow.instance'; + +describe('parseTypedKey', () => { + it.each([ + ['willExecute', 'will'], + ['didExecute', 'did'], + ['aroundExecute', 'around'], + ['finallyExecute', 'finally'], + ] as const)('reads %s as a %s hook key of the execute stage, never as a stage of its own', (key, type) => { + expect(parseTypedKey(key)).toEqual({ base: 'execute', type }); + }); + + it.each(['execute', 'finalize', 'finally', 'will'])('reads %s as a plain stage name', (key) => { + expect(parseTypedKey(key)).toEqual({ base: key }); + }); +}); + +describe('stageBasesOf', () => { + it('runs a typed key with its base stage, never as a stage of its own', () => { + expect(stageBasesOf(['execute', 'finallyExecute', 'willExecute'])).toEqual(['execute']); + }); + + it('keeps a stage named finally a stage when the group lists no stage, as before Finally hooks', () => { + expect(stageBasesOf(['parse', 'finallyCheck'])).toEqual(['parse', 'finallyCheck']); + }); +}); diff --git a/libs/sdk/src/flows/flow.instance.ts b/libs/sdk/src/flows/flow.instance.ts index 6995b1ecc..3c2c4834a 100644 --- a/libs/sdk/src/flows/flow.instance.ts +++ b/libs/sdk/src/flows/flow.instance.ts @@ -68,6 +68,7 @@ interface StageKeys { will: string; did: string; around: string; + finally: string; } // Stage names come from flow plans, so this stays as small as the set of stages. @@ -76,34 +77,59 @@ const stageKeysByName = new Map(); function stageKeysOf(stage: string): StageKeys { let keys = stageKeysByName.get(stage); if (!keys) { - keys = { will: `will${CAP(stage)}`, did: `did${CAP(stage)}`, around: `around${CAP(stage)}` }; + keys = { + will: `will${CAP(stage)}`, + did: `did${CAP(stage)}`, + around: `around${CAP(stage)}`, + finally: `finally${CAP(stage)}`, + }; stageKeysByName.set(stage, keys); } return keys; } -function parseTypedKey(k: string): { base: string; type?: 'will' | 'did' | 'around' } { +export function parseTypedKey(k: string): { base: string; type?: 'will' | 'did' | 'around' | 'finally' } { const lc = k.toLowerCase(); if (lc.startsWith('will') && k.length > 4) return { base: k.slice(4, 5).toLowerCase() + k.slice(5), type: 'will' }; if (lc.startsWith('did') && k.length > 3) return { base: k.slice(3, 4).toLowerCase() + k.slice(4), type: 'did' }; if (lc.startsWith('around') && k.length > 6) return { base: k.slice(6, 7).toLowerCase() + k.slice(7), type: 'around' }; + if (lc.startsWith('finally') && k.length > 7) + return { base: k.slice(7, 8).toLowerCase() + k.slice(8), type: 'finally' }; return { base: k }; } +/** `String(value)`, or its `Object.prototype.toString` tag for a value that cannot be converted (a null-prototype object). */ +function describeValue(value: unknown): string { + try { + return String(value); + } catch { + return Object.prototype.toString.call(value); + } +} + +/** The error a fail() carried, or an Error naming why the flow was aborted. */ +function failureOf(control: FlowControl): unknown { + const originalError = (control as { originalError?: unknown }).originalError; + if (originalError !== undefined) return originalError; + return control.type === 'abort' ? new InternalMcpError(`Flow aborted: ${String(control.output)}`) : control; +} + const stageBasesByGroup = new WeakMap(); /** The base stages a plan group runs, in order: a typed key runs with its base, and a repeated base runs once. */ -function stageBasesOf(group: readonly string[]): string[] { +export function stageBasesOf(group: readonly string[]): string[] { let bases = stageBasesByGroup.get(group); if (!bases) { const seen = new Set(); bases = []; for (const rawKey of group) { const { base, type } = parseTypedKey(rawKey); - if (type || seen.has(base)) continue; - seen.add(base); - bases.push(base); + // A stage named `finallyCheck` predates Finally hooks: it is `check`'s hook key only when the group lists `check` + const stage = type === 'finally' && !group.includes(base) ? rawKey : base; + if ((type && stage === base) || seen.has(stage)) continue; + seen.add(stage); + bases.push(stage); } stageBasesByGroup.set(group, bases); } @@ -500,8 +526,29 @@ export class FlowInstance extends FlowEntry { // Whether any stage body or hook is registered under `key` right now. const hasEntries = (key: string): boolean => (stages[key]?.length ?? 0) > 0; + // A Finally hook cannot respond or fail its stage: what it throws is logged and dropped. + const ignoreFinallyThrow = (key: string, thrown: unknown) => { + const description = thrown instanceof FlowControl ? `FlowControl '${thrown.type}'` : describeValue(thrown); + this.logger.warn(`Ignored a throw from a Finally hook of '${key}': it cannot change the stage's outcome`, { + flow: name, + thrown: description, + }); + }; + + // The values a Finally hook set on the state before the throw the runner drops, such as a job's answer, are put back + const restoreState = (before: Readonly>) => { + const state = context.state as unknown as Record; + for (const key of Object.keys(context.state.snapshot())) { + if (!(key in before)) delete state[key]; + } + context.state.set(before as never); + }; + // Robust list runner (doesn't skip mid-run insertions) - const runList = async (key: string, opts?: { ignoreRespond?: boolean }): Promise => { + const runList = async ( + key: string, + opts?: { ignoreRespond?: boolean; ignoreThrows?: boolean }, + ): Promise => { const seen = new Set(); while (true) { @@ -516,9 +563,15 @@ export class FlowInstance extends FlowEntry { if (!item) break; seen.add(item); + const stateBefore = opts?.ignoreThrows ? (context.state.snapshot() as Record) : undefined; try { await item.method(context); } catch (e: any) { + if (opts?.ignoreThrows) { + if (stateBefore) restoreState(stateBefore); + ignoreFinallyThrow(key, e); + continue; + } if (e instanceof FlowControl && e.type === 'respond' && opts?.ignoreRespond) continue; return toStageResult(e); } @@ -548,9 +601,20 @@ export class FlowInstance extends FlowEntry { return FAILED_OUTCOMES.has(innerResult.outcome) ? { outcome: 'ok' } : innerResult; }; + // `state.flowError` keeps the run's first failure (a thrown Error, a `fail()` or an abort) for what runs after it + const recordFailure = (control: unknown) => { + if (context.state.get('flowError' as never)) return; + const failure = control instanceof FlowControl ? failureOf(control) : control; + const flowError = + failure instanceof Error + ? failure + : new InternalMcpError(`Flow failed with a non-Error value: ${describeValue(failure)}`); + context.state.set('flowError' as never, flowError as never); + }; + // Run exactly one stage in order: will → around(stage) → did (did runs once) // An empty hook list is skipped without a call: running it would only return 'ok'. - const runOneStage = async (stageName: string, stopOnRespond: boolean): Promise => { + const runStageSteps = async (stageName: string, stopOnRespond: boolean): Promise => { const keys = stageKeysOf(stageName); // 1) willStage if (hasEntries(keys.will)) { @@ -578,6 +642,15 @@ export class FlowInstance extends FlowEntry { return { outcome: 'ok' }; }; + // Then the stage's Finally hooks run, whatever its outcome, with its failure recorded for them. + const runOneStage = async (stageName: string, stopOnRespond: boolean): Promise => { + const result = await runStageSteps(stageName, stopOnRespond); + if (FAILED_OUTCOMES.has(result.outcome)) recordFailure(result.control); + const finallyKey = stageKeysOf(stageName).finally; + if (hasEntries(finallyKey)) await runList(finallyKey, { ignoreThrows: true }); + return result; + }; + // IMPORTANT: ignore typed stage names in plan arrays. // Only base stage names in the plan drive execution; typed hooks run *with* their base. const runStageGroup = async ( @@ -599,15 +672,6 @@ export class FlowInstance extends FlowEntry { return { outcome: 'ok' }; }; - // The error and finalize stages read why the flow failed from `state.flowError`, always an Error - const recordFailure = (control: unknown) => { - const flowError = - control instanceof Error - ? control - : new InternalMcpError(`Flow failed with a non-Error value: ${String(control)}`); - context.state.set('flowError' as never, flowError as never); - }; - const runErrorStage = async () => { await runStageGroup((plan as any).error, false, { ignoreRespond: true }); }; @@ -638,7 +702,6 @@ export class FlowInstance extends FlowEntry { if (pre.outcome === 'respond') { const post = await runStageGroup((plan as any).post, false); if (post.outcome === 'unknown_error' || post.outcome === 'fail') { - recordFailure(post.control); try { await runErrorStage(); } finally { @@ -657,7 +720,6 @@ export class FlowInstance extends FlowEntry { return responded; } if (pre.outcome === 'unknown_error' || pre.outcome === 'fail') { - recordFailure(pre.control); try { await runErrorStage(); } finally { @@ -681,7 +743,6 @@ export class FlowInstance extends FlowEntry { if (exec.outcome === 'respond') { // continue to post + finalize } else if (exec.outcome === 'unknown_error' || exec.outcome === 'fail') { - recordFailure(exec.control); try { await runErrorStage(); } finally { @@ -702,7 +763,6 @@ export class FlowInstance extends FlowEntry { const post = await runStageGroup((plan as any).post, false); this.logger.verbose(`run: POST completed, outcome=${post.outcome}`); if (post.outcome === 'unknown_error' || post.outcome === 'fail') { - recordFailure(post.control); try { await runErrorStage(); } finally { diff --git a/libs/sdk/src/flows/flow.stages.ts b/libs/sdk/src/flows/flow.stages.ts index 1e6f8e9c8..8a46635cf 100644 --- a/libs/sdk/src/flows/flow.stages.ts +++ b/libs/sdk/src/flows/flow.stages.ts @@ -33,13 +33,7 @@ export function cloneStageMap(table: StageMap): StageMap { } export function resolveStageKey(type: HookMetadata['type'], stage: string) { - return type === 'will' - ? `will${cap(stage)}` - : type === 'did' - ? `did${cap(stage)}` - : type === 'around' - ? `around${cap(stage)}` - : stage; + return type === 'stage' ? stage : `${type}${cap(stage)}`; } export function collectFlowHookMap(FlowClass: any): StageMap { diff --git a/libs/sdk/src/hooks/__tests__/flow-hooks.behaviour.spec.ts b/libs/sdk/src/hooks/__tests__/flow-hooks.behaviour.spec.ts index 8e385ac5c..d84abc03b 100644 --- a/libs/sdk/src/hooks/__tests__/flow-hooks.behaviour.spec.ts +++ b/libs/sdk/src/hooks/__tests__/flow-hooks.behaviour.spec.ts @@ -35,6 +35,16 @@ class TracePlugin { trace.push('Did p100'); } + @ToolHook.Finally('execute', { priority: 100 }) + finallyPriorityHundred() { + trace.push('Finally p100'); + } + + @ToolHook.Finally('execute', { priority: 0 }) + finallyPriorityZero() { + trace.push('Finally p0'); + } + @ToolHook.Around('execute') async aroundExecute(_ctx: FlowCtxOf<'tools:call-tool'>, next: () => Promise) { trace.push('Around: before next()'); @@ -184,6 +194,15 @@ describe('tools:call-tool flow hooks', () => { expect(deskTrace.filter((entry) => entry.startsWith('Did p'))).toEqual(['Did p0', 'Did p100']); }); + it('runs Finally hooks after the Did hooks, with a lower priority first', () => { + expect(deskTrace.filter((entry) => /^(Did|Finally) p/.test(entry))).toEqual([ + 'Did p0', + 'Did p100', + 'Finally p0', + 'Finally p100', + ]); + }); + it('runs a hook declared on the @Tool class', () => { expect(deskTrace).toContain('hook on @Tool class'); }); diff --git a/libs/sdk/src/hooks/__tests__/flow-hooks.finally.spec.ts b/libs/sdk/src/hooks/__tests__/flow-hooks.finally.spec.ts new file mode 100644 index 000000000..f09300c35 --- /dev/null +++ b/libs/sdk/src/hooks/__tests__/flow-hooks.finally.spec.ts @@ -0,0 +1,359 @@ +/** + * A stage's Finally hooks run after its Will hooks, its body and its Did hooks whatever the outcome, read why the + * stage failed from `state.flowError`, and never change what the client gets: what one throws or responds is + * logged and dropped. + */ +import 'reflect-metadata'; + +import { z } from '@frontmcp/lazy-zod'; + +import { + createTestFetchServer, + rpc20260728, + type TestFetchServer, +} from '../../__test-utils__/helpers/mcp-20260728.helpers'; +import { + App, + FlowControl, + FlowHooksOf, + LogLevel, + LogTransport, + LogTransportInterface, + Plugin, + Tool, + ToolContext, + type FlowCtxOf, + type LogRecord, +} from '../../common'; +import { PublicMcpError } from '../../errors'; + +const ToolHook = FlowHooksOf('tools:call-tool'); +type CallToolFlow = FlowCtxOf<'tools:call-tool'>; + +const trace: string[] = []; +const warnings: LogRecord[] = []; + +const calling = + (...names: string[]) => + (ctx: CallToolFlow) => + names.includes(String(ctx.state.input?.name)); +const outcomeOf = (ctx: CallToolFlow) => ctx.state.flowError?.message ?? 'ok'; + +@LogTransport({ name: 'FinallyWarningCapture', description: 'Captures the warnings a run logs' }) +class WarningCapture extends LogTransportInterface { + log(record: LogRecord): void { + if (record.level === LogLevel.Warn) warnings.push(record); + } +} + +function postingTool(name: string, answer: unknown = { posted: true }) { + @Tool({ name, inputSchema: {}, outputSchema: { posted: z.boolean() } }) + class PostingTool extends ToolContext { + async execute() { + return answer as { posted: boolean }; + } + } + return PostingTool; +} + +@Tool({ name: 'post_crash', inputSchema: {}, outputSchema: { posted: z.boolean() } }) +class PostCrashTool extends ToolContext { + async execute(): Promise<{ posted: boolean }> { + throw new PublicMcpError('ledger offline'); + } +} + +const PostEntryTool = postingTool('post_entry'); +const PostRefusedTool = postingTool('post_refused', { posted: 'yes' }); + +@Plugin({ name: 'ledger-finally' }) +class LedgerFinallyPlugin { + @ToolHook.Will('execute', { filter: calling('post_blocked') }) + refuse() { + throw new PublicMcpError('blocked by policy'); + } + + @ToolHook.Will('execute', { filter: calling('post_gated') }) + closeGate(ctx: CallToolFlow) { + ctx.fail(new PublicMcpError('gate closed')); + } + + @ToolHook.Will('execute', { filter: calling('post_aborted') }) + abortExecute() { + FlowControl.abort('maintenance window'); + } + + @ToolHook.Will('execute', { filter: calling('post_cached') }) + answerFromCache(ctx: CallToolFlow) { + ctx.respond({ content: [{ type: 'text', text: 'from cache' }] }); + } + + @ToolHook.Around('execute', { filter: calling('post_wrapped') }) + async breakAround() { + throw new PublicMcpError('around broke'); + } + + @ToolHook.Did('execute') + didExecute() { + trace.push('Did execute'); + } + + @ToolHook.Did('execute', { filter: calling('post_did_broke') }) + breakDidExecute() { + throw new PublicMcpError('did hook broke'); + } + + @ToolHook.Finally('execute', { priority: 100 }) + lastFinallyExecute(ctx: CallToolFlow) { + trace.push(`Finally p100 execute: ${outcomeOf(ctx)}`); + } + + @ToolHook.Finally('execute', { priority: 0 }) + firstFinallyExecute(ctx: CallToolFlow) { + trace.push(`Finally p0 execute: ${outcomeOf(ctx)}`); + } + + @ToolHook.Did('finalize') + didFinalize() { + trace.push('Did finalize'); + } + + @ToolHook.Finally('finalize') + finallyFinalize(ctx: CallToolFlow) { + trace.push(`Finally finalize: ${outcomeOf(ctx)}`); + } +} + +@App({ + id: 'ledger', + name: 'Ledger', + plugins: [LedgerFinallyPlugin], + tools: [ + PostEntryTool, + PostCrashTool, + PostRefusedTool, + postingTool('post_blocked'), + postingTool('post_gated'), + postingTool('post_cached'), + postingTool('post_wrapped'), + postingTool('post_did_broke'), + postingTool('post_aborted'), + ], +}) +class LedgerApp {} + +@Plugin({ name: 'unruly-finally' }) +class UnrulyFinallyPlugin { + @ToolHook.Finally('finalize', { priority: -10 }) + hijack(ctx: CallToolFlow) { + ctx.respond({ content: [{ type: 'text', text: 'hijacked' }] }); + } + + @ToolHook.Finally('finalize', { priority: -5 }) + explode() { + throw new Error('finally hook broke'); + } + + @ToolHook.Finally('finalize', { priority: -3 }) + throwUnprintable() { + throw Object.create(null); + } + + @ToolHook.Finally('finalize') + recordAfterUnruly(ctx: CallToolFlow) { + trace.push(`Finally after unruly hooks: ${outcomeOf(ctx)}`); + } +} + +@App({ + id: 'unruly', + name: 'Unruly', + plugins: [UnrulyFinallyPlugin], + tools: [PostEntryTool, PostCrashTool, PostRefusedTool], +}) +class UnrulyApp {} + +async function callTool(server: TestFetchServer, name: string) { + trace.length = 0; + const { message } = await rpc20260728(server.handler, 'tools/call', { name, arguments: {} }); + const result = message.result ?? {}; + const content = (result['content'] as Array<{ text?: string }> | undefined) ?? []; + const resultMeta = result['_meta'] as Record | undefined; + return { + isError: result['isError'] === true, + text: content.map((block) => block.text ?? '').join('\n'), + code: resultMeta?.['code'], + structuredContent: result['structuredContent'], + trace: [...trace], + }; +} + +describe('Finally hooks', () => { + let server: TestFetchServer; + + beforeAll(async () => { + server = await createTestFetchServer({ info: { name: 'finally-hooks', version: '1.0.0' }, apps: [LedgerApp] }); + }); + + it('run after the Did hooks of a stage that succeeded, lower priority first', async () => { + const call = await callTool(server, 'post_entry'); + + expect(call.structuredContent).toEqual({ posted: true }); + expect(call.trace).toEqual([ + 'Did execute', + 'Finally p0 execute: ok', + 'Finally p100 execute: ok', + 'Did finalize', + 'Finally finalize: ok', + ]); + }); + + it('run when the stage body fails, and read the failure from state.flowError', async () => { + const call = await callTool(server, 'post_crash'); + + expect({ isError: call.isError, text: call.text }).toEqual({ isError: true, text: 'ledger offline' }); + expect(call.trace).toEqual([ + 'Finally p0 execute: ledger offline', + 'Finally p100 execute: ledger offline', + 'Did finalize', + 'Finally finalize: ledger offline', + ]); + }); + + it('run when a Will hook fails the stage before its body runs', async () => { + const call = await callTool(server, 'post_blocked'); + + expect({ isError: call.isError, text: call.text }).toEqual({ isError: true, text: 'blocked by policy' }); + expect(call.trace).toEqual([ + 'Finally p0 execute: blocked by policy', + 'Finally p100 execute: blocked by policy', + 'Did finalize', + 'Finally finalize: blocked by policy', + ]); + }); + + it('read the error a Will hook passed to fail(), not the signal that carried it', async () => { + const call = await callTool(server, 'post_gated'); + + expect({ isError: call.isError, text: call.text }).toEqual({ isError: true, text: 'gate closed' }); + expect(call.trace).toEqual([ + 'Finally p0 execute: gate closed', + 'Finally p100 execute: gate closed', + 'Did finalize', + 'Finally finalize: gate closed', + ]); + }); + + it('read an abort as the failure, like a fail()', async () => { + const call = await callTool(server, 'post_aborted'); + + expect(call.trace.slice(0, 2)).toEqual([ + 'Finally p0 execute: Flow aborted: maintenance window', + 'Finally p100 execute: Flow aborted: maintenance window', + ]); + }); + + it('run when a Will hook responds in place of the stage', async () => { + const call = await callTool(server, 'post_cached'); + + expect({ isError: call.isError, text: call.text }).toEqual({ isError: false, text: 'from cache' }); + expect(call.trace).toEqual([ + 'Finally p0 execute: ok', + 'Finally p100 execute: ok', + 'Did finalize', + 'Finally finalize: ok', + ]); + }); + + it('run when an Around hook fails the stage', async () => { + const call = await callTool(server, 'post_wrapped'); + + expect({ isError: call.isError, text: call.text }).toEqual({ isError: true, text: 'around broke' }); + expect(call.trace).toEqual([ + 'Finally p0 execute: around broke', + 'Finally p100 execute: around broke', + 'Did finalize', + 'Finally finalize: around broke', + ]); + }); + + it('run when a Did hook fails a stage whose body succeeded, and read that failure, which the client gets', async () => { + const call = await callTool(server, 'post_did_broke'); + + expect({ isError: call.isError, text: call.text }).toEqual({ isError: true, text: 'did hook broke' }); + expect(call.trace).toEqual([ + 'Did execute', + 'Finally p0 execute: did hook broke', + 'Finally p100 execute: did hook broke', + 'Did finalize', + 'Finally finalize: did hook broke', + ]); + }); + + it('run on finalize when the output schema refuses the answer, while Did on finalize does not', async () => { + const call = await callTool(server, 'post_refused'); + + expect({ isError: call.isError, code: call.code }).toEqual({ isError: true, code: 'INVALID_OUTPUT' }); + expect(call.trace).toEqual([ + 'Did execute', + 'Finally p0 execute: ok', + 'Finally p100 execute: ok', + 'Finally finalize: Tool output validation failed (output does not match outputSchema at posted)', + ]); + }); +}); + +describe('a Finally hook that responds or throws', () => { + let server: TestFetchServer; + + beforeAll(async () => { + server = await createTestFetchServer({ + info: { name: 'unruly-finally-hooks', version: '1.0.0' }, + apps: [UnrulyApp], + logging: { level: LogLevel.Warn, enableConsole: false, transports: [WarningCapture] }, + }); + }); + + beforeEach(() => { + warnings.length = 0; + }); + + it('leaves a successful answer as it was, and the later Finally hooks still run', async () => { + const call = await callTool(server, 'post_entry'); + + expect({ isError: call.isError, structuredContent: call.structuredContent }).toEqual({ + isError: false, + structuredContent: { posted: true }, + }); + expect(call.trace).toEqual(['Finally after unruly hooks: ok']); + }); + + it('leaves the error of a failed call as it was', async () => { + const call = await callTool(server, 'post_crash'); + + expect({ isError: call.isError, text: call.text }).toEqual({ isError: true, text: 'ledger offline' }); + expect(call.trace).toEqual(['Finally after unruly hooks: ledger offline']); + }); + + it('leaves the refusal of an answer the output schema refuses as it was', async () => { + const call = await callTool(server, 'post_refused'); + + expect({ isError: call.isError, code: call.code }).toEqual({ isError: true, code: 'INVALID_OUTPUT' }); + expect(call.trace).toEqual([ + 'Finally after unruly hooks: Tool output validation failed (output does not match outputSchema at posted)', + ]); + }); + + it('logs what each of them threw as a warning naming the stage', async () => { + await callTool(server, 'post_entry'); + + const logged = warnings.map((record) => + [record.message, ...record.args].map((part) => JSON.stringify(part)).join(' '), + ); + expect(logged).toEqual([ + expect.stringMatching(/finallyFinalize.*respond/), + expect.stringMatching(/finallyFinalize.*finally hook broke/), + expect.stringMatching(/finallyFinalize.*\[object Object\]/), + ]); + }); +}); diff --git a/libs/sdk/src/job/__tests__/execute-job.flow.spec.ts b/libs/sdk/src/job/__tests__/execute-job.flow.spec.ts index a278607db..84eab17d5 100644 --- a/libs/sdk/src/job/__tests__/execute-job.flow.spec.ts +++ b/libs/sdk/src/job/__tests__/execute-job.flow.spec.ts @@ -87,6 +87,16 @@ class JobAuditPlugin { await next(); } + @JobHook.Finally('validateOutput') + overrideAnswer(flowCtx: JobFlowCtx) { + // A Finally hook's answer is ignored, so it cannot replace the job's result + if (flowCtx.state.job?.name === 'overridden_job') flowCtx.respond({ result: { count: 999 }, logs: [] }); + if (flowCtx.state.job?.name === 'replaced_job') { + flowCtx.state.set('answer', { result: { count: 999 }, logs: [] }); + throw new Error('the recorder is down'); + } + } + @JobHook.Will('finalize') finalizing(flowCtx: JobFlowCtx) { trace.push(at('will', 'finalize', flowCtx)); @@ -198,6 +208,20 @@ class FailingCompletionStore extends MemoryJobStateStore { } } +@Job({ name: 'overridden_job', inputSchema: {}, outputSchema: { count: z.number() } }) +class OverriddenJob extends JobContext { + async execute() { + return { count: 1 }; + } +} + +@Job({ name: 'replaced_job', inputSchema: {}, outputSchema: { count: z.number() } }) +class ReplacedJob extends JobContext { + async execute() { + return { count: 1 }; + } +} + @Job({ name: 'aborted_job', inputSchema: {}, outputSchema: {} }) class AbortedJob extends JobContext { async execute() { @@ -285,6 +309,8 @@ class WhoAmIFlow {} ThrowerJob, SendJob, AbortedJob, + OverriddenJob, + ReplacedJob, WhoAmIJob, HookedJob, ], @@ -510,18 +536,34 @@ describe('jobs:execute-job flow (#700)', () => { ); }); - it('records an attempt a hook ended without an answer as failed', async () => { + it('records an attempt a hook aborted as failed, with the reason it gave', async () => { await expect(manager.executeJob(jobNamed('aborted_job'), {})).rejects.toBeInstanceOf(FlowControl); const [run] = await manager.listRuns({ jobId: 'aborted_job' }); expect(run).toEqual( expect.objectContaining({ state: 'failed', - error: expect.objectContaining({ message: 'The job attempt ended before it produced a result' }), + error: expect.objectContaining({ message: 'Flow aborted: stopped by a hook' }), }), ); }); + it("keeps the job's result when a Finally hook answers in its place", async () => { + const result = await manager.executeJob(jobNamed('overridden_job'), {}); + + expect(result).toEqual(expect.objectContaining({ state: 'completed', result: { count: 1 } })); + const [run] = await manager.listRuns({ jobId: 'overridden_job' }); + expect(run).toEqual(expect.objectContaining({ state: 'completed', result: { count: 1 } })); + }); + + it("keeps the job's result when a Finally hook replaces it in state and then throws", async () => { + const result = await manager.executeJob(jobNamed('replaced_job'), {}); + + expect(result).toEqual(expect.objectContaining({ state: 'completed', result: { count: 1 } })); + const [run] = await manager.listRuns({ jobId: 'replaced_job' }); + expect(run).toEqual(expect.objectContaining({ state: 'completed', result: { count: 1 } })); + }); + it('builds the job with the providers a caller without a context passes', async () => { const result = await manager.executeJob(jobNamed('count_job'), { n: 7 }, { contextProviders: scope.providers }); diff --git a/libs/sdk/src/tool/flows/__tests__/call-tool.output-schema.spec.ts b/libs/sdk/src/tool/flows/__tests__/call-tool.output-schema.spec.ts index f03f8001d..979d805b4 100644 --- a/libs/sdk/src/tool/flows/__tests__/call-tool.output-schema.spec.ts +++ b/libs/sdk/src/tool/flows/__tests__/call-tool.output-schema.spec.ts @@ -7,7 +7,19 @@ import { rpc20260728, type TestFetchServer, } from '../../../__test-utils__/helpers/mcp-20260728.helpers'; -import { App, Tool, ToolContext } from '../../../common'; +import { App, FlowHooksOf, Plugin, Tool, ToolContext, type FlowCtxOf } from '../../../common'; +import { InvalidOutputError } from '../../../errors'; + +const ToolHook = FlowHooksOf('tools:call-tool'); +const finalizeFailures: unknown[] = []; + +@Plugin({ name: 'refusal-probe' }) +class RefusalProbe { + @ToolHook.Finally('finalize') + record(ctx: FlowCtxOf<'tools:call-tool'>) { + finalizeFailures.push(ctx.state.flowError); + } +} @Tool({ name: 'get_invoice', @@ -37,7 +49,12 @@ class LabelRatioTool extends ToolContext { } } -@App({ id: 'billing', name: 'Billing', tools: [GetInvoiceTool, MeasureRatioTool, LabelRatioTool] }) +@App({ + id: 'billing', + name: 'Billing', + tools: [GetInvoiceTool, MeasureRatioTool, LabelRatioTool], + plugins: [RefusalProbe], +}) class BillingApp {} describe('tools/call output schema enforcement', () => { @@ -73,6 +90,16 @@ describe('tools/call output schema enforcement', () => { expect(content.map((block) => block.text ?? '').join('\n')).not.toContain('CC-7731'); }); + it('records the refusal as state.flowError for the Finally hooks of finalize', async () => { + finalizeFailures.length = 0; + + await callGetInvoice(); + await rpc20260728(server.handler, 'tools/call', { name: 'measure_ratio', arguments: {} }); + + expect(finalizeFailures).toEqual([expect.any(InvalidOutputError), expect.any(InvalidOutputError)]); + expect((finalizeFailures[1] as InvalidOutputError).details?.reason).toBe('non-finite-number'); + }); + describe('non-finite numbers', () => { async function callTool(target: TestFetchServer, name: string) { const { message } = await rpc20260728(target.handler, 'tools/call', { name, arguments: {} }); diff --git a/libs/sdk/src/tool/flows/call-tool.flow.ts b/libs/sdk/src/tool/flows/call-tool.flow.ts index bc971eb50..2b12cecd6 100644 --- a/libs/sdk/src/tool/flows/call-tool.flow.ts +++ b/libs/sdk/src/tool/flows/call-tool.flow.ts @@ -141,6 +141,8 @@ const stateSchema = z.object({ // Store the raw executed output for plugins to see rawOutput: z.any().optional(), output: outputSchema, + // The run's first failure, set by the flow runner for the stages and Finally hooks after it + flowError: z.instanceof(Error).optional(), // UI result from applyUI stage (if UI config exists) uiResult: z.any().optional() as z.ZodType, // UI metadata from rendering (merged into _meta) diff --git a/libs/skills/catalog/frontmcp-development/SKILL.md b/libs/skills/catalog/frontmcp-development/SKILL.md index d1f442dac..5bc8b80eb 100644 --- a/libs/skills/catalog/frontmcp-development/SKILL.md +++ b/libs/skills/catalog/frontmcp-development/SKILL.md @@ -79,7 +79,7 @@ This is a router skill. The "steps" here are how to choose the right reference ( | Use official plugins (caching, remember, feature flags) | `official-plugins` | Built-in plugins for caching, session memory, approval, and feature flags (dashboard is beta) | | Connect to an external data source via a custom adapter | `create-adapter` | Create custom adapters for external data sources | | Configure LLM settings for an agent component | `create-agent-llm-config` | Configure LLM settings for agent components | -| Add will/did/around lifecycle hooks to a plugin | `create-plugin-hooks` | Add lifecycle hooks to plugins (will/did/around) | +| Add will/did/around/finally lifecycle hooks to a plugin | `create-plugin-hooks` | Add lifecycle hooks to plugins (will/did/around/finally) | ## Recommended Reading Order diff --git a/libs/skills/catalog/frontmcp-development/references/create-plugin-hooks.md b/libs/skills/catalog/frontmcp-development/references/create-plugin-hooks.md index cca52d962..a1daad079 100644 --- a/libs/skills/catalog/frontmcp-development/references/create-plugin-hooks.md +++ b/libs/skills/catalog/frontmcp-development/references/create-plugin-hooks.md @@ -1,11 +1,11 @@ --- name: create-plugin-hooks -description: Intercept and extend FrontMCP flows using before, after, around, and stage hook decorators +description: Intercept and extend FrontMCP flows using before, after, around, finally, and stage hook decorators --- # Creating Plugins with Flow Lifecycle Hooks -Plugins intercept and extend FrontMCP flows using lifecycle hook decorators. Every flow (tool calls, resource reads, prompt gets, etc.) is composed of **stages**, and hooks let you run logic before, after, around, or as part of any stage. +Plugins intercept and extend FrontMCP flows using lifecycle hook decorators. Every flow (tool calls, resource reads, prompt gets, etc.) is composed of **stages**, and hooks let you run logic before, after, around, or as part of any stage, or after it whatever its outcome. ## When to Use This Skill @@ -14,6 +14,7 @@ Plugins intercept and extend FrontMCP flows using lifecycle hook decorators. Eve - Adding before/after logic to tool execution (logging, metrics, input enrichment) - Implementing authorization checks that intercept flows before they reach the tool - Wrapping stage execution with caching, retry, or timing logic via `@Around` +- Closing what a hook opened (a span, a timer, a lock) after a stage, whether it succeeded or failed, via `@Finally` ### Recommended @@ -27,28 +28,64 @@ Plugins intercept and extend FrontMCP flows using lifecycle hook decorators. Eve - You want to use an existing official plugin that already provides hooks (see `official-plugins`) - You are building a simple tool with no cross-cutting concerns (see `create-tool`) -> **Decision:** Use this skill when you need to intercept or wrap flow stages with `@Will`, `@Did`, `@Around`, or `@Stage` decorators. +> **Decision:** Use this skill when you need to intercept or wrap flow stages with `@Will`, `@Did`, `@Around`, `@Finally`, or `@Stage` decorators. ## Hook Decorator Types -FrontMCP provides four hook decorators obtained via `FlowHooksOf(flowName)`: +FrontMCP provides five hook decorators obtained via `FlowHooksOf(flowName)`: -| Decorator | Timing | Use Case | -| --------- | ------------------------------------------------ | ---------------------------------------------- | -| `@Will` | **Before** a stage runs | Validate input, inject headers, check auth | -| `@Did` | **After** a stage completes | Log results, emit metrics, transform output | -| `@Stage` | **Joins** a stage, alongside the flow's own step | Extra work in the stage, ending the flow early | -| `@Around` | **Wraps** a stage (before + after) | Caching, timing, retry, skipping the stage | +| Decorator | Timing | Use Case | +| ---------- | -------------------------------------------------------- | ------------------------------------------------ | +| `@Will` | **Before** a stage runs | Validate input, inject headers, check auth | +| `@Did` | **After** a stage succeeds; skipped when it fails | Log results, emit metrics, transform output | +| `@Stage` | **Joins** a stage, alongside the flow's own step | Extra work in the stage, ending the flow early | +| `@Around` | **Wraps** a stage (before + after) | Caching, timing, retry, skipping the stage | +| `@Finally` | **After** a stage, whatever its outcome; can't change it | Close spans, timers or locks; record the failure | ### FlowHooksOf API ```typescript import { FlowHooksOf } from '@frontmcp/sdk'; -const { Stage, Will, Did, Around } = FlowHooksOf('tools:call-tool'); +const { Stage, Will, Did, Around, Finally } = FlowHooksOf('tools:call-tool'); ``` -`FlowHooksOf(flowName)` returns an object with all four decorator factories bound to the specified flow. +`FlowHooksOf(flowName)` returns an object with all five decorator factories bound to the specified flow. + +### `@Did` Runs on Success, `@Finally` Always Runs + +A stage runs its `@Will` hooks, then its body (wrapped by its `@Around` hooks), then its `@Did` hooks, then its `@Finally` hooks. + +- `@Did` runs only when the stage succeeded. When a `@Will` hook, the body or an `@Around` hook fails, the stage's `@Did` hooks are skipped. So a `@Did` hook that closes what a `@Will` hook opened (a span, a timer, a lock) leaks it on every failure. +- `@Finally` runs after the stage on every outcome: when it succeeded or responded, when it failed, when a `@Will` hook failed or responded in its place, when an `@Around` hook failed, and when `finalize` refused an answer the tool's output schema rejects (`InvalidOutputError`). Read the failure from `ctx.state.flowError`: the first error the run failed with (what a stage or hook threw or passed to `ctx.fail()`; a `FlowControl.abort(reason)` reads as `Flow aborted: `), typed on `tools:call-tool`, `resources:read-resource`, `prompts:get-prompt` and `jobs:execute-job`. +- A `@Finally` hook cannot change the outcome. If it calls `ctx.respond()` or `ctx.fail()`, or throws, the flow logs a warning and goes on as if it had not: the client gets the same answer or the same error, the values the hook set on `ctx.state` before it responded, failed or threw are put back (a job's answer, for one; an object it changed in place is not), and the stage's other `@Finally` hooks still run. A `@Finally` hook that returns normally keeps what it set on `ctx.state`, like any other hook: on `jobs:execute-job`, setting `state.answer` before `updateRunState` changes the result the run records and returns. +- `@Finally` hooks of one stage run in `priority` order, like every other hook. A stage the flow never reaches (because an earlier stage failed) runs none of its hooks, `@Finally` included: hook `finalize` to see the end of every call. + +```typescript +import { FlowCtxOf, Plugin, ToolHook } from '@frontmcp/sdk'; + +const { Will, Finally } = ToolHook; + +@Plugin({ name: 'execute-timer' }) +export class ExecuteTimerPlugin { + private readonly startedAt = new WeakMap, number>(); + + @Will('execute') + start(ctx: FlowCtxOf<'tools:call-tool'>) { + this.startedAt.set(ctx, Date.now()); + } + + @Finally('execute') + stop(ctx: FlowCtxOf<'tools:call-tool'>) { + const startedAt = this.startedAt.get(ctx); + if (startedAt === undefined) return; + const outcome = ctx.state.flowError ? `failed: ${ctx.state.flowError.message}` : 'succeeded'; + ctx.logger.info(`execute ${outcome} in ${Date.now() - startedAt}ms`); + } +} +``` + +Up to 1.10.15 there was no `@Finally`: a failure in `finalize` (an output schema refusing the answer) skipped the stage's `@Did` hooks and left `state.flowError` unset. ### What `@Stage` Does @@ -56,7 +93,7 @@ A `@Stage('execute')` hook does not replace the flow's own `execute` step: it ad To keep the flow's own step from running: -- Call `ctx.respond(result)` from the `@Stage` (or a `@Will`) hook. The flow ends with that result: it skips the rest of the stage and the main stages after it, while the stage's `@Did` hooks and the cleanup stages (in `tools:call-tool`: `releaseSemaphore`, `releaseQuota`, `applyUI`, `finalize`) still run. In `tools:call-tool`, `result` is a complete MCP `CallToolResult`; it does not go through the tool's output schema. +- Call `ctx.respond(result)` from the `@Stage` (or a `@Will`) hook. The flow ends with that result: it skips the rest of the stage and the main stages after it, while the stage's `@Finally` hooks (and its `@Did` hooks, when a `@Stage` hook answered) and the cleanup stages (in `tools:call-tool`: `releaseSemaphore`, `releaseQuota`, `applyUI`, `finalize`) still run. In `tools:call-tool`, `result` is a complete MCP `CallToolResult`; it does not go through the tool's output schema. - Or use an `@Around` hook that does not call `next()`. The stage is skipped and the flow goes on; in `tools:call-tool`, set `ctx.state.required.toolContext.output`, and the result is validated and formatted like the tool's own. ## Available Flow Names @@ -189,7 +226,7 @@ import { Usage: ```typescript -const { Will, Did, Around, Stage } = ToolHook; +const { Will, Did, Around, Finally, Stage } = ToolHook; ``` > **Note:** Other flows (e.g., `skills:filter`, transport flows) can be hooked by passing the flow name to `FlowHooksOf('flow:name')`. Prefer the pre-built exports above when one is available: exporting them is also what puts a flow's types in the published package, which is why `FlowHooksOf('prompts:get-prompt')`, `'prompts:list-prompts'` and `'completion:complete'` did not typecheck in consumer projects up to 1.8.7. @@ -217,8 +254,8 @@ A hook method on a plugin or provider receives the running flow as its first arg | `rawInput` | The input the flow was started with; for `tools:call-tool`, the MCP `request` and its `ctx` | | `logger` | The flow's logger (the tool, resource and prompt flows define one; every flow has `scopeLogger`) | | `get(token)` | Resolves a provider registered on the server, or a context-scoped provider of the current call | -| `respond(output)` | Ends the flow with this output | -| `fail(error)` | Ends the flow with this error (an `Error`, such as an MCP error class) | +| `respond(output)` | Ends the flow with this output (ignored in a `@Finally` hook) | +| `fail(error)` | Ends the flow with this error (an `Error`, such as an MCP error class; ignored in a `@Finally` hook) | The flow has no `toolName`, `elapsed` or `tryGet`, and its `input` is protected. In `tools:call-tool`, read the tool name and arguments from `ctx.state.required.input` (`name`, `arguments`), the tool entry from `ctx.state.tool`, and the tool instance from `ctx.state.toolContext` (set from `createToolCallContext` on). The state accepts only the keys the flow declares: `ctx.state.set('startedAt', Date.now())` does not compile. @@ -255,7 +292,7 @@ export class TimingPlugin { ## HookOptions -All four decorators accept an optional options object: +All five decorators accept an optional options object: ```typescript @Will('execute', { @@ -265,7 +302,7 @@ All four decorators accept an optional options object: }) ``` -- **priority** (`number`) - Execution order when multiple hooks target the same stage. Lower values run first, for `@Will`, `@Did`, `@Around` and `@Stage` alike. Default: `0`. +- **priority** (`number`) - Execution order when multiple hooks target the same stage. Lower values run first, for `@Will`, `@Did`, `@Around`, `@Finally` and `@Stage` alike. Default: `0`. - **filter** (`(ctx) => boolean`) - A predicate that receives the flow context. Return `false` to skip this hook for the current invocation. - **appliesTo** (`'own-app' | 'uncovered-apps'`) - A hook of a plugin installed on an app runs, in `tools/call`, `resources/read`, `prompts/get` and `completion/complete`, only for that app's entries (`'own-app'`), unless the plugin has `scope: 'server'`. With `'uncovered-apps'` it also runs for the entries of any app that has no instance of the same hook (same class and method) of its own or from a server-level plugin. Use it for gates an entry's metadata asks for (approval, feature flags), so the entry is not left ungated when the plugin sits on another app. Server-level plugins' hooks, and list-flow hooks, already run for every app. @@ -452,6 +489,7 @@ class ProcessOrderTool extends ToolContext { - `@Will('execute')` on a tool class runs **before** the `execute()` method of that specific tool - `@Did('execute')` runs **after** `execute()` completes successfully +- `@Finally('execute')` runs **after** `execute()` whether it succeeded or failed - `@Will('validateInput')` runs before input validation — useful for input enrichment - `@Did('validateOutput')` runs after output validation — useful for output transformation - The hook has full access to `this` (the tool context) including `this.input`, `this.get()`, `this.fail()` @@ -464,7 +502,7 @@ parseInput → ensureRemoteCapabilities → findTool → checkToolAuthorization → validateInput → execute → validateOutput → releaseSemaphore → releaseQuota → applyUI → finalize ``` -A tool-class hook declared as an instance method runs on the tool instance, which `createToolCallContext` builds. So it can hook `Did`/`Stage` on `createToolCallContext` and any hook on a later stage. The same holds for `@Resource` (`createResourceContext`), `@Prompt` (`createPromptContext`) and `@Agent` (`createAgentContext`) classes. +A tool-class hook declared as an instance method runs on the tool instance, which `createToolCallContext` builds. So it can hook `Did`/`Stage`/`Finally` on `createToolCallContext` and any hook on a later stage. The same holds for `@Resource` (`createResourceContext`), `@Prompt` (`createPromptContext`) and `@Agent` (`createAgentContext`) classes. To hook an earlier stage (`parseInput`, `findTool`, `checkToolAuthorization`, `checkEntryAuthorities`, and `findResource`, `findPrompt`, `findAgent`, `checkAgentAuthorization`), declare the hook as a `static` method. It runs without an instance -- `this` is the class, and the flow context is its argument, as for every hook -- and joins only the calls for its own entry, from their first stage: @@ -493,6 +531,7 @@ An instance method on an earlier stage, or `Will`/`Around` on `createToolCallCon | Hook decorator source | `const { Will, Did } = ToolHook;` or `FlowHooksOf('tools:call-tool')` | Importing `Will` directly from `@frontmcp/sdk` | Decorators must be bound to a specific flow via `FlowHooksOf` or pre-built exports | | Hook priority | `@Will('execute', { priority: -100 })` for early hooks | Relying on array order without priority | Multiple hooks on the same stage need explicit priority; lower runs first | | Around next() | `await next();` | Forgetting to call `next()` in `@Around` | Omitting `next()` skips the wrapped stage; its `@Did` hooks still run | +| Cleanup after a stage | `@Finally('execute')` closes the span or timer `@Will` opened | Closing it in `@Did('execute')` | `@Did` is skipped when the stage fails; `@Finally` runs on every outcome | | Around error recovery | `catch { ctx.state.required.toolContext.output = fallback; }` | Catching a rejected `next()` without setting the output | Returning normally handles the failure: the stage counts as successful | | Filter predicate | `filter: (ctx) => ctx.state.required.input.name !== 'health_check'` | Checking tool name inside the hook body and returning early | A filtered-out hook is skipped cleanly; for `@Around`, the stage still runs | | Tool-level hooks | `@Will('execute')` on a `@Tool` class (scoped to that tool) | `@Will('execute')` on a `@Plugin` class expecting tool-scoped behavior | Plugin hooks fire for every tool of the app; tool-level hooks only for that tool | @@ -508,21 +547,24 @@ An instance method on an earlier stage, or `Will`/`Around` on `createToolCallCon ### Runtime - [ ] `@Will` hook fires before the targeted stage -- [ ] `@Did` hook fires after the targeted stage completes +- [ ] `@Did` hook fires after the targeted stage succeeds +- [ ] `@Finally` hook fires after the targeted stage on success and on failure, and reads the failure from `ctx.state.flowError` - [ ] `@Around` hook calls `next()` and the wrapped stage executes - [ ] A `@Stage` hook meant to stand in for the stage ends the flow with `ctx.respond()` and a valid result for the flow - [ ] Hook `filter` correctly skips invocations for excluded tools ## Troubleshooting -| Problem | Cause | Solution | -| --------------------------------------------- | --------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------- | -| Hook never fires | Plugin not registered in `plugins` array | Add plugin class to `@App` or `@FrontMcp` `plugins` array | -| `InvalidHookFlowError` at startup | Entry-class hook that could never run | Make early-stage hooks `static`; move list-flow hooks (and a `@Job` hook for a flow other than `jobs:execute-job`) to a plugin or a provider | -| Hook fires for wrong flow | Used wrong flow name in `FlowHooksOf` | Verify flow name matches (e.g., `'tools:call-tool'` not `'tool:call'`) | -| `@Around` skips the stage entirely | `next()` not called inside the around handler | Always `await next()` to execute the wrapped stage | -| Multiple hooks execute in wrong order | Priorities not set or conflicting | Set explicit `priority` values; lower numbers execute first | -| The tool still runs despite a plugin `@Stage` | `@Stage` adds a step; it does not replace the stage | End the flow with `ctx.respond(result)`, or use `@Around` without calling `next()` | +| Problem | Cause | Solution | +| ------------------------------------------------- | --------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------- | +| Hook never fires | Plugin not registered in `plugins` array | Add plugin class to `@App` or `@FrontMcp` `plugins` array | +| `InvalidHookFlowError` at startup | Entry-class hook that could never run | Make early-stage hooks `static`; move list-flow hooks (and a `@Job` hook for a flow other than `jobs:execute-job`) to a plugin or a provider | +| Hook fires for wrong flow | Used wrong flow name in `FlowHooksOf` | Verify flow name matches (e.g., `'tools:call-tool'` not `'tool:call'`) | +| `@Around` skips the stage entirely | `next()` not called inside the around handler | Always `await next()` to execute the wrapped stage | +| Multiple hooks execute in wrong order | Priorities not set or conflicting | Set explicit `priority` values; lower numbers execute first | +| The tool still runs despite a plugin `@Stage` | `@Stage` adds a step; it does not replace the stage | End the flow with `ctx.respond(result)`, or use `@Around` without calling `next()` | +| A span or timer opened in `@Will` stays open | It is closed in `@Did`, which a failed stage skips | Close it in `@Finally` on the same stage | +| `ctx.respond()` in a `@Finally` hook does nothing | `@Finally` cannot change a stage's outcome | Answer from `@Will`, `@Stage` or `@Around`; the flow logs a warning for each ignored `@Finally` response or throw | ## Examples diff --git a/libs/skills/catalog/frontmcp-development/references/decorators-guide.md b/libs/skills/catalog/frontmcp-development/references/decorators-guide.md index 7d582ade0..ad38d5f01 100644 --- a/libs/skills/catalog/frontmcp-development/references/decorators-guide.md +++ b/libs/skills/catalog/frontmcp-development/references/decorators-guide.md @@ -24,7 +24,7 @@ FrontMCP uses a hierarchical decorator system. The nesting order is: +-- @Job (long-running job) +-- @Workflow (multi-step workflow) +-- @Flow (custom flow) - +-- @Hook (@Will, @Did, @Stage, @Around) + +-- @Hook (@Will, @Did, @Stage, @Around, @Finally) ``` --- @@ -61,34 +61,34 @@ FrontMCP uses a hierarchical decorator system. The nesting order is: **Key fields:** -| Field | Description | -| --------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `info` | `serverInfo` of every `initialize`: `name`, `version`, optional `title` (else the name), `websiteUrl`, `icons` | -| `instructions?` | Top-level server-level instructions (system prompt) surfaced in the MCP `initialize` response and the 2026-07-28 `server/discover` result. Combined with the channel hint and the skill catalog summary per `skillsConfig.injectInstructions`; sent by every transport, `createFetchHandler()` and `connect()` included | -| `apps` | Array of `@App` classes to mount | -| `serve?` | Auto-start HTTP server (default: `true`). Set `false` for programmatic usage | -| `splitByApp?` | If `true`, each app gets its own scope and basePath. Default: `false` | -| `redis?` | Redis / Vercel KV connection for sessions, transport persistence, auth tokens | -| `plugins?` | Global plugins (instantiated per scope) | -| `providers?` | Global DI providers available to all apps | -| `tools?` | Standalone tools (outside apps, merged with app tools) | -| `resources?` | Standalone resources (merged with app resources) | -| `skills?` | Standalone skills (merged with app skills) | -| `skillsConfig?` | Skills HTTP endpoints (`/llm.txt`, `/skills`), MCP tool config, `injectInstructions` policy, and tamper-evident `audit` log — see `configure-skills-http` | -| `transport?` | Transport preset (`'modern'`, `'legacy'`, `'stateless-api'`, `'full'`) or object | -| `auth?` | Authentication mode: `'public'`, `'transparent'`, `'local'`, `'remote'` | -| `http?` | HTTP server options (port, host, cors, socketPath) | -| `logging?` | Logging configuration (transports and levels); without `level`, the level `FRONTMCP_LOG_LEVEL` names, else `info` (`warn` in a browser); per-request lines are at `verbose` | -| `elicitation?` | Enable interactive user input during tool execution | -| `sqlite?` | SQLite storage for local deployments (sessions, events) | -| `pubsub?` | Redis pub/sub for distributed resource subscriptions across multiple instances (single-server deployments use in-memory subscriptions; falls back to `redis` config) | -| `jobs?` | Background jobs/workflows system (`{ enabled, store? }`) | -| `throttle?` | Server-level guard config (see note below) | -| `pagination?` | List operation pagination (`tools/list` endpoint) | -| `fetch?` | What `this.fetch()` adds upstream: `forwardCallerTokenTo` / `forwardCustomHeadersTo` origin allow-lists (default: none), `autoInjectTracingHeaders`, `requestTimeout` | -| `ui?` | UI rendering config (CDN overrides for widget imports) | -| `extApps?` | MCP Apps widget requests on a session: `ui/open-link`, `ui/request-display-mode`, `ui/update-model-context`, `notifications/message` (or the earlier `ui/openLink`, `ui/setDisplayMode`, `ui/updateModelContext`, `ui/log`). `ui/callServerTool` returns the tool's data, no page; a no-result method, `result: {}` | -| `loader?` | Default npm/ESM package loader for `App.esm()` / `App.remote()` apps | +| Field | Description | +| --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `info` | `serverInfo` of every `initialize`: `name`, `version`, optional `title` (else the name), `websiteUrl`, `icons` | +| `instructions?` | Top-level server-level instructions (system prompt) surfaced in the MCP `initialize` response and the 2026-07-28 `server/discover` result. Combined with the channel hint and the skill catalog summary per `skillsConfig.injectInstructions`; sent by every transport, `createFetchHandler()` and `connect()` included | +| `apps` | Array of `@App` classes to mount | +| `serve?` | Auto-start HTTP server (default: `true`). Set `false` for programmatic usage | +| `splitByApp?` | If `true`, each app gets its own scope and basePath. Default: `false` | +| `redis?` | Redis / Vercel KV connection for sessions, transport persistence, auth tokens | +| `plugins?` | Global plugins (instantiated per scope) | +| `providers?` | Global DI providers available to all apps | +| `tools?` | Standalone tools (outside apps, merged with app tools) | +| `resources?` | Standalone resources (merged with app resources) | +| `skills?` | Standalone skills (merged with app skills) | +| `skillsConfig?` | Skills HTTP endpoints (`/llm.txt`, `/skills`), MCP tool config, `injectInstructions` policy, and tamper-evident `audit` log — see `configure-skills-http` | +| `transport?` | Transport preset (`'modern'`, `'legacy'`, `'stateless-api'`, `'full'`) or object | +| `auth?` | Authentication mode: `'public'`, `'transparent'`, `'local'`, `'remote'` | +| `http?` | HTTP server options (port, host, cors, socketPath) | +| `logging?` | Logging configuration (transports and levels); without `level`, the level `FRONTMCP_LOG_LEVEL` names, else `info` (`warn` in a browser); per-request lines are at `verbose` | +| `elicitation?` | Enable interactive user input during tool execution | +| `sqlite?` | SQLite storage for local deployments (sessions, events) | +| `pubsub?` | Redis pub/sub for distributed resource subscriptions across multiple instances (single-server deployments use in-memory subscriptions; falls back to `redis` config) | +| `jobs?` | Background jobs/workflows system (`{ enabled, store? }`) | +| `throttle?` | Server-level guard config (see note below) | +| `pagination?` | List operation pagination (`tools/list` endpoint) | +| `fetch?` | What `this.fetch()` adds upstream: `forwardCallerTokenTo` / `forwardCustomHeadersTo` origin allow-lists (default: none), `autoInjectTracingHeaders`, `requestTimeout` | +| `ui?` | UI rendering config (CDN overrides for widget imports) | +| `extApps?` | MCP Apps widget requests on a session: `ui/open-link`, `ui/request-display-mode`, `ui/update-model-context`, `notifications/message` (or the earlier `ui/openLink`, `ui/setDisplayMode`, `ui/updateModelContext`, `ui/log`). `ui/callServerTool` returns the tool's data, no page; a no-result method, `result: {}` | +| `loader?` | Default npm/ESM package loader for `App.esm()` / `App.remote()` apps | | `registry?` | MCP Registry listing: `name` (`com.acme/weather`), `description`, `repository`, `publicKey` / `publicKeyEnv`. Serves the domain proof at `/.well-known/mcp-registry-auth`, the SEP-2127 server card at `/server-card` and `/.well-known/ai-catalog.json` — see `frontmcp-deployment`'s `list-in-mcp-registry` | > **Throttle vs per-tool guards:** Server-level `throttle` is a `GuardConfig` object with `global`, `defaultRateLimit`, `defaultConcurrency`, `defaultTimeout` sub-fields that set server-wide defaults. Tool-level `rateLimit`, `concurrency`, `timeout` fields (on `@Tool`) override these defaults per tool. @@ -649,22 +649,23 @@ class DeployPipeline {} --- -## 15. Hook Decorators (@Will, @Did, @Stage, @Around) +## 15. Hook Decorators (@Will, @Did, @Stage, @Around, @Finally) -**Purpose:** Run logic before, after, around, or as part of a stage of a flow, such as a tool call or a resource read. +**Purpose:** Run logic before, after, around, or as part of a stage of a flow, such as a tool call or a resource read, or after it whatever its outcome. **When to use:** When you need cross-cutting logic (logging, timing, authorization, caching) around tool calls, resource reads, prompt gets and the other flows. -`@frontmcp/sdk` does not export `Will`, `Did`, `Stage` or `Around` on their own. Take them from the export for the flow you hook (`ToolHook` for `tools:call-tool`, `ResourceHook` for `resources:read-resource`, `PromptHook` for `prompts:get-prompt`, ...) or from `FlowHooksOf('')`. The flow comes from where you take the decorator; the decorator itself takes a **stage** of that flow, such as `'execute'`. +`@frontmcp/sdk` does not export `Will`, `Did`, `Stage`, `Around` or `Finally` on their own. Take them from the export for the flow you hook (`ToolHook` for `tools:call-tool`, `ResourceHook` for `resources:read-resource`, `PromptHook` for `prompts:get-prompt`, ...) or from `FlowHooksOf('')`. The flow comes from where you take the decorator; the decorator itself takes a **stage** of that flow, such as `'execute'`. **Variants:** -| Decorator | Timing | Description | -| --------- | -------- | --------------------------------------- | -| `@Will` | Before | Runs before the stage | -| `@Did` | After | Runs after the stage completes | -| `@Stage` | During | Runs as part of the stage itself | -| `@Around` | Wrapping | Wraps the stage; `await next()` runs it | +| Decorator | Timing | Description | +| ---------- | ------------- | ------------------------------------------------------------------------------------------------- | +| `@Will` | Before | Runs before the stage | +| `@Did` | After success | Runs after the stage succeeds; skipped when it fails | +| `@Stage` | During | Runs as part of the stage itself | +| `@Around` | Wrapping | Wraps the stage; `await next()` runs it | +| `@Finally` | After, always | Runs after the stage whatever its outcome; reads `ctx.state.flowError`; cannot change the outcome | Declare hooks on a `@Plugin` (or a provider) to run them for every tool, resource or prompt of the app it is registered on: @@ -703,6 +704,7 @@ A hook method receives the running flow, typed `FlowCtxOf<''>`: - `ctx.state` holds the flow's data (for `tools:call-tool`: `input`, `tool`, `toolContext`, ...); `ctx.state.required.` throws when the key is unset. - `ctx.logger` logs, and `ctx.fail(error)` or `ctx.respond(output)` ends the flow. - An `@Around` hook also receives `next`, which runs the stage. +- A `@Finally` hook runs when the stage failed too, so close spans, timers and locks there rather than in `@Did`; its `ctx.respond()`, `ctx.fail()` and throws are logged and ignored. - `this` is the plugin or provider instance. A hook declared on a `@Tool`, `@Resource` or `@Prompt` class runs only for that entry, on its context (`this.input`, `this.get()`). See `create-plugin-hooks` for the stages of each flow, `priority`, `filter` and entry-class hooks. @@ -711,23 +713,23 @@ A hook declared on a `@Tool`, `@Resource` or `@Prompt` class runs only for that ## Quick Reference Table -| Decorator | Extends | Registered In | Purpose | -| --------------------------- | ----------------- | ------------------------------- | ------------------------ | -| `@FrontMcp` | - | Root | Server configuration | -| `@App` | - | `@FrontMcp.apps` | Module grouping | -| `@Tool` | `ToolContext` | `@App.tools` | Executable action | -| `@Prompt` | `PromptContext` | `@App.prompts` | Prompt template | -| `@Resource` | `ResourceContext` | `@App.resources` | Static data | -| `@ResourceTemplate` | `ResourceContext` | `@App.resources` | Parameterized data | -| `@Agent` | `AgentContext` | `@App.agents` | Autonomous AI agent | -| `@Skill` | - | `@App.skills` | Knowledge package | -| `@Plugin` | - | `@App.plugins` | Cross-cutting concern | -| `@Adapter` | - | `@App.adapters` | External integration | -| `@Provider` | - | `@App.providers` | DI binding | -| `@Flow` | - | `@App` | Custom flow | -| `@Job` | `JobContext` | `@App.jobs` | Background task | -| `@Workflow` | - | `@App.workflows` | Multi-step orchestration | -| `@Will/@Did/@Stage/@Around` | - | Plugin, provider or entry class | Flow stage hooks | +| Decorator | Extends | Registered In | Purpose | +| ------------------------------------ | ----------------- | ------------------------------- | ------------------------ | +| `@FrontMcp` | - | Root | Server configuration | +| `@App` | - | `@FrontMcp.apps` | Module grouping | +| `@Tool` | `ToolContext` | `@App.tools` | Executable action | +| `@Prompt` | `PromptContext` | `@App.prompts` | Prompt template | +| `@Resource` | `ResourceContext` | `@App.resources` | Static data | +| `@ResourceTemplate` | `ResourceContext` | `@App.resources` | Parameterized data | +| `@Agent` | `AgentContext` | `@App.agents` | Autonomous AI agent | +| `@Skill` | - | `@App.skills` | Knowledge package | +| `@Plugin` | - | `@App.plugins` | Cross-cutting concern | +| `@Adapter` | - | `@App.adapters` | External integration | +| `@Provider` | - | `@App.providers` | DI binding | +| `@Flow` | - | `@App` | Custom flow | +| `@Job` | `JobContext` | `@App.jobs` | Background task | +| `@Workflow` | - | `@App.workflows` | Multi-step orchestration | +| `@Will/@Did/@Stage/@Around/@Finally` | - | Plugin, provider or entry class | Flow stage hooks | --- diff --git a/libs/skills/catalog/frontmcp-observability/examples/telemetry-api/plugin-telemetry.md b/libs/skills/catalog/frontmcp-observability/examples/telemetry-api/plugin-telemetry.md index 35eb9e369..acbcbc0dd 100644 --- a/libs/skills/catalog/frontmcp-observability/examples/telemetry-api/plugin-telemetry.md +++ b/libs/skills/catalog/frontmcp-observability/examples/telemetry-api/plugin-telemetry.md @@ -7,7 +7,7 @@ tags: [telemetry, plugin, hooks, cache, audit] features: - 'Plugin hooks can access toolCtx.telemetry to add events to the active span' - 'Events from plugins appear in the same trace as the tool execution' - - "One `@ToolHook.Around('execute')` hook records success and failure: the flow state has no `error` key, and a `Did` hook runs only after a successful execute" + - "One `@ToolHook.Around('execute')` hook records success and failure while the execute span is open: a `Did` hook runs only after a successful execute" - 'Graceful degradation when observability is not enabled' --- @@ -94,7 +94,7 @@ tool my_tool - Plugin hooks can access toolCtx.telemetry to add events to the active span - Events from plugins appear in the same trace as the tool execution -- One `@ToolHook.Around('execute')` hook records success and failure: the flow state has no `error` key, and a `Did` hook runs only after a successful execute +- One `@ToolHook.Around('execute')` hook records success and failure while the execute span is open: a `Did` hook runs only after a successful execute - Graceful degradation when observability is not enabled ## Related diff --git a/libs/skills/catalog/skills-manifest.json b/libs/skills/catalog/skills-manifest.json index 65a310d09..0a9a4646c 100644 --- a/libs/skills/catalog/skills-manifest.json +++ b/libs/skills/catalog/skills-manifest.json @@ -1854,7 +1854,7 @@ }, { "name": "create-plugin-hooks", - "description": "Intercept and extend FrontMCP flows using before, after, around, and stage hook decorators", + "description": "Intercept and extend FrontMCP flows using before, after, around, finally, and stage hook decorators", "examples": [ { "name": "basic-logging-plugin", @@ -2692,7 +2692,7 @@ "features": [ "Plugin hooks can access toolCtx.telemetry to add events to the active span", "Events from plugins appear in the same trace as the tool execution", - "One `@ToolHook.Around('execute')` hook records success and failure: the flow state has no `error` key, and a `Did` hook runs only after a successful execute", + "One `@ToolHook.Around('execute')` hook records success and failure while the execute span is open: a `Did` hook runs only after a successful execute", "Graceful degradation when observability is not enabled" ] }, From e4db9b39269c374fc3696d82a1fd118d14160a12 Mon Sep 17 00:00:00 2001 From: "agentfront[bot]" Date: Mon, 2 Nov 2026 06:31:57 +0000 Subject: [PATCH 2/2] Cherry-pick: fix(sdk): list union output schemas and stop stripping undeclared output keys silently Cherry-picked from #981 (merged to release/1.10.x) Original commit: 1975c1b5b021224c1f15b8e73daba0d861dd1ff3 Co-Authored-By: frontegg-david <69419539+frontegg-david@users.noreply.github.com> --- .../e2e/tool-output-schema-modes.e2e.spec.ts | 162 +++++++++- .../e2e/tools-list-output-schema.e2e.spec.ts | 145 ++++++++- .../e2e/dts-consumer.e2e.spec.ts | 5 + .../fixture/output-schemas-must-compile.ts | 38 +++ .../fixture/tool-must-fail.ts | 10 +- .../fixture/tsconfig.json | 3 +- libs/lazy-zod/README.md | 1 + .../src/__tests__/has-transform.spec.ts | 43 +++ libs/lazy-zod/src/index.ts | 2 +- libs/lazy-zod/src/utils.ts | 26 ++ .../src/common/decorators/tool.decorator.ts | 4 +- libs/sdk/src/common/entries/tool.entry.ts | 136 +++++--- .../metadata/__tests__/output-policy.spec.ts | 82 +++++ .../src/common/metadata/front-mcp.metadata.ts | 3 + libs/sdk/src/common/metadata/output-policy.ts | 86 ++++- libs/sdk/src/common/metadata/tool.metadata.ts | 9 +- .../__tests__/extend-output-schema.spec.ts | 21 ++ .../helpers/extend-output-schema.ts | 5 +- libs/sdk/src/scope/scope.instance.ts | 4 + .../tool-instance.output-schema.spec.ts | 296 ++++++++++++++++++ .../__tests__/call-tool.output-schema.spec.ts | 239 +++++++++++++- .../tools-list.output-schema.spec.ts | 191 +++++++++++ libs/sdk/src/tool/flows/call-tool.flow.ts | 30 +- libs/sdk/src/tool/flows/tools-list.flow.ts | 27 +- libs/sdk/src/tool/tool-output-policy.ts | 57 ++++ libs/sdk/src/tool/tool.instance.ts | 12 +- libs/sdk/src/tool/tool.utils.ts | 71 ++++- .../04-tool-with-zod-schema-output.md | 6 +- .../references/decorator-options.md | 34 +- .../create-tool/references/output-schema.md | 50 ++- .../rules/always-define-output-schema.md | 4 +- .../references/common-checklist.md | 2 +- .../references/setup-testing.md | 19 +- libs/testing/README.md | 2 +- .../mcp-matchers.output-schema.spec.ts | 61 ++++ libs/testing/src/matchers/matcher-types.ts | 42 ++- libs/testing/src/matchers/mcp-matchers.ts | 49 ++- .../src/content/content.dropped-keys.spec.ts | 59 ++++ libs/utils/src/content/content.ts | 34 ++ libs/utils/src/content/index.ts | 2 +- libs/utils/src/index.ts | 2 +- 41 files changed, 1931 insertions(+), 143 deletions(-) create mode 100644 apps/e2e/demo-e2e-dts-consumer/fixture/output-schemas-must-compile.ts create mode 100644 libs/lazy-zod/src/__tests__/has-transform.spec.ts create mode 100644 libs/sdk/src/tool/__tests__/tool-instance.output-schema.spec.ts create mode 100644 libs/sdk/src/tool/flows/__tests__/tools-list.output-schema.spec.ts create mode 100644 libs/sdk/src/tool/tool-output-policy.ts create mode 100644 libs/testing/src/matchers/__tests__/mcp-matchers.output-schema.spec.ts create mode 100644 libs/utils/src/content/content.dropped-keys.spec.ts diff --git a/apps/e2e/demo-e2e-direct/e2e/tool-output-schema-modes.e2e.spec.ts b/apps/e2e/demo-e2e-direct/e2e/tool-output-schema-modes.e2e.spec.ts index 1e69744ac..def70bf9e 100644 --- a/apps/e2e/demo-e2e-direct/e2e/tool-output-schema-modes.e2e.spec.ts +++ b/apps/e2e/demo-e2e-direct/e2e/tool-output-schema-modes.e2e.spec.ts @@ -13,8 +13,24 @@ * - 'none': expose it nowhere. * * Driven through the real `tools/list` flow via `server.listTools()`. + * + * `output.undeclaredKeys` decides what `tools/call` does with keys an answer carries that its + * `outputSchema` does not declare: 'strip', 'warn' (strip and log once), 'keep' or 'fail'. + * A call that renders the tool's UI sends declared keys only, whatever the policy. */ -import { App, FrontMcpInstance, Tool, ToolContext, z, type DirectMcpServer } from '@frontmcp/sdk'; +import { + App, + connect, + FrontMcpInstance, + LogLevel, + LogTransport, + LogTransportInterface, + Tool, + ToolContext, + z, + type DirectMcpServer, + type LogRecord, +} from '@frontmcp/sdk'; // A structured (top-level object) output so the schema is eligible for both // outputSchema advertisement and the description rendering. @@ -204,3 +220,147 @@ describe('tools/list output.schemaMode (E2E)', () => { }); }); }); + +const UNDECLARED_VALUE = 'QCC-4410'; +const quoteAnswer = { price: 10, quoteCostCenter: UNDECLARED_VALUE } as { price: number }; +const quoteOutput = { price: z.number() }; + +@Tool({ name: 'quote_default', inputSchema: {}, outputSchema: quoteOutput }) +class DefaultQuoteTool extends ToolContext { + async execute() { + return quoteAnswer; + } +} + +@Tool({ name: 'quote_warned', inputSchema: {}, outputSchema: quoteOutput, output: { undeclaredKeys: 'warn' } }) +class WarnedQuoteTool extends ToolContext { + async execute() { + return quoteAnswer; + } +} + +@Tool({ name: 'quote_kept', inputSchema: {}, outputSchema: quoteOutput, output: { undeclaredKeys: 'keep' } }) +class KeptQuoteTool extends ToolContext { + async execute() { + return quoteAnswer; + } +} + +@Tool({ name: 'quote_strict', inputSchema: {}, outputSchema: quoteOutput, output: { undeclaredKeys: 'fail' } }) +class StrictQuoteTool extends ToolContext { + async execute() { + return quoteAnswer; + } +} + +@Tool({ + name: 'quote_view', + inputSchema: {}, + outputSchema: quoteOutput, + output: { undeclaredKeys: 'keep' }, + ui: { template: (ctx) => `
${ctx.helpers.escapeHtml(JSON.stringify(ctx.output))}
` }, +}) +class QuoteViewTool extends ToolContext { + async execute() { + return quoteAnswer; + } +} + +@App({ + name: 'UndeclaredKeys', + description: 'Tools exercising output.undeclaredKeys', + tools: [DefaultQuoteTool, WarnedQuoteTool, KeptQuoteTool, StrictQuoteTool, QuoteViewTool], +}) +class UndeclaredKeysApp {} + +const policyWarnings: string[] = []; + +@LogTransport({ name: 'UndeclaredKeysE2eCapture', description: 'Keeps warnings for assertions' }) +class PolicyWarningCapture extends LogTransportInterface { + log(record: LogRecord): void { + if (record.level === LogLevel.Warn) policyWarnings.push(record.message); + } +} + +const undeclaredKeysServerConfig = { + info: { name: 'undeclared-keys-e2e', version: '0.0.0' }, + apps: [UndeclaredKeysApp], + auth: { mode: 'public' as const }, + // A client named quote-viewer is treated as Claude, which renders the UI without structuredContent. + transport: { platformDetection: { mappings: [{ pattern: 'quote-viewer', platform: 'claude' as const }] } }, + logging: { level: LogLevel.Warn, enableConsole: false, transports: [PolicyWarningCapture] }, +}; + +describe('tools/call output.undeclaredKeys (E2E)', () => { + let server: DirectMcpServer; + + beforeAll(async () => { + server = await FrontMcpInstance.createDirect(undeclaredKeysServerConfig); + }); + + afterAll(async () => { + await server.dispose(); + }); + + it("strips undeclared keys silently by default outside development ('strip')", async () => { + const result = await server.callTool('quote_default', {}); + + expect(result.structuredContent).toEqual({ price: 10 }); + expect(JSON.stringify(result)).not.toContain(UNDECLARED_VALUE); + expect(policyWarnings.filter((line) => line.includes('quote_default'))).toEqual([]); + }); + + it("'warn' strips undeclared keys and warns once for the tool and key set", async () => { + const first = await server.callTool('quote_warned', {}); + await server.callTool('quote_warned', {}); + + expect(first.structuredContent).toEqual({ price: 10 }); + const warnings = policyWarnings.filter((line) => line.includes('quote_warned')); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('quoteCostCenter'); + }); + + it("'keep' passes undeclared keys through, and an MCP client accepts them against the listed schema", async () => { + const client = await connect(undeclaredKeysServerConfig, { + clientInfo: { name: 'policy-probe', version: '1.0.0' }, + }); + try { + await client.listTools(); + const result = (await client.callTool('quote_kept', {})) as { + structuredContent?: unknown; + content?: Array<{ text?: string }>; + }; + + expect(result.structuredContent).toEqual({ price: 10, quoteCostCenter: UNDECLARED_VALUE }); + expect(result.content?.[0]?.text).toContain(UNDECLARED_VALUE); + } finally { + await client.close(); + } + }); + + it("'fail' refuses the answer with INVALID_OUTPUT", async () => { + await expect(server.callTool('quote_strict', {})).rejects.toMatchObject({ code: 'INVALID_OUTPUT' }); + }); + + it('a call that renders the UI sends declared keys only, even under keep', async () => { + const result = await server.callTool('quote_view', {}); + + expect(result._meta?.['ui/html']).toBeDefined(); + expect(result.structuredContent).toEqual({ price: 10 }); + expect(JSON.stringify(result)).not.toContain(UNDECLARED_VALUE); + }); + + it('a UI render for a client that takes no structuredContent sends declared keys only, even under keep', async () => { + const client = await connect(undeclaredKeysServerConfig, { + clientInfo: { name: 'quote-viewer', version: '1.0.0' }, + }); + try { + const result = await client.callTool('quote_view', {}); + + expect(JSON.stringify(result)).toContain('ui/html'); + expect(JSON.stringify(result)).not.toContain(UNDECLARED_VALUE); + } finally { + await client.close(); + } + }); +}); diff --git a/apps/e2e/demo-e2e-direct/e2e/tools-list-output-schema.e2e.spec.ts b/apps/e2e/demo-e2e-direct/e2e/tools-list-output-schema.e2e.spec.ts index 9f5b095d0..804cde3c5 100644 --- a/apps/e2e/demo-e2e-direct/e2e/tools-list-output-schema.e2e.spec.ts +++ b/apps/e2e/demo-e2e-direct/e2e/tools-list-output-schema.e2e.spec.ts @@ -9,8 +9,9 @@ * Verifies the SDK now converts structured object outputs (symmetric with * inputSchema), while primitive outputs correctly remain unadvertised (they flow * through `content`, and MCP requires `outputSchema` to be a top-level object). + * A union whose members are all objects is listed too, typed `object` at the top. */ -import { App, FrontMcpInstance, Tool, ToolContext, z, type DirectMcpServer } from '@frontmcp/sdk'; +import { App, connect, FrontMcpInstance, Tool, ToolContext, z, type DirectMcpServer } from '@frontmcp/sdk'; const weatherOutput = { city: z.string(), tempF: z.number() }; @@ -50,24 +51,83 @@ class CountPrimitiveTool extends ToolContext { } } +const documentHit = z.object({ kind: z.literal('document'), title: z.string() }); +const personHit = z.object({ kind: z.literal('person'), name: z.string() }); + +@Tool({ + name: 'search_union', + description: 'Tool with a z.union() of objects outputSchema', + inputSchema: { query: z.string() }, + outputSchema: z.union([documentHit, personHit]), +}) +class SearchUnionTool extends ToolContext { + async execute(input: { query: string }) { + return { kind: 'document' as const, title: input.query }; + } +} + +@Tool({ + name: 'resolve_discriminated', + description: 'Tool with a z.discriminatedUnion() outputSchema', + inputSchema: { handle: z.string() }, + outputSchema: z.discriminatedUnion('kind', [documentHit, personHit]), +}) +class ResolveDiscriminatedTool extends ToolContext { + async execute(input: { handle: string }) { + return { kind: 'person' as const, name: input.handle }; + } +} + +// Schemas registered with an id serialize to `$ref`s into `$defs` +const registeredHit = z.union([ + documentHit.meta({ id: 'ToolsListE2eDocumentHit' }), + personHit.meta({ id: 'ToolsListE2ePersonHit' }), +]); + +@Tool({ + name: 'lookup_registered', + description: 'Tool with a z.union() of objects registered with an id', + inputSchema: { name: z.string() }, + outputSchema: registeredHit, +}) +class LookupRegisteredTool extends ToolContext { + async execute(input: { name: string }) { + return { kind: 'person' as const, name: input.name }; + } +} + @App({ name: 'OutputSchema', description: 'Tools exercising outputSchema advertisement', - tools: [WeatherStructTool, ZodObjectOutTool, CountPrimitiveTool], + tools: [ + WeatherStructTool, + ZodObjectOutTool, + CountPrimitiveTool, + SearchUnionTool, + ResolveDiscriminatedTool, + LookupRegisteredTool, + ], }) class OutputSchemaApp {} -type JsonSchemaLike = { type?: string; properties?: Record }; +const outputSchemaServerConfig = { + info: { name: 'output-schema-e2e', version: '0.0.0' }, + apps: [OutputSchemaApp], + auth: { mode: 'public' as const }, +}; + +type JsonSchemaLike = { + type?: string; + properties?: Record; + anyOf?: JsonSchemaLike[]; + oneOf?: JsonSchemaLike[]; +}; describe('tools/list outputSchema advertisement (E2E)', () => { let server: DirectMcpServer; beforeAll(async () => { - server = await FrontMcpInstance.createDirect({ - info: { name: 'output-schema-e2e', version: '0.0.0' }, - apps: [OutputSchemaApp], - auth: { mode: 'public' as const }, - }); + server = await FrontMcpInstance.createDirect(outputSchemaServerConfig); }); afterAll(async () => { @@ -98,4 +158,73 @@ describe('tools/list outputSchema advertisement (E2E)', () => { expect(tool).toBeDefined(); expect(tool?.outputSchema).toBeUndefined(); }); + + it('advertises a z.union() of objects with a top-level object type and anyOf', async () => { + const { tools } = await server.listTools(); + const out = tools.find((t) => t.name === 'search_union')?.outputSchema as JsonSchemaLike | undefined; + + expect(out?.type).toBe('object'); + expect(out?.anyOf?.map((member) => member.type)).toEqual(['object', 'object']); + expect(out?.anyOf?.[0]?.properties?.['title']).toBeDefined(); + }); + + it('advertises a z.discriminatedUnion() with a top-level object type and oneOf', async () => { + const { tools } = await server.listTools(); + const out = tools.find((t) => t.name === 'resolve_discriminated')?.outputSchema as JsonSchemaLike | undefined; + + expect(out?.type).toBe('object'); + expect(out?.oneOf?.map((member) => member.type)).toEqual(['object', 'object']); + }); + + it('answers a union tool with structuredContent an MCP client validates against the listed schema', async () => { + const client = await connect(outputSchemaServerConfig, { clientInfo: { name: 'schema-probe', version: '1.0.0' } }); + try { + await client.listTools(); + const result = (await client.callTool('resolve_discriminated', { handle: 'Ada' })) as { + structuredContent?: unknown; + }; + + expect(result.structuredContent).toEqual({ kind: 'person', name: 'Ada' }); + } finally { + await client.close(); + } + }); + + it('advertises a union of objects registered with an id, and an MCP client validates its answer', async () => { + const { tools } = await server.listTools(); + const out = tools.find((t) => t.name === 'lookup_registered')?.outputSchema as + | (JsonSchemaLike & { $defs?: Record }) + | undefined; + expect(out?.type).toBe('object'); + expect(Object.keys(out?.$defs ?? {})).toEqual(['ToolsListE2eDocumentHit', 'ToolsListE2ePersonHit']); + + const client = await connect(outputSchemaServerConfig, { clientInfo: { name: 'schema-probe', version: '1.0.0' } }); + try { + await client.listTools(); + const result = (await client.callTool('lookup_registered', { name: 'Ada' })) as { structuredContent?: unknown }; + + expect(result.structuredContent).toEqual({ kind: 'person', name: 'Ada' }); + } finally { + await client.close(); + } + }); + + it('lists a union registered with an id that an MCP client compiles when elicitation wraps it', async () => { + const client = await connect( + { ...outputSchemaServerConfig, elicitation: { enabled: true } }, + { clientInfo: { name: 'schema-probe', version: '1.0.0' } }, + ); + try { + const tools = (await client.listTools()) as Array<{ name: string; outputSchema?: unknown }>; + expect(tools.find((t) => t.name === 'lookup_registered')?.outputSchema).toMatchObject({ + type: 'object', + $defs: { ToolsListE2eDocumentHit: { type: 'object' } }, + }); + + const result = (await client.callTool('lookup_registered', { name: 'Ada' })) as { structuredContent?: unknown }; + expect(result.structuredContent).toEqual({ kind: 'person', name: 'Ada' }); + } finally { + await client.close(); + } + }); }); diff --git a/apps/e2e/demo-e2e-dts-consumer/e2e/dts-consumer.e2e.spec.ts b/apps/e2e/demo-e2e-dts-consumer/e2e/dts-consumer.e2e.spec.ts index e5bbbe45e..ecdf0a0f3 100644 --- a/apps/e2e/demo-e2e-dts-consumer/e2e/dts-consumer.e2e.spec.ts +++ b/apps/e2e/demo-e2e-dts-consumer/e2e/dts-consumer.e2e.spec.ts @@ -30,6 +30,7 @@ const fixtureFiles = [ 'hooks-must-compile.ts', 'host-must-compile.ts', 'skills-must-compile.ts', + 'output-schemas-must-compile.ts', ].map((fileName) => path.join(fixtureDir, fileName)); function compileFixture(): { diagnostics: FixtureDiagnostic[]; sourceFiles: string[] } { @@ -102,6 +103,10 @@ describe('@frontmcp/sdk declarations in a strict consumer project', () => { expect(describeEach(inFixtureFile('skills-must-compile.ts'))).toEqual([]); }); + it('types a z.union() or z.discriminatedUnion() outputSchema with no cast, and output.undeclaredKeys', () => { + expect(describeEach(inFixtureFile('output-schemas-must-compile.ts'))).toEqual([]); + }); + it('ships declaration files that type-check with skipLibCheck disabled', () => { const sdkDeclarationDiagnostics = diagnostics.filter((diagnostic) => diagnostic.fileName.startsWith(sdkDeclarationsDir + path.sep), diff --git a/apps/e2e/demo-e2e-dts-consumer/fixture/output-schemas-must-compile.ts b/apps/e2e/demo-e2e-dts-consumer/fixture/output-schemas-must-compile.ts new file mode 100644 index 000000000..3d30388f5 --- /dev/null +++ b/apps/e2e/demo-e2e-dts-consumer/fixture/output-schemas-must-compile.ts @@ -0,0 +1,38 @@ +import 'reflect-metadata'; + +import { App, FrontMcp, Tool, ToolContext, z } from '@frontmcp/sdk'; + +const searchHit = z.union([ + z.object({ kind: z.literal('document'), title: z.string() }), + z.object({ kind: z.literal('person'), name: z.string() }), +]); + +@Tool({ name: 'search', inputSchema: { query: z.string() }, outputSchema: searchHit }) +export class SearchTool extends ToolContext { + async execute(input: { query: string }) { + return { kind: 'document' as const, title: input.query }; + } +} + +const principal = z.discriminatedUnion('kind', [ + z.object({ kind: z.literal('user'), id: z.string() }), + z.object({ kind: z.literal('group'), members: z.array(z.string()) }), +]); + +@Tool({ + name: 'resolve_principal', + inputSchema: {}, + outputSchema: principal, + output: { undeclaredKeys: 'fail' }, +}) +export class ResolvePrincipalTool extends ToolContext { + async execute() { + return { kind: 'group' as const, members: ['u_1'] }; + } +} + +@App({ name: 'Directory', tools: [SearchTool, ResolvePrincipalTool], output: { undeclaredKeys: 'warn' } }) +export class DirectoryApp {} + +@FrontMcp({ info: { name: 'directory', version: '1.0.0' }, apps: [DirectoryApp], output: { undeclaredKeys: 'keep' } }) +export class DirectoryServer {} diff --git a/apps/e2e/demo-e2e-dts-consumer/fixture/tool-must-fail.ts b/apps/e2e/demo-e2e-dts-consumer/fixture/tool-must-fail.ts index f847e524d..d4d3dc25c 100644 --- a/apps/e2e/demo-e2e-dts-consumer/fixture/tool-must-fail.ts +++ b/apps/e2e/demo-e2e-dts-consumer/fixture/tool-must-fail.ts @@ -1,6 +1,6 @@ import 'reflect-metadata'; -import { Agent, Tool, ToolContext, z } from '@frontmcp/sdk'; +import { Tool, ToolContext, z, type Agent } from '@frontmcp/sdk'; type IsAny = 0 extends 1 & T ? true : false; @@ -33,3 +33,11 @@ export class Total extends ToolContext { return { total: '12.50 EUR' }; } } + +// @ts-expect-error output.undeclaredKeys accepts 'strip' | 'warn' | 'keep' | 'fail' only +@Tool({ name: 'drop_extra', inputSchema: {}, output: { undeclaredKeys: 'drop' } }) +export class DropExtra extends ToolContext { + async execute() { + return 1; + } +} diff --git a/apps/e2e/demo-e2e-dts-consumer/fixture/tsconfig.json b/apps/e2e/demo-e2e-dts-consumer/fixture/tsconfig.json index 282aa3e60..023df07ba 100644 --- a/apps/e2e/demo-e2e-dts-consumer/fixture/tsconfig.json +++ b/apps/e2e/demo-e2e-dts-consumer/fixture/tsconfig.json @@ -15,6 +15,7 @@ "prompts-must-compile.ts", "hooks-must-compile.ts", "host-must-compile.ts", - "skills-must-compile.ts" + "skills-must-compile.ts", + "output-schemas-must-compile.ts" ] } diff --git a/libs/lazy-zod/README.md b/libs/lazy-zod/README.md index c1cc9cecd..ed8fc1f33 100644 --- a/libs/lazy-zod/README.md +++ b/libs/lazy-zod/README.md @@ -46,6 +46,7 @@ A stand-in behaves like the schema it stands for: it parses the same, `instanceo | `lazyZ(build)` | A schema that calls `build()` the first time it's used, and only then. | | `isLazy(schema)` | `true` for a stand-in, from `z` or from `lazyZ()`. | | `forceMaterialize(schema)` | The real Zod schema, with every stand-in inside it built. | +| `hasTransform(schema)` | `true` when the schema runs a transform anywhere in its tree (`.transform()`, `z.preprocess()`, `.pipe()`, a codec), so what it parses to can differ in shape from its input. | | `toJSONSchema(schema)` | Zod's JSON Schema converter, run on `forceMaterialize(schema)`. Use it instead of Zod's own `z.toJSONSchema()`, which throws `TypeError: Cannot set properties of undefined (setting 'ref')` when an object or union in the schema is `.optional()` or has a `.default()`. | | `ZodObject`, `ZodError`, … | Zod's classes and types (`ZodTypeAny`, `infer`, `input`, `output`), so your code needs no import from `zod`. | diff --git a/libs/lazy-zod/src/__tests__/has-transform.spec.ts b/libs/lazy-zod/src/__tests__/has-transform.spec.ts new file mode 100644 index 000000000..244f08d05 --- /dev/null +++ b/libs/lazy-zod/src/__tests__/has-transform.spec.ts @@ -0,0 +1,43 @@ +import { z as realZ } from 'zod'; + +import { z } from '../lazy-z'; +import { hasTransform } from '../utils'; + +describe('hasTransform', () => { + it.each([ + ['a .transform()', z.object({ a: z.string() }).transform((value) => ({ renamed: value.a }))], + ['a z.preprocess()', z.preprocess((value) => value, z.object({ a: z.string() }))], + ['a .pipe()', z.string().pipe(z.coerce.number())], + ['a codec', z.codec(z.string(), z.number(), { decode: Number, encode: String })], + ['a transform on an object field', z.object({ at: z.string().transform((value) => new Date(value)) })], + ['a transform in an array element', z.array(z.object({ a: z.string() }).transform((value) => value.a))], + ['a transform in a union option', z.union([z.object({ a: z.string() }), z.string().transform(Number)])], + ['a transform under optional and nullable', z.object({ a: z.string().transform(Number).optional().nullable() })], + ['a transform in a record value', z.record(z.string(), z.string().transform(Number))], + ['a transform behind z.lazy()', z.object({ next: z.lazy(() => z.string().transform(Number)) })], + ['a real zod transform', realZ.object({ a: realZ.string() }).transform((value) => value.a)], + ])('is true for %s', (_label, schema) => { + expect(hasTransform(schema)).toBe(true); + }); + + it.each([ + ['an object', z.object({ a: z.string(), b: z.array(z.number()) })], + [ + 'a discriminated union', + z.discriminatedUnion('k', [z.object({ k: z.literal('a') }), z.object({ k: z.literal('b') })]), + ], + ['refinements and defaults', z.object({ a: z.string().refine(Boolean).default('x') })], + ['an intersection and a tuple', z.intersection(z.object({ a: z.string() }), z.tuple([z.number()]))], + ['a real zod object', realZ.object({ a: realZ.string() })], + ['something that is not a schema', { a: 'b' }], + ])('is false for %s', (_label, schema) => { + expect(hasTransform(schema)).toBe(false); + }); + + it('terminates on a recursive schema', () => { + type Node = { children: Node[] }; + const node: z.ZodType = z.object({ children: z.array(z.lazy(() => node)) }); + + expect(hasTransform(node)).toBe(false); + }); +}); diff --git a/libs/lazy-zod/src/index.ts b/libs/lazy-zod/src/index.ts index 3c56ae162..a306e63af 100644 --- a/libs/lazy-zod/src/index.ts +++ b/libs/lazy-zod/src/index.ts @@ -21,7 +21,7 @@ export { z } from './lazy-z'; export { z as default } from './lazy-z'; export { eagerZ } from './eager-z'; export { lazyZ, LazyZodSchema, LAZY_BRAND, LAZY_TARGET, type InferLazy } from './lazy-schema'; -export { isLazy, forceMaterialize } from './utils'; +export { isLazy, forceMaterialize, hasTransform } from './utils'; // JSON-Schema conversion. Kept here so consumer code never reaches past the // `@frontmcp/lazy-zod` boundary into `zod/v4` directly — one place to swap the diff --git a/libs/lazy-zod/src/utils.ts b/libs/lazy-zod/src/utils.ts index 269f1e8de..f14d3d091 100644 --- a/libs/lazy-zod/src/utils.ts +++ b/libs/lazy-zod/src/utils.ts @@ -11,6 +11,32 @@ export function isLazy(value: unknown): boolean { return (value as Record)[LAZY_BRAND] === true; } +/** + * True when a schema runs a transform anywhere in its tree: `.transform()`, `z.preprocess()`, + * `.pipe()` and codecs all parse through a pipe, so what the schema returns can differ in shape + * from what it was given. + */ +export function hasTransform(schema: unknown): boolean { + return containsPipe(schema, new WeakSet()); +} + +function containsPipe(value: unknown, seen: WeakSet): boolean { + const def = (value as { _zod?: { def?: Record } } | null | undefined)?._zod?.def; + if (!def || seen.has(def)) return false; + seen.add(def); + if (def['type'] === 'pipe') return true; + if (def['type'] === 'lazy') return containsPipe((def['getter'] as () => unknown)(), seen); + return Object.values(def).some((child) => nestedSchemas(child).some((nested) => containsPipe(nested, seen))); +} + +/** The schemas a definition field can hold: one schema, a list (options, items) or a shape. */ +function nestedSchemas(field: unknown): unknown[] { + if (Array.isArray(field)) return field; + const isShape = field !== null && typeof field === 'object' && (field as { _zod?: unknown })._zod === undefined; + if (isShape) return Object.values(field); + return [field]; +} + /** * Materialize a lazy schema and return the underlying real zod schema, * recursively materializing any lazy wrappers found in the schema tree diff --git a/libs/sdk/src/common/decorators/tool.decorator.ts b/libs/sdk/src/common/decorators/tool.decorator.ts index f73b939da..e4101e6d2 100644 --- a/libs/sdk/src/common/decorators/tool.decorator.ts +++ b/libs/sdk/src/common/decorators/tool.decorator.ts @@ -200,8 +200,8 @@ type __StructuredOutputType = | z.ZodRawShape | z.ZodObject | z.ZodArray - | z.ZodUnion<[z.ZodObject, ...z.ZodObject[]]> - | z.ZodDiscriminatedUnion<[z.ZodObject, ...z.ZodObject[]]>; + | z.ZodUnion, ...z.ZodObject[]]> + | z.ZodDiscriminatedUnion, ...z.ZodObject[]]>; type __ToolSingleOutputType = | __PrimitiveOutputType diff --git a/libs/sdk/src/common/entries/tool.entry.ts b/libs/sdk/src/common/entries/tool.entry.ts index 9ea5d97bc..c9e2e99fc 100644 --- a/libs/sdk/src/common/entries/tool.entry.ts +++ b/libs/sdk/src/common/entries/tool.entry.ts @@ -15,7 +15,7 @@ import type ProviderRegistry from '../../provider/provider.registry'; import { type ToolInputOf, type ToolOutputOf } from '../decorators'; import { type ToolContext } from '../interfaces'; import { type ProviderRegistryInterface } from '../interfaces/internal'; -import { type ToolInputType, type ToolMetadata, type ToolOutputType } from '../metadata'; +import { type ToolInputType, type ToolMetadata, type ToolOutputType, type UndeclaredKeysPolicy } from '../metadata'; import { type ToolRecord } from '../records'; import { BaseEntry, type EntryOwnerRef } from './base.entry'; @@ -42,6 +42,51 @@ export type ToolCallExtra = RequestHandlerExtra & { export type ParsedToolResult = CallToolResult; export type SafeTransformResult = { success: true; data: T } | { success: false; error: Error }; +/** How a tool's answer is parsed against its `outputSchema`. */ +export interface OutputParseOptions { + /** What happens to keys the answer carries that the schema does not declare. @default 'strip' */ + undeclaredKeys?: UndeclaredKeysPolicy; + /** Called with the undeclared key paths an answer carried, under the `'warn'` policy. */ + onUndeclaredKeys?: (keyPaths: string[]) => void; +} + +type OutputJsonSchemaResolution = { schema: Record | null; unlistedReason?: string }; + +/** Zod output types that are primitive outputs: they flow through `content` and are never listed. */ +const PRIMITIVE_ZOD_OUTPUT_TYPES = new Set(['string', 'number', 'boolean', 'bigint', 'date']); + +/** How zod refers to a schema registered with an id, or to a reused one: `#/$defs/`. */ +const DEFINITION_REF_PREFIX = '#/$defs/'; + +/** + * The JSON Schema MCP accepts as a tool's `outputSchema`: an object schema as is, or a union + * (`anyOf` / `oneOf`) or `$ref` that resolves to object schemas only, typed `object` at the top level. + */ +function asObjectOutputSchema(json: Record): Record | null { + if (json['type'] === 'object') return json; + return isObjectSchema(json, json['$defs'], new Set()) ? { type: 'object', ...json } : null; +} + +/** True for an object schema, a `#/$defs/` reference to one, or a union of those. */ +function isObjectSchema(schema: unknown, definitions: unknown, followedRefs: ReadonlySet): boolean { + if (typeof schema !== 'object' || schema === null) return false; + const node = schema as Record; + if (node['type'] === 'object') return true; + const ref = node['$ref']; + if (typeof ref === 'string') { + // Each `#/$defs/` reference is followed once: a recursive union can loop a validator forever + if (!ref.startsWith(DEFINITION_REF_PREFIX) || followedRefs.has(ref)) return false; + const definition = (definitions as Record | undefined)?.[ref.slice(DEFINITION_REF_PREFIX.length)]; + return isObjectSchema(definition, definitions, new Set([...followedRefs, ref])); + } + const members = node['anyOf'] ?? node['oneOf']; + return ( + Array.isArray(members) && + members.length > 0 && + members.every((member) => isObjectSchema(member, definitions, followedRefs)) + ); +} + export abstract class ToolEntry< InSchema extends ToolInputType = ToolInputType, OutSchema extends ToolOutputType = ToolOutputType, @@ -145,38 +190,47 @@ export abstract class ToolEntry< return null; } - /** Cached output JSON Schema result (undefined = not yet computed, null = none) */ - private _cachedOutputJsonSchema?: Record | null; + /** The output JSON Schema and, when a declared schema is left out of tools/list, why (undefined = not yet computed) */ + private outputJsonSchemaResolution?: OutputJsonSchemaResolution; /** * Get the tool's output schema as JSON Schema (cached after first call). * * Prefers an explicit `rawOutputSchema` (the JSON Schema set by the OpenAPI adapter / - * remote tools); otherwise converts a Zod-shape or `z.object()` `outputSchema` to JSON - * Schema — symmetric with {@link getInputJsonSchema}. Returns `null` for output forms - * that have no object-typed schema to advertise: primitive / media string literals - * (`'string'`, `'image'`, …), multi-content arrays (`['string', 'image']`), and any - * schema that does not serialize to a top-level `type: 'object'` (e.g. a union) — those - * flow through `content`, not `structuredContent`, per the MCP spec. + * remote tools); otherwise converts a Zod-shape or Zod `outputSchema` to JSON Schema — + * symmetric with {@link getInputJsonSchema}. MCP requires a top-level `type: 'object'`, so + * an object schema is returned as is and a union whose members are all object schemas is + * returned typed `object` (`{ type: 'object', anyOf | oneOf }`). Returns `null` for + * primitive / media outputs and multi-content arrays, which flow through `content`, and for + * any other schema (see {@link getUnlistedOutputSchemaReason}). * * @returns JSON Schema object, or null if there is no advertisable output schema. */ getOutputJsonSchema(): Record | null { - if (this._cachedOutputJsonSchema !== undefined) return this._cachedOutputJsonSchema; + return this.resolveOutputJsonSchema().schema; + } - const result = this.computeOutputJsonSchema(); - this._cachedOutputJsonSchema = result; - return result; + /** + * Why the declared `outputSchema` is left out of tools/list, or undefined when it is listed + * or is a primitive / media / multi-content output, which is never listed by design. + */ + getUnlistedOutputSchemaReason(): string | undefined { + return this.resolveOutputJsonSchema().unlistedReason; + } + + private resolveOutputJsonSchema(): OutputJsonSchemaResolution { + this.outputJsonSchemaResolution ??= this.computeOutputJsonSchema(); + return this.outputJsonSchemaResolution; } - private computeOutputJsonSchema(): Record | null { + private computeOutputJsonSchema(): OutputJsonSchemaResolution { // Prefer rawOutputSchema when it is already a JSON Schema object (OpenAPI / remote passthrough). if ( typeof this.rawOutputSchema === 'object' && this.rawOutputSchema !== null && !Array.isArray(this.rawOutputSchema) ) { - return this.rawOutputSchema as Record; + return { schema: this.rawOutputSchema as Record }; } const schema = this.outputSchema as unknown; @@ -184,36 +238,37 @@ export abstract class ToolEntry< // Primitive / media literals ('string' | 'number' | 'image' | …) and multi-content // arrays (['string', 'image']) have no object outputSchema — they flow via `content`. if (!schema || typeof schema === 'string' || Array.isArray(schema)) { - return null; + return { schema: null }; } - try { - // Duck-type a Zod schema (z.object(), z.discriminatedUnion(), …) vs a raw shape - // ({ field: z.string() }); identity-based instanceof is avoided so a duplicated zod - // module copy cannot break detection. - const isZodSchema = - typeof (schema as { safeParse?: unknown }).safeParse === 'function' && - typeof (schema as { parse?: unknown }).parse === 'function'; - - if (!isZodSchema && Object.keys(schema as Record).length === 0) { - // Empty raw shape — nothing meaningful to advertise. - return null; - } + // Duck-type a Zod schema (z.object(), z.discriminatedUnion(), …) vs a raw shape + // ({ field: z.string() }); identity-based instanceof is avoided so a duplicated zod + // module copy cannot break detection. + const isZodSchema = + typeof (schema as { safeParse?: unknown }).safeParse === 'function' && + typeof (schema as { parse?: unknown }).parse === 'function'; + + if (!isZodSchema && Object.keys(schema as Record).length === 0) { + // Empty raw shape — nothing meaningful to advertise. + return { schema: null }; + } + + if (isZodSchema && PRIMITIVE_ZOD_OUTPUT_TYPES.has((schema as { type?: unknown }).type)) { + return { schema: null }; + } + try { // `schema` is a Zod type (z.object()/discriminatedUnion/…) or a raw shape at // runtime, but `unknown` to TS. Narrow to the Zod-object type both z.object() and // toJSONSchema() accept (the raw-shape cast mirrors the inputSchema path above). const zodSchema = isZodSchema ? (schema as ReturnType) : z.object(schema as ToolInputType); - const json = toJSONSchema(zodSchema) as Record; - - // MCP requires outputSchema to be a top-level object schema; skip anything else - // (e.g. a discriminated union serializes to `anyOf`, not `type: 'object'`). - return json && json['type'] === 'object' ? json : null; + const objectSchema = asObjectOutputSchema(toJSONSchema(zodSchema) as Record); + return objectSchema + ? { schema: objectSchema } + : { schema: null, unlistedReason: 'it is not an object schema or a union of object schemas' }; } catch (error) { - if (isDebug() || isDevelopment()) { - console.warn('[ToolEntry] Failed to convert output Zod schema to JSON Schema:', error); - } - return null; + const message = error instanceof Error ? error.message : String(error); + return { schema: null, unlistedReason: `it cannot be converted to JSON Schema (${message})` }; } } @@ -231,10 +286,13 @@ export abstract class ToolEntry< * Convert the raw tool return value (Out) into an MCP CallToolResult-shaped object. * Concrete logic is implemented in ToolInstance. */ - abstract parseOutput(result: Out | Partial | any): ParsedToolResult; + abstract parseOutput(result: Out | Partial | any, options?: OutputParseOptions): ParsedToolResult; /** * Convert the raw tool return value (Out) into an MCP CallToolResult-shaped object. * Concrete logic is implemented in ToolInstance. */ - abstract safeParseOutput(raw: Out | Partial | any): SafeTransformResult; + abstract safeParseOutput( + raw: Out | Partial | any, + options?: OutputParseOptions, + ): SafeTransformResult; } diff --git a/libs/sdk/src/common/metadata/__tests__/output-policy.spec.ts b/libs/sdk/src/common/metadata/__tests__/output-policy.spec.ts index cee5be394..9c83ff674 100644 --- a/libs/sdk/src/common/metadata/__tests__/output-policy.spec.ts +++ b/libs/sdk/src/common/metadata/__tests__/output-policy.spec.ts @@ -196,6 +196,88 @@ describe('output-policy', () => { const out = formatOutputSchemaForDescription({ type: 'array', items: { type: 'number' } }, 'summary'); expect(out).toBe('\n\n**Returns:** `number[]`'); }); + + it('renders each member of an object union', () => { + const schema = { + type: 'object', + anyOf: [ + { type: 'object', properties: { id: { type: 'string' } }, required: ['id'] }, + { type: 'object', properties: { error: { type: 'string' } } }, + ], + }; + + const out = formatOutputSchemaForDescription(schema, 'summary'); + + expect(out).toBe( + '\n\n**Returns** one of:\n- Option 1:\n - `id`: string (required)\n- Option 2:\n - `error`: string (optional)', + ); + }); + + it('follows $ref members of a oneOf union into the root $defs', () => { + const schema = { + type: 'object', + oneOf: [{ $ref: '#/$defs/Invoice' }, { $ref: '#/$defs/Refund' }], + $defs: { + Invoice: { type: 'object', properties: { total: { type: 'number' } }, required: ['total'] }, + Refund: { type: 'object', properties: { reason: { type: 'string' } } }, + }, + }; + + const out = formatOutputSchemaForDescription(schema, 'summary'); + + expect(out).toContain('- Option 1:\n - `total`: number (required)'); + expect(out).toContain('- Option 2:\n - `reason`: string (optional)'); + }); + + it('follows a root $ref into the root $defs', () => { + const schema = { + $ref: '#/$defs/Invoice', + $defs: { Invoice: { type: 'object', properties: { total: { type: 'number' } }, required: ['total'] } }, + }; + + expect(formatOutputSchemaForDescription(schema, 'summary')).toBe( + '\n\n**Returns:**\n- `total`: number (required)', + ); + }); + + it('renders the shared properties and each union member when the root has both', () => { + const schema = { + type: 'object', + properties: { kind: { type: 'string' } }, + required: ['kind'], + oneOf: [ + { type: 'object', properties: { id: { type: 'string' } } }, + { type: 'object', properties: { error: { type: 'string' } } }, + ], + }; + + expect(formatOutputSchemaForDescription(schema, 'summary')).toBe( + '\n\n**Returns:**\n- `kind`: string (required)\n\nand one of:\n' + + '- Option 1:\n - `id`: string (optional)\n- Option 2:\n - `error`: string (optional)', + ); + }); + + it('leaves the summary out when the root has properties and a union member cannot be summarized', () => { + const schema = { + type: 'object', + properties: { kind: { type: 'string' } }, + anyOf: [{ type: 'object', properties: { id: { type: 'string' } } }, { type: 'string' }], + }; + + expect(formatOutputSchemaForDescription(schema, 'summary')).toBe(''); + }); + + it('leaves the summary out when a union member cannot be summarized', () => { + const schema = { + type: 'object', + anyOf: [ + { type: 'object', properties: { id: { type: 'string' } } }, + { $ref: 'https://example.com/remote.json' }, + ], + }; + + expect(formatOutputSchemaForDescription(schema, 'summary')).toBe(''); + }); }); describe('formatOutputSchemaForDescription — jsonSchema', () => { diff --git a/libs/sdk/src/common/metadata/front-mcp.metadata.ts b/libs/sdk/src/common/metadata/front-mcp.metadata.ts index fc053c161..623278b36 100644 --- a/libs/sdk/src/common/metadata/front-mcp.metadata.ts +++ b/libs/sdk/src/common/metadata/front-mcp.metadata.ts @@ -323,6 +323,9 @@ export interface FrontMcpBaseMetadata { * `InvalidOutputError`, surfacing silent `JSON.stringify` corruption. * - `schemaMode`: how `outputSchema` is exposed in `tools/list` — `'definition'` * (default) / `'description'` / `'both'` / `'none'`. + * - `undeclaredKeys`: what `tools/call` does with keys an answer carries that its + * `outputSchema` does not declare — `'strip'` / `'warn'` / `'keep'` / `'fail'`. Default + * `'warn'` when `NODE_ENV` is `'development'`, otherwise `'strip'`. * * @default { allowNonFinite: false, schemaMode: 'definition' } */ diff --git a/libs/sdk/src/common/metadata/output-policy.ts b/libs/sdk/src/common/metadata/output-policy.ts index 18d14614b..7c50b0947 100644 --- a/libs/sdk/src/common/metadata/output-policy.ts +++ b/libs/sdk/src/common/metadata/output-policy.ts @@ -1,5 +1,6 @@ // file: libs/sdk/src/common/metadata/output-policy.ts import { z } from '@frontmcp/lazy-zod'; +import { isDevelopment } from '@frontmcp/utils'; /** * How a tool's output schema is exposed to clients in `tools/list`. @@ -28,6 +29,22 @@ export type OutputSchemaMode = (typeof OUTPUT_SCHEMA_MODES)[number]; export const SCHEMA_DESCRIPTION_FORMATS = ['summary', 'jsonSchema'] as const; export type SchemaDescriptionFormat = (typeof SCHEMA_DESCRIPTION_FORMATS)[number]; +/** + * What `tools/call` does with keys an answer carries that its `outputSchema` does not declare. + * + * - `'strip'`: drop them. + * - `'warn'`: drop them and log a warning naming each key once per tool (up to 100 keys per tool). + * - `'keep'`: send them in `structuredContent` and the text content; `tools/list` then lists the + * schema's objects open to undeclared keys. + * - `'fail'`: refuse the answer with `InvalidOutputError` naming the keys. + * + * A call that renders the tool's UI sends declared keys only, whatever the policy, and so does an + * `outputSchema` with a transform (`.transform()`, `z.preprocess()`, `.pipe()`), whose output cannot + * be compared with the answer key by key. + */ +export const UNDECLARED_KEYS_POLICIES = ['strip', 'warn', 'keep', 'fail'] as const; +export type UndeclaredKeysPolicy = (typeof UNDECLARED_KEYS_POLICIES)[number]; + /** Default exposure mode when unset at every level. */ export const DEFAULT_OUTPUT_SCHEMA_MODE: OutputSchemaMode = 'definition'; /** Default description rendering when unset at every level. */ @@ -60,6 +77,14 @@ export interface OutputPolicy { * @default 'summary' */ schemaDescriptionFormat?: SchemaDescriptionFormat; + + /** + * What `tools/call` does with keys an answer carries that its `outputSchema` does not + * declare. See {@link UndeclaredKeysPolicy}. + * + * @default 'warn' when NODE_ENV is 'development', otherwise 'strip' + */ + undeclaredKeys?: UndeclaredKeysPolicy; } /** @@ -70,6 +95,7 @@ export const outputPolicySchema = z.object({ allowNonFinite: z.boolean().optional(), schemaMode: z.enum(OUTPUT_SCHEMA_MODES).optional(), schemaDescriptionFormat: z.enum(SCHEMA_DESCRIPTION_FORMATS).optional(), + undeclaredKeys: z.enum(UNDECLARED_KEYS_POLICIES).optional(), }); /** Resolve the effective output-schema mode: Tool > App > server > default. */ @@ -95,6 +121,26 @@ export function resolveSchemaDescriptionFormat( ); } +/** Resolve the undeclared-keys policy: Tool > App > server > 'warn' in development, 'strip' otherwise. */ +export function resolveUndeclaredKeysPolicy( + tool?: OutputPolicy, + app?: OutputPolicy, + server?: OutputPolicy, +): UndeclaredKeysPolicy { + return tool?.undeclaredKeys ?? app?.undeclaredKeys ?? server?.undeclaredKeys ?? (isDevelopment() ? 'warn' : 'strip'); +} + +/** A copy of a JSON Schema whose objects accept undeclared keys: every `additionalProperties: false` is left out. */ +export function withUndeclaredKeysAllowed(schema: unknown): unknown { + if (Array.isArray(schema)) return schema.map(withUndeclaredKeysAllowed); + if (typeof schema !== 'object' || schema === null) return schema; + return Object.fromEntries( + Object.entries(schema) + .filter(([keyword, value]) => !(keyword === 'additionalProperties' && value === false)) + .map(([keyword, value]) => [keyword, withUndeclaredKeysAllowed(value)]), + ); +} + /** Render a JSON Schema's type as a short string (e.g. `string`, `number[]`, `a | b`). */ function schemaTypeOf(schema: Record): string { const type = schema['type']; @@ -120,15 +166,45 @@ export function formatOutputSchemaForDescription( return `\n\n**Output schema:**\n\`\`\`json\n${JSON.stringify(schema, null, 2)}\n\`\`\``; } - const properties = schema['properties'] as Record> | undefined; - if (!properties || Object.keys(properties).length === 0) { - return `\n\n**Returns:** \`${schemaTypeOf(schema)}\``; + const target = resolveLocalRef(schema, schema) ?? schema; + const lines = propertyLinesOf(target); + const members = target['anyOf'] ?? target['oneOf']; + if (!Array.isArray(members)) { + return lines ? `\n\n**Returns:**\n${lines.join('\n')}` : `\n\n**Returns:** \`${schemaTypeOf(target)}\``; } + const memberLines = members.map((member: Record) => { + const resolvedMember = resolveLocalRef(member, schema); + return resolvedMember ? propertyLinesOf(resolvedMember) : undefined; + }); + if (memberLines.some((memberLine) => memberLine === undefined)) return ''; + + const options = memberLines.map( + (memberLine, index) => `- Option ${index + 1}:\n${(memberLine ?? []).map((line) => ` ${line}`).join('\n')}`, + ); + const heading = lines ? `**Returns:**\n${lines.join('\n')}\n\nand one of:` : '**Returns** one of:'; + return `\n\n${heading}\n${options.join('\n')}`; +} + +function propertyLinesOf(schema: Record): string[] | undefined { + const properties = schema['properties'] as Record> | undefined; + if (!properties || Object.keys(properties).length === 0) return undefined; + const required = new Set(Array.isArray(schema['required']) ? (schema['required'] as string[]) : []); - const lines = Object.entries(properties).map(([key, prop]) => { + return Object.entries(properties).map(([key, prop]) => { const desc = typeof prop['description'] === 'string' ? ` — ${prop['description']}` : ''; return `- \`${key}\`: ${schemaTypeOf(prop)} (${required.has(key) ? 'required' : 'optional'})${desc}`; }); - return `\n\n**Returns:**\n${lines.join('\n')}`; +} + +function resolveLocalRef( + schema: Record, + root: Record, +): Record | undefined { + const ref = schema['$ref']; + if (typeof ref !== 'string') return schema; + const definitionsPrefix = '#/$defs/'; + if (!ref.startsWith(definitionsPrefix)) return undefined; + const definitions = root['$defs'] as Record> | undefined; + return definitions?.[ref.slice(definitionsPrefix.length)]; } diff --git a/libs/sdk/src/common/metadata/tool.metadata.ts b/libs/sdk/src/common/metadata/tool.metadata.ts index 0f6707b57..457689d78 100644 --- a/libs/sdk/src/common/metadata/tool.metadata.ts +++ b/libs/sdk/src/common/metadata/tool.metadata.ts @@ -236,8 +236,8 @@ type StructuredOutputType = | z.ZodRawShape | z.ZodObject | z.ZodArray - | z.ZodUnion<[z.ZodObject, ...z.ZodObject[]]> - | z.ZodDiscriminatedUnion<[z.ZodObject, ...z.ZodObject[]]>; + | z.ZodUnion, ...z.ZodObject[]]> + | z.ZodDiscriminatedUnion, ...z.ZodObject[]]>; export type ToolSingleOutputType = | PrimitiveOutputType @@ -504,9 +504,10 @@ export interface ToolMetadata { }); }); + describe('with original schema whose references point into its $defs', () => { + const registeredUnion: Record = { + type: 'object', + anyOf: [{ $ref: '#/$defs/Document' }, { $ref: '#/$defs/Person' }], + $defs: { + Document: { type: 'object', properties: { title: { type: 'string' } } }, + Person: { type: 'object', properties: { name: { type: 'string' } } }, + }, + }; + + it('keeps $defs at the root, where the references resolve from', () => { + const extended = extendOutputSchemaForElicitation(registeredUnion); + + expect(extended['$defs']).toEqual(registeredUnion['$defs']); + expect(extended['oneOf']).toEqual([ + { type: 'object', anyOf: registeredUnion['anyOf'] }, + ELICITATION_FALLBACK_JSON_SCHEMA, + ]); + }); + }); + describe('MCP SDK ToolSchema compatibility', () => { function buildToolDescriptor(outputSchema: Record) { return { diff --git a/libs/sdk/src/elicitation/helpers/extend-output-schema.ts b/libs/sdk/src/elicitation/helpers/extend-output-schema.ts index bf8b9900e..a5438fd80 100644 --- a/libs/sdk/src/elicitation/helpers/extend-output-schema.ts +++ b/libs/sdk/src/elicitation/helpers/extend-output-schema.ts @@ -70,8 +70,11 @@ export function extendOutputSchemaForElicitation( // Wrap in oneOf: either original output OR elicitation fallback // type: 'object' is required by the MCP spec's ToolSchema for outputSchema + // $defs stays at the root, where the original's `#/$defs/...` references resolve from + const { $defs: definitions, ...wrappedSchema } = originalSchema; return { type: 'object', - oneOf: [originalSchema, ELICITATION_FALLBACK_JSON_SCHEMA], + oneOf: [wrappedSchema, ELICITATION_FALLBACK_JSON_SCHEMA], + ...(definitions !== undefined && { $defs: definitions }), }; } diff --git a/libs/sdk/src/scope/scope.instance.ts b/libs/sdk/src/scope/scope.instance.ts index e333e035d..e9e8533ff 100644 --- a/libs/sdk/src/scope/scope.instance.ts +++ b/libs/sdk/src/scope/scope.instance.ts @@ -71,6 +71,7 @@ import { pluginsRegisterDynamicSkills, registerSkillCapabilities } from '../skil import SkillRegistry from '../skill/skill.registry'; import type { InProcessTaskRunner, TaskRegistry, TaskStore } from '../task'; import { resolvesToVercelKvTaskBackend } from '../task/store/task-backend.utils'; +import { describeUnlistedOutputSchemas } from '../tool/tool-output-policy'; import { ToolInstance } from '../tool/tool.instance'; import ToolRegistry from '../tool/tool.registry'; import { normalizeTool } from '../tool/tool.utils'; @@ -692,6 +693,9 @@ export class Scope extends ScopeEntry { // Fail-fast: entries with metadata only a plugin enforces (`approval`, `featureFlag`, ...) that no such plugin covers this.validateEnforcedMetadata(); + // One warning per tool whose declared outputSchema tools/list has to leave out + for (const message of describeUnlistedOutputSchemas(this)) this.logger.warn(message); + await this.initGuardForDeclaredLimits(); // ═══ BATCH 3: Cross-registry finalization (sequential) ═══ diff --git a/libs/sdk/src/tool/__tests__/tool-instance.output-schema.spec.ts b/libs/sdk/src/tool/__tests__/tool-instance.output-schema.spec.ts new file mode 100644 index 000000000..80ba84283 --- /dev/null +++ b/libs/sdk/src/tool/__tests__/tool-instance.output-schema.spec.ts @@ -0,0 +1,296 @@ +import { z } from '@frontmcp/lazy-zod'; + +import { createMockOwner, createMockProviderRegistry } from '../../__test-utils__/mocks'; +import { ToolContext, ToolKind, type ToolMetadata, type ToolRecord } from '../../common'; +import { InvalidOutputError } from '../../errors'; +import { ToolInstance } from '../tool.instance'; + +class AnswerTool extends ToolContext { + async execute() { + return {}; + } +} + +function toolWithOutput(outputSchema: unknown): { tool: ToolInstance } { + const record = { + kind: ToolKind.CLASS_TOKEN, + provide: AnswerTool, + metadata: { name: 'answer', inputSchema: {}, outputSchema } as unknown as ToolMetadata, + } as unknown as ToolRecord; + return { tool: new ToolInstance(record, createMockProviderRegistry(), createMockOwner()) }; +} + +const documentHit = z.object({ kind: z.literal('document'), title: z.string() }); +const personHit = z.object({ kind: z.literal('person'), name: z.string() }); + +describe('ToolInstance output JSON Schema', () => { + it('lists a union of object schemas as an object schema with anyOf', () => { + const { tool } = toolWithOutput(z.union([documentHit, personHit])); + + expect(tool.getOutputJsonSchema()).toMatchObject({ + type: 'object', + anyOf: [ + { type: 'object', required: ['kind', 'title'] }, + { type: 'object', required: ['kind', 'name'] }, + ], + }); + expect(tool.getUnlistedOutputSchemaReason()).toBeUndefined(); + }); + + it('lists a discriminated union as an object schema with oneOf', () => { + const { tool } = toolWithOutput(z.discriminatedUnion('kind', [documentHit, personHit])); + + expect(tool.getOutputJsonSchema()).toMatchObject({ + type: 'object', + oneOf: [{ type: 'object' }, { type: 'object' }], + }); + }); + + it('lists a union whose members are unions of object schemas', () => { + const nested = z.union([z.union([documentHit, personHit]), z.object({ kind: z.literal('team') })]); + + expect(toolWithOutput(nested).tool.getOutputJsonSchema()).toMatchObject({ type: 'object' }); + }); + + it('lists a union whose members are object schemas registered with an id', () => { + const registeredUnion = z.union([ + documentHit.meta({ id: 'ToolInstanceSpecDocument' }), + personHit.meta({ id: 'ToolInstanceSpecPerson' }), + ]); + const { tool } = toolWithOutput(registeredUnion); + + expect(tool.getOutputJsonSchema()).toMatchObject({ + type: 'object', + anyOf: [{ $ref: '#/$defs/ToolInstanceSpecDocument' }, { $ref: '#/$defs/ToolInstanceSpecPerson' }], + $defs: { ToolInstanceSpecDocument: { type: 'object' }, ToolInstanceSpecPerson: { type: 'object' } }, + }); + expect(tool.getUnlistedOutputSchemaReason()).toBeUndefined(); + }); + + it('lists an object schema registered with an id', () => { + const { tool } = toolWithOutput(documentHit.meta({ id: 'ToolInstanceSpecHit' })); + + expect(tool.getOutputJsonSchema()).toMatchObject({ type: 'object', $ref: '#/$defs/ToolInstanceSpecHit' }); + }); + + it('leaves out a union of a registered object schema and a non-object member', () => { + const { tool } = toolWithOutput(z.union([documentHit.meta({ id: 'ToolInstanceSpecMixed' }), z.string()])); + + expect(tool.getOutputJsonSchema()).toBeNull(); + expect(tool.getUnlistedOutputSchemaReason()).toContain('not an object schema or a union of object schemas'); + }); + + it('leaves out a recursive union instead of following its references forever', () => { + const recursiveUnion: z.ZodType = z.union([z.object({ leaf: z.string() }), z.lazy(() => recursiveUnion)]); + const outerUnion = z.union([z.object({ root: z.string() }), recursiveUnion]); + + expect(toolWithOutput(recursiveUnion).tool.getOutputJsonSchema()).toBeNull(); + expect(toolWithOutput(outerUnion).tool.getOutputJsonSchema()).toBeNull(); + expect(toolWithOutput(outerUnion).tool.getUnlistedOutputSchemaReason()).toContain('not an object schema'); + }); + + it.each([ + ['a union with a non-object member', z.union([documentHit, z.string()])], + ['a nullable object', documentHit.nullable()], + ['an array of objects', z.array(documentHit)], + ])('leaves out %s and says why', (_label, outputSchema) => { + const { tool } = toolWithOutput(outputSchema); + + expect(tool.getOutputJsonSchema()).toBeNull(); + expect(tool.getUnlistedOutputSchemaReason()).toBe('it is not an object schema or a union of object schemas'); + }); + + it('says why when the output schema cannot be converted to JSON Schema', () => { + const { tool } = toolWithOutput(z.object({ at: z.date() })); + + expect(tool.getOutputJsonSchema()).toBeNull(); + expect(tool.getUnlistedOutputSchemaReason()).toContain('Date cannot be represented in JSON Schema'); + }); + + it.each([ + ['a primitive literal', 'number'], + ['a Zod primitive', z.string()], + ['a Zod date', z.date()], + ['a media literal', 'image'], + ['a multi-content array', ['string', 'image']], + ['an empty raw shape', {}], + ['no output schema', undefined], + ])('lists nothing for %s, by design and with no reason to report', (_label, outputSchema) => { + const { tool } = toolWithOutput(outputSchema); + + expect(tool.getOutputJsonSchema()).toBeNull(); + expect(tool.getUnlistedOutputSchemaReason()).toBeUndefined(); + }); +}); + +describe('ToolInstance.parseOutput and undeclared keys', () => { + const invoiceShape = { id: z.string(), lines: z.array(z.object({ sku: z.string() })) }; + const answer = { + id: 'INV-1', + costCenter: 'CC-7', + lines: [ + { sku: 'A', margin: 0.4 }, + { sku: 'B', margin: 0.1 }, + ], + }; + + it("strips undeclared keys by default and under 'strip'", () => { + const { tool } = toolWithOutput(invoiceShape); + const declared = { id: 'INV-1', lines: [{ sku: 'A' }, { sku: 'B' }] }; + + expect(tool.parseOutput(answer).structuredContent).toEqual(declared); + expect(tool.parseOutput(answer, { undeclaredKeys: 'strip' }).structuredContent).toEqual(declared); + }); + + it("strips under 'warn' and reports each undeclared key path once", () => { + const { tool } = toolWithOutput(invoiceShape); + const onUndeclaredKeys = jest.fn(); + + const result = tool.parseOutput(answer, { undeclaredKeys: 'warn', onUndeclaredKeys }); + + expect(result.structuredContent).toEqual({ id: 'INV-1', lines: [{ sku: 'A' }, { sku: 'B' }] }); + expect(onUndeclaredKeys).toHaveBeenCalledWith(['costCenter', 'lines[].margin']); + }); + + it("does not report under 'warn' when the answer has no undeclared key", () => { + const { tool } = toolWithOutput(invoiceShape); + const onUndeclaredKeys = jest.fn(); + + tool.parseOutput({ id: 'INV-2', lines: [] }, { undeclaredKeys: 'warn', onUndeclaredKeys }); + + expect(onUndeclaredKeys).not.toHaveBeenCalled(); + }); + + it("passes undeclared keys through to structuredContent and the text under 'keep'", () => { + const { tool } = toolWithOutput(invoiceShape); + + const result = tool.parseOutput(answer, { undeclaredKeys: 'keep' }); + + expect(result.structuredContent).toEqual(answer); + expect(JSON.parse((result.content[0] as { text: string }).text)).toEqual(answer); + }); + + it("refuses the answer under 'fail' with InvalidOutputError naming the keys", () => { + const { tool } = toolWithOutput(invoiceShape); + + const parsed = tool.safeParseOutput(answer, { undeclaredKeys: 'fail' }); + + expect(parsed.success).toBe(false); + const error = (parsed as { error: Error }).error; + expect(error).toBeInstanceOf(InvalidOutputError); + expect(error.message).toContain('costCenter, lines[].margin'); + }); + + it("accepts an answer with declared keys only under 'fail'", () => { + const { tool } = toolWithOutput(invoiceShape); + + expect(tool.safeParseOutput({ id: 'INV-2', lines: [{ sku: 'A' }] }, { undeclaredKeys: 'fail' })).toMatchObject({ + success: true, + }); + }); + + it('applies the policy to a union output schema', () => { + const { tool } = toolWithOutput(z.union([documentHit, personHit])); + const hit = { kind: 'person', name: 'Ada', email: 'ada@example.com' }; + + expect(tool.parseOutput(hit).structuredContent).toEqual({ kind: 'person', name: 'Ada' }); + expect(tool.parseOutput(hit, { undeclaredKeys: 'keep' }).structuredContent).toEqual(hit); + }); + + it('keeps an undeclared key next to a non-finite number left for the finalize stage', () => { + const { tool } = toolWithOutput({ ratio: z.number() }); + + expect(tool.parseOutput({ ratio: Infinity, note: 'n/a' }, { undeclaredKeys: 'keep' }).structuredContent).toEqual({ + ratio: Infinity, + note: 'n/a', + }); + }); + + describe('with an object schema that sets its own unknown-key handling, which wins over the policy', () => { + const policies = ['strip', 'warn', 'keep', 'fail'] as const; + + it.each(policies)("lets z.strictObject() refuse an undeclared key under '%s'", (undeclaredKeys) => { + const { tool } = toolWithOutput(z.strictObject({ id: z.string() })); + + const parsed = tool.safeParseOutput({ id: '1', extra: true }, { undeclaredKeys }); + + expect(parsed.success).toBe(false); + expect((parsed as { error: Error }).error).toBeInstanceOf(InvalidOutputError); + }); + + it.each(policies)("lets z.looseObject() keep an undeclared key under '%s'", (undeclaredKeys) => { + const { tool } = toolWithOutput(z.looseObject({ id: z.string() })); + + expect(tool.parseOutput({ id: '1', extra: true }, { undeclaredKeys }).structuredContent).toEqual({ + id: '1', + extra: true, + }); + }); + }); + + it("refuses under 'keep' an undeclared key whose object the schema coerced to a primitive", () => { + const { tool } = toolWithOutput({ payload: z.coerce.string() }); + + const parsed = tool.safeParseOutput({ payload: { secret: 'x' } }, { undeclaredKeys: 'keep' }); + + expect(parsed.success).toBe(false); + const error = (parsed as { error: Error }).error; + expect(error).toBeInstanceOf(InvalidOutputError); + expect(error.message).toContain('payload.secret'); + }); + + describe('with a cycle under a z.any() field, which the parse keeps by reference', () => { + function cyclicAnswer() { + const meta: Record = { name: 'node' }; + meta['self'] = meta; + return { id: '1', note: 'n/a', meta }; + } + + it("reports the undeclared key under 'warn' without following the cycle", () => { + const { tool } = toolWithOutput({ id: z.string(), meta: z.any() }); + const onUndeclaredKeys = jest.fn(); + + tool.parseOutput(cyclicAnswer(), { undeclaredKeys: 'warn', onUndeclaredKeys }); + + expect(onUndeclaredKeys).toHaveBeenCalledWith(['note']); + }); + + it("keeps the undeclared key under 'keep' without following the cycle", () => { + const { tool } = toolWithOutput({ id: z.string(), meta: z.any() }); + + expect(tool.parseOutput(cyclicAnswer(), { undeclaredKeys: 'keep' }).structuredContent).toEqual({ + id: '1', + note: 'n/a', + meta: { name: 'node' }, + }); + }); + }); + + describe('with a transform, whose output cannot be compared with the answer key by key', () => { + const renamingSchema = z.object({ a: z.string() }).transform((value) => ({ renamed: value.a })); + const droppingSchema = z + .object({ id: z.string(), internal: z.string() }) + .transform(({ internal: _internal, ...rest }) => rest); + + it.each(['warn', 'keep', 'fail'] as const)( + "returns the transformed output and strips an undeclared key under '%s'", + (undeclaredKeys) => { + const { tool } = toolWithOutput(renamingSchema); + const onUndeclaredKeys = jest.fn(); + + const result = tool.parseOutput({ a: 'x', extra: 1 }, { undeclaredKeys, onUndeclaredKeys }); + + expect(result.structuredContent).toEqual({ renamed: 'x' }); + expect(onUndeclaredKeys).not.toHaveBeenCalled(); + }, + ); + + it("never puts back a key the transform dropped under 'keep'", () => { + const { tool } = toolWithOutput(droppingSchema); + + const result = tool.parseOutput({ id: '1', internal: 'secret' }, { undeclaredKeys: 'keep' }); + + expect(result.structuredContent).toEqual({ id: '1' }); + }); + }); +}); diff --git a/libs/sdk/src/tool/flows/__tests__/call-tool.output-schema.spec.ts b/libs/sdk/src/tool/flows/__tests__/call-tool.output-schema.spec.ts index 979d805b4..3017b480d 100644 --- a/libs/sdk/src/tool/flows/__tests__/call-tool.output-schema.spec.ts +++ b/libs/sdk/src/tool/flows/__tests__/call-tool.output-schema.spec.ts @@ -1,13 +1,26 @@ import 'reflect-metadata'; import { z } from '@frontmcp/lazy-zod'; +import { MCP_20260728_META } from '@frontmcp/protocol'; import { createTestFetchServer, rpc20260728, type TestFetchServer, } from '../../../__test-utils__/helpers/mcp-20260728.helpers'; -import { App, FlowHooksOf, Plugin, Tool, ToolContext, type FlowCtxOf } from '../../../common'; +import { + App, + FlowHooksOf, + LogLevel, + LogTransport, + LogTransportInterface, + Plugin, + Tool, + ToolContext, + type FlowCtxOf, + type LogRecord, + type TemplateContext, +} from '../../../common'; import { InvalidOutputError } from '../../../errors'; const ToolHook = FlowHooksOf('tools:call-tool'); @@ -138,3 +151,227 @@ describe('tools/call output schema enforcement', () => { }); }); }); + +const quoteWarnings: string[] = []; + +@LogTransport({ name: 'UndeclaredKeysCapture', description: 'Keeps warnings for assertions' }) +class WarningCapture extends LogTransportInterface { + log(record: LogRecord): void { + if (record.level === LogLevel.Warn) quoteWarnings.push(record.message); + } +} + +const quoteAnswer = { price: 10, quoteCostCenter: 'QCC-4410' }; + +@Tool({ name: 'get_quote', inputSchema: {}, outputSchema: { price: z.number() } }) +class GetQuoteTool extends ToolContext { + async execute() { + return quoteAnswer as { price: number }; + } +} + +@Tool({ + name: 'get_strict_quote', + inputSchema: {}, + outputSchema: { price: z.number() }, + output: { undeclaredKeys: 'fail' }, +}) +class GetStrictQuoteTool extends ToolContext { + async execute() { + return quoteAnswer as { price: number }; + } +} + +@Tool({ + name: 'show_quote', + inputSchema: {}, + outputSchema: { price: z.number() }, + ui: { template: (ctx: TemplateContext) => JSON.stringify(ctx.output) }, +}) +class ShowQuoteTool extends ToolContext { + async execute() { + return quoteAnswer as { price: number }; + } +} + +@Tool({ name: 'get_partner_quote', inputSchema: {}, outputSchema: { price: z.number() } }) +class GetPartnerQuoteTool extends ToolContext { + async execute() { + return quoteAnswer as { price: number }; + } +} + +@Tool({ + name: 'get_tagged_quote', + inputSchema: { tags: z.array(z.string()) }, + outputSchema: { price: z.number() }, + output: { undeclaredKeys: 'warn' }, +}) +class GetTaggedQuoteTool extends ToolContext { + async execute(input: { tags: string[] }) { + return { price: 10, ...Object.fromEntries(input.tags.map((tag) => [tag, true])) } as { price: number }; + } +} + +@App({ id: 'quotes', name: 'Quotes', tools: [GetQuoteTool, GetStrictQuoteTool, ShowQuoteTool, GetTaggedQuoteTool] }) +class QuotesApp {} + +@Tool({ name: 'get_plugin_quote', inputSchema: {}, outputSchema: { price: z.number() } }) +class GetPluginQuoteTool extends ToolContext { + async execute() { + return quoteAnswer as { price: number }; + } +} + +@Plugin({ name: 'partner-pricing', tools: [GetPluginQuoteTool] }) +class PartnerPricingPlugin {} + +@App({ + id: 'partners', + name: 'Partners', + tools: [GetPartnerQuoteTool], + plugins: [PartnerPricingPlugin], + output: { undeclaredKeys: 'keep' }, +}) +class PartnersApp {} + +describe('tools/call undeclared output keys', () => { + const originalNodeEnv = process.env['NODE_ENV']; + + afterEach(() => { + process.env['NODE_ENV'] = originalNodeEnv; + quoteWarnings.length = 0; + }); + + function startServer(output?: { undeclaredKeys?: 'strip' | 'keep' | 'warn' | 'fail' }) { + return createTestFetchServer({ + info: { name: 'undeclared-keys', version: '1.0.0' }, + apps: [QuotesApp, PartnersApp], + logging: { level: LogLevel.Warn, enableConsole: false, transports: [WarningCapture] }, + ...(output && { output }), + }); + } + + async function callQuote(target: TestFetchServer, name: string, clientInfo?: { name: string; version: string }) { + const { message } = await rpc20260728( + target.handler, + 'tools/call', + { name, arguments: {} }, + clientInfo && { meta: { [MCP_20260728_META.clientInfo]: clientInfo } }, + ); + const result = message.result ?? {}; + const resultMeta = result['_meta'] as Record | undefined; + return { + isError: result['isError'] === true, + code: resultMeta?.['code'], + structuredContent: result['structuredContent'], + wire: JSON.stringify(result), + }; + } + + it('strips undeclared keys without a warning by default outside development', async () => { + process.env['NODE_ENV'] = 'production'; + const server = await startServer(); + + expect(await callQuote(server, 'get_quote')).toMatchObject({ isError: false, structuredContent: { price: 10 } }); + expect(quoteWarnings.filter((line) => line.includes('quoteCostCenter'))).toEqual([]); + }); + + it('strips undeclared keys and warns once per tool and key set by default in development', async () => { + process.env['NODE_ENV'] = 'development'; + const server = await startServer(); + + const first = await callQuote(server, 'get_quote'); + await callQuote(server, 'get_quote'); + + expect(first).toMatchObject({ isError: false, structuredContent: { price: 10 } }); + expect(first.wire).not.toContain('QCC-4410'); + const warnings = quoteWarnings.filter((line) => line.includes('quoteCostCenter')); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('get_quote'); + }); + + async function callTaggedQuote(target: TestFetchServer, tags: string[]): Promise { + await rpc20260728(target.handler, 'tools/call', { name: 'get_tagged_quote', arguments: { tags } }); + } + + function taggedQuoteWarnings(): string[] { + return quoteWarnings.filter((line) => line.includes('get_tagged_quote')); + } + + it("names under 'warn' only the undeclared keys not reported before", async () => { + const server = await startServer(); + + await callTaggedQuote(server, ['rush']); + await callTaggedQuote(server, ['rush', 'gift']); + await callTaggedQuote(server, ['gift', 'rush']); + + expect(taggedQuoteWarnings()).toHaveLength(2); + expect(taggedQuoteWarnings()[0]).toContain('stripped: rush.'); + expect(taggedQuoteWarnings()[1]).toContain('stripped: gift.'); + }); + + it("stops reporting under 'warn' once a tool has named 100 undeclared keys", async () => { + const server = await startServer(); + const manyTags = Array.from({ length: 120 }, (_unused, index) => `tag${String(index).padStart(3, '0')}`); + + await callTaggedQuote(server, manyTags); + await callTaggedQuote(server, ['late-tag']); + + expect(taggedQuoteWarnings()).toHaveLength(1); + expect(taggedQuoteWarnings()[0]).toContain('tag099.'); + expect(taggedQuoteWarnings()[0]).not.toContain('tag100'); + }); + + it("passes undeclared keys through under a server-level 'keep'", async () => { + const server = await startServer({ undeclaredKeys: 'keep' }); + + const result = await callQuote(server, 'get_quote'); + + expect(result).toMatchObject({ isError: false, structuredContent: quoteAnswer }); + }); + + it("lets a tool's 'fail' override the server and refuse the answer with INVALID_OUTPUT", async () => { + const server = await startServer({ undeclaredKeys: 'keep' }); + + const result = await callQuote(server, 'get_strict_quote'); + + expect(result).toMatchObject({ isError: true, code: 'INVALID_OUTPUT' }); + expect(result.wire).not.toContain('QCC-4410'); + }); + + it("lets an app's policy override the server", async () => { + const server = await startServer({ undeclaredKeys: 'fail' }); + + expect(await callQuote(server, 'get_partner_quote')).toMatchObject({ structuredContent: quoteAnswer }); + expect(await callQuote(server, 'get_quote')).toMatchObject({ isError: true, code: 'INVALID_OUTPUT' }); + }); + + it("applies an app's policy to a tool a plugin of that app contributes", async () => { + const server = await startServer({ undeclaredKeys: 'fail' }); + + expect(await callQuote(server, 'get_plugin_quote')).toMatchObject({ + isError: false, + structuredContent: quoteAnswer, + }); + }); + + it.each([ + ['a client that takes structuredContent', undefined], + ['a client that takes no structuredContent', { name: 'claude-desktop', version: '1.0.0' }], + ])("renders a tool's UI from declared keys only under 'keep', for %s", async (_label, clientInfo) => { + const server = await startServer({ undeclaredKeys: 'keep' }); + + const result = await callQuote(server, 'show_quote', clientInfo); + + expect(result.isError).toBe(false); + expect(result.wire).toContain('ui/html'); + expect(result.wire).not.toContain('QCC-4410'); + }); + + it('refuses an unknown undeclaredKeys policy at boot', async () => { + await expect(startServer({ undeclaredKeys: 'drop' } as unknown as { undeclaredKeys: 'strip' })).rejects.toThrow( + /undeclaredKeys/, + ); + }); +}); diff --git a/libs/sdk/src/tool/flows/__tests__/tools-list.output-schema.spec.ts b/libs/sdk/src/tool/flows/__tests__/tools-list.output-schema.spec.ts new file mode 100644 index 000000000..b8d0e2b49 --- /dev/null +++ b/libs/sdk/src/tool/flows/__tests__/tools-list.output-schema.spec.ts @@ -0,0 +1,191 @@ +import 'reflect-metadata'; + +import { z } from '@frontmcp/lazy-zod'; + +import { createTestFetchServer, rpc20260728 } from '../../../__test-utils__/helpers/mcp-20260728.helpers'; +import { + App, + LogLevel, + LogTransport, + LogTransportInterface, + Plugin, + Tool, + ToolContext, + type LogRecord, + type OutputPolicy, +} from '../../../common'; + +const documentHit = z.object({ kind: z.literal('document'), title: z.string() }); +const personHit = z.object({ kind: z.literal('person'), name: z.string() }); + +@Tool({ name: 'search', inputSchema: {}, outputSchema: z.union([documentHit, personHit]) }) +class SearchTool extends ToolContext { + async execute() { + return { kind: 'document' as const, title: 'Roadmap' }; + } +} + +@Tool({ name: 'resolve', inputSchema: {}, outputSchema: z.discriminatedUnion('kind', [documentHit, personHit]) }) +class ResolveTool extends ToolContext { + async execute() { + return { kind: 'person' as const, name: 'Ada' }; + } +} + +@Tool({ + name: 'invoice', + inputSchema: {}, + outputSchema: { id: z.string(), lines: z.array(z.object({ sku: z.string() })) }, +}) +class InvoiceTool extends ToolContext { + async execute() { + return { id: 'INV-1', lines: [] }; + } +} + +@App({ id: 'directory', name: 'Directory', tools: [SearchTool, ResolveTool, InvoiceTool] }) +class DirectoryApp {} + +@Tool({ + name: 'archive_entry', + inputSchema: {}, + outputSchema: { id: z.string(), tags: z.array(z.object({ label: z.string() })) }, +}) +class ArchiveEntryTool extends ToolContext { + async execute() { + return { id: 'A-1', tags: [] }; + } +} + +@Plugin({ name: 'archive-index', tools: [ArchiveEntryTool] }) +class ArchiveIndexPlugin {} + +@App({ id: 'archive', name: 'Archive', plugins: [ArchiveIndexPlugin], output: { undeclaredKeys: 'keep' } }) +class ArchiveApp {} + +async function listOutputSchemas(output?: OutputPolicy): Promise | undefined>> { + const server = await createTestFetchServer({ + info: { name: 'tools-list-output-schema', version: '1.0.0' }, + apps: [DirectoryApp, ArchiveApp], + ...(output && { output }), + }); + const { message } = await rpc20260728(server.handler, 'tools/list'); + const tools = (message.result?.['tools'] as Array<{ name: string; outputSchema?: Record }>) ?? []; + return new Map(tools.map((tool) => [tool.name, tool.outputSchema])); +} + +describe('tools/list output schema', () => { + it('lists a union of object schemas with a top-level object type', async () => { + const schemas = await listOutputSchemas(); + + expect(schemas.get('search')).toMatchObject({ + type: 'object', + anyOf: [{ properties: { title: { type: 'string' } } }, { properties: { name: { type: 'string' } } }], + }); + }); + + it('lists a discriminated union with a top-level object type', async () => { + const schemas = await listOutputSchemas(); + + expect(schemas.get('resolve')).toMatchObject({ type: 'object', oneOf: [{ type: 'object' }, { type: 'object' }] }); + }); + + it('lists object schemas closed to undeclared keys by default', async () => { + const schemas = await listOutputSchemas(); + + expect(JSON.stringify(schemas.get('invoice'))).toContain('"additionalProperties":false'); + }); + + it("lists object schemas open to undeclared keys under 'keep', at every level", async () => { + const schemas = await listOutputSchemas({ undeclaredKeys: 'keep' }); + + expect(schemas.get('invoice')).toMatchObject({ type: 'object', properties: { id: { type: 'string' } } }); + expect(JSON.stringify([...schemas.values()])).not.toContain('"additionalProperties":false'); + }); + + it("lists a plugin's tool open under its app's 'keep', and other apps' tools closed", async () => { + const schemas = await listOutputSchemas(); + + expect(schemas.get('archive_entry')).toMatchObject({ type: 'object', properties: { id: { type: 'string' } } }); + expect(JSON.stringify(schemas.get('archive_entry'))).not.toContain('"additionalProperties":false'); + expect(JSON.stringify(schemas.get('invoice'))).toContain('"additionalProperties":false'); + }); +}); + +const startupWarnings: string[] = []; + +@LogTransport({ name: 'UnlistedOutputSchemaCapture', description: 'Keeps warnings for assertions' }) +class StartupWarningCapture extends LogTransportInterface { + log(record: LogRecord): void { + if (record.level === LogLevel.Warn) startupWarnings.push(record.message); + } +} + +@Tool({ name: 'list_hits', inputSchema: {}, outputSchema: z.array(documentHit) }) +class ListHitsTool extends ToolContext { + async execute() { + return []; + } +} + +@Tool({ + name: 'list_hits_quietly', + inputSchema: {}, + outputSchema: z.array(documentHit), + output: { schemaMode: 'none' }, +}) +class ListHitsQuietlyTool extends ToolContext { + async execute() { + return []; + } +} + +@Tool({ name: 'list_archived_hits', inputSchema: {}, outputSchema: z.array(documentHit) }) +class ListArchivedHitsTool extends ToolContext { + async execute() { + return []; + } +} + +@Plugin({ name: 'archived-hits', tools: [ListArchivedHitsTool] }) +class ArchivedHitsPlugin {} + +@App({ id: 'hits', name: 'Hits', tools: [ListHitsTool, ListHitsQuietlyTool, SearchTool] }) +class HitsApp {} + +@App({ id: 'quiet-hits', name: 'QuietHits', plugins: [ArchivedHitsPlugin], output: { schemaMode: 'none' } }) +class QuietHitsApp {} + +describe('startup warning for an outputSchema tools/list leaves out', () => { + afterEach(() => { + startupWarnings.length = 0; + }); + + function unlistedOutputSchemaWarnings(): string[] { + return startupWarnings.filter((line) => line.includes('leaves out its outputSchema')); + } + + async function startServer(output?: OutputPolicy) { + await createTestFetchServer({ + info: { name: 'unlisted-output-schema', version: '1.0.0' }, + apps: [HitsApp, QuietHitsApp], + logging: { level: LogLevel.Warn, enableConsole: false, transports: [StartupWarningCapture] }, + ...(output && { output }), + }); + } + + it("names the tool, once, and stays quiet for a tool or an app whose schemaMode is 'none'", async () => { + await startServer(); + + expect(unlistedOutputSchemaWarnings()).toEqual([ + 'Tool "list_hits": tools/list leaves out its outputSchema because it is not an object schema or a union of ' + + 'object schemas. Declare a z.object() or a union of z.object() schemas to list it.', + ]); + }); + + it("stays quiet for every tool when the server's schemaMode is 'none'", async () => { + await startServer({ schemaMode: 'none' }); + + expect(unlistedOutputSchemaWarnings()).toEqual([]); + }); +}); diff --git a/libs/sdk/src/tool/flows/call-tool.flow.ts b/libs/sdk/src/tool/flows/call-tool.flow.ts index 2b12cecd6..b72a12e41 100644 --- a/libs/sdk/src/tool/flows/call-tool.flow.ts +++ b/libs/sdk/src/tool/flows/call-tool.flow.ts @@ -26,6 +26,7 @@ import { FlowHooksOf, GLOBAL_RATE_LIMIT_CHECKED, isOrchestratedMode, + resolveUndeclaredKeysPolicy, ToolContext, ToolEntry, type FlowPlan, @@ -71,6 +72,7 @@ import { generateTaskId } from '../../task/helpers/task-id'; import { TaskNotifier } from '../../task/helpers/task-notifier'; import { TASK_DEFAULTS, toWireShape, type TaskRecord } from '../../task/task.types'; import { hookOwnerIdOf, isServerEntryOwner } from '../../utils/lineage.utils'; +import { toolOutputPolicies } from '../tool-output-policy'; import { toolServingMode } from '../ui/serving-mode-default'; import { hasUIConfig, WIDGET_CALL_META_KEY } from '../ui/ui-shared'; import { evaluateToolCredentialGate } from './tool-credentials.gate'; @@ -173,6 +175,8 @@ const stateSchema = z.object({ .optional(), }); +type CallToolEntry = z.infer['tool']; + const plan = { pre: [ 'parseInput', @@ -222,6 +226,12 @@ const { Stage } = FlowHooksOf<'tools:call-tool'>(name); /** Tools `resolveHookOwnerId` found, reused by the same run's `findTool`. */ const resolvedTools = new ResolvedEntries(); +/** The undeclared key paths already reported per tool, so the 'warn' policy names each one once. */ +const reportedUndeclaredKeyPaths = new WeakMap>(); + +/** Answers keyed by ids yield new key paths forever, so a tool stops reporting after this many. */ +const MAX_REPORTED_UNDECLARED_KEY_PATHS = 100; + @Flow({ name, plan, @@ -1531,7 +1541,12 @@ export default class CallToolFlow extends FlowBase { } // Parse and construct the MCP-compliant output using safeParseOutput - const parseResult = tool.safeParseOutput(rawOutput); + const undeclaredKeys = resolveUndeclaredKeysPolicy(...toolOutputPolicies(tool, this.scope)); + const parseResult = tool.safeParseOutput(rawOutput, { + // A call that renders the tool's UI sends declared keys only, whatever the policy + undeclaredKeys: uiResult && undeclaredKeys === 'keep' ? 'strip' : undeclaredKeys, + onUndeclaredKeys: (keyPaths) => this.warnUndeclaredKeysOnce(tool, keyPaths), + }); if (!parseResult.success) { this.logger.error('finalize: output validation failed', { @@ -1620,6 +1635,19 @@ export default class CallToolFlow extends FlowBase { this.respond(result); this.logger.verbose('finalize:done'); } + + private warnUndeclaredKeysOnce(tool: CallToolEntry, keyPaths: string[]): void { + const reportedKeyPaths = reportedUndeclaredKeyPaths.get(tool) ?? new Set(); + reportedUndeclaredKeyPaths.set(tool, reportedKeyPaths); + const roomLeft = MAX_REPORTED_UNDECLARED_KEY_PATHS - reportedKeyPaths.size; + const newKeyPaths = keyPaths.filter((keyPath) => !reportedKeyPaths.has(keyPath)).slice(0, roomLeft); + if (newKeyPaths.length === 0) return; + newKeyPaths.forEach((keyPath) => reportedKeyPaths.add(keyPath)); + this.logger.warn( + `Tool "${tool.metadata.name}" answered with keys its outputSchema does not declare, so they were stripped: ` + + `${newKeyPaths.join(', ')}. Declare them, or set output.undeclaredKeys to 'strip', 'keep' or 'fail'.`, + ); + } } /** diff --git a/libs/sdk/src/tool/flows/tools-list.flow.ts b/libs/sdk/src/tool/flows/tools-list.flow.ts index 3e3bc5e4d..1e4e2b87e 100644 --- a/libs/sdk/src/tool/flows/tools-list.flow.ts +++ b/libs/sdk/src/tool/flows/tools-list.flow.ts @@ -7,10 +7,11 @@ import { formatOutputSchemaForDescription, resolveOutputSchemaMode, resolveSchemaDescriptionFormat, + resolveUndeclaredKeysPolicy, ToolEntry, + withUndeclaredKeysAllowed, type FlowPlan, type FlowRunOptions, - type OutputPolicy, } from '../../common'; import 'reflect-metadata'; @@ -27,6 +28,7 @@ import { DEFAULT_TOOL_PAGINATION, type ToolPaginationOptions } from '../../commo import { InternalMcpError, InvalidInputError, InvalidMethodError } from '../../errors'; import { type Scope } from '../../scope/scope.instance'; import { isSkillsOnlyRequest } from '../../skill/skill-mode.utils'; +import { toolOutputPolicies } from '../tool-output-policy'; import { buildStaticWidgetUri, hasUIConfig } from '../ui/ui-shared'; const inputSchema = z.object({ @@ -475,12 +477,6 @@ export default class ToolsListFlow extends FlowBase { // All platforms now use ui/* keys per the MCP Apps specification - // Resolve the output-schema exposure policy once (per-tool: Tool > App > server). - const serverOutput = (this.scope.metadata as { output?: OutputPolicy }).output; - const appOutputById = new Map( - this.scope.apps.getApps().map((app) => [app.id, (app.metadata as { output?: OutputPolicy }).output]), - ); - const uiRenderer = resolved.some(({ tool }) => hasUIConfig(tool.metadata)) ? await this.loadToolUIRenderer() : undefined; @@ -511,21 +507,24 @@ export default class ToolsListFlow extends FlowBase { // Expose the output schema per the resolved mode (Tool > App > server; default // 'definition'). 'definition'/'both' advertise it as item.outputSchema; // 'description'/'both' fold a readable rendering into the description; 'none' skips it. - const toolOutput = (tool.metadata as { output?: OutputPolicy }).output; - const appOutput = appOutputById.get(tool.owner.id); - const schemaMode = resolveOutputSchemaMode(toolOutput, appOutput, serverOutput); + const outputPolicies = toolOutputPolicies(tool, this.scope); + const schemaMode = resolveOutputSchemaMode(...outputPolicies); if (schemaMode === 'definition' || schemaMode === 'both') { // getRawOutputSchema() returns the explicit rawOutputSchema (OpenAPI / remote - // passthrough) or a JSON Schema derived from the declared Zod-shape / z.object() - // outputSchema (elicitation-extended when elicitation is enabled). + // passthrough) or a JSON Schema derived from the declared Zod-shape, z.object() or + // union-of-objects outputSchema (elicitation-extended when elicitation is enabled). const rawOutput = tool.getRawOutputSchema(); if (rawOutput) { // MCP spec requires outputSchema to have type: 'object' at the top level. // Strip non-compliant schemas to prevent a single tool from breaking the entire tools/list response. const schema = rawOutput as Record; if (schema['type'] === 'object') { - item.outputSchema = schema as typeof item.outputSchema; + // An answer that keeps undeclared keys must still validate against the listed schema + const keepsUndeclaredKeys = resolveUndeclaredKeysPolicy(...outputPolicies) === 'keep'; + item.outputSchema = ( + keepsUndeclaredKeys ? withUndeclaredKeysAllowed(schema) : schema + ) as typeof item.outputSchema; } else { this.logger.warn( `parseTools: tool "${finalName}" has outputSchema without type: 'object' — stripping to maintain MCP compliance`, @@ -538,7 +537,7 @@ export default class ToolsListFlow extends FlowBase { // Use the clean (non-elicitation-extended) schema for the human-readable description. const outputJsonSchema = tool.getOutputJsonSchema(); if (outputJsonSchema) { - const format = resolveSchemaDescriptionFormat(toolOutput, appOutput, serverOutput); + const format = resolveSchemaDescriptionFormat(...outputPolicies); item.description = (item.description ?? '') + formatOutputSchemaForDescription(outputJsonSchema, format); } } diff --git a/libs/sdk/src/tool/tool-output-policy.ts b/libs/sdk/src/tool/tool-output-policy.ts new file mode 100644 index 000000000..1265d2dd7 --- /dev/null +++ b/libs/sdk/src/tool/tool-output-policy.ts @@ -0,0 +1,57 @@ +// file: libs/sdk/src/tool/tool-output-policy.ts + +import { + resolveOutputSchemaMode, + type EntryLineage, + type OutputPolicy, + type ScopeEntry, + type ToolEntry, +} from '../common'; +import { appOwnerIdOf } from '../utils/lineage.utils'; + +/** A tool's `output` policies, most specific first, for the `resolve*` cascades in `output-policy.ts`. */ +export type ToolOutputPolicies = [tool?: OutputPolicy, app?: OutputPolicy, server?: OutputPolicy]; + +/** The parts of a tool the output-policy lookup reads. */ +type OutputPolicyTool = Pick; + +/** What the output-policy lookup reads from a scope: its metadata, its apps and its tools' lineage. */ +export interface OutputPolicyScope { + readonly metadata: { readonly output?: OutputPolicy }; + readonly apps: Pick; + readonly tools: { lineageOf(entry: Tool): EntryLineage | undefined }; +} + +/** + * The `output` policies that apply to `tool`: its own, its app's and the server's. The app is the + * one the tool is in, so a tool a plugin or an adapter of that app contributes follows it too. + * `tools/list` and `tools/call` both read it, so a tool is listed and answered under one policy. + */ +export function toolOutputPolicies( + tool: Tool, + scope: OutputPolicyScope, +): ToolOutputPolicies { + const appId = appOwnerIdOf(scope.tools.lineageOf(tool) ?? [], tool.owner); + const app = appId === undefined ? undefined : scope.apps.getApps().find((candidate) => candidate.id === appId); + // Remote apps have no `output` option + const appOutput = (app?.metadata as { output?: OutputPolicy } | undefined)?.output; + return [tool.metadata.output, appOutput, scope.metadata.output]; +} + +/** + * One startup warning for each listed tool whose declared `outputSchema` tools/list has to leave + * out, naming the tool and the reason. A tool whose `output.schemaMode` resolves to `'none'` asked + * for no schema, so it stays quiet. + */ +export function describeUnlistedOutputSchemas( + scope: OutputPolicyScope & { readonly tools: { getTools(): ToolEntry[] } }, +): string[] { + return scope.tools.getTools().flatMap((tool) => { + const reason = tool.getUnlistedOutputSchemaReason(); + if (!reason || resolveOutputSchemaMode(...toolOutputPolicies(tool, scope)) === 'none') return []; + return [ + `Tool "${tool.metadata.name}": tools/list leaves out its outputSchema because ${reason}. ` + + 'Declare a z.object() or a union of z.object() schemas to list it.', + ]; + }); +} diff --git a/libs/sdk/src/tool/tool.instance.ts b/libs/sdk/src/tool/tool.instance.ts index a1284d92c..84bb750f1 100644 --- a/libs/sdk/src/tool/tool.instance.ts +++ b/libs/sdk/src/tool/tool.instance.ts @@ -8,6 +8,7 @@ import { ToolEntry, ToolKind, type EntryOwnerRef, + type OutputParseOptions, type ParsedToolResult, type SafeTransformResult, type ScopeEntry, @@ -221,16 +222,19 @@ export class ToolInstance< * - image/audio/resource/resource_link → passed through as-is. * - JSON / structured → JSON.stringify for text, and full sanitized JSON in structuredContent. */ - override parseOutput(raw: Out | Partial | any): ParsedToolResult { + override parseOutput(raw: Out | Partial | any, options?: OutputParseOptions): ParsedToolResult { const descriptor = this.outputSchema as any; - return buildParsedToolResult(descriptor, raw); + return buildParsedToolResult(descriptor, raw, options); } - override safeParseOutput(raw: Out | Partial | any): SafeTransformResult { + override safeParseOutput( + raw: Out | Partial | any, + options?: OutputParseOptions, + ): SafeTransformResult { const descriptor = this.outputSchema as any; try { - return { success: true, data: buildParsedToolResult(descriptor, raw) }; + return { success: true, data: buildParsedToolResult(descriptor, raw, options) }; } catch (error: any) { return { success: false, error }; } diff --git a/libs/sdk/src/tool/tool.utils.ts b/libs/sdk/src/tool/tool.utils.ts index 3cdcb1e5a..8ee8364ea 100644 --- a/libs/sdk/src/tool/tool.utils.ts +++ b/libs/sdk/src/tool/tool.utils.ts @@ -1,6 +1,6 @@ // file: libs/sdk/src/tool/tool.utils.ts import { depsOfClass, depsOfFunc, getMetadata, isClass, type Token, type Type } from '@frontmcp/di'; -import { z, ZodBigInt, ZodBoolean, ZodDate, ZodNumber, ZodString } from '@frontmcp/lazy-zod'; +import { hasTransform, z, ZodBigInt, ZodBoolean, ZodDate, ZodNumber, ZodString } from '@frontmcp/lazy-zod'; import { type AudioContent, type ContentBlock, @@ -9,12 +9,14 @@ import { type ResourceLink, type TextContent, } from '@frontmcp/protocol'; +import { findDroppedKeys } from '@frontmcp/utils'; import { extendedToolMetadata, FrontMcpToolTokens, ToolKind, type AgentToolDefinition, + type OutputParseOptions, type ParsedToolResult, type ToolContext, type ToolEntry, @@ -143,7 +145,11 @@ function isCallToolResult(value: unknown): value is ParsedToolResult { return true; } -export function buildParsedToolResult(descriptor: any, raw: unknown): ParsedToolResult { +export function buildParsedToolResult( + descriptor: any, + raw: unknown, + options: OutputParseOptions = {}, +): ParsedToolResult { // If raw is already a valid CallToolResult (e.g., from remote tools), return it as-is // This prevents double-wrapping of MCP-formatted responses if (isCallToolResult(raw)) { @@ -169,7 +175,7 @@ export function buildParsedToolResult(descriptor: any, raw: unknown): ParsedTool // Parse all items first descriptor.forEach((singleDescriptor: any, idx: number) => { const value = values[idx]; - const { blocks, parsedValue, isPrimitive } = parseSingleValue(singleDescriptor, value); + const { blocks, parsedValue, isPrimitive } = parseSingleValue(singleDescriptor, value, options); parsedItems.push({ blocks, parsedValue, isPrimitive }); content.push(...blocks); }); @@ -206,7 +212,7 @@ export function buildParsedToolResult(descriptor: any, raw: unknown): ParsedTool }); } } else { - const { blocks, parsedValue, isPrimitive } = parseSingleValue(descriptor, raw); + const { blocks, parsedValue, isPrimitive } = parseSingleValue(descriptor, raw, options); content.push(...blocks); // Single item: wrap primitives in {content: value}, use objects directly @@ -244,6 +250,7 @@ export function buildParsedToolResult(descriptor: any, raw: unknown): ParsedTool function parseSingleValue( descriptor: any, value: unknown, + options: OutputParseOptions, ): { blocks: ContentBlock[]; parsedValue?: any; isPrimitive: boolean } { // Literal primitives + special content types if (typeof descriptor === 'string') { @@ -373,9 +380,9 @@ function parseSingleValue( let parsedValue: any; if (descriptor instanceof z.ZodType) { - parsedValue = toStructuredContent(parseAgainstOutputSchema(descriptor, value)); + parsedValue = toStructuredContent(parseAgainstOutputSchema(descriptor, value, options)); } else if (isZodRawShape(descriptor)) { - parsedValue = toStructuredContent(parseAgainstOutputSchema(z.object(descriptor), value)); + parsedValue = toStructuredContent(parseAgainstOutputSchema(z.object(descriptor), value, options)); } else { parsedValue = toStructuredContent(value); } @@ -388,9 +395,57 @@ function parseSingleValue( } /** - * The value an output schema accepts, or an InvalidOutputError naming the first mismatch. + * The value an output schema accepts, with undeclared keys handled per `options.undeclaredKeys`, + * or an InvalidOutputError naming the first mismatch, the undeclared keys `'fail'` refuses, or those `'keep'` cannot + * put back because the schema replaced the object holding them (a coercion to a primitive). + * A schema with a transform is always stripped: its output cannot be compared with the answer key by key. */ -function parseAgainstOutputSchema(schema: z.ZodType, value: unknown): unknown { +function parseAgainstOutputSchema(schema: z.ZodType, value: unknown, options: OutputParseOptions = {}): unknown { + const declared = parseDeclaredOutput(schema, value); + const policy = options.undeclaredKeys ?? 'strip'; + if (policy === 'strip' || hasTransform(schema)) return declared; + const undeclaredKeys = findDroppedKeys(value, declared); + if (undeclaredKeys.length === 0) return declared; + switch (policy) { + case 'keep': { + const kept = withUndeclaredKeys(value, declared); + const unkeptKeys = findDroppedKeys(value, kept); + if (unkeptKeys.length > 0) { + throw new InvalidOutputError({ + reason: 'undeclared keys the outputSchema cannot keep', + value: unkeptKeys.join(', '), + }); + } + return kept; + } + case 'fail': + throw new InvalidOutputError({ reason: 'undeclared keys', value: undeclaredKeys.join(', ') }); + case 'warn': + options.onUndeclaredKeys?.(undeclaredKeys); + return declared; + } +} + +/** `declared` with the keys of `original` its schema dropped put back, at every level. */ +function withUndeclaredKeys(original: unknown, declared: unknown): unknown { + if (original === declared) return declared; + if (Array.isArray(original) && Array.isArray(declared)) { + return declared.map((item, index) => withUndeclaredKeys(original[index], item)); + } + if (!isRecord(original) || !isRecord(declared)) return declared; + const declaredEntries = Object.entries(declared).map(([key, item]) => [key, withUndeclaredKeys(original[key], item)]); + const undeclaredEntries = Object.entries(original).filter( + ([key, item]) => item !== undefined && !Object.prototype.hasOwnProperty.call(declared, key), + ); + return Object.fromEntries([...declaredEntries, ...undeclaredEntries]); +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +/** The value an output schema accepts, undeclared keys stripped, or an InvalidOutputError naming the first mismatch. */ +function parseDeclaredOutput(schema: z.ZodType, value: unknown): unknown { const parseResult = schema.safeParse(value); if (parseResult.success) { return parseResult.data; diff --git a/libs/skills/catalog/create-tool/examples/04-tool-with-zod-schema-output.md b/libs/skills/catalog/create-tool/examples/04-tool-with-zod-schema-output.md index 97f4ae36f..200caa240 100644 --- a/libs/skills/catalog/create-tool/examples/04-tool-with-zod-schema-output.md +++ b/libs/skills/catalog/create-tool/examples/04-tool-with-zod-schema-output.md @@ -14,7 +14,7 @@ features: Tool returning a discriminated union via a full `z.discriminatedUnion(...)` outputSchema — for outputs that branch on a kind field. -When the output has more than one shape (e.g. `user` vs `group`), use a full Zod schema instead of a raw shape. `z.discriminatedUnion` is the cleanest pattern. +When the output has more than one shape (e.g. `user` vs `group`), use a full Zod schema instead of a raw shape. `z.discriminatedUnion` is the cleanest pattern. A union whose members are all objects is advertised in `tools/list` as `{ type: 'object', oneOf: [...] }` (`anyOf` for a plain `z.union`), and needs no cast in `@Tool({ outputSchema })`. ## Code @@ -92,6 +92,8 @@ export class ResolvePrincipalTool extends ToolContext { | Use raw shape | Use full Zod schema | | ---------------------------------------- | -------------------------------------------------------- | | Single object with a fixed set of fields | Union of multiple shapes (`z.discriminatedUnion`) | -| All fields known statically | Arrays of complex objects (`z.array(z.object(...))`) | +| All fields known statically | Arrays of complex objects (`z.array(z.object(...))`)¹ | | No transforms / refinements needed | Need `z.transform(...)`, `z.refine(...)`, `z.brand(...)` | | Default and recommended | When the raw shape can't express the contract | + +¹ A top-level array is not an object, so `tools/list` leaves it out (with a startup warning naming the tool). Wrap it, e.g. `z.object({ items: z.array(...) })`, to advertise it. diff --git a/libs/skills/catalog/create-tool/references/decorator-options.md b/libs/skills/catalog/create-tool/references/decorator-options.md index 01a97a949..6960e4248 100644 --- a/libs/skills/catalog/create-tool/references/decorator-options.md +++ b/libs/skills/catalog/create-tool/references/decorator-options.md @@ -7,23 +7,23 @@ description: Every field on @Tool({...}) — what it does, default, when to set Full surface of the `@Tool` decorator. Mandatory fields are bolded. -| Field | Type | Default | When to set | -| ----------------- | ------------------------------------------------------------------------------ | ------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| **`name`** | `string` (snake_case) | — | Always. MCP protocol convention. `get_weather`, not `getWeather`. | -| `title` | `string` | — | When the tool needs a display name in `tools/list`. Without it, `annotations.title` is the only title. | -| `description` | `string` | — | Almost always. Shows in `tools/list` and helps clients choose the right tool. | -| **`inputSchema`** | Zod raw shape | — | Always. `{ field: z.string() }` — never wrapped in `z.object`. See [`input-schema.md`](./input-schema.md). | -| `outputSchema` | Zod raw shape / Zod schema / primitive literal / media literal / array | — | **Always** — prevents data leaks and enables CodeCall chaining. See [`output-schema.md`](./output-schema.md). | -| `output` | `{ allowNonFinite?, schemaMode?, schemaDescriptionFormat? }` | `definition` | Control how `outputSchema` is exposed in `tools/list` (`'definition'` \| `'description'` \| `'both'` \| `'none'`) and `allowNonFinite`. Cascades Tool > App > server. See [`output-schema.md`](./output-schema.md#exposure-mode). | -| `annotations` | `{ title?, readOnlyHint?, destructiveHint?, idempotentHint?, openWorldHint? }` | — | When the tool has notable behavioral semantics clients should be hinted about. See [`annotations.md`](./annotations.md). | -| `rateLimit` | `{ maxRequests, windowMs }` | — | Expensive / external-dependent / abuse-prone tools. See [`throttling.md`](./throttling.md). | -| `concurrency` | `{ maxConcurrent }` | — | Tools that hold a scarce resource (DB connection, GPU). See [`throttling.md`](./throttling.md). | -| `timeout` | `{ executeMs }` | — | Any tool that could legitimately hang. See [`throttling.md`](./throttling.md). | -| `authProviders` | `string[]` \| `Array<{ name, scopes?, required?, alias? }>` | — | Tool requires user OAuth credentials. See [`auth-providers.md`](./auth-providers.md). | -| `availableWhen` | `{ os?, runtime?, deployment?, provider?, target?, surface?, env? }` | — | Hard registry-level constraint — tool is filtered out of `tools/list` and execution when context doesn't match. See [`availability.md`](./availability.md). | -| `examples` | `Array<{ description, input, output? }>` | — | Discovery / docs surfacing. The client may show these to the user. | -| `ui` | `ToolUIConfig` | — | Tool result should render as a widget in the host UI. See [`ui-widgets.md`](./ui-widgets.md). | -| `visibility` | `'public' \| 'hidden' \| 'internal'` | `'public'` | Control discoverability/reachability. `'public'`: listed in `tools/list` + callable via `tools/call`. `'hidden'`: not listed, still callable by name. `'internal'`: not listed AND not externally callable — only via `this.callTool(...)` from inside the SDK. | +| Field | Type | Default | When to set | +| ----------------- | ------------------------------------------------------------------------------ | ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **`name`** | `string` (snake_case) | — | Always. MCP protocol convention. `get_weather`, not `getWeather`. | +| `title` | `string` | — | When the tool needs a display name in `tools/list`. Without it, `annotations.title` is the only title. | +| `description` | `string` | — | Almost always. Shows in `tools/list` and helps clients choose the right tool. | +| **`inputSchema`** | Zod raw shape | — | Always. `{ field: z.string() }` — never wrapped in `z.object`. See [`input-schema.md`](./input-schema.md). | +| `outputSchema` | Zod raw shape / Zod schema / primitive literal / media literal / array | — | **Always** — prevents data leaks and enables CodeCall chaining. See [`output-schema.md`](./output-schema.md). | +| `output` | `{ allowNonFinite?, schemaMode?, schemaDescriptionFormat?, undeclaredKeys? }` | `definition` | Control how `outputSchema` is exposed in `tools/list` (`'definition'` \| `'description'` \| `'both'` \| `'none'`), `allowNonFinite`, and what happens to undeclared answer keys (`undeclaredKeys`: `'strip'` \| `'warn'` \| `'keep'` \| `'fail'`; `'warn'` in development, else `'strip'`). Cascades Tool > App > server. See [`output-schema.md`](./output-schema.md#exposure-mode). | +| `annotations` | `{ title?, readOnlyHint?, destructiveHint?, idempotentHint?, openWorldHint? }` | — | When the tool has notable behavioral semantics clients should be hinted about. See [`annotations.md`](./annotations.md). | +| `rateLimit` | `{ maxRequests, windowMs }` | — | Expensive / external-dependent / abuse-prone tools. See [`throttling.md`](./throttling.md). | +| `concurrency` | `{ maxConcurrent }` | — | Tools that hold a scarce resource (DB connection, GPU). See [`throttling.md`](./throttling.md). | +| `timeout` | `{ executeMs }` | — | Any tool that could legitimately hang. See [`throttling.md`](./throttling.md). | +| `authProviders` | `string[]` \| `Array<{ name, scopes?, required?, alias? }>` | — | Tool requires user OAuth credentials. See [`auth-providers.md`](./auth-providers.md). | +| `availableWhen` | `{ os?, runtime?, deployment?, provider?, target?, surface?, env? }` | — | Hard registry-level constraint — tool is filtered out of `tools/list` and execution when context doesn't match. See [`availability.md`](./availability.md). | +| `examples` | `Array<{ description, input, output? }>` | — | Discovery / docs surfacing. The client may show these to the user. | +| `ui` | `ToolUIConfig` | — | Tool result should render as a widget in the host UI. See [`ui-widgets.md`](./ui-widgets.md). | +| `visibility` | `'public' \| 'hidden' \| 'internal'` | `'public'` | Control discoverability/reachability. `'public'`: listed in `tools/list` + callable via `tools/call`. `'hidden'`: not listed, still callable by name. `'internal'`: not listed AND not externally callable — only via `this.callTool(...)` from inside the SDK. | > `hideFromDiscovery: boolean` is a **deprecated** alias for `visibility`: when `visibility` is unset, `hideFromDiscovery: true` is treated as `visibility: 'hidden'`. Prefer `visibility` in new code. diff --git a/libs/skills/catalog/create-tool/references/output-schema.md b/libs/skills/catalog/create-tool/references/output-schema.md index 79d09aa72..039a27ca7 100644 --- a/libs/skills/catalog/create-tool/references/output-schema.md +++ b/libs/skills/catalog/create-tool/references/output-schema.md @@ -5,7 +5,7 @@ description: Define the tool's output contract — Zod shape, primitives, media, # `outputSchema` reference -`outputSchema` is **always required** ([rule](../rules/always-define-output-schema.md)). It declares what `execute()` returns and gives the framework permission to strip any fields you didn't declare, which guards against accidental PII / token / debug-trace leaks in the result. The exception is a `_meta` object on the returned value: it is copied into the response's `_meta` without validation, so never put secrets there. A result that does not match fails the call with `_meta.code: 'INVALID_OUTPUT'`, and nothing from the rejected result is sent. A number field holding `Infinity`, `-Infinity`, or `NaN` fails the same way, unless the server sets `output: { allowNonFinite: true }`. JSON cannot encode those values, so with that setting the client receives `null` in their place. +`outputSchema` is **always required** ([rule](../rules/always-define-output-schema.md)). It declares what `execute()` returns, and the framework strips any fields you didn't declare, which guards against accidental PII / token / debug-trace leaks in the result. In development (`NODE_ENV=development`) it also warns once per tool about the keys it stripped; see [Undeclared keys](#undeclared-keys) to change that. The exception is a `_meta` object on the returned value: it is copied into the response's `_meta` without validation, so never put secrets there. A result that does not match fails the call with `_meta.code: 'INVALID_OUTPUT'`, and nothing from the rejected result is sent. A number field holding `Infinity`, `-Infinity`, or `NaN` fails the same way, unless the server sets `output: { allowNonFinite: true }`. JSON cannot encode those values, so with that setting the client receives `null` in their place. ## Supported shapes @@ -44,7 +44,7 @@ class GetWeatherTool extends ToolContext { ## Zod schema (full Zod) -When the output is a union, discriminated union, or array of objects: +Use a full Zod schema when the output is a union, discriminated union, or array of objects. `z.union([...])` and `z.discriminatedUnion(...)` type-check as `outputSchema` with no cast: ```typescript const outputSchema = z.discriminatedUnion('kind', [ @@ -129,10 +129,11 @@ class AnalyzeImageTool extends ToolContext { Structured object outputs are converted to JSON Schema and advertised on the tool's `outputSchema` in `tools/list` — symmetric with how `inputSchema` is advertised: -- **Advertised:** a Zod raw shape (`{ field: z.string() }`) or a top-level `z.object({...})` — these serialize to a `type: 'object'` JSON Schema. -- **Not advertised:** primitives (`'string'` / `'number'` / `'boolean'` / `'date'`), media (`'image'` / `'audio'` / `'resource'` / `'resource_link'`), multi-content arrays (`['string', 'image']`), and unions (`z.discriminatedUnion(...)`). These flow through `content` rather than `structuredContent`, and the MCP spec requires `outputSchema` to be a top-level `type: 'object'` — so there is nothing object-shaped to advertise. Runtime output validation still applies to every form. +- **Advertised:** a Zod raw shape (`{ field: z.string() }`) or a top-level `z.object({...})` — these serialize to a `type: 'object'` JSON Schema. A `z.union([...])` or `z.discriminatedUnion(...)` whose members are all objects is advertised too, as `{ type: 'object', anyOf: [...] }` (or `oneOf` for a discriminated union), since MCP requires a top-level `type: 'object'`. A member registered with an id (`.meta({ id })`) serializes to a `$ref` into `$defs` and counts as an object too. +- **Not advertised, by design:** primitives (`'string'` / `'number'` / `'boolean'` / `'date'`, `z.string()`, …), media (`'image'` / `'audio'` / `'resource'` / `'resource_link'`), and multi-content arrays (`['string', 'image']`). These flow through `content` rather than `structuredContent`. +- **Not advertised, with a startup warning:** any other Zod schema — an array (`z.array(...)`), a nullable object, a union with a non-object member, or a schema that has no JSON Schema form (a `z.date()` field). The server logs `Tool "": tools/list leaves out its outputSchema because …` once at startup, naming the tool, unless the tool's `schemaMode` resolves to `'none'`. Wrap the value in a `z.object({...})` to advertise it. -If a client reports _"Output schema recommended,"_ declare a structured object `outputSchema` (a raw shape or `z.object`) — that's the form that gets advertised. +Runtime output validation still applies to every form. If a client reports _"Output schema recommended,"_ declare a structured object `outputSchema` (a raw shape, a `z.object`, or a union of `z.object`s) — that's the form that gets advertised. ### Exposure mode @@ -142,6 +143,7 @@ A cascading `output` policy controls _how_ the (object-shaped) schema reaches cl output?: { schemaMode?: 'definition' | 'description' | 'both' | 'none'; // default 'definition' schemaDescriptionFormat?: 'summary' | 'jsonSchema'; // default 'summary' + undeclaredKeys?: 'strip' | 'warn' | 'keep' | 'fail'; // see Undeclared keys }; ``` @@ -154,7 +156,7 @@ output?: { `schemaDescriptionFormat` controls the rendering for `'description'` / `'both'`: `'summary'` (default) is a compact property list; `'jsonSchema'` is a fenced JSON Schema block. -The effective value resolves with a **Tool > App > server > default** cascade — set a server-wide default on `@FrontMcp` and override per tool. This mirrors the OpenAPI adapter's `outputSchema.mode`. +The effective value resolves with a **Tool > App > server > default** cascade — set a server-wide default on `@FrontMcp` and override per tool. A tool that a plugin or an adapter of an app contributes follows that app's `output`. This mirrors the OpenAPI adapter's `outputSchema.mode`. ```typescript // This tool folds its schema into the description instead of advertising `outputSchema` @@ -166,9 +168,43 @@ class GetStatusTool extends ToolContext { The default (`'definition'`) preserves the current behavior described above — omit `output` entirely and nothing changes. +## Undeclared keys + +`output.undeclaredKeys` decides what `tools/call` does with keys an answer carries that `outputSchema` does not declare, at any depth (`costCenter`, `lines[].margin`). It cascades **Tool > App > server > default**, like `schemaMode`. + +| `undeclaredKeys` | Effect | +| ---------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `'strip'` | Drop them. **Default** outside development. | +| `'warn'` | Drop them and log a warning naming each key once per tool (up to 100 keys per tool). **Default** when `NODE_ENV=development`. | +| `'keep'` | Send them in `structuredContent` and the text content. `tools/list` lists the schema's objects without `additionalProperties: false`, so clients accept them. A key it cannot put back, because the schema turned the object holding it into a primitive (`z.coerce.string()`), refuses the answer as `'fail'` does. | +| `'fail'` | Refuse the answer with `_meta.code: 'INVALID_OUTPUT'`; the error message in the server log names the keys. Nothing from the answer is sent. | + +```typescript +// Fail loudly when an upstream API starts returning fields the tool never declared +@Tool({ name: 'get_invoice', inputSchema, outputSchema, output: { undeclaredKeys: 'fail' } }) +class GetInvoiceTool extends ToolContext { + /* … */ +} +``` + +A call that renders the tool's UI (`@Tool({ ui })`) is parsed with `'strip'`, whatever the policy: the widget, the text content and `structuredContent` are built from that parsed output. An `outputSchema` with a transform anywhere (`.transform()`, `z.preprocess()`, `.pipe()`) is always parsed with `'strip'` too, and the answer is what the schema returns: its output cannot be compared with the answer key by key, so `'warn'`, `'keep'` and `'fail'` do not apply to it. A JSON Schema output (the OpenAPI adapter, a remote app) is not parsed, so its answer is sent as the upstream returned it, whatever the policy. An object schema that sets its own unknown-key handling also wins over the policy: `z.strictObject()` (`.strict()`) refuses an answer with undeclared keys (`INVALID_OUTPUT`), even under `'keep'`, and `z.looseObject()` (`.passthrough()`) keeps them, even under `'strip'` or `'fail'` and on the UI path. + +A `z.union([...])` keeps the first member that matches, so a key that only a later member declares counts as undeclared: `'strip'` drops it, `'warn'` drops and logs it, `'fail'` refuses the answer, and `'keep'` sends it. Put the widest member first, or use `z.discriminatedUnion(...)`, which picks the member by its discriminator. + +To check in a test that an answer has no undeclared keys, use the `@frontmcp/testing` matcher: + +```typescript +import { expect } from '@frontmcp/testing'; + +expect(await tool.execute({ id: 'INV-1' })).toPassOutputSchemaWhole(outputSchema); +// fails with: … parsing drops keys it does not declare: costCenter, lines[].margin +``` + +The matcher fails for a schema with a transform, for the same reason; assert on the parsed answer there. + ## Why this matters -- **Data leak prevention** — without `outputSchema`, accidentally returning `{ result, internalApiKey }` leaks the key. With it, only `result` flows. +- **Data leak prevention** — without `outputSchema`, accidentally returning `{ result, internalApiKey }` leaks the key. With it, only `result` flows (unless `output.undeclaredKeys` is `'keep'`). - **CodeCall compatibility** — the CodeCall plugin uses `outputSchema` to chain tool calls in its VM. Tools without it degrade chain-ability. - **Compile-time type safety** — `ToolContext` infers `execute()`'s return type from `outputSchema` (when `ToolOutputOf<>` is used). The compiler catches divergence at build time. - **Self-documenting** — a structured object `outputSchema` is converted to JSON Schema and exposed in `tools/list` (see [Advertisement in `tools/list`](#advertisement-in-toolslist)); AI clients pick tools partly based on what they return. diff --git a/libs/skills/catalog/create-tool/rules/always-define-output-schema.md b/libs/skills/catalog/create-tool/rules/always-define-output-schema.md index f5c858375..f58329170 100644 --- a/libs/skills/catalog/create-tool/rules/always-define-output-schema.md +++ b/libs/skills/catalog/create-tool/rules/always-define-output-schema.md @@ -52,10 +52,10 @@ class GetWeatherTool extends ToolContext { ## Why -1. **Output validation prevents data leaks.** Without `outputSchema`, every field your code accidentally includes (or that an upstream API drops in unsolicited — auth tokens, internal IDs, debug traces, PII) reaches the MCP client. With it, only declared fields pass through; everything else is stripped. +1. **Output validation prevents data leaks.** Without `outputSchema`, every field your code accidentally includes (or that an upstream API drops in unsolicited — auth tokens, internal IDs, debug traces, PII) reaches the MCP client. With it, only declared fields pass through; everything else is stripped (and, in development, named in a warning). `output: { undeclaredKeys: 'fail' }` refuses such an answer instead. 2. **CodeCall plugin compatibility.** The CodeCall plugin uses `outputSchema` to understand what a tool returns, enabling correct VM-based orchestration and pass-by-reference. Tools without `outputSchema` degrade CodeCall's ability to chain calls. 3. **Type safety on `execute()`'s return type.** With `outputSchema` declared, `ToolContext` infers the expected return type from it. The compiler tells you when your return value diverges from the declared shape. -4. **Self-documenting tools.** A structured object `outputSchema` (a Zod raw shape or `z.object`) is converted to JSON Schema and advertised in `tools/list`, so AI clients can choose the right tool by what it returns. Primitive / media / multi-content-array / union outputs flow through `content` instead and aren't advertised as `outputSchema` — that's expected (the MCP `outputSchema` must be a top-level object). +4. **Self-documenting tools.** A structured object `outputSchema` (a Zod raw shape, a `z.object`, or a union of `z.object`s) is converted to JSON Schema and advertised in `tools/list`, so AI clients can choose the right tool by what it returns. Primitive / media / multi-content-array outputs flow through `content` instead and aren't advertised as `outputSchema` — that's expected (the MCP `outputSchema` must be a top-level object). Any other schema (e.g. `z.array(...)`) is left out with a startup warning naming the tool. ## How to apply diff --git a/libs/skills/catalog/frontmcp-production-readiness/references/common-checklist.md b/libs/skills/catalog/frontmcp-production-readiness/references/common-checklist.md index 57d97ad1b..a09efd56b 100644 --- a/libs/skills/catalog/frontmcp-production-readiness/references/common-checklist.md +++ b/libs/skills/catalog/frontmcp-production-readiness/references/common-checklist.md @@ -45,7 +45,7 @@ These checks apply to ALL deployment targets. Run them first, then proceed to yo ### Input Validation - [ ] All tool inputs use Zod schemas (never trust raw input) -- [ ] All tool outputs use `outputSchema` to prevent data leaks +- [ ] All tool outputs use `outputSchema` to prevent data leaks; `output.undeclaredKeys` is not `'keep'` for a tool whose answers can carry secrets (the default strips undeclared keys) - [ ] Path parameters and query params are validated - [ ] File paths are sanitized to prevent directory traversal - [ ] SQL queries use parameterized statements (never string interpolation) diff --git a/libs/skills/catalog/frontmcp-testing/references/setup-testing.md b/libs/skills/catalog/frontmcp-testing/references/setup-testing.md index b562a67af..806426572 100644 --- a/libs/skills/catalog/frontmcp-testing/references/setup-testing.md +++ b/libs/skills/catalog/frontmcp-testing/references/setup-testing.md @@ -454,15 +454,16 @@ A nested `test.describe` inherits an outer skip. The `(name, fn)` form still ski import { expect } from '@frontmcp/testing'; ``` -| Matcher | Asserts | -| ------------------------- | ------------------------------------------------------------------------------------------------------------------ | -| `toContainTool(name)` | Tools list includes a tool with the given name | -| `toContainResource(uri)` | Resources list includes a resource with the given URI | -| `toContainPrompt(name)` | Prompts list includes a prompt with the given name | -| `toBeSuccessful()` | Tool call result is not an error | -| `toBeError(code?)` | Result is an error; a string code matches `_meta.code` (`'INVALID_INPUT'`), a number matches a JSON-RPC error code | -| `toHaveTextContent(text)` | Result contains text content matching the string | -| `toHaveMimeType(mime)` | Resource content has the expected MIME type | +| Matcher | Asserts | +| --------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------- | +| `toContainTool(name)` | Tools list includes a tool with the given name | +| `toContainResource(uri)` | Resources list includes a resource with the given URI | +| `toContainPrompt(name)` | Prompts list includes a prompt with the given name | +| `toBeSuccessful()` | Tool call result is not an error | +| `toBeError(code?)` | Result is an error; a string code matches `_meta.code` (`'INVALID_INPUT'`), a number matches a JSON-RPC error code | +| `toHaveTextContent(text)` | Result contains text content matching the string | +| `toHaveMimeType(mime)` | Resource content has the expected MIME type | +| `toPassOutputSchemaWhole(outputSchema)` | A tool's answer matches its `outputSchema` (Zod schema or raw shape) and has no undeclared key the server would strip; the failure names the keys | ## Running Tests with Nx diff --git a/libs/testing/README.md b/libs/testing/README.md index 36ccdc16e..9daf048e5 100644 --- a/libs/testing/README.md +++ b/libs/testing/README.md @@ -50,7 +50,7 @@ npx frontmcp test ## Matchers -**Tools**: `toContainTool`, `toBeSuccessful`, `toBeError`, `toHaveTextContent`, `toHaveImageContent`, `toHaveResourceContent` +**Tools**: `toContainTool`, `toBeSuccessful`, `toBeError`, `toHaveTextContent`, `toHaveImageContent`, `toHaveResourceContent`, `toPassOutputSchemaWhole` **Resources**: `toContainResource`, `toContainResourceTemplate`, `toHaveMimeType` diff --git a/libs/testing/src/matchers/__tests__/mcp-matchers.output-schema.spec.ts b/libs/testing/src/matchers/__tests__/mcp-matchers.output-schema.spec.ts new file mode 100644 index 000000000..66f232e0f --- /dev/null +++ b/libs/testing/src/matchers/__tests__/mcp-matchers.output-schema.spec.ts @@ -0,0 +1,61 @@ +import { z } from '@frontmcp/lazy-zod'; + +import { mcpMatchers } from '../mcp-matchers'; + +import type {} from '../matcher-types'; + +expect.extend(mcpMatchers); + +const invoiceShape = { id: z.string(), lines: z.array(z.object({ sku: z.string() })) }; + +describe('toPassOutputSchemaWhole', () => { + it('passes an answer that holds declared keys only, for a raw shape', () => { + expect({ id: 'INV-1', lines: [{ sku: 'A' }] }).toPassOutputSchemaWhole(invoiceShape); + }); + + it('passes an answer that holds declared keys only, for a Zod schema', () => { + const hit = z.union([z.object({ title: z.string() }), z.object({ name: z.string() })]); + + expect({ name: 'Ada' }).toPassOutputSchemaWhole(hit); + }); + + it('fails an answer whose undeclared keys the schema would drop, naming them', () => { + const answer = { id: 'INV-1', costCenter: 'CC-7', lines: [{ sku: 'A', margin: 0.4 }] }; + + expect(() => expect(answer).toPassOutputSchemaWhole(invoiceShape)).toThrow( + 'drops keys it does not declare: costCenter, lines[].margin', + ); + }); + + it('fails an answer that does not match the schema, naming the first mismatch', () => { + expect(() => expect({ id: 7, lines: [] }).toPassOutputSchemaWhole(invoiceShape)).toThrow( + /does not match its output schema at id/, + ); + }); + + it('supports .not for an answer with undeclared keys', () => { + expect({ id: 'INV-1', lines: [], debug: true }).not.toPassOutputSchemaWhole(invoiceShape); + }); + + it('fails .not for an answer that passes whole', () => { + expect(() => expect({ id: 'INV-1', lines: [] }).not.toPassOutputSchemaWhole(invoiceShape)).toThrow( + 'Expected the answer not to pass its output schema whole', + ); + }); + + it('fails an answer whose keys parsing loses by coercing their object to a primitive', () => { + const coercing = z.object({ payload: z.coerce.string() }); + + expect(() => expect({ payload: { secret: 'x' } }).toPassOutputSchemaWhole(coercing)).toThrow( + 'drops keys it does not declare: payload.secret', + ); + }); + + it('fails for a schema with a transform, whose output cannot be compared key by key', () => { + const renaming = z.object({ a: z.string() }).transform((value) => ({ renamed: value.a })); + + expect(() => expect({ a: 'x' }).toPassOutputSchemaWhole(renaming)).toThrow( + /cannot compare keys through a transform/, + ); + }); +}); diff --git a/libs/testing/src/matchers/matcher-types.ts b/libs/testing/src/matchers/matcher-types.ts index 60c4c8123..35d63be14 100644 --- a/libs/testing/src/matchers/matcher-types.ts +++ b/libs/testing/src/matchers/matcher-types.ts @@ -6,13 +6,12 @@ * Import this file or ensure it's included in your tsconfig to get type checking. */ -// Note: These imports are used for documentation/JSDoc purposes in the interface comments -// The actual runtime types are in mcp-matchers.ts +import type { ZodRawShape, ZodType } from '@frontmcp/lazy-zod'; /** - * Custom MCP matchers for Jest + * Custom MCP matchers for Jest: tool, resource, prompt and protocol results */ -export interface McpMatchers { +export interface McpResultMatchers { // ═══════════════════════════════════════════════════════════════════ // TOOL MATCHERS // ═══════════════════════════════════════════════════════════════════ @@ -89,6 +88,21 @@ export interface McpMatchers { */ toHaveResourceContent(): R; + /** + * Check that a tool's answer passes its output schema whole: it matches the schema, and parsing + * drops no key the schema does not declare. The failure message names the undeclared keys, which + * the server would strip, warn about or refuse, per its `output.undeclaredKeys` policy. A schema with + * a transform (`.transform()`, `z.preprocess()`, `.pipe()`) cannot be compared key by key and fails. + * @param outputSchema - The tool's `outputSchema`: a Zod schema or a raw shape + * + * @example + * ```typescript + * const answer = await tool.execute({ id: 'INV-1' }); + * expect(answer).toPassOutputSchemaWhole(invoiceOutputSchema); + * ``` + */ + toPassOutputSchemaWhole(outputSchema: ZodType | ZodRawShape): R; + // ═══════════════════════════════════════════════════════════════════ // RESOURCE MATCHERS // ═══════════════════════════════════════════════════════════════════ @@ -205,7 +219,12 @@ export interface McpMatchers { * ``` */ toHaveErrorCode(code: number): R; +} +/** + * Custom MCP matchers for Jest: rendered tool UI + */ +export interface McpUiMatchers { // ═══════════════════════════════════════════════════════════════════ // UI MATCHERS // ═══════════════════════════════════════════════════════════════════ @@ -301,25 +320,30 @@ export interface McpMatchers { toHaveProperHtmlStructure(): R; } +/** + * Custom MCP matchers for Jest + */ +export interface McpMatchers extends McpResultMatchers, McpUiMatchers {} + // ═══════════════════════════════════════════════════════════════════ // JEST TYPE AUGMENTATION // ═══════════════════════════════════════════════════════════════════ -/* eslint-disable @typescript-eslint/no-namespace, @typescript-eslint/no-empty-object-type */ +/* eslint-disable @typescript-eslint/no-namespace */ declare global { namespace jest { // Extend expect matchers - interface Matchers extends McpMatchers {} + interface Matchers extends McpResultMatchers, McpUiMatchers {} // Extend asymmetric matchers (for expect.toContainTool etc.) - interface Expect extends McpMatchers {} + interface Expect extends McpResultMatchers, McpUiMatchers {} // Extend inverse matchers (for expect.not.toContainTool etc.) - interface InverseAsymmetricMatchers extends McpMatchers {} + interface InverseAsymmetricMatchers extends McpResultMatchers, McpUiMatchers {} } } -/* eslint-enable @typescript-eslint/no-namespace, @typescript-eslint/no-empty-object-type */ +/* eslint-enable @typescript-eslint/no-namespace */ // This export is needed to make this a module export {}; diff --git a/libs/testing/src/matchers/mcp-matchers.ts b/libs/testing/src/matchers/mcp-matchers.ts index 2ec923c92..56f60d3b2 100644 --- a/libs/testing/src/matchers/mcp-matchers.ts +++ b/libs/testing/src/matchers/mcp-matchers.ts @@ -18,8 +18,12 @@ */ import type { MatcherFunction } from 'expect'; -import type { Tool, Resource, ResourceTemplate, Prompt } from '@frontmcp/protocol'; -import type { ToolResultWrapper, ResourceContentWrapper, PromptResultWrapper } from '../client/mcp-test-client.types'; + +import { hasTransform, z, type ZodRawShape, type ZodType } from '@frontmcp/lazy-zod'; +import type { Prompt, Resource, ResourceTemplate, Tool } from '@frontmcp/protocol'; +import { findDroppedKeys } from '@frontmcp/utils'; + +import type { PromptResultWrapper, ResourceContentWrapper, ToolResultWrapper } from '../client/mcp-test-client.types'; import { uiMatchers } from '../ui/ui-matchers'; // ═══════════════════════════════════════════════════════════════════ @@ -195,6 +199,46 @@ const toHaveResourceContent: MatcherFunction<[]> = function (received) { }; }; +/** + * Check that a tool's answer passes its output schema whole: it matches, and parsing drops no + * key the schema does not declare (which the server would strip, warn about or refuse) + */ +const toPassOutputSchemaWhole: MatcherFunction<[outputSchema: ZodType | ZodRawShape]> = function ( + received, + outputSchema, +) { + const schema = outputSchema instanceof z.ZodType ? outputSchema : z.object(outputSchema); + if (hasTransform(schema)) { + return { + pass: false, + message: () => + 'toPassOutputSchemaWhole cannot compare keys through a transform (.transform(), z.preprocess(), .pipe()): ' + + 'assert on the parsed answer instead', + }; + } + const parsed = schema.safeParse(received); + if (!parsed.success) { + const [firstIssue] = parsed.error.issues; + const location = firstIssue?.path.length ? ` at ${firstIssue.path.join('.')}` : ''; + return { + pass: false, + message: () => + `Expected the answer to pass its output schema whole, but it does not match its output schema${location}: ${firstIssue?.message}`, + }; + } + + const droppedKeys = findDroppedKeys(received, parsed.data); + const pass = droppedKeys.length === 0; + + return { + pass, + message: () => + pass + ? 'Expected the answer not to pass its output schema whole' + : `Expected the answer to pass its output schema whole, but parsing drops keys it does not declare: ${droppedKeys.join(', ')}`, + }; +}; + // ═══════════════════════════════════════════════════════════════════ // RESOURCE MATCHERS // ═══════════════════════════════════════════════════════════════════ @@ -457,6 +501,7 @@ export const mcpMatchers = { toHaveTextContent, toHaveImageContent, toHaveResourceContent, + toPassOutputSchemaWhole, // Resource matchers toContainResource, diff --git a/libs/utils/src/content/content.dropped-keys.spec.ts b/libs/utils/src/content/content.dropped-keys.spec.ts new file mode 100644 index 000000000..528d8e5d6 --- /dev/null +++ b/libs/utils/src/content/content.dropped-keys.spec.ts @@ -0,0 +1,59 @@ +import { findDroppedKeys } from './content'; + +describe('findDroppedKeys', () => { + it('names the keys the parsed value lacks, nested ones by their dotted path', () => { + const original = { id: 'INV-1', costCenter: 'CC-7', customer: { name: 'Ada', taxId: 'T-1' } }; + const parsed = { id: 'INV-1', customer: { name: 'Ada' } }; + + expect(findDroppedKeys(original, parsed)).toEqual(['costCenter', 'customer.taxId']); + }); + + it('names a key dropped from array items once, with an [] segment', () => { + const original = { lines: [{ sku: 'A', margin: 0.4 }, { sku: 'B', margin: 0.1 }, { sku: 'C' }] }; + const parsed = { lines: [{ sku: 'A' }, { sku: 'B' }, { sku: 'C' }] }; + + expect(findDroppedKeys(original, parsed)).toEqual(['lines[].margin']); + }); + + it('walks a top-level array', () => { + expect(findDroppedKeys([{ id: 1, debug: true }], [{ id: 1 }])).toEqual(['[].debug']); + }); + + it('returns the paths sorted', () => { + expect(findDroppedKeys({ zeta: 1, alpha: 2, kept: 3 }, { kept: 3 })).toEqual(['alpha', 'zeta']); + }); + + it('ignores keys holding undefined, which never reach JSON', () => { + expect(findDroppedKeys({ id: 1, note: undefined }, { id: 1 })).toEqual([]); + }); + + it('finds nothing when the parsed value keeps every key', () => { + const value = { id: 1, tags: ['a'], owner: { name: 'Ada' } }; + + expect(findDroppedKeys(value, structuredClone(value))).toEqual([]); + }); + + it('does not count inherited properties of the parsed value as kept', () => { + expect(findDroppedKeys({ id: 1, toString: 'shadow' }, { id: 1 })).toEqual(['toString']); + }); + + it('counts every key of an object that parsing replaced with a non-object as dropped', () => { + expect(findDroppedKeys({ payload: { secret: 'x', nested: { pin: 1 } } }, { payload: '[object Object]' })).toEqual([ + 'payload.nested', + 'payload.secret', + ]); + expect(findDroppedKeys([{ id: 1 }], { 0: { id: 1 } })).toEqual(['[].id']); + }); + + it('stops at a value the parsed answer holds by reference, so a cycle under it is not followed', () => { + const node: Record = { name: 'node' }; + node['self'] = node; + + expect(findDroppedKeys({ id: 1, extra: true, meta: node }, { id: 1, meta: node })).toEqual(['extra']); + }); + + it('finds nothing to drop in a primitive answer', () => { + expect(findDroppedKeys('text', { value: 'text' })).toEqual([]); + expect(findDroppedKeys(['a', 'b'], 'a,b')).toEqual([]); + }); +}); diff --git a/libs/utils/src/content/content.ts b/libs/utils/src/content/content.ts index 9791ccce6..3e90fd70c 100644 --- a/libs/utils/src/content/content.ts +++ b/libs/utils/src/content/content.ts @@ -139,6 +139,40 @@ export function findNonFiniteNumber(value: unknown): { path: string; value: numb return walk(value, ''); } +/** + * The key paths `original` holds that `parsed` lacks: what parsing `original` against a schema + * dropped. Nested keys are dotted (`customer.taxId`), array items share an `[]` segment + * (`lines[].margin`), keys holding `undefined` are skipped, and the paths come back sorted and + * unique. The walk follows `parsed`, so it stops wherever the two values differ in shape, and + * wherever `parsed` holds the very same object, which also keeps it out of cycles. + */ +export function findDroppedKeys(original: unknown, parsed: unknown): string[] { + const droppedPaths = new Set(); + + function walk(source: unknown, target: unknown, path: string): void { + if (source === target) return; + if (Array.isArray(source)) { + source.forEach((item, index) => walk(item, Array.isArray(target) ? target[index] : undefined, `${path}[]`)); + return; + } + if (!isPlainRecord(source)) return; + const targetRecord = isPlainRecord(target) ? target : {}; + for (const [key, value] of Object.entries(source)) { + if (value === undefined) continue; + const keyPath = path ? `${path}.${key}` : key; + if (Object.prototype.hasOwnProperty.call(targetRecord, key)) walk(value, targetRecord[key], keyPath); + else droppedPaths.add(keyPath); + } + } + + walk(original, parsed, ''); + return [...droppedPaths].sort(); +} + +function isPlainRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + /** * Common MIME types by file extension. */ diff --git a/libs/utils/src/content/index.ts b/libs/utils/src/content/index.ts index 909f0a16c..6c7659621 100644 --- a/libs/utils/src/content/index.ts +++ b/libs/utils/src/content/index.ts @@ -1 +1 @@ -export { sanitizeToJson, inferMimeType, findNonFiniteNumber } from './content'; +export { sanitizeToJson, inferMimeType, findNonFiniteNumber, findDroppedKeys } from './content'; diff --git a/libs/utils/src/index.ts b/libs/utils/src/index.ts index ab1255ff4..bf100c814 100644 --- a/libs/utils/src/index.ts +++ b/libs/utils/src/index.ts @@ -38,7 +38,7 @@ export { } from './path'; // Content utilities -export { sanitizeToJson, inferMimeType, findNonFiniteNumber } from './content'; +export { sanitizeToJson, inferMimeType, findNonFiniteNumber, findDroppedKeys } from './content'; // HTTP utilities export { validateBaseUrl, buildSetCookie, parseCookies, getCookie, isLocalhost, isSecureRequest } from './http';