diff --git a/AGENTS.md b/AGENTS.md index aa442e66..d85e29a0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -95,7 +95,11 @@ v2/main/ │ │ # 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 — #1919; +│ │ # 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; diff --git a/README.md b/README.md index 0f29e549..3642aece 100644 --- a/README.md +++ b/README.md @@ -265,6 +265,8 @@ The `;` and `:3` rows are the ones a user sees directly: on the SDK's parse the 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. +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 diff --git a/clients/tui/__tests__/ResourceTestModal.test.tsx b/clients/tui/__tests__/ResourceTestModal.test.tsx index 26dfdc4d..525051a0 100644 --- a/clients/tui/__tests__/ResourceTestModal.test.tsx +++ b/clients/tui/__tests__/ResourceTestModal.test.tsx @@ -122,7 +122,10 @@ describe("ResourceTestModal", () => { makeTemplate({ uriTemplate: "x://{a,b}" }), { a: "only-a", b: "" }, ); - expect(read).toHaveBeenCalledWith("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(); }); diff --git a/clients/tui/src/components/ResourceTestModal.tsx b/clients/tui/src/components/ResourceTestModal.tsx index 0ac77a5d..87c3ddeb 100644 --- a/clients/tui/src/components/ResourceTestModal.tsx +++ b/clients/tui/src/components/ResourceTestModal.tsx @@ -6,6 +6,7 @@ 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"; @@ -168,9 +169,14 @@ export function ResourceTestModal({ 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/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index cb6a1589..16db62e7 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -15,6 +15,7 @@ import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprot import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; import { + definedValues, hasRequiredValues, previewUriTemplate, requiredGroups, @@ -71,9 +72,15 @@ const DescriptionText = Text.withProps({ // 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. const ExpansionErrorText = Text.withProps({ size: "sm", c: "red", + role: "alert", }); // Left-aligned so the action sits closest to the sidebar controls / the form @@ -242,7 +249,12 @@ export function ResourceTemplatePanel({ // 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". - const expansion = tryExpandUriTemplate(uriTemplate, variables); + // `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)); const canSubmit = expansion.error === undefined && hasRequiredValues(groups, variables); diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index 7c9d5241..596f3aae 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from "vitest"; import { + definedValues, expandUriTemplate, expandUriTemplateStrict, hasRequiredValues, @@ -135,10 +136,18 @@ describe("expandUriTemplate", () => { expect(expandUriTemplate("x://{+path}", { path: "a/b" })).toBe("x://a/b"); }); - it("omits an expression whose variable was left blank", () => { - expect(expandUriTemplate("foobar://events{?topic}", { topic: "" })).toBe( + it("omits an expression whose variable is undefined", () => { + expect(expandUriTemplate("foobar://events{?topic}", {})).toBe( "foobar://events", ); + // A form's untouched field arrives as "", which is a *defined* value; the + // form drops it on the way in. See "a value defined as the empty string". + expect( + expandUriTemplate( + "foobar://events{?topic}", + definedValues({ topic: "" }), + ), + ).toBe("foobar://events"); }); it("expands each query expression independently, per RFC 6570", () => { @@ -288,9 +297,9 @@ describe("requiredGroups / hasRequiredValues", () => { const groups = requiredGroups("x://{a,b}"); expect(groups).toEqual([["a", "b"]]); expect(hasRequiredValues(groups, { a: "only-a", b: "" })).toBe(true); - expect(expandUriTemplate("x://{a,b}", { a: "only-a", b: "" })).toBe( - "x://only-a", - ); + expect( + expandUriTemplate("x://{a,b}", definedValues({ a: "only-a", b: "" })), + ).toBe("x://only-a"); }); it("rejects a multi-name expression with nothing filled", () => { @@ -480,11 +489,15 @@ describe("variable names that collide with Object.prototype", () => { // every one of them, so a *blank* field read as supplied: measured, // `({})["toString"] !== undefined` is true and its typeof is "function". it.each(["toString", "constructor", "valueOf", "hasOwnProperty"])( - "omits a blank {?%s} instead of expanding a prototype member", + "reads a blank {?%s} as its own empty value, not a prototype member", (name) => { + // The own value is "", so the expression expands to a valueless pair -- + // NOT the inherited function's body, which is what a bare lookup gave. expect(expandUriTemplate(`x://a{?${name}}`, { [name]: "" })).toBe( - "x://a", + `x://a?${name}=`, ); + // And with the key absent, the expression is omitted entirely. + expect(expandUriTemplate(`x://a{?${name}}`, {})).toBe("x://a"); }, ); @@ -628,6 +641,38 @@ describe("unmetRequiredGroups", () => { }); }); +describe("a value defined as the empty string", () => { + // RFC 6570 distinguishes an *undefined* variable (the expression is omitted) + // from one defined as "". The expander used to collapse the two, which made + // these URIs unrequestable through any caller -- `readResourceFromTemplate` + // included. Dropping blanks is a *form* concern (a text input cannot express + // "defined but empty"), so each client applies `definedValues` on its way in. + it.each([ + ["a query expression keeps the =", "x{?q}", "x?q="], + ["a continuation keeps the =", "x?a=1{&q}", "x?a=1&q="], + // RFC 6570 3.2.7: the matrix operator drops the `=` for an empty value. + ["the matrix operator drops the =", "x{;q}", "x;q"], + ["a path segment expands to a bare separator", "x{/q}", "x/"], + ["a label expands to a bare separator", "x{.q}", "x."], + ["a simple expression contributes nothing visible", "x/{q}", "x/"], + ])("%s", (_label, template, expected) => { + expect(expandUriTemplate(template, { q: "" })).toBe(expected); + }); + + it("omits the expression only when the key is absent", () => { + expect(expandUriTemplate("x{?q}", {})).toBe("x"); + expect(expandUriTemplate("x{;q}", {})).toBe("x"); + }); + + it("keeps a defined-but-empty name alongside a filled one", () => { + expect(expandUriTemplate("x{?a,b}", { a: "", b: "2" })).toBe("x?a=&b=2"); + }); + + it("definedValues is what a form applies to drop its untouched fields", () => { + expect(expandUriTemplate("x{?q}", definedValues({ q: "" }))).toBe("x"); + }); +}); + describe("tryExpandUriTemplate", () => { it("returns the URI and no error when the template expands", () => { expect( diff --git a/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts index c79872c8..77b54eae 100644 --- a/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts +++ b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts @@ -4,6 +4,7 @@ import { fileURLToPath } from "node:url"; import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; import { + definedValues, expandUriTemplate, tryExpandUriTemplate, } from "@inspector/core/mcp/uriTemplate.js"; @@ -144,12 +145,18 @@ describe("RFC 6570 resource templates over the wire (#1919)", () => { it("resolves the base URI a blank query expression expands to", async () => { const connected = await connectToShowcase(); - // RFC 6570 drops the whole expression when the variable is undefined, so + // RFC 6570 drops the whole expression when the variable is *undefined*, so // an unfilled `topic` legitimately requests the unfiltered collection. The + // form is what turns its untouched blank into "undefined" (`definedValues`) + // — a key present with "" is a defined value and would expand to + // `?topic=`, which is a different (and here, unregistered) resource. The // SDK's matcher compiles `{?topic}` to a *required* `\?topic=([^&]+)`, so a // template alone cannot serve this — the config registers the plain // resource, which is what a real server would do. - const uri = expandUriTemplate("foobar://events{?topic}", { topic: "" }); + const uri = expandUriTemplate( + "foobar://events{?topic}", + definedValues({ topic: "" }), + ); expect(uri).toBe("foobar://events"); const { result } = await connected.readResource(uri); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 54114c3e..941ef23c 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -17,6 +17,7 @@ import { } from "@inspector/core/mcp/uriTemplate.js"; export { + definedValues, expandUriTemplate, hasRequiredValues, parseUriTemplate, diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 5150ac37..a9278e81 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -353,6 +353,18 @@ function readValue( * Drops empty entries so an untouched optional field reads as *undefined* * (the expression disappears) rather than as the empty string (which would * expand to a valueless `?topic=`). + * + * **A UI-boundary concern, deliberately not part of the expander.** RFC 6570 + * distinguishes an undefined variable (the expression is omitted) from one + * defined as `""` (it expands — `x{?q}` gives `x?q=`, `x{;q}` gives `x;q`), and + * collapsing the two inside {@link expandUriTemplateStrict} made those URIs + * unrequestable through any caller, `readResourceFromTemplate` included. + * + * What forces the collapse is a *form*: both clients seed every declared + * variable with `""`, and a text input cannot express "defined but empty", so + * an untouched field is indistinguishable from a deliberately empty one. That + * is a property of the form, not of the template — so each form applies this + * on its way in, and the expander honors exactly what it is handed. */ export function definedValues( values: Record, @@ -461,9 +473,17 @@ function expandExpression( const { operator } = part; if (NAMED_OPERATORS.has(operator)) { - const pairs = present.map( - ({ spec, value }) => `${spec.name}=${renderValue(value, spec, operator)}`, - ); + const pairs = present.map(({ spec, value }) => { + const rendered = renderValue(value, spec, operator); + // RFC 6570 §3.2.7: the matrix operator drops the `=` for an empty value, + // so `x{;q}` with `q = ""` is `x;q` — while `?`/`&` keep it (`x?q=`). + // Both are only reachable now that the expander honors a defined-but- + // empty value instead of collapsing it into "undefined"; see + // `definedValues` for where that collapse legitimately happens instead. + return operator === ";" && rendered.length === 0 + ? spec.name + : `${spec.name}=${rendered}`; + }); // `;` repeats its separator per pair; `?`/`&` join with `&`. return operator === ";" ? `;${pairs.join(";")}` @@ -516,9 +536,15 @@ export function expandParts( } /** - * Expands a template against the entered values per RFC 6570 — percent-encoding - * each value according to its operator, and omitting expressions whose - * variables were left blank. **Throws** on a template that is not valid. + * Expands a template against the supplied values per RFC 6570 — percent-encoding + * each value according to its operator, and omitting an expression none of + * whose variables are defined. **Throws** on a template that is not valid. + * + * A key present with an empty string is *defined*, and expands: `x{?q}` gives + * `x?q=` and `x{;q}` gives `x;q`. Only an absent key omits its expression. A + * form that cannot tell "untouched" from "deliberately empty" drops its blanks + * with {@link definedValues} before calling — which is where that judgement + * belongs, since it is a fact about the form rather than about the template. * * Every expression is expanded by {@link expandParts}; the SDK's `UriTemplate` * is constructed only because that is what rejects an unclosed expression, and @@ -535,7 +561,6 @@ export function expandUriTemplateStrict( uriTemplate: string, values: Record, ): string { - const defined = definedValues(values); new UriTemplate(uriTemplate); const parts = parseUriTemplate(uriTemplate); const bad = parts.find((part) => part.kind === "expression" && part.invalid); @@ -546,7 +571,7 @@ export function expandUriTemplateStrict( } — each varspec must name a variable, optionally with an explode (*) or a 1-9999 prefix modifier.`, ); } - return expandParts(parts, defined); + return expandParts(parts, values); } /**