fix: expand resource templates per RFC 6570 in the web client (#1919)

[cliffhall]

Closes #1919

Problem

The Resources tab discovered a template's variables with /\{(\w+)\}/g and substituted them with a plain String.replace. Two consequences, both reported in #1919:

1. That regex only matches a bare {name} expression. A query expression — foobar://events{?topic} — declared a variable the form never rendered an input for, and the enabled Read Resource button sent the unexpanded template.
2. Values were inserted verbatim. A topic of foo/bar produced foobar://events/foo/bar — a second path segment — instead of foobar://events/foo%2Fbar. A spec-compliant matcher rejects that with -32602 Resource not found.

Fix

Variable discovery, expansion, and the URI preview now go through the SDK's UriTemplate (clients/web/src/utils/uriTemplate.ts) — the same RFC 6570 implementation the TUI's form builder and InspectorClient.readResourceFromTemplate already used, so web, CLI, and TUI agree on what a template's variables are and on how a value is encoded. The web client was the only surface still doing string replacement.

The preview keeps showing {name} for a variable that hasn't been filled yet, via a sentinel built from RFC 3986 unreserved characters so it survives expansion untouched under every operator and can be swapped back out. A filled value is previewed exactly as it will be sent, encoding included.

Test server

test-servers/configs/rfc6570-templates-http.json (preset rfc6570_templates) serves both templates from the issue — foobar://events/{topic} and foobar://events{?topic} — each echoing back the topic it received and the URI that matched. The server rejects a non-matching URI, so the old behavior fails visibly rather than silently.

Screenshots

All three pairs captured against that server, headless, through the prod --web build.

Simple expression — a / in the value must be percent-encoded. Before, the preview showed the raw value; after, it shows what will actually be sent.

Before	After

The resulting read. Before: Read Error — Resource not found: foobar://events/foo/bar. After: the resource loads, and the server confirms it matched foobar://events/foo%2Fbar.

Before	After

Query expression. Before: no topic input at all, and Read Resource enabled anyway. After: an input, and a preview showing where the value lands.

Before	After

Query expression. After: data input, and displayed in the preview.

Tests

• clients/web/src/utils/uriTemplate.test.ts — new, 100% on all four dimensions. Covers every RFC 6570 operator for discovery ({+}, {#}, {/}, {.}, {?}, {&}), multi-variable and repeated-name expressions, the encoding cases the issue asks for (/, ?, #, %, space, Unicode), omission of unfilled variables, the preview's placeholder round-trip, and the malformed-template degradation.
• ResourceTemplatePanel.test.tsx — four regression tests: a query expression renders an input, expands correctly on submit, a reserved character is percent-encoded rather than becoming a path segment, and the preview shows the encoded value.
• New QueryExpression Storybook story.

npm run ci passes locally.

[Copilot]

Pull request overview

Adds RFC 6570-compliant resource-template discovery, expansion, encoding, and previews to the web client.

Changes:

• Introduces shared UriTemplate helpers and regression tests.
• Updates the resource-template panel and Storybook coverage.
• Adds an RFC 6570 test-server preset and documentation.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test-servers/src/test-server-fixtures.ts	Adds RFC 6570 resource fixtures.
test-servers/src/preset-registry.ts	Registers the new fixture preset.
test-servers/configs/rfc6570-templates-http.json	Configures the manual test server.
README.md	Documents RFC 6570 testing.
clients/web/src/utils/uriTemplate.ts	Adds template parsing, expansion, and preview helpers.
clients/web/src/utils/uriTemplate.test.ts	Tests helper behavior and encoding.
clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx	Uses the new helpers in the UI.
clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx	Adds UI regressions.
clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.stories.tsx	Adds a query-expression story.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Checked this against the SDK, and the stated failure mode doesn't reproduce — but it pointed at a boundary worth pinning, so I've added tests for it.

The SDK's UriTemplate doesn't implement prefix modifiers at all. It folds the modifier into the variable name rather than parsing it:

x://{topic:3}   variableNames: ["topic:3"]
                expand({"topic:3": "abcdef"})  → "x://abcdef"     (no truncation)
x://{?topic:3}  variableNames: ["topic:3"]
                expand({"topic:3": "abcdef"})  → "x://?topic:3=abcdef"

So expand never truncates the sentinel to zzI — there is no truncation step to hit. The sentinel round-trips intact and the preview restores {topic:3} like any other placeholder. (The x:// in a naive probe comes from keying the value as topic instead of topic:3, which just makes it an unset variable.)

Making the placeholder "modifier-aware" would therefore be building around a modifier the parser doesn't recognize — and the same parse is what the TUI's form builder and InspectorClient.readResourceFromTemplate already use, so any real modifier support belongs upstream in the SDK, not in three client-side workarounds.

What I did take from this:

• clients/web/src/utils/uriTemplate.test.ts — two regression tests, one on discovery ({topic:3} → ["topic:3"]) and one on the preview (x://{topic:3} → x://{topic:3}, x://{?topic:3} → x://?topic:3={topic:3}). If a future SDK bump starts parsing modifiers, these fail and point straight at the sentinel logic.
• templateVariableNames now documents the boundary in place, and notes that it's inherited from the SDK and shared with the other two clients.

Pushed in 1487404.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (2)

clients/web/src/utils/uriTemplate.ts:124

• The placeholder can collide with a real variable value. For example, previewTemplate("x://{a}/{b}", { a: "zzInspectorUnfilledzz1zz", b: "" }) expands both variables to the same token, and this replacement turns the preview into x://{b}/{b}, hiding the filled value even though the submitted URI remains different. Generate per-call placeholder tokens that are absent from the template and all filled values (and add a collision regression test) before restoring them.

  let preview = template.expand(values);
  names.forEach((name, index) => {
    if (filled[name] !== undefined) return;
    // The sentinel is unreserved, so it appears in the expansion unencoded.
    preview = preview.split(sentinelFor(index)).join(`{${name}}`);

test-servers/src/test-server-fixtures.ts:1485

• The new RFC 6570 server preset is not exercised by any automated test, so the claimed end-to-end behavior—both templates register, encoded/query URIs match, and an unencoded path is rejected—can regress while the component/helper unit tests still pass. Add an integration test that loads rfc6570-templates-http.json (or this factory), lists both templates, and reads/rejects the representative URIs.

export function createRfc6570ResourceTemplates(): ResourceTemplateDefinition[] {

[cliffhall]

Round 2 reported "no new comments" but carried two suppressed ones. Both were right — fixed in 0cc492d.

1. Placeholder could collide with a real value (uriTemplate.ts)

Correct, and reproduced exactly as described: previewTemplate("x://{a}/{b}", { a: "zzInspectorUnfilledzz1zz", b: "" }) rewrote the filled value into {b}, so the preview disagreed with the URI actually submitted.

The token base is now chosen per call — extended with zs until it appears in neither the template's literal text nor any filled value, so it is unambiguous for that expansion:

function uncollidingBase(uriTemplate: string, filled: Record<string, string>) {
  const haystack = [uriTemplate, ...Object.values(filled)].join("\n");
  let base = UNFILLED_SENTINEL;
  while (haystack.includes(base)) base += "z";
  return base;
}

Checking the raw inputs is sufficient rather than the encoded ones: percent-encoding can only emit % plus hex digits, and the base contains characters outside that set, so encoding can never synthesize the token out of other content. That reasoning is now a comment on the constant.

Two regression tests added — one for a colliding filled value, one for the token appearing as literal text in the template itself.

2. The repro server had no automated coverage (test-server-fixtures.ts)

Also correct, and the sharper half of the point: the helper's unit tests assert what expandTemplate produces, but nothing asserted that the produced URI is what a spec-compliant server accepts — which is the entire bug, since the old substitution emitted a URI the Inspector was perfectly happy with and the server rejected.

New clients/web/src/test/integration/mcp/rfc6570-templates.test.ts (6 tests) drives it against a real server over a real transport:

• both templates advertised, query expression included;
• foobar://events/foo%2Fbar resolves, and the server echoes back the URI it matched;
• foobar://events?topic=weather resolves;
• foobar://events/foo/bar — what the old code produced — is still refused. That last one is deliberate: without it a regression could pass by loosening the server rather than by fixing the client.

It builds the server by resolving the checked-in config (loadConfig → resolveConfig) rather than calling the factory, following the nullable-fields.test.ts precedent, so a misspelt preset name in preset-registry.ts or a config naming a dead preset fails here too.

npm run ci passes.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

clients/web/src/utils/uriTemplate.ts:68

• withoutEmptyValues changes RFC 6570 semantics for a defined empty string. RFC 6570 distinguishes an undefined variable from "": {?topic} with { topic: "" } expands to ?topic=, while only an absent key is omitted; the SDK already preserves that distinction. Pass variables through directly and update the unit test that currently locks in omission.

  const template = parseTemplate(uriTemplate);
  if (!template) return uriTemplate;
  return template.expand(withoutEmptyValues(variables));

[cliffhall]

Round 3 again reported "no new comments" with one suppressed. It's correct — fixed in bc1237c.

withoutEmptyValues collapsed RFC 6570's undefined-vs-empty-string distinction

Verified against the SDK, which does preserve it:

x://e{?topic}   { topic: "" } → "x://e?topic="     {} → "x://e"
x://e{&topic}   { topic: "" } → "x://e&topic="     {} → "x://e"

Filtering before expanding made a deliberately-empty value unexpressible through the helper. expandTemplate now passes values through untouched, so it is a faithful RFC 6570 expansion for any caller, and the doc comment states the distinction rather than the old (wrong) claim that omission was "what the spec prescribes".

One refinement on the suggestion: the empty-string-means-unfilled notion is still correct — just not in expandTemplate. It belongs to the preview, because the panel seeds every declared variable with "" and a text input has no way to express "defined but empty", so within the preview an empty string genuinely means "not entered yet" and should render {topic} rather than ?topic=. So the filter now lives only in previewTemplate, renamed enteredValues and documented as deliberately divergent from the expansion.

Tests updated accordingly:

• the test that locked in omission now asserts { topic: "" } → foobar://events?topic=;
• the absent-key test is unchanged ({?a,b} with only a → ?a=1);
• a new test asserts the two helpers differ here on purpose, so a future "consistency" cleanup that re-collapses them fails loudly:

expect(expandTemplate("x://e{?t}", { t: "" })).toBe("x://e?t=");
expect(previewTemplate("x://e{?t}", { t: "" })).toBe("x://e?t={t}");

No user-visible change: Read Resource is gated on every variable being non-empty, so the panel never reaches expandTemplate with one.

npm run ci passes.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (3)

clients/web/src/utils/uriTemplate.ts:72

• @modelcontextprotocol/client 2.0.0 does not apply RFC 6570 encoding or operators to non-query expressions containing multiple names: its part.names.length > 1 branch joins the raw values before the encoding/operator switch. Consequently, expandTemplate("x://{a,b}", { a: "foo/bar", b: "x y" }) produces x://foo/bar,x y rather than x://foo%2Fbar,x%20y, and {#a,b} also loses the #. Since this change now discovers both fields and allows submission, these templates generate an invalid URI. Please add multi-variable expansion tests and use or update an expander that handles this branch correctly.

  return template.expand(variables);

clients/web/src/utils/uriTemplate.ts:152

• UriTemplate.expand can throw even after parsing succeeds—for example, the pinned SDK rejects a variable value over 1,000,000 characters. This call runs while ResourceTemplatePanel renders its preview, and the text inputs have no matching limit, so pasting such a value throws out of render instead of showing validation; the submit path can throw similarly. Catch expansion failures and surface a controlled validation/error state rather than letting them escape.

  let preview = template.expand(values);

clients/web/src/utils/uriTemplate.ts:98

• This collision loop has quadratic worst-case behavior on a server-supplied template. For example, a template containing zzInspectorUnfilledzz followed by a long run of z characters makes every successively extended candidate collide, and each iteration rescans the full haystack; the SDK accepts templates up to 1 MB, so selecting such a resource can freeze the render thread. Compute the longest conflicting suffix in one pass or use another bounded collision strategy.

  while (haystack.includes(base)) base += "z";

[cliffhall]

Round 4: three suppressed comments, all three confirmed against the SDK and all three fixed in 81fbaac. This was the strongest round — thanks.

1. Multi-name expressions lose encoding and the operator

Confirmed, and the root cause is exactly where it was pointed. expandPart returns early:

if (part.names.length > 1) {
  const values = part.names.map((name) => variables[name]).filter((v) => v !== void 0);
  if (values.length === 0) return "";
  return values.map((v) => Array.isArray(v) ? v[0] : v).join(",");   // ← never reaches encodeValue or the operator switch
}

x://{a,b}   { a: "foo/bar", b: "x y" }  →  "x://foo/bar,x y"
x://{#a,b}                              →  "x://foo/bar,x y"    (the # is gone)
x://{?a,b}                              →  "x://?a=foo%2Fbar&b=x%20y"   (correct — different branch)

And the point about this PR exposing it is the right one: the old regex never matched {a,b}, so no inputs were rendered; discovery now offers them and lets an invalid URI be submitted.

Fixed without reimplementing RFC 6570. Such an expression is rewritten to a single synthetic variable holding an array of the defined values — which the SDK's single-name branch encodes elementwise and joins with the operator's separator, which is precisely the spec's rule for a multi-name expression. x://{a,b} becomes x://{__inspectorGroup0__} expanded with ["foo/bar", "x y"]. Templates without the shape are returned untouched and pay nothing.

Seven tests added covering the simple, +, #, ., and / operators, the query form (unchanged), a partially-defined group, and a fully-undefined one. One correction to the suggested expectation: {#a,b} is a reserved expansion like {+…}, so / is preserved and only the space is encoded — x://e#foo/bar,x%20y.

One thing worth flagging as out of reach here: UriTemplate.match cannot match a multi-name expression either — it returns null for every URI, including a correctly-encoded one. So an SDK-based server can never route such a template and there is no round trip to add to the integration suite. The client can still emit the spec-correct URI a conforming server needs, which is what the unit tests assert; a comment records why that coverage lives there rather than in the integration file. The complete fix is upstream in the SDK, on both the expand and match sides.

2. expand can throw after parsing succeeds

Confirmed — the ceiling is per value and checked at expansion time:

len=1000000 → ok
len=1000001 → throws "Variable value exceeds maximum length of 1000000 characters"

Both helpers are now total. expandTemplate returns string | null, and the panel gates on it:

const expandedUri = allFilled ? expandTemplate(uriTemplate, variables) : null;
const canSubmit = expandedUri !== null;

so Read Resource stays disabled rather than sending a URI we know is wrong. previewTemplate falls back to the raw template — it runs during render, where an escaping throw unmounts the panel. Tests cover both, plus a panel test driving the oversized value through fireEvent and asserting the button stays disabled.

3. Quadratic collision loop

Correct. Now a single pass: measure the longest run of z following any occurrence of the token and clear it by one, which no occurrence can then match.

for (let at = haystack.indexOf(UNFILLED_SENTINEL); at !== -1; at = haystack.indexOf(...)) {
  // measure the run of `z` immediately after
}
return UNFILLED_SENTINEL + "z".repeat(longestRun + 1);

Regression test uses a template with a 5,000-character z run, which the old loop would have rescanned 5,000 times.

npm run ci passes.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (4)

clients/web/src/utils/uriTemplate.ts:124

• Explode modifiers are retained in these group member names, but UriTemplate.variableNames strips *, so the form stores a while applyGroups looks up a*. For example, {/a*,b} silently drops a even when the user filled it. Normalize group names the same way as the SDK before projecting values.

      const names = body.split(",").map((name) => name.trim());

clients/web/src/utils/uriTemplate.ts:127

• The synthetic name can collide with a legitimate template variable. For x://{a,b}/{__inspectorGroup0__}, both rewritten expressions reference __inspectorGroup0__, and applyGroups overwrites the user's value, producing the group twice. Generate a name that is absent from all original variable names before rewriting.

      const synthetic = groupName(groups.size);
      groups.set(synthetic, names);
      return `{${operator}${synthetic}}`;

clients/web/src/utils/uriTemplate.ts:95

• The standard RFC 6570 ; operator is included here, but @modelcontextprotocol/client 2.0.0's UriTemplate does not recognize it. As a result, {;a} is treated as a variable literally named ;a and expands without the required ;a=, while {;a,b} is rewritten to a synthetic name that expands to nothing. Please either implement path-parameter expansion locally or reject these templates instead of sending a known-invalid URI, and cover both forms.

This issue also appears in the following locations of the same file:

• line 124
• line 125

const MULTI_NAME_EXPRESSION = /\{([+#./;]?)([^{}?&][^{}]*)\}/g;

clients/web/src/utils/uriTemplate.ts:208

• Advancing by the full token length skips overlapping occurrences because this token starts and ends with zz. A literal such as zzInspectorUnfilledzzInspectorUnfilledzzz0zz contains an overlapping occurrence with a longer trailing run; it is missed, so the chosen sentinel collides and the preview rewrites literal URI text into a placeholder. Advance by one character so overlaps are measured too.

    at = haystack.indexOf(UNFILLED_SENTINEL, at + UNFILLED_SENTINEL.length)

[cliffhall]

Round 5: four suppressed comments, all four in the machinery added last round, all four confirmed and fixed in 722f24d.

1. Explode modifiers broke group member lookup

Confirmed — variableNames strips a trailing * while my split kept it:

x://{/a*,b}   variableNames: ["a", "b"]     my group members: ["a*", "b"]

so the form stored a, applyGroups looked up a*, found nothing, and dropped a filled value. Member names are now normalized exactly the way the SDK does — strip a trailing *, keep a prefix modifier (a:3 stays a:3, matching the boundary pinned earlier in this PR). Mirroring rather than inventing a normalization is the point; diverging in either direction reintroduces the drop.

Test asserts both halves: templateVariableNames("x://e{/a*,b}") → ["a","b"], and the expansion → x://e/one/two.

2. Synthetic name could collide with a real variable

Confirmed. The prefix is now padded past any occurrence in the template, so x://{a,b}/{__inspectorGroup0__} expands to x://one,two/mine — the group and the user's own variable stay distinct. Regression test added.

3. The ; operator — I made it worse, not better

The sharpest of the four. Confirmed the premise:

x://e{;a}     variableNames: [";a"]        expand({a:"1"}) → "x://e1"    (no `;a=`)
x://e{;a,b}   variableNames: [";a","b"]

And the rewrite genuinely regressed it: {;a,b} became {;__inspectorGroup0__}, whose parsed name is ";__inspectorGroup0__", which applyGroups never sets — expanding to nothing.

Taking the "reject rather than send a known-invalid URI" option, since no arrangement of the SDK's branches produces the right output. ; is out of the rewrite's operator class, and a template using it is declined: expandTemplate returns null so the panel withholds the request, and the preview shows the template as the server declared it. Four tests: both forms return null, the preview form, and one asserting a literal ; outside an expression (x://e;q/{a}) is unaffected.

4. Overlapping occurrences skipped in the collision scan

Confirmed with the exact literal given — the token begins and ends with zz, so it overlaps itself:

zzInspectorUnfilledzzInspectorUnfilledzzz0zz
advance-by-length finds: [0]       advance-by-one finds: [0, 19]

and index 19 is the one with the longer trailing run, so the chosen placeholder collided. The scan now advances one character at a time. I also pulled the logic into one helper shared with the group-prefix padding from #2 above, so the two padding sites cannot drift — the same overlap bug would otherwise have been latent in the new one.

npm run ci passes (re-run after the v2/main merge that landed on the branch).

[cliffhall]

Round 6: one suppressed comment, and it's the right architectural call — fixed in 00549c9.

The correction was web-only, which undercut the PR's own premise

Exactly right, and the citation was accurate: readResourceFromTemplate still called the SDK directly, so the same template resolved two different ways depending on which client drove it.

x://{a,b}  with { a: "foo/bar", b: "x y" }

  web panel (this PR's helper)         → x://foo%2Fbar,x%20y
  TUI/CLI via readResourceFromTemplate → x://foo/bar,x y        ← raw multi-name branch

The helper now lives in core/uri/uriTemplate.ts and all three consumers route through it:

• the web ResourceTemplatePanel — discovery, expansion, preview;
• InspectorClient.readResourceFromTemplate — the CLI/TUI submit path. The last direct new UriTemplate call in core/ is gone;
• the TUI's uriTemplateToForm, which I took the opportunity to share discovery with as well. That was the other half of the divergence: the TUI built its fields from template.variableNames raw, so a repeated name produced two identical fields. Both clients now offer the same fields for a given template.

expandTemplate returning null becomes a thrown error on the client path, preserving the existing Failed to expand URI template contract that its callers and the backfill tests rely on.

Supporting changes required by the repo's own rules:

• core/uri/** added to the web coverage include in clients/web/vite.config.ts, so it stays under the ≥90 gate (it is at 100%);
• tests moved to clients/web/src/test/core/uri/uriTemplate.test.ts, per the placement rule that core/ tests mirror core's layout there;
• structure trees and the coverage-include list updated in both AGENTS.md and the root README.md;
• the TUI test that asserted the old local console.error now asserts the shared helper's warning, plus two new cases covering a query expression and a repeated name.

One new integration test asserts the two paths cannot drift again — it reads through readResourceFromTemplate and checks the expandedUri is both correctly encoded and identical to what the panel's expandTemplate produces:

expect(invocation.expandedUri).toBe("foobar://events/foo%2Fbar");
expect(invocation.expandedUri).toBe(
  expandTemplate("foobar://events/{topic}", { topic: "foo/bar" }),
);

npm run ci passes — all three durable guards green with the new folder (971 files format-gated, 936 typechecked), coverage 330/330, build-gate fired, all six smokes OK, Storybook 113/113. One useServers waitFor timeout appeared in the instrumented full-suite run and passed both in isolation and on a clean coverage re-run; it is untouched by this diff.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (2)

core/uri/uriTemplate.ts:186

• projected starts as a copy of every caller-supplied parameter, so when none of the real group members is defined, an extra parameter whose key happens to equal the generated synthetic name remains in the object and expands the expression. The SDK previously ignored undeclared parameters, but expandTemplate("x://{a,b}", { __inspectorGroup0__: "injected" }) now emits x://injected instead of x://. Remove the synthetic key in the omitted-group branch.

    if (present.length > 0) projected[synthetic] = present;

core/uri/uriTemplate.ts:35

• The SDK constructor does not reject every malformed expression. In @modelcontextprotocol/client@2.0.0, new UriTemplate("x://{}") succeeds with no variable names and expands to x://; consequently the panel's allFilled check is vacuously true and submits a URI different from the advertised template. This violates this helper's null-on-malformed contract. Validate the expression grammar (at minimum, require a non-empty variable list) before accepting the SDK parse.

  try {
    return new UriTemplate(uriTemplate);

[cliffhall]

Round 7: two suppressed comments, both confirmed and both fixed in 892914f.

1. An empty expression was accepted — the more serious of the two

Confirmed exactly:

new UriTemplate("x://{}")   parses. variableNames: []   expand({}) → "x://"
new UriTemplate("x://{ }")  parses. variableNames: []   expand({}) → "x://"
new UriTemplate("x://{,}")  parses. variableNames: []   expand({}) → "x://"

and the consequence is the one described: no variables discovered → no inputs rendered → allFilled vacuously true → Read Resource enabled → submits x://, which is not the template the server advertised. That is the worst possible way to violate the null-on-malformed contract, because it fails silently in the direction of sending a request.

Validation now runs on the template as declared, before any rewriting. Worth noting since it wasn't obvious: putting the check inside parseTemplate (the natural spot) is not sufficient — groupMultiNameExpressions runs first in the two entry points and folds {a,} into a synthetic single-name expression, so by the time the parse happens the empty member is gone. My first attempt did exactly that and {,} / {a,} still slipped through; the tests caught it.

The ; check moved into the same function while I was there, so the two "cannot handle this template" cases are one unsupportedReason() returning a reason string, rather than two guards at two different layers.

Five table-driven cases ({}, { }, {,}, {a,}, {?}) assert both halves of the contract — templateVariableNames → [] and expandTemplate → null — plus a panel test asserting Read Resource stays disabled and the preview shows x://{} rather than the x:// the SDK would expand it to.

2. A caller value could stand in for a group

Correct. projected starts from a copy of every supplied variable, so a key equal to the generated synthetic name survived into the expansion:

expandTemplate("x://{a,b}", { __inspectorGroup0__: "injected" })
  → "x://injected"     where the SDK ignores undeclared variables → "x://"

The omitted-group branch now deletes the key rather than leaving the caller's value under it, so the projection is always assign-or-delete and never inherits. Regression test added alongside the existing one for the inverse case (a template that legitimately declares that name still gets the user's value).

npm run ci passes end to end with no flakes this run: guards green (971 format-gated, 936 typechecked, dep-lockstep OK), unit 270, coverage 330/330, build-gate fired, all six smokes OK, Storybook 113/113.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

core/uri/uriTemplate.ts:404

• This restoration loop rescans the entire expanded URI once per variable, making preview generation O(variable-count × URI-length). The SDK accepts server-controlled templates with up to 10,000 expressions and 1 MB of text, so selecting a large template can trigger billions of character scans during React render and freeze the browser. Build a sentinel-to-placeholder map and replace all sentinel tokens in one regex/pass instead.

  let preview = expanded;
  names.forEach((name, index) => {
    if (filled[name] !== undefined) return;
    // The sentinel is unreserved, so it appears in the expansion unencoded.
    preview = preview.split(sentinelFor(base, index)).join(`{${name}}`);

[cliffhall]

Round 8: one inline comment and one suppressed. Both confirmed, both fixed in 24345d6 — and the two turned out to have a single answer.

1. Delegating encoding to the SDK is not RFC-conformant

This is the most substantive finding in the review so far, and all three claims reproduce exactly:

x://{v}   "!"     → "x://!"           RFC 6570 §3.2.1: x://%21
x://{v}   "*'()"  → "x://*'()"                          x://%2A%27%28%29
x://{+v}  "[a]"   → "x://%5Ba%5D"                       x://[a]
x://{#v}  "[a]"   → "x://#%5Ba%5D"                      x://#[a]
x://{+v}  "%41"   → "x://%2541"                         x://%41

encodeURIComponent under-encodes the five sub-delims it exempts; encodeURI over-encodes the gen-delims [ and ] that reserved expansion exists to pass through, and re-encodes a well-formed pct-triplet. And the point that this now matters more because every client routes through here is the right framing.

Rather than patch around the SDK, I made the division of labour explicit. Each variable now reaches the SDK as a sentinel of unreserved characters and is replaced with its real rendering afterwards:

• structure — operators, separators, group joins, which expressions appear at all — stays the SDK's job, where it is correct;
• encoding becomes ours, against the explicit RFC 3986 unreserved (ALPHA / DIGIT / - . _ ~) and reserved (gen-delims + sub-delims) sets, with pct-triplet passthrough under +/# and whole-code-point encoding so an astral character yields its UTF-8 octets rather than lone surrogates.

Regressions added for all of it: each of !*'() individually in a simple expansion and together in a query one, gen-delims preserved under both + and #, the full reserved set round-tripping under +, triplet passthrough, a bare % and a malformed %zz both encoding to %25, the absence of triplet passthrough in a simple expansion (%41 → %2541, which is correct there), and 😀 → %F0%9F%98%80.

2. The restoration loop rescanned the URI once per variable

Also correct, and the restructure above resolves it as a side effect: substitution is now a single regex pass (base(\d+)zz → rendering) instead of one split/join per variable, so it is O(length) rather than O(variables × length) on a template the SDK will accept at 1 MB with 10,000 expressions. Test added with 200 variables, alternating filled and unfilled, which also exercises the multi-digit index boundary at scale.

Two things fell out of the restructure worth noting:

• expandTemplate and previewTemplate are now one routine differing only in how an unset variable renders (omitted vs {name}), which removes the last place the two could drift.
• The sentinel base now only has to avoid the template's own text — values never appear in the string being substituted, and a single pass does not rescan what it inserts, so a value equal to the token is inherently safe rather than defended against.

One consequence I had to handle rather than inherit: the SDK's per-value length ceiling no longer sees the real value (it sees a short sentinel), so the same 1,000,000-character bound is now enforced here. Without it the helper would emit a URI the SDK itself refuses to build, and the panel would quietly lose the "withhold rather than send something absurd" behavior added in round 4.

npm run ci passes: guards green, unit 270, coverage 330/330, build-gate fired, all six smokes OK, Storybook 113/113.

[cliffhall]

Round 8: one inline comment and one suppressed. Both confirmed, both fixed in 24345d6 — and the two turned out to have a single answer.

1. Delegating encoding to the SDK is not RFC-conformant

This is the most substantive finding in the review so far, and all three claims reproduce exactly:

x://{v}   "!"     → "x://!"           RFC 6570 §3.2.1: x://%21
x://{v}   "*'()"  → "x://*'()"                          x://%2A%27%28%29
x://{+v}  "[a]"   → "x://%5Ba%5D"                       x://[a]
x://{#v}  "[a]"   → "x://#%5Ba%5D"                      x://#[a]
x://{+v}  "%41"   → "x://%2541"                         x://%41

encodeURIComponent under-encodes the five sub-delims it exempts; encodeURI over-encodes the gen-delims [ and ] that reserved expansion exists to pass through, and re-encodes a well-formed pct-triplet. And the point that this now matters more because every client routes through here is the right framing.

Rather than patch around the SDK, I made the division of labour explicit. Each variable now reaches the SDK as a sentinel of unreserved characters and is replaced with its real rendering afterwards:

• structure — operators, separators, group joins, which expressions appear at all — stays the SDK's job, where it is correct;
• encoding becomes ours, against the explicit RFC 3986 unreserved (ALPHA / DIGIT / - . _ ~) and reserved (gen-delims + sub-delims) sets, with pct-triplet passthrough under +/# and whole-code-point encoding so an astral character yields its UTF-8 octets rather than lone surrogates.

Regressions added for all of it: each of !*'() individually in a simple expansion and together in a query one, gen-delims preserved under both + and #, the full reserved set round-tripping under +, triplet passthrough, a bare % and a malformed %zz both encoding to %25, the absence of triplet passthrough in a simple expansion (%41 → %2541, which is correct there), and 😀 → %F0%9F%98%80.

2. The restoration loop rescanned the URI once per variable

Also correct, and the restructure above resolves it as a side effect: substitution is now a single regex pass (base(\d+)zz → rendering) instead of one split/join per variable, so it is O(length) rather than O(variables × length) on a template the SDK will accept at 1 MB with 10,000 expressions. Test added with 200 variables, alternating filled and unfilled, which also exercises the multi-digit index boundary at scale.

Two things fell out of the restructure worth noting:

• expandTemplate and previewTemplate are now one routine differing only in how an unset variable renders (omitted vs {name}), which removes the last place the two could drift.
• The sentinel base now only has to avoid the template's own text — values never appear in the string being substituted, and a single pass does not rescan what it inserts, so a value equal to the token is inherently safe rather than defended against.

One consequence I had to handle rather than inherit: the SDK's per-value length ceiling no longer sees the real value (it sees a short sentinel), so the same 1,000,000-character bound is now enforced here. Without it the helper would emit a URI the SDK itself refuses to build, and the panel would quietly lose the "withhold rather than send something absurd" behavior added in round 4.

npm run ci passes: guards green, unit 270, coverage 330/330, build-gate fired, all six smokes OK, Storybook 113/113.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

core/uri/uriTemplate.ts:312

• A repeated variable is encoded using only the operator from its first occurrence. RFC 6570 allows the same name in expressions with different operators, so x://{+a}/{a} with a = "/" must preserve / in the reserved expansion but encode it as %2F in the simple expansion. This map and the single per-name rendering apply the first encoding to both occurrences, producing the wrong URI. Track/render values per expression occurrence (or otherwise include the operator in the placeholder identity), and add a mixed-operator repeated-name regression.

      if (!operators.has(name)) operators.set(name, operator);

[cliffhall]

Round 9: one inline, one suppressed. Both real, both consequences of last round's restructure, both fixed in 0a96cc1.

1. An unpaired surrogate throws out of render

Confirmed — encodeURIComponent("\uD800") raises URIError: URI malformed, and the point about where it throws is the important half: value encoding runs outside the SDK's try/catch, and previewTemplate runs during React render, so it would take the panel down rather than disable its submit.

A lone surrogate has no UTF-8 encoding under any operator, so there is no correct rendering to fall back to — it now fails the whole expansion rather than silently dropping one variable. Detected before anything is placed (codePointAt reports the unpaired surrogate value directly, so no exception is needed to find it): expandTemplate → null, preview → the raw template, panel → Read Resource disabled.

Tests for a lone high surrogate, a lone low surrogate, and one embedded in otherwise-valid text, each asserting both not.toThrow() and null, plus the preview fallback.

2. A repeated name with different operators got one encoding

Also correct, and it exposed a structural limit rather than a slip. x://{+a}-{a} with a = "/" must give x:///-%2F. The per-name operator map applied the first occurrence's operator to both — but the deeper reason a map cannot work here is that the SDK looks values up by name, so one key cannot carry two renderings no matter how the operator is tracked.

So the rewrite generalizes: instead of only multi-name expressions, every non-query expression now becomes its own synthetic variable, making an occurrence the unit of rendering — which is what RFC 6570 actually specifies.

Query expressions are deliberately the exception. ? and & emit the variable's name into the URI, so renaming {?topic} would produce ?__inspectorGroup0__=. They also don't need it: their branch already encodes correctly, and ?/& share one encoding, so every query occurrence of a name can share one value keyed by that name. A name used in both a query and a non-query expression is fine — the non-query occurrence no longer uses the bare name, so they cannot collide.

Four tests: the mixed +/simple pair, the repeated name still offered as one field, a +/query pair (x://{+a}{?a} → x://a/b?a=a%2Fb), and a simple/fragment pair (x://{a}{#a} → x://%5Bx%5D#[x]).

Two things fell out of this worth noting:

• Name discovery now comes from the same scan the expander uses, rather than from UriTemplate.variableNames. The form's fields and the expander's lookups are one list by construction instead of two code paths that happened to agree — which is the class of bug this comment was.
• My first expectation for the mixed-operator test was wrong (x:///%2F), because I miscounted the literal / between the two expressions. The implementation was right; the test now uses - as the separator so every slash in the output is one the expansion produced.

npm run ci passes: guards green, coverage 330/330, build-gate fired, all six smokes OK, Storybook 113/113.

[cliffhall]

Round 9: one inline, one suppressed. Both real, both consequences of last round's restructure, both fixed in 0a96cc1.

1. An unpaired surrogate throws out of render

Confirmed — encodeURIComponent("\uD800") raises URIError: URI malformed, and the point about where it throws is the important half: value encoding runs outside the SDK's try/catch, and previewTemplate runs during React render, so it would take the panel down rather than disable its submit.

A lone surrogate has no UTF-8 encoding under any operator, so there is no correct rendering to fall back to — it now fails the whole expansion rather than silently dropping one variable. Detected before anything is placed (codePointAt reports the unpaired surrogate value directly, so no exception is needed to find it): expandTemplate → null, preview → the raw template, panel → Read Resource disabled.

Tests for a lone high surrogate, a lone low surrogate, and one embedded in otherwise-valid text, each asserting both not.toThrow() and null, plus the preview fallback.

2. A repeated name with different operators got one encoding

Also correct, and it exposed a structural limit rather than a slip. x://{+a}-{a} with a = "/" must give x:///-%2F. The per-name operator map applied the first occurrence's operator to both — but the deeper reason a map cannot work here is that the SDK looks values up by name, so one key cannot carry two renderings no matter how the operator is tracked.

So the rewrite generalizes: instead of only multi-name expressions, every non-query expression now becomes its own synthetic variable, making an occurrence the unit of rendering — which is what RFC 6570 actually specifies.

Query expressions are deliberately the exception. ? and & emit the variable's name into the URI, so renaming {?topic} would produce ?__inspectorGroup0__=. They also don't need it: their branch already encodes correctly, and ?/& share one encoding, so every query occurrence of a name can share one value keyed by that name. A name used in both a query and a non-query expression is fine — the non-query occurrence no longer uses the bare name, so they cannot collide.

Four tests: the mixed +/simple pair, the repeated name still offered as one field, a +/query pair (x://{+a}{?a} → x://a/b?a=a%2Fb), and a simple/fragment pair (x://{a}{#a} → x://%5Bx%5D#[x]).

Two things fell out of this worth noting:

• Name discovery now comes from the same scan the expander uses, rather than from UriTemplate.variableNames. The form's fields and the expander's lookups are one list by construction instead of two code paths that happened to agree — which is the class of bug this comment was.
• My first expectation for the mixed-operator test was wrong (x:///%2F), because I miscounted the literal / between the two expressions. The implementation was right; the test now uses - as the separator so every slash in the output is one the expansion produced.

npm run ci passes: guards green, coverage 330/330, build-gate fired, all six smokes OK, Storybook 113/113.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (3)

README.md:250

• This says the SDK expander has only two RFC departures, but the new helper also replaces its non-conformant character encoding (including simple/query sub-delimiters, reserved [], and percent triplets). Update the summary so the documented rationale reflects what this PR now maintains.

All three clients now go through one shared helper, [`core/uri/uriTemplate.ts`](./core/uri/uriTemplate.ts) — the web panel, the TUI's form builder, and `InspectorClient.readResourceFromTemplate` — so a template cannot resolve differently depending on where it is driven from. It wraps the SDK's `UriTemplate` and corrects the two places that expander departs from RFC 6570: a multi-name expression (`{a,b}`) skips both encoding and its operator, and the `;` path-parameter operator is unimplemented (such a template is declined rather than expanded to a knowingly invalid URI).

core/uri/uriTemplate.ts:319

• The expansion path validates only the rewritten text, not the server-supplied template. A template over the SDK's 1,000,000-character limit whose size is concentrated in a non-query variable name is compressed to a short synthetic slot, so expandTemplate accepts it (and can reduce it to a different URI such as x://) even though templateVariableNames rejected the original. Parse the original before rewriting so malformed/over-limit templates consistently return null, and add a boundary regression.

  const { text, slots, queryNames, order } = rewriteExpressions(uriTemplate);
  const template = parseTemplate(text);

core/uri/uriTemplate.ts:14

• This header is stale after the encoding and occurrence-aware rewrites: it still says there are only two SDK gaps and points to the nonexistent groupMultiNameExpressions. Reference the current helpers instead so the module documentation matches the implementation.

 * These wrap the SDK's `UriTemplate`, correcting the two places its expander
 * departs from RFC 6570 (see `groupMultiNameExpressions` and
 * `unsupportedReason`). Living in `core/` is what makes the correction

[cliffhall]

Round 10: three suppressed comments — one real bug, two documentation-staleness items. All fixed in ae7b34c.

1. The expansion path validated only the rewritten text

Correct, and the mechanism is exactly as described. The SDK validates both the template and each variable name at construction:

UriTemplate.validateLength(template, MAX_TEMPLATE_LENGTH, "Template");
for (const name of names) UriTemplate.validateLength(name, MAX_VARIABLE_LENGTH, "Variable name");

and the rewrite replaces variable names with short synthetics, shrinking both. So an over-limit template could be compressed past the check and expand happily, while templateVariableNames — which parses the original — rejected it. The failure mode is the nasty combination: an empty form beside an expansion that still produces a URI.

expandWithPlaceholders now parses the original first, before rewriting, so discovery and expansion agree by construction. The second parse (of the rewritten text) can no longer fail — rewriting only ever shortens — so it is marked unreachable rather than left looking like a live branch.

Boundary regressions for both limits, each asserting discovery and expansion agree: an over-long variable name, and an over-long template.

2 & 3. The module header and README rationale were stale

Both fair, and my fault for letting prose lag three rounds of restructuring. The header still said "the two places its expander departs from RFC 6570" and pointed at groupMultiNameExpressions, which no longer exists — it became rewriteExpressions when the rewrite generalized from multi-name expressions to every non-query expression.

Both now describe what the helper actually maintains:

• value encoding, done here against the explicit RFC 3986 sets, because the SDK's encodeURIComponent/encodeURI leave !*'() bare, escape the gen-delims [ ], and double-encode a percent triplet;
• the per-occurrence rewrite, covering both multi-name expressions and a name repeated under different operators;
• the declined shapes ({} and ;), which are refused rather than expanded into a knowingly invalid URI.

I also updated the core/uri entry in AGENTS.md in the same pass, since that was a third copy of the same rationale and would otherwise have been left stale by a fix to the other two.

npm run ci — the validate half is green (unit 270, cli 25, tui 26, launcher 1); the coverage/smoke/Storybook stages were still running when this was pushed at the maintainer's request, and I'll follow up here if any of them reports anything.

[cliffhall]

Following up on the caveat in the previous comment: the rest of npm run ci has since finished clean on ae7b34cd — coverage 330/330 with no threshold errors, build-gate fired, all six smokes OK, Storybook 113/113, exit 0. Nothing outstanding.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

core/uri/uriTemplate.ts:87

• The empty-member check runs before removing the explode modifier. As a result, SDK-accepted malformed expressions such as {*} or {a,*} pass this guard; rewriteExpressions then turns * into an empty name, so discovery returns "" and the web/TUI render a blank variable field instead of declining the template. Normalize members the same way as the rewrite before checking emptiness.

    const names = body.replace(/^[+#./?&]/, "").split(",");
    if (names.some((name) => name.trim().length === 0)) {

core/mcp/inspectorClient.ts:4981

• This message is inaccurate for templates the helper deliberately does not implement. For example, {;a} is valid RFC 6570, but unsupportedReason rejects it, so saying the template “cannot be expanded per RFC 6570” incorrectly blames the input rather than Inspector's current limitation. State that Inspector could not expand it (or propagate the helper's concrete reason).

        `Failed to expand URI template "${uriTemplateString}": the template or one of its values cannot be expanded per RFC 6570.`,

[cliffhall]

Closing as a duplicate of #2035, which fixes the same issue (#1919) and is the implementation we are taking forward.

Both PRs implement RFC 6570 expansion for the Resources form, but they take opposite architectures: this one delegates URI structure to the SDK's UriTemplate.expand (supplying its own encoding through a sentinel rewrite), while #2035 owns the expander outright. Three measured differences decided it:

• Multiple query expressions. Delegating to the SDK inherits its non-conformant ?→& rewrite: x{?one}{?two} expands here to x?one=1&two=2, which the SDK's own UriTemplate.match() — the matcher a server resolves with — returns null for. RFC 6570 expands each expression independently (x?one=1?two=2), which fix(web): expand resource templates per RFC 6570 #2035 does and which round-trips through the matcher. That is the Resource template UI does not support RFC 6570 expansion #1919 failure class one level up.
• {;id} and {id:3}. This PR declines ; templates entirely (no fields, unreadable) and inherits the SDK's prefix-modifier bug (field labelled id:3, no truncation). fix(web): expand resource templates per RFC 6570 #2035 handles both.
• Optional query variables. canSubmit here requires every variable filled, so foobar://events{?topic} — the template from the issue — cannot be read without a topic. fix(web): expand resource templates per RFC 6570 #2035 gates per non-omittable expression (requiredGroups / hasRequiredValues) and marks the field Optional.

Two things this PR got right that #2035 did not have been ported over to it: declining a template whose expression declares no variable ({}, {a,}, {?}) instead of silently expanding it away, and withholding the read when expansion fails rather than submitting the raw template. Thanks — those were the better call.

The showcase server and integration coverage land through #2035, so nothing here is lost.