fix: address Copilot round 5 on the list-salvage path (#1909)
Both findings are the same shape: the fallback accepting what the strict path
would not.
- `ModernResultEnvelopeSchema` typed `ttlMs` as any number, where the codec
requires a non-negative integer. `ttlMs: -1` (or `0.5`) plus one bad entry
would salvage, reporting the entry and hiding the envelope violation. Tightened
to `z.int().min(0)`.
- The item schema was the SDK's era-NEUTRAL `ToolSchema`, and the negotiated
era's wire schema is stricter in ways the SDK does not export. Verified
against SDK 2.0.0 on a real connection: on legacy, a tool with
`outputSchema: { type: "array" }` is refused by the strict aggregate but
accepted by the neutral schema; on modern, `execution` is STRIPPED by the
strict path but kept by the neutral one. Either way a salvaged list could show
— and let the user call — a tool the connection's era says isn't valid.
`toolItemSchemaForEra` closes both. Replicating rules that live in the SDK's
internals is a real cost; `listSalvage-era.test.ts` is what makes it safe to
pay, pinning each rule to the strict path's OBSERVED behavior so a divergence
that closes or widens fails loudly instead of rotting.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
573b14e14e
commit
71cbe27d83
2 changed files with 278 additions and 5 deletions
|
|
@ -161,8 +161,8 @@ import {
|
|||
ListPromptsResultSchema,
|
||||
// Per-primitive item schemas — the salvage fallback validates a list's
|
||||
// entries one at a time with these when the whole-result parse rejects
|
||||
// (#1909, see `listAllSalvaging`).
|
||||
ToolSchema,
|
||||
// (#1909, see `listAllSalvaging`). Tools go through `toolItemSchemaForEra`
|
||||
// instead, since the neutral schema is looser than the negotiated era's.
|
||||
ResourceSchema,
|
||||
ResourceTemplateSchema,
|
||||
PromptSchema,
|
||||
|
|
@ -174,6 +174,7 @@ import { validateToolOutput } from "./toolOutputValidation.js";
|
|||
import {
|
||||
ModernResultEnvelopeSchema,
|
||||
isSalvageableRejection,
|
||||
toolItemSchemaForEra,
|
||||
nextCursorOf,
|
||||
lenientListPageSchema,
|
||||
rawItemsOf,
|
||||
|
|
@ -3190,7 +3191,9 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
method: "tools/list",
|
||||
itemsKey: "tools",
|
||||
resultSchema: ListToolsResultSchema,
|
||||
itemSchema: ToolSchema,
|
||||
// Era-aware: the neutral `ToolSchema` is more permissive than the
|
||||
// negotiated era's wire schema — see `toolItemSchemaForEra`.
|
||||
itemSchema: toolItemSchemaForEra(this.isModernEra()),
|
||||
metadata: options?.metadata,
|
||||
finalize: (salvaged) => this.excludeInvalidXMcpHeaderTools(salvaged),
|
||||
aggregate: async () => [
|
||||
|
|
@ -3656,7 +3659,7 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
const { valid, malformed } = salvageListItems({
|
||||
method: "tools/list",
|
||||
items,
|
||||
schema: ToolSchema,
|
||||
schema: toolItemSchemaForEra(this.isModernEra()),
|
||||
});
|
||||
if (rejectedResponseId !== undefined && malformed.length > 0) {
|
||||
this.dispatchTypedEvent("responseRejected", {
|
||||
|
|
|
|||
|
|
@ -25,6 +25,8 @@
|
|||
|
||||
import { z } from "zod/v4";
|
||||
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
|
||||
import { ToolSchema } from "@modelcontextprotocol/core";
|
||||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
|
||||
/**
|
||||
* One entry the Inspector dropped from a list result because it failed the
|
||||
|
|
@ -140,7 +142,10 @@ export function nextCursorOf(page: unknown): string | undefined {
|
|||
*/
|
||||
export const ModernResultEnvelopeSchema = z.looseObject({
|
||||
resultType: z.literal("complete"),
|
||||
ttlMs: z.number(),
|
||||
// A non-negative INTEGER, matching the codec: `ttlMs: -1` or `0.5` is an
|
||||
// envelope violation the strict path rejects, and accepting it here on the
|
||||
// strength of an unrelated bad entry would hide it.
|
||||
ttlMs: z.int().min(0),
|
||||
cacheScope: z.enum(["public", "private"]),
|
||||
});
|
||||
|
||||
|
|
@ -261,3 +266,40 @@ export function summarizeMalformed(malformed: MalformedListItem[]): string {
|
|||
malformed.length === 1 ? "entry" : "entries"
|
||||
} — ${where}: ${first.reason}${rest}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* The item contract for `tools/list` on a given era.
|
||||
*
|
||||
* The public `ToolSchema` is era-NEUTRAL, and the negotiated era's wire schema
|
||||
* is stricter in ways the SDK does not export. Validating candidates against
|
||||
* the neutral schema alone would let the fallback keep an entry the strict path
|
||||
* refused — an entry the user would then see, and could call, on a connection
|
||||
* whose era says it isn't valid. The two known divergences (verified against
|
||||
* SDK 2.0.0 by `listSalvage-era.test.ts`, which fails if either changes):
|
||||
*
|
||||
* - **legacy (2025)** requires `outputSchema.type === "object"`; the neutral
|
||||
* schema accepts any object, so a tool with `outputSchema: { type: "array" }`
|
||||
* would survive salvage while the strict parse rejects the whole page.
|
||||
* - **modern (2026)** deletes `execution`, and the strict path STRIPS it. Left
|
||||
* in place, a salvaged tool would carry a field the era doesn't have, and the
|
||||
* UI would render capability the connection can't honor.
|
||||
*
|
||||
* Replicating rules that live in the SDK's internals is a real cost, taken
|
||||
* deliberately: the alternative is a fallback that is quietly more permissive
|
||||
* than the path it stands in for. The tests pin each rule to observed SDK
|
||||
* behavior, so a divergence that closes (or widens) fails loudly rather than
|
||||
* rotting.
|
||||
*/
|
||||
export function toolItemSchemaForEra(isModern: boolean): z.ZodType<Tool> {
|
||||
if (isModern) {
|
||||
// `execution` is not part of the 2026 wire shape; drop it as the codec does.
|
||||
return ToolSchema.transform((tool) => {
|
||||
if (tool.execution === undefined) return tool;
|
||||
const { execution: _execution, ...rest } = tool;
|
||||
return rest;
|
||||
});
|
||||
}
|
||||
return ToolSchema.extend({
|
||||
outputSchema: z.looseObject({ type: z.literal("object") }).optional(),
|
||||
});
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue