From 3a13686c3db2e915ec16e7f9fffcea93671d473d Mon Sep 17 00:00:00 2001 From: Noah Schatz Date: Sat, 29 Aug 2026 05:31:01 +0000 Subject: [PATCH] S0089-cli-agent-3: publish an output schema per MCP tool and conform every result Every advertised tool now declares an `outputSchema` (and a `title`), every dispatch path returns structured content conforming to the schema its own tool declared, and the text content block is the serialized JSON of that same structured value. The structured result carries `ok`, a `status` of success / verdict / failed, the `exit` code, a stable `code` on a failed call, and the tool's own payload under `data` (the parsed model and warnings, or a record stream for a multi-record input; the validation verdict and findings; the value-free structural summary; the converted Bundle). `status` is the declared property that separates a negative verdict about the message from a call that produced nothing, which the old `{ exit, ok }` pair could not express. Both handlers in `src/mcp/server.ts` are explicit allow-lists, so both were widened: `tools/list` now copies the title and the output schema, and `tools/call` passes the whole structured value through instead of rebuilding `{ exit, ok }`. PHI posture: on a failed call every property of the structured result is drawn from a fixed set (the outcome vocabulary, the exit-code contract, the CLI_CODES registry), so no part of the caller's input can appear in it. The diagnostic code is matched against the registry rather than lifted out of stderr, and an unknown tool name is no longer echoed back, because a tool name is caller-supplied text like any other argument. Test coverage validates each emitted structured value against its own tool's declared schema using a dependency-free checker (test/helpers) that refuses any schema keyword it does not implement, with a negative control proving it can fail; the text-block round trip; the advertised-schema assertions over the SDK client (which validates the replies itself once tools/list has been called); and the unhappy paths. The PHI-leak matrix now covers the agent surface. No dependency was added or moved. Tool names, tool count, tools/list ordering, input schemas, the exit-code contract and the verdict-vs-isError rule are unchanged. --- .changeset/olive-cameras-shave.md | 28 +++ CHANGELOG.md | 28 +++ README.md | 6 + docs-content/mcp.md | 44 ++++ src/mcp/index.ts | 3 + src/mcp/server.ts | 17 +- src/mcp/tools.ts | 328 +++++++++++++++++++++++++---- test/helpers/schema-conformance.ts | 166 +++++++++++++++ test/mcp-server.test.ts | 87 +++++++- test/mcp-tools.test.ts | 302 ++++++++++++++++++++++++-- test/phi-leak.test.ts | 64 ++++++ 11 files changed, 1016 insertions(+), 57 deletions(-) create mode 100644 .changeset/olive-cameras-shave.md create mode 100644 test/helpers/schema-conformance.ts diff --git a/.changeset/olive-cameras-shave.md b/.changeset/olive-cameras-shave.md new file mode 100644 index 0000000..8c6ccfd --- /dev/null +++ b/.changeset/olive-cameras-shave.md @@ -0,0 +1,28 @@ +--- +"@cosyte/cli": patch +--- + +Publish an output schema for every MCP tool, and make every tool result conform to it. + +An agent calling a cosyte tool previously received `structuredContent: { exit, ok }` with no schema to +check it against, and a text content block holding the command's stdout, which was a different value +from the structured one. Deciding whether a result held data or a diagnostic meant pattern-matching a +text blob. + +Each of the four tools now advertises an `outputSchema` (and a `title`) on `tools/list`, and every +dispatch path returns structured content conforming to the schema its own tool declared. The result +carries `ok`, a `status` of `success` / `verdict` / `failed`, the `exit` code from the documented +exit-code contract, a stable `code` on a failed call, and the tool's own payload under `data`: the +parsed model and warnings, the validation verdict and findings, the structural summary, or the +converted Bundle. `status` is the property that separates a negative verdict about the message (the +tool ran; the payload is present) from a call that produced nothing, which no text blob could +distinguish reliably. + +The text content block is now the serialized JSON of that same structured result, so a client that +reads only text sees exactly the value a schema-aware client validates. + +On a failed call the structured result is value-free by construction: every property is drawn from a +fixed set (the outcome vocabulary, the exit-code contract, the diagnostic-code registry), so no part +of the caller's input can appear in it. The tool name of an unknown tool is no longer echoed back, for +the same reason. Tool names, tool count, input schemas, the exit-code contract and the rule that a +parsed-but-invalid `validate` is a successful call are all unchanged, and no dependency was added. diff --git a/CHANGELOG.md b/CHANGELOG.md index b7ebee7..3629f46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,29 @@ still do. Each entry was assigned to the release whose tag first contains it, re ### Added +- **Every MCP tool now publishes an `outputSchema`, and every tool result conforms to it.** An agent + calling a cosyte tool used to receive `structuredContent: { exit, ok }` with no schema to check it + against, so deciding whether a result held data or a diagnostic meant pattern-matching a text blob. + Each of the four tools now advertises an output schema and a `title` on `tools/list`, and every + dispatch path (success, negative verdict, hard failure, usage error, internal error) returns + structured content conforming to the schema its own tool declared. + - **The result carries `ok`, `status`, `exit`, `code` and `data`.** `status` is `success`, + `verdict` or `failed`, and it is the property that separates a negative verdict about the message + (the tool ran, the payload is present, the call is not an error) from a call that produced + nothing: the distinction no text blob could make reliably. `code` is the stable diagnostic code on + a failed call. `data` is the tool's own payload: the parsed model and warnings (or a record stream + for a multi-record input), the validation verdict and findings, the value-free structural summary, + or the converted `Bundle`. + - **On a failed call the structured result is value-free by construction**, not by review: every + property is drawn from a fixed set (the outcome vocabulary, the exit-code contract, the diagnostic + code registry), so no part of the caller's input can appear in it. One consequence is deliberate: + an unknown tool name is no longer echoed back, because a tool name is caller-supplied text like + any other argument. + - Tool names, tool count, `tools/list` ordering, input schemas, the exit-code contract, and the rule + that a parsed-but-invalid `validate` is a successful call are all unchanged, and no dependency was + added. The suite validates each emitted result against its own tool's declared schema with a + dependency-free checker that refuses any schema keyword it does not implement. + - **The two-file agent-guidance contract is now gated (`pnpm check:agent-notes`).** `CLAUDE.md` was split from `documentation/agent-notes.md` on 2026-08-04, which made every anchor between them load-bearing, and nothing checked them. `scripts/check-agent-notes.ts` now verifies that the @@ -96,6 +119,11 @@ still do. Each entry was assigned to the release whose tag first contains it, re ### Changed +- **The MCP text content block is now the serialized JSON of the structured result**, replacing the + command's raw stdout (on a success) or its stderr diagnostic (on a failure). A client that reads + only text now sees exactly the value a schema-aware client validates: there is one value, serialized + once, so the two channels cannot disagree. A client that read the text block expecting the bare + command output will find the same payload one level down, under `data`. - **Multi-record `parse` output (`--ndjson` and MLLP) is now emitted record by record, as each record is parsed**, instead of being accumulated and written once at the end. The first line reaches stdout before the rest of the input has been read, so a bulk batch pipes into the next process instead of diff --git a/README.md b/README.md index aef6ef2..4d75c8a 100644 --- a/README.md +++ b/README.md @@ -323,6 +323,12 @@ and the agent get identical results. The PHI posture is inherited and hardened: tool _error_ carries only value-free diagnostics (a stable code + position, never an input value). A parsed-but-invalid `validate` is a **successful** call reporting the verdict, not a tool error. +**Every tool publishes an `outputSchema`**, so a client validates a result against a declared contract +rather than pattern-matching prose. Each result carries `ok`, a `status` of `success` / `verdict` / +`failed`, the `exit` code, a stable `code` on a failure, and the tool's own payload under `data`; the +text content block carries the serialized JSON of that same structured result. See the docs for the +per-tool payload shapes. + The MCP SDK (`@modelcontextprotocol/sdk`) is the CLI's only third-party runtime dependency; it is declared **optional** and loaded only on the MCP path, so a `cosyte parse` invocation never pulls it and the core works with the SDK absent. The server surface is importable via the `@cosyte/cli/mcp` subpath diff --git a/docs-content/mcp.md b/docs-content/mcp.md index 2db7052..4728178 100644 --- a/docs-content/mcp.md +++ b/docs-content/mcp.md @@ -54,6 +54,50 @@ cosyte mcp # start the server on stdio (also: cosyte-mcp) Every tool takes a `content` string (the raw message); `parse`/`validate`/`inspect` accept an optional `format` override. +## The result contract + +**Every tool publishes an `outputSchema`**, so a client validates a result against a declared contract +instead of pattern-matching a text blob to work out what it is holding. `tools/list` carries that +schema next to each tool's input schema and title; every reply from that tool conforms to it. + +The structured result has the same three outcome properties for every tool, plus that tool's own +payload: + +| Property | Type | What it says | +| -------- | --------- | --------------------------------------------------------------------------------- | +| `ok` | boolean | Whether the call produced data. A negative verdict about the message is still `true`. | +| `status` | string | `success`, `verdict`, or `failed`: the one property to branch on. | +| `exit` | integer | The exit code from the documented [exit-code contract](./reference-commands#exit-codes). | +| `code` | string | On a failed call, the stable diagnostic code. Absent when data was produced. | +| `data` | object | The tool's own payload. Absent on a failed call, which produced none. | + +`status` is the distinction a text blob could never make reliably: + +- **`success`** the operation completed cleanly. +- **`verdict`** the tool ran and reports a negative finding _about the message_: a resource that + parsed but is not conformant, or a conversion with an error-severity issue. The payload is present + and the call is not an error. +- **`failed`** the call produced no data: a usage mistake, unparseable input, an unavailable parser, + or an internal error. `code` says which, `data` is absent. + +`data` is the payload the terminal command puts on stdout: `parse` gives `format` + `model` + +`warnings` (or `records`, one entry per record, for a multi-record input), `validate` gives `format` + +`valid` + `findings`, `inspect` gives the value-free structural summary, and `convert` gives `format` + +`bundle` + `findings`. + +```json +{ + "ok": true, + "status": "verdict", + "exit": 1, + "data": { "format": "fhir", "valid": false, "findings": [{ "code": "value-not-in-set", "severity": "error", "location": "Patient.gender" }] } +} +``` + +**The text content block carries the serialized JSON of that same structured result**, so a client that +reads only text sees exactly the value a schema-aware client validates. There is one value, serialized +once: the two channels cannot disagree. + ## PHI posture on the agent surface The value-free discipline is **hardened** for agents: there is **no `--unsafe-show-values` door** over diff --git a/src/mcp/index.ts b/src/mcp/index.ts index 467b99c..26e25b9 100644 --- a/src/mcp/index.ts +++ b/src/mcp/index.ts @@ -13,7 +13,10 @@ export { TOOL_DEFS, type McpToolDef, type McpToolInputSchema, + type McpToolOutputSchema, type McpToolResult, type McpToolMeta, + type McpStructuredResult, + type McpToolStatus, type McpTextContent, } from "./tools.js"; diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 22a9026..0401be9 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -30,6 +30,10 @@ export const SERVER_INFO = { name: "cosyte", version: VERSION } as const; * the caller connects it to stdio (in production) or to an in-memory transport (in tests), so the * handler wiring is drivable without a subprocess. * + * Both handlers are **explicit about what reaches the wire**: `tools/list` copies each tool's title, + * description, input schema and **output schema**, and `tools/call` passes the whole structured result + * through, so what a client validates is exactly what the tool declared and produced. + * * @returns A configured, unconnected {@link Server}. * @example * ```ts @@ -45,6 +49,7 @@ export function createMcpServer(): Server { server.setRequestHandler(ListToolsRequestSchema, () => ({ tools: TOOL_DEFS.map((t) => ({ name: t.name, + title: t.title, description: t.description, inputSchema: { type: t.inputSchema.type, @@ -54,6 +59,14 @@ export function createMcpServer(): Server { ? { additionalProperties: t.inputSchema.additionalProperties } : {}), }, + // Every advertised tool publishes its result schema; a client validates a reply against it + // instead of pattern-matching the text block. Declared, so it is copied unconditionally. + outputSchema: { + type: t.outputSchema.type, + properties: { ...t.outputSchema.properties }, + required: [...t.outputSchema.required], + additionalProperties: t.outputSchema.additionalProperties, + }, })), })); @@ -63,7 +76,9 @@ export function createMcpServer(): Server { return { content: result.content.map((c) => ({ type: c.type, text: c.text })), isError: result.isError, - structuredContent: { exit: result.structuredContent.exit, ok: result.structuredContent.ok }, + // The whole structured value reaches the wire. A field-by-field copy here would silently + // truncate the payload the called tool's own schema promises. + structuredContent: { ...result.structuredContent }, }; }); diff --git a/src/mcp/tools.ts b/src/mcp/tools.ts index fd472a5..58199b3 100644 --- a/src/mcp/tools.ts +++ b/src/mcp/tools.ts @@ -17,6 +17,14 @@ * carries only the value-free diagnostic the command already produced (a stable code + positional * context), never an input value. * + * **The published contract.** Every tool declares an {@link McpToolOutputSchema}, and every dispatch + * path returns {@link McpStructuredResult} conforming to the schema its own tool declared, so a client + * validates an object instead of pattern-matching prose. The text content block carries the serialized + * JSON of that same structured value. On a **failed** call every field of the structured result is + * drawn from a fixed enumeration (the outcome vocabulary, the exit-code contract, the + * {@link CLI_CODES} registry), so no part of the caller's input can appear in it; the tool's own + * payload is present only on a call that produced data, which is the explicit request. + * * @packageDocumentation */ @@ -24,7 +32,7 @@ import { convertCommand } from "../commands/convert.js"; import { inspectCommand } from "../commands/inspect.js"; import { parseCommand } from "../commands/parse.js"; import { validateCommand } from "../commands/validate.js"; -import { CLI_CODES, CliError, toCliError } from "../core/diagnostics.js"; +import { CLI_CODES, CliError, toCliError, type CliCode } from "../core/diagnostics.js"; import { EXIT } from "../core/exit-codes.js"; import type { RunDeps } from "../core/io.js"; import { VALUE_FREE } from "../core/phi.js"; @@ -36,22 +44,49 @@ export interface McpTextContent { readonly text: string; } -/** Value-free metadata every tool result carries so an agent can branch on the outcome. */ -export interface McpToolMeta { - /** The CLI exit code the underlying command resolved to (the documented exit-code contract). */ - readonly exit: number; +/** + * The outcome of a tool call, as the single property a client branches on with **no text parsing**: + * `success` the operation completed cleanly, `verdict` the tool ran and reports a negative finding + * *about the message* (an invalid resource, an error-severity conversion issue), `failed` the call + * itself produced no data (a usage mistake, unparseable input, an unavailable parser, an internal + * error). `verdict` and `failed` are the two a text blob could never reliably separate. + */ +export type McpToolStatus = "success" | "verdict" | "failed"; + +/** + * The **structured result** of one tool call: the value-free outcome fields every tool carries, plus + * that tool's own payload on a call that produced data. This is the object an MCP `tools/call` reply + * carries as `structuredContent`, and the value whose serialization is the reply's text block. + */ +export interface McpStructuredResult { /** `true` iff the tool *call* succeeded (data was produced): distinct from a negative verdict. */ readonly ok: boolean; + /** The three-way outcome a client branches on: see {@link McpToolStatus}. */ + readonly status: McpToolStatus; + /** The CLI exit code the underlying command resolved to (the documented exit-code contract). */ + readonly exit: number; + /** The stable, value-free diagnostic code on a failed call; absent when data was produced. */ + readonly code?: CliCode; + /** The tool's own payload (the parsed model, the verdict, the summary, the Bundle); absent on a + * failed call, which produced none. */ + readonly data?: unknown; } +/** + * The historical name for a tool result's structured content, kept as the published alias of + * {@link McpStructuredResult}. + */ +export type McpToolMeta = McpStructuredResult; + /** * The value-free result of dispatching one MCP tool call. Structurally an MCP `CallToolResult`: a text - * content channel, an `isError` flag, and value-free `structuredContent` metadata. + * content channel carrying the serialized structured result, an `isError` flag, and the + * {@link McpStructuredResult} itself. */ export interface McpToolResult { readonly content: readonly McpTextContent[]; readonly isError: boolean; - readonly structuredContent: McpToolMeta; + readonly structuredContent: McpStructuredResult; } /** A JSON-Schema description of a tool's input (the wire schema advertised to `tools/list`). */ @@ -62,11 +97,30 @@ export interface McpToolInputSchema { readonly additionalProperties?: boolean; } -/** A tool advertised by the server: an agent-callable name, a description, and its input schema. */ +/** + * A JSON-Schema description of a tool's **structured result**: the schema advertised alongside the + * tool on `tools/list` and the one every reply from that tool conforms to. The protocol fixes the + * root type as `object`; `required` and `additionalProperties` are declared (never omitted) so the + * published contract states exactly which properties a client may rely on and forbids the rest. + */ +export interface McpToolOutputSchema { + readonly type: "object"; + readonly properties: Readonly>; + readonly required: readonly string[]; + readonly additionalProperties: boolean; +} + +/** + * A tool advertised by the server: an agent-callable name, a human-readable title, a description, and + * the schemas of its input and of its structured result. + */ export interface McpToolDef { readonly name: string; + /** A human-readable title, so these four generic names stay distinguishable in an aggregating client. */ + readonly title: string; readonly description: string; readonly inputSchema: McpToolInputSchema; + readonly outputSchema: McpToolOutputSchema; } /** The `content` property shared by every tool: the raw message text to operate on. */ @@ -89,6 +143,55 @@ const FORMAT_PROP = { }, } as const; +/** + * The outcome properties every tool's structured result carries, declared identically in every tool's + * output schema. Each is drawn from a **fixed enumeration** (the outcome vocabulary, the published + * exit-code contract, the {@link CLI_CODES} registry), which is what makes a failed call's structured + * result value-free by construction rather than by review. + */ +const OUTCOME_PROPS = { + ok: { + type: "boolean", + description: + "True iff the tool call produced data. A negative verdict about the message is still true " + + "(the tool worked); only a call that produced no data at all is false.", + }, + status: { + type: "string", + enum: ["success", "verdict", "failed"], + description: + "The outcome to branch on without parsing text: 'success' the operation completed cleanly, " + + "'verdict' the tool ran and reports a negative finding about the message, 'failed' the call " + + "produced no data (a usage mistake, unparseable input, an unavailable parser, an internal error).", + }, + exit: { + type: "integer", + enum: Object.values(EXIT), + description: + "The exit code the underlying command resolved to, from the documented exit-code contract.", + }, + code: { + type: "string", + enum: Object.values(CLI_CODES), + description: + "The stable diagnostic code on a failed call, drawn from a fixed registry so it never carries " + + "an input value. Absent on a call that produced data.", + }, +} as const; + +/** The properties every structured result must carry, whatever the outcome. */ +const OUTCOME_REQUIRED = ["ok", "status", "exit"] as const; + +/** Build one tool's output schema: the shared outcome properties plus that tool's own `data` payload. */ +function outputSchema(data: Readonly>): McpToolOutputSchema { + return { + type: "object", + properties: { ...OUTCOME_PROPS, data }, + required: [...OUTCOME_REQUIRED], + additionalProperties: false, + }; +} + /** * The tools this server exposes: the read/convert operations that share the `core` cleanly and whose * results are safe to hand an agent. `redact`/`deid` (gated on `@cosyte/deid`) @@ -98,6 +201,7 @@ const FORMAT_PROP = { export const TOOL_DEFS: readonly McpToolDef[] = [ { name: "parse", + title: "Parse a healthcare message", description: "Parse a healthcare message (HL7 v2 or FHIR R4) to typed JSON. Format is autodetected by content. " + "Returns the parsed model plus value-free warnings.", @@ -107,9 +211,30 @@ export const TOOL_DEFS: readonly McpToolDef[] = [ required: ["content"], additionalProperties: false, }, + outputSchema: outputSchema({ + type: "object", + description: + "The parse payload, present on a call that produced data. A single message parses to " + + "format + model + warnings. A multi-record input (an MLLP stream) parses to `records`, one " + + "entry per record: a parsed record, or a value-free per-record error code.", + properties: { + format: { type: "string", description: "The format the input was parsed as." }, + model: { description: "The wrapped library's parsed model, verbatim: the data channel." }, + warnings: { + type: "array", + description: "Value-free parse warnings: a stable code plus positional context.", + }, + records: { + type: "array", + description: "One entry per record of a multi-record input, in stream order.", + }, + }, + additionalProperties: false, + }), }, { name: "validate", + title: "Validate a healthcare message", description: "Validate a message and carry the verdict: ok=true with a valid result, or a result reporting " + "value-free findings. The parsed-but-invalid verdict is a successful call (not a tool error).", @@ -119,9 +244,30 @@ export const TOOL_DEFS: readonly McpToolDef[] = [ required: ["content"], additionalProperties: false, }, + outputSchema: outputSchema({ + type: "object", + description: + "The validation payload, present on a call that produced data (a valid result and a " + + "negative verdict alike). Value-free throughout: findings are codes, severities and " + + "positional locators, never a field value.", + properties: { + format: { type: "string", description: "The format the input was validated as." }, + valid: { + type: "boolean", + description: "The verdict: false means parsed-but-non-conformant, not a failed call.", + }, + findings: { + type: "array", + description: "Value-free findings: a stable code, a severity, and a positional locator.", + }, + }, + required: ["format", "valid", "findings"], + additionalProperties: false, + }), }, { name: "inspect", + title: "Inspect a healthcare message's structure", description: "Return a value-free structural summary of a message: its type, segment/entry counts, and a " + "warning/issue count. Never includes a field value.", @@ -131,9 +277,22 @@ export const TOOL_DEFS: readonly McpToolDef[] = [ required: ["content"], additionalProperties: false, }, + outputSchema: outputSchema({ + type: "object", + description: + "The value-free structural summary, present on a call that produced data. `format` names " + + "the shape of the rest, which is that format's own counts and structural type codes " + + "(a message type, a resource type, a transaction-set id): classification, never a value.", + properties: { + format: { type: "string", description: "The format the summary describes." }, + }, + required: ["format"], + additionalProperties: true, + }), }, { name: "convert", + title: "Convert HL7 v2 to a FHIR R4 Bundle", description: "Convert an HL7 v2 message to a FHIR R4 Bundle via @cosyte/transform. Returns the converted " + "Bundle; an error-severity conversion issue is reported with ok=false.", @@ -150,6 +309,23 @@ export const TOOL_DEFS: readonly McpToolDef[] = [ required: ["content"], additionalProperties: false, }, + outputSchema: outputSchema({ + type: "object", + description: + "The conversion payload, present on a call that produced data. An error-severity finding " + + "is a negative verdict (the Bundle is still returned), not a failed call.", + properties: { + format: { type: "string", description: "The conversion target the Bundle is in." }, + bundle: { type: "object", description: "The converted FHIR R4 Bundle: the data channel." }, + findings: { + type: "array", + description: + "Value-free conversion findings: a code, a severity, and a positional locator.", + }, + }, + required: ["format", "bundle", "findings"], + additionalProperties: false, + }), }, ]; @@ -176,31 +352,104 @@ function stringArg(args: Readonly>, key: string): string } /** - * Map a command's {@link RunResult} onto a value-free {@link McpToolResult}. A command emits its data on - * `stdout` (non-empty) and only ever leaves `stdout` empty on a **hard** failure (unparseable / no - * input / usage / unavailable / internal), so `stdout === ""` is exactly the "tool call failed" signal. - * A negative *verdict* (validate-invalid, a convert error-severity issue) still emits its JSON on - * stdout, so it is a successful call whose value-free payload reports the verdict. + * Wrap one {@link McpStructuredResult} as a tool result. The text content block is the **serialized + * JSON of that same structured value** (the protocol's backwards-compatibility route for a client that + * reads only text), so the two channels can never disagree: there is one value, serialized once. + * `isError` marks a failed *call*, which is exactly the `failed` status: a negative verdict about the + * message is a successful call and stays `isError: false`. */ -function toToolResult(result: RunResult): McpToolResult { - const hardError = result.stdout === ""; - const text = hardError ? result.stderr.trim() : result.stdout.trim(); +function structuredResult(structuredContent: McpStructuredResult): McpToolResult { return { - content: [ - { type: "text", text: text.length > 0 ? text : `cosyte: exit ${String(result.exit)}` }, - ], - isError: hardError, - structuredContent: { exit: result.exit, ok: !hardError }, + content: [{ type: "text", text: JSON.stringify(structuredContent) }], + isError: structuredContent.status === "failed", + structuredContent, }; } -/** Build a value-free usage-error tool result (a bad/missing argument, never echoes the argument). */ -function usageError(message: string): McpToolResult { - return { - content: [{ type: "text", text: `cosyte: ${CLI_CODES.CLI_USAGE}: ${message}` }], - isError: true, - structuredContent: { exit: EXIT.USAGE, ok: false }, - }; +/** + * True iff a parsed data-channel line is one record of a **record stream** rather than a whole + * message: `parse` gives every streamed record its own zero-based `record` index, and a single-message + * envelope never carries one. Used so a one-record stream is collected like any other stream instead of + * being mistaken for a single parsed message. + */ +function isRecordLine(value: unknown): boolean { + return typeof value === "object" && value !== null && "record" in value; +} + +/** + * Read a command's data channel back as the structured payload. Every dispatch runs its command under + * `--json`, so `stdout` is one compact JSON document, or (for a multi-record input) one per line; a + * record stream is collected under `records` so the payload has one declared shape whether the stream + * held one record or many. + */ +function parsePayload(stdout: string): unknown { + const values: unknown[] = []; + for (const line of stdout.split("\n")) { + if (line.trim().length === 0) continue; + try { + values.push(JSON.parse(line) as unknown); + // Defensive: every dispatch runs its command under `--json`, so the data channel is JSON on + // every path here. An unreadable one yields no payload rather than a half-parsed one. + /* v8 ignore start -- defensive: no dispatch path emits a non-JSON data channel */ + } catch { + return undefined; + } + /* v8 ignore stop */ + } + const only = values.length === 1 ? values[0] : undefined; + return only !== undefined && !isRecordLine(only) ? only : { records: values }; +} + +/** + * The stable diagnostic code a failed command reported, or `undefined` when its diagnostic carried + * none. **Value-free by construction**: the answer is a member of the {@link CLI_CODES} registry that + * the diagnostic line matched, never a substring lifted out of `stderr`, so no input value can reach + * the structured result through this door even if a future diagnostic were to carry one. + */ +function diagnosticCode(stderr: string): CliCode | undefined { + return Object.values(CLI_CODES).find((code) => stderr.includes(`cosyte: ${code}: `)); +} + +/** + * Map a command's {@link RunResult} onto an {@link McpToolResult}. A command emits its data on + * `stdout` (non-empty) and only ever leaves `stdout` empty on a **hard** failure (unparseable / no + * input / usage / unavailable / internal), so `stdout === ""` is exactly the "tool call failed" signal + * and resolves to `failed` plus the value-free diagnostic code. A negative *verdict* (validate-invalid, + * a convert error-severity issue) still emits its JSON on stdout: a successful call, distinguished from + * both a clean success and a failed call by its own `status`, carrying its payload like any other. + */ +function toToolResult(result: RunResult): McpToolResult { + if (result.stdout === "") { + const code = diagnosticCode(result.stderr); + return structuredResult({ + ok: false, + status: "failed", + exit: result.exit, + ...(code !== undefined ? { code } : {}), + }); + } + const data = parsePayload(result.stdout); + return structuredResult({ + ok: true, + status: result.exit === EXIT.OK ? "success" : "verdict", + exit: result.exit, + ...(data !== undefined ? { data } : {}), + }); +} + +/** + * Build a value-free usage-error tool result for a call that failed **before any tool ran** (an + * unknown tool name, a missing or non-string `content`). It carries the outcome, the exit code and the + * stable code, and **no part of what the caller sent**: not an argument value, and not the tool name + * either, which is caller-supplied text like any other and has no place in an agent's context. + */ +function usageError(): McpToolResult { + return structuredResult({ + ok: false, + status: "failed", + exit: EXIT.USAGE, + code: CLI_CODES.CLI_USAGE, + }); } /** @@ -208,15 +457,12 @@ function usageError(message: string): McpToolResult { * mirror of the terminal dispatcher's `try/catch → toCliError` boundary (`core/run.ts`). {@link toCliError} * discards the original message, so a library exception that embedded input bytes can never reach the * client (which would otherwise see the SDK surface the raw `error.message`). Both adapters inherit the - * value-free posture *in code*, not by trusting the wrapped libraries never to throw. + * value-free posture *in code*, not by trusting the wrapped libraries never to throw. Only the error's + * registry code and its exit code reach the result; its message never does. */ function internalError(e: unknown): McpToolResult { const err = toCliError(e); - return { - content: [{ type: "text", text: `cosyte: ${err.code}: ${err.message}` }], - isError: true, - structuredContent: { exit: err.exit, ok: false }, - }; + return structuredResult({ ok: false, status: "failed", exit: err.exit, code: err.code }); } /** @@ -226,6 +472,11 @@ function internalError(e: unknown): McpToolResult { * posture), and maps the {@link RunResult} onto an MCP result. An unknown tool name or a missing * `content` argument is a value-free usage error, never a thrown stack trace carrying input. * + * Every path returns an {@link McpStructuredResult} conforming to the called tool's declared + * {@link McpToolDef.outputSchema}, with the same value serialized into the text content block. An + * unknown tool name has no schema of its own, so it answers with the shared outcome fields alone, + * which every tool's schema declares. + * * @param name - The tool name (one of {@link TOOL_DEFS}). * @param args - The tool-call arguments object. * @param deps - Optional {@link RunDeps} override (tests inject fakes); defaults to feeding `content` as @@ -238,6 +489,7 @@ function internalError(e: unknown): McpToolResult { * * const r = await dispatchTool("parse", { content: '{"resourceType":"Patient"}' }); * r.isError; // => false + * r.structuredContent.status; // => "success" * ``` */ export async function dispatchTool( @@ -246,9 +498,7 @@ export async function dispatchTool( deps?: RunDeps, ): Promise { const content = stringArg(args, "content"); - if (content === null) { - return usageError("missing required 'content' argument (the message text to operate on)"); - } + if (content === null) return usageError(); const runDeps = deps ?? inlineDeps(content); const format = stringArg(args, "format"); const fmtFlag = format !== null ? ["--format", format] : []; @@ -268,7 +518,9 @@ export async function dispatchTool( return toToolResult(await convertCommand(["-", "--to", to, "--json"], runDeps, VALUE_FREE)); } default: - return usageError(`unknown tool '${name}'`); + // The tool name is caller-supplied text, so it is answered with a value-free usage error and + // never echoed back: the same posture the arguments get. + return usageError(); } } catch (e) { // The agent-surface mirror of core/run.ts's dispatcher boundary: any unexpected throw becomes a diff --git a/test/helpers/schema-conformance.ts b/test/helpers/schema-conformance.ts new file mode 100644 index 0000000..f55c328 --- /dev/null +++ b/test/helpers/schema-conformance.ts @@ -0,0 +1,166 @@ +/** + * A **dependency-free JSON-Schema conformance checker**, sized to exactly the keyword subset the MCP + * output schemas in `src/mcp/tools.ts` use. This repo declares **zero third-party CLI-core runtime + * deps** and a hard dependency cap, so the suite validates the published contract with this instead of + * pulling in a validator. + * + * Two properties make that safe to rely on: + * + * 1. **It refuses what it does not implement.** An unknown keyword or an unknown `type` throws rather + * than being ignored, so the checker can never report green over a schema it did not understand. + * That is the failure mode a hand-rolled validator normally has, and it is closed here by + * construction. `test/mcp-tools.test.ts` pins it with a negative control. + * 2. **It never echoes a value.** A violation names the JSON path and the expectation, never the data + * at that path, so a failing assertion cannot print an input value into a CI log. That is the same + * value-free posture the surface under test is being checked for. + * + * @packageDocumentation + */ + +/** The JSON-Schema keywords this checker implements. Any other keyword in a schema is a refusal. */ +const SUPPORTED_KEYWORDS: ReadonlySet = new Set([ + "type", + "enum", + "properties", + "required", + "additionalProperties", + "description", +]); + +/** The JSON-Schema primitive types this checker implements. Any other `type` is a refusal. */ +const SUPPORTED_TYPES: ReadonlySet = new Set([ + "object", + "array", + "string", + "number", + "integer", + "boolean", + "null", +]); + +/** True iff `value` is a plain JSON object (not an array, not `null`). */ +function isJsonObject(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** The JSON type name of `value`, for a violation message. Names the type only, never the value. */ +function typeName(value: unknown): string { + if (value === null) return "null"; + if (Array.isArray(value)) return "array"; + return typeof value; +} + +/** True iff `value` satisfies the JSON-Schema `type` keyword `type`. Throws on an unimplemented type. */ +function matchesType(type: string, value: unknown): boolean { + if (!SUPPORTED_TYPES.has(type)) { + throw new Error(`schema-conformance: unimplemented JSON-Schema type '${type}'`); + } + switch (type) { + case "object": + return isJsonObject(value); + case "array": + return Array.isArray(value); + case "integer": + return typeof value === "number" && Number.isInteger(value); + case "number": + return typeof value === "number" && Number.isFinite(value); + case "null": + return value === null; + default: + return typeof value === type; + } +} + +/** + * Check `value` against `schema` and return every violation as a value-free message. An empty array + * means the value conforms. + * + * @param schema - A JSON Schema using only the implemented keyword subset. + * @param value - The value to check. + * @param path - The JSON path prefix used in violation messages (defaults to the root). + * @returns One value-free message per violation; `[]` when the value conforms. + * @throws {Error} If the schema uses a keyword or a `type` this checker does not implement, so an + * unhandled schema can never be reported as a pass. + * @example + * ```ts + * import { schemaViolations } from "./schema-conformance.js"; + * + * schemaViolations({ type: "object", properties: {}, required: ["a"] }, {}).length; // => 1 + * ``` + */ +export function schemaViolations(schema: unknown, value: unknown, path = "$"): string[] { + if (!isJsonObject(schema)) { + throw new Error(`schema-conformance: the schema at ${path} is not a JSON-Schema object`); + } + for (const keyword of Object.keys(schema)) { + if (!SUPPORTED_KEYWORDS.has(keyword)) { + throw new Error( + `schema-conformance: unimplemented JSON-Schema keyword '${keyword}' at ${path}`, + ); + } + } + + const violations: string[] = []; + const type = schema["type"]; + if (typeof type === "string" && !matchesType(type, value)) { + // The value is the wrong shape entirely; checking its members would only add noise. + return [`${path}: expected type '${type}', got '${typeName(value)}'`]; + } + + const members = schema["enum"]; + if (Array.isArray(members) && !members.includes(value)) { + violations.push( + `${path}: value is not one of the ${String(members.length)} declared enum members`, + ); + } + + if (!isJsonObject(value)) return violations; + + // `Object.hasOwn`, never `in`: an inherited `constructor`/`toString` must not read as a declared + // property (which would let an undeclared key through) or as a present one. + const declared = schema["properties"]; + const properties = isJsonObject(declared) ? declared : {}; + const named = schema["required"]; + const required = Array.isArray(named) ? named : []; + for (const key of required) { + const name = String(key); + if (!Object.hasOwn(value, name)) + violations.push(`${path}.${name}: required property is missing`); + } + if (schema["additionalProperties"] === false) { + for (const key of Object.keys(value)) { + if (!Object.hasOwn(properties, key)) { + violations.push(`${path}.${key}: undeclared property (additionalProperties is false)`); + } + } + } + for (const [key, subSchema] of Object.entries(properties)) { + if (Object.hasOwn(value, key)) { + violations.push(...schemaViolations(subSchema, value[key], `${path}.${key}`)); + } + } + return violations; +} + +/** + * Assert that `value` conforms to `schema`, throwing a value-free error listing every violation. + * + * @param schema - A JSON Schema using only the implemented keyword subset. + * @param value - The value to check. + * @param label - A label for the error message (which tool / which case is being checked). + * @throws {Error} If the value violates the schema, or the schema uses an unimplemented keyword. + * @example + * ```ts + * import { assertConforms } from "./schema-conformance.js"; + * + * assertConforms({ type: "object", properties: {} }, {}, "empty"); // => undefined + * ``` + */ +export function assertConforms(schema: unknown, value: unknown, label: string): void { + const violations = schemaViolations(schema, value); + if (violations.length > 0) { + throw new Error( + `${label} does not conform to its declared schema:\n ${violations.join("\n ")}`, + ); + } +} diff --git a/test/mcp-server.test.ts b/test/mcp-server.test.ts index c4c96fb..63fe9f4 100644 --- a/test/mcp-server.test.ts +++ b/test/mcp-server.test.ts @@ -3,12 +3,20 @@ import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { createMcpServer, SERVER_INFO } from "../src/mcp/server.js"; +import { TOOL_DEFS } from "../src/mcp/tools.js"; +import { assertConforms } from "./helpers/schema-conformance.js"; /** * Integration test for the MCP stdio adapter (`src/mcp/server.ts`), driven over the SDK's in-process - * transport (cli roadmap §6 "MCP tool tests"). A real {@link Client} connects to the server through a - * linked in-memory transport pair, lists the tools, and calls them: exercising the ListTools and - * CallTool handlers the same way an LLM client would, without spawning a subprocess. + * transport. A real {@link Client} connects to the server through a linked in-memory transport pair, + * lists the tools, and calls them: exercising the ListTools and CallTool handlers the same way an LLM + * client would, without spawning a subprocess. + * + * Both handlers copy fields by an explicit allow-list, so this is the suite that proves the published + * output schema and the whole structured result actually reach the wire rather than stopping at + * `tools.ts`. Where a test calls `listTools()` before `callTool()`, the SDK client also validates the + * reply against the advertised schema itself, so the server is checked by the protocol's own client + * as well as by this repo's checker. */ describe("cosyte MCP server, in-process client/server", () => { let client: Client; @@ -38,6 +46,29 @@ describe("cosyte MCP server, in-process client/server", () => { } }); + it("advertises an output schema and a title for EVERY tool; none lacks one", async () => { + const { tools } = await client.listTools(); + expect(tools).toHaveLength(TOOL_DEFS.length); + for (const t of tools) { + expect(t.title, `${t.name} carries a title`).toBeTruthy(); + expect(t.outputSchema, `${t.name} carries an output schema`).toBeDefined(); + expect(t.outputSchema?.type, `${t.name} output schema root type`).toBe("object"); + expect(Object.keys(t.outputSchema?.properties ?? {}).sort()).toEqual([ + "code", + "data", + "exit", + "ok", + "status", + ]); + expect(t.outputSchema?.required).toEqual(["ok", "status", "exit"]); + } + // The advertised schema is the declared one, not a truncated copy made by the handler. + for (const def of TOOL_DEFS) { + const wire = tools.find((t) => t.name === def.name); + expect(wire?.outputSchema).toEqual(def.outputSchema); + } + }); + it("parse over tools/call returns the typed model and is not an error", async () => { const res = await client.callTool({ name: "parse", @@ -70,4 +101,54 @@ describe("cosyte MCP server, in-process client/server", () => { const content = res.content as { type: string; text: string }[]; expect(content[0]?.text).toContain("CLI_FORMAT_UNDETECTED"); }); + + it("every tool's reply over the wire conforms to that tool's advertised schema", async () => { + const HL7 = + "MSH|^~\\&|A|B|C|D|20240101120000||ADT^A01|1|P|2.5\rEVN|A01|20240101120000\rPID|1||X^^^H^MR||DOE^JANE||19800101|F\r"; + const cases: { tool: string; label: string; args: Record }[] = [ + { tool: "parse", label: "success", args: { content: '{"resourceType":"Patient"}' } }, + { tool: "parse", label: "hard failure", args: { content: "not a healthcare message" } }, + { tool: "validate", label: "success", args: { content: '{"resourceType":"Patient"}' } }, + { + tool: "validate", + label: "negative verdict", + args: { content: '{"resourceType":"Patient","gender":"purple"}' }, + }, + { tool: "validate", label: "hard failure", args: { content: "" } }, + { tool: "inspect", label: "success", args: { content: HL7 } }, + { tool: "inspect", label: "hard failure", args: { content: "" } }, + { tool: "convert", label: "success", args: { content: HL7 } }, + { tool: "convert", label: "hard failure", args: { content: HL7, to: "x12" } }, + ]; + + // listTools() first: it is what caches the SDK client's own validator for each advertised + // schema, so every callTool below is validated by the protocol client as well as here. + const { tools } = await client.listTools(); + for (const c of cases) { + const res = await client.callTool({ name: c.tool, arguments: c.args }); + const schema = tools.find((t) => t.name === c.tool)?.outputSchema; + assertConforms(schema, res.structuredContent, `${c.tool}/${c.label} over the wire`); + // The text block is the serialization of that same structured value. + const content = res.content as { type: string; text: string }[]; + expect(content, `${c.tool}/${c.label}: one text block`).toHaveLength(1); + expect(JSON.parse(content[0]?.text ?? ""), `${c.tool}/${c.label}`).toStrictEqual( + res.structuredContent, + ); + } + }); + + it("passes the WHOLE structured result to the wire, payload included", async () => { + await client.listTools(); + const res = await client.callTool({ + name: "validate", + arguments: { content: '{"resourceType":"Patient","gender":"purple"}' }, + }); + // Not truncated to the outcome fields: the verdict an agent came for is still reachable. + expect(res.structuredContent).toMatchObject({ + ok: true, + status: "verdict", + exit: 1, + data: { format: "fhir", valid: false }, + }); + }); }); diff --git a/test/mcp-tools.test.ts b/test/mcp-tools.test.ts index f97a2f2..426f385 100644 --- a/test/mcp-tools.test.ts +++ b/test/mcp-tools.test.ts @@ -1,18 +1,63 @@ import { describe, expect, it } from "vitest"; import type { RunDeps } from "../src/core/io.js"; -import { dispatchTool, TOOL_DEFS } from "../src/mcp/tools.js"; +import { CLI_CODES } from "../src/core/diagnostics.js"; +import { EXIT } from "../src/core/exit-codes.js"; +import { dispatchTool, TOOL_DEFS, type McpToolResult } from "../src/mcp/tools.js"; +import { assertConforms, schemaViolations } from "./helpers/schema-conformance.js"; /** * Unit tests for the SDK-free MCP tool surface (`src/mcp/tools.ts`). The tools are the *second adapter* * over the same command core; these tests prove the dispatch/mapping without any `@modelcontextprotocol` * transport: the SDK wiring is covered separately in `mcp-server.test.ts`. + * + * The published contract is checked here in three layers: every emitted structured value is validated + * against **its own tool's** declared `outputSchema` (with a dependency-free checker, proved able to + * fail), the text content block is proved to be the serialization of that same structured value, and + * the value-free posture is asserted over the whole serialized result on every failure path. */ // Synthetic, PHI-free fixtures (mirrors dispatch.test.ts). const FHIR_PATIENT = '{"resourceType":"Patient","gender":"male"}'; const HL7_ADT = "MSH|^~\\&|A|B|C|D|20240101120000||ADT^A01|1|P|2.5\rEVN|A01|20240101120000\rPID|1||X^^^H^MR||DOE^JANE||19800101|F\r"; +/** One MLLP frame around the HL7 message (VT opens it, FS + CR close it): a multi-record `parse` input. */ +const MLLP_ONE_FRAME = + String.fromCharCode(0x0b) + HL7_ADT + String.fromCharCode(0x1c) + String.fromCharCode(0x0d); +const MLLP_TWO_FRAMES = MLLP_ONE_FRAME + MLLP_ONE_FRAME; + +/** The declared output schema of one advertised tool, by name. */ +function outputSchemaOf(tool: string): unknown { + const def = TOOL_DEFS.find((t) => t.name === tool); + if (def === undefined) throw new Error(`no advertised tool named '${tool}'`); + return def.outputSchema; +} + +/** Narrow a `parse` payload to its record stream, without a cast. */ +function recordsOf(data: unknown): readonly unknown[] { + if ( + typeof data === "object" && + data !== null && + "records" in data && + Array.isArray(data.records) + ) { + return data.records; + } + throw new Error("expected a record-stream payload carrying `records`"); +} + +/** + * The one assertion every dispatch case runs: the structured content conforms to the called tool's + * OWN declared schema, and the text content block is the serialization of that same value. + */ +function assertPublishedContract(tool: string, r: McpToolResult, label: string): void { + assertConforms(outputSchemaOf(tool), r.structuredContent, label); + expect(r.content, `${label}: exactly one text content block`).toHaveLength(1); + expect(r.content[0]?.type).toBe("text"); + expect(JSON.parse(r.content[0]?.text ?? ""), `${label}: text block round trip`).toStrictEqual( + r.structuredContent, + ); +} describe("TOOL_DEFS", () => { it("advertises the four wired tools, each with a required `content` input", () => { @@ -23,27 +68,129 @@ describe("TOOL_DEFS", () => { expect(t.description.length).toBeGreaterThan(0); } }); + + it("declares an output schema and a title for every advertised tool (none lacks one)", () => { + for (const t of TOOL_DEFS) { + expect(t.title.length, `${t.name} has a title`).toBeGreaterThan(0); + expect(t.outputSchema.type, `${t.name} output schema root type`).toBe("object"); + // Every tool's result carries the same branchable outcome properties. + expect([...t.outputSchema.required].sort()).toEqual(["exit", "ok", "status"]); + expect(t.outputSchema.additionalProperties).toBe(false); + expect(Object.keys(t.outputSchema.properties).sort()).toEqual([ + "code", + "data", + "exit", + "ok", + "status", + ]); + } + }); + + it("gives each tool its OWN payload schema (not one schema shared under four names)", () => { + const payloads = TOOL_DEFS.map((t) => JSON.stringify(t.outputSchema.properties["data"])); + expect(new Set(payloads).size).toBe(TOOL_DEFS.length); + }); +}); + +describe("schema conformance checker (the dependency-free validator itself)", () => { + const SCHEMA = { + type: "object", + properties: { + ok: { type: "boolean" }, + status: { type: "string", enum: ["success", "failed"] }, + exit: { type: "integer" }, + }, + required: ["ok", "status", "exit"], + additionalProperties: false, + }; + const VALID = { ok: true, status: "success", exit: 0 }; + + it("passes a conforming value", () => { + expect(schemaViolations(SCHEMA, VALID)).toEqual([]); + }); + + it("says NO to a deliberately corrupted value, one way per declared constraint", () => { + // A required property removed. + expect(schemaViolations(SCHEMA, { status: "success", exit: 0 })).toHaveLength(1); + // A declared type violated. + expect(schemaViolations(SCHEMA, { ...VALID, exit: "0" })).toHaveLength(1); + // A declared enum violated. + expect(schemaViolations(SCHEMA, { ...VALID, status: "not-a-status" })).toHaveLength(1); + // An undeclared property, with additionalProperties: false. + expect(schemaViolations(SCHEMA, { ...VALID, surprise: 1 })).toHaveLength(1); + // The root itself the wrong type. + expect(schemaViolations(SCHEMA, [])).toHaveLength(1); + }); + + it("REFUSES a schema keyword or type it does not implement (never a silent pass)", () => { + expect(() => schemaViolations({ type: "object", patternProperties: {} }, {})).toThrow( + /unimplemented JSON-Schema keyword 'patternProperties'/, + ); + expect(() => schemaViolations({ type: "tuple" }, [])).toThrow( + /unimplemented JSON-Schema type 'tuple'/, + ); + }); + + it("names the path and the expectation in a violation, never the value at that path", () => { + const secret = "ZZZNEVERPRINTED"; + const violations = schemaViolations(SCHEMA, { ...VALID, status: secret }); + expect(violations).toHaveLength(1); + expect(violations.join("\n")).not.toContain(secret); + expect(violations[0]).toContain("$.status"); + }); + + it("does not mistake an inherited property for a declared or a present one", () => { + expect(schemaViolations({ type: "object", required: ["toString"] }, {})).toHaveLength(1); + expect( + schemaViolations( + { type: "object", properties: {}, additionalProperties: false }, + { constructor: 1 }, + ), + ).toHaveLength(1); + }); }); describe("dispatchTool: success paths (shared core, value-free)", () => { - it("parse returns the typed model, ok=true, exit 0", async () => { + it("parse returns the typed model, ok=true, exit 0, conforming to parse's schema", async () => { const r = await dispatchTool("parse", { content: FHIR_PATIENT }); expect(r.isError).toBe(false); - expect(r.structuredContent).toEqual({ exit: 0, ok: true }); + expect(r.structuredContent.ok).toBe(true); + expect(r.structuredContent.status).toBe("success"); + expect(r.structuredContent.exit).toBe(EXIT.OK); + expect(r.structuredContent.code).toBeUndefined(); + // The data an agent gets today is still there, now under a declared property. + expect(r.structuredContent.data).toMatchObject({ format: "fhir" }); expect(r.content[0]?.text).toContain('"fhir"'); + assertPublishedContract("parse", r, "parse/success"); }); it("parse honours an explicit --format override (the fmtFlag branch)", async () => { const r = await dispatchTool("parse", { content: HL7_ADT, format: "hl7" }); expect(r.isError).toBe(false); expect(r.content[0]?.text).toContain('"hl7"'); + assertPublishedContract("parse", r, "parse/format-override"); + }); + + it("parse of a multi-record (MLLP) input carries the record stream, still conforming", async () => { + for (const [label, content, count] of [ + ["one frame", MLLP_ONE_FRAME, 1], + ["two frames", MLLP_TWO_FRAMES, 2], + ] as const) { + const r = await dispatchTool("parse", { content }); + expect(r.isError, label).toBe(false); + expect(recordsOf(r.structuredContent.data), label).toHaveLength(count); + assertPublishedContract("parse", r, `parse/mllp ${label}`); + } }); it("validate carries a VALID verdict as a successful call (exit 0)", async () => { const r = await dispatchTool("validate", { content: FHIR_PATIENT }); expect(r.isError).toBe(false); expect(r.structuredContent.ok).toBe(true); + expect(r.structuredContent.status).toBe("success"); + expect(r.structuredContent.data).toMatchObject({ valid: true }); expect(r.content[0]?.text).toContain('"valid":true'); + assertPublishedContract("validate", r, "validate/valid"); }); it("validate carries an INVALID verdict as a successful call (exit 1, not a tool error)", async () => { @@ -51,20 +198,34 @@ describe("dispatchTool: success paths (shared core, value-free)", () => { content: '{"resourceType":"Patient","gender":"purple"}', }); expect(r.isError).toBe(false); // the tool worked; the verdict is negative - expect(r.structuredContent.exit).toBe(1); + expect(r.structuredContent.ok).toBe(true); + expect(r.structuredContent.status).toBe("verdict"); + expect(r.structuredContent.exit).toBe(EXIT.INVALID); + expect(r.structuredContent.data).toMatchObject({ valid: false }); expect(r.content[0]?.text).toContain('"valid":false'); + assertPublishedContract("validate", r, "validate/invalid"); }); it("inspect returns a value-free structural summary", async () => { - const r = await dispatchTool("inspect", { content: HL7_ADT }); - expect(r.isError).toBe(false); - expect(r.content[0]?.text).toContain('"hl7"'); + for (const [format, content] of [ + ["hl7", HL7_ADT], + ["fhir", FHIR_PATIENT], + ] as const) { + const r = await dispatchTool("inspect", { content }); + expect(r.isError, format).toBe(false); + expect(r.structuredContent.data, format).toMatchObject({ format }); + expect(r.content[0]?.text, format).toContain(`"${format}"`); + assertPublishedContract("inspect", r, `inspect/${format}`); + } }); - it("convert (HL7 v2 → FHIR) returns the Bundle, ok=true", async () => { + it("convert (HL7 v2 to FHIR) returns the Bundle, ok=true", async () => { const r = await dispatchTool("convert", { content: HL7_ADT }); expect(r.isError).toBe(false); + expect(r.structuredContent.status).toBe("success"); + expect(r.structuredContent.data).toMatchObject({ format: "fhir" }); expect(r.content[0]?.text).toContain('"resourceType":"Bundle"'); + assertPublishedContract("convert", r, "convert/success"); }); }); @@ -72,52 +233,130 @@ describe("dispatchTool: value-free error paths", () => { it("a missing `content` argument is a value-free usage error", async () => { const r = await dispatchTool("parse", {}); expect(r.isError).toBe(true); - expect(r.structuredContent).toEqual({ exit: 2, ok: false }); + expect(r.structuredContent).toStrictEqual({ + ok: false, + status: "failed", + exit: EXIT.USAGE, + code: CLI_CODES.CLI_USAGE, + }); expect(r.content[0]?.text).toContain("CLI_USAGE"); + assertPublishedContract("parse", r, "parse/missing-content"); }); it("a non-string `content` argument is a usage error (never coerced)", async () => { const r = await dispatchTool("parse", { content: 123 }); expect(r.isError).toBe(true); - expect(r.content[0]?.text).toContain("CLI_USAGE"); + expect(r.structuredContent.status).toBe("failed"); + expect(r.structuredContent.code).toBe(CLI_CODES.CLI_USAGE); + expect(r.structuredContent.data).toBeUndefined(); + assertPublishedContract("parse", r, "parse/non-string-content"); }); - it("an unknown tool name is a value-free usage error naming the tool, not the input", async () => { + it("an EMPTY `content` string is a failed call, never a successful parse of nothing", async () => { + for (const tool of ["parse", "validate", "inspect", "convert"]) { + const r = await dispatchTool(tool, { content: "" }); + expect(r.isError, tool).toBe(true); + expect(r.structuredContent.ok, tool).toBe(false); + expect(r.structuredContent.status, tool).toBe("failed"); + expect(r.structuredContent.data, tool).toBeUndefined(); + expect(r.structuredContent.code, tool).toBe(CLI_CODES.CLI_EMPTY_INPUT); + expect(r.structuredContent.exit, tool).toBe(EXIT.DATAERR); + assertPublishedContract(tool, r, `${tool}/empty-content`); + } + }); + + it("an unknown tool name is a failed call that does not echo the caller's tool name", async () => { const r = await dispatchTool("frobnicate", { content: FHIR_PATIENT }); expect(r.isError).toBe(true); - expect(r.content[0]?.text).toContain("unknown tool 'frobnicate'"); + expect(r.structuredContent).toStrictEqual({ + ok: false, + status: "failed", + exit: EXIT.USAGE, + code: CLI_CODES.CLI_USAGE, + }); + // The tool name is caller-supplied text: it must not come back on any channel. + expect(JSON.stringify(r)).not.toContain("frobnicate"); + // The shared outcome fields are declared by every tool's schema, so an unroutable call still + // conforms to whichever tool schema a client reaches for. + for (const t of TOOL_DEFS) assertPublishedContract(t.name, r, `unknown-tool vs ${t.name}`); }); it("an unparseable input is a hard tool error (isError, exit 65) with a value-free code", async () => { const r = await dispatchTool("parse", { content: "this is not a healthcare message" }); expect(r.isError).toBe(true); - expect(r.structuredContent.exit).toBe(65); + expect(r.structuredContent.status).toBe("failed"); + expect(r.structuredContent.exit).toBe(EXIT.DATAERR); + expect(r.structuredContent.code).toBe(CLI_CODES.CLI_FORMAT_UNDETECTED); + expect(r.structuredContent.data).toBeUndefined(); expect(r.content[0]?.text).toContain("CLI_FORMAT_UNDETECTED"); + assertPublishedContract("parse", r, "parse/undetected"); + }); + + it("an operation its parser does not support is a hard tool error carrying its own code", async () => { + const r = await dispatchTool("parse", { content: FHIR_PATIENT, format: "dicom" }); + expect(r.isError).toBe(true); + expect(r.structuredContent.status).toBe("failed"); + expect(r.structuredContent.code).toBe(CLI_CODES.CLI_FORMAT_UNSUPPORTED); + assertPublishedContract("parse", r, "parse/unsupported-op"); }); it("convert with an unsupported --to target is a hard tool error (usage), never a fake conversion", async () => { const r = await dispatchTool("convert", { content: HL7_ADT, to: "x12" }); expect(r.isError).toBe(true); expect(r.structuredContent.ok).toBe(false); + expect(r.structuredContent.status).toBe("failed"); + expect(r.structuredContent.code).toBe(CLI_CODES.CLI_USAGE); + expect(r.structuredContent.data).toBeUndefined(); + // The rejected target is a caller argument: it must not reach the result. + expect(JSON.stringify(r)).not.toContain("x12"); + assertPublishedContract("convert", r, "convert/bad-target"); }); }); describe("dispatchTool: PHI posture (no value ever reaches a tool error)", () => { const SENTINEL = "ZZZSENTINELPHI"; + /** Every value a failed call's structured result may carry, as fixed sets a value cannot join. */ + const ENUMERATED = new Set([ + ...Object.values(CLI_CODES), + ...Object.values(EXIT), + "success", + "verdict", + "failed", + true, + false, + ]); it("an invalid resource's value-free findings never echo a field value", async () => { const r = await dispatchTool("validate", { content: `{"resourceType":"Patient","gender":"purple","name":[{"family":"${SENTINEL}"}]}`, }); // A negative verdict, reported with value-free findings (codes + FHIRPath only). - expect(r.structuredContent.exit).toBe(1); + expect(r.structuredContent.exit).toBe(EXIT.INVALID); + expect(r.structuredContent.status).toBe("verdict"); + expect(r.structuredContent.ok).toBe(true); expect(JSON.stringify(r)).not.toContain(SENTINEL); + expect(JSON.stringify(r.structuredContent)).not.toContain(SENTINEL); + assertPublishedContract("validate", r, "validate/sentinel"); + }); + + it("a negative verdict and a failed call differ in a declared, branchable property", async () => { + const verdict = await dispatchTool("validate", { + content: `{"resourceType":"Patient","gender":"purple","name":[{"family":"${SENTINEL}"}]}`, + }); + const failed = await dispatchTool("parse", { content: `garbage ${SENTINEL} bytes` }); + // `status` and `ok` are declared properties of both tools' schemas; either settles it with no + // text parsing at all. + expect(verdict.structuredContent.status).toBe("verdict"); + expect(failed.structuredContent.status).toBe("failed"); + expect(verdict.structuredContent.ok).not.toBe(failed.structuredContent.ok); + for (const r of [verdict, failed]) expect(JSON.stringify(r)).not.toContain(SENTINEL); }); it("a hard parse error never echoes the offending input (no unsafe door on the agent surface)", async () => { const r = await dispatchTool("parse", { content: `garbage ${SENTINEL} bytes` }); expect(r.isError).toBe(true); expect(JSON.stringify(r)).not.toContain(SENTINEL); + assertPublishedContract("parse", r, "parse/sentinel"); }); it("an unexpected exception is scrubbed to a value-free CLI_INTERNAL (never surfaced to the client)", async () => { @@ -129,8 +368,41 @@ describe("dispatchTool: PHI posture (no value ever reaches a tool error)", () => }; const r = await dispatchTool("parse", { content: "anything" }, boom); expect(r.isError).toBe(true); - expect(r.structuredContent.exit).toBe(70); + expect(r.structuredContent.exit).toBe(EXIT.SOFTWARE); + expect(r.structuredContent.status).toBe("failed"); + expect(r.structuredContent.code).toBe(CLI_CODES.CLI_INTERNAL); + expect(r.structuredContent.data).toBeUndefined(); expect(r.content[0]?.text).toContain("CLI_INTERNAL"); expect(JSON.stringify(r)).not.toContain(SENTINEL); + assertPublishedContract("parse", r, "parse/internal-error"); + }); + + it("EVERY failed-call structured result is built only from enumerated, value-free tokens", async () => { + const boom: RunDeps = { + readFile: () => Promise.reject(new Error(`SECRET ${SENTINEL} IN MESSAGE`)), + readStdin: () => Promise.reject(new Error(`SECRET ${SENTINEL} IN MESSAGE`)), + }; + const failures: [string, McpToolResult][] = [ + ["missing content", await dispatchTool("parse", {})], + ["non-string content", await dispatchTool("parse", { content: 1 })], + ["unknown tool", await dispatchTool(`frobnicate-${SENTINEL}`, { content: FHIR_PATIENT })], + ["empty content", await dispatchTool("validate", { content: "" })], + ["unparseable", await dispatchTool("parse", { content: `garbage ${SENTINEL}` })], + ["unsupported target", await dispatchTool("convert", { content: HL7_ADT, to: SENTINEL })], + ["unsupported op", await dispatchTool("parse", { content: FHIR_PATIENT, format: "dicom" })], + ["internal error", await dispatchTool("parse", { content: "anything" }, boom)], + ]; + for (const [label, r] of failures) { + const sc = r.structuredContent; + expect(sc.status, label).toBe("failed"); + expect(sc.data, label).toBeUndefined(); + // Exactly these four properties, each drawn from a fixed set no input value can join. + expect(Object.keys(sc).sort(), label).toEqual(["code", "exit", "ok", "status"]); + expect(ENUMERATED.has(sc.ok), `${label}: ok`).toBe(true); + expect(ENUMERATED.has(sc.status), `${label}: status`).toBe(true); + expect(ENUMERATED.has(sc.exit), `${label}: exit`).toBe(true); + expect(ENUMERATED.has(sc.code), `${label}: code`).toBe(true); + expect(JSON.stringify(r), label).not.toContain(SENTINEL); + } }); }); diff --git a/test/phi-leak.test.ts b/test/phi-leak.test.ts index 8577a90..1585f35 100644 --- a/test/phi-leak.test.ts +++ b/test/phi-leak.test.ts @@ -5,12 +5,17 @@ import { describe, expect, it } from "vitest"; import { run } from "../src/core/run.js"; import type { RunDeps } from "../src/core/io.js"; +import { dispatchTool, type McpToolResult } from "../src/mcp/tools.js"; /** * The load-bearing PHI safety layer (cli roadmap §7): the parsed model goes to **stdout** (the * explicit data channel), but **no input value ever reaches stderr**: under any command, flag, or * failure mode. Our synthetic fixtures carry sentinel identifiers; this suite proves they appear only * on the stdout data channel and never in a diagnostic. + * + * The **agent surface** is in the matrix too, because a tool result's `structuredContent` is a second + * place a value could reach a caller. It splits the same way: the tool's own payload is the data + * channel (the explicit request), and every other property of the structured result is value-free. */ const FIXTURES = join(import.meta.dirname, "__fixtures__"); @@ -137,6 +142,65 @@ describe("PHI leak matrix: validate / inspect are value-free on BOTH channels", } }); +describe("PHI leak matrix: the agent surface's structured result", () => { + const HL7_TEXT = new TextDecoder().decode(HL7); + const FHIR_TEXT = new TextDecoder().decode(FHIR); + + /** The structured result minus the tool's own payload: the part that must never carry a value. */ + function outcomeOnly(r: McpToolResult): string { + const sc = r.structuredContent; + return JSON.stringify({ ok: sc.ok, status: sc.status, exit: sc.exit, code: sc.code }); + } + + // `parse` / `convert` answer with the requested data, so their payload carries values by design; + // every other property of the structured result, and the whole result on a failure, must not. + for (const c of [ + { name: "parse hl7", tool: "parse", args: { content: HL7_TEXT } }, + { name: "parse fhir", tool: "parse", args: { content: FHIR_TEXT } }, + { name: "convert hl7", tool: "convert", args: { content: HL7_TEXT } }, + ]) { + it(`${c.name}: the outcome fields are value-free; the payload IS the data channel`, async () => { + const r = await dispatchTool(c.tool, c.args); + assertNoSentinelOnStderr(outcomeOnly(r)); + // Assert the premise as well as the remedy: the payload really did carry the requested data, + // so a green here cannot mean "there was nothing to leak". + expect(JSON.stringify(r.structuredContent.data).length).toBeGreaterThan(100); + expect(r.structuredContent.status).toBe("success"); + }); + } + + // `validate` / `inspect` report a verdict and a structural summary: value-free on every property, + // so no sentinel may appear anywhere in the result, payload included. + for (const c of [ + { name: "validate hl7", tool: "validate", args: { content: HL7_TEXT } }, + { name: "validate fhir", tool: "validate", args: { content: FHIR_TEXT } }, + { name: "inspect hl7", tool: "inspect", args: { content: HL7_TEXT } }, + { name: "inspect fhir", tool: "inspect", args: { content: FHIR_TEXT } }, + ]) { + it(`${c.name}: no sentinel anywhere in the structured result, payload included`, async () => { + const r = await dispatchTool(c.tool, c.args); + assertNoSentinelOnStderr(JSON.stringify(r)); + expect(r.structuredContent.data).toBeDefined(); + }); + } + + // Every failure mode, over PHI-laden input: nothing of the input reaches the result at all. + for (const c of [ + { name: "unsupported operation", tool: "parse", args: { content: HL7_TEXT, format: "dicom" } }, + { name: "not a convertible source", tool: "convert", args: { content: FHIR_TEXT } }, + { name: "unsupported target", tool: "convert", args: { content: HL7_TEXT, to: "x12" } }, + { name: "unknown tool", tool: HL7_TEXT, args: { content: HL7_TEXT } }, + { name: "non-string content", tool: "parse", args: { content: 1 } }, + ]) { + it(`${c.name}: the whole failed result is value-free`, async () => { + const r = await dispatchTool(c.tool, c.args); + expect(r.structuredContent.status).toBe("failed"); + expect(r.structuredContent.data).toBeUndefined(); + assertNoSentinelOnStderr(JSON.stringify(r)); + }); + } +}); + describe("PHI leak matrix: fmt keeps stderr value-free (stdout IS the data channel)", () => { // `fmt`'s stdout is a re-serialization of the message (values included, by request); only its // secondary channel (stderr) must be value-free.