fix(mcp): compare property schemas in checkInputNarrowing

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(),
 });

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

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<string>(["minimum", "minLength", "minItems"]);
+
+function isPlainObject(value: unknown): value is Record<string, unknown> {
+  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<string, unknown> = {};
+  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<string>(["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<string, unknown> }).properties ?? {}));
+    const contractProperties = (tool.inputSchema as { properties?: Record<string, unknown> }).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`);

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,