fix(web): expand resource templates per RFC 6570

[cliffhall]

Closes #1919

The Resources screen discovered and substituted template variables with a bare /\{(\w+)\}/g regex, which is wrong in two independent ways:

1. It cannot see an expression carrying an operator. foobar://events{?topic} rendered no topic input at all — the template was un-fillable.
2. It splices the raw value in. A /, ?, #, %, space, or non-ASCII character in a simple {topic} landed unencoded, so foo/bar produced foobar://events/foo/bar — an extra path segment, which a conforming resource-template matcher rejects with -32602 Resource not found.

The fix

Parsing, variable classification, and expansion moved to core/mcp/uriTemplate.ts, shared by the web Resources form and the TUI — both expand through it and derive their form fields from it, which is the half that makes the sharing real: a form submits values under the names it rendered, so a parser that mangles a name silently drops the value at expansion. (The CLI is not a consumer — it has no template form and passes an already-expanded --uri straight to readResource.)

The SDK's UriTemplate is used only to validate a template. Its expander is not, because it is incomplete in five ways a form makes visible — each measured against the pinned SDK, not inferred:

Shape	SDK behavior	Correct
{a,b}	raw-joins the values — no encoding, operator prefix dropped	foo%2Fbar,q
{;id}	; missing from its operator list, so the variable parses as ;id	;id=7
{id:3}	prefix modifier folded into the name, giving id:3	abc
{+v} / {#v}	encodeURI mangles reserved [::1] → %5B::1%5D and double-encodes %2F → %252F	unchanged
{v}	encodeURIComponent leaves the sub-delims !'()* bare	%21%27%28%29%2A

Two behaviors are deliberately not copied from the SDK:

• No ?-to-& rewrite. RFC 6570 expands each expression independently. The SDK rewrites a second {?two}'s ? to &, and its own matcher then rejects the result: for x{?one}{?two}, match("x?one=1&two=2") is null while match("x?one=1?two=2") returns both variables. A server wanting a continuation advertises {?one}{&two}.
• Invalid varspecs are rejected, not guessed. RFC 6570's max-length is %x31-39 0*3DIGIT, so {id:}, {id:0}, {id:abc} and {id:10000} are invalid templates. The SDK's constructor accepts them all. Strict expansion throws; the lenient variant returns the raw template so the panel does not blow up on render.

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). The TUI enforces the same rule in its submit handler, since ink-form has no way to express "any one of these".

Lookups are own-property only. toString, constructor, valueOf and __proto__ are all valid RFC 6570 variable names, and a bare values[name] finds Object.prototype's member for each — a blank {?toString} would have expanded a function body into the URI.

A template that cannot expand withholds the read. An out-of-grammar modifier ({id:abc}) or an expression declaring no variable ({}, {,}, {a,}, {?}, {*}) makes the template invalid, and tryExpandUriTemplate returns the reason as a value rather than a URI: the panel disables Read Resource and prints it. The lenient expandUriTemplate — which answers with the raw template — is for display only (the preview runs during render, where a throw takes the panel down). Submitting that fallback would read the template itself, braces intact, and draw a confusing "not found" for a defect that is not the user's. Skipping an empty varspec instead of rejecting it is worse still: x://{} would expand to x:// with no inputs rendered, so the "everything required is filled" check passes vacuously and the read goes out for a URI that is not the template the server published. The same gate covers a value that cannot be encoded — an unpaired surrogate has no UTF-8 encoding, so encodeURIComponent throws URIError on it, and a text input can hold one via paste. (Both behaviors are ported from #2033, the parallel attempt at this issue, now closed as a duplicate.)

Screenshots

Captured against the new rfc6570-templates-http.json showcase server (below).

Simple expression foobar://events/{topic} with foo/bar entered — the preview beside the title is the URI that will be sent.

Before	After
foobar://events/foo/bar — unencoded, server answers Resource not found	foobar://events/foo%2Fbar — encoded, resolves

Query expression foobar://events{?topic} — before, there is no input to type into at all.

Before	After
No topic field rendered	topic field, marked Optional

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

And the same topic field with a value entered — the preview beside the title is the URI the read will send, foobar://events?topic=foo%2Fbar, with the / percent-encoded. There is no "before" counterpart because on the old build the field does not exist.

A malformed template, refused. events_malformed (foobar://events/{topic:abc}) on the showcase server below: Read Resource is disabled, the reason is printed under the form, and the preview shows the template as the server declared it. There is no "before" pair — on the old build the modifier was folded into the variable's name, so the form asked for a field labelled topic:abc and the read went out against a URI the server never advertised.

Test server

test-servers/configs/rfc6570-templates-http.json (preset rfc6570_templates) serves the two templates straight out of the issue — events_by_topic (foobar://events/{topic}) and events_by_query (foobar://events{?topic}) — each echoing the URI it was matched against. Verified end-to-end through the CLI against the real SDK matcher:

$ --method resources/read --uri 'foobar://events/foo%2Fbar'   → 200, matched
$ --method resources/read --uri 'foobar://events?topic=foo%2Fbar' → 200, matched
$ --method resources/read --uri 'foobar://events/foo/bar'     → Resource not found  ← the bug

Documented in the root README's showcase table and a new RFC 6570 resource templates section.

Tests

• clients/web/src/test/core/mcp/uriTemplate.test.ts — the expander: parsing, operator classification, required groups, encoding under every operator (/, ?, #, %, spaces, Unicode, !'()*, [::1], pct-triplets), the ; and :N shapes, varspec-grammar rejection, expression independence, strict-vs-lenient, and the Object.prototype name collisions.
• clients/web/src/test/integration/mcp/rfc6570-templates.test.ts — resolves the checked-in config and drives four reads over a real transport: the base URI, the encoded simple value, the encoded query value, and the unencoded URI that must still be refused. A misspelt preset fails here rather than only when someone runs the repro by hand.
• ResourceTemplatePanel.test.tsx — the rendered input for {?topic}, the encoded URIs handed to onReadResource, the Optional marker and its effect on the submit gate, and the preview.
• clients/tui/__tests__/ — uriTemplateToForm field naming for {;id} / {id:3}, shared-group optionality, and ResourceTestModal's group-aware submit guard.

npm run ci passes.

[Copilot]

Pull request overview

This PR aims to make web resource-template expansion RFC 6570-aware by using the SDK’s UriTemplate, updating the form behavior, and adding a showcase server.

Changes:

• Adds URI-template parsing, expansion, preview, and variable classification utilities.
• Updates the Resources UI and tests for encoded and optional variables.
• Adds an RFC 6570 test-server preset and documentation.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

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 preset.
test-servers/configs/rfc6570-templates-http.json	Configures the showcase server.
README.md	Documents the showcase workflow.
clients/web/src/utils/uriTemplate.ts	Implements template utilities.
clients/web/src/utils/uriTemplate.test.ts	Tests parsing and expansion.
ResourceTemplatePanel.tsx	Integrates RFC-aware form behavior.
ResourceTemplatePanel.test.tsx	Tests the updated UI behavior.

---

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

[cliffhall]

ping @copilot — review round 1 addressed in 409dce3. All three findings were correct; each was reproduced against the pinned SDK before acting, and each has an inline reply with the measurements.

1. # was misclassified as required. x://a{#frag} with no frag expands to exactly x://a — a well-formed URI naming a broader resource — unlike {+path} (x://a/) or a simple {userId} (file:///users//profile), which leave an empty path segment. # moved into the omittable set, and the required cases now assert what the URI becomes when blank, so the rule is checked rather than restated.

2. Delegating to the SDK did not actually give RFC 6570 expansion. expandPart takes an early names.length > 1 branch that raw-joins values, skipping both encodeValue and the operator prefix — x://{a,b} with a = "foo/bar" gives x://foo/bar,q, the very unencoded-slash defect this PR is about, and x://a{/p,q} gives x://ax y,z.

Fixing that in the web client alone would have left the TUI and CLI wrong, since readResourceFromTemplate expands through the same class — worse than the uniform wrongness it started from. So parse/classify/expand moved to core/mcp/uriTemplate.ts and both call sites use it. The correction is surgical: multi-name non-query expressions are expanded there and spliced in as literal text before the SDK sees the template (safe — both encoders escape {/}), while every single-name and query expression still goes through the SDK untouched. The preview applies the same correction so it cannot promise a URI that submitting would not send.

3. The showcase promised a blank {?topic} read that 404s. match("foobar://events") returns null — {?topic} compiles to a required \?topic=([^&]+). A plain foobar://events resource is now registered so the documented step resolves, with the matcher constraint recorded in both the fixture and the README.

Verified end to end through the CLI:

resources/read foobar://events                  -> 200 {"collection":"events","filtered":false}
resources/read foobar://events?topic=foo%2Fbar  -> 200 (template match)
resources/read foobar://events/foo%2Fbar        -> 200 (template match)
resources/read foobar://events/foo/bar          -> Resource not found   <- the original bug

Note for round 2: the SDK's matcher has the mirrored multi-name gap (partToRegExp emits one capture for {a,b}), so no SDK-backed server can round-trip those templates. That is why the showcase carries no multi-name template — the fix is about emitting a correct URI, which is the half the client controls; the unit tests cover the expansion directly.

npm run ci passes (including a tsc -b --force, which caught a never-narrowing type predicate the incremental cache had hidden).

[Copilot]

Pull request overview

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

Suppressed comments (3)

core/mcp/uriTemplate.ts:123

• Requiredness cannot be assigned independently to every name in a multi-name expression. For {a,b}, this marks both fields required and the panel disables submission when only a is filled, even though RFC 6570 omits undefined names and this module can validly expand that input to just a's value. Model the non-omittable constraint per expression (at least one value for this group) instead of requiring every member.

  for (const part of parseUriTemplate(uriTemplate)) {
    if (part.kind !== "expression") continue;
    const required = !OMITTABLE_OPERATORS.has(part.operator);
    for (const name of part.names) {

core/mcp/uriTemplate.ts:98

• RFC 6570 prefix modifiers are currently folded into the variable name. For {id:3}, this creates an id:3 form field instead of id, and entering abcdef cannot produce the required abc expansion. Parse the :length modifier separately and apply it before encoding rather than treating the whole varspec as the lookup key.

    const names = body
      .slice(operator.length)
      .split(",")
      .map((name) => name.replace("*", "").trim())
      .filter((name) => name.length > 0);

core/mcp/uriTemplate.ts:15

• The RFC 6570 path-parameter operator ; is missing from this operator list. As a result, {;id} is parsed as a simple variable named ;id, so the form renders the wrong field and expansion cannot produce ;id=value (and incorrectly treats it as required). Add ; parsing and its named expansion semantics, including multi-name and empty-value handling.

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

• line 94
• line 120

const OPERATORS = ["+", "#", ".", "/", "?", "&"] as const;

[cliffhall]

ping @copilot — round 2 addressed in 25c3ad2.

The review reported "no new comments" but carried three suppressed comments. All three were real, and each was reproduced against the pinned SDK before acting:

x://a{;id}   {id:"7"}       => "x://a"     variableNames: [";id"]
x://a/{id:3} {id:"abcdef"}  => "x://a/"    variableNames: ["id:3"]
x://{a,b}    {a:"only-a"}   => "x://only-a"   <- expandable, but the form blocked it

1. The ; operator is missing from the SDK's operator list, so {;id} parsed as a variable literally named ;id. Added the operator and its named expansion (;a=1;b=2).

2. A prefix modifier was folded into the variable name — {id:3} yielded a variable called id:3. Varspecs are now parsed properly and the value truncated before encoding, by code point: String.prototype.slice counts UTF-16 units and would split an astral character into a lone surrogate.

For both of these the wrong URI was the lesser problem. A form has to name the variables it asks the user to fill, so the panel was rendering fields labelled ;id and id:3 that nobody could usefully fill in.

3. 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 expands to a's value — the SDK does this correctly — and the panel was refusing input the expander would have accepted. hasRequiredValues now encodes "any one name in a required group suffices", and such a field reads Any one of: a, b instead of falsely marking each mandatory.

Two structural consequences fell out, each pinned by a test:

• Takeover is now per template, not per expression. Splicing corrected fragments into a template the SDK then re-expands would leave its cross-expression ?-to-& rewrite blind to the fragments already resolved.
• Expansion is split strict/lenient. readResourceFromTemplate must throw — three pre-existing integration tests assert Failed to expand URI template, and my first pass regressed them by swallowing the error. The form must not throw, since the template comes from the server and it would take out the panel on render. The strict variant constructs the SDK template unconditionally, because that construction is what validates syntax: otherwise x://{;a}{b,c would take the own-expansion path and its unclosed tail would pass as literal text with nothing objecting.

The preview for a partially-filled {?one,two} now shows x://a?one=1 rather than the literal expression — the old assertion encoded the wrong behavior, since that is what submitting actually sends.

npm run ci passes: 4102 unit, 5283 under the coverage gate, 478 Storybook. (One run failed on Port 63315 is already in use from my own leftover processes; re-run clean after clearing them.)

[Copilot]

Pull request overview

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

Suppressed comments (2)

core/mcp/uriTemplate.ts:199

• Deduplication keeps only the first occurrence's groupNames, even when a later occurrence makes the variable required. For {?a}{?b}{a,b}, both variables retain singleton groups, so hasRequiredValues incorrectly requires both fields although RFC expansion accepts either one; the UI also omits the “Any one of” hint. Preserve and evaluate every required expression group separately, while deduplicating names only for rendering.

        existing.required = existing.required || required;

core/mcp/uriTemplate.ts:249

• encodeURI is not an RFC 6570 allow-reserved encoder: it percent-encodes reserved [/] characters and double-encodes existing percent triplets, both of which must remain unchanged under + and #. For example, the own-expansion path for {+v,w} turns v = "[::1]" into %5B::1%5D and %2F into %252F. The ordinary single-name path delegates to the SDK behavior this function mirrors, so use an RFC 6570 reserved-value encoder for both paths (or fix/upgrade the SDK) and cover these cases.

    ? encodeURI(value)

[cliffhall]

ping @copilot — round 3 addressed in e7d1246 (plus a merge of v2/main, which arrived on the branch meanwhile).

Round 3 also reported "no new comments" while carrying two suppressed ones. Both were real, and both reproduced against the pinned SDK:

encodeURI("[::1]")  => "%5B::1%5D"     <- reserved, must survive
encodeURI("%2F")    => "%252F"         <- double-encoded
x{?a}{?b}{a,b} with only a  => "x?a=11"  <- expandable, but the form demanded both

1. Requiredness could not live on a variable at all. Deduplication kept the first occurrence's group, so x{?a}{?b}{a,b} left both names required with singleton groups. Widening the stored group would not have been enough either: in {a,b}{a,c}, filling b and c satisfies both expressions — something no per-variable flag can express, whatever it stores.

So requiredness is now returned per expression by requiredGroups, and hasRequiredValues asks that each group be satisfied by any one of its names. I removed TemplateVariable.groupNames rather than leave a field that quietly meant something narrower than it read; required stays, documented as driving the "Optional" marker and nothing else. Two new tests cover the recurring-name and two-shared-groups cases directly.

2. encodeURI is not an allow-reserved encoder. It escapes [/], which are reserved and must survive, and it escapes %, so an already-encoded value is double-encoded. Both corrupt the URI rather than over-escaping it — an IPv6 literal or a pre-encoded path reaches the server altered, which is the same defect class this PR exists to fix.

Added an RFC 6570 §3.2.1 encoder that preserves reserved characters and existing pct-triplets: it splits on %XX so triplets pass through whole, a lone % still lands in a scanned chunk and encodes to %25, and the class carries the u flag so an astral character is handed to encodeURIComponent whole rather than as surrogates. +/# expressions are taken over even for a single name, so both expansion paths agree on what those operators mean. Seven tests, including that the simple operator still encodes [::1] — the allowance is scoped to +/# only.

Also added a screenshot of the topic field with a value entered, showing the preview resolve to foobar://events?topic=foo%2Fbar.

npm run ci passes on the merged tree (4102 unit, 5283 coverage-gated, 478 Storybook).

[Copilot]

Pull request overview

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

Suppressed comments (3)

core/mcp/uriTemplate.ts:294

• encodeURIComponent does not implement RFC 3986's unreserved set: it leaves !, ', (, ), and * unchanged. RFC 6570 requires those characters to be percent-encoded for simple, path, matrix, and query expansions (only +/# allow reserved characters), so {v} with a!b still produces a non-conforming expansion. Use strict RFC 3986 encoding and ensure ordinary SDK-delegated expressions also take the corrected path for these values; add cases for this character set.

/** Encodes one value for its operator: reserved characters survive `+` and `#`. */
function encodeValue(value: string, operator: string): string {
  return operator === "+" || operator === "#"
    ? encodeAllowReserved(value)
    : encodeURIComponent(value);

core/mcp/uriTemplate.ts:433

• The normalized names used here no longer match the TUI form's submitted keys. clients/tui/src/utils/uriTemplateToForm.ts:18-28 still uses the SDK's variableNames, so {;id} submits { ";id": "7" } and {id:3} submits { "id:3": "abc" }; this parser looks up id, finds no value, and drops the expression. Update the TUI form to derive fields from this module's templateVariables and cover both shapes so the shared helper actually works for every client.

  const sdkTemplate = new UriTemplate(uriTemplate);
  const parts = parseUriTemplate(uriTemplate);
  return parts.some(needsOwnExpansion)
    ? expandParts(parts, defined)
    : sdkTemplate.expand(defined);

clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx:263

• A variable can belong to both a singleton required expression and a shared group. For x://{a}/{a,b}, this labels a as “Any one of: a, b”, but the ['a'] group still requires a; entering only b therefore leaves Read Resource disabled despite the guidance. Suppress the shared-group message when a singleton group independently requires this field.

          const sharedGroup = groups.find(
            (names) => names.length > 1 && names.includes(varName),
          );
          const description = !required
            ? "Optional"

[Copilot]

Pull request overview

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

Suppressed comments (2)

core/mcp/uriTemplate.ts:320

• Repeated names make one logical requirement look like a shared group. For the valid template {a,a}, this returns ['a', 'a'], so the TUI's names.length === 1 check marks a optional while its submit guard still rejects a blank value, and the web UI displays “Any one of: a, a”. Deduplicate names within each expression before storing the group.

    if (part.names.length === 0) continue;
    groups.push(part.names);

clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx:305

• Using find hides later overlapping requirements. For {a,b}{b,c}{a,c}, the displayed hints only mention the first two groups; after filling b, every visible hint appears satisfied but Read Resource remains disabled because the hidden {a,c} group is unmet. Render every shared group containing this variable (or provide a form-level unmet-groups message).

          const sharedGroup = individuallyRequired
            ? undefined
            : groups.find(
                (names) => names.length > 1 && names.includes(varName),
              );

[cliffhall]

ping @copilot — round 13 addressed in 1c595e96. Both real, and both in the required-group work from round 7 rather than in the recent preview rework.

1. {a,a} was stored as a two-name group. It is one requirement named twice, and reading it as "either of these will do" left the TUI with a form it could not submit — the length === 1 test marked a optional while ResourceTestModal's guard still refused a blank — and the web panel offering "Any one of: a, a". requiredGroups now deduplicates within the expression:

requiredGroups("x://{a,a}")            =>  [["a"]]
expandUriTemplate("x://{a,a}", {a:"1"}) =>  "x://1,1"   // both occurrences still emit

2. The per-field hint used find. Your {a,b}{b,c}{a,c} case is exactly right: filling b satisfies the first two groups, every visible hint then looks met, and Read Resource stays disabled on an {a,c} nothing on screen points at.

Fields now hint every shared group they sit in (Any one of each: a, b; a, c). But that alone does not fix your case, because no per-field hint can say which group is still outstanding — so the form says it:

(nothing filled)  Still needed: a or b; b or c; a or c
(b filled)        Still needed: a or c          <- button still disabled, and now explained
(a filled too)    (line disappears, button enabled)

It is built from the same unmetRequiredGroups the submit gate uses — hasRequiredValues is no longer called in the panel at all, both come from the one list — so the message and the button cannot drift apart. Dimmed rather than red: an incomplete form is the expected starting state, not an error.

Tests: the dedupe and its expansion in core, the TUI form field being required for {a,a}, and three panel tests — the full Still needed progression above, the multi-group hint, and {a,a} showing a plain required field with no "Any one of".

npm run ci passes.

[Copilot]

Pull request overview

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

Suppressed comments (3)

core/mcp/uriTemplate.ts:276

• This makes variable discovery quadratic for a single multi-name expression: for every name, it scans all varspecs again. Since templates are server-controlled and the SDK permits templates up to 1 MB, an expression containing many short comma-separated names can freeze the Resources screen during render. Iterate part.varspecs directly so each spec supplies both its name and conformance in one pass.

    for (const name of part.names) {
      const conforming = !part.varspecs.some(
        (spec) => spec.name === name && spec.conforming === false,
      );

core/mcp/uriTemplate.ts:552

• Replacing UriTemplate.expand() drops its 1,000,000-character guard on every variable value: constructing UriTemplate validates only the template and names, while values now reach these allocation-heavy encoding paths without a bound. This is a reliability regression for readResourceFromTemplate and both forms when a very large value is pasted or supplied programmatically. Preserve the pinned SDK's value limit before truncating/encoding.

function renderValue(value: string, spec: VarSpec, operator: string): string {
  const truncated =
    spec.maxLength === undefined
      ? value
      : prefixUnits(value).slice(0, spec.maxLength).join("");
  return encodeValue(truncated, operator);

core/mcp/uriTemplate.ts:671

• new UriTemplate() does not fully validate brace syntax: the pinned SDK rejects an unclosed { but treats an unmatched } as literal text. The local parser does the same, and encodeLiteral deliberately preserves braces, so expandUriTemplateStrict("x://a}", {}) returns the malformed URI and enables the read instead of withholding it. Add explicit validation for braces in literal parts (and a regression test for an unmatched closing brace) before expansion.

  new UriTemplate(uriTemplate);
  const parts = parseUriTemplate(uriTemplate);
  const bad = parts.find((part) => part.kind === "expression" && part.invalid);

[cliffhall]

ping @copilot — round 14 addressed in c6b17c52. Three findings, all real; each measured on 1c595e96 before acting.

1. A stray } produced a URI with a brace in it. Confirmed, including the asymmetry you identify — the SDK's constructor catches one half and not the other:

"x://a}"        SDK ctor: accepted   ours strict:  "x://a}"   <- read enabled
"x://a/{oops"   SDK ctor: rejected   ours strict:  THROW

Any brace surviving in a literal is now refused, so the withhold behavior covers the whole class rather than the half the SDK happened to catch. encodeLiteral still passes braces through — that is for the preview, which renders an unexpandable template as the server wrote it.

2. The per-value ceiling went missing with the SDK's expander. A 1,000,001-character value expanded fine here where UriTemplate.expand would have thrown. Restored at the SDK's own 1,000,000 limit — deliberately the same number, so this is the guard it was rather than a new policy — checked before the allocation-heavy encoding paths, with the offending variable named in the error. Tested at the limit and one past it.

3. Discovery was quadratic per multi-name expression. templateVariables re-scanned the whole varspec list for every name to compute conforming; iterating the varspecs gives both in one pass. Agreed on why it matters rather than being a micro-optimization: the template is server-controlled, the SDK admits one up to 1 MB, and this runs on the render thread.

npm run ci passes.

[Copilot]

Pull request overview

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

Suppressed comments (2)

core/mcp/uriTemplate.ts:2

• This says the helper is shared by every client, but lines 10–13 explicitly state that the CLI is not a consumer. Describe it as shared by the web and TUI clients so the module summary matches its actual scope.

 * RFC 6570 URI Template parsing and expansion, shared by every client (#1919).

core/mcp/inspectorClient.ts:4999

• The new InspectorClient wiring is not covered by a regression that distinguishes it from the old SDK expander. The existing readResourceFromTemplate test uses report.txt, while the new transport test expands the URI separately and calls readResource, so both still pass if this line is reverted. Add an InspectorClient test using a value such as foo/bar (and ideally a query expression) and assert expandedUri/the actual read URI is RFC 6570-encoded.

      expandedUri = expandUriTemplateStrict(uriTemplateString, params);

[cliffhall]

ping @copilot — round 15 addressed in a3dd8563. Both findings taken, and the second one caught a factual error of mine that had been in this PR since the first commit.

1. The readResourceFromTemplate wiring had no regression test. You are right that the existing coverage passes either way — report.txt has nothing to encode, and the transport tests expand separately and call readResource.

Writing that test corrected something I had asserted twice, including in a commit message. My first version used foo/bar, and I verified it by actually reverting the wiring to new UriTemplate(t).expand(v). It still passed. encodeURIComponent does encode /, so the SDK expands foobar://events/{topic} to foobar://events/foo%2Fbar exactly as this module does:

SDK expander,  topic = "foo/bar"  =>  "foobar://events/foo%2Fbar"   // identical
SDK expander,  topic = "a!b"      =>  "foobar://events/a!b"         // differs
ours,          topic = "a!b"      =>  "foobar://events/a%21b"

So #1919 was the web panel's own String.replace, not the SDK expander — the TUI path was using the SDK correctly all along. The test now uses a!b, a sub-delim RFC 6570 requires encoded and encodeURIComponent leaves bare. Re-verified by reverting the wiring: 2 of 10 tests now fail (this one and the malformed-template refusal via readResourceFromTemplate), where before the change none did.

2. Module summary fixed — it said "shared by every client" a dozen lines above the paragraph explaining the CLI is not a consumer.

And the audit your round-14 finding prompted, now recorded on MAX_VALUE_LENGTH so nobody re-derives it: the per-value ceiling was the only guard lost with UriTemplate.expand. The template-length (1e6), expression-count (1e4) and variable-name-length (1e6) limits all live in the SDK's parse(), which runs from the constructor — and strict expansion still constructs a UriTemplate, so they remain enforced. The restored value is the SDK's MAX_VARIABLE_LENGTH, read from the pinned dist rather than assumed to be 1e6.

npm run ci passes.

[Copilot]

Pull request overview

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

Suppressed comments (2)

core/mcp/uriTemplate.ts:540

• prefixUnits keeps each %XX byte as one unit, but RFC 6570 requires prefix lengths to count Unicode characters without splitting the octets of a multi-octet encoded character. For example, {+v:1} with v = "%C3%A9x" currently truncates to %C3, splitting the encoded é; it should retain the complete %C3%A9. Tokenize valid percent-encoded UTF-8 sequences as code points (while defining safe behavior for invalid sequences) and add a multibyte pct-encoded prefix case.

function prefixUnits(value: string): string[] {
  return value.match(/%[0-9A-Fa-f]{2}|[\s\S]/gu) ?? [];

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

• This preview path bypasses the new per-value ceiling: tryExpandUriTemplate rejects values over 1,000,000 characters before the allocation-heavy encoders run, but the panel then calls previewUriTemplate, which reaches expandTemplateExpression here with the same oversized value and encodes it during render. An arbitrarily large paste can therefore still block the UI despite the guard. Share the core length validation with the partial-preview path and fall back before encoding.

        return part.names.some((name) => Object.hasOwn(defined, name))
          ? expandTemplateExpression(part, defined)
          : part.source;

[cliffhall]

ping @copilot — round 16 addressed in 7d9410cb. Both real, and both refinements of the previous two rounds rather than defects in the original code.

1. A multi-octet pct-encoded character was being split. Round 11 settled on "a triplet counts as one character", which is right for %61 and wrong for %C3%A9 — that is one é across two triplets. Measured on a3dd8563:

x{+v:1}  v = "%C3%A9x"       =>  "x%C3"           // lone lead byte, decodes to nothing
                             =>  "x%C3%A9"        // now
x{+v:1}  v = "%E2%82%ACy"    =>  "x%E2%82%AC"     // three-octet
x{+v:1}  v = "%F0%9F%98%80z" =>  "x%F0%9F%98%80"  // four-octet

prefixUnits recognizes a well-formed sequence from its lead byte (RFC 3629: C2-DF two octets, E0-EF three, F0-F4 four, continuations 80-BF), longest alternative first so a four-octet sequence is not read as a two-octet one plus stragglers.

On your "define safe behavior for invalid sequences": a lead byte with too few continuations (%C3x) or a stray continuation (%A9%C3%A9) matches no sequence alternative and falls back to per-triplet counting. That is deliberately the conservative direction — it counts as more units, so truncation keeps less rather than emitting octets that were never in the value. Both are tested.

2. The value ceiling only covered the read. Correct, and it made round 14 half a guard: the panel calls previewUriTemplate on every keystroke, that path expands expression by expression, and it reached the encoders with the same oversized value — so submission was refused while the UI still froze on the paste that caused it. Both paths now share one exported valueLengthError, checked before any encoding, with tests at the limit and one past it on both sides.

npm run ci passes.

[Copilot]

Pull request overview

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

[cliffhall]

ping @copilot — round 17 addressed in 05bbfe50. Real, and a crash rather than a correctness nit — I had been about to call the cycle done, and this changed my mind.

completions["toString"] ?? []   =>  Object.prototype.toString   // a FUNCTION

?? catches only null and undefined, so that went to Mantine's Autocomplete as its data and the field died on first render. Reproduced by reverting the fix under the new test:

TypeError: data.map is not a function

What makes this one interesting is why it survived the earlier prototype rounds. Those fixed the values map — which the panel seeds with every declared variable, so the key is always an own property and a bare lookup happens to be safe. completions is the one name-keyed map this component reads without having seeded it, which is exactly where the hazard was still live. Both of its reads now go through Object.hasOwn: the render, and the stale-dropdown check that clears a variable's options on the next keystroke (an inherited member is not a stale dropdown either).

Only the autocomplete branch can reach it, so the regression drives that branch specifically — completionsSupported, an onCompleteArgument, a {toString} template, then type and submit. One test-side note: the field is queried getByRole("textbox", { name: "toString" }) rather than by label, because "toString" now also appears in the "Still needed" line from round 13.

npm run ci passes.

[Copilot]

ping @copilot — round 14 addressed in c6b17c52. Three findings, all real; each measured on 1c595e96 before acting.

1. A stray } produced a URI with a brace in it. Confirmed, including the...

Fixed in 05bbfe5. Both reads of completions now go through Object.hasOwn — the initial render and the stale-dropdown check — and the regression drives the autocomplete branch with a {toString} template to confirm the crash no longer occurs.

[Copilot]

Pull request overview

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

Suppressed comments (2)

core/mcp/uriTemplate.ts:534

• The UTF-8 grouping accepts malformed sequences that the comment says should fall back to single-triplet counting. For example, %C0%80 matches the two-byte branch even though RFC 3629 permits only C2–DF; the E0, ED, F0, and F4 branches likewise need restricted second-byte ranges. Consequently {+v:1} can retain an entire invalid sequence instead of one %XX unit. Constrain these branches to valid UTF-8 scalar encodings (or validate/decode candidates) so malformed input follows the documented fallback.

const PREFIX_UNIT =
  /%F[0-4](?:%[89AB][0-9A-F]){3}|%E[0-9A-F](?:%[89AB][0-9A-F]){2}|%[CD][0-9A-F]%[89AB][0-9A-F]|%[0-9A-F]{2}|[\s\S]/giu;

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

• This preview path bypasses the strict validator before expanding parts. A template such as x://{a}}/{b} is refused by tryExpandUriTemplate for its unmatched }, but with both fields filled this function previews x://1}/2, a URI the disabled form can never send. SDK limit failures (for example, too many expressions) have the same mismatch. Run the shared template validation before partial expansion, returning the raw template on failure, so the preview preserves its “never over-promise” contract.

  try {
    return parseUriTemplate(uriTemplate)
      .map((part) => {
        if (part.kind === "literal") return encodeLiteral(part.text);
        if (part.invalid) return part.source;

[cliffhall]

ping @copilot — round 18 addressed in 9caac43d. Both real, and both holes in my own recent fixes.

1. The UTF-8 grouping keyed off the lead byte alone. You are right, and my round-16 comment claiming malformed sequences fell back was simply false. Measured on 05bbfe50:

x{+v:1}  "%C0%80x"        =>  "x%C0%80"        overlong
         "%E0%80%80x"     =>  "x%E0%80%80"     overlong
         "%ED%A0%80x"     =>  "x%ED%A0%80"     UTF-16 surrogate
         "%F4%90%80%80x"  =>  "x%F4%90%80%80"  past U+10FFFF
                          =>  "x%C0" / "x%E0" / "x%ED" / "x%F4"   // now

The ranges are now RFC 3629's well-formed ones — C2-DF, E0 only with A0-BF, ED only with 80-9F, F0 only with 90-BF, F4 only with 80-8F. I also pinned the legal boundary cases (%E0%A0%80, %ED%9F%BF, %F4%8F%BF%BF) in the same table, so tightening cannot quietly start splitting valid characters in the other direction.

2. The preview skipped the read's template validation. Confirmed exactly as described — x://{a}}/{b} with both fields filled previewed x://1}/2.

The fix is the one you suggest, and it turns out to subsume what I added in round 12. That round made the preview leave an invalid part standing, which still let the rest of the template expand; the refusal is per template, so x://{a,}/{b} was previewing x://{a,}/2 for something no read would ever send. Validation now lives in one exported templateError that both the read and the preview consult, and the part.invalid branch in the preview is gone as dead code. A template that fails it is shown verbatim — literals unencoded too, since half-normalizing an unreadable template invents a string the server never published.

Two existing preview tests asserted the weaker behavior and now assert this one; templateError has its own tests including a case asserting that anything it rejects also refuses to expand, so the two cannot drift.

npm run ci passes.

[Copilot]

Pull request overview

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

Suppressed comments (1)

core/mcp/uriTemplate.ts:718

• The length guard currently examines every entry in the caller’s map, including variables the template never references. URI-template expansion ignores extra variables, and the previous SDK path only validated values after looking up a declared name, so an unrelated stale/auxiliary value over 1,000,000 characters now makes x://{id} fail even when id is valid. Filter the guard to the names in the parsed template and reuse those parsed parts for expansion.

  const tooLong = valueLengthError(values);
  if (tooLong) throw new Error(tooLong);

[cliffhall]

ping @copilot — round 19 addressed in 7bcf155e. Real, and a divergence my own round-14 guard introduced rather than one in the original code.

tryExpandUriTemplate("x://{id}", { id: "7", stale: "a".repeat(1_000_001) })
  =>  error: The value for "stale" exceeds the 1000000-character limit    // before
  =>  uri: "x://7"                                                       // now

Your reading of the SDK is the decisive detail: it validated inside encodeValue, i.e. only after looking a value up by a declared name, so the guard I restored was broader than the one it was replacing. RFC 6570 ignores an extra variable, so refusing over one is refusing a valid read. valueLengthError now takes the names to consider, and both callers pass the template's declared set — with a test that an oversized referenced value is still refused, so this did not just loosen the guard.

The second half was worth taking on its own. Strict expansion was parsing the template three times — inside templateError, again for the name list, again to expand — on something server-controlled and up to 1 MB. It now parses once and reuses the parts, which meant splitting validation into sdkTemplateError (the constructor's unclosed-brace and length/count limits) and partsError (this module's grammar) so templateError and strict expansion share both halves rather than duplicating either. Same verdicts, one parse.

npm run ci passes.

[Copilot]

Pull request overview

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

[cliffhall]

Review cycle complete — round 20 returned clean (no comments, no suppressed comments) on 7bcf155e.

Fourteen rounds, every finding taken. The shape of them is worth recording, because it says something about where the risk in this PR actually was:

Rounds	Found in	Examples
7–10	the shipped behavior	varspecs accepted but mangled ({id*:3} silently truncated), the empty-string/undefined collapse, missing literal normalization
11–19	my own fixes to those	operator-specific triplet counting, preview losing its error containment, a guard on one path but not the other, then the same guard too broad on both

Two of them were things the SDK had been doing for us that replacing its expander quietly removed — the per-value ceiling and half the brace validation — which prompted an audit of the rest: the template-length, expression-count and variable-name limits all live in the constructor, which strict expansion still calls, so nothing else went missing. That is recorded on MAX_VALUE_LENGTH rather than left to be re-derived.

One correction that matters for the record: #1919 was the web panel's own String.replace, not the SDK expander. encodeURIComponent encodes /, so the SDK handled the issue's foo/bar case correctly all along and the TUI path was never affected. I had asserted otherwise earlier in this PR, including in a commit message; it was caught by reverting the wiring under a new test and finding the test still passed. The regression test now uses a!b — a sub-delim RFC 6570 requires encoded and encodeURIComponent leaves bare — which genuinely distinguishes the two expanders, and reverting the wiring now fails 2 of 10 integration tests.

One decision is deliberately left open for a human, and is a one-line change if you disagree: non-conforming variable names ({default-graph-uri}, {~thing}) expand rather than being refused, with the tolerance labelled (conforming: false on TemplateVariable) rather than hidden in the grammar. The conformance suite rejects them; real servers publish them and the SDK's matcher round-trips them, and for a debugging tool I judged refusing to read a resource that demonstrably works to be the worse failure.

npm run ci passes locally on the merged tree; GitHub CI is running.