From ba91db97e241d5afa0b00ae5453543e11e1d2295 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 13:39:16 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2034?= =?UTF-8?q?=20=E2=80=94=20structured=20constants,=20non-required=20const?= =?UTF-8?q?=20controls?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A `const` may be an object or an array, with or without a `type`, and `String({...})` is "[object Object]" — which no CLI argument can equal, so the only value such a schema accepts never matched. Supplied text is parsed and compared structurally through the resolver's own comparison (exported as `sameJsonValue` so the two cannot disagree), for both branch selection and the substitution. - The TUI's one-option const control is never marked required: its option may legitimately be the empty string, which ink-form's required gate can never accept, so submission would not even reach `decodeFormValues`. The value is fixed by the schema, and `missingRequiredFields` still validates the call. Signed-off-by: cliffhall --- clients/tui/__tests__/schemaToForm.test.ts | 18 ++++++++++++++++ clients/tui/src/utils/schemaToForm.ts | 6 ++++++ clients/web/src/test/core/jsonUtils.test.ts | 22 +++++++++++++++++++ core/json/jsonUtils.ts | 24 +++++++++++++++++++-- core/json/rootUnion.ts | 12 ++++++++++- 5 files changed, 79 insertions(+), 3 deletions(-) diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index ec8d3aeb..1ced5357 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -503,6 +503,24 @@ describe("schemaToForm", () => { }); }); + it("never marks a const control required", () => { + // Its one option may be the empty string, which ink-form's required gate + // can never accept — the call would not even reach `decodeFormValues`. + const form = schemaToForm( + { + type: "object", + properties: { kind: { type: "string", const: "" } }, + required: ["kind"], + }, + "empty_const", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ + name: "kind", + type: "select", + required: false, + }); + }); + it("renders a const outside a union the same way", () => { const form = schemaToForm( { diff --git a/clients/tui/src/utils/schemaToForm.ts b/clients/tui/src/utils/schemaToForm.ts index dd034cf6..4eb4fc9f 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -357,6 +357,12 @@ function buildFields(schema: JsonSchemaObject): FormField[] { fields.push({ type: "select", ...baseField, + // Never required: the one option may legitimately be the empty string, + // which ink-form's required gate can never accept — submission would + // not even reach `decodeFormValues`, which reapplies the constant. The + // value is fixed by the schema, and `missingRequiredFields` still + // validates the decoded call. + required: false, initialValue: String(pinned), options: [{ label: String(pinned), value: String(pinned) }], } as FormField); diff --git a/clients/web/src/test/core/jsonUtils.test.ts b/clients/web/src/test/core/jsonUtils.test.ts index 017a5621..68c4190b 100644 --- a/clients/web/src/test/core/jsonUtils.test.ts +++ b/clients/web/src/test/core/jsonUtils.test.ts @@ -312,6 +312,28 @@ describe("JSON Utils", () => { ).toEqual({ value: "true" }); }); + it("matches a structured const by parsing the supplied text (#2123)", () => { + const structured: Tool = { + name: "structured-const", + inputSchema: { + type: "object", + properties: { tag: { const: { kind: "x", n: 1 } } }, + }, + }; + // `String({...})` is "[object Object]", which no argument can equal — so + // the only value the schema accepts would never have matched. + expect( + convertToolParameters(structured, { tag: '{"n":1,"kind":"x"}' }), + ).toEqual({ tag: { kind: "x", n: 1 } }); + // Text that is not that value, or not JSON at all, is left alone. + expect(convertToolParameters(structured, { tag: "{}" })).toEqual({ + tag: "{}", + }); + expect(convertToolParameters(structured, { tag: "nope" })).toEqual({ + tag: "nope", + }); + }); + it("leaves an ambiguously typed argument uncoerced (#2123)", () => { const ambiguous: Tool = { name: "ambiguous", diff --git a/core/json/jsonUtils.ts b/core/json/jsonUtils.ts index 00402915..cecce758 100644 --- a/core/json/jsonUtils.ts +++ b/core/json/jsonUtils.ts @@ -2,6 +2,7 @@ import type { Tool } from "@modelcontextprotocol/client"; import { normalizeNullableUnion } from "./nullableUnion.js"; import { narrowBySuppliedNames, + sameJsonValue, resolveRootUnion, type RootUnionBranch, type RootUnionSchema, @@ -253,10 +254,29 @@ function matchesConstants( // read the inherited one and rule out every branch that pins that name. if (!Object.hasOwn(params, name)) return true; const supplied = params[name]; - return supplied === undefined || supplied === String(constValue); + return supplied === undefined || suppliedMatchesConst(supplied, constValue); }); } +/** + * Whether the text a CLI argument carries is the value a `const` fixes. + * + * A primitive constant is compared as text, which is all a command line has. A + * structured one — a `const` may be an object or an array, with or without a + * `type` — is parsed first: `String({...})` is `"[object Object]"`, which no + * argument can equal, so the only value the schema accepts would never match. + */ +function suppliedMatchesConst(value: string, constValue: unknown): boolean { + if (constValue === null || typeof constValue !== "object") { + return value === String(constValue); + } + try { + return sameJsonValue(JSON.parse(value), constValue); + } catch { + return false; + } +} + /** * Convert string parameters to JSON values based on tool schema */ @@ -288,7 +308,7 @@ export function convertToolParameters( // user's input and is left alone. const pinned = (paramSchema as { const?: unknown } | undefined)?.const; const converted = - pinned !== undefined && String(pinned) === value + pinned !== undefined && suppliedMatchesConst(value, pinned) ? (pinned as JsonValue) : paramSchema ? convertParameterValue(value, paramSchema) diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index 06104742..01db8290 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -240,7 +240,17 @@ function canonicalJson(value: unknown): string { return JSON.stringify(value) ?? "undefined"; } -/** Structural equality, via canonical JSON — enough for schema keyword values. */ +/** + * Structural equality, via canonical JSON — enough for schema keyword values. + * + * Exported because a caller comparing a *supplied* value against a schema's + * `const` has to reach the same answer this module does; two implementations + * would disagree the moment one of them met an object. + */ +export function sameJsonValue(a: unknown, b: unknown): boolean { + return sameValue(a, b); +} + function sameValue(a: unknown, b: unknown): boolean { return a === b || canonicalJson(a) === canonicalJson(b); }