From c8c5442dcf737994c773fdfe9d5dd8375d9e64cc Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 10:07:35 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2027?= =?UTF-8?q?=20=E2=80=94=20unconstrained=20null=20branches,=20typed=20discr?= =?UTF-8?q?iminators?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - anyOfAdmitsNull recognizes a branch that constrains nothing (`{}`, or annotations only): it admits every value, null among them, so a field pinned to `const: null` beside such a union stays submittable. `admitsNull` cannot say that on its own — an unconstrained schema tells it nothing either way and it declines rather than guesses — but here the question is narrower. - convertToolParameters sends a matched `const` as the schema's own typed value: `kind=2` selects a branch pinned to `const: 2` and was then sent `"2"`, which that same branch rejects. Only an exact match is substituted; anything else is the user's input and is left alone. Signed-off-by: cliffhall --- clients/web/src/test/core/jsonUtils.test.ts | 30 +++++++++++++++++++ .../web/src/test/core/nullableUnion.test.ts | 9 ++++++ core/json/jsonUtils.ts | 10 ++++++- core/json/nullableUnion.ts | 27 +++++++++++++++++ 4 files changed, 75 insertions(+), 1 deletion(-) diff --git a/clients/web/src/test/core/jsonUtils.test.ts b/clients/web/src/test/core/jsonUtils.test.ts index 6cd4c5f3..e71b1e30 100644 --- a/clients/web/src/test/core/jsonUtils.test.ts +++ b/clients/web/src/test/core/jsonUtils.test.ts @@ -215,6 +215,36 @@ describe("JSON Utils", () => { }); }); + it("sends a non-string discriminator as its typed constant (#2123)", () => { + const numericallyPinned: Tool = { + name: "numerically-pinned", + inputSchema: { + type: "object", + oneOf: [ + { + type: "object", + properties: { kind: { const: 1 }, a: { type: "string" } }, + required: ["kind"], + }, + { + type: "object", + properties: { kind: { const: 2 }, b: { type: "string" } }, + required: ["kind"], + }, + ], + }, + }; + // `kind=2` selects the second branch, which then rejects `"2"` — the + // schema's own typed value is what goes on the wire. + expect(convertToolParameters(numericallyPinned, { kind: "2" })).toEqual({ + kind: 2, + }); + // Text that matches no constant is the user's input and is left alone. + expect(convertToolParameters(numericallyPinned, { kind: "9" })).toEqual({ + kind: "9", + }); + }); + it("leaves an ambiguously typed argument uncoerced (#2123)", () => { const ambiguous: Tool = { name: "ambiguous", diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts index d7090864..c1054c83 100644 --- a/clients/web/src/test/core/nullableUnion.test.ts +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -759,6 +759,15 @@ describe("admitsNull", () => { ); }); + it("admits null when a sibling branch constrains nothing", () => { + // `{}` is the equivalent of `true` — it admits every value, null among + // them — so the pinned null stays reachable and its field submittable. + expect(admitsNull({ const: null, anyOf: [{}] })).toBe(true); + expect(admitsNull({ const: null, anyOf: [{ title: "anything" }] })).toBe( + true, + ); + }); + it("does not override a sibling that rejects null", () => { // `const` is conjunctive with its siblings, not an override: both of // these reject every value, so claiming nullability would let the diff --git a/core/json/jsonUtils.ts b/core/json/jsonUtils.ts index aa5030c6..147a6180 100644 --- a/core/json/jsonUtils.ts +++ b/core/json/jsonUtils.ts @@ -264,7 +264,15 @@ export function convertToolParameters( for (const [key, value] of Object.entries(params)) { const paramSchema = properties[key] as ParameterSchema | undefined; - if (paramSchema) { + // A `const` the supplied text names is sent as the schema's own typed + // value, not as the text: a branch pinned to `const: 2` is selected by + // `kind=2` and would otherwise be sent `"2"`, which that same branch + // rejects. Only an exact match is substituted — anything else is the + // user's input and is left alone. + const pinned = (paramSchema as { const?: unknown } | undefined)?.const; + if (pinned !== undefined && String(pinned) === value) { + result[key] = pinned as JsonValue; + } else if (paramSchema) { result[key] = convertParameterValue(value, paramSchema); } else { result[key] = value; diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index e935b77c..3217b0f1 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -427,6 +427,28 @@ export function admitsNull(schema: NullableUnionSchema): boolean { return anyOfAdmitsNull(schema); } +/** + * Keywords that annotate rather than constrain — a schema carrying only these + * (or nothing at all) is the equivalent of `true`. + */ +const ANNOTATION_ONLY_KEYWORDS = new Set([ + "title", + "description", + "examples", + "default", + "deprecated", + "readOnly", + "writeOnly", + "$comment", +]); + +/** Whether a schema states no assertion at all. */ +function constrainsNothing(schema: NullableUnionSchema): boolean { + return Object.keys(schema).every((keyword) => + ANNOTATION_ONLY_KEYWORDS.has(keyword), + ); +} + /** Whether some `anyOf` branch is one that admits `null`. */ function anyOfAdmitsNull(schema: NullableUnionSchema): boolean { const branches = schema.anyOf; @@ -440,6 +462,11 @@ function anyOfAdmitsNull(schema: NullableUnionSchema): boolean { // a composition-free schema, which `admitsNull` answers without reaching // back into this function. if (hasUnevaluatedComposition(branchSchema)) return false; + // A branch that constrains nothing admits every value, `null` among them. + // `admitsNull` cannot say so on its own — an unconstrained schema tells it + // nothing either way, and it declines rather than guesses — but here the + // question is only whether this alternative leaves null on the table. + if (constrainsNothing(branchSchema)) return true; // Asked through `admitsNull` rather than by testing `type` alone, so a // branch spelling its nullability another way — `{ const: null }`, // `{ nullable: true }` — is recognized the same way the wrapper's own