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 <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-17 00:59:30 -04:00
commit a3dd856380
2 changed files with 62 additions and 3 deletions

View file

@ -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

View file

@ -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;