From bfbb2d1718e9706822ded547f92563336bc8096b Mon Sep 17 00:00:00 2001 From: andriypolanski Date: Fri, 31 Jul 2026 07:59:04 +0000 Subject: [PATCH] fix(mcp): compare property schemas in checkInputNarrowing Enforce recursive JSON Schema narrowing for shared advertised properties so stdio overrides cannot widen inside nested items unnoticed, and keep real server schemas green under the strengthened check. --- .../src/tools/local-branch.ts | 15 +- .../loopover-contract/src/tools/miner-ops.ts | 23 +-- scripts/lib/validate-mcp/invariants.ts | 131 +++++++++++++++- test/unit/miner-mcp-ops-tools.test.ts | 11 ++ test/unit/validate-mcp-helpers.test.ts | 143 +++++++++++++++++- 5 files changed, 294 insertions(+), 29 deletions(-) diff --git a/packages/loopover-contract/src/tools/local-branch.ts b/packages/loopover-contract/src/tools/local-branch.ts index f2e6ac4a0c..f13769c612 100644 --- a/packages/loopover-contract/src/tools/local-branch.ts +++ b/packages/loopover-contract/src/tools/local-branch.ts @@ -68,7 +68,10 @@ const localValidationEntry = z.object({ * `cwd` is what makes this family `local-git`: it names a checkout only the caller's machine has. */ export const CurrentBranchInput = z.object({ - login: z.string().min(1).optional(), + // Bounds match LocalBranchAnalysisInput on shared fields so a stdio override that serves this + // shape (e.g. StdioCompareLocalVariantsInput) is a true narrowing under checkInputNarrowing (#10041), + // not a silent loosening of maxLength / maxItems the wider contract already publishes. + login: z.string().min(1).max(SCENARIO_LIMITS.branchRefChars).optional(), cwd: z.string().optional(), repoFullName: z.string().min(3).max(SCENARIO_LIMITS.repoFullNameChars).optional(), baseRef: z.string().max(SCENARIO_LIMITS.branchRefChars).optional(), @@ -77,15 +80,15 @@ export const CurrentBranchInput = z.object({ title: z.string().optional(), body: z.string().optional(), labels: z.array(z.string()).optional(), - linkedIssues: z.array(z.number().int().positive()).optional(), + linkedIssues: z.array(z.number().int().positive()).max(SCENARIO_MAX_LINKED_ISSUE_NUMBERS).optional(), pendingMergedPrCount: z.number().int().min(0).optional(), pendingClosedPrCount: z.number().int().min(0).optional(), approvedPrCount: z.number().int().min(0).optional(), expectedOpenPrCountAfterMerge: z.number().int().min(0).optional(), projectedCredibility: z.number().min(0).max(1).optional(), - scenarioNotes: z.array(z.string()).optional(), + scenarioNotes: z.array(z.string()).max(20).optional(), branchEligibility: callerBranchEligibilitySchema.optional(), - validation: z.array(localValidationEntry).optional(), + validation: z.array(localValidationEntry).max(50).optional(), scorePreviewCommand: z.string().optional(), }); @@ -307,7 +310,9 @@ export const LocalScoreInput = z.object({ approvedPrCount: z.number().int().min(0).optional(), expectedOpenPrCountAfterMerge: z.number().int().min(0).optional(), projectedCredibility: z.number().min(0).max(1).optional(), - scenarioNotes: z.array(z.string()).optional(), + // Cap matches ExplainScoreBreakdownInput / api-requests so RemoteLocalScorePreviewInput is a + // true narrowing of that contract field under checkInputNarrowing (#10041). + scenarioNotes: z.array(z.string()).max(20).optional(), branchEligibility: callerBranchEligibilitySchema.optional(), scorePreviewCommand: z.string().optional(), }); diff --git a/packages/loopover-contract/src/tools/miner-ops.ts b/packages/loopover-contract/src/tools/miner-ops.ts index c824facf0d..ca6470cb54 100644 --- a/packages/loopover-contract/src/tools/miner-ops.ts +++ b/packages/loopover-contract/src/tools/miner-ops.ts @@ -56,15 +56,20 @@ export const minerDoctorTool = defineTool({ export const MinerMetricsSnapshotInput = z.object({}); export const MinerMetricsSnapshotOutput = z.looseObject({ - generatedAt: z.string(), - families: z.array( - z.looseObject({ - name: z.string(), - type: z.string(), - help: z.string().optional(), - samples: z.array(z.looseObject({ value: z.number(), labels: z.record(z.string(), z.string()).optional() })), - }), - ), + // Optional: a store-failure envelope from `withMinerToolErrorHandling` carries only `error`, and + // the MCP SDK validates structuredContent against this schema even when `isError` is set — required + // success fields made that path a -32602 instead of a clean tool error. + generatedAt: z.string().optional(), + families: z + .array( + z.looseObject({ + name: z.string(), + type: z.string(), + help: z.string().optional(), + samples: z.array(z.looseObject({ value: z.number(), labels: z.record(z.string(), z.string()).optional() })), + }), + ) + .optional(), // #9659: every miner tool answers a store failure with the shared error envelope // (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only // the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller diff --git a/scripts/lib/validate-mcp/invariants.ts b/scripts/lib/validate-mcp/invariants.ts index 278bdb4858..9213415826 100644 --- a/scripts/lib/validate-mcp/invariants.ts +++ b/scripts/lib/validate-mcp/invariants.ts @@ -78,8 +78,115 @@ export function checkAdvertisedMetadata(expected: readonly McpToolDefinition[], return failures; } +const BOUND_KEYS = ["minimum", "maximum", "minLength", "maxLength", "minItems", "maxItems"] as const; +const LOWER_BOUNDS = new Set(["minimum", "minLength", "minItems"]); + +function isPlainObject(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function jsonDeepEqual(left: unknown, right: unknown): boolean { + if (left === right) return true; + if (Array.isArray(left) && Array.isArray(right)) { + return left.length === right.length && left.every((entry, index) => jsonDeepEqual(entry, right[index])); + } + if (!isPlainObject(left) || !isPlainObject(right)) return false; + const leftKeys = Object.keys(left); + const rightKeys = Object.keys(right); + if (leftKeys.length !== rightKeys.length) return false; + return leftKeys.every((key) => Object.prototype.hasOwnProperty.call(right, key) && jsonDeepEqual(left[key], right[key])); +} + +/** + * The MCP SDK's zod→JSON-Schema path omits `additionalProperties: false` that `z.toJSONSchema` + * (draft-2020-12) emits for the same object. Both sides of this check are already JSON Schema, but + * they are not produced by the same converter, so a closed object on the contract and an omitted + * keyword on the wire are the same schema, not a widening. A present `true` (or a non-false schema) + * is still a real difference and must not be stripped. + */ +function withoutClosedAdditionalProperties(value: unknown): unknown { + if (Array.isArray(value)) return value.map(withoutClosedAdditionalProperties); + if (!isPlainObject(value)) return value; + const out: Record = {}; + for (const [key, entry] of Object.entries(value)) { + if (key === "additionalProperties" && entry === false) continue; + out[key] = withoutClosedAdditionalProperties(entry); + } + return out; +} + +/** + * Whether an advertised JSON Schema subtree is identical to the contract's, or a recognised + * narrowing of it (#10041). + * + * Recognised differences only: (a) a removed key under `properties`, (b) an added or tightened + * minimum/maximum/minLength/maxLength/minItems/maxItems, (c) an `enum` that is a subset of the + * contract's, (d) the same rules applied recursively through `items` / `properties`. Nested + * `required` follows the top-level rule: every advertised requirement must already be required by + * the contract (dropping a requirement is allowed; inventing one is not). + */ +function isJsonSchemaNarrowing(advertised: unknown, contract: unknown): boolean { + advertised = withoutClosedAdditionalProperties(advertised); + contract = withoutClosedAdditionalProperties(contract); + if (jsonDeepEqual(advertised, contract)) return true; + if (!isPlainObject(advertised) || !isPlainObject(contract)) return false; + + if (advertised.type !== undefined && advertised.type !== contract.type) return false; + if (contract.type !== undefined && advertised.type === undefined) return false; + + if (advertised.enum !== undefined || contract.enum !== undefined) { + if (!Array.isArray(contract.enum) || !Array.isArray(advertised.enum)) return false; + if (!advertised.enum.every((value) => (contract.enum as unknown[]).some((entry) => jsonDeepEqual(entry, value)))) { + return false; + } + } + + if (advertised.properties !== undefined || contract.properties !== undefined) { + if (advertised.properties !== undefined && !isPlainObject(advertised.properties)) return false; + if (contract.properties !== undefined && !isPlainObject(contract.properties)) return false; + const advertisedProperties = isPlainObject(advertised.properties) ? advertised.properties : {}; + const contractProperties = isPlainObject(contract.properties) ? contract.properties : {}; + for (const key of Object.keys(advertisedProperties)) { + if (!Object.prototype.hasOwnProperty.call(contractProperties, key)) return false; + if (!isJsonSchemaNarrowing(advertisedProperties[key], contractProperties[key])) return false; + } + } + + if (advertised.items !== undefined || contract.items !== undefined) { + if (advertised.items === undefined || contract.items === undefined) return false; + if (!isJsonSchemaNarrowing(advertised.items, contract.items)) return false; + } + + if (Array.isArray(advertised.required)) { + const contractRequired = new Set(Array.isArray(contract.required) ? contract.required : []); + for (const property of advertised.required) { + if (typeof property !== "string" || !contractRequired.has(property)) return false; + } + } else if (advertised.required !== undefined) { + return false; + } + + for (const key of BOUND_KEYS) { + const advertisedBound = advertised[key]; + const contractBound = contract[key]; + if (advertisedBound === undefined && contractBound === undefined) continue; + if (typeof advertisedBound !== "number" && advertisedBound !== undefined) return false; + if (typeof contractBound !== "number" && contractBound !== undefined) return false; + if (advertisedBound === undefined) return false; // contract had a bound the advertisement dropped + if (contractBound === undefined) continue; // added bound = tightening + if (LOWER_BOUNDS.has(key) ? advertisedBound < contractBound : advertisedBound > contractBound) return false; + } + + const handled = new Set(["type", "enum", "properties", "items", "required", ...BOUND_KEYS]); + for (const key of new Set([...Object.keys(advertised), ...Object.keys(contract)])) { + if (handled.has(key)) continue; + if (!jsonDeepEqual(advertised[key], contract[key])) return false; + } + return true; +} + /** - * An advertised input may only NARROW the contract's, never widen it (#9662). + * An advertised input may only NARROW the contract's, never widen it (#9662, #10041). * * `registerStdioTool`'s override is documented as one-way -- "a server may serve LESS than the contract * when its own route cannot honour a field... never used to widen" -- and nothing enforced it. The @@ -88,10 +195,10 @@ export function checkAdvertisedMetadata(expected: readonly McpToolDefinition[], * and the smoke arguments are synthesized FROM the advertised schema, so a widened schema simply gets * widened arguments and passes. * - * Narrowing is defined mechanically, which is all a schema comparison can honestly do here: every - * advertised property exists in the contract's, and every advertised requirement is one the contract - * also requires. Making an optional contract field required is therefore a widening of the caller's - * obligations and fails -- which is the case a hand-written override is most likely to get wrong. + * Narrowing is the property-name / required checks below, plus a recursive comparison of every shared + * property's JSON Schema subtree (see `isJsonSchemaNarrowing`). Making an optional contract field + * required is therefore a widening of the caller's obligations and fails -- which is the case a + * hand-written override is most likely to get wrong. */ export function checkInputNarrowing(expected: readonly McpToolDefinition[], listed: readonly ListedTool[]): string[] { const listedByName = new Map(listed.map((tool) => [tool.name, tool])); @@ -100,10 +207,18 @@ export function checkInputNarrowing(expected: readonly McpToolDefinition[], list const advertised = listedByName.get(tool.name); // Absent is diffToolSets' finding; a schema-less advertisement is checkAdvertisedShape's. if (!advertised?.inputSchema) continue; - const contractProperties = new Set(Object.keys((tool.inputSchema as { properties?: Record }).properties ?? {})); + const contractProperties = (tool.inputSchema as { properties?: Record }).properties ?? {}; + const contractPropertyNames = new Set(Object.keys(contractProperties)); const contractRequired = new Set((tool.inputSchema as { required?: string[] }).required ?? []); - for (const property of Object.keys(advertised.inputSchema.properties ?? {})) { - if (!contractProperties.has(property)) failures.push(`${tool.name} advertises input property ${property}, which its contract does not declare`); + const advertisedProperties = advertised.inputSchema.properties ?? {}; + for (const property of Object.keys(advertisedProperties)) { + if (!contractPropertyNames.has(property)) { + failures.push(`${tool.name} advertises input property ${property}, which its contract does not declare`); + continue; + } + if (!isJsonSchemaNarrowing(advertisedProperties[property], contractProperties[property])) { + failures.push(`${tool.name} advertises input property ${property}, which is not a narrowing of its contract`); + } } for (const property of advertised.inputSchema.required ?? []) { if (!contractRequired.has(property)) failures.push(`${tool.name} requires input property ${property}, which its contract does not require`); diff --git a/test/unit/miner-mcp-ops-tools.test.ts b/test/unit/miner-mcp-ops-tools.test.ts index 956176d747..b4df82cf1a 100644 --- a/test/unit/miner-mcp-ops-tools.test.ts +++ b/test/unit/miner-mcp-ops-tools.test.ts @@ -136,6 +136,17 @@ describe("loopover_miner_get_metrics_snapshot (#9523)", () => { expect(result).toBeTruthy(); }); + it("surfaces a ledger that will not open as isError rather than an SDK schema rejection", async () => { + const client = await connect({ + initPredictionLedger: () => { + throw new Error("prediction ledger unreadable"); + }, + }); + const result = (await client.callTool({ name: "loopover_miner_get_metrics_snapshot", arguments: {} })) as ToolResult; + expect(result.isError).toBe(true); + expect(structured(result).error).toMatchObject({ message: "prediction ledger unreadable" }); + }); + it("emits every counter even for an empty ledger, so the surface is well-formed before any prediction", async () => { const client = await connect({ initPredictionLedger: () => ({ readPredictions: () => [], close: () => undefined }) as never, diff --git a/test/unit/validate-mcp-helpers.test.ts b/test/unit/validate-mcp-helpers.test.ts index 9ec831947e..d5437872e6 100644 --- a/test/unit/validate-mcp-helpers.test.ts +++ b/test/unit/validate-mcp-helpers.test.ts @@ -91,13 +91,22 @@ describe("validate-mcp invariants", () => { }); }); - describe("an advertised input may only narrow the contract's (#9662)", () => { - const projected = (properties: string[], required: string[] = []): McpToolDefinition => - ({ name: "a", inputSchema: { type: "object", properties: Object.fromEntries(properties.map((k) => [k, {}])), required } }) as unknown as McpToolDefinition; - const advertised = (properties: string[], required: string[] = []) => ({ - name: "a", - inputSchema: { type: "object", properties: Object.fromEntries(properties.map((k) => [k, {}])), required }, - }); + describe("an advertised input may only narrow the contract's (#9662, #10041)", () => { + const projected = ( + properties: Record | string[], + required: string[] = [], + ): McpToolDefinition => { + const props = Array.isArray(properties) + ? Object.fromEntries(properties.map((k) => [k, {}])) + : properties; + return { name: "a", inputSchema: { type: "object", properties: props, required } } as unknown as McpToolDefinition; + }; + const advertised = (properties: Record | string[], required: string[] = []) => { + const props = Array.isArray(properties) + ? Object.fromEntries(properties.map((k) => [k, {}])) + : properties; + return { name: "a", inputSchema: { type: "object", properties: props, required } }; + }; it("passes when the advertised schema is the contract's", () => { expect(checkInputNarrowing([projected(["login", "repo"], ["repo"])], [advertised(["login", "repo"], ["repo"])])).toEqual([]); @@ -126,6 +135,126 @@ describe("validate-mcp invariants", () => { expect(checkInputNarrowing([projected(["login"])], [])).toEqual([]); expect(checkInputNarrowing([projected(["login"])], [{ name: "a" }])).toEqual([]); }); + + it("passes when a shared property's subtree is identical", () => { + expect( + checkInputNarrowing( + [projected({ login: { type: "string", minLength: 1 } })], + [advertised({ login: { type: "string", minLength: 1 } })], + ), + ).toEqual([]); + }); + + it("passes when the advertisement removes a nested property", () => { + expect( + checkInputNarrowing( + [projected({ meta: { type: "object", properties: { a: { type: "string" }, b: { type: "number" } } } })], + [advertised({ meta: { type: "object", properties: { a: { type: "string" } } } })], + ), + ).toEqual([]); + }); + + it("passes when the advertisement tightens a bound", () => { + expect( + checkInputNarrowing( + [projected({ title: { type: "string", maxLength: 100 } })], + [advertised({ title: { type: "string", maxLength: 50 } })], + ), + ).toEqual([]); + }); + + it("passes when the advertisement's enum is a subset of the contract's", () => { + expect( + checkInputNarrowing( + [projected({ status: { type: "string", enum: ["a", "b", "c"] } })], + [advertised({ status: { type: "string", enum: ["a", "c"] } })], + ), + ).toEqual([]); + }); + + it("reports a shared property whose type diverges", () => { + expect( + checkInputNarrowing( + [projected({ login: { type: "string" } })], + [advertised({ login: { type: "number" } })], + ), + ).toEqual(["a advertises input property login, which is not a narrowing of its contract"]); + }); + + it("reports a nested property the contract never declared", () => { + expect( + checkInputNarrowing( + [projected({ meta: { type: "object", properties: { a: { type: "string" } } } })], + [advertised({ meta: { type: "object", properties: { a: { type: "string" }, invented: { type: "boolean" } } } })], + ), + ).toEqual(["a advertises input property meta, which is not a narrowing of its contract"]); + }); + + it("reports a loosened bound on a shared property", () => { + expect( + checkInputNarrowing( + [projected({ title: { type: "string", maxLength: 50 } })], + [advertised({ title: { type: "string", maxLength: 100 } })], + ), + ).toEqual(["a advertises input property title, which is not a narrowing of its contract"]); + }); + + it("reports an enum member the contract does not list", () => { + expect( + checkInputNarrowing( + [projected({ status: { type: "string", enum: ["a", "b"] } })], + [advertised({ status: { type: "string", enum: ["a", "invented"] } })], + ), + ).toEqual(["a advertises input property status, which is not a narrowing of its contract"]); + }); + + it("reports a difference nested two levels deep under items.properties (#10041)", () => { + // The hole the name-only check left open: both sides advertise `variants`, both require it, + // and the divergence lives only inside the element type. + expect( + checkInputNarrowing( + [ + projected({ + variants: { + type: "array", + items: { type: "object", properties: { repo: { type: "string" } } }, + }, + }), + ], + [ + advertised({ + variants: { + type: "array", + items: { + type: "object", + properties: { repo: { type: "string" }, invented: { type: "number" } }, + }, + }, + }), + ], + ), + ).toEqual(["a advertises input property variants, which is not a narrowing of its contract"]); + }); + + it("treats additionalProperties:false on the contract as equal to the SDK omitting it", () => { + // z.toJSONSchema emits the closed-object keyword; the MCP SDK's wire schema drops it. Same zod + // input, two converters -- not a widening. + expect( + checkInputNarrowing( + [projected({ meta: { type: "object", properties: { a: { type: "string" } }, additionalProperties: false } })], + [advertised({ meta: { type: "object", properties: { a: { type: "string" } } } })], + ), + ).toEqual([]); + }); + + it("still reports additionalProperties:true as a non-narrowing difference", () => { + expect( + checkInputNarrowing( + [projected({ meta: { type: "object", properties: { a: { type: "string" } }, additionalProperties: false } })], + [advertised({ meta: { type: "object", properties: { a: { type: "string" } }, additionalProperties: true } })], + ), + ).toEqual(["a advertises input property meta, which is not a narrowing of its contract"]); + }); }); describe("the version lock's serverInfo leg (#9661)", () => {