Skip to content

Commit ba91db9

Browse files
committed
fix: address Copilot review round 34 — structured constants, non-required const controls
- 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 <cliff@futurescale.com>
1 parent ffa08d7 commit ba91db9

5 files changed

Lines changed: 79 additions & 3 deletions

File tree

‎clients/tui/__tests__/schemaToForm.test.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -503,6 +503,24 @@ describe("schemaToForm", () => {
503503
});
504504
});
505505

506+
it("never marks a const control required", () => {
507+
// Its one option may be the empty string, which ink-form's required gate
508+
// can never accept — the call would not even reach `decodeFormValues`.
509+
const form = schemaToForm(
510+
{
511+
type: "object",
512+
properties: { kind: { type: "string", const: "" } },
513+
required: ["kind"],
514+
},
515+
"empty_const",
516+
);
517+
expect(form.sections[0]!.fields[0]).toMatchObject({
518+
name: "kind",
519+
type: "select",
520+
required: false,
521+
});
522+
});
523+
506524
it("renders a const outside a union the same way", () => {
507525
const form = schemaToForm(
508526
{

‎clients/tui/src/utils/schemaToForm.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,12 @@ function buildFields(schema: JsonSchemaObject): FormField[] {
357357
fields.push({
358358
type: "select",
359359
...baseField,
360+
// Never required: the one option may legitimately be the empty string,
361+
// which ink-form's required gate can never accept — submission would
362+
// not even reach `decodeFormValues`, which reapplies the constant. The
363+
// value is fixed by the schema, and `missingRequiredFields` still
364+
// validates the decoded call.
365+
required: false,
360366
initialValue: String(pinned),
361367
options: [{ label: String(pinned), value: String(pinned) }],
362368
} as FormField);

‎clients/web/src/test/core/jsonUtils.test.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,28 @@ describe("JSON Utils", () => {
312312
).toEqual({ value: "true" });
313313
});
314314

315+
it("matches a structured const by parsing the supplied text (#2123)", () => {
316+
const structured: Tool = {
317+
name: "structured-const",
318+
inputSchema: {
319+
type: "object",
320+
properties: { tag: { const: { kind: "x", n: 1 } } },
321+
},
322+
};
323+
// `String({...})` is "[object Object]", which no argument can equal — so
324+
// the only value the schema accepts would never have matched.
325+
expect(
326+
convertToolParameters(structured, { tag: '{"n":1,"kind":"x"}' }),
327+
).toEqual({ tag: { kind: "x", n: 1 } });
328+
// Text that is not that value, or not JSON at all, is left alone.
329+
expect(convertToolParameters(structured, { tag: "{}" })).toEqual({
330+
tag: "{}",
331+
});
332+
expect(convertToolParameters(structured, { tag: "nope" })).toEqual({
333+
tag: "nope",
334+
});
335+
});
336+
315337
it("leaves an ambiguously typed argument uncoerced (#2123)", () => {
316338
const ambiguous: Tool = {
317339
name: "ambiguous",

‎core/json/jsonUtils.ts‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type { Tool } from "@modelcontextprotocol/client";
22
import { normalizeNullableUnion } from "./nullableUnion.js";
33
import {
44
narrowBySuppliedNames,
5+
sameJsonValue,
56
resolveRootUnion,
67
type RootUnionBranch,
78
type RootUnionSchema,
@@ -253,10 +254,29 @@ function matchesConstants(
253254
// read the inherited one and rule out every branch that pins that name.
254255
if (!Object.hasOwn(params, name)) return true;
255256
const supplied = params[name];
256-
return supplied === undefined || supplied === String(constValue);
257+
return supplied === undefined || suppliedMatchesConst(supplied, constValue);
257258
});
258259
}
259260

261+
/**
262+
* Whether the text a CLI argument carries is the value a `const` fixes.
263+
*
264+
* A primitive constant is compared as text, which is all a command line has. A
265+
* structured one — a `const` may be an object or an array, with or without a
266+
* `type` — is parsed first: `String({...})` is `"[object Object]"`, which no
267+
* argument can equal, so the only value the schema accepts would never match.
268+
*/
269+
function suppliedMatchesConst(value: string, constValue: unknown): boolean {
270+
if (constValue === null || typeof constValue !== "object") {
271+
return value === String(constValue);
272+
}
273+
try {
274+
return sameJsonValue(JSON.parse(value), constValue);
275+
} catch {
276+
return false;
277+
}
278+
}
279+
260280
/**
261281
* Convert string parameters to JSON values based on tool schema
262282
*/
@@ -288,7 +308,7 @@ export function convertToolParameters(
288308
// user's input and is left alone.
289309
const pinned = (paramSchema as { const?: unknown } | undefined)?.const;
290310
const converted =
291-
pinned !== undefined && String(pinned) === value
311+
pinned !== undefined && suppliedMatchesConst(value, pinned)
292312
? (pinned as JsonValue)
293313
: paramSchema
294314
? convertParameterValue(value, paramSchema)

‎core/json/rootUnion.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,17 @@ function canonicalJson(value: unknown): string {
240240
return JSON.stringify(value) ?? "undefined";
241241
}
242242

243-
/** Structural equality, via canonical JSON — enough for schema keyword values. */
243+
/**
244+
* Structural equality, via canonical JSON — enough for schema keyword values.
245+
*
246+
* Exported because a caller comparing a *supplied* value against a schema's
247+
* `const` has to reach the same answer this module does; two implementations
248+
* would disagree the moment one of them met an object.
249+
*/
250+
export function sameJsonValue(a: unknown, b: unknown): boolean {
251+
return sameValue(a, b);
252+
}
253+
244254
function sameValue(a: unknown, b: unknown): boolean {
245255
return a === b || canonicalJson(a) === canonicalJson(b);
246256
}

0 commit comments

Comments
 (0)