Skip to content

Commit f8fe08d

Browse files
fix(mcp): compare property schemas in checkInputNarrowing (#10124)
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.
1 parent d1c105e commit f8fe08d

5 files changed

Lines changed: 294 additions & 29 deletions

File tree

packages/loopover-contract/src/tools/local-branch.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,10 @@ const localValidationEntry = z.object({
6868
* `cwd` is what makes this family `local-git`: it names a checkout only the caller's machine has.
6969
*/
7070
export const CurrentBranchInput = z.object({
71-
login: z.string().min(1).optional(),
71+
// Bounds match LocalBranchAnalysisInput on shared fields so a stdio override that serves this
72+
// shape (e.g. StdioCompareLocalVariantsInput) is a true narrowing under checkInputNarrowing (#10041),
73+
// not a silent loosening of maxLength / maxItems the wider contract already publishes.
74+
login: z.string().min(1).max(SCENARIO_LIMITS.branchRefChars).optional(),
7275
cwd: z.string().optional(),
7376
repoFullName: z.string().min(3).max(SCENARIO_LIMITS.repoFullNameChars).optional(),
7477
baseRef: z.string().max(SCENARIO_LIMITS.branchRefChars).optional(),
@@ -77,15 +80,15 @@ export const CurrentBranchInput = z.object({
7780
title: z.string().optional(),
7881
body: z.string().optional(),
7982
labels: z.array(z.string()).optional(),
80-
linkedIssues: z.array(z.number().int().positive()).optional(),
83+
linkedIssues: z.array(z.number().int().positive()).max(SCENARIO_MAX_LINKED_ISSUE_NUMBERS).optional(),
8184
pendingMergedPrCount: z.number().int().min(0).optional(),
8285
pendingClosedPrCount: z.number().int().min(0).optional(),
8386
approvedPrCount: z.number().int().min(0).optional(),
8487
expectedOpenPrCountAfterMerge: z.number().int().min(0).optional(),
8588
projectedCredibility: z.number().min(0).max(1).optional(),
86-
scenarioNotes: z.array(z.string()).optional(),
89+
scenarioNotes: z.array(z.string()).max(20).optional(),
8790
branchEligibility: callerBranchEligibilitySchema.optional(),
88-
validation: z.array(localValidationEntry).optional(),
91+
validation: z.array(localValidationEntry).max(50).optional(),
8992
scorePreviewCommand: z.string().optional(),
9093
});
9194

@@ -307,7 +310,9 @@ export const LocalScoreInput = z.object({
307310
approvedPrCount: z.number().int().min(0).optional(),
308311
expectedOpenPrCountAfterMerge: z.number().int().min(0).optional(),
309312
projectedCredibility: z.number().min(0).max(1).optional(),
310-
scenarioNotes: z.array(z.string()).optional(),
313+
// Cap matches ExplainScoreBreakdownInput / api-requests so RemoteLocalScorePreviewInput is a
314+
// true narrowing of that contract field under checkInputNarrowing (#10041).
315+
scenarioNotes: z.array(z.string()).max(20).optional(),
311316
branchEligibility: callerBranchEligibilitySchema.optional(),
312317
scorePreviewCommand: z.string().optional(),
313318
});

packages/loopover-contract/src/tools/miner-ops.ts

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,20 @@ export const minerDoctorTool = defineTool({
5656
export const MinerMetricsSnapshotInput = z.object({});
5757

5858
export const MinerMetricsSnapshotOutput = z.looseObject({
59-
generatedAt: z.string(),
60-
families: z.array(
61-
z.looseObject({
62-
name: z.string(),
63-
type: z.string(),
64-
help: z.string().optional(),
65-
samples: z.array(z.looseObject({ value: z.number(), labels: z.record(z.string(), z.string()).optional() })),
66-
}),
67-
),
59+
// Optional: a store-failure envelope from `withMinerToolErrorHandling` carries only `error`, and
60+
// the MCP SDK validates structuredContent against this schema even when `isError` is set — required
61+
// success fields made that path a -32602 instead of a clean tool error.
62+
generatedAt: z.string().optional(),
63+
families: z
64+
.array(
65+
z.looseObject({
66+
name: z.string(),
67+
type: z.string(),
68+
help: z.string().optional(),
69+
samples: z.array(z.looseObject({ value: z.number(), labels: z.record(z.string(), z.string()).optional() })),
70+
}),
71+
)
72+
.optional(),
6873
// #9659: every miner tool answers a store failure with the shared error envelope
6974
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
7075
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller

scripts/lib/validate-mcp/invariants.ts

Lines changed: 123 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,115 @@ export function checkAdvertisedMetadata(expected: readonly McpToolDefinition[],
7878
return failures;
7979
}
8080

81+
const BOUND_KEYS = ["minimum", "maximum", "minLength", "maxLength", "minItems", "maxItems"] as const;
82+
const LOWER_BOUNDS = new Set<string>(["minimum", "minLength", "minItems"]);
83+
84+
function isPlainObject(value: unknown): value is Record<string, unknown> {
85+
return typeof value === "object" && value !== null && !Array.isArray(value);
86+
}
87+
88+
function jsonDeepEqual(left: unknown, right: unknown): boolean {
89+
if (left === right) return true;
90+
if (Array.isArray(left) && Array.isArray(right)) {
91+
return left.length === right.length && left.every((entry, index) => jsonDeepEqual(entry, right[index]));
92+
}
93+
if (!isPlainObject(left) || !isPlainObject(right)) return false;
94+
const leftKeys = Object.keys(left);
95+
const rightKeys = Object.keys(right);
96+
if (leftKeys.length !== rightKeys.length) return false;
97+
return leftKeys.every((key) => Object.prototype.hasOwnProperty.call(right, key) && jsonDeepEqual(left[key], right[key]));
98+
}
99+
100+
/**
101+
* The MCP SDK's zod→JSON-Schema path omits `additionalProperties: false` that `z.toJSONSchema`
102+
* (draft-2020-12) emits for the same object. Both sides of this check are already JSON Schema, but
103+
* they are not produced by the same converter, so a closed object on the contract and an omitted
104+
* keyword on the wire are the same schema, not a widening. A present `true` (or a non-false schema)
105+
* is still a real difference and must not be stripped.
106+
*/
107+
function withoutClosedAdditionalProperties(value: unknown): unknown {
108+
if (Array.isArray(value)) return value.map(withoutClosedAdditionalProperties);
109+
if (!isPlainObject(value)) return value;
110+
const out: Record<string, unknown> = {};
111+
for (const [key, entry] of Object.entries(value)) {
112+
if (key === "additionalProperties" && entry === false) continue;
113+
out[key] = withoutClosedAdditionalProperties(entry);
114+
}
115+
return out;
116+
}
117+
118+
/**
119+
* Whether an advertised JSON Schema subtree is identical to the contract's, or a recognised
120+
* narrowing of it (#10041).
121+
*
122+
* Recognised differences only: (a) a removed key under `properties`, (b) an added or tightened
123+
* minimum/maximum/minLength/maxLength/minItems/maxItems, (c) an `enum` that is a subset of the
124+
* contract's, (d) the same rules applied recursively through `items` / `properties`. Nested
125+
* `required` follows the top-level rule: every advertised requirement must already be required by
126+
* the contract (dropping a requirement is allowed; inventing one is not).
127+
*/
128+
function isJsonSchemaNarrowing(advertised: unknown, contract: unknown): boolean {
129+
advertised = withoutClosedAdditionalProperties(advertised);
130+
contract = withoutClosedAdditionalProperties(contract);
131+
if (jsonDeepEqual(advertised, contract)) return true;
132+
if (!isPlainObject(advertised) || !isPlainObject(contract)) return false;
133+
134+
if (advertised.type !== undefined && advertised.type !== contract.type) return false;
135+
if (contract.type !== undefined && advertised.type === undefined) return false;
136+
137+
if (advertised.enum !== undefined || contract.enum !== undefined) {
138+
if (!Array.isArray(contract.enum) || !Array.isArray(advertised.enum)) return false;
139+
if (!advertised.enum.every((value) => (contract.enum as unknown[]).some((entry) => jsonDeepEqual(entry, value)))) {
140+
return false;
141+
}
142+
}
143+
144+
if (advertised.properties !== undefined || contract.properties !== undefined) {
145+
if (advertised.properties !== undefined && !isPlainObject(advertised.properties)) return false;
146+
if (contract.properties !== undefined && !isPlainObject(contract.properties)) return false;
147+
const advertisedProperties = isPlainObject(advertised.properties) ? advertised.properties : {};
148+
const contractProperties = isPlainObject(contract.properties) ? contract.properties : {};
149+
for (const key of Object.keys(advertisedProperties)) {
150+
if (!Object.prototype.hasOwnProperty.call(contractProperties, key)) return false;
151+
if (!isJsonSchemaNarrowing(advertisedProperties[key], contractProperties[key])) return false;
152+
}
153+
}
154+
155+
if (advertised.items !== undefined || contract.items !== undefined) {
156+
if (advertised.items === undefined || contract.items === undefined) return false;
157+
if (!isJsonSchemaNarrowing(advertised.items, contract.items)) return false;
158+
}
159+
160+
if (Array.isArray(advertised.required)) {
161+
const contractRequired = new Set(Array.isArray(contract.required) ? contract.required : []);
162+
for (const property of advertised.required) {
163+
if (typeof property !== "string" || !contractRequired.has(property)) return false;
164+
}
165+
} else if (advertised.required !== undefined) {
166+
return false;
167+
}
168+
169+
for (const key of BOUND_KEYS) {
170+
const advertisedBound = advertised[key];
171+
const contractBound = contract[key];
172+
if (advertisedBound === undefined && contractBound === undefined) continue;
173+
if (typeof advertisedBound !== "number" && advertisedBound !== undefined) return false;
174+
if (typeof contractBound !== "number" && contractBound !== undefined) return false;
175+
if (advertisedBound === undefined) return false; // contract had a bound the advertisement dropped
176+
if (contractBound === undefined) continue; // added bound = tightening
177+
if (LOWER_BOUNDS.has(key) ? advertisedBound < contractBound : advertisedBound > contractBound) return false;
178+
}
179+
180+
const handled = new Set<string>(["type", "enum", "properties", "items", "required", ...BOUND_KEYS]);
181+
for (const key of new Set([...Object.keys(advertised), ...Object.keys(contract)])) {
182+
if (handled.has(key)) continue;
183+
if (!jsonDeepEqual(advertised[key], contract[key])) return false;
184+
}
185+
return true;
186+
}
187+
81188
/**
82-
* An advertised input may only NARROW the contract's, never widen it (#9662).
189+
* An advertised input may only NARROW the contract's, never widen it (#9662, #10041).
83190
*
84191
* `registerStdioTool`'s override is documented as one-way -- "a server may serve LESS than the contract
85192
* 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[],
88195
* and the smoke arguments are synthesized FROM the advertised schema, so a widened schema simply gets
89196
* widened arguments and passes.
90197
*
91-
* Narrowing is defined mechanically, which is all a schema comparison can honestly do here: every
92-
* advertised property exists in the contract's, and every advertised requirement is one the contract
93-
* also requires. Making an optional contract field required is therefore a widening of the caller's
94-
* obligations and fails -- which is the case a hand-written override is most likely to get wrong.
198+
* Narrowing is the property-name / required checks below, plus a recursive comparison of every shared
199+
* property's JSON Schema subtree (see `isJsonSchemaNarrowing`). Making an optional contract field
200+
* required is therefore a widening of the caller's obligations and fails -- which is the case a
201+
* hand-written override is most likely to get wrong.
95202
*/
96203
export function checkInputNarrowing(expected: readonly McpToolDefinition[], listed: readonly ListedTool[]): string[] {
97204
const listedByName = new Map(listed.map((tool) => [tool.name, tool]));
@@ -100,10 +207,18 @@ export function checkInputNarrowing(expected: readonly McpToolDefinition[], list
100207
const advertised = listedByName.get(tool.name);
101208
// Absent is diffToolSets' finding; a schema-less advertisement is checkAdvertisedShape's.
102209
if (!advertised?.inputSchema) continue;
103-
const contractProperties = new Set(Object.keys((tool.inputSchema as { properties?: Record<string, unknown> }).properties ?? {}));
210+
const contractProperties = (tool.inputSchema as { properties?: Record<string, unknown> }).properties ?? {};
211+
const contractPropertyNames = new Set(Object.keys(contractProperties));
104212
const contractRequired = new Set((tool.inputSchema as { required?: string[] }).required ?? []);
105-
for (const property of Object.keys(advertised.inputSchema.properties ?? {})) {
106-
if (!contractProperties.has(property)) failures.push(`${tool.name} advertises input property ${property}, which its contract does not declare`);
213+
const advertisedProperties = advertised.inputSchema.properties ?? {};
214+
for (const property of Object.keys(advertisedProperties)) {
215+
if (!contractPropertyNames.has(property)) {
216+
failures.push(`${tool.name} advertises input property ${property}, which its contract does not declare`);
217+
continue;
218+
}
219+
if (!isJsonSchemaNarrowing(advertisedProperties[property], contractProperties[property])) {
220+
failures.push(`${tool.name} advertises input property ${property}, which is not a narrowing of its contract`);
221+
}
107222
}
108223
for (const property of advertised.inputSchema.required ?? []) {
109224
if (!contractRequired.has(property)) failures.push(`${tool.name} requires input property ${property}, which its contract does not require`);

test/unit/miner-mcp-ops-tools.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,17 @@ describe("loopover_miner_get_metrics_snapshot (#9523)", () => {
136136
expect(result).toBeTruthy();
137137
});
138138

139+
it("surfaces a ledger that will not open as isError rather than an SDK schema rejection", async () => {
140+
const client = await connect({
141+
initPredictionLedger: () => {
142+
throw new Error("prediction ledger unreadable");
143+
},
144+
});
145+
const result = (await client.callTool({ name: "loopover_miner_get_metrics_snapshot", arguments: {} })) as ToolResult;
146+
expect(result.isError).toBe(true);
147+
expect(structured(result).error).toMatchObject({ message: "prediction ledger unreadable" });
148+
});
149+
139150
it("emits every counter even for an empty ledger, so the surface is well-formed before any prediction", async () => {
140151
const client = await connect({
141152
initPredictionLedger: () => ({ readPredictions: () => [], close: () => undefined }) as never,

0 commit comments

Comments
 (0)