diff --git a/AGENTS.md b/AGENTS.md index 4ba943ecb..c7f5e841d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -79,7 +79,34 @@ v2/main/ │ │ # a nullable field entirely — #1928/#2015) │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime + state stores -│ │ # (modernTaskSchemas.ts: SEP-2663 modern Tasks +│ │ # (uriTemplate.ts: RFC 6570 parse/classify/expand. +│ │ # The ONE expander for the web Resources form and +│ │ # readResourceFromTemplate (TUI); the CLI is NOT a +│ │ # consumer — it has no template form and passes an +│ │ # already-expanded --uri to readResource. Both +│ │ # consumers derive their FORM FIELDS from it too +│ │ # (clients/tui uriTemplateToForm), +│ │ # which is what makes the sharing real: a form +│ │ # submits under the names it rendered, so a mangled +│ │ # name silently drops the value at expansion. The +│ │ # SDK's UriTemplate is used only to VALIDATE; its +│ │ # expander is wrong for `{a,b}` (raw-joined), +│ │ # `{;id}` (operator missing), `{id:3}` (modifier in +│ │ # the name), `{+v}`/`{#v}` (encodeURI mangles `[`/`]` +│ │ # and double-encodes pct-triplets), and `{v}` +│ │ # (encodeURIComponent leaves `!'()*` bare). +│ │ # Requiredness is per EXPRESSION, not per variable — +│ │ # requiredGroups + hasRequiredValues. A template that +│ │ # cannot expand ({id:abc}, {}, {a,}) WITHHOLDS the read: +│ │ # tryExpandUriTemplate returns the reason as a value, and +│ │ # the panel disables Read Resource rather than sending the +│ │ # raw template — expandUriTemplate's raw-template fallback +│ │ # is for DISPLAY (the preview) only. An UNDEFINED +│ │ # variable omits its expression; one defined as "" +│ │ # expands (`x{?q}` → `x?q=`), so each FORM — not the +│ │ # expander — drops its untouched blanks via +│ │ # definedValues — #1919; +│ │ # modernTaskSchemas.ts: SEP-2663 modern Tasks │ │ # extension wire schemas + normalize/handle helpers, │ │ # used by the raw-wire tasks/* channel — #1631; │ │ # listSalvage.ts: per-item salvage for list results — diff --git a/README.md b/README.md index 36765355d..62514114a 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,8 @@ inspector/ │ ├── json/ # JSON + parameter/argument conversion utilities, and the nullable-union │ │ # schema collapse shared by the web and TUI form builders │ ├── logging/ # Silent pino logger singleton -│ ├── mcp/ # InspectorClient runtime, state stores, transports, config import +│ ├── mcp/ # InspectorClient runtime, state stores, transports, config import, +│ │ # and the RFC 6570 URI-template helpers the web form and TUI expand through │ ├── node/ # Node-only shared helpers: version reader, hostUrl (host normalize/canonicalize + all-interfaces/loopback detection) │ ├── react/ # React hooks over the state stores │ └── storage/ # File I/O helpers for the OAuth persist backends @@ -147,6 +148,7 @@ Each config below is a ready-made server for exercising one feature by hand. Loa | `structured-output-http.json` | Tools tab: a result's `structuredContent` section | [#1908](https://github.com/modelcontextprotocol/inspector/issues/1908) | | `duplicate-tool-names-http.json` | A `tools/list` that repeats a tool name | [#1957](https://github.com/modelcontextprotocol/inspector/issues/1957) | | `nullable-fields-http.json` | Tools tab: nullable (`anyOf` + `null`) arguments | [#1928](https://github.com/modelcontextprotocol/inspector/issues/1928) | +| `rfc6570-templates-http.json` | Resources tab: RFC 6570 resource-template expansion | [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) | | `advertised-extensions-http.json` | Tool registration gated on advertised extensions | [#1739](https://github.com/modelcontextprotocol/inspector/issues/1739) | | `logging-{legacy,modern}-http.json` | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | | `subscriptions-{legacy,modern}-http.json` | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | @@ -237,6 +239,38 @@ Open the Tools tab and select `record_shipment`: `direction` must render as a ** The **TUI** had the same gap and is worth checking against the same server (`--tui`, then test `record_shipment`): `direction` is a select, `quantity` an integer field, `express` a boolean. Both clients now share one collapse step — `normalizeNullableUnion` in [`core/json/nullableUnion.ts`](./core/json/nullableUnion.ts) — precisely so they cannot drift on which schemas they can render. +#### RFC 6570 resource templates + +`rfc6570-templates-http.json` serves two resource templates straight out of [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) — `events_by_topic` (`foobar://events/{topic}`) and `events_by_query` (`foobar://events{?topic}`) — each echoing the URI it was matched against, plus a plain `foobar://events` resource (see below). Plain streamable-HTTP; connect with the **default (legacy)** protocol era. + +Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The request must go out as `foobar://events/foo%2Fbar`, and the result echoes back the URI the server matched. On the broken build the value was spliced in raw, so the slash created a second path segment and the SDK's matcher answered `-32602 Resource not found: foobar://events/foo/bar` — the exact failure in the issue. The same holds for `?`, `#`, `%`, spaces, and non-ASCII text. + +**events_by_query** is the half that was invisible: the old `/\{(\w+)\}/g` scan could not see an expression carrying an operator, so no `topic` input was rendered at all. It now appears, marked **Optional** — RFC 6570 drops the whole expression when the variable is undefined, so reading with the field blank requests `foobar://events`, and filling it in requests `foobar://events?topic=foo%2Fbar`. The URI preview beside the title shows the partially-expanded form as you type, leaving unfilled expressions standing as written. + +> The plain `foobar://events` resource is registered deliberately, not as filler. The SDK's `UriTemplate.match()` compiles `{?topic}` to a **required** `\?topic=([^&]+)`, so a template alone cannot serve the blank read — `match("foobar://events")` returns `null`. A real server exposes the unfiltered collection as its own resource; the showcase does the same so that step actually resolves. + +The web client and the TUI expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI via `InspectorClient.readResourceFromTemplate` — and both derive their **form fields** from its parser too, which is the half that makes the sharing real: a form submits values under the names it rendered, so a parser that mangles a name silently drops the value at expansion time. (The CLI is not a consumer: it has no template form, and its `resources/read` passes the already-expanded `--uri` straight through.) + +The SDK's `UriTemplate` is still used, but only to *validate* a template (constructing it is what rejects an unclosed expression). Its expander is not, because it is incomplete in five ways — each measured against the pinned SDK, not inferred: + +| Shape | SDK behavior | +| --- | --- | +| `{a,b}` | raw-joins the values — no encoding, operator prefix dropped | +| `{;id}` | `;` is missing from its operator list, so the variable parses as `;id` | +| `{id:3}` | the prefix modifier is folded into the name, giving `id:3` | +| `{+v}` / `{#v}` | `encodeURI` mangles reserved `[`/`]` (`[::1]` → `%5B::1%5D`) and double-encodes pct-triplets (`%2F` → `%252F`) | +| `{v}` | `encodeURIComponent` leaves the sub-delims `!'()*` bare, which RFC 6570 requires encoded | + +The `;` and `:3` rows are the ones a user sees directly: on the SDK's parse the form renders fields literally labelled `;id` and `id:3`. The `+`/`#` row is silent corruption rather than over-escaping — an IPv6 literal or an already-encoded path arrives at the server altered. + +A template that cannot be expanded at all — an out-of-grammar modifier (`{id:abc}`), or an expression declaring no variable (`{}`, `{a,}`, `{?}`) — **withholds the read** rather than sending something. Pick **events_malformed** (`foobar://events/{topic:abc}`) to see it: Read Resource is disabled, the reason is printed under the form, and the preview shows the template as the server declared it. The alternative is worse than it looks: `x://{}` would otherwise expand to `x://` with no inputs rendered, so the form's "everything required is filled" check passes vacuously and it reads a URI that is not the template the server published. + +Literals are pct-encoded on expansion too (RFC 6570 §3.1): `café/{var}` sends `caf%C3%A9/value`, not raw UTF-8 in the path — something the SDK's expander does not do either. And the *names* a template may use are RFC 6570's `varchar` plus a labelled tolerance for `-` and `~`: the conformance suite rejects `{default-graph-uri}`, but real servers publish such names and the SDK's matcher round-trips them, so the Inspector expands them and marks the variable `conforming: false` rather than refusing a resource that demonstrably works. + +An **undefined** variable is what omits its expression — a variable defined as the empty string expands (`x{?q}` gives `x?q=`, `x{;q}` gives `x;q`, per RFC 6570 §3.2.7). The expander honors that distinction, so a caller such as `readResourceFromTemplate` can request either URI. Collapsing the two is a *form* concern, not a template one: both clients seed every declared variable with `""` and a text input cannot express "defined but empty", so each form drops its blanks (`definedValues`) on the way in. + +Requiredness is a property of the **expression**, not the variable: RFC 6570 drops undefined names from a multi-name expression, so `{a,b}` with only `a` filled is expandable and a form must not block it. `requiredGroups` returns one entry per non-omittable expression and `hasRequiredValues` asks that each be satisfied by any one of its names — which no per-variable flag can express once a name recurs across expressions (`{a,b}{a,c}` is satisfied by filling `b` and `c`). + #### Advertised extensions `advertised-extensions-http.json` serves `echo` (always) and a `get_weather` tool **gated on the `io.modelcontextprotocol/tasks` extension** (`extensionGatedTools`): the tool is registered but starts disabled, and the server enables it on `notifications/initialized` only when the client declared that extension in its `capabilities.extensions`. diff --git a/clients/tui/__tests__/ResourceTestModal.test.tsx b/clients/tui/__tests__/ResourceTestModal.test.tsx index d9de004d6..525051a0b 100644 --- a/clients/tui/__tests__/ResourceTestModal.test.tsx +++ b/clients/tui/__tests__/ResourceTestModal.test.tsx @@ -77,6 +77,73 @@ const renderAndSubmit = async ( }; describe("ResourceTestModal", () => { + // RFC 6570 keeps a required expression satisfied by any ONE of its names, so + // `{a,b}` cannot be expressed with ink-form's per-field `required` flag and + // its members are left optional there. Without the modal's own group check + // the TUI would submit blank -- dropping the expression and reading a + // different resource -- while the web panel blocks the same request (#1919). + describe("required-group validation", () => { + it("refuses to submit when no name in a required group has a value", async () => { + const read = vi.fn(); + const { onClose, unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://{a,b}" }), + { a: "", b: "" }, + ); + expect(read).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + unmount(); + }); + + it("blocks a required variable whose name collides with Object.prototype", async () => { + // `constructor` is a valid RFC 6570 varname, and the modal's own filter + // used to read `Object` (length 1) as a filled value. The *message* built + // from that filter came out naming no field; since frames render empty + // here (see the header note), that half is asserted on + // `unmetRequiredGroups` in the core suite. This asserts the gate. + const read = vi.fn(); + const { onClose, unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://{constructor}" }), + { constructor: "" }, + ); + expect(read).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + unmount(); + }); + + it("submits once any one name in the group has a value", async () => { + const read = vi.fn().mockResolvedValue({ + result: { contents: [] }, + expandedUri: "x://only-a", + }); + const { unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://{a,b}" }), + { a: "only-a", b: "" }, + ); + // The untouched field is dropped before the read: `""` is a *defined* + // RFC 6570 value that would expand to a valueless pair, and ink-form + // cannot distinguish "never touched" from "deliberately empty". + expect(read).toHaveBeenCalledWith("x://{a,b}", { a: "only-a" }); + unmount(); + }); + + it("does not block a template whose only expression is omittable", async () => { + const read = vi.fn().mockResolvedValue({ + result: { contents: [] }, + expandedUri: "x://events", + }); + const { unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://events{?topic}" }), + { topic: "" }, + ); + expect(read).toHaveBeenCalled(); + unmount(); + }); + }); + it("renders the form initially without invoking the client", async () => { const read = vi.fn(); const api = render( diff --git a/clients/tui/__tests__/uriTemplateToForm.test.ts b/clients/tui/__tests__/uriTemplateToForm.test.ts index 1c522c659..c44b6b370 100644 --- a/clients/tui/__tests__/uriTemplateToForm.test.ts +++ b/clients/tui/__tests__/uriTemplateToForm.test.ts @@ -11,7 +11,38 @@ describe("uriTemplateToForm", () => { expect(form.title).toBe("Read Resource: file"); const fields = form.sections[0]!.fields; expect(fields.map((f) => f.name)).toEqual(["path", "name"]); - expect(fields[0]).toMatchObject({ type: "string", required: false }); + // Simple `{path}` sits mid-URI, so omitting it would leave an empty path + // segment rather than a shorter URI -- it is mandatory, matching the web + // panel. Both clients read this from core's `requiredGroups`. + expect(fields[0]).toMatchObject({ type: "string", required: true }); + }); + + it("names fields as the expander looks them up, not as the SDK parses them", () => { + // The SDK reports these as ";id" and "id:3"; submitting under those keys + // would make the expander find nothing and drop the expression (#1919). + expect( + uriTemplateToForm("x://a{;id}", "matrix").sections[0]!.fields.map( + (f) => f.name, + ), + ).toEqual(["id"]); + expect( + uriTemplateToForm("x://a/{id:3}", "prefix").sections[0]!.fields.map( + (f) => f.name, + ), + ).toEqual(["id"]); + }); + + it("leaves a shared required group optional rather than demanding every name", () => { + // `{a,b}` is satisfied by either name, and ink-form cannot say "any one + // of"; marking both required would refuse input the expander accepts. + const fields = uriTemplateToForm("x://{a,b}", "pair").sections[0]!.fields; + expect(fields.map((f) => f.name)).toEqual(["a", "b"]); + expect(fields.every((f) => f.required === false)).toBe(true); + }); + + it("marks an omittable variable optional", () => { + const fields = uriTemplateToForm("x://a{?topic}", "q").sections[0]!.fields; + expect(fields[0]).toMatchObject({ name: "topic", required: false }); }); it("returns an empty Template Variables section for a static URI", () => { @@ -33,3 +64,15 @@ describe("uriTemplateToForm", () => { expect(form.sections[0]!.fields).toEqual([]); }); }); + +describe("a name repeated inside one expression", () => { + it("is required, not treated as a shared group", () => { + // `{a,a}` is one requirement named twice. Before core deduplicated the + // group, `requiredGroups` returned ["a","a"], so this form's + // `length === 1` test left the field optional while ResourceTestModal's + // submit guard still refused a blank -- an un-submittable form. + const [field] = uriTemplateToForm("x://{a,a}", "T").sections[0].fields; + expect(field.name).toBe("a"); + expect(field.required).toBe(true); + }); +}); diff --git a/clients/tui/src/components/ResourceTestModal.tsx b/clients/tui/src/components/ResourceTestModal.tsx index 914b4bca6..87c3ddeb2 100644 --- a/clients/tui/src/components/ResourceTestModal.tsx +++ b/clients/tui/src/components/ResourceTestModal.tsx @@ -5,6 +5,11 @@ import { InspectorClient } from "@inspector/core/mcp/index.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import type { ReadResourceResult } from "@modelcontextprotocol/client"; import { uriTemplateToForm } from "../utils/uriTemplateToForm.js"; +import { + definedValues, + requiredGroups, + unmetRequiredGroups, +} from "@inspector/core/mcp/uriTemplate.js"; import { ScrollView, type ScrollViewRef } from "ink-scroll-view"; // Helper to extract error message from various error types @@ -133,14 +138,45 @@ export function ResourceTestModal({ const handleFormSubmit = async (values: Record) => { if (!inspectorClient || !template) return; + // RFC 6570 keeps a required expression satisfied by any ONE of its names, + // so `{a,b}` cannot be expressed with ink-form's per-field `required` flag + // and its members are left optional there. Without this check the TUI would + // submit `{a,b}` completely blank -- dropping the expression and reading a + // different resource -- while the web panel blocks the same request (#1919). + // The message and the gate come from the SAME pass. Filtering the groups + // again here with a bare `values[name]` disagreed with the gate for a + // variable named `constructor` or `toString`: the inherited member read as + // filled, so the list came out empty and the error named no field at all. + const unmetGroups = unmetRequiredGroups( + requiredGroups(template.uriTemplate), + values, + ); + if (unmetGroups.length > 0) { + const unmet = unmetGroups.map((names) => names.join(" or ")); + setResult({ + input: values, + output: null, + error: `Missing required template variable(s): ${unmet.join(", ")}`, + duration: 0, + uri: template.uriTemplate, + }); + setState("results"); + return; + } + setState("loading"); const startTime = Date.now(); try { // Use InspectorClient's readResourceFromTemplate method which encapsulates template expansion and resource reading + // Blanks are dropped HERE, not in the expander: a key present with `""` + // is a defined RFC 6570 value that legitimately expands to `?topic=`, + // but ink-form hands back `""` for every field the user never touched, + // so this form cannot tell the two apart. The web panel does the same at + // its own boundary. const invocation = await inspectorClient.readResourceFromTemplate( template.uriTemplate, - values, + definedValues(values), ); const duration = Date.now() - startTime; diff --git a/clients/tui/src/utils/uriTemplateToForm.ts b/clients/tui/src/utils/uriTemplateToForm.ts index c8a027e9c..af580a984 100644 --- a/clients/tui/src/utils/uriTemplateToForm.ts +++ b/clients/tui/src/utils/uriTemplateToForm.ts @@ -4,32 +4,50 @@ import type { FormStructure, FormSection, FormField } from "ink-form"; import { UriTemplate } from "@modelcontextprotocol/client"; +import { + requiredGroups, + templateVariables, +} from "@inspector/core/mcp/uriTemplate.js"; /** - * Converts a URI Template to ink-form structure + * Converts a URI Template to ink-form structure. + * + * Fields come from core's `templateVariables`, the same parser + * `InspectorClient.readResourceFromTemplate` expands through, so the key this + * form submits is the key the expander looks up. Using the SDK's + * `variableNames` here instead is not merely untidy -- it mangles the name: + * `{;id}` yields `";id"` and `{id:3}` yields `"id:3"`, so the form would submit + * `{ ";id": "7" }` while the expander looks for `id`, silently dropping the + * value and the whole expression with it (#1919). + * + * The SDK template is still constructed, but only to validate: core's parser is + * deliberately lenient (an unclosed `{` becomes literal text), so this is what + * still surfaces a malformed template as an empty form plus a logged error. */ export function uriTemplateToForm( uriTemplate: string, templateName: string, ): FormStructure { - const fields: FormField[] = []; + let fields: FormField[] = []; try { - const template = new UriTemplate(uriTemplate); - /* v8 ignore next -- UriTemplate.variableNames is a getter that always - returns a string[]; the `|| []` fallback is an unreachable guard. */ - const variableNames = template.variableNames || []; - - for (const variableName of variableNames) { - const field: FormField = { - name: variableName, - label: variableName, - type: "string", - required: false, // URI template variables are typically optional - }; - - fields.push(field); - } + new UriTemplate(uriTemplate); + // Only a variable that is the *sole* member of a non-omittable expression + // is genuinely mandatory. RFC 6570 drops undefined names from a multi-name + // expression, so `{a,b}` needs only one of the two -- ink-form cannot + // express "any one of these", and marking both required would refuse input + // the expander accepts. + const mandatory = new Set( + requiredGroups(uriTemplate) + .filter((names) => names.length === 1) + .map(([name]) => name), + ); + fields = templateVariables(uriTemplate).map(({ name }) => ({ + name, + label: name, + type: "string", + required: mandatory.has(name), + })); } catch (error) { // If parsing fails, return empty form console.error("Failed to parse URI template:", error); diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx index 596a9340f..083a11c63 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx @@ -27,6 +27,11 @@ const noVarTemplate: ResourceTemplate = { uriTemplate: "file:///static.txt", }; +const queryTemplate: ResourceTemplate = { + name: "Events", + uriTemplate: "foobar://events{?topic}", +}; + describe("ResourceTemplatePanel", () => { it("renders the template title (or name) and description", () => { renderWithMantine( @@ -154,6 +159,200 @@ describe("ResourceTemplatePanel", () => { ).not.toBeDisabled(); }); + describe("RFC 6570 expansion (#1919)", () => { + it("renders an input for a query expression the old regex could not see", () => { + renderWithMantine( + , + ); + expect(screen.getByLabelText("topic")).toBeInTheDocument(); + }); + + it("percent-encodes a reserved character in a simple variable", async () => { + const user = userEvent.setup(); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + await user.type(screen.getByLabelText("userId"), "foo/bar"); + await user.click(screen.getByRole("button", { name: "Read Resource" })); + expect(onReadResource).toHaveBeenCalledWith( + "file:///users/foo%2Fbar/profile", + ); + }); + + it("builds an encoded query expression for {?topic}", async () => { + const user = userEvent.setup(); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + await user.type(screen.getByLabelText("topic"), "foo/bar"); + await user.click(screen.getByRole("button", { name: "Read Resource" })); + expect(onReadResource).toHaveBeenCalledWith( + "foobar://events?topic=foo%2Fbar", + ); + }); + + it("marks a query variable Optional and does not gate the read on it", async () => { + const user = userEvent.setup(); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + expect(screen.getByText("Optional")).toBeInTheDocument(); + const button = screen.getByRole("button", { name: "Read Resource" }); + expect(button).not.toBeDisabled(); + // Left blank, the whole expression drops out per RFC 6570. + await user.click(button); + expect(onReadResource).toHaveBeenCalledWith("foobar://events"); + }); + + it("does not mark a required simple variable Optional", () => { + renderWithMantine( + , + ); + expect(screen.queryByText("Optional")).not.toBeInTheDocument(); + }); + + it("previews the query expression verbatim until it is filled", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + expect(screen.getByText("foobar://events{?topic}")).toBeInTheDocument(); + await user.type(screen.getByLabelText("topic"), "news"); + expect( + screen.getByText("foobar://events?topic=news"), + ).toBeInTheDocument(); + }); + + // A template the server advertised can be malformed. There is no URI to + // send for one, so the read is withheld and the reason shown -- reading the + // raw template with its braces intact would draw a confusing "not found" + // from the server for a defect that is not the user's. + it.each([ + ["an out-of-grammar prefix modifier", "x://items/{id:abc}"], + ["an expression declaring no variable", "x://items/{}"], + ])("refuses to read a template with %s", (_label, uriTemplate) => { + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + expect( + screen.getByRole("button", { name: "Read Resource" }), + ).toBeDisabled(); + expect(screen.getByText(/Invalid RFC 6570 varspec/)).toBeInTheDocument(); + expect(onReadResource).not.toHaveBeenCalled(); + }); + + it("names every unmet group, so a disabled button always has a reason", async () => { + // With `{a,b}{b,c}{a,c}`, filling `b` satisfies the first two groups. A + // per-field hint then looks satisfied while Read Resource stays disabled + // on the third -- so the outstanding requirement is stated form-level. + const user = userEvent.setup(); + renderWithMantine( + , + ); + expect(screen.getByText(/Still needed:/)).toHaveTextContent( + "Still needed: a or b; b or c; a or c", + ); + await user.type(screen.getByLabelText("b"), "1"); + expect(screen.getByText(/Still needed:/)).toHaveTextContent( + "Still needed: a or c", + ); + expect( + screen.getByRole("button", { name: "Read Resource" }), + ).toBeDisabled(); + await user.type(screen.getByLabelText("a"), "2"); + expect(screen.queryByText(/Still needed:/)).not.toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Read Resource" }), + ).toBeEnabled(); + }); + + it("hints every shared group a variable sits in, not just the first", () => { + renderWithMantine( + , + ); + expect( + screen.getByText("Any one of each: a, b; a, c"), + ).toBeInTheDocument(); + }); + + it("treats a name repeated in one expression as a plain requirement", () => { + renderWithMantine( + , + ); + // Not "Any one of: a, a" -- it is one field, required. + expect(screen.queryByText(/Any one of/)).not.toBeInTheDocument(); + expect(screen.getByText("Still needed: a")).toBeInTheDocument(); + }); + + it("renders an autocomplete field for a name that collides with Object.prototype", async () => { + // `toString` is a valid RFC 6570 varname and `completions` starts empty, + // so a bare `completions[varName] ?? []` handed Mantine the prototype's + // *function* as its options array (`??` catches only null/undefined) and + // the field crashed on first render. + const user = userEvent.setup(); + const onCompleteArgument = vi.fn().mockResolvedValue(["a", "b"]); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + // By role, not label: "toString" also appears in the "Still needed" line. + const input = screen.getByRole("textbox", { name: "toString" }); + expect(input).toBeInTheDocument(); + await user.type(input, "v"); + await user.click(screen.getByRole("button", { name: "Read Resource" })); + expect(onReadResource).toHaveBeenCalledWith("x://v"); + }); + + it("shows the malformed template unexpanded in the preview", () => { + renderWithMantine( + , + ); + expect(screen.getByText("x://items/{}")).toBeInTheDocument(); + }); + }); + describe("completions", () => { it("fires a completion immediately on focus before any keystroke", async () => { const user = userEvent.setup(); diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index 6b506228b..e38845832 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -14,6 +14,14 @@ import { useValueChange } from "../../../hooks/useValueChange"; import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprotocol/client"; import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; +import { + definedValues, + previewUriTemplate, + requiredGroups, + templateVariables, + tryExpandUriTemplate, + unmetRequiredGroups, +} from "../../../utils/uriTemplate"; export interface ResourceTemplatePanelProps { template: ResourceTemplate; @@ -39,32 +47,22 @@ export interface ResourceTemplatePanelProps { const COMPLETION_DEBOUNCE_MS = 300; -function parseVariableNames(uriTemplate: string): string[] { - const names: string[] = []; - const regex = /\{(\w+)\}/g; - let match: RegExpExecArray | null; - - while ((match = regex.exec(uriTemplate)) !== null) { - names.push(match[1]); - } - - return names; -} - -function resolveUri( - uriTemplate: string, - variables: Record, -): string { - return uriTemplate.replace(/\{(\w+)\}/g, (_, key: string) => variables[key]); -} - -function previewUri( - uriTemplate: string, - variables: Record, -): string { - return uriTemplate.replace(/\{(\w+)\}/g, (match, key: string) => - variables[key]?.length > 0 ? variables[key] : match, - ); +/** + * The completions fetched for one variable, ignoring anything inherited from + * `Object.prototype`. + * + * `toString`, `constructor` and `__proto__` are valid RFC 6570 variable names, + * and this map starts empty — so a bare `completions[varName] ?? []` returned + * the prototype's *function* for such a name (`??` only catches null and + * undefined) and handed it to Mantine as its `data` array, crashing the field + * on first render. Same hazard the expansion path fixed for values; this is + * the one place the component reads a name-keyed map it did not seed. + */ +function completionsFor( + completions: Record, + varName: string, +): string[] { + return Object.hasOwn(completions, varName) ? completions[varName] : []; } const HeaderRow = Group.withProps({ @@ -90,6 +88,26 @@ const DescriptionText = Text.withProps({ c: "dimmed", }); +// Why Read Resource is disabled when the template itself cannot be expanded. +// Without it the button is inert with nothing on screen explaining the refusal. +// +// `role="alert"` because the message can appear *while typing* -- a pasted +// value that cannot be encoded -- so a screen-reader user would otherwise meet +// a silently disabled action. An alert is an assertive live region, which is +// what announces text that arrives after first render. +// What is still missing before Read Resource can fire. Dimmed rather than red: +// an incomplete form is the expected starting state, not an error. +const RequirementText = Text.withProps({ + size: "sm", + c: "dimmed", +}); + +const ExpansionErrorText = Text.withProps({ + size: "sm", + c: "red", + role: "alert", +}); + // Left-aligned so the action sits closest to the sidebar controls / the form // fields above; annotation badges trail it. const FooterRow = Group.withProps({ @@ -108,10 +126,20 @@ export function ResourceTemplatePanel({ }: ResourceTemplatePanelProps) { const { name, title, uriTemplate, description, annotations } = template; - const variableNames = useMemo( - () => parseVariableNames(uriTemplate), + // Every variable the template declares, with the operator it appears under + // and whether omitting it would change the URI's shape (see `utils/uriTemplate`). + const declaredVariables = useMemo( + () => templateVariables(uriTemplate), [uriTemplate], ); + const variableNames = useMemo( + () => declaredVariables.map((v) => v.name), + [declaredVariables], + ); + // The names of each expression that cannot be omitted. Kept separate from + // `declaredVariables` (which is deduplicated for rendering) because each + // required expression has to be satisfied on its own — see `requiredGroups`. + const groups = useMemo(() => requiredGroups(uriTemplate), [uriTemplate]); const [variables, setVariables] = useState>(() => Object.fromEntries(variableNames.map((n) => [n, ""])), @@ -196,7 +224,8 @@ export function ResourceTemplatePanel({ // show ghost suggestions from the old keystroke while the new // request is in flight (300ms debounce + network latency). setCompletions((prev) => { - if (prev[varName] === undefined) return prev; + // Own-property, as above: an inherited member is not a stale dropdown. + if (!Object.hasOwn(prev, varName)) return prev; const next = { ...prev }; delete next[varName]; return next; @@ -232,13 +261,40 @@ export function ResourceTemplatePanel({ void runCompletion(varName, value, buildContext(varName)); } - const canSubmit = variableNames.every((n) => variables[n]?.length > 0); + // Two independent gates on the read. + // + // First, whether the values cover what expansion structurally needs. Only the + // expressions whose absence would change the URI's shape count; an unfilled + // `{?topic}` is a legitimate request for the unfiltered resource, and RFC 6570 + // drops the whole expression for it. The rule is per-expression rather than + // per-variable -- `{a,b}` with only `a` filled expands to `a`'s value -- so it + // lives in core beside the expander. + // + // Second, whether the template expands at all. A template the server + // advertised can be malformed (`{id:abc}`, `{a,}`) and a pasted value can be + // unencodable, and in neither case is there a URI to send -- so withhold the + // request and say why, rather than reading the raw template with its braces + // intact and letting the server answer with a confusing "not found". + // `definedValues` is applied here rather than inside the expander: a key + // present with `""` is a *defined* RFC 6570 value and legitimately expands to + // `?topic=`, but this form seeds every declared variable with `""`, so an + // untouched field is indistinguishable from a deliberately empty one. That is + // a fact about the form, so the form is what resolves it. + const expansion = tryExpandUriTemplate(uriTemplate, definedValues(variables)); + // Named rather than merely counted, so a disabled Read Resource always has a + // reason on screen. Per-field hints cannot carry this: a variable shared + // across several groups looks satisfied once any one of them is met. + const unmet = unmetRequiredGroups(groups, variables); + const canSubmit = expansion.error === undefined && unmet.length === 0; function handleSubmit() { - onReadResource(resolveUri(uriTemplate, variables)); + /* v8 ignore next -- unreachable: the button is disabled whenever the + expansion failed, which is the only way `uri` is undefined. */ + if (expansion.uri === undefined) return; + onReadResource(expansion.uri); } - const preview = previewUri(uriTemplate, variables); + const preview = previewUriTemplate(uriTemplate, variables); return ( @@ -251,16 +307,47 @@ export function ResourceTemplatePanel({ {description && {description}} - {variableNames.map((varName) => { + {declaredVariables.map(({ name: varName, required }) => { /* v8 ignore next -- `?? ""` fallback unreachable: `variables` is seeded with every declared variable, so the key is always present. */ const fieldValue = variables[varName] ?? ""; + // RFC 6570 omits an undefined variable under a query/path-segment + // operator entirely, so those fields are genuinely optional. In a + // required multi-name expression no single field is mandatory either + // -- any one of them satisfies it -- so say which, rather than + // marking each one required and blocking valid input. + // A name can sit in a singleton required group *and* a shared one + // (`x://{a}/{a,b}`). The singleton demands this exact field, so the + // "any one of" hint would contradict the disabled submit button -- + // suppress it and let the field read as plainly required. + const individuallyRequired = groups.some( + (names) => names.length === 1 && names[0] === varName, + ); + // EVERY shared group this variable sits in, not the first. With + // `{a,b}{b,c}{a,c}`, showing only the first left `b` looking like it + // satisfied everything while Read Resource stayed disabled on the + // unmet `{a,c}` -- a hidden requirement is worse than a wordy hint. + // The form-level "Still needed" line below names what is actually + // outstanding, which is the part a per-field hint cannot express. + const sharedGroups = individuallyRequired + ? [] + : groups.filter( + (names) => names.length > 1 && names.includes(varName), + ); + const description = !required + ? "Optional" + : sharedGroups.length > 0 + ? `Any one of${sharedGroups.length > 1 ? " each" : ""}: ${sharedGroups + .map((names) => names.join(", ")) + .join("; ")}` + : undefined; return useAutocomplete ? ( @@ -290,6 +378,14 @@ export function ResourceTemplatePanel({ ); })} + {expansion.error !== undefined && ( + {expansion.error} + )} + {expansion.error === undefined && unmet.length > 0 && ( + + Still needed: {unmet.map((names) => names.join(" or ")).join("; ")} + + )}