fix: address Copilot review — const precedence, TUI variant model, decline both-keyword unions
- rootUnion: decline a schema carrying BOTH oneOf and anyOf rather than reading one and silently dropping the other's constraints. - SchemaForm: handle `const` before every type dispatch and for every constant type, so a numeric/boolean constant, or a schema carrying both `const` and `enum`, can no longer offer a forbidden value. - collectSchemaDefaults / TUI initialValue: `const` outranks `default`, which is an annotation a schema may set to something its const rejects. - convertToolParameters: resolve from the first branch's merged schema, so a branch's specialization of a root-declared property is what types the arg. - TUI: ink-form keys values by field name across the whole form, so two branches' `kind` were one field and the later section's initial value decided what the earlier submitted. Branch fields now render under prefixed names behind a Variant select, and decodeFormValues translates back on submit, dropping every branch but the chosen one. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
5933162dd0
commit
2782ed75d1
3 changed files with 370 additions and 68 deletions
|
|
@ -1,5 +1,9 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import { schemaToForm } from "../src/utils/schemaToForm.js";
|
||||
import {
|
||||
decodeFormValues,
|
||||
schemaToForm,
|
||||
VARIANT_FIELD,
|
||||
} from "../src/utils/schemaToForm.js";
|
||||
|
||||
describe("schemaToForm", () => {
|
||||
it("returns an empty Parameters section when there is no schema", () => {
|
||||
|
|
@ -441,18 +445,43 @@ describe("schemaToForm", () => {
|
|||
"sms",
|
||||
]);
|
||||
expect(form.sections[0]!.fields.map((field) => field.name)).toEqual([
|
||||
VARIANT_FIELD,
|
||||
"note",
|
||||
]);
|
||||
});
|
||||
|
||||
it("names branch fields uniquely, since ink-form scopes by name alone", () => {
|
||||
// Both branches declare `kind`. Rendered under their real names they
|
||||
// would be one field, and the later section's initial value would decide
|
||||
// what the earlier section submits.
|
||||
const form = schemaToForm(UNION, "union_tool");
|
||||
expect(form.sections[1]!.fields.map((field) => field.name)).toEqual([
|
||||
"kind",
|
||||
"address",
|
||||
"__b0__kind",
|
||||
"__b0__address",
|
||||
]);
|
||||
expect(form.sections[2]!.fields.map((field) => field.name)).toEqual([
|
||||
"__b1__kind",
|
||||
"__b1__count",
|
||||
]);
|
||||
});
|
||||
|
||||
it("offers a variant select listing the alternatives", () => {
|
||||
const form = schemaToForm(UNION, "union_tool");
|
||||
expect(form.sections[0]!.fields[0]).toMatchObject({
|
||||
name: VARIANT_FIELD,
|
||||
type: "select",
|
||||
initialValue: "0",
|
||||
options: [
|
||||
{ label: "email", value: "0" },
|
||||
{ label: "sms", value: "1" },
|
||||
],
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps a branch's typed fields typed", () => {
|
||||
const form = schemaToForm(UNION, "union_tool");
|
||||
expect(form.sections[2]!.fields[1]).toMatchObject({
|
||||
name: "count",
|
||||
name: "__b1__count",
|
||||
type: "integer",
|
||||
});
|
||||
});
|
||||
|
|
@ -467,11 +496,74 @@ describe("schemaToForm", () => {
|
|||
it("seeds a branch's discriminator const so it need not be typed", () => {
|
||||
const form = schemaToForm(UNION, "union_tool");
|
||||
expect(form.sections[1]!.fields[0]).toMatchObject({
|
||||
name: "kind",
|
||||
name: "__b0__kind",
|
||||
initialValue: "email",
|
||||
});
|
||||
});
|
||||
|
||||
it("prefers a const over a conflicting default", () => {
|
||||
const form = schemaToForm(
|
||||
{
|
||||
type: "object",
|
||||
properties: { v: { type: "string", const: "a", default: "b" } },
|
||||
},
|
||||
"const_tool",
|
||||
);
|
||||
expect(form.sections[0]!.fields[0]).toMatchObject({ initialValue: "a" });
|
||||
});
|
||||
|
||||
describe("decodeFormValues", () => {
|
||||
it("submits the chosen branch's fields under their real names", () => {
|
||||
expect(
|
||||
decodeFormValues(UNION, {
|
||||
[VARIANT_FIELD]: "0",
|
||||
note: "hi",
|
||||
__b0__kind: "email",
|
||||
__b0__address: "a@b.c",
|
||||
__b1__kind: "sms",
|
||||
__b1__count: 3,
|
||||
}),
|
||||
).toEqual({ note: "hi", kind: "email", address: "a@b.c" });
|
||||
});
|
||||
|
||||
it("drops the branches the call is not making", () => {
|
||||
expect(
|
||||
decodeFormValues(UNION, {
|
||||
[VARIANT_FIELD]: "1",
|
||||
__b0__kind: "email",
|
||||
__b0__address: "a@b.c",
|
||||
__b1__kind: "sms",
|
||||
__b1__count: 3,
|
||||
}),
|
||||
).toEqual({ kind: "sms", count: 3 });
|
||||
});
|
||||
|
||||
it("omits a branch field the user never filled", () => {
|
||||
expect(
|
||||
decodeFormValues(UNION, {
|
||||
[VARIANT_FIELD]: "0",
|
||||
__b0__kind: "email",
|
||||
}),
|
||||
).toEqual({ kind: "email" });
|
||||
});
|
||||
|
||||
it("falls back to the first branch on an unusable selection", () => {
|
||||
expect(
|
||||
decodeFormValues(UNION, {
|
||||
[VARIANT_FIELD]: "nonsense",
|
||||
__b0__kind: "email",
|
||||
}),
|
||||
).toEqual({ kind: "email" });
|
||||
});
|
||||
|
||||
it("returns the values untouched for a schema with no root union", () => {
|
||||
const values = { message: "hi" };
|
||||
expect(decodeFormValues({ properties: { message: {} } }, values)).toBe(
|
||||
values,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
it("merges a root allOf into the parameters section", () => {
|
||||
const form = schemaToForm(
|
||||
{
|
||||
|
|
|
|||
|
|
@ -5,7 +5,7 @@ import { InspectorClient } from "@inspector/core/mcp/index.js";
|
|||
import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
|
||||
import type { Tool, CallToolResult } from "@modelcontextprotocol/client";
|
||||
import type { JsonValue } from "@inspector/core/mcp/index.js";
|
||||
import { schemaToForm } from "../utils/schemaToForm.js";
|
||||
import { decodeFormValues, schemaToForm } from "../utils/schemaToForm.js";
|
||||
import { ScrollView, type ScrollViewRef } from "ink-scroll-view";
|
||||
|
||||
interface ToolTestModalProps {
|
||||
|
|
@ -114,9 +114,15 @@ export function ToolTestModal({
|
|||
{ isActive: true },
|
||||
);
|
||||
|
||||
const handleFormSubmit = async (values: Record<string, JsonValue>) => {
|
||||
const handleFormSubmit = async (rawValues: Record<string, JsonValue>) => {
|
||||
if (!inspectorClient || !tool) return;
|
||||
|
||||
// A root union renders every alternative as its own section, under prefixed
|
||||
// field names, because ink-form scopes values by name across the whole form
|
||||
// (#2123). This turns them back into the arguments the server declared:
|
||||
// the base fields plus the chosen branch's, and nothing from the others.
|
||||
const values = decodeFormValues(tool.inputSchema, rawValues);
|
||||
|
||||
setState("loading");
|
||||
const startTime = Date.now();
|
||||
|
||||
|
|
|
|||
|
|
@ -70,6 +70,23 @@ interface JsonSchemaObject {
|
|||
discriminator?: { propertyName?: string };
|
||||
}
|
||||
|
||||
/**
|
||||
* The select that names which alternative of a root union the call is making.
|
||||
*
|
||||
* ink-form keeps **one** value object for the whole form, keyed by field name
|
||||
* alone — sections are visual grouping, not scope. So two branches of a
|
||||
* discriminated union both declaring `kind` are the *same* field: the later
|
||||
* section's initial value wins, and filling the first branch's section would
|
||||
* 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 form-local name a branch's field is rendered under. */
|
||||
function branchFieldName(branchIndex: number, name: string): string {
|
||||
return `__b${branchIndex}__${name}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Converts a JSON Schema to ink-form structure
|
||||
*/
|
||||
|
|
@ -88,30 +105,88 @@ export function schemaToForm(
|
|||
// could only be called with empty arguments.
|
||||
const { base, branches } = resolveRootUnion(schema);
|
||||
|
||||
const sections: FormSection[] = [
|
||||
{ title: "Parameters", fields: buildFields(base) },
|
||||
];
|
||||
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({
|
||||
type: "select",
|
||||
name: VARIANT_FIELD,
|
||||
label: "Variant",
|
||||
required: true,
|
||||
initialValue: "0",
|
||||
options: branches.map((branch, index) => ({
|
||||
label: branch.label,
|
||||
value: String(index),
|
||||
})),
|
||||
} as FormField);
|
||||
}
|
||||
|
||||
// ink-form is static — there is no branch picker to hide the alternatives
|
||||
// behind — so every branch gets its own section and the user fills the one
|
||||
// they mean. A branch's fields are rendered **optional** whatever the branch
|
||||
// says: only one alternative applies to a given call, so requiring them would
|
||||
// make a form that can never be submitted. An untouched field reports no
|
||||
// value and is dropped before the call, so the sections the user skipped
|
||||
// contribute nothing to the arguments.
|
||||
for (const branch of branches) {
|
||||
const sections: FormSection[] = [{ title: "Parameters", fields: parameters }];
|
||||
|
||||
// One section per alternative, its fields **optional** whatever the branch
|
||||
// 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) => [name, branch.schema.properties?.[name]]),
|
||||
branch.ownFields.map((name) => [
|
||||
branchFieldName(index, name),
|
||||
branch.schema.properties?.[name],
|
||||
]),
|
||||
);
|
||||
sections.push({
|
||||
title: branch.label,
|
||||
fields: buildFields({ properties: ownProperties }),
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
return { title, sections };
|
||||
}
|
||||
|
||||
/**
|
||||
* 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.
|
||||
*
|
||||
* 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.
|
||||
*/
|
||||
export function decodeFormValues<T>(
|
||||
schema: JsonSchemaObject | null | undefined,
|
||||
values: Record<string, T>,
|
||||
): Record<string, T> {
|
||||
const { branches } = resolveRootUnion(schema ?? {});
|
||||
if (branches.length === 0) {
|
||||
return values;
|
||||
}
|
||||
|
||||
const raw = values[VARIANT_FIELD];
|
||||
const selected = Number(raw);
|
||||
const branchIndex =
|
||||
Number.isInteger(selected) && selected >= 0 && selected < branches.length
|
||||
? selected
|
||||
: 0;
|
||||
|
||||
const decoded: Record<string, T> = {};
|
||||
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")) {
|
||||
decoded[name] = value;
|
||||
}
|
||||
}
|
||||
for (const name of branches[branchIndex]!.ownFields) {
|
||||
const value = values[branchFieldName(branchIndex, name)];
|
||||
if (value !== undefined) {
|
||||
decoded[name] = value;
|
||||
}
|
||||
}
|
||||
return decoded;
|
||||
}
|
||||
|
||||
/** Build the ink-form fields for one already-flattened object schema. */
|
||||
function buildFields(schema: JsonSchemaObject): FormField[] {
|
||||
const fields: FormField[] = [];
|
||||
|
|
@ -204,10 +279,14 @@ 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: 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).
|
||||
const initialValue = property.default ?? property.const;
|
||||
// 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) {
|
||||
(field as FormField & { initialValue?: unknown }).initialValue =
|
||||
initialValue;
|
||||
|
|
|
|||
Loading…
Reference in a new issue