Addresses Copilot's round-9 review on #2035.
RFC 6570 2.4.1 counts the prefix in *characters* and is explicit that a
pct-encoded triplet counts as a single one. Truncation went by code point,
so it cut a triplet in half -- measured on 6acaff70:
x{+v:5} v = "%61%62%63%64%65%66" => "x%61%256"
The orphaned "%6" is not a triplet, so encodeAllowReserved then (correctly)
escaped its `%`, and the value went out carrying a byte the caller never
asked for. Now "x%61%62%63%64%65".
Triplets are units only under `+` and `#`. Under every other operator the
encoder escapes a `%` anyway (`%61` -> `%2561`), so the three characters
really are three and grouping them would mis-count the prefix -- asserted
both ways. Astral code points remain single units under both, which is what
`Array.from` was there for.
Also corrects the comment on `readResourceFromTemplate`, which still said
the shared helper drops blank values. It has not since 6acaff70: `params`
passes through as given, so only an absent key omits its expression and a
direct API caller can request `?q=` deliberately. A caller whose values come
from a form drops its own blanks first.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-8 review on #2035 (one inline comment, one
suppressed). Both real.
1. RFC 6570 distinguishes an UNDEFINED variable, which omits its whole
expression, from one defined as the empty string, which expands. The
expander was calling `definedValues` on the way in, collapsing the two --
so `x{?q}` with `{ q: "" }` gave `x` instead of `x?q=`, and there was no
way for any caller, `readResourceFromTemplate` included, to request
either that or `x;q`. Key presence is now honored, including §3.2.7's
rule that the matrix operator drops the `=` for an empty value (`x{;q}`
-> `x;q`, while `?`/`&` keep it).
Dropping blanks is a FORM concern, so it moved to the two form
boundaries: the web panel seeds every declared variable with "" and the
TUI's ink-form returns "" for every untouched field, so neither can tell
"never touched" from "deliberately empty" -- but that is a fact about a
text input, not about the template, and it has no business deciding what
a template means for every other caller.
Six unit tests and one integration test encoded the old collapse and now
assert the corrected meaning. The prototype-collision cases get stronger
rather than weaker: a blank `{?toString}` expands to `?toString=` -- its
OWN empty value -- where the original bug expanded the inherited
function's body.
2. The expansion error is now `role="alert"`. It can appear while typing (a
pasted value that cannot be encoded), and plain text is not announced, so
a screen-reader user met a disabled Read Resource with no stated reason.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-7 review on #2035. Two suppressed comments, both
real; each reproduced against this branch before acting.
1. `parseVarSpec` was a "strip the first `*`, trim, split on `:`" pass, so
it accepted shapes RFC 6570's varspec production does not admit --
measured on the previous commit:
x://{*id} name "id" => "x://abcdef" explode is a TRAILING modifier
x://{id*:3} name "id" => "x://abc" the two modifiers are exclusive
x://{ id } name "id" => "x://abcdef" whitespace is not a varchar
x://{a b} name "a b" => "x://1" ditto -- and it emits into the URI
`{id*:3}` is the worst of them: it silently truncated the value through a
combination the grammar forbids. Parsing is now one anchored regex --
a varname, then at most one trailing `*` or `:1-9999`.
The name set is `varchar` widened by `-` and `~`, deliberately. A name is
the one thing expansion emits into the URI WITHOUT encoding it (`;`, `?`
and `&` write `name=value`), so the rule it has to satisfy is "needs no
encoding" -- RFC 3986 unreserved plus pct-encoded, of which the RFC's own
varchar is a subset. Strict varchar would refuse `{user-id}`, which real
servers publish and which the SDK's matcher round-trips; refusing to read
a resource that demonstrably works is a worse outcome for a debugging
tool than a name one character outside the production. Everything now
rejected is a shape we would otherwise have to guess at.
2. The TUI built its "Missing required template variable(s): ..." list with
its own bare `values[name]` filter while gating on `hasRequiredValues`,
which reads own properties only. For a variable legitimately named
`constructor` the two disagreed: `({})["constructor"]` is `Object`, whose
`.length` is 1, so the list came out empty and the error named no field
at all while the submit stayed blocked. Both now come from one exported
`unmetRequiredGroups`, with `hasRequiredValues` defined in terms of it.
Also renames a test whose title still said multi-name query expressions were
left "to the SDK, which handles them" -- expansion has been owned here since
935113d1; the behavior it asserts is naming, not delegation.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Ports the two behaviors PR #2033 (the parallel attempt at #1919, now closed
as a duplicate) got right and this branch did not.
1. An expression declaring no variable is a malformed template, not one
with a member to skip. RFC 6570 requires at least one varspec per
expression and admits no empty member, so `{}`, `{,}`, `{a,}`, `{?}`
and `{*}` (an explode modifier is not a name) now make the template
invalid. Skipping them was the more dangerous reading: `x://{}`
expanded to `x://` while the form rendered no inputs, so its
"everything required is filled" check was vacuously true and it
submitted a URI that is not the template the server published.
2. The read is withheld when expansion fails. `expandUriTemplate`'s
raw-template fallback exists for the preview, which runs during render
-- but the submit path used it too, so an invalid template was read
with its braces intact and the server answered with a confusing "not
found" for a defect that is not the user's. `tryExpandUriTemplate`
returns the URI or the reason as a value the caller cannot mistake for
one; the panel gates Read Resource on it and prints the reason.
This also 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.
`requiredGroups` now skips an expression naming no variable: an empty
group can never be satisfied, so it would gate the form a second time on
a condition nothing can meet, and the "any one of" hint built from it
would name no fields. The accurate reason is the malformed template,
which the expansion gate reports.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-6 review on #2035.
`toString`, `constructor`, `valueOf` and `__proto__` are all valid RFC
6570 variable names -- `varname` allows ALPHA / DIGIT / `_` / pct-encoded
-- and every lookup here was a bare `values[name]`, which finds
`Object.prototype`'s member for each of them. Measured: `({})["toString"]
!== undefined` is true with typeof "function", and `({}["constructor"] ??
"").length` is 1.
So a *blank* `{?toString}` field read as supplied and expanded a function
body into the URI instead of omitting the expression, and
`hasRequiredValues([["constructor"]], {})` returned true because
`Object.length` is 1 -- a required group satisfied by nothing.
All reads now go through an own-property `readValue`, and the web
preview's filled-check uses `Object.hasOwn`. `expandExpression` carries
the value through its filter rather than re-reading it afterwards, so the
"is it defined" test and the read cannot disagree and nothing needs a
non-null assertion.
The remaining six findings were documentation drifting behind the code,
which matters here because the comments are the argument for why this
module exists:
- The web helper still described the SDK's second-`?`-to-`&` rewrite as
something the preview relies on. That was removed last round.
- Three places still named the CLI as a consumer. It has no template form
and passes an already-expanded `--uri` to `readResource`.
- The PR description still described the superseded architecture
(expansion delegated to the SDK), i.e. the opposite of what now ships.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-5 review on #2035 — eight suppressed comments,
all of them real.
The consequential one overturns behavior I copied from the SDK and
defended in two earlier rounds. RFC 6570 expands each expression
independently; it carries no cross-expression state. The SDK tracks
whether a query expression has already emitted and rewrites a later
`{?two}`'s `?` to `&`. That looks friendlier and is wrong, and its own
matcher says so: for `x{?one}{?two}` it expands to `x?one=1&two=2`, and
`match("x?one=1&two=2")` on that same template returns **null**, while
`match("x?one=1?two=2")` returns both variables. So the rewrite emitted a
URI the advertised template cannot match — #1919 one level up. A server
that wants a continuation advertises `{?one}{&two}`, which expands to `&`
and matches; producing that shape is the server's choice to declare, not
ours to infer.
Also:
- `{id:abc}` was silently treated as `{id}`, and a test codified it. RFC
6570's max-length is `%x31-39 0*3DIGIT`, so `{id:}`, `{id:0}`,
`{id:abc}` and `{id:10000}` are invalid templates rather than templates
with an ignorable modifier — and the SDK's constructor accepts them all,
so nothing else rejects them. Strict expansion now throws; lenient
returns the raw template. Discovery stays lenient so the panel still
renders rather than going blank.
- The TUI could submit `{a,b}` completely blank while the web panel
blocked the same request: ink-form has no way to express "any one of
these", so its members are left optional and the modal needs its own
group check. Added, with tests — it was also below the coverage gate
without them (functions 76.92%).
- Added the integration test the config never had: it resolves the
checked-in config and drives all four reads over a real transport, so a
misspelt preset fails there rather than only when someone runs the repro
by hand.
- "All three clients" was false. The CLI has no template form and its
`resources/read` passes the already-expanded `--uri` straight to
`readResource`, so nothing here runs for it. Corrected in the README,
AGENTS.md, and the module comment, and dropped a docblock still
describing the SDK-delegation path removed last round.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-4 review on #2035 — again reported as "no new
comments" while carrying three suppressed ones. All three reproduced.
1. The TUI form still named its fields from the SDK. `uriTemplateToForm`
read `UriTemplate.variableNames`, which mangles a name: `{;id}` yields
";id" and `{id:3}` yields "id:3". The form therefore submitted
`{ ";id": "7" }` while the shared expander looked up `id`, found
nothing, and dropped the expression — the value vanished silently.
This is the finding that mattered most, because it falsified the claim
this change is built on. Moving the expander into core only makes the
clients agree if each client's FORM derives its names from the same
parser: a form submits under the names it rendered. The TUI now reads
`templateVariables`, and the SDK template is constructed only to
validate, preserving the existing malformed-template diagnostic.
Its `required` flag comes from `requiredGroups`, and only a variable
that is the sole member of a non-omittable expression is marked: ink-
form cannot express "any one of these", so marking every member of
`{a,b}` required would refuse input the expander accepts.
2. `encodeURIComponent` is not RFC 3986's unreserved set — it leaves the
sub-delims !'()* bare, which RFC 6570 requires encoded for every
operator except + and #.
Fixing that settled a design question left open in the previous round.
With two encoders, the SAME value encoded differently depending on
whether its expression happened to carry a modifier, since only the
modifier pushed it onto our path. So delegation is gone entirely: one
expander, one set of rules. The SDK's `UriTemplate` is still
constructed, but only to validate a template.
3. The "Any one of: a, b" hint could contradict the disabled submit
button. A name can sit in a singleton required group AND a shared one
(`x://{a}/{a,b}`), where the singleton demands that exact field. The
hint is suppressed in that case.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-3 review on #2035. It again reported "no new
comments" while carrying two suppressed ones; both reproduced.
1. Requiredness could not live on a variable at all. Deduplication kept the
first occurrence's group, so in `x{?a}{?b}{a,b}` both names ended up
required with singleton groups and the form demanded both -- while the
SDK expands that template with only `a` to "x?a=11". Widening the stored
group would not have been enough either: in `{a,b}{a,c}`, filling `b` and
`c` satisfies both expressions, which no per-variable flag can express.
So requiredness is now returned per expression by `requiredGroups`, and
`hasRequiredValues` asks that each group be satisfied by any one of its
names. `TemplateVariable.groupNames` is gone rather than left as a field
that quietly means something narrower than it reads; `required` remains,
documented as driving the "Optional" marker and nothing else.
2. `encodeURI` is not the allow-reserved encoder `+` and `#` call for. It
escapes `[` and `]`, which are reserved and must survive, and it escapes
`%`, so an already-encoded value is double-encoded. Measured:
"[::1]" -> "%5B::1%5D" and "%2F" -> "%252F". Both corrupt the URI rather
than merely over-escaping it -- an IPv6 literal or a pre-encoded path
reaches the server altered, which is the same class of defect #1919 is
about.
Added an RFC 6570 3.2.1 encoder that preserves reserved characters and
existing pct-triplets, splitting on `%XX` so a lone `%` is still encoded
to `%25`, and matching with the `u` flag so an astral character is
encoded whole. `+` and `#` expressions are now taken over even for a
single name, so both expansion paths agree on what those operators mean.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-2 review on #2035. It reported "no new comments"
but carried three *suppressed* ones; all three reproduced against the
pinned SDK.
1. The `;` path-parameter operator is absent from the SDK's operator list,
so `{;id}` parsed as a variable literally named ";id" and expanded to
nothing. Added the operator and its named expansion (`;a=1;b=2`).
2. An RFC 6570 prefix modifier was folded into the variable name: `{id:3}`
yielded a variable called "id:3" and expanded to nothing. Varspecs are
now parsed properly and the value truncated before encoding. Truncation
is by code point, since `String.prototype.slice` counts UTF-16 units and
would split an astral character into a lone surrogate.
For both of these the wrong URI is 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 use.
3. Requiredness was applied per variable, but it is a property of the
*expression*: 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 too. 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"
rather than falsely claiming each is mandatory.
Two structural consequences, each pinned by a test:
- Takeover is now per TEMPLATE rather than per expression. Splicing
corrected fragments into a template the SDK re-expands would leave its
cross-expression `?`-to-`&` rewrite blind to the fragments already
resolved.
- Expansion is split into a strict variant that throws and a lenient one
that returns the raw template. `readResourceFromTemplate` wraps the
thrown error with the template name -- three pre-existing integration
tests assert that -- while the form must not throw on a server-supplied
template, since that 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.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's review on #2035. All three findings reproduced against
the pinned SDK before acting.
1. `#` was misclassified as required. Measured: `x://a{#frag}` with no
`frag` expands to exactly `x://a`, a well-formed URI naming a real
resource -- unlike `{+path}` (`x://a/`) or a simple `{userId}`
(`file:///users//profile`), which leave an empty path segment. Moved `#`
into the omittable set; the required cases now assert what the URI
*becomes* when blank, so the rule is checked rather than asserted.
2. Delegating to the SDK did not actually give RFC 6570 expansion.
`UriTemplate.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"` expands to `x://foo/bar,q` -- the very
unencoded-slash defect this PR is about -- and `x://a{/p,q}` to
`x://ax y,z`. Only `?`/`&` are correct, being dispatched earlier.
Fixing that in the web client alone would have left the TUI and CLI
wrong, since `readResourceFromTemplate` expands through the same class.
So parse/classify/expand now live in `core/mcp/uriTemplate.ts` and both
call sites use it. The correction is surgical: a multi-name non-query
expression is expanded here 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 did not work.
`UriTemplate.match()` compiles `{?topic}` to a *required*
`\?topic=([^&]+)`, so `match("foobar://events")` returns null and the
read 404s. A real server exposes the unfiltered collection as its own
resource; the showcase now registers `foobar://events` so the
documented step resolves. Verified end to end through the CLI.
Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall