diff --git a/AGENTS.md b/AGENTS.md index 4636e17e..aa442e66 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,7 +90,12 @@ v2/main/ │ │ # and double-encodes pct-triplets), and `{v}` │ │ # (encodeURIComponent leaves `!'()*` bare). │ │ # Requiredness is per EXPRESSION, not per variable — -│ │ # requiredGroups + hasRequiredValues — #1919; +│ │ # 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 — #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 c7fa48e0..ec2af96c 100644 --- a/README.md +++ b/README.md @@ -263,6 +263,8 @@ The SDK's `UriTemplate` is still used, but only to *validate* a template (constr 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. The panel disables Read Resource, prints the reason, and leaves the raw template in the preview. 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. + 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/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx index 4955b385..87b2ba15 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx @@ -243,6 +243,38 @@ describe("ResourceTemplatePanel", () => { 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("shows the malformed template unexpanded in the preview", () => { + renderWithMantine( + , + ); + expect(screen.getByText("x://items/{}")).toBeInTheDocument(); + }); }); describe("completions", () => { diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index 9c104821..cb6a1589 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -15,11 +15,11 @@ import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprot import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; import { - expandUriTemplate, hasRequiredValues, previewUriTemplate, requiredGroups, templateVariables, + tryExpandUriTemplate, } from "../../../utils/uriTemplate"; export interface ResourceTemplatePanelProps { @@ -69,6 +69,13 @@ 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. +const ExpansionErrorText = Text.withProps({ + size: "sm", + c: "red", +}); + // Left-aligned so the action sits closest to the sidebar controls / the form // fields above; annotation badges trail it. const FooterRow = Group.withProps({ @@ -221,15 +228,29 @@ export function ResourceTemplatePanel({ void runCompletion(varName, value, buildContext(varName)); } - // Only the expressions whose absence would change the URI's shape gate the - // read; 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. - const canSubmit = hasRequiredValues(groups, variables); + // 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". + const expansion = tryExpandUriTemplate(uriTemplate, variables); + const canSubmit = + expansion.error === undefined && hasRequiredValues(groups, variables); function handleSubmit() { - onReadResource(expandUriTemplate(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 = previewUriTemplate(uriTemplate, variables); @@ -308,6 +329,9 @@ export function ResourceTemplatePanel({ ); })} + {expansion.error !== undefined && ( + {expansion.error} + )}