diff --git a/README.md b/README.md index 9890e969..5f78f6b3 100644 --- a/README.md +++ b/README.md @@ -304,9 +304,11 @@ The **TUI** has the same gap and is worth checking against the same server (`--t The sections are not as independent as they look, which is why the select is not cosmetic: ink-form keeps one value object for the whole form, keyed by field name alone, so two branches both declaring `kind` would be **one** field and the later section's initial value would decide what the earlier one submits. Each branch's fields are therefore rendered under a prefixed name and translated back on submit, where every branch but the chosen one is dropped. -The **CLI** has no form at all, but the same flattening decides how `--tool-arg` values are typed: a branch's `count: { "type": "number" }` is what turns `--tool-arg count=3` into `3` rather than `"3"`. All three read one helper, [`core/json/rootUnion.ts`](./core/json/rootUnion.ts), so they cannot drift on which schemas they can render. +The **CLI** has no form at all, but the same flattening decides how `--tool-arg` values are typed: a branch's `count: { "type": "number" }` is what turns `--tool-arg count=3` into `3` rather than `"3"`. Which branch is *inferred* rather than chosen — a discriminated union pins its discriminator with `const`, so the supplied arguments either identify one branch or they do not. When nothing identifies one, only the names every branch that declares them types the same way are coerced; a name one branch calls a number and another a boolean is passed through as the string it was typed as, rather than run through an arbitrary branch's schema. -Three things it deliberately does **not** do, each falling back to whatever the root `properties` describe rather than claiming something untrue. A union whose members are not all field-carrying object schemas is left alone rather than offered as a picker with options that render nothing. A schema carrying **both** `oneOf` and `anyOf` is declined outright: they are independent keywords a value satisfies together, not two spellings of one union, so reading one and dropping the other builds a form that silently omits real constraints — and satisfying both honestly means offering the cross product of their alternatives, which no real schema has yet asked for. And `not` is not interpreted at all: there is no faithful form for "anything except this". +All three read one helper, [`core/json/rootUnion.ts`](./core/json/rootUnion.ts), so they cannot drift on which schemas they can render. + +Four things it deliberately does **not** do, each falling back to whatever the root `properties` describe rather than claiming something untrue. A union whose members are not all field-carrying object schemas is left alone rather than offered as a picker with options that render nothing — as is one whose member `type` rules objects out, since tool arguments are a JSON object and such a member can never match. A union whose branch **contradicts** the root about a property's `type` is declined too: the two constraints are conjunctive, so `string` under a root `number` describes a value that cannot exist, and rendering either type would accept what the schema rejects. (A property both declare *compatibly* is merged rather than replaced, so a root's `minimum` survives a branch's `maximum`.) A schema carrying **both** `oneOf` and `anyOf` is declined outright: they are independent keywords a value satisfies together, not two spellings of one union, so reading one and dropping the other builds a form that silently omits real constraints — and satisfying both honestly means offering the cross product of their alternatives, which no real schema has yet asked for. And `not` is not interpreted at all: there is no faithful form for "anything except this". #### Unportable tool schemas diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index cf5afb8c..3a7fd456 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -1,9 +1,5 @@ import { describe, it, expect } from "vitest"; -import { - decodeFormValues, - schemaToForm, - VARIANT_FIELD, -} from "../src/utils/schemaToForm.js"; +import { decodeFormValues, schemaToForm } from "../src/utils/schemaToForm.js"; describe("schemaToForm", () => { it("returns an empty Parameters section when there is no schema", () => { @@ -445,7 +441,7 @@ describe("schemaToForm", () => { "sms", ]); expect(form.sections[0]!.fields.map((field) => field.name)).toEqual([ - VARIANT_FIELD, + "__variant", "note", ]); }); @@ -468,7 +464,7 @@ describe("schemaToForm", () => { it("offers a variant select listing the alternatives", () => { const form = schemaToForm(UNION, "union_tool"); expect(form.sections[0]!.fields[0]).toMatchObject({ - name: VARIANT_FIELD, + name: "__variant", type: "select", initialValue: "0", options: [ @@ -493,15 +489,17 @@ describe("schemaToForm", () => { } }); - it("seeds a branch's discriminator const so it need not be typed", () => { + it("renders a const as a one-option select so it cannot be changed", () => { const form = schemaToForm(UNION, "union_tool"); expect(form.sections[1]!.fields[0]).toMatchObject({ name: "__b0__kind", + type: "select", initialValue: "email", + options: [{ label: "email", value: "email" }], }); }); - it("prefers a const over a conflicting default", () => { + it("renders a const outside a union the same way", () => { const form = schemaToForm( { type: "object", @@ -509,14 +507,62 @@ describe("schemaToForm", () => { }, "const_tool", ); - expect(form.sections[0]!.fields[0]).toMatchObject({ initialValue: "a" }); + expect(form.sections[0]!.fields[0]).toMatchObject({ + type: "select", + initialValue: "a", + options: [{ label: "a", value: "a" }], + }); + }); + + it("renders a branch's specialization of a root property in its section", () => { + const form = schemaToForm( + { + type: "object", + properties: { count: {} }, + anyOf: [ + { type: "object", properties: { count: { type: "integer" } } }, + { type: "object", properties: { other: { type: "string" } } }, + ], + }, + "specializing", + ); + // The untyped base declaration is not rendered a second time as a string. + expect(form.sections[0]!.fields.map((field) => field.name)).toEqual([ + "__variant", + ]); + expect(form.sections[1]!.fields[0]).toMatchObject({ + name: "__b0__count", + type: "integer", + }); + }); + + it("keeps its generated names clear of the schema's own", () => { + const form = schemaToForm( + { + type: "object", + properties: { __variant: { type: "string" } }, + anyOf: [ + { type: "object", properties: { __b0__x: { type: "string" } } }, + { type: "object", properties: { y: { type: "string" } } }, + ], + }, + "colliding", + ); + const names = form.sections.flatMap((section) => + section.fields.map((field) => field.name), + ); + // The select steps aside for the declared `__variant`, and the branch + // prefix steps aside for the declared `__b0__x`. + expect(names).toContain("__variant_"); + expect(names).toContain("__variant"); + expect(names).toContain("__b_0____b0__x"); }); describe("decodeFormValues", () => { it("submits the chosen branch's fields under their real names", () => { expect( decodeFormValues(UNION, { - [VARIANT_FIELD]: "0", + __variant: "0", note: "hi", __b0__kind: "email", __b0__address: "a@b.c", @@ -529,7 +575,7 @@ describe("schemaToForm", () => { it("drops the branches the call is not making", () => { expect( decodeFormValues(UNION, { - [VARIANT_FIELD]: "1", + __variant: "1", __b0__kind: "email", __b0__address: "a@b.c", __b1__kind: "sms", @@ -541,7 +587,7 @@ describe("schemaToForm", () => { it("omits a branch field the user never filled", () => { expect( decodeFormValues(UNION, { - [VARIANT_FIELD]: "0", + __variant: "0", __b0__kind: "email", }), ).toEqual({ kind: "email" }); @@ -550,7 +596,7 @@ describe("schemaToForm", () => { it("falls back to the first branch on an unusable selection", () => { expect( decodeFormValues(UNION, { - [VARIANT_FIELD]: "nonsense", + __variant: "nonsense", __b0__kind: "email", }), ).toEqual({ kind: "email" }); @@ -562,6 +608,44 @@ describe("schemaToForm", () => { values, ); }); + + it("restores a const from the schema rather than trusting the form", () => { + // ink-form has no immutable field, and a select hands back a string — + // so the pinned value is re-applied on the way out, with its own type. + expect( + decodeFormValues( + { + type: "object", + anyOf: [ + { + type: "object", + properties: { n: { const: 7 }, a: { type: "string" } }, + }, + { type: "object", properties: { n: { const: 8 } } }, + ], + }, + { __variant: "0", __b0__n: "tampered", __b0__a: "x" }, + ), + ).toEqual({ n: 7, a: "x" }); + }); + + it("keeps a base argument whose name looks generated", () => { + const schema = { + type: "object", + properties: { __b0__x: { type: "string" } }, + anyOf: [ + { type: "object", properties: { a: { type: "string" } } }, + { type: "object", properties: { b: { type: "string" } } }, + ], + }; + expect( + decodeFormValues(schema, { + __variant: "0", + __b0__x: "mine", + __b_0__a: "chosen", + }), + ).toEqual({ __b0__x: "mine", a: "chosen" }); + }); }); it("merges a root allOf into the parameters section", () => { diff --git a/clients/tui/src/utils/schemaToForm.ts b/clients/tui/src/utils/schemaToForm.ts index 2558dc31..99cff40e 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -80,11 +80,46 @@ interface JsonSchemaObject { * submit the second branch's discriminator. Prefixing each branch's fields and * choosing between them explicitly is what makes the alternatives independent. */ -export const VARIANT_FIELD = "__variant"; +/** + * The generated field names a root-union form uses, chosen so they cannot + * collide with a property the schema itself declares. + * + * JSON object property names have no reserved namespace: a server may declare + * an argument called `__variant`, or one starting with `__b`. A fixed prefix + * would then either be shadowed by that argument or would silently swallow it + * on the way out, so both names are extended with `_` until nothing declared + * can be confused with them. Derived from the schema alone, so + * {@link schemaToForm} and {@link decodeFormValues} compute the same names + * without passing anything between them. + */ +function generatedNames( + base: { properties?: Record }, + branches: { declaredFields: string[] }[], +): { variant: string; prefix: string } { + const declared = [ + ...Object.keys(base.properties ?? {}), + ...branches.flatMap((branch) => branch.declaredFields), + ]; + let variant = "__variant"; + while (declared.includes(variant)) variant += "_"; + let prefix = "__b"; + while (declared.some((name) => name.startsWith(prefix))) prefix += "_"; + return { variant, prefix }; +} /** The form-local name a branch's field is rendered under. */ -function branchFieldName(branchIndex: number, name: string): string { - return `__b${branchIndex}__${name}`; +function branchFieldName( + prefix: string, + branchIndex: number, + name: string, +): string { + return `${prefix}${branchIndex}__${name}`; +} + +/** The `const` a property schema pins its value to, if any. */ +function constOf(schema: unknown): unknown { + if (typeof schema !== "object" || schema === null) return undefined; + return (schema as { const?: unknown }).const; } /** @@ -105,14 +140,35 @@ export function schemaToForm( // could only be called with empty arguments. const { base, branches } = resolveRootUnion(schema); - const parameters = buildFields(base); - if (branches.length > 0) { - // ink-form is static, so there is no picker that can swap the fields out. - // Every branch is rendered instead, and this select says which one the - // call means — read back by {@link decodeFormValues}, which drops the rest. - parameters.unshift({ + if (branches.length === 0) { + return { + title, + sections: [{ title: "Parameters", fields: buildFields(base) }], + }; + } + + const { variant, prefix } = generatedNames(base, branches); + const branchDeclared = new Set( + branches.flatMap((branch) => branch.declaredFields), + ); + + // The base section renders what the base *alone* declares. A property a + // branch also declares is rendered in that branch's section instead, showing + // the branch's specialization — root `count: {}` under branch + // `count: { type: "number" }` is a number field there, not a string here. + const baseProperties = Object.fromEntries( + Object.entries(base.properties ?? {}).filter( + ([name]) => !branchDeclared.has(name), + ), + ); + + // ink-form is static, so there is no picker that can swap the fields out. + // Every branch is rendered instead, and this select says which one the call + // means — read back by `decodeFormValues`, which drops the rest. + const parameters: FormField[] = [ + { type: "select", - name: VARIANT_FIELD, + name: variant, label: "Variant", required: true, initialValue: "0", @@ -120,8 +176,9 @@ export function schemaToForm( label: branch.label, value: String(index), })), - } as FormField); - } + } as FormField, + ...buildFields({ properties: baseProperties, required: base.required }), + ]; const sections: FormSection[] = [{ title: "Parameters", fields: parameters }]; @@ -129,15 +186,15 @@ export function schemaToForm( // says: only one alternative applies to a call, so requiring them would build // a form that can never be submitted. branches.forEach((branch, index) => { - const ownProperties = Object.fromEntries( - branch.ownFields.map((name) => [ - branchFieldName(index, name), + const properties = Object.fromEntries( + branch.declaredFields.map((name) => [ + branchFieldName(prefix, index, name), branch.schema.properties?.[name], ]), ); sections.push({ title: branch.label, - fields: buildFields({ properties: ownProperties }), + fields: buildFields({ properties }), }); }); @@ -146,45 +203,75 @@ export function schemaToForm( /** * Turn what the form submitted back into the arguments the server expects: - * the base fields, plus the fields of the branch the {@link VARIANT_FIELD} - * select names, under their real property names. + * the base fields, plus the fields of the branch the variant select names, + * under their real property names. * * Every other branch's fields are dropped rather than sent — they describe a - * shape this call is not making, and the user filled at most one section. Call - * this on the way out of the form; for a schema with no root union it returns - * the values unchanged, so it is safe to apply unconditionally. + * shape this call is not making, and the user filled at most one section. The + * generated names are filtered by exact match rather than by prefix, so an + * argument the server really named `__b0__x` survives. + * + * A `const` is re-applied from the schema rather than taken from the form. + * ink-form has no immutable field, so a discriminator is rendered as a + * one-option select and its value is restored here regardless — which also + * keeps a non-string constant's type, since a select hands back a string. + * + * Call this on the way out of the form; for a schema with no root union it + * returns the values unchanged, so it is safe to apply unconditionally. */ export function decodeFormValues( schema: JsonSchemaObject | null | undefined, values: Record, ): Record { - const { branches } = resolveRootUnion(schema ?? {}); + const { base, branches } = resolveRootUnion(schema ?? {}); if (branches.length === 0) { - return values; + return applyConstants(base.properties ?? {}, values); } - const raw = values[VARIANT_FIELD]; - const selected = Number(raw); + const { variant, prefix } = generatedNames(base, branches); + const selected = Number(values[variant]); const branchIndex = Number.isInteger(selected) && selected >= 0 && selected < branches.length ? selected : 0; + const branch = branches[branchIndex]!; + + const generated = new Set([variant]); + branches.forEach((each, index) => { + for (const name of each.declaredFields) { + generated.add(branchFieldName(prefix, index, name)); + } + }); const decoded: Record = {}; for (const [name, value] of Object.entries(values)) { - // Skip the select itself and every branch's prefixed field; the chosen - // branch's are re-added below under the names the schema declares. - if (name !== VARIANT_FIELD && !name.startsWith("__b")) { + if (!generated.has(name)) { decoded[name] = value; } } - for (const name of branches[branchIndex]!.ownFields) { - const value = values[branchFieldName(branchIndex, name)]; + for (const name of branch.declaredFields) { + const value = values[branchFieldName(prefix, branchIndex, name)]; if (value !== undefined) { decoded[name] = value; } } - return decoded; + return applyConstants(branch.schema.properties ?? {}, decoded); +} + +/** Overwrite every `const`-pinned field with the value its schema fixes. */ +function applyConstants( + properties: Record, + values: Record, +): Record { + const pinned = Object.entries(properties).filter( + ([, schema]) => constOf(schema) !== undefined, + ); + if (pinned.length === 0) return values; + const result = { ...values }; + for (const [name, schema] of pinned) { + result[name] = constOf(schema) as T; + } + return result; } /** Build the ink-form fields for one already-flattened object schema. */ @@ -215,6 +302,21 @@ function buildFields(schema: JsonSchemaObject): FormField[] { let field: FormField; + // A `const` admits exactly one value, and ink-form has no read-only field — + // so it is rendered as a select with that single option, which the user + // cannot change (#2123). `decodeFormValues` restores the schema's own typed + // value on submit, since a select hands back a string. + const pinned = property.const; + if (pinned !== undefined) { + fields.push({ + type: "select", + ...baseField, + initialValue: String(pinned), + options: [{ label: String(pinned), value: String(pinned) }], + } as FormField); + continue; + } + // Handle enum -> select. Detect the array-of-enums case on `items.enum` // alone (matching the web SchemaForm guard) — a standard array-of-enums // schema carries no top-level `enum`, so gating on it would drop the field @@ -279,17 +381,11 @@ function buildFields(schema: JsonSchemaObject): FormField[] { } // Set initial value from default (ink-form FormField allows initialValue for some types). - // A `const` is seeded the same way and OUTRANKS `default`: it is a - // one-value enumeration, so the only submittable value is already known and - // the user would otherwise have to hand-type a union's discriminator - // (#2123), while `default` is an annotation a schema may set to something - // its own `const` rejects. Tested against `undefined` rather than `??` - // chained, so an explicit `null` default is honored as a value. - const initialValue = - property.const !== undefined ? property.const : property.default; - if (initialValue !== undefined) { + // A `const` never reaches here — it was rendered as its own one-option + // select above, which is also why `default` needs no precedence rule. + if (property.default !== undefined) { (field as FormField & { initialValue?: unknown }).initialValue = - initialValue; + property.default; } fields.push(field); diff --git a/clients/web/src/test/core/jsonUtils.test.ts b/clients/web/src/test/core/jsonUtils.test.ts index 522c0bbd..fea4a725 100644 --- a/clients/web/src/test/core/jsonUtils.test.ts +++ b/clients/web/src/test/core/jsonUtils.test.ts @@ -123,6 +123,152 @@ describe("JSON Utils", () => { }); }); + it("picks the branch its discriminator names (#2123)", () => { + const discriminated: Tool = { + name: "discriminated", + inputSchema: { + type: "object", + oneOf: [ + { + type: "object", + properties: { + kind: { type: "string", const: "a" }, + value: { type: "number" }, + }, + }, + { + type: "object", + properties: { + kind: { type: "string", const: "b" }, + value: { type: "boolean" }, + }, + }, + ], + }, + }; + // `value` is a number in one branch and a boolean in the other, so the + // discriminator is the only thing that says how to coerce it. + expect( + convertToolParameters(discriminated, { kind: "b", value: "true" }), + ).toEqual({ kind: "b", value: true }); + expect( + convertToolParameters(discriminated, { kind: "a", value: "3" }), + ).toEqual({ kind: "a", value: 3 }); + }); + + it("leaves an ambiguously typed argument uncoerced (#2123)", () => { + const ambiguous: Tool = { + name: "ambiguous", + inputSchema: { + type: "object", + anyOf: [ + { + type: "object", + properties: { + value: { type: "number" }, + a: { type: "string" }, + }, + }, + { + type: "object", + properties: { + value: { type: "boolean" }, + b: { type: "string" }, + }, + }, + ], + }, + }; + // Nothing identifies a branch, so coercing `value` by an arbitrary one + // would turn `true` into `Number("true")` — `NaN`, i.e. `null` on the + // wire. The raw string is honest; it is also what shipped before. + expect(convertToolParameters(ambiguous, { value: "true" })).toEqual({ + value: "true", + }); + }); + + it("agrees on an array-form type across branches (#2123)", () => { + const arrayTyped: Tool = { + name: "array-typed", + inputSchema: { + type: "object", + anyOf: [ + { + type: "object", + properties: { v: { type: ["number", "null"] }, a: {} }, + }, + { + type: "object", + properties: { v: { type: ["number", "null"] }, b: {} }, + }, + ], + }, + }; + // Both spell the type the same way, so it is not ambiguous. + expect(convertToolParameters(arrayTyped, { v: "2" })).toEqual({ v: "2" }); + }); + + it("ignores a malformed branch declaration when matching constants (#2123)", () => { + const malformed: Tool = { + name: "malformed", + inputSchema: { + type: "object", + anyOf: [ + { + type: "object", + properties: { + kind: { type: "string", const: "a" }, + broken: null as unknown, + value: { type: "number" }, + }, + }, + { + type: "object", + properties: { + kind: { type: "string", const: "b" }, + value: { type: "boolean" }, + }, + }, + ], + }, + }; + // A `properties: { broken: null }` entry must not throw, and a branch is + // still identifiable by the discriminator that *is* well-formed. + expect( + convertToolParameters(malformed, { kind: "a", value: "3" }), + ).toEqual({ kind: "a", value: 3 }); + }); + + it("falls back to the branch-agreement path when no constant is supplied (#2123)", () => { + const discriminated: Tool = { + name: "no-discriminator-supplied", + inputSchema: { + type: "object", + anyOf: [ + { + type: "object", + properties: { + kind: { type: "string", const: "a" }, + shared: { type: "number" }, + }, + }, + { + type: "object", + properties: { + kind: { type: "string", const: "b" }, + shared: { type: "number" }, + }, + }, + ], + }, + }; + // Both branches match vacuously, so no single branch is identified — but + // they agree about `shared`, so it is still coerced. + expect(convertToolParameters(discriminated, { shared: "4" })).toEqual({ + shared: 4, + }); + }); + it("coerces a value whose schema lives on a root allOf branch (#2123)", () => { const allOfTool: Tool = { name: "allof-tool", diff --git a/clients/web/src/test/core/rootUnion.test.ts b/clients/web/src/test/core/rootUnion.test.ts index 21419d51..16713bad 100644 --- a/clients/web/src/test/core/rootUnion.test.ts +++ b/clients/web/src/test/core/rootUnion.test.ts @@ -50,8 +50,8 @@ describe("resolveRootUnion", () => { "address", ]); expect(branches[0].schema.required).toEqual(["note", "kind", "address"]); - expect(branches[0].ownFields).toEqual(["kind", "address"]); - expect(branches[1].ownFields).toEqual(["kind", "phone"]); + expect(branches[0].declaredFields).toEqual(["kind", "address"]); + expect(branches[1].declaredFields).toEqual(["kind", "phone"]); }); it("strips the composition keywords it has absorbed", () => { @@ -106,16 +106,40 @@ describe("resolveRootUnion", () => { ); }); - it("lets a branch's declaration win a name collision with the root", () => { + it("merges a name collision rather than replacing the root's declaration", () => { + // Both apply, so the root's floor must survive the branch's ceiling. const { branches } = resolveRootUnion({ type: "object", - properties: { id: { type: "string" } }, + properties: { id: { type: "number", minimum: 0 } }, required: ["id"], - anyOf: [{ type: "object", properties: { id: { type: "number" } } }, SMS], + anyOf: [ + { type: "object", properties: { id: { maximum: 10 } } }, + { type: "object", properties: { other: { type: "string" } } }, + ], + }); + expect(branches[0].schema.properties?.id).toEqual({ + type: "number", + minimum: 0, + maximum: 10, }); - expect(branches[0].schema.properties?.id).toEqual({ type: "number" }); // `required` unions rather than duplicating. expect(branches[0].schema.required).toEqual(["id"]); + // The branch declares the name, even though the base did too. + expect(branches[0].declaredFields).toEqual(["id"]); + }); + + it("declines a union whose branch contradicts the root's type for a field", () => { + // `string` under a base `number` describes a value that cannot exist, so + // flattening it would render one type and accept what the schema rejects. + const { branches } = resolveRootUnion({ + type: "object", + properties: { id: { type: "number" } }, + anyOf: [ + { type: "object", properties: { id: { type: "string" } } }, + { type: "object", properties: { other: { type: "string" } } }, + ], + }); + expect(branches).toEqual([]); }); describe("branch labels", () => { @@ -221,6 +245,26 @@ describe("resolveRootUnion", () => { expect(branches).toEqual([]); }); + it("declines a member whose type rules objects out", () => { + // Tool arguments are a JSON object, so a `{ type: "string" }` member can + // never match — a fillable form for it would offer an invalid call. + const { branches } = resolveRootUnion({ + type: "object", + anyOf: [EMAIL, { type: "string", properties: { a: {} } }], + }); + expect(branches).toEqual([]); + }); + + it("declines a member whose properties are not an object", () => { + // Members arrive as `unknown`, so this is reachable and must not throw. + expect( + resolveRootUnion({ + type: "object", + anyOf: [EMAIL, { type: "object", properties: null as unknown }], + }).branches, + ).toEqual([]); + }); + it("declines an empty union", () => { expect(resolveRootUnion({ type: "object", anyOf: [] }).branches).toEqual( [], diff --git a/core/json/jsonUtils.ts b/core/json/jsonUtils.ts index da9c51a2..27ed6690 100644 --- a/core/json/jsonUtils.ts +++ b/core/json/jsonUtils.ts @@ -125,6 +125,94 @@ export function convertParameterValue( return value; } +/** + * Which property schema types each supplied argument, for a tool whose + * `inputSchema` puts its fields on root composition branches (#2123). + * + * Reading only the root's `properties` finds no schema for any of them, so + * every value would be sent as the string the user typed — `--tool-arg count=3` + * reaching the server as `"3"`. + * + * The CLI has no branch picker, so which branch a call means is *inferred*: + * a discriminated union pins its discriminator with `const`, and the supplied + * arguments either match one branch's constants or they do not. + * + * - **Exactly one branch matches** — use its merged schema, which is also where + * a branch's specialization of a root-declared property lives. + * - **No branch is identifiable** — coerce only the names every branch that + * declares them types the *same* way. A name two branches type differently + * is left uncoerced rather than coerced by an arbitrary branch: `value` as a + * number in branch 0 and a boolean in branch 1 would otherwise turn + * `value=true` into `Number("true")`, i.e. `NaN`. Passing the raw string + * through is what this function did for every argument before it existed. + */ +function coercionProperties( + base: { properties?: Record }, + branches: { + schema: { properties?: Record }; + declaredFields: string[]; + }[], + params: Record, +): Record { + if (branches.length === 0) { + return { ...base.properties }; + } + + const matching = branches.filter((branch) => + matchesConstants(branch.schema.properties ?? {}, params), + ); + if (matching.length === 1) { + return { ...matching[0].schema.properties }; + } + + const properties: Record = { ...base.properties }; + for (const name of new Set(branches.flatMap((b) => b.declaredFields))) { + // Only the branches that *declare* the name have an opinion about it — a + // branch that merely inherited the root's declaration is not a second, + // disagreeing vote. Read through the merged schema so a branch's + // specialization of a root property carries the root's keywords too. + const declarations = branches + .filter((branch) => branch.declaredFields.includes(name)) + .map((branch) => branch.schema.properties?.[name]); + const types = new Set(declarations.map((schema) => typeNameOf(schema))); + if (types.size === 1) { + properties[name] = declarations[0]; + } else { + delete properties[name]; + } + } + return properties; +} + +/** A schema's `type`, as a comparable string (`""` when it states none). */ +function typeNameOf(schema: unknown): string { + if (typeof schema !== "object" || schema === null) return ""; + const { type } = schema as { type?: unknown }; + return Array.isArray(type) + ? type.join(",") + : typeof type === "string" + ? type + : ""; +} + +/** + * Whether every `const`-pinned property of a branch agrees with what was + * supplied. Values arrive as strings, so the comparison is stringified — which + * is exactly right for a discriminator, whose constants are string literals. + */ +function matchesConstants( + properties: Record, + params: Record, +): boolean { + return Object.entries(properties).every(([name, schema]) => { + if (typeof schema !== "object" || schema === null) return true; + const constValue = (schema as { const?: unknown }).const; + if (constValue === undefined) return true; + const supplied = params[name]; + return supplied === undefined || supplied === String(constValue); + }); +} + /** * Convert string parameters to JSON values based on tool schema */ @@ -134,33 +222,10 @@ export function convertToolParameters( ): Record { const result: Record = {}; // A property's schema can live on a root composition branch rather than on - // the root itself (#2123). Reading only the root's `properties` there finds - // no schema for any argument, so every value would be sent as the string the - // user typed — `--tool-arg count=3` reaching the server as `"3"`. The union - // is flattened by merging every branch, because the CLI has no branch - // selection to consult: an argument named by one branch is coerced by that - // branch's schema, and a name two branches type differently keeps the first, - // which is no worse than the untyped passthrough it replaces. + // the root itself (#2123); see `coercionProperties` for how the branch is + // identified when it does. const { base, branches } = resolveRootUnion(tool.inputSchema ?? {}); - // Start from the FIRST branch's merged schema rather than from the base: a - // branch may *specialize* a property the root also declares (root - // `count: {}`, branch `count: { type: "number" }`), and merging branch-last - // is what gives the typed declaration — starting from the base would keep the - // untyped one and send `count=3` as `"3"`, the very coercion this restores. - // Later branches then contribute only names not seen yet, so first-branch - // precedence matches what the web form seeds and what a name two branches - // type differently resolves to. - const properties: Record = { - ...(branches[0]?.schema.properties ?? base.properties), - }; - for (const branch of branches.slice(1)) { - for (const [name, schema] of Object.entries( - branch.schema.properties ?? {}, - )) { - properties[name] ??= schema; - } - } - + const properties = coercionProperties(base, branches, params); for (const [key, value] of Object.entries(params)) { const paramSchema = properties[key] as ParameterSchema | undefined; diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index 88f4c008..dd731907 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -64,8 +64,13 @@ export interface RootUnionBranch { schema: ResolvedSchema; /** Human-readable name for the picker — see {@link branchLabel}. */ label: string; - /** Names this branch contributes that the base does not. */ - ownFields: string[]; + /** + * The names **this branch itself declares** — including one the base also + * declares, since a branch commonly *specializes* a root property (root + * `count: {}`, branch `count: { type: "number" }`) and a renderer that showed + * only the base's version would render the untyped one. + */ + declaredFields: string[]; } /** What {@link resolveRootUnion} decomposes a root schema into. */ @@ -92,34 +97,120 @@ function toBranch(value: unknown): RootUnionSchema | null { return value as RootUnionSchema; } +/** Whether a schema's `type` permits an object instance. */ +function admitsObject(schema: RootUnionSchema): boolean { + const { type } = schema; + if (type === undefined) return true; + return Array.isArray(type) ? type.includes("object") : type === "object"; +} + +/** A readable `properties` map, or `null` when the value is not one. */ +function propertiesOf(schema: RootUnionSchema): Record | null { + const { properties } = schema; + if ( + typeof properties !== "object" || + properties === null || + Array.isArray(properties) + ) { + return null; + } + return properties; +} + /** - * Whether a branch contributes anything a form can render. + * Whether a branch is one a form can offer as an alternative. * - * A `{ type: "null" }` member — the nullable encoding {@link - * ./nullableUnion.ts} owns — and a `$ref`-only or empty branch carry no - * properties, so offering them as alternatives would produce a picker whose - * options render nothing. + * Three ways it is not, all of which would put an option in the picker that + * cannot be filled in: + * + * - **It carries no fields.** A `{ type: "null" }` member — the nullable + * encoding {@link ./nullableUnion.ts} owns — and a `$ref`-only or empty + * branch render nothing. + * - **`properties` is not an object.** Members arrive as `unknown`, so a + * malformed `properties: null` is reachable and would throw in `Object.keys` + * rather than being declined. + * - **Its `type` rules objects out.** Tool arguments are a JSON object, so a + * `{ type: "string", properties: {…} }` member can never match — rendering it + * as a fillable form would offer a call that cannot be valid. */ -function hasFields(branch: RootUnionSchema): boolean { +function isOfferable(branch: RootUnionSchema): boolean { + const properties = propertiesOf(branch); return ( - branch.properties !== undefined && Object.keys(branch.properties).length > 0 + properties !== null && + Object.keys(properties).length > 0 && + admitsObject(branch) ); } +/** + * Merge two declarations of the same property name. + * + * Where a base and a branch both name a property, JSON Schema applies **both** + * — so keeping only the branch's would silently drop the base's constraints + * (root `{ minimum: 0 }` plus branch `{ maximum: 10 }` must keep the floor). + * A shallow union preserves every keyword only one side states, with the + * branch winning a keyword both state, being the more specific declaration. + * + * A *contradiction* is not resolvable this way and is not resolved here — see + * {@link contradicts}, which declines such a union outright. + */ +function mergeProperty( + baseProperty: unknown, + branchProperty: unknown, +): unknown { + const a = toBranch(baseProperty); + const b = toBranch(branchProperty); + if (a === null || b === null) { + return branchProperty; + } + return { ...a, ...b }; +} + +/** + * Whether a branch states a property `type` its base declaration rules out. + * + * The two are conjunctive, so `string` under a base `number` describes a value + * that cannot exist. No form can express that, and flattening it would render + * one of the two types and accept values the schema rejects — so the caller + * declines the union instead of picking a side. + */ +function contradicts(base: RootUnionSchema, branch: RootUnionSchema): boolean { + const baseProperties = propertiesOf(base) ?? {}; + const branchProperties = propertiesOf(branch) ?? {}; + return Object.entries(branchProperties).some(([name, branchProperty]) => { + const a = toBranch(baseProperties[name]); + const b = toBranch(branchProperty); + if (a === null || b === null) return false; + return ( + typeof a.type === "string" && + typeof b.type === "string" && + a.type !== b.type + ); + }); +} + /** * Merge a composition branch's `properties` and `required` into a base schema. * * Both keywords are **conjunctive** where they meet: a value satisfying an * `allOf` branch satisfies the base *and* the branch, and a value matching a * union branch must satisfy the root's own constraints too. So properties union - * (branch wins a name collision, being the more specific declaration) and - * `required` unions. + * — a name both declare merged through {@link mergeProperty} rather than + * replaced — and `required` unions. */ function mergeBranch( base: T, branch: RootUnionSchema, ): ResolvedSchema { - const properties = { ...base.properties, ...branch.properties }; + const baseProperties = propertiesOf(base) ?? {}; + const branchProperties = propertiesOf(branch) ?? {}; + const properties: Record = { ...baseProperties }; + for (const [name, branchProperty] of Object.entries(branchProperties)) { + properties[name] = + name in baseProperties + ? mergeProperty(baseProperties[name], branchProperty) + : branchProperty; + } const required = [ ...(base.required ?? []), ...(branch.required ?? []).filter( @@ -165,7 +256,7 @@ function branchLabel( if (typeof branch.title === "string" && branch.title.trim() !== "") { return branch.title; } - const properties = branch.properties ?? {}; + const properties = propertiesOf(branch) ?? {}; const constOf = (name: string): string | null => { const property = toBranch(properties[name]) as { const?: unknown } | null; const value = property?.const; @@ -224,13 +315,15 @@ export function resolveRootUnion( const branches = members.map(toBranch); if ( branches.length === 0 || - branches.some((branch) => branch === null || !hasFields(branch)) + branches.some( + (branch) => + branch === null || !isOfferable(branch) || contradicts(base, branch), + ) ) { return { base, branches: [] }; } const discriminatorProperty = schema.discriminator?.propertyName; - const baseFields = new Set(Object.keys(base.properties ?? {})); return { base, branches: branches @@ -240,9 +333,7 @@ export function resolveRootUnion( // `properties`/`required` off the branch, so the merge stays that way. schema: mergeBranch(base, branch), label: branchLabel(branch, index, discriminatorProperty), - ownFields: Object.keys(branch.properties ?? {}).filter( - (name) => !baseFields.has(name), - ), + declaredFields: Object.keys(propertiesOf(branch) ?? {}), })), }; }