Merge branch 'v2/main' into v2/chore/2130-inspectorview-prop-bundles
This commit is contained in:
commit
aacfef3ee2
14 changed files with 644 additions and 31 deletions
|
|
@ -298,6 +298,29 @@ Every surface in this client where JSON is **typed**, plus `ContentViewer`'s rea
|
|||
| `groups/ImportServerJsonPanel`, `groups/ExperimentalFeaturesPanel` | The panel owns the text and validates it itself. |
|
||||
| `elements/ContentViewer`'s JSON branch | Read-only — see below. |
|
||||
|
||||
**A raw-JSON draft is refused when the client would retype it.** `callTool`
|
||||
converts every *string-valued* argument to the type the tool's `inputSchema`
|
||||
declares, because the widget form hands everything over as text — so `"2"`
|
||||
against a numeric field has to become `2`. A JSON draft already carries its own
|
||||
types, so a value that 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. Both JSON-authoring surfaces therefore
|
||||
refuse such a draft and name the value to rewrite
|
||||
([#2171](https://github.com/modelcontextprotocol/inspector/issues/2171)):
|
||||
**Edit as JSON** in the Tools and Apps tabs (which had been sending it, coerced
|
||||
and silently), and **Edit and replay**, which already did.
|
||||
|
||||
One helper decides it for both — `coercedArgumentNames` in
|
||||
[`core/json/jsonUtils.ts`](../../core/json/jsonUtils.ts), which runs the real
|
||||
conversion and compares rather than restating its rules, so a surface cannot
|
||||
drift from what would actually be sent. `coercedArgumentsError` is the sentence
|
||||
they share, so the same refusal cannot be worded two ways.
|
||||
|
||||
The check is **opt-in** (`SchemaFormProps.enforceToolArgumentTypes`), not
|
||||
inferred from the schema: an elicitation renders through the same `SchemaForm`
|
||||
and its values are never converted, so enforcing there would refuse a draft for
|
||||
a reason that is not true of it. Only the Tools and Apps panels pass it.
|
||||
|
||||
**Read-only mode is what `ContentViewer` renders JSON as**, and through it every payload that reaches the app that way: Protocol and Network entries, tool results, structured output, resource previews, server cards. Highlighting is *not* what that buys — JSON already highlighted, via the lazily-imported Prism grammar `CodeHighlight` loads. What Ace adds is **folding**, line numbers and a gutter on a large payload. The Prism `json` grammar was dropped in the same change rather than kept beside it: two highlighters for one language drift, and nothing else asks for `json`. Two cases stay on the plain renderer — a `wrap={false}` caller (the server card's fixed-height, single-line box) and untyped text that only *looks* like JSON but does not parse, which in an editor would frame a server's prose as a malformed document.
|
||||
|
||||
Two read-only JSON displays deliberately do **not** go through `ContentViewer`, and so are not on this editor — worth knowing before assuming it is the route for all of them. `ExperimentalFeaturesPanel` renders its JSON-RPC **response** in a read-only `Textarea` (its *request* box is on the editor), and `ConnectionInfoContent/OAuthTokenField` renders a decoded JWT in a `Code` block beside the raw token, where the decoded/raw toggle and the copy affordance belong to the field rather than to a viewer. Both are candidates for the same treatment; neither is in [#2151](https://github.com/modelcontextprotocol/inspector/issues/2151)'s scope.
|
||||
|
|
|
|||
|
|
@ -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} />}
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ import {
|
|||
screen,
|
||||
within,
|
||||
} from "../../../test/renderWithMantine";
|
||||
import { setAceTextByLabel } from "../../../test/aceEditor";
|
||||
import {
|
||||
AppsScreen,
|
||||
type AppsScreenProps,
|
||||
|
|
@ -320,6 +321,35 @@ describe("AppsScreen", () => {
|
|||
expect(screen.getByTitle("Weather Widget")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// The Apps counterpart of the same wire claim (#2171): an App's arguments are
|
||||
// a `tools/call` like any other, so a refused draft must not open the app.
|
||||
it("dispatches nothing for a draft the schema would retype", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onOpenApp = vi.fn();
|
||||
const numericApp: Tool = {
|
||||
name: "chart",
|
||||
title: "Chart Widget",
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { count: { type: "number" } },
|
||||
},
|
||||
_meta: { ui: { resourceUri: "ui://apps/chart" } },
|
||||
};
|
||||
renderWithMantine(
|
||||
<ControlledAppsScreen tools={[numericApp]} onOpenApp={onOpenApp} />,
|
||||
);
|
||||
await user.click(screen.getByText("Chart Widget"));
|
||||
await user.click(screen.getByLabelText("Edit as JSON"));
|
||||
await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}');
|
||||
await user.click(screen.getByRole("button", { name: /Open App/ }));
|
||||
|
||||
expect(onOpenApp).not.toHaveBeenCalled();
|
||||
|
||||
await setAceTextByLabel(/Arguments JSON/, '{"count":1}');
|
||||
await user.click(screen.getByRole("button", { name: /Open App/ }));
|
||||
expect(onOpenApp).toHaveBeenCalledWith("chart", { count: 1 });
|
||||
});
|
||||
|
||||
it("seeds schema defaults so untouched fields are sent on Open App", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onOpenApp = vi.fn();
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@ import { describe, it, expect, vi } from "vitest";
|
|||
import userEvent from "@testing-library/user-event";
|
||||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
import { renderWithMantine, screen } from "../../../test/renderWithMantine";
|
||||
import { setAceTextByLabel } from "../../../test/aceEditor";
|
||||
import { noopPagination } from "../../../test/fixtures/pagination";
|
||||
import {
|
||||
ToolsScreen,
|
||||
|
|
@ -157,6 +158,43 @@ describe("ToolsScreen", () => {
|
|||
);
|
||||
});
|
||||
|
||||
// #2171's acceptance asks for a test that pins the WIRE rather than the form
|
||||
// state. Under the refusal answer the wire claim is that nothing is sent, and
|
||||
// `onCallTool` is where a dispatch would begin — so a screen that flagged the
|
||||
// draft but still fired the callback would satisfy every other test here.
|
||||
//
|
||||
// The plain and "Run as task" paths need no separate case: the split between
|
||||
// `callTool` and `callToolStream` happens in `App.tsx`, downstream of this
|
||||
// callback, so a gate that stops the callback stops both.
|
||||
it("dispatches nothing for a draft the schema would retype", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onCallTool = vi.fn();
|
||||
const numeric: Tool[] = [
|
||||
{
|
||||
name: "add",
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { count: { type: "number" } },
|
||||
},
|
||||
},
|
||||
];
|
||||
renderWithMantine(
|
||||
<ControlledToolsScreen tools={numeric} onCallTool={onCallTool} />,
|
||||
);
|
||||
await user.click(screen.getByText("add"));
|
||||
await user.click(screen.getByLabelText("Edit as JSON"));
|
||||
await setAceTextByLabel(/Arguments JSON/, '{"count":"01"}');
|
||||
await user.click(screen.getByRole("button", { name: /Execute/ }));
|
||||
|
||||
expect(onCallTool).not.toHaveBeenCalled();
|
||||
|
||||
// And it is the draft that blocks it, not the screen: rewritten with the
|
||||
// declared type, the very same click dispatches.
|
||||
await setAceTextByLabel(/Arguments JSON/, '{"count":1}');
|
||||
await user.click(screen.getByRole("button", { name: /Execute/ }));
|
||||
expect(onCallTool).toHaveBeenCalledWith("add", { count: 1 }, false);
|
||||
});
|
||||
|
||||
it("filters the sidebar list as the search text changes", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<ControlledToolsScreen />);
|
||||
|
|
|
|||
|
|
@ -862,6 +862,118 @@ describe("subscriptions and completion", () => {
|
|||
expect(h.api().onSubscribeResource).toBeTypeOf("function");
|
||||
});
|
||||
|
||||
// #2174. Both used to discard the promise with a bare `void`, so a failure
|
||||
// was invisible in the UI and surfaced only as an unhandled rejection.
|
||||
it("toasts a failed subscribe rather than swallowing it", async () => {
|
||||
const h = harness({
|
||||
client: client({
|
||||
subscribeToResource: vi
|
||||
.fn()
|
||||
.mockRejectedValue(
|
||||
new Error("Server does not support resource subscriptions"),
|
||||
),
|
||||
}),
|
||||
});
|
||||
await act(async () => h.api().onSubscribeResource("u"));
|
||||
await waitFor(() =>
|
||||
expect(notificationsMock.show).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
title: "Failed to subscribe to resource",
|
||||
message: "Server does not support resource subscriptions",
|
||||
}),
|
||||
),
|
||||
);
|
||||
});
|
||||
|
||||
it("toasts a failed unsubscribe rather than swallowing it", async () => {
|
||||
const h = harness({
|
||||
client: client({
|
||||
unsubscribeFromResource: vi
|
||||
.fn()
|
||||
.mockRejectedValue(new Error("Client is not connected")),
|
||||
}),
|
||||
});
|
||||
await act(async () => h.api().onUnsubscribeResource("u"));
|
||||
await waitFor(() =>
|
||||
expect(notificationsMock.show).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
title: "Failed to unsubscribe from resource",
|
||||
message: "Client is not connected",
|
||||
}),
|
||||
),
|
||||
);
|
||||
});
|
||||
|
||||
it("recovers a lapsed authorization on subscribe, and retries", async () => {
|
||||
const recover = vi.fn().mockResolvedValue(true);
|
||||
const subscribeToResource = vi
|
||||
.fn()
|
||||
.mockRejectedValueOnce(authError())
|
||||
.mockResolvedValue(undefined);
|
||||
const h = harness({
|
||||
client: client({ subscribeToResource }),
|
||||
activeServerId: "a",
|
||||
recovery: { handleCommandScopedAuthRecovery: recover },
|
||||
});
|
||||
await act(async () => h.api().onSubscribeResource("u"));
|
||||
await waitFor(() => expect(subscribeToResource).toHaveBeenCalledTimes(2));
|
||||
// `ambient`, not `resource`: a `resource` step-up failure is routed into
|
||||
// the read-preview panel, which this command has nothing to do with.
|
||||
expect(recover).toHaveBeenCalledWith(
|
||||
expect.any(AuthRecoveryRequiredError),
|
||||
{
|
||||
serverId: "a",
|
||||
source: "ambient",
|
||||
retryOperation: expect.any(Function),
|
||||
},
|
||||
);
|
||||
// The recovery owns the prompt, so the command itself stays quiet.
|
||||
expect(notificationsMock.show).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("recovers a lapsed authorization on unsubscribe, and retries", async () => {
|
||||
const recover = vi.fn().mockResolvedValue(true);
|
||||
const unsubscribeFromResource = vi
|
||||
.fn()
|
||||
.mockRejectedValueOnce(authError())
|
||||
.mockResolvedValue(undefined);
|
||||
const h = harness({
|
||||
client: client({ unsubscribeFromResource }),
|
||||
activeServerId: "a",
|
||||
recovery: { handleCommandScopedAuthRecovery: recover },
|
||||
});
|
||||
await act(async () => h.api().onUnsubscribeResource("u"));
|
||||
await waitFor(() =>
|
||||
expect(unsubscribeFromResource).toHaveBeenCalledTimes(2),
|
||||
);
|
||||
expect(recover).toHaveBeenCalledWith(
|
||||
expect.any(AuthRecoveryRequiredError),
|
||||
{
|
||||
serverId: "a",
|
||||
source: "ambient",
|
||||
retryOperation: expect.any(Function),
|
||||
},
|
||||
);
|
||||
expect(notificationsMock.show).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("does not toast a subscribe whose recovery was left unsatisfied", async () => {
|
||||
// The recovery has taken over — a redirect is pending or a step-up prompt
|
||||
// is open — so the wrapper resolves `undefined` rather than rejecting, and
|
||||
// a toast here would talk over the prompt the user is looking at.
|
||||
const recover = vi.fn().mockResolvedValue(false);
|
||||
const h = harness({
|
||||
client: client({
|
||||
subscribeToResource: vi.fn().mockRejectedValue(authError()),
|
||||
}),
|
||||
activeServerId: "a",
|
||||
recovery: { handleCommandScopedAuthRecovery: recover },
|
||||
});
|
||||
await act(async () => h.api().onSubscribeResource("u"));
|
||||
await waitFor(() => expect(recover).toHaveBeenCalled());
|
||||
expect(notificationsMock.show).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("returns the completion values", async () => {
|
||||
const c = client();
|
||||
const h = harness({ client: c });
|
||||
|
|
|
|||
|
|
@ -564,20 +564,45 @@ export function useServerCommands({
|
|||
[inspectorClient, activeServerId, handleCommandScopedAuthRecovery],
|
||||
);
|
||||
|
||||
// Both route through the shared recovery like every other command (#2174).
|
||||
// They used to discard the promise with a bare `void`, which neither the
|
||||
// callee nor anything else owned: `subscribeToResource` throws when the
|
||||
// client is disconnected, when the server declares no subscription support,
|
||||
// and (wrapped) on a failed request, so a subscribe that failed did nothing
|
||||
// visible and surfaced only as an unhandled rejection.
|
||||
//
|
||||
// The source is `ambient`, not `resource`. A `resource` step-up failure is
|
||||
// routed into `readResourceState` — the *preview* panel — which describes a
|
||||
// read of whatever resource is selected, not this subscribe; marking it
|
||||
// errored would contradict a read that succeeded. Subscribing has no panel
|
||||
// of its own (the tile only flips its button label), which is the same
|
||||
// position the refreshes and the pagination toggle are in, and they are
|
||||
// `ambient` for the same reason.
|
||||
//
|
||||
// Both pass an `errorTitle`: nothing else records these failures, so without
|
||||
// one the button would go on silently doing nothing.
|
||||
const onSubscribeResource = useCallback(
|
||||
(uri: string) => {
|
||||
if (!inspectorClient) return;
|
||||
void inspectorClient.subscribeToResource(uri);
|
||||
runCommandInBackground(
|
||||
() => inspectorClient.subscribeToResource(uri),
|
||||
"ambient",
|
||||
"Failed to subscribe to resource",
|
||||
);
|
||||
},
|
||||
[inspectorClient],
|
||||
[inspectorClient, runCommandInBackground],
|
||||
);
|
||||
|
||||
const onUnsubscribeResource = useCallback(
|
||||
(uri: string) => {
|
||||
if (!inspectorClient) return;
|
||||
void inspectorClient.unsubscribeFromResource(uri);
|
||||
runCommandInBackground(
|
||||
() => inspectorClient.unsubscribeFromResource(uri),
|
||||
"ambient",
|
||||
"Failed to unsubscribe from resource",
|
||||
);
|
||||
},
|
||||
[inspectorClient],
|
||||
[inspectorClient, runCommandInBackground],
|
||||
);
|
||||
|
||||
const onCompleteArgument = useCallback(
|
||||
|
|
|
|||
|
|
@ -9,7 +9,10 @@ import type { MessageEntry } from "@inspector/core/mcp/types.js";
|
|||
// `components → lib → utils` direction still holds: `LogEntryData` is the shape
|
||||
// the Logs screen renders, and this module exists to produce exactly that.
|
||||
import type { LogEntryData } from "../components/elements/LogEntry/LogEntry";
|
||||
import { convertToolParameters } from "@inspector/core/json/jsonUtils.js";
|
||||
import {
|
||||
coercedArgumentNames,
|
||||
coercedArgumentsError,
|
||||
} from "@inspector/core/json/jsonUtils.js";
|
||||
import { isReplayableProtocolMethod } from "../utils/replayableProtocolMethods";
|
||||
|
||||
// Derive `LogEntryData[]` from the MessageLog by filtering for the
|
||||
|
|
@ -167,13 +170,19 @@ export function replayableParams(
|
|||
* spec types `GetPromptRequest.params.arguments` as `Record<string, string>`,
|
||||
* so a string is the only thing a prompt argument can be.
|
||||
* - For **`tools/call`**, the mirror image. `callTool` runs every *string*
|
||||
* entry through `convertToolParameters`, because the Tools form hands
|
||||
* entry through `convertToolParameters`, because the widget form hands
|
||||
* everything over as text — so `{"count": "2"}` against a schema declaring a
|
||||
* number is sent as `{"count": 2}`. Detected by running that same conversion
|
||||
* and comparing, rather than by reimplementing its rules, so the two cannot
|
||||
* drift. Needs the `tool`; without one this check is skipped, since nothing
|
||||
* can be said about a coercion whose schema is unknown.
|
||||
*
|
||||
* The Tools and Apps tabs' own "Edit as JSON" switch refuses the identical
|
||||
* draft, through the identical helper (#2171). Both are JSON documents whose
|
||||
* values already carry their types, so in both the conversion would send
|
||||
* something other than what the editor showed — and one of the two answering
|
||||
* differently is the inconsistency #2171 was filed to remove.
|
||||
*
|
||||
* `name` and `uri` are deliberately not checked here: the dispatch already
|
||||
* refuses a missing or non-string one with a reason the caller surfaces as a
|
||||
* toast, so those fail visibly rather than silently.
|
||||
|
|
@ -203,34 +212,20 @@ export function reshapedReplayParam(
|
|||
}
|
||||
}
|
||||
if (method === "tools/call" && tool) {
|
||||
const coerced = coercedToolArgs(tool, args as Record<string, unknown>);
|
||||
// The same check the Tools/Apps form's raw-JSON editor makes, from the same
|
||||
// helper, so the two JSON-authoring surfaces cannot disagree about which
|
||||
// drafts are sendable (#2171).
|
||||
const coerced = coercedArgumentNames(
|
||||
tool.inputSchema,
|
||||
args as Record<string, unknown>,
|
||||
);
|
||||
if (coerced.length > 0) {
|
||||
return `${coerced.join(", ")} would be converted to the type ${tool.name}'s schema declares — write the value with that type instead`;
|
||||
return coercedArgumentsError(coerced, tool.name);
|
||||
}
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* The string-valued argument names `callTool` would convert, per the tool's
|
||||
* schema.
|
||||
*
|
||||
* Runs the conversion the client runs and compares, rather than restating its
|
||||
* rules: `convertToolParameters` is the function on the other side, so asking
|
||||
* it is the only way this cannot drift from what is actually sent.
|
||||
*/
|
||||
function coercedToolArgs(tool: Tool, args: Record<string, unknown>): string[] {
|
||||
const stringArgs: Record<string, string> = {};
|
||||
for (const [key, value] of Object.entries(args)) {
|
||||
if (typeof value === "string") stringArgs[key] = value;
|
||||
}
|
||||
if (Object.keys(stringArgs).length === 0) return [];
|
||||
const converted = convertToolParameters(tool, stringArgs);
|
||||
return Object.keys(stringArgs)
|
||||
.filter((key) => converted[key] !== stringArgs[key])
|
||||
.map((key) => `\`${key}\``);
|
||||
}
|
||||
|
||||
export async function replayProtocolRequest(
|
||||
client: ReplayClient,
|
||||
method: string,
|
||||
|
|
|
|||
|
|
@ -3,6 +3,8 @@ import {
|
|||
convertParameterValue,
|
||||
convertToolParameters,
|
||||
convertPromptArguments,
|
||||
coercedArgumentNames,
|
||||
coercedArgumentsError,
|
||||
toRecord,
|
||||
} from "@inspector/core/json/jsonUtils.js";
|
||||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
|
|
@ -61,6 +63,122 @@ describe("JSON Utils", () => {
|
|||
});
|
||||
});
|
||||
|
||||
// #2171: the predicate both raw-JSON editors refuse on. It must agree with
|
||||
// the conversion exactly — a false negative sends a payload other than the
|
||||
// one shown, and a false positive blocks a draft that was fine.
|
||||
describe("coercedArgumentNames", () => {
|
||||
const numeric: Tool = {
|
||||
name: "add",
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { count: { type: "number" }, label: { type: "string" } },
|
||||
},
|
||||
};
|
||||
|
||||
it("names a string the schema would retype", () => {
|
||||
expect(
|
||||
coercedArgumentNames(numeric.inputSchema, { count: "01" }),
|
||||
).toEqual(["count"]);
|
||||
});
|
||||
|
||||
// The conversion only inspects strings, so a value already written with
|
||||
// its declared type is untouched and must not be reported.
|
||||
it("ignores a value already of the declared type", () => {
|
||||
expect(coercedArgumentNames(numeric.inputSchema, { count: 1 })).toEqual(
|
||||
[],
|
||||
);
|
||||
});
|
||||
|
||||
it("ignores a string the schema declares as a string", () => {
|
||||
expect(
|
||||
coercedArgumentNames(numeric.inputSchema, { label: "01" }),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
// An argument the schema does not declare is passed through by the
|
||||
// conversion, so there is nothing to warn about.
|
||||
it("ignores an undeclared argument", () => {
|
||||
expect(coercedArgumentNames(numeric.inputSchema, { extra: "x" })).toEqual(
|
||||
[],
|
||||
);
|
||||
});
|
||||
|
||||
it("reports every offending name, in supplied order", () => {
|
||||
const two: Tool = {
|
||||
name: "pair",
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: { type: "number" }, b: { type: "boolean" } },
|
||||
},
|
||||
};
|
||||
expect(
|
||||
coercedArgumentNames(two.inputSchema, { a: "1", b: "true" }),
|
||||
).toEqual(["a", "b"]);
|
||||
});
|
||||
|
||||
// Root composition (#2123): the declared type can live on a branch, and
|
||||
// the conversion selects that branch from the supplied values — so this
|
||||
// must too, or a branch's arguments would look unconvertible.
|
||||
it("resolves a type declared on a root union branch", () => {
|
||||
const union: Tool = {
|
||||
name: "u",
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
oneOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: {
|
||||
kind: { const: "n" },
|
||||
value: { type: "number" },
|
||||
},
|
||||
required: ["kind"],
|
||||
},
|
||||
{
|
||||
type: "object",
|
||||
properties: {
|
||||
kind: { const: "s" },
|
||||
value: { type: "string" },
|
||||
},
|
||||
required: ["kind"],
|
||||
},
|
||||
],
|
||||
},
|
||||
};
|
||||
expect(
|
||||
coercedArgumentNames(union.inputSchema, { kind: "n", value: "2" }),
|
||||
).toEqual(["value"]);
|
||||
expect(
|
||||
coercedArgumentNames(union.inputSchema, { kind: "s", value: "2" }),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it("says nothing when no argument is a string", () => {
|
||||
expect(coercedArgumentNames(numeric.inputSchema, { count: 1 })).toEqual(
|
||||
[],
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("coercedArgumentsError", () => {
|
||||
// Both surfaces refuse the same drafts, so they must not word it two ways;
|
||||
// the tool name is included only where the caller knows it.
|
||||
it("names the tool when it is known", () => {
|
||||
expect(coercedArgumentsError(["count"], "add")).toBe(
|
||||
"`count` would be converted to the type add's schema declares — write the value with that type instead",
|
||||
);
|
||||
});
|
||||
|
||||
it("omits the tool when the caller is already rendering one", () => {
|
||||
expect(coercedArgumentsError(["count"])).toBe(
|
||||
"`count` would be converted to the type the schema declares — write the value with that type instead",
|
||||
);
|
||||
});
|
||||
|
||||
it("lists several names", () => {
|
||||
expect(coercedArgumentsError(["a", "b"], "pair")).toContain("`a`, `b`");
|
||||
});
|
||||
});
|
||||
|
||||
describe("convertToolParameters", () => {
|
||||
const tool: Tool = {
|
||||
name: "test-tool",
|
||||
|
|
|
|||
|
|
@ -294,12 +294,32 @@ function suppliedMatchesConst(value: string, constValue: unknown): boolean {
|
|||
export function convertToolParameters(
|
||||
tool: Tool,
|
||||
params: Record<string, string>,
|
||||
): Record<string, JsonValue> {
|
||||
return convertParametersForSchema(tool.inputSchema, params);
|
||||
}
|
||||
|
||||
/**
|
||||
* {@link convertToolParameters} against a bare `inputSchema` rather than a
|
||||
* `Tool`.
|
||||
*
|
||||
* Split out because the two callers hold different things: the client has the
|
||||
* `Tool`, while a form holds only the schema it is rendering (which is that
|
||||
* same object, structurally narrowed). Both must reach the SAME conversion —
|
||||
* one of them decides what goes on the wire and the other decides whether to
|
||||
* let the user send it, so a second implementation would let them disagree
|
||||
* about which values are convertible.
|
||||
*/
|
||||
export function convertParametersForSchema(
|
||||
inputSchema: unknown,
|
||||
params: Record<string, string>,
|
||||
): Record<string, JsonValue> {
|
||||
const result: Record<string, JsonValue> = {};
|
||||
// A property's schema can live on a root composition branch rather than on
|
||||
// the root itself (#2123); see `coercionProperties` for how the branch is
|
||||
// identified when it does.
|
||||
const { base, branches } = resolveRootUnion(tool.inputSchema ?? {});
|
||||
const { base, branches } = resolveRootUnion(
|
||||
(inputSchema ?? {}) as RootUnionSchema,
|
||||
);
|
||||
const properties = coercionProperties(base, branches, params);
|
||||
for (const [key, value] of Object.entries(params)) {
|
||||
const declared = properties[key];
|
||||
|
|
@ -338,6 +358,62 @@ export function convertToolParameters(
|
|||
return result;
|
||||
}
|
||||
|
||||
/**
|
||||
* The argument names a `tools/call` would silently retype, per the tool's
|
||||
* schema — empty when every value is already the type the schema declares.
|
||||
*
|
||||
* Runs the conversion the client runs and compares, rather than restating its
|
||||
* rules: {@link convertParametersForSchema} is the function on the other side,
|
||||
* so asking it is the only way a caller cannot drift from what is actually
|
||||
* sent.
|
||||
*
|
||||
* Only string-valued arguments can be retyped, because that is all the
|
||||
* conversion looks at — a JSON draft that already writes `2` as a number is
|
||||
* passed through untouched, and is not reported here.
|
||||
*
|
||||
* Used by the two places a payload is authored as JSON rather than through
|
||||
* widgets — the Tools/Apps form's "Edit as JSON" switch and the
|
||||
* Edit-and-replay modal — each of which refuses a draft this returns anything
|
||||
* for, so what the editor shows is what the wire carries (#2171).
|
||||
*/
|
||||
export function coercedArgumentNames(
|
||||
inputSchema: unknown,
|
||||
args: Record<string, unknown>,
|
||||
): string[] {
|
||||
const stringArgs: Record<string, string> = {};
|
||||
for (const [key, value] of Object.entries(args)) {
|
||||
if (typeof value === "string") stringArgs[key] = value;
|
||||
}
|
||||
if (Object.keys(stringArgs).length === 0) return [];
|
||||
const converted = convertParametersForSchema(inputSchema, stringArgs);
|
||||
// `Object.keys` of the *supplied* names, so an argument the schema does not
|
||||
// declare (which the conversion passes through) cannot be reported.
|
||||
return Object.keys(stringArgs).filter(
|
||||
(key) => converted[key] !== stringArgs[key],
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* The refusal {@link coercedArgumentNames} justifies, as one sentence.
|
||||
*
|
||||
* Shared for the same reason the check is: the Tools/Apps raw-JSON editor and
|
||||
* the Edit-and-replay modal refuse the same drafts, so they must not word it
|
||||
* two different ways. `toolName` is included when the caller knows which tool
|
||||
* the draft targets — the replay modal does, because its `name` is editable
|
||||
* and the schema is looked up from it; a form is already rendering one tool
|
||||
* and would only be repeating itself.
|
||||
*/
|
||||
export function coercedArgumentsError(
|
||||
names: string[],
|
||||
toolName?: string,
|
||||
): string {
|
||||
const quoted = names.map((name) => `\`${name}\``).join(", ");
|
||||
const whose = toolName
|
||||
? `the type ${toolName}'s schema declares`
|
||||
: "the type the schema declares";
|
||||
return `${quoted} would be converted to ${whose} — write the value with that type instead`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Convert prompt arguments (JsonValue) to strings for prompt API
|
||||
*/
|
||||
|
|
|
|||
Loading…
Reference in a new issue