From a3dd85638059c4d14aa264f85b7754214745e448 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 17 Aug 2026 00:59:30 -0400 Subject: [PATCH] test: pin the InspectorClient wiring, and correct a false claim about the SDK Addresses Copilot's round-15 review on #2035. 1. `readResourceFromTemplate` had no regression distinguishing this module from the SDK expander it replaced: the existing test uses `report.txt` and the transport tests expand separately and call `readResource`, so all of them pass either way. Writing that test corrected something I had asserted twice in this PR. The first version used `foo/bar` -- and verified by reverting the wiring, it still passed. `encodeURIComponent` DOES encode `/`, so the SDK expands `foobar://events/{topic}` to `foobar://events/foo%2Fbar` exactly as this module does. #1919 was the WEB PANEL's own `String.replace`, not the SDK expander, which the TUI path was using correctly all along. The test now uses `a!b`. `!` is a sub-delim: RFC 6570 requires it encoded, `encodeURIComponent` leaves it bare, so the SDK returns `foobar://events/a!b` and the assertion fails. Re-verified by reverting the wiring -- 2 of the 10 tests fail (this one and the malformed-template refusal), where before the change none did. 2. The module summary said "shared by every client" a dozen lines above the paragraph explaining the CLI is not a consumer. Now says web and TUI. Also records, on MAX_VALUE_LENGTH, the audit prompted by round 14: 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 limit is the SDK's `MAX_VARIABLE_LENGTH`, read from the pinned dist rather than assumed. npm run ci passes. Signed-off-by: cliffhall --- .../integration/mcp/rfc6570-templates.test.ts | 51 +++++++++++++++++++ core/mcp/uriTemplate.ts | 14 +++-- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts index 77b54eae..d71699af 100644 --- a/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts +++ b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts @@ -143,6 +143,57 @@ describe("RFC 6570 resource templates over the wire (#1919)", () => { expect(result.contents[0]?.uri).toBe(uri); }); + // The tests above expand with the helper and then call `readResource`, which + // leaves `InspectorClient.readResourceFromTemplate` -- the API the TUI drives + // and the one this PR rewired -- unproven: they pass whether or not that + // method still uses the SDK's expander. These drive it end to end with a + // value that the two expanders disagree about, so reverting the wiring fails + // here rather than silently. + it("expands and reads a simple expression through readResourceFromTemplate", async () => { + const connected = await connectToShowcase(); + const invocation = await connected.readResourceFromTemplate( + "foobar://events/{topic}", + { topic: "foo/bar" }, + ); + expect(invocation.expandedUri).toBe("foobar://events/foo%2Fbar"); + expect(invocation.result.contents[0]?.uri).toBe(invocation.expandedUri); + }); + + it("encodes a sub-delim the SDK's expander leaves bare", async () => { + const connected = await connectToShowcase(); + // Verified by reverting the wiring: `encodeURIComponent` does encode `/`, + // so `foo/bar` alone does NOT distinguish the two expanders -- the SDK + // handles that case correctly and #1919 was the web panel's own string + // substitution. `!` is where they differ: it is a sub-delim, which RFC 6570 + // requires encoded and `encodeURIComponent` leaves alone, so the SDK + // expander returns `foobar://events/a!b` and fails this assertion. + const invocation = await connected.readResourceFromTemplate( + "foobar://events/{topic}", + { topic: "a!b" }, + ); + expect(invocation.expandedUri).toBe("foobar://events/a%21b"); + expect(invocation.result.contents[0]?.uri).toBe(invocation.expandedUri); + }); + + it("expands and reads a query expression through readResourceFromTemplate", async () => { + const connected = await connectToShowcase(); + const invocation = await connected.readResourceFromTemplate( + "foobar://events{?topic}", + { topic: "foo/bar" }, + ); + expect(invocation.expandedUri).toBe("foobar://events?topic=foo%2Fbar"); + expect(invocation.result.contents[0]?.uri).toBe(invocation.expandedUri); + }); + + it("refuses the malformed template through readResourceFromTemplate", async () => { + const connected = await connectToShowcase(); + await expect( + connected.readResourceFromTemplate("foobar://events/{topic:abc}", { + topic: "news", + }), + ).rejects.toThrow(/Invalid RFC 6570 varspec/); + }); + it("resolves the base URI a blank query expression expands to", async () => { const connected = await connectToShowcase(); // RFC 6570 drops the whole expression when the variable is *undefined*, so diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 485d5a79..5ffc786d 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -1,5 +1,6 @@ /** - * RFC 6570 URI Template parsing and expansion, shared by every client (#1919). + * RFC 6570 URI Template parsing and expansion, shared by the web and TUI + * clients (#1919). * * This lives in `core/` rather than in a client so the web Resources form and * the TUI cannot disagree about what a template means. Both derive their form @@ -710,8 +711,15 @@ export function expandUriTemplateStrict( } /** - * The pinned SDK's per-value ceiling, enforced here because this module no - * longer calls `UriTemplate.expand` — which is where that check lived. + * The pinned SDK's per-value ceiling (`MAX_VARIABLE_LENGTH`, 1e6), enforced + * here because this module no longer calls `UriTemplate.expand` — which is + * where that check lived. + * + * It is the only guard that moved. Auditing the rest of the SDK's expander + * against what this replaced: the template-length (1e6), expression-count + * (1e4) and variable-name-length (1e6) limits all live in `parse()`, which + * runs from the **constructor** — and {@link expandUriTemplateStrict} still + * constructs a `UriTemplate`, so those remain enforced by it. */ const MAX_VALUE_LENGTH = 1_000_000;