fix(web): refuse a raw-JSON tool argument the schema would retype (#2171)
The Tools and Apps tabs' "Edit as JSON" editor sent its draft through the
string-to-declared-type conversion `callTool` applies for the widget form, so
`{"count":"01"}` against a numeric schema left as `{"count":1}` — the editor
showed one payload and the wire carried another.
Both JSON-authoring surfaces now refuse such a draft and name the value to
rewrite. Edit-and-replay already did; this makes the Tools and Apps forms
agree with it, which is the consistency #2171's acceptance asks for.
`coercedArgumentNames` and `coercedArgumentsError` move into core/json so the
two surfaces share one predicate and one sentence — the check runs the real
conversion and compares rather than restating its rules, so it cannot drift
from what would actually be sent. Enforcement is opt-in
(`SchemaFormProps.enforceToolArgumentTypes`): an elicitation renders through
the same form and its values are never converted, so enforcing there would
refuse a draft for a reason that is not true of it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
4d69dbf999
commit
9516299d8d
6 changed files with 435 additions and 27 deletions
|
|
@ -136,6 +136,29 @@ describe("AppDetailPanel", () => {
|
|||
expect(screen.getByRole("button", { name: /open app/i })).toBeDisabled();
|
||||
});
|
||||
|
||||
// #2171: an App's arguments are a `tools/call` like any other, so its form
|
||||
// opts into the same enforcement the Tools tab uses.
|
||||
it("refuses a raw-JSON argument the schema would retype", async () => {
|
||||
const user = userEvent.setup();
|
||||
const numericTool: Tool = {
|
||||
name: "chart",
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { count: { type: "number" } },
|
||||
},
|
||||
};
|
||||
renderWithMantine(
|
||||
<AppDetailPanel {...baseProps} tool={numericTool} formValues={{}} />,
|
||||
);
|
||||
await user.click(screen.getByLabelText("Edit as JSON"));
|
||||
await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}');
|
||||
|
||||
expect(
|
||||
screen.getByText(/`count` would be converted to the type/),
|
||||
).toBeInTheDocument();
|
||||
expect(screen.getByRole("button", { name: /open app/i })).toBeDisabled();
|
||||
});
|
||||
|
||||
it("invokes onOpenApp when the button is clicked", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onOpenApp = vi.fn();
|
||||
|
|
|
|||
|
|
@ -89,6 +89,10 @@ export function AppDetailPanel({
|
|||
// SchemaFormProps.resetKey.
|
||||
resetKey={tool.name}
|
||||
onValidityChange={setHasInvalidDraft}
|
||||
// These values become `tools/call` arguments, so a raw-JSON draft the
|
||||
// client would retype is refused rather than sent as something other
|
||||
// than what the editor shows (#2171).
|
||||
enforceToolArgumentTypes
|
||||
/>
|
||||
|
||||
<OpenAppButton
|
||||
|
|
|
|||
|
|
@ -1834,11 +1834,13 @@ describe("SchemaForm raw JSON (#2151)", () => {
|
|||
function RawHarness({
|
||||
initial = {},
|
||||
onValidityChange,
|
||||
enforceToolArgumentTypes,
|
||||
resetKey,
|
||||
schema: override,
|
||||
}: {
|
||||
initial?: Record<string, unknown>;
|
||||
onValidityChange?: (hasInvalidDraft: boolean) => void;
|
||||
enforceToolArgumentTypes?: boolean;
|
||||
resetKey?: string;
|
||||
schema?: InspectorFormSchema;
|
||||
}) {
|
||||
|
|
@ -1850,6 +1852,7 @@ describe("SchemaForm raw JSON (#2151)", () => {
|
|||
onChange={setValues}
|
||||
resetKey={resetKey}
|
||||
onValidityChange={onValidityChange}
|
||||
enforceToolArgumentTypes={enforceToolArgumentTypes}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
|
@ -1858,6 +1861,87 @@ describe("SchemaForm raw JSON (#2151)", () => {
|
|||
await user.click(screen.getByLabelText("Edit as JSON"));
|
||||
}
|
||||
|
||||
// #2171. The widgets hand every value over as text, so `callTool` retypes a
|
||||
// string to whatever the schema declares — which for a JSON draft means the
|
||||
// wire would carry something other than what the editor showed. The draft is
|
||||
// refused instead, and the value to rewrite is named.
|
||||
describe("tool-argument type enforcement", () => {
|
||||
it("refuses a draft the client would retype, and says which value", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onChange = vi.fn();
|
||||
renderWithMantine(
|
||||
<SchemaForm
|
||||
schema={schema}
|
||||
values={{}}
|
||||
onChange={onChange}
|
||||
enforceToolArgumentTypes
|
||||
/>,
|
||||
);
|
||||
await enableRawJson(user);
|
||||
await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}');
|
||||
|
||||
expect(
|
||||
screen.getByText(/`count` would be converted to the type/),
|
||||
).toBeInTheDocument();
|
||||
// Not merely flagged: the value must not reach the parent either, or a
|
||||
// submit gated on something else would still send it.
|
||||
expect(onChange).not.toHaveBeenCalledWith({ count: "01" });
|
||||
});
|
||||
|
||||
it("blocks submission while such a draft is held", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onValidityChange = vi.fn();
|
||||
renderWithMantine(
|
||||
<RawHarness
|
||||
onValidityChange={onValidityChange}
|
||||
enforceToolArgumentTypes
|
||||
/>,
|
||||
);
|
||||
await enableRawJson(user);
|
||||
await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}');
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(true);
|
||||
|
||||
// Rewritten with the declared type, it is sendable again.
|
||||
await setAceTextByLabel(/Arguments JSON/, '{"count":1}');
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(false);
|
||||
});
|
||||
|
||||
// The conversion only ever looks at strings, so a value already written
|
||||
// with its declared type is not a mismatch — and neither is a string in a
|
||||
// field the schema declares as one.
|
||||
it("accepts values already written with the declared type", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onChange = vi.fn();
|
||||
renderWithMantine(
|
||||
<SchemaForm
|
||||
schema={schema}
|
||||
values={{}}
|
||||
onChange={onChange}
|
||||
enforceToolArgumentTypes
|
||||
/>,
|
||||
);
|
||||
await enableRawJson(user);
|
||||
await setAceTextByLabel(/Arguments JSON/, '{"name":"a","count":2}');
|
||||
expect(screen.queryByText(/would be converted/)).toBeNull();
|
||||
expect(onChange).toHaveBeenLastCalledWith({ name: "a", count: 2 });
|
||||
});
|
||||
|
||||
// Off by default, and that default is load-bearing: an elicitation renders
|
||||
// through this same form and its values are never converted, so enforcing
|
||||
// there would refuse a draft for a reason that is not true of it.
|
||||
it("does not enforce when the caller has not opted in", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onChange = vi.fn();
|
||||
renderWithMantine(
|
||||
<SchemaForm schema={schema} values={{}} onChange={onChange} />,
|
||||
);
|
||||
await enableRawJson(user);
|
||||
await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}');
|
||||
expect(screen.queryByText(/would be converted/)).toBeNull();
|
||||
expect(onChange).toHaveBeenLastCalledWith({ count: "01" });
|
||||
});
|
||||
});
|
||||
|
||||
it("replaces the widgets with one editor holding the current values", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<RawHarness initial={{ name: "a", count: 2 }} />);
|
||||
|
|
|
|||
|
|
@ -33,6 +33,10 @@ import {
|
|||
} from "../../../utils/jsonObjectDraft";
|
||||
import { isSerializableJson } from "@inspector/core/json/jsonUtils.js";
|
||||
import { useValueChange } from "../../../hooks/useValueChange";
|
||||
import {
|
||||
coercedArgumentNames,
|
||||
coercedArgumentsError,
|
||||
} from "@inspector/core/json/jsonUtils.js";
|
||||
import type {
|
||||
InspectorFormSchema,
|
||||
JsonSchemaConst,
|
||||
|
|
@ -626,6 +630,7 @@ function SchemaNumberInput({
|
|||
*/
|
||||
function parseRawArgumentsDraft(
|
||||
text: string,
|
||||
coercionSchema?: InspectorFormSchema,
|
||||
): { ok: true; value: Record<string, unknown> } | { ok: false; error: string } {
|
||||
if (text.trim() === "") return { ok: true, value: {} };
|
||||
let parsed: unknown;
|
||||
|
|
@ -661,6 +666,21 @@ function parseRawArgumentsDraft(
|
|||
if (duplicate !== null) {
|
||||
return { ok: false, error: duplicateKeyError(duplicate) };
|
||||
}
|
||||
// Last, because it is the only check that needs a *valid* object to run
|
||||
// against — and the only one that is about the schema rather than the JSON.
|
||||
// `callTool` retypes every string-valued argument to what the schema
|
||||
// declares, so a draft it would touch is one whose visible text is not what
|
||||
// the wire would carry; refuse it here instead, and say which value to
|
||||
// rewrite (#2171). Skipped entirely when no schema is supplied: only the
|
||||
// Tools and Apps forms feed `tools/call`, and an elicitation's values are
|
||||
// never converted, so imposing this there would block a submission for a
|
||||
// reason that is not true of it.
|
||||
if (coercionSchema) {
|
||||
const coerced = coercedArgumentNames(coercionSchema, parsed);
|
||||
if (coerced.length > 0) {
|
||||
return { ok: false, error: coercedArgumentsError(coerced) };
|
||||
}
|
||||
}
|
||||
return { ok: true, value: parsed };
|
||||
}
|
||||
|
||||
|
|
@ -670,6 +690,12 @@ interface RawArgumentsFieldProps {
|
|||
disabled: boolean;
|
||||
/** Mirrors {@link SchemaFormProps.onValidityChange} for this one editor. */
|
||||
onInvalidChange: (hasInvalidDraft: boolean) => void;
|
||||
/**
|
||||
* The tool `inputSchema` this draft's values will be sent against, when they
|
||||
* will be — see {@link SchemaFormProps.enforceToolArgumentTypes}. Undefined
|
||||
* turns the type check off.
|
||||
*/
|
||||
coercionSchema?: InspectorFormSchema;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -697,6 +723,7 @@ function RawArgumentsField({
|
|||
onChange,
|
||||
disabled,
|
||||
onInvalidChange,
|
||||
coercionSchema,
|
||||
}: RawArgumentsFieldProps) {
|
||||
const [draft, setDraft] = useState(() => serializeJson(values));
|
||||
// Canonical JSON of the last object this editor emitted, so the re-sync below
|
||||
|
|
@ -705,7 +732,7 @@ function RawArgumentsField({
|
|||
// change would then look external. See `JsonObjectInput` for the long form.
|
||||
const [echoed, setEchoed] = useState(() => serializeJson(values));
|
||||
|
||||
const parsed = parseRawArgumentsDraft(draft);
|
||||
const parsed = parseRawArgumentsDraft(draft, coercionSchema);
|
||||
|
||||
useValueChange(serializeJson(values), (next) => {
|
||||
if (next === echoed) return;
|
||||
|
|
@ -732,7 +759,7 @@ function RawArgumentsField({
|
|||
maxLines={24}
|
||||
onChange={(text) => {
|
||||
setDraft(text);
|
||||
const next = parseRawArgumentsDraft(text);
|
||||
const next = parseRawArgumentsDraft(text, coercionSchema);
|
||||
if (!next.ok) return;
|
||||
setEchoed(serializeJson(next.value));
|
||||
onChange(next.value);
|
||||
|
|
@ -790,6 +817,30 @@ export interface SchemaFormProps {
|
|||
* inside the first the moment both were on.
|
||||
*/
|
||||
allowRawJson?: boolean;
|
||||
/**
|
||||
* Whether these values become `tools/call` arguments, and so are subject to
|
||||
* the string-to-declared-type conversion `InspectorClient.callTool` applies
|
||||
* (#2171).
|
||||
*
|
||||
* That conversion exists for the widgets, which hand every value over as
|
||||
* text: `"2"` against a numeric field has to become `2`. A **raw-JSON**
|
||||
* draft already carries its own types, so a value the conversion would touch
|
||||
* is one whose visible text is not what the wire would carry — and showing
|
||||
* one payload while sending another is the one thing an inspector must not
|
||||
* do. With this on, the raw editor refuses such a draft and names the value
|
||||
* to rewrite, exactly as the Edit-and-replay modal does for the same
|
||||
* conversion.
|
||||
*
|
||||
* Off by default, and deliberately opt-in rather than inferred from the
|
||||
* schema: an elicitation renders through this same form and its values are
|
||||
* never converted, so the check would be refusing those drafts for a reason
|
||||
* that is not true of them. Only the Tools and Apps panels pass it.
|
||||
*
|
||||
* Read against {@link SchemaFormProps.schema} — the tool's own `inputSchema`,
|
||||
* root composition included, which is what the client resolves the
|
||||
* conversion against too.
|
||||
*/
|
||||
enforceToolArgumentTypes?: boolean;
|
||||
}
|
||||
|
||||
function getDefaultValue(fieldSchema: InspectorFormSchema): unknown {
|
||||
|
|
@ -817,6 +868,7 @@ export function SchemaForm({
|
|||
resetKey,
|
||||
onValidityChange,
|
||||
allowRawJson = true,
|
||||
enforceToolArgumentTypes = false,
|
||||
}: SchemaFormProps) {
|
||||
// Composition at the root of the schema, flattened before anything is
|
||||
// rendered (#2123). `allOf` is folded into `base`; a top-level `oneOf`/`anyOf`
|
||||
|
|
@ -1524,6 +1576,11 @@ export function SchemaForm({
|
|||
onChange={handleRawArgumentsChange}
|
||||
disabled={disabled}
|
||||
onInvalidChange={reportRawJsonValidity}
|
||||
// The schema as the CLIENT resolves it — the whole `inputSchema`,
|
||||
// root composition and all — not the branch this form happens to be
|
||||
// rendering. `coercedArgumentNames` does its own branch selection
|
||||
// from the supplied values, the same way the conversion does.
|
||||
coercionSchema={enforceToolArgumentTypes ? schema : undefined}
|
||||
/>
|
||||
) : (
|
||||
Object.entries(properties).map(([fieldName, fieldSchema]) =>
|
||||
|
|
|
|||
|
|
@ -246,6 +246,30 @@ describe("ToolDetailPanel", () => {
|
|||
expect(onExecute).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
// #2171: the panel opts its form into tool-argument type enforcement, so a
|
||||
// raw-JSON draft the client would retype is refused here rather than sent as
|
||||
// something other than what the editor showed.
|
||||
it("refuses a raw-JSON argument the schema would retype", async () => {
|
||||
const user = userEvent.setup();
|
||||
const numericTool: Tool = {
|
||||
name: "add",
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { count: { type: "number" } },
|
||||
},
|
||||
};
|
||||
renderWithMantine(
|
||||
<ToolDetailPanel {...baseProps} tool={numericTool} formValues={{}} />,
|
||||
);
|
||||
await user.click(screen.getByLabelText("Edit as JSON"));
|
||||
await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}');
|
||||
|
||||
expect(
|
||||
screen.getByText(/`count` would be converted to the type/),
|
||||
).toBeInTheDocument();
|
||||
expect(screen.getByRole("button", { name: "Execute Tool" })).toBeDisabled();
|
||||
});
|
||||
|
||||
it("disables the Execute Tool button while executing and renders Cancel", () => {
|
||||
renderWithMantine(
|
||||
<ToolDetailPanel {...baseProps} tool={simpleTool} isExecuting={true} />,
|
||||
|
|
|
|||
|
|
@ -355,6 +355,10 @@ export function ToolDetailPanel({
|
|||
// tool's in-progress field text. See SchemaFormProps.resetKey.
|
||||
resetKey={resetKey ?? name}
|
||||
onValidityChange={setHasInvalidDraft}
|
||||
// These values become `tools/call` arguments, so a raw-JSON draft the
|
||||
// client would retype is refused rather than sent as something other
|
||||
// than what the editor shows (#2171).
|
||||
enforceToolArgumentTypes
|
||||
/>
|
||||
|
||||
{progress && <ProgressDisplay params={progress} />}
|
||||
|
|
|
|||
Loading…
Reference in a new issue