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 b261aa1..28f9ad9 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. + - **`redact` / `deid` produces a real de-identified copy, delegated whole to `@cosyte/deid`.** The command that shipped as an honest `CLI_NOT_IMPLEMENTED` is wired. For `ccda`, `fhir`, `hl7` and `x12` stdout carries the de-identified document (serialized exactly as `cosyte fmt` serializes that @@ -127,6 +150,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 d8fff0e..06a8acc 100644 --- a/README.md +++ b/README.md @@ -341,6 +341,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 c97a00f..5736df2 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", () => { @@ -24,6 +69,28 @@ describe("TOOL_DEFS", () => { } }); + 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); + }); + it("does NOT advertise redact/deid: wiring it terminal-side does not put it on the agent surface", () => { // The de-identification command is deliberately terminal-only. An agent that could call it would // be handed a document the CLI asserts nothing about, over a channel with no human in it. @@ -39,31 +106,116 @@ describe("TOOL_DEFS", () => { for (const name of ["redact", "deid"]) { const r = await dispatchTool(name, { content: HL7_ADT }); expect(r.isError).toBe(true); - expect(r.content[0]?.text).toContain(`unknown tool '${name}'`); expect(r.structuredContent.ok).toBe(false); + expect(r.structuredContent.status).toBe("failed"); + expect(r.structuredContent.code).toBe(CLI_CODES.CLI_USAGE); + // Value-free now reaches the tool name too: the result says the call failed and why, without + // repeating any of what the caller sent. + expect(JSON.stringify(r)).not.toContain(name); + for (const t of TOOL_DEFS) assertPublishedContract(t.name, r, `${name} vs ${t.name}`); } }); }); +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 () => { @@ -71,20 +223,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"); }); }); @@ -92,52 +258,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 () => { @@ -149,8 +393,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 1ab3bcd..d2c6e6d 100644 --- a/test/phi-leak.test.ts +++ b/test/phi-leak.test.ts @@ -7,12 +7,17 @@ import { redactCommand } from "../src/commands/redact.js"; import { loadDeidDelegate } from "../src/core/deid.js"; 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__"); @@ -132,6 +137,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: redact is value-free on stderr in EVERY mode", () => { // `redact` renders the de-identifier's own manifest onto stderr, which makes it the one command // whose diagnostic channel is built from the library's report of what it touched. That report is