diff --git a/sidecar/claude-sdk/src/main.ts b/sidecar/claude-sdk/src/main.ts index 0582881..4c119ad 100644 --- a/sidecar/claude-sdk/src/main.ts +++ b/sidecar/claude-sdk/src/main.ts @@ -32,7 +32,7 @@ import { type SidecarFrame, type SidecarTool, } from "./frames.ts"; -import { toolRawShape } from "./schema.ts"; +import { toolInputSchema } from "./schema.ts"; import { resolve } from "node:path"; import { pathToFileURL } from "node:url"; @@ -105,7 +105,7 @@ export class QuerySession { } } -function apiRetryReason(category: unknown): SidecarFaultReason { +export function apiRetryReason(category: unknown): SidecarFaultReason { switch (category) { case "rate_limit": return "rate_limited"; @@ -130,7 +130,10 @@ function apiRetryReason(category: unknown): SidecarFaultReason { } } -function resultReason(subtype: string, apiErrorStatus: unknown): SidecarFaultReason { +export function resultReason( + subtype: string, + apiErrorStatus: unknown, +): SidecarFaultReason { if (typeof apiErrorStatus === "number") { if (apiErrorStatus === 429) { return "rate_limited"; @@ -150,6 +153,31 @@ function resultReason(subtype: string, apiErrorStatus: unknown): SidecarFaultRea } } +/** + * One `result` message's verdict: `null` when the round is an ordinary output, + * a fault otherwise. Exported so the whole subtype mapping is testable without + * a credential. + * + * `error_max_turns` is not a fault: the cap is the lane's own remaining round + * budget, so reaching it means the model spent turns the evaluator would also + * have allowed. The lane sees one ordinary round. + */ +export function resultFault(message: Record): SidecarFault | null { + const subtype = String(message.subtype ?? ""); + if (subtype === "success" || subtype === "error_max_turns") { + return null; + } + const errors = Array.isArray(message.errors) + ? (message.errors as unknown[]).map(String).join("; ") + : ""; + const category = Array.isArray(message.errors) ? undefined : message.error; + const reason = + category === undefined + ? resultReason(subtype, message.api_error_status) + : apiRetryReason(category); + return new SidecarFault(reason, `${subtype}: ${errors}`); +} + function usageRows(modelUsage: unknown): SdkModelUsage[] { if (typeof modelUsage !== "object" || modelUsage === null) { return []; @@ -185,16 +213,20 @@ export function lowerQuery( ); } const tools = frame.tools.map((entry: SidecarTool) => { - let shape; + let schema; try { - shape = toolRawShape(entry.parameters_json, entry.name); + schema = toolInputSchema(entry.parameters_json, entry.name); } catch (error) { throw new SidecarFault("tool_registration_failed", String(error)); } return tool( entry.name, entry.description, - shape, + // `tool()` names a raw shape and the server it registers with takes a + // whole schema in the same slot. The whole object is what carries the + // looseness the one-validator rule needs: a raw shape is wrapped in a + // stripping `z.object` before the arguments are ever seen. + schema as unknown as Parameters[2], async (args: unknown) => ({ content: [ { type: "text" as const, text: await session.ask(entry.name, args) }, @@ -267,6 +299,12 @@ export function assembleEvents( if (registered.has(name)) { const dispatch = dispatches[taken++]; if (!dispatch || dispatch.name !== name) { + // No argument reaches this any more: the registered schema refuses + // nothing, so a call the model emits is a call the handler runs. + // What still reaches it is the call never offered to the handler at + // all — a permission decision or an abort taken above this server, a + // tool the server disabled, or a pairing where the assistant stream + // and the dispatch log disagree about order. throw new SidecarFault( "protocol_violation", `a registered tool_use block for ${name} has no matching dispatch`, @@ -345,21 +383,9 @@ async function runQuery( material.num_turns = Number(message.num_turns ?? 0); material.usage = usageRows(message.modelUsage); material.total_cost_usd_estimate = String(message.total_cost_usd ?? 0); - // `error_max_turns` is not a fault: the cap is the lane's own remaining - // round budget, so reaching it means the model spent turns the evaluator - // would also have allowed. The lane sees one ordinary round. - if (subtype !== "success" && subtype !== "error_max_turns") { - const errors = Array.isArray(message.errors) - ? (message.errors as unknown[]).map(String).join("; ") - : ""; - const category = Array.isArray(message.errors) - ? undefined - : (message as Record).error; - const reason = - category === undefined - ? resultReason(subtype, message.api_error_status) - : apiRetryReason(category); - throw new SidecarFault(reason, `${subtype}: ${errors}`); + const fault = resultFault(message); + if (fault) { + throw fault; } } } catch (error) { diff --git a/sidecar/claude-sdk/src/schema.ts b/sidecar/claude-sdk/src/schema.ts index cf885c7..a3d0b67 100644 --- a/sidecar/claude-sdk/src/schema.ts +++ b/sidecar/claude-sdk/src/schema.ts @@ -1,7 +1,17 @@ // The JSON Schema grammar Ghostlight's one emitter produces, converted to the -// Zod raw shape the SDK's `tool()` takes. It is closed on purpose: anything -// outside the grammar throws, so an emitter that grows a construct fails a test -// here rather than silently registering a looser tool. +// Zod object the SDK's `tool()` registers. +// +// The conversion exists for one reason: the model must see the same parameter +// schema on both transports, and the in-process MCP server derives the schema +// it advertises from this Zod object. It is not a validator. Rust's decoder is +// the one validator on both transports, so every object here is loose and every +// property catches: an extra key, a missing required property, and a mistyped +// value all reach the handler, travel to Rust unstripped, and become the gap +// the evaluator already records instead of a tool call the SDK answers itself. +// +// A construct outside the emitted grammar still throws. That is a registration +// failure, not an argument judgement: a schema this module cannot convert is a +// tool the model would otherwise be shown blind. import { z } from "zod"; @@ -13,6 +23,17 @@ function isObject(value: unknown): value is Json { return typeof value === "object" && value !== null && !Array.isArray(value); } +/** + * Wraps one property so it can never refuse the call. The catch value is + * static because Zod refuses to render a dynamic one as JSON Schema, and the + * advertised schema is the whole point of converting. A value the property's + * type refuses therefore arrives at Rust as an absent field, which its decoder + * reports the same way it reports any other missing field. + */ +function keep(schema: z.ZodTypeAny): z.ZodTypeAny { + return schema.catch(undefined as never); +} + function convert(node: unknown, at: string): z.ZodTypeAny { if (!isObject(node)) { throw new Error(`${at}: schema node is not an object`); @@ -49,7 +70,7 @@ function convert(node: unknown, at: string): z.ZodTypeAny { } switch (kind) { case "object": - return z.object(rawShape(node, at)).strict(); + return objectSchema(node, at); case "string": { if ("const" in node) { if (typeof node.const !== "string") { @@ -91,36 +112,24 @@ function convert(node: unknown, at: string): z.ZodTypeAny { } } -function rawShape(node: Json, at: string): Record { - if (node.additionalProperties !== false) { - throw new Error(`${at}: every emitted object is closed`); - } +/** One object node, loose so an unknown key survives to Rust with its value. */ +function objectSchema(node: Json, at: string): z.ZodObject { const properties = node.properties; if (!isObject(properties)) { throw new Error(`${at}: object without properties`); } - const names = Object.keys(properties).sort(); - const required = Array.isArray(node.required) - ? [...(node.required as unknown[])].map(String).sort() - : []; - if (names.length !== required.length || names.some((name, index) => name !== required[index])) { - throw new Error(`${at}: every emitted property is required`); - } const shape: Record = {}; - for (const name of names) { - shape[name] = convert(properties[name], `${at}/${name}`); + for (const name of Object.keys(properties).sort()) { + shape[name] = keep(convert(properties[name], `${at}/${name}`)); } - return shape; + return z.looseObject(shape); } /** - * One tool's `parameters_json` as the raw shape `tool()` takes. Throws on any + * One tool's `parameters_json` as the schema `tool()` registers. Throws on any * node outside the grammar the emitter produces. */ -export function toolRawShape( - parametersJson: string, - at: string, -): Record { +export function toolInputSchema(parametersJson: string, at: string): z.ZodObject { let parsed: unknown; try { parsed = JSON.parse(parametersJson); @@ -130,5 +139,5 @@ export function toolRawShape( if (!isObject(parsed) || parsed.type !== "object") { throw new Error(`${at}: a tool's parameters must be an object schema`); } - return rawShape(parsed, at); + return objectSchema(parsed, at); } diff --git a/sidecar/claude-sdk/test/result.test.ts b/sidecar/claude-sdk/test/result.test.ts new file mode 100644 index 0000000..9ab4938 --- /dev/null +++ b/sidecar/claude-sdk/test/result.test.ts @@ -0,0 +1,83 @@ +// Which `result` message becomes a round and which becomes a named fault. The +// mapping is the sidecar's whole judgement of a finished query, so it is +// asserted directly against the exported decision, with no credential, no +// network, and no `query()`. + +import assert from "node:assert/strict"; +import test from "node:test"; +import { apiRetryReason, resultFault, resultReason, SidecarFault } from "../src/main.ts"; + +function reasonOf(message: Record): string | null { + const fault = resultFault(message); + if (fault === null) { + return null; + } + assert.ok(fault instanceof SidecarFault); + return fault.reason; +} + +test("a spent turn cap is an ordinary output, not a fault", () => { + // The cap is the lane's own remaining round budget, so reaching it means the + // model spent turns the evaluator would also have allowed. + assert.equal(resultFault({ subtype: "error_max_turns", num_turns: 24 }), null); + assert.equal(resultFault({ subtype: "success" }), null); +}); + +test("every other result subtype carries its named fault", () => { + assert.equal(reasonOf({ subtype: "error_max_budget_usd" }), "max_budget_usd"); + assert.equal(reasonOf({ subtype: "error_during_execution" }), "execution_error"); + assert.equal( + reasonOf({ subtype: "error_max_structured_output_retries" }), + "execution_error", + ); + assert.equal(reasonOf({ subtype: "error_nobody_has_named_yet" }), "unknown"); +}); + +test("an API status outranks the subtype it arrived on", () => { + assert.equal( + reasonOf({ subtype: "error_during_execution", api_error_status: 429 }), + "rate_limited", + ); + assert.equal( + reasonOf({ subtype: "error_during_execution", api_error_status: 503 }), + "server_error", + ); + assert.equal( + reasonOf({ subtype: "error_during_execution", api_error_status: 400 }), + "execution_error", + ); + assert.equal(resultReason("error_during_execution", "429"), "execution_error"); +}); + +test("a retry category names the fault when the result carries one", () => { + const categories: [string, string][] = [ + ["rate_limit", "rate_limited"], + ["overloaded", "overloaded"], + ["server_error", "server_error"], + ["authentication_failed", "authentication_failed"], + ["oauth_org_not_allowed", "org_not_allowed"], + ["billing_error", "billing_error"], + ["invalid_request", "invalid_request"], + ["model_not_found", "model_not_found"], + ["max_output_tokens", "max_output_tokens"], + ["something_new", "unknown"], + ]; + for (const [category, reason] of categories) { + assert.equal(apiRetryReason(category), reason, category); + assert.equal( + reasonOf({ subtype: "error_during_execution", error: category }), + reason, + category, + ); + } +}); + +test("a fault's detail carries the subtype and the reported errors", () => { + const fault = resultFault({ + subtype: "error_during_execution", + errors: ["the pipe closed", "and stayed closed"], + }); + assert.ok(fault instanceof SidecarFault); + assert.equal(fault.reason, "execution_error"); + assert.equal(fault.message, "error_during_execution: the pipe closed; and stayed closed"); +}); diff --git a/sidecar/claude-sdk/test/schema.test.ts b/sidecar/claude-sdk/test/schema.test.ts index 8d151f4..261062a 100644 --- a/sidecar/claude-sdk/test/schema.test.ts +++ b/sidecar/claude-sdk/test/schema.test.ts @@ -7,7 +7,7 @@ import assert from "node:assert/strict"; import { readFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; import test from "node:test"; -import { toolRawShape } from "../src/schema.ts"; +import { toolInputSchema } from "../src/schema.ts"; const emitted: { name: string; parameters_json: string }[] = JSON.parse( readFileSync(fileURLToPath(new URL("./schemas.json", import.meta.url)), "utf8"), @@ -16,16 +16,16 @@ const emitted: { name: string; parameters_json: string }[] = JSON.parse( test("every emitted tool schema converts", () => { assert.ok(emitted.length > 0, "the schema fixture is empty"); for (const entry of emitted) { - const shape = toolRawShape(entry.parameters_json, entry.name); + const schema = toolInputSchema(entry.parameters_json, entry.name); const declared = Object.keys( JSON.parse(entry.parameters_json).properties as Record, ).sort(); - assert.deepEqual(Object.keys(shape).sort(), declared, entry.name); + assert.deepEqual(Object.keys(schema.shape).sort(), declared, entry.name); } }); -test("a converted shape accepts a value its schema admits", () => { - const shape = toolRawShape( +test("a converted schema carries every property's own type", () => { + const schema = toolInputSchema( JSON.stringify({ type: "object", additionalProperties: false, @@ -41,12 +41,32 @@ test("a converted shape accepts a value its schema admits", () => { }), "sample", ); - assert.equal(shape.detail!.safeParse("ok").success, true); - assert.equal(shape.count!.safeParse(2).success, true); - assert.equal(shape.count!.safeParse(9).success, false); - assert.equal(shape.choice!.safeParse("c").success, false); - assert.equal(shape.maybe!.safeParse(null).success, true); - assert.equal(shape.many!.safeParse(["a"]).success, true); + const admitted = schema.parse({ + detail: "ok", + count: 2, + flag: true, + choice: "a", + maybe: null, + many: ["a"], + }) as Record; + assert.deepEqual(admitted, { + detail: "ok", + count: 2, + flag: true, + choice: "a", + maybe: null, + many: ["a"], + }); + + // A value outside a property's own type empties that property and nothing + // else. Rust decides what an absent field means. + const dropped = schema.parse({ detail: "ok", count: 9, choice: "c" }) as Record< + string, + unknown + >; + assert.equal(dropped.detail, "ok"); + assert.equal(dropped.count, undefined); + assert.equal(dropped.choice, undefined); }); test("anything outside the grammar throws", () => { @@ -60,7 +80,7 @@ test("anything outside the grammar throws", () => { for (const node of refused) { assert.throws( () => - toolRawShape( + toolInputSchema( JSON.stringify({ type: "object", additionalProperties: false, @@ -74,22 +94,23 @@ test("anything outside the grammar throws", () => { } }); -test("an open object or a partly optional one throws", () => { - assert.throws(() => - toolRawShape( - JSON.stringify({ type: "object", required: [], properties: {} }), - "open", - ), +test("an open object and a partly optional one convert and refuse nothing", () => { + // Openness and optionality are Rust's judgements, so neither is a + // registration failure here. + const open = toolInputSchema( + JSON.stringify({ type: "object", required: [], properties: {} }), + "open", ); - assert.throws(() => - toolRawShape( - JSON.stringify({ - type: "object", - additionalProperties: false, - required: ["a"], - properties: { a: { type: "string" }, b: { type: "string" } }, - }), - "partial", - ), + assert.deepEqual(open.parse({ anything: 1 }), { anything: 1 }); + + const partial = toolInputSchema( + JSON.stringify({ + type: "object", + additionalProperties: false, + required: ["a"], + properties: { a: { type: "string" }, b: { type: "string" } }, + }), + "partial", ); + assert.deepEqual(partial.parse({ b: "set" }), { b: "set" }); }); diff --git a/sidecar/claude-sdk/test/soul-zod-liability.test.ts b/sidecar/claude-sdk/test/soul-zod-liability.test.ts index 499b24f..b890590 100644 --- a/sidecar/claude-sdk/test/soul-zod-liability.test.ts +++ b/sidecar/claude-sdk/test/soul-zod-liability.test.ts @@ -1,18 +1,18 @@ -// Soul: characterizes what the SDK's own Zod validation does to a tool call -// whose arguments Ghostlight's evaluators would have recorded as a gap, and -// asserts the sidecar names no credential and reads no environment. +// One validator, both transports. These assert that the schema the sidecar +// registers judges nothing: an argument Ghostlight's evaluators would record as +// a gap travels through the real in-process MCP server unstripped and reaches +// the handler, so Rust's decoder is the only thing that can refuse it. // -// Nothing here proposes a fix. The chain is established end to end against the -// real SDK, with no credential and no network, so the divergence from the -// connector path is a recorded fact rather than a guess. +// Established end to end against the real SDK, with no credential and no +// network. Also asserts the sidecar names no credential and reads no +// environment. import assert from "node:assert/strict"; import { readFileSync, readdirSync } from "node:fs"; import { fileURLToPath } from "node:url"; import test from "node:test"; -import { z } from "zod"; import { createSdkMcpServer, tool } from "@anthropic-ai/claude-agent-sdk"; -import { toolRawShape } from "../src/schema.ts"; +import { toolInputSchema } from "../src/schema.ts"; import { assembleEvents, SidecarFault } from "../src/main.ts"; const emitted: { name: string; parameters_json: string }[] = JSON.parse( @@ -25,40 +25,54 @@ function schemaFor(name: string): string { return entry.parameters_json; } -test("the emitted grammar's top level admits an extra property the connector refuses", () => { - // `rawShape` builds a bare record; only nested objects are `.strict()`. The - // SDK's `tool()` wraps the top-level record in a plain `z.object`, which - // strips unknown keys instead of rejecting them — while every emitted schema - // says `additionalProperties: false` and every Rust decoder carries - // `deny_unknown_fields`. - const gap = z.object(toolRawShape(schemaFor("record_gap"), "record_gap")); +test("the registered schema keeps an extra property instead of stripping it", () => { + const gap = toolInputSchema(schemaFor("record_gap"), "record_gap"); const admitted = gap.safeParse({ detail: "no route", extra: true }); - assert.equal(admitted.success, true, "the top-level object refused an extra property"); + assert.equal(admitted.success, true); assert.deepEqual( admitted.success && admitted.data, - { detail: "no route" }, - "the extra property was not stripped", + { detail: "no route", extra: true }, + "an extra property was stripped before Rust could refuse it", ); - // Nested objects are closed, so the leak is exactly one level deep. - const nested = z.object(toolRawShape(schemaFor("speak"), "speak")); - assert.equal(nested.safeParse({ text: "hold", extra: 1 }).success, true); - const declare = z.object(toolRawShape(schemaFor("bind"), "bind")); - const deep = declare.safeParse({ subject: { ref: "draft", value: "x", extra: true } }); - assert.equal(deep.success, false, "a nested object admitted an extra property"); + // A nested object keeps its own, so the emitter's depth does not matter. + const declare = toolInputSchema(schemaFor("bind"), "bind"); + const deep = declare.safeParse({ + subject: { ref: "draft", value: "x", extra: true }, + }); + assert.equal(deep.success, true); + assert.deepEqual(deep.success && (deep.data as { subject: unknown }).subject, { + ref: "draft", + value: "x", + extra: true, + }); }); -test("the emitted grammar refuses a missing or mistyped required property", () => { - const gap = z.object(toolRawShape(schemaFor("record_gap"), "record_gap")); - assert.equal(gap.safeParse({}).success, false); - assert.equal(gap.safeParse({ detail: 5 }).success, false); - // A deep emitted shape gives a compliant model many more required leaves to - // miss. - const subject = z.object(toolRawShape(schemaFor("declare_subject"), "declare_subject")); - assert.equal(subject.safeParse({ handle: "smith" }).success, false); +test("the registered schema admits a missing or mistyped required property", () => { + const gap = toolInputSchema(schemaFor("record_gap"), "record_gap"); + const absent = gap.safeParse({}); + assert.equal(absent.success, true); + assert.deepEqual(absent.success && absent.data, {}); + + // A mistyped value is the one thing a property's catch cannot carry through: + // it reaches Rust as an absent field, which the decoder reports the way it + // reports any other missing field, rather than as a call the SDK refused. + const mistyped = gap.safeParse({ detail: 5 }); + assert.equal(mistyped.success, true); + assert.equal( + mistyped.success && (mistyped.data as { detail?: unknown }).detail, + undefined, + ); + + const subject = toolInputSchema(schemaFor("declare_subject"), "declare_subject"); + assert.equal( + subject.safeParse({ handle: "smith" }).success, + true, + "a partly filled call was refused here instead of in Rust", + ); }); -test("the SDK answers a refused argument itself and never reaches the handler", async () => { +test("the SDK dispatches a call Rust would refuse and never answers it itself", async () => { // The middle link, against the real in-process MCP server the sidecar // registers. No credential, no network, no `query()`. let mcp: { Client: new (info: { name: string; version: string }) => never }; @@ -68,15 +82,17 @@ test("the SDK answers a refused argument itself and never reaches the handler", memory = await import("@modelcontextprotocol/sdk/inMemory.js"); } catch { // The MCP client is the SDK's own transitive dependency, not this - // package's. If a future install stops hoisting it, the two assertions - // either side of this test still stand on their own. + // package's. If a future install stops hoisting it, the assertions either + // side of this test still stand on their own. return; } const seen: unknown[] = []; const handle = tool( "record_gap", "Record a gap.", - toolRawShape(schemaFor("record_gap"), "record_gap"), + toolInputSchema(schemaFor("record_gap"), "record_gap") as unknown as Parameters< + typeof tool + >[2], async (args: unknown) => { seen.push(args); return { content: [{ type: "text" as const, text: "gap recorded" }] }; @@ -90,6 +106,7 @@ test("the SDK answers a refused argument itself and never reaches the handler", await instance.connect(serverSide); const client = new mcp.Client({ name: "ghostlight-soul-probe", version: "0" }) as unknown as { connect(t: unknown): Promise; + listTools(): Promise<{ tools: { inputSchema: Record }[] }>; callTool(request: { name: string; arguments: unknown }): Promise<{ isError?: boolean; content: { text: string }[]; @@ -98,32 +115,35 @@ test("the SDK answers a refused argument itself and never reaches the handler", }; await client.connect(clientSide); try { - const stripped = await client.callTool({ + // The model still sees the emitted parameters. Looseness is what the + // server judges arguments by, not what it advertises. + const advertised = (await client.listTools()).tools[0]!.inputSchema; + assert.equal(advertised.type, "object"); + assert.deepEqual(Object.keys(advertised.properties as object), ["detail"]); + assert.deepEqual(advertised.required, ["detail"]); + + // An extra key reaches the handler with the key present. + const extra = await client.callTool({ name: "record_gap", arguments: { detail: "no route", extra: true }, }); - assert.notEqual(stripped.isError, true, "an extra property was refused after all"); - assert.deepEqual( - seen, - [{ detail: "no route" }], - "the handler saw arguments other than the stripped ones", - ); + assert.notEqual(extra.isError, true, "an extra property was refused"); + assert.deepEqual(seen.at(-1), { detail: "no route", extra: true }); - for (const refused of [{ detail: 5 }, {}]) { - const answer = await client.callTool({ name: "record_gap", arguments: refused }); - assert.equal(answer.isError, true, `${JSON.stringify(refused)} reached the handler`); - assert.match(answer.content[0]!.text, /Input validation error/); - } - assert.equal(seen.length, 1, "a refused argument still reached the handler"); + // A missing required property reaches the handler too. + const missing = await client.callTool({ name: "record_gap", arguments: {} }); + assert.notEqual(missing.isError, true, "a missing property was refused"); + assert.deepEqual(seen.at(-1), {}); + + assert.equal(seen.length, 2, "a call was answered without reaching the handler"); } finally { await client.close(); } }); test("a registered tool_use block with no dispatch is a protocol violation", () => { - // What the sidecar then sees: the assistant message still carries a - // `tool_use` block for a registered name, but `session.ask` was never - // reached, so there is no dispatch to pair with it. + // No argument reaches this any more. What still does is a call never offered + // to the handler, or an assistant stream and a dispatch log that disagree. const registered = new Set(["record_gap"]); assert.throws( () =>