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)", () => {