Addresses Copilot's round-19 review on #2035 -- a divergence introduced by
my own round-14 guard rather than by the original code.
The SDK validated a value inside `encodeValue`, i.e. only after looking one
up by a DECLARED name. The restored guard checked the caller's whole map, so
an unrelated entry failed a template that never mentions it. Measured on
9caac43d:
tryExpandUriTemplate("x://{id}", { id: "7", stale: "a".repeat(1_000_001) })
=> error: The value for "stale" exceeds the 1000000-character limit
RFC 6570 ignores an extra variable, so this refused a perfectly valid read.
`valueLengthError` now takes the names to consider and both callers pass the
template's declared set.
Copilot's second half is worth taking on its own: strict expansion parsed
the template THREE times -- inside `templateError`, again for the name list,
again to expand -- and the template is server-controlled and may be 1 MB.
It is now parsed once and reused, with validation split into
`sdkTemplateError` (the constructor's unclosed-brace and length/count
limits) and `partsError` (this module's own grammar), so `templateError` and
strict expansion share both halves rather than duplicating either.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-18 review on #2035. Two suppressed comments, both
holes in my own recent fixes rather than in the original code.
1. Round 16's grouping keyed off the length-announcing lead byte alone, so
it accepted sequences RFC 3629 forbids -- and the comment claiming they
fell back to per-triplet counting was simply wrong. 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
The lead-byte ranges are now the 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 -- so
each of the above falls back to one triplet, which is what the fallback
was always documented to do. The legal boundary cases (%E0%A0%80,
%ED%9F%BF, %F4%8F%BF%BF) are tested alongside, so tightening the ranges
cannot quietly start splitting valid characters.
2. The preview never ran the read's template validation. It expands
expression by expression, so `x://{a}}/{b}` with both fields filled
previewed `x://1}/2` -- a URI whose stray `}` makes every read refuse.
Validation moved into an exported `templateError` that both callers
consult, so they cannot drift. It subsumes the narrower rule round 12
added here ("leave the invalid part standing"), which still let the REST
of such a template expand: the refusal is per template, not per
expression, so the preview now shows the template verbatim -- literals
unencoded included, since half-normalizing an unreadable template invents
a string the server never published. Two existing tests asserted the
weaker behavior and now assert this one.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-17 review on #2035. A real crash, and a class the
earlier prototype work missed rather than a narrower version of it.
`toString`, `constructor` and `__proto__` are valid RFC 6570 variable names,
and `completions` starts as `{}`. So for such a name:
completions["toString"] ?? [] => the prototype's FUNCTION
`??` catches only null and undefined, so that function went to Mantine's
Autocomplete as its `data`, and the field crashed on first render with
`TypeError: data.map is not a function` -- reproduced by reverting the fix
under the new test.
The earlier rounds fixed this for the *values* map, which is seeded with
every declared variable and so always has the key as an own property. The
completions map is the one name-keyed map this component reads WITHOUT
having seeded it, which is what made it the surviving instance. Both reads
now go through `Object.hasOwn` -- the render, and the stale-dropdown check
that clears a variable's options on the next keystroke.
Only the autocomplete branch could reach it, so the regression drives that
branch: completions supported, a `{toString}` template, type and submit.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-16 review on #2035. Two suppressed comments, both
real, and both refinements of fixes from the previous two rounds rather than
defects in the original code.
1. Round 11 settled on "a pct-encoded triplet counts as one character", but
a multi-octet character spans several triplets -- `%C3%A9` is one `é`.
Measured on a3dd8563:
x{+v:1} v = "%C3%A9x" => "x%C3" a lone lead byte, decoding to nothing
`prefixUnits` now recognizes a well-formed pct-encoded UTF-8 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 followed by loose triplets.
A malformed sequence -- a lead byte with too few continuations, or a stray
continuation -- matches no sequence alternative and falls back to
per-triplet counting. That is the conservative direction: it counts as
MORE units, so truncation keeps less rather than emitting octets that
were never in the value.
2. Round 14's per-value ceiling only covered the read. The panel calls
`previewUriTemplate` on every keystroke, and that path expands expression
by expression, so it reached the allocation-heavy encoders with the same
oversized value -- submission refused while the UI froze on the paste
anyway. Both paths now share one exported `valueLengthError`, checked
before any encoding happens.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
Addresses Copilot's round-14 review on #2035. Three suppressed comments,
all real; each measured on 1c595e96 before acting.
1. A brace surviving in a literal is an unmatched one, and not a legal
literal character. The SDK's constructor rejects an unclosed `{` but
reads a stray `}` as text, and this parser did the same:
expandUriTemplateStrict("x://a}", {}) => "x://a}"
-- a "URI" carrying a brace, with the panel enabling the read on it.
Both halves are now refused the same way, so the withhold behavior
covers the whole class rather than the half the SDK happened to catch.
2. Replacing `UriTemplate.expand` also removed its 1,000,000-character
per-value ceiling. Nothing else bounded what reached the
allocation-heavy encoders, so a pasted or programmatically supplied
megabyte-plus value went straight through. Restored at the SDK's own
limit -- the guard it was, not a new policy -- and the error names the
offending variable.
3. `templateVariables` re-scanned an expression's whole varspec list for
every one of its names to compute `conforming`, which is quadratic in a
multi-name expression. Templates are server-controlled and the SDK
admits one up to 1 MB, so a single expression can carry thousands of
short comma-separated names, and this runs on the render thread.
Iterating the varspecs supplies the name and its conformance in one
pass.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-13 review on #2035. Two suppressed comments, both
real, both in the required-group work from round 7 rather than in the recent
preview rework.
1. `{a,a}` is ONE requirement named twice, but `requiredGroups` stored it as
a two-name group, so everything downstream read it as "either of these
will do". The TUI was left with a form it could not submit: its
`length === 1` test marked `a` optional while ResourceTestModal's guard
still refused a blank. The web panel offered the useless hint
"Any one of: a, a". Names are now deduplicated within the expression --
expansion still emits both occurrences (`x://{a,a}` -> `x://1,1`), which
RFC 6570 requires.
2. The per-field hint used `find`, so a variable in several shared groups
advertised only the first. With `{a,b}{b,c}{a,c}`, filling `b` satisfies
the first two and every visible hint then looks met while Read Resource
stays disabled on the unmet `{a,c}` -- a requirement with nothing on
screen pointing at it.
Fields now hint every shared group they sit in. That alone is not enough,
because no per-field hint can say WHICH group is still outstanding, so
the form states it directly: "Still needed: a or c", built from the same
`unmetRequiredGroups` the submit gate uses, so the message and the button
cannot disagree. `hasRequiredValues` is no longer called here -- the
panel derives both from the one list.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-12 review on #2035. Both halves are regressions
from my own round-10 rework, which replaced "expand a rewritten template
through the lenient expandUriTemplate" with a part-by-part assembly and so
lost two properties that implementation had for free. Reproduced on
c6fb6271:
previewUriTemplate("x://{v}", { v: "\ud800" }) => throws URIError
previewUriTemplate("x://{a,}", { a: "1" }) => "x://1"
The first is the worse one: the preview runs during render, so an escaping
throw unmounts the whole panel rather than disabling its button -- and an
unpaired surrogate is real input, since `encodeURIComponent` raises URIError
on one and a paste can deliver it. The panel already reports that case
properly through `tryExpandUriTemplate`; the display path now degrades to
the raw template instead of taking the panel down with it.
The second promised a URI the user could never send: `{a,}` is an invalid
template whose submission is refused, but `a` survives into the
expression's varspecs, so filling it previewed `x://1`. An invalid
expression is now left standing as written, which is what the rest of the
panel already says about it.
Tests for both, plus one asserting the valid expressions around an invalid
one still preview.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-11 review on #2035, which corrects my own round-9
fix rather than the original code.
That fix grouped pct-triplets only under `+` and `#`, on the reasoning that
every other operator escapes a `%` anyway, so `%61` is three characters
rather than one. Wrong, in a way the narrower rule made visible one operator
over -- measured on 2e06b369:
x/{v:1} v = "%61%62" => "x/%25" truncated to a `%` that was never
a character of the value
RFC 6570 2.4.1 counts characters and says a pct-encoded triplet counts as a
single one, without reference to the operator. Grouping per-operator also
made the same input mean two different things: `%61` indivisible under `+`,
three loose characters under `{v}`. What the operator decides is how a
retained unit is *encoded* afterwards -- a simple expansion still escapes
the kept triplet's `%`, so `%61` goes out as `%2561` -- not what counts as
one character.
`prefixUnits` no longer takes an operator, and the test that codified the
old split now asserts the uniform rule across simple, named, and reserved
expansion.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-10 review on #2035. One inline comment, one
suppressed; both real.
1. Literals were copied through verbatim, but RFC 6570 3.1 requires them
pct-encoded on expansion -- the conformance case `café/{var}` expands to
`caf%C3%A9/value`. Measured on ef3f5e74 it produced `café/value`, raw
UTF-8 in the path; since this module replaced the SDK's expander for both
clients, nothing else was going to encode it. (The pinned SDK does not
either -- it returns `café/value` too -- so this is a gap we inherited
rather than introduced.)
`encodeLiteral` keeps reserved delimiters (a literal's `/`, `?` and `#`
are structure) and existing pct-triplets, and encodes the rest. A BRACE
passes through: it can only reach a literal from a malformed template,
where an unclosed `{` becomes trailing text, and `%7B` would leave a
string resembling neither the template nor anything the server can match.
This is also what removed the preview's placeholder machinery. No token
can survive a round trip now that literals are encoded, so the preview
assembles part by part instead -- `encodeLiteral` for literal runs,
`expandTemplateExpression` for filled expressions, `part.source` for the
rest. Both halves still come from the shared expander, so the preview
cannot promise a URI that submitting would not send, and the question of
what token could never collide with a server-supplied template is gone.
2. On the varname set I took the structural point rather than the
conclusion. `VARCHAR` is RFC-exact again; `-` and `~` moved into a
separately named `TOLERATED_VARCHAR` tier, and a name needing it parses
with `conforming: false`, surfaced on `TemplateVariable`.
So the grammar in the code says what the RFC says and the tolerance is
explicit and separately tested -- but the Inspector still expands
`{default-graph-uri}`, because real servers publish hyphenated names and
the SDK's matcher round-trips them. Refusing to read a resource that
demonstrably works is a worse failure for a debugging tool than expanding
a name the conformance suite would reject. A caller wanting RFC-exact
behavior refuses on the flag rather than re-deriving the grammar.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
Copilot round 3 (PR #2039): clearing the banner when `selectedServer`
changes is not enough. Start disconnecting A, switch A → B → A, and by
the time the original request rejects both guards match again — the
attempt token is unchanged and the selected server is A once more — so
the stale failure lands on the freshly re-selected A. Bump the token on
every switch, so a round trip invalidates prior disconnects the same way
a one-way switch does.
New test drives exactly that interleaving; it fails without the bump
(the one-way test does not, which is why this needed its own).
Also attaches the before/after TUI screenshots the review asked for —
the review was right that "no UI change" stopped being true once the
banner was added — and corrects the PR description accordingly.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two suppressed review findings.
With an override configured, the wrapper cloned and parsed every
successful application/json response — every tools/call result and every
resource payload on the direct CLI/TUI transports — only to conclude it
was not a metadata document. It now tests the request URL for the RFC
8414 / OIDC well-known paths first, so ordinary traffic costs one string
test and is returned untouched.
The read path also trusted StoredMCPServer's `string` type for the two
override fields, which a hand-edited mcp.json does not honor: the CLI and
TUI read the file directly and then `.trim()` these, so a number would
crash the server load. Guarded with cleanEndpointOverride, mirroring
cleanAuthorizationParams.
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>
Copilot round 2 (PR #2039) surfaced no new visible comments but six
suppressed ones. Four were actionable.
`clients/tui/src/App.tsx` — a stale-completion race in the new banner:
start disconnecting server A, switch to B, then let A's disconnect
reject, and A's failure appears under B. Rapid same-server retries had
the same shape, and the stale attempt is the one most likely to reject.
Claim a monotonic attempt token before awaiting and publish only if it
is still current and the server has not changed; `handleConnect` bumps
the same counter so a connect retires an in-flight disconnect. The
rejection is still owned when superseded — just not published. The
regression test drives a deferred rejection across a server switch, and
fails without the guard.
`tsconfig.lint.json` — the root type-aware block matches
`test-servers/src/**/*.tsx` but the parser project omitted that
extension, so such a file would have failed with "not found in any of
the provided project(s)" instead of being linted. Added, and AGENTS.md
now states the invariant: the include must stay a superset of the lint
globs.
`clients/web/server/{server,vite-hono-plugin}.ts` — the `open()`
rejection branch was untested, and duplicated. Both were fixed by
removing the duplication: `openBrowser` now lives beside
`printServerBanner` in `web-server-config.ts`, with one implementation
to test. Two copies of a one-line `.catch` is two chances for one to
drift back to a bare `open(url)`. The dev plugin is not booted in tests
— it returns early under VITEST by design — but it calls the same
tested helper.
`core/mcp/remote/node/server.ts` — `closeAbortedStream`'s rejecting-close
path, the entire reason the helper exists, had no test. Exported it (as
this file already does for `requestIdForSendWait` / `mcpParamHeadersOnly`)
and covered both legs, asserting no unhandledRejection fires.
Also corrects AGENTS.md, which said each client lists "both" tsconfig
projects — true for cli/tui/launcher, but web lists four leaf projects.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two review findings on the fetch wrapper:
The content-type test was a substring match for "json", which also
matches `application/x-ndjson` — how the streamable-HTTP transport can
serve its long-lived server-push channel (`isLongLivedStreamResponse`).
Awaiting `.json()` on a clone of an unbounded stream never resolves, so
the MCP connection would hang for as long as an override was configured.
It now matches the media type exactly (`application/json` or a `+json`
suffix), with a regression test driven by a stream that never closes.
The overrides also leave `issuer` as discovery returned it, so pointing
one at an authorization server that advertises a different issuer is
rejected on callback by the RFC 9207 / SEP-2352 mix-up defense. That
check is deliberately not weakened; the limit is now stated on the
module, in the field description, and in the configuration guide.
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>
Two review findings on the fetch wrapper:
Reading the body with `response.text()` meant every successful JSON
response was rebuilt while overrides were configured, even when it was
not a metadata document. That dropped native properties a synthesized
`Response` cannot carry (`url`, `redirected`, `type`) from responses the
wrapper has no business altering, and threw outright on a body-less
204/205, whose status Fetch forbids a body on — so an unrelated transport
response could fail. It now parses a clone and returns the caller's own
response unless a document is actually patched.
`new URL` also accepts embedded credentials while Fetch rejects a request
URL carrying them, so `https://user:pass@host/token` passed both the form
and the runtime and then failed deep in the OAuth exchange.
`oauthEndpointUrlError` now rejects a username or password.
Signed-off-by: cliffhall <cliff@futurescale.com>
Review catch (Copilot, PR #2039): folding the rejected disconnect into
`connectError` meant nobody ever saw it. InfoTab renders that value only
when `serverState.status === "error"`, and a failed disconnect leaves the
status "connected" — so the message existed in state and never on screen.
Worse, it fed `connectError ?? inspectorLastError`, where a stale value
would go on masking a later, real connection error.
Give it its own `disconnectError` state and a header banner that renders
regardless of status, cleared on the next connect or disconnect attempt
and on a server switch, so it can neither linger nor be attributed to
the wrong server. The tests now assert the rendered banner rather than
noting that nothing is visible.
Signed-off-by: cliffhall <cliff@futurescale.com>
The overrides are per-server configuration for the *resource* server's
authorization server, but the fetch wrapper that applies them is shared
with the enterprise-managed (EMA) flow, whose IdP OIDC discovery runs
through the same fetch and satisfies the metadata predicate. An
authorization override could therefore redirect the IdP login to the
resource AS, and a token override could send the IdP code or refresh
token there.
OAuthManager.getEndpointOverrides() now returns nothing when the server
is enterprise-managed — the same boundary redirectToExternalAuthorization
draws for the custom authorization parameters (#2018) — with a regression
test, the note surfaced on both form fields, and the rule documented.
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>
A floated promise is invisible at review time — the call reads like an
awaited one minus four characters — and the unhandled rejection it
produces surfaces in a different test, in a different file, as a stack
pointing at SDK internals. In #1947 two un-held `callTool` calls made
`npm run ci` unpassable and took a full investigation to attribute; the
fix was two lines. `@typescript-eslint/no-floating-promises` catches the
class outright, so enable it at `error` everywhere.
The rule is type-aware and `tseslint.configs.recommended` is not, so each
scope's config gains a parser project. The four clients name their own
tsconfig projects (both of them — `src` and `__tests__`/`server` live in
different ones). The root scope has no tsconfig at all, so add
`tsconfig.lint.json`: a lint-only project covering `core/**`,
`test-servers/src/**`, and `vitest.shared.mts`, emitting nothing and
gating nothing. It sets `moduleResolution: bundler` deliberately — under
NodeNext `core/`'s extensionless imports fail to resolve, every import
degrades to `any`, and the rule silently stops seeing promises at all.
Measured surface, and the fixes:
- core (2) — `stream.close()` in two Hono `onAbort` listeners. Hono
invokes subscribers with a bare `subscriber()`, so a promise returned
from the listener is floated one layer up; `closeAbortedStream` owns
the rejection where it can actually be owned.
- tui (5) — `handleDisconnect` had no catch of its own and its only
caller is a key handler that cannot await, so a failing disconnect was
a live unhandled rejection; it now surfaces the message the way
`handleConnect` does. The rest are `void` at callers that genuinely
cannot await, over callees that already own their failures.
- web (92) — 83 are Storybook `expect(...)`, which is instrumented and
returns a promise, now awaited (`scrollAreaStoryAssertions` is async
for the same reason). Two tests were asserting against a provider call
they never awaited. `open(url)` in both web servers now warns instead
of escaping a synchronous callback.
- cli, launcher — already clean.
Type-aware linting costs time: `clients/web`'s `eslint .` goes from ~8s
to ~19s, inside `validate`. Narrowing the loaded projects is the lever if
that needs reducing later; dropping the rule is not.
Closes#1959
Signed-off-by: cliffhall <cliff@futurescale.com>
Both new #2006 regressions faked the events response with an object cast
`as unknown as Response`, which AGENTS.md rejects — it erases the check
that the mock actually satisfies the API the hook consumes.
Each now builds a real `ReadableStream` inside a real `Response`. `pull`
runs once per chunk the hook's reader consumes, so awaiting a promise
inside it gates delivery exactly the way the hand-rolled reader did,
while the mock stays type-checked against the Response contract. The
chunk counter replaces the read counter as the "the reader has consumed
the previous chunk" signal.
Both still fail against the pre-fix parser, verified.
Raised by Copilot on PR #2036.
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds two optional per-server fields — `oauth.authorizationUrl` and
`oauth.tokenUrl` — that override the endpoints authorization-server
metadata discovery resolved, so a server can be pointed at a development
or staging authorization server without changing what it advertises.
SDK v2 routes both endpoints through the discovered metadata document and
neither through OAuthClientProvider, so the override is applied by
wrapping the client's base fetch and patching the metadata document in
flight (core/auth/endpointOverrides.ts). Wrapping the base fetch — rather
than only the auth fetch — also covers the discovery the SDK runs from
inside the transport on the 401/refresh path.
The fields round-trip through mcp.json, the /api/servers routes, the
runner (CLI/TUI), and the Server Settings → Authorization panel, where a
value that is not an absolute http(s) URL is flagged inline; at runtime
such a value is dropped with a single warning rather than failing the
connection.
Signed-off-by: cliffhall <cliff@futurescale.com>
The `/api/servers/events` reader in `useServers` searched for `\n\n` and
consumed exactly two characters as the separator. SSE permits CRLF line
endings, so a `event: change\r\n\r\n` frame stayed buffered forever:
`isSseDataFrame()` was never reached and no background refresh fired,
leaving the server list stale until a manual refresh or an LF-delimited
event arrived.
Frame boundaries are now located with a CRLF-aware matcher that reports
the separator's real length, and frame lines split on either terminator.
A bare CR is deliberately not accepted as a terminator: it cannot be
distinguished from the first half of a CRLF straddling a chunk boundary,
so a buffer ending in `…\r\n\r` would read as a completed frame and the
`\n` arriving next would open a bogus one.
Two regressions cover it, both verified failing against the old parser.
Each uses a referentially stable `fetchFn` and holds the frame behind a
promise released after the disk write — with an inline `fetchFn` the hook
re-subscribes every render and that re-subscription's own mount refresh
picks the edit up regardless, so the test would pass unfixed.
Closes#2006
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>
The Resources screen discovered and substituted template variables with a
bare `/\{(\w+)\}/g` regex, which is wrong two ways: it cannot see an
expression carrying an operator, so `foobar://events{?topic}` rendered no
input at all; and it splices the raw value in, so a `/`, `?`, `#`, `%`,
space or non-ASCII character in a simple `{topic}` landed unencoded and
changed the URI's structure -- `foo/bar` produced an extra path segment
that a conforming matcher rejects with `-32602 Resource not found`.
Delegate expansion to the SDK's `UriTemplate`, the same implementation
`InspectorClient.readResourceFromTemplate` (and so the TUI) already
expands through, so the two clients cannot disagree about what a template
means. The new `utils/uriTemplate` supplies only what that class does
not: which variables to render an input for, which of them a read cannot
proceed without, and a partially-expanded preview.
Required-ness follows the operator. Under `?`, `&`, `.` or `/` the whole
expression is omitted when the variable is undefined, so those fields are
marked Optional and reading with them blank is a legitimate request for
the unfiltered resource; under `""`, `+` or `#` the variable sits mid-URI,
so it stays required. Blank fields are dropped before expanding so an
untouched optional field reads as undefined rather than as the empty
string, which would expand to a valueless `?topic=`.
Adds the `rfc6570_templates` preset and a `rfc6570-templates-http.json`
showcase server serving the two templates from the issue.
Closes#1919
Signed-off-by: cliffhall <cliff@futurescale.com>
`ServerTasksCapability`'s `list`/`cancel` are objects, not booleans, so
the legacy-tasks fixture failed `tsc -b` in CI. A stale incremental
build cache hid it locally.
Signed-off-by: cliffhall <cliff@futurescale.com>
SEP-2663 moved task support from `capabilities.tasks` to the negotiated
`capabilities.extensions["io.modelcontextprotocol/tasks"]`, but the
Connection Info modal's capability grid tested only for the top-level
key. A modern server that advertised the extension in `server/discover`
therefore showed "Tasks ✗" while the very same extension id was listed
under "Server Extensions" two sections below.
Server capability rows now fall back to an extension id for the
capabilities the modern era expresses that way (tasks is the only one
today), gated on the negotiated era so a legacy connection is still
judged purely on its `initialize` capabilities. Keyed the same way
`InspectorClient.isTasksExtensionNegotiated()` gates the Tasks tab, so
the checkmark and the tab can't disagree.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings from the Copilot review of #1965, both real.
1. The candidates were flattened to package names before the version
comparison, so a third install holding a different copy failed the guard
even when no program loads that copy beside another — and the diagnostic
named an install that never took part. `crossInstallPackages` now returns
the co-occurrences themselves (name → program → install → entry paths) and
`findSkew` compares per program, across only the installs that met there.
2. Folding a nested copy onto its outermost install lost the path that
identifies which copy was loaded, and the version was then read from the
install's top-level entry — absent or differently versioned, either way
letting a real pair pass. `classifyModulePath` now also returns the
`entryPath` npm keys the lockfile by, `topLevelLockVersions` becomes
`lockVersionsByPath`, and each holder is priced from the entry the program
resolved. A resolved copy with no lockfile entry is now a hard failure
rather than a silently dropped holder.
The failure output prints `in <program>` with each holder's install path
beside its version, so a nested copy is distinguishable from a top-level one.
Signed-off-by: cliffhall <cliff@futurescale.com>
`verify:dep-lockstep` derived its candidate set from the packages the shared
first-party sources named *directly*, so a package whose declarations reach a
program only through another package's `.d.ts` was invisible to it — it could
skew across installs and recreate the two-copy blow-up #1896 was about while
the guard stayed green. `@modelcontextprotocol/sdk` was the live example: never
written in first-party code, yet 16 of its `.d.ts` files land in clients/web's
test program.
Replace the derivation with the measured one: list every client tsconfig
project with `tsc --listFilesOnly`, map each resolved `node_modules` file to
its owning install and package, and keep the packages that reach ONE program
from TWO installs. That is exactly the set that can put two structurally
distinct copies of a type in front of one checker. The rest of the guard is
unchanged — top-level lockfile comparison, deny-by-default, `TOLERATED_SKEW` as
a name allowlist scoped to within-a-major.
The listing machinery moves to `scripts/lib/tsc-program.mjs`, shared with
`verify:typecheck-coverage`, so the two guards can't disagree about what a
program contains. It is deliberately not cached to disk between the two
processes: a stale fingerprint would make a guard measure a program that no
longer exists and pass on a real miss. The second pass costs ~14s.
`TOLERATED_SKEW` is now empty. `jose` and `@modelcontextprotocol/ext-apps` are
root-only since #1970 and cannot skew; `react` (whose types are `@types/react`,
resolved from one install) and `hono` never reach one program from two
installs. Each would be a rationale for a skew that cannot occur.
Closes#1965
Signed-off-by: cliffhall <cliff@futurescale.com>
Extends the previous commit's fix rather than changing it. `waitUntil` now
settles, but the same window is open wherever a key is written without having
just polled: `press()` reaches its `stdin.write` on a turn that may still have
React's passive-effect flush queued (straight after a plain `tick`, or after an
earlier key committed a render), and `mount()`'s absorbing keypress lands one
tick after the mount commit. Both now `await settleInputHandlers()` before
writing, so "frame visible" implies "input handlers armed" at every dispatch
point, not only after a frame wait.
Adds a `test helpers` case that pins the contract: a `setImmediate` sentinel
stands in for the pending effect flush and must have run by the time
`waitUntil` resolves. Verified it fails when the yield is removed, so a future
regression surfaces here instead of as a differently-named flaky OAuth test
under coverage instrumentation.
Signed-off-by: cliffhall <cliff@futurescale.com>
