fix: honor a defined-but-empty value, and announce the expansion error
Addresses Copilot's round-8 review on #2035 (one inline comment, one suppressed). Both real. 1. RFC 6570 distinguishes an UNDEFINED variable, which omits its whole expression, from one defined as the empty string, which expands. The expander was calling `definedValues` on the way in, collapsing the two -- so `x{?q}` with `{ q: "" }` gave `x` instead of `x?q=`, and there was no way for any caller, `readResourceFromTemplate` included, to request either that or `x;q`. Key presence is now honored, including §3.2.7's rule that the matrix operator drops the `=` for an empty value (`x{;q}` -> `x;q`, while `?`/`&` keep it). Dropping blanks is a FORM concern, so it moved to the two form boundaries: the web panel seeds every declared variable with "" and the TUI's ink-form returns "" for every untouched field, so neither can tell "never touched" from "deliberately empty" -- but that is a fact about a text input, not about the template, and it has no business deciding what a template means for every other caller. Six unit tests and one integration test encoded the old collapse and now assert the corrected meaning. The prototype-collision cases get stronger rather than weaker: a blank `{?toString}` expands to `?toString=` -- its OWN empty value -- where the original bug expanded the inherited function's body. 2. The expansion error is now `role="alert"`. It can appear while typing (a pasted value that cannot be encoded), and plain text is not announced, so a screen-reader user met a disabled Read Resource with no stated reason. npm run ci passes. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
1c24eda429
commit
6acaff70ca
9 changed files with 126 additions and 21 deletions
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@ import {
|
|||
} from "@inspector/core/mcp/uriTemplate.js";
|
||||
|
||||
export {
|
||||
definedValues,
|
||||
expandUriTemplate,
|
||||
hasRequiredValues,
|
||||
parseUriTemplate,
|
||||
|
|
|
|||
|
|
@ -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<string, string>,
|
||||
|
|
@ -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, string>,
|
||||
): 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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Reference in a new issue