The direct server→client `elicitation/create` handler ignored its request
context, so `notifications/cancelled` aborted `ctx.mcpReq.signal` and nothing
was listening: the modal and its bridge outlived the cancelled request and
could still answer work the server had abandoned.
The handler now threads that signal into `enqueuePendingElicitation`, which
already wires it to both answer surfaces — the native queue entry and the app
renderer. The task-augmented branch deliberately does not use it: that request
is answered immediately with a `CreateTaskResult`, so its lifetime is the
task's, which carries its own abort.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two of the three findings; the third is answered in the PR thread.
**A replaced client could still enqueue.** The one-shot sweep only rejected what
was already queued, but the outgoing `InspectorClient` disconnects
asynchronously and can enqueue during its own teardown — and that late entry
would be rendered by a factory bound to the REPLACEMENT client, reading its
resource and answering through a different server. Requests now belong to a
session: each constructed client gets one, closing it rejects that session's
entries AND refuses anything it queues afterwards, and another session's
entries are untouched.
**The advertisement no longer has to guess.** Whether a sandbox exists is only
known once `/api/config` resolves, and the answer is baked into the client at
construction — guessing "available" over-claims, guessing "unavailable" strands
the whole session on the native form despite having a sandbox. The connect path
now awaits that fetch (already in flight since mount, so no human ever waits;
it only orders a deep-link auto-connect racing the same page load), after which
an absent `sandboxUrl` means confirmed-absent.
Signed-off-by: cliffhall <cliff@futurescale.com>
`tasks/cancel` marked the task cancelled and rejected its payload, but left
whatever was collecting the answer running — the native pending-request entry,
and now an app-rendered elicitation's renderer and bridge. A modal therefore
outlived the task it belonged to, and an answer arriving afterwards re-settled
it, overwriting `cancelled` with `completed`: a task the server had been told
it cancelled.
Each receiver task now owns an `AbortController`, aborted by `tasks/cancel`
and by session teardown. The task-augmented elicitation branch passes its
signal to the app attempt and wires it to the native queue entry, so both
answer routes are torn down the same way. Both settle helpers return early on
a terminal status, so a late answer through either route cannot resurrect the
task.
The overwrite predates the app path — a native answer after a cancel did the
same thing — so the guard fixes both.
Signed-off-by: cliffhall <cliff@futurescale.com>
**The task-augmented inbound branch bypassed the routing.** A server→client
`elicitation/create` carrying `params.task` answers immediately with a
`CreateTaskResult` and settles the TASK when the user answers, so it cannot
ride `enqueuePendingElicitation` — and it was therefore never offered to an
app, even with a valid `_meta.ui.resourceUri`. It now makes the same attempt,
completing the task with the app's result and falling back to the native queue
otherwise; an abort fails the task rather than reopening it natively. The two
task-settling closures are extracted so both answer routes settle it
identically.
**The web client advertised the capability without a sandbox.**
`useInitialConfig` leaves `sandboxUrl` undefined while `/api/config` is in
flight and when the sandbox controller could not start, but the renderer was
supplied unconditionally — so the client claimed it could host an app in
exactly the window where it could not. The option is now gated on a resolved
sandbox URL: such a connection behaves like the CLI/TUI, native queue and no
claim made.
**Concurrent modals each owned the keyboard.** Every open `Modal.Root`
installed its own focus trap, Escape handler and overlay, so the traps fought
and one Escape could dismiss more than one pending request. Only the topmost
owns them now; the rest stay mounted, keeping their bridges and handshakes,
but inert to the keyboard.
Signed-off-by: cliffhall <cliff@futurescale.com>
The hand-rolled accept check was shallower than the bridge's, so frames it
rejects could still set `elicitation` here — `appInfo: {}` passed, though
`McpUiInitializeRequestSchema` requires an implementation name and version.
Any approximation drifts; the schema itself does not, and ext-apps exports it.
Capabilities are still read from the ORIGINAL frame rather than the parse
output, since that schema is precisely what strips `elicitation` — validating
with it and reading through it would defeat the module. The one check the
schema cannot make is that the frame is a request at all (a notification
carries the same method and params), so the id check stays.
Also stops treating a truthy non-object as an advertisement: the draft declares
`elicitation` as an object, so `elicitation: true` is a value it does not
define and the bridge's schema will reject it the moment it carries the key.
Signed-off-by: cliffhall <cliff@futurescale.com>
The previous round froze the recorded capabilities at the first handshake, on
the belief that the bridge ignores re-initialization. It does not: ext-apps
1.7.5's `_oninitialize` warns about the double-mount and then assigns
`_appCapabilities` and `_appInfo` from the new frame — "the latest appInfo/
appCapabilities replace the previous values", in its own words. Freezing left
this gate reporting capabilities the bridge no longer held, in both directions:
a handshake advertising `elicitation` followed by one without it still read as
advertised.
Every frame that passes the accept gate now replaces the recorded value, and
`appCapabilities` joins `protocolVersion` and `appInfo` in that gate since the
bridge's own schema requires all three. A frame the bridge would reject still
records nothing AND leaves the previous value alone — it is a route to changing
the gate in neither direction.
Signed-off-by: cliffhall <cliff@futurescale.com>
The capability observer recorded `params.appCapabilities` from ANY frame whose
method was `ui/initialize`, before the bridge validated it. A view could
therefore send a second, malformed initialize — one the bridge rejects, keeping
the capabilities it had already accepted — and flip `elicitation` on in this
gate, after which the host would forward an elicitation the bridge never
negotiated.
Now only a well-formed initialize REQUEST is recorded (a JSON-RPC id, plus the
handshake's required `protocolVersion` and `appInfo`), and only the first one:
the bridge keeps what it accepted and ignores re-initialization, so this gate
agrees rather than offering a second, laxer path to the same flag. A malformed
first frame records nothing.
This is a gate, not a second copy of the bridge's schema — the bridge stays the
authority on the rest of the frame.
Signed-off-by: cliffhall <cliff@futurescale.com>
Both review findings from round two.
The routing change sits in the funnel BOTH entry points share, but only the
legacy inbound handler was driven by a test — the modern leg, where an
elicitation arrives as an `input_required` result the MRTR driver unpacks and
retries, was covered by construction alone. Adds
`mrtr_app_choose_option` (a modern MRTR tool whose *embedded* elicitation
carries `_meta.ui.resourceUri`) and drives it live over a real transport: the
app answers, the retry carries that answer through `inputResponses`, and the
server echoes it back as the tool's result. A second case cancels the tool
call while the app is up and asserts the request-scoped signal aborts; a third
runs the same tool against a modern server that never advertised the
capability and asserts the native queue takes it.
The cancellation case turned up a real gap: `tryAppElicitation` mounted an app
for an already-aborted signal (the caller cancelled during an earlier MRTR
round), leaving a modal nobody was waiting on and a promise that could never
settle. It now throws the abort straight through.
Also fixes the smoke count in the root README, which still said "two"
headless-Chromium smokes and then introduced a third.
Signed-off-by: cliffhall <cliff@futurescale.com>
Eight findings from the Copilot review, all accepted.
Ownership across connections was the substantive one, in three parts. A
mid-session transport close reaches teardown only through
`clearAndAnnouncePendingPeerRequests`, whose emptiness check looked at the
native queues alone — so a lone app elicitation survived a dropped connection
with its modal still open; the check now counts the active set too. The web
bridge factory resolves its client at call time, so an entry queued by a
replaced `InspectorClient` could rebuild against the *next* one and answer
through a different server; the host now drops every entry synchronously on
the swap (`AppElicitationController.failAll`) rather than racing core's
awaited teardown. And the request id was only monotonic within one client, so
each replacement client's first request was `app-elicitation-1` — settling
that id could resolve the wrong server's request. It now carries a
per-instance prefix.
Result validation now parses the whole value with `ElicitResultSchema` before
the requested-schema check, instead of inspecting `action` by hand: a
`{ action: "decline", content: { x: {} } }` used to pass, though the standard
result permits only primitives and string arrays in `content`.
The dialog's accessible name now includes the request id and the prompt — two
concurrent elicitations can be for the same app URI, and the previous name
made those indistinguishable, which is what the comment beside it claimed to
prevent.
`observeAppCapabilities` takes a minimal structural `onmessage` contract
(generic over the message type, since a handler typed for a narrower message
is not assignable to one typed for `unknown`), removing the double cast at the
call site.
`smoke:web:elicit` now fails on an uncaught page error after a successful
drive, and captures the async half of that class off the console channel, as
its two sibling smokes do — it could otherwise print OK over a broken bundle.
Adds the tests each fix needs, including the two the review asked for
directly: that `advertiseElicitation` reaches the `AppBridge` constructor, and
that it is absent by default.
Signed-off-by: cliffhall <cliff@futurescale.com>
Let a server hand a form `elicitation/create` to an MCP App and return the
app's ordinary `ElicitResult`, with the native elicitation form as the
fallback on every failure.
No second extension, no custom method, and no custom result shape: the only
new wire surface is a nested `elicitation` flag on the existing
`io.modelcontextprotocol/ui` extension on each side, plus
`_meta.ui.resourceUri` on the request. App rendering is selected only when
all four gates hold — client `elicitation.form`, client MCP Apps MIME type,
the nested `elicitation` setting on BOTH peers, and a valid absolute `ui://`
URI on the request.
Only the web client advertises the nested client-side setting, and only
because it has a sandbox renderer: supplying
`InspectorClientOptions.appElicitation` is what opts a client in, so CLI and
TUI keep advertising the MIME type without ever claiming they can resolve an
elicitation through an app.
Routing lives in the one funnel both entry points already use
(`enqueuePendingElicitation`), so the inbound handler and the MRTR driver
cannot diverge. Ownership is request-scoped — a per-request id keys the
renderer, iframe and bridge — so two concurrent elicitations can never
resolve through each other's bridges. An explicit `decline`/`cancel` is a
completed elicitation and goes back to the server; everything else (absent or
malformed metadata, resource/sandbox/bridge failure, an app with no
elicitation capability, a timeout, an invalid result or one that fails the
requested schema) falls back.
The Inspector speaks the ext-apps#733 / SEP-3118 wire protocol but cannot yet
consume its helpers — the released `@modelcontextprotocol/ext-apps` (1.7.5)
predates that PR. `core/mcp/appElicitation.ts` and
`AppRenderer/requestAppElicitation.ts` mirror it exactly and are marked for
deletion once a release containing it ships. One consequence needed its own
seam: ext-apps 1.7.5 parses the view's `ui/initialize` through a schema that
strips `elicitation`, so an app that correctly advertises it would look like
one that did not, silently turning every negotiated elicitation into a
fallback. `AppRenderer/appCapabilities.ts` records the raw frame instead, and
prefers the bridge's own value once it carries the key.
`AppRenderer` now takes an `AppRenderSource` union rather than a `Tool`, so
the same renderer and bridge factory serve an App tool and an elicitation
without either faking the other's shape.
Adds the public fixture (`app_choose_option` + a self-contained
`ui://demo/choose-option.html` app covering accept/decline/cancel), its two
showcase configs, and `smoke:web:elicit`, which drives the negotiated path
end to end in headless Chromium and then the same tool against a server that
never advertised the capability, asserting the native form takes it.
Closes#1854
Signed-off-by: cliffhall <cliff@futurescale.com>
`npm audit --omit=dev` reported one high-severity finding in the
production tree, on both `main` and `v2/main`:
nanoid <3.3.18 — GHSA-2v37-7h3g-55p8
custom generators can loop indefinitely when size is zero
It arrives transitively, via vite -> postcss -> nanoid. `postcss`
depends on `nanoid@^3.3.11`, so the fix is an in-range lockfile bump in
all five installs — no package.json changed. `npm audit --omit=dev` is
now clean at the root and in each of the four client installs.
Applied as a targeted `npm update nanoid` per install rather than
`npm audit fix`. The latter reaches the same clean audit but re-resolves
the whole tree, and here dragged in an unrelated esbuild downgrade
(0.27.7 -> 0.27.2) across web/cli/tui — roughly 700 lines of lockfile
noise for a one-package security patch.
The issue also asked whether `vite` belongs in root `dependencies`,
since that placement is why this advisory appears under `--omit=dev` at
all. It does, and deliberately: `start-vite-dev-server.ts` imports
`vite` and `@vitejs/plugin-react` at runtime for `--web --dev`, and
`tsup.runner.config.ts` lists both as `external`, so a published install
resolves them from the root manifest. Moving them to `devDependencies`
would break `--web --dev` for consumers while passing every local check.
Left in place, with the reasoning recorded in the README's
dependency-placement paragraph so it need not be re-derived.
Verified: `npm audit --omit=dev` reports 0 vulnerabilities in all five
installs; `npm run ci` passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Dependabot security updates always target the default branch, so #1833,
#1840, #1913, #1925 and #2004 all landed on `main`. `main` is never merged
back into `v2/main`, so those fixes had no path to the develop branch and
every install off `v2/main` still resolved the vulnerable versions —
Dependabot will not re-raise them, since it considers them fixed on the
branch it watches.
Brings v2/main to parity: hono 4.12.25 -> 4.13.1, undici 8.5.0 -> 8.9.0,
body-parser 2.2.2 -> 2.3.0, ip-address 10.1.0 -> 10.5.0, fast-uri
3.1.0 -> 3.1.5, plus qs, type-is, express-rate-limit, hasown,
side-channel and es-object-atoms, across the root, web, cli and tui
installs. Two direct declarations move with them: cli undici ^8.9.0 and
web hono ^4.13.1.
The files are taken from the reconciled v2.3.0 milestone-merge tree, so
the root version stays 2.3.0 rather than regressing to main's 2.2.0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
`runWithCommandAuthRecovery` rethrows anything that isn't an
`AuthRecoveryRequiredError`, and 17 of its 18 call sites were bare
`void runWithCommandAuthRecovery(...)` — so any non-auth failure of a
user-initiated list reload (Retry, Load next page, the pagination-mode
toggle) became an unhandled rejection in the browser.
Neither gate that should catch it could: `void` is the sanctioned
suppression for `no-floating-promises`, and `smoke:web:browser` — which
hard-fails on exactly this signal — never connects to a server.
Add `runCommandInBackground`, a fire-and-forget wrapper that owns the
rejection, and route every background command through it, so a new call
site cannot reintroduce the gap by omission. An optional `errorTitle`
picks the reporting: omitted where the list panel already renders the
failure with a Retry (a toast would duplicate it), passed where nothing
else records it — `logging/setLevel` and the tasks refresh.
Signed-off-by: cliffhall <cliff@futurescale.com>
#1954 wired the list-load error off the aggregate managed stores only.
With `paginatedLists` on, those stores deliberately skip their all-page
walk, so their error is permanently null — while the paged stores that
actually drive the sidebar had no error state at all and `loadPage` had
a try/finally with no catch. A failing list showed an empty panel with
no alert and no Retry, and the connect-time `void loadPage(undefined)`
left an unhandled rejection.
- Give PagedTools/Prompts/ResourcesState observable error state: an
`errorChange` event plus `getError()`, set in a new catch in
`loadPage`, cleared on the next success and on disconnect. The
rejection is still re-thrown, matching ManagedListState — callers'
auth-recovery wrappers key off it to detect a 401.
- Catch the connect-time load's rejection (`.catch(() => {})`) now that
the failure is recorded as state rather than lost.
- Select the error by mode in `usePaginatedList`, the same way `items`
already is, and read it in App.tsx from the pagination model.
- Rename `useManagedListError` to `useListError`: both store families
expose an identical `errorChange`, so the hook declares that one-event
contract itself instead of importing ManagedListEventMap.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1. The helper's contract is exactly-once teardown
and the test asserted it must not lean on Hono's own re-entry guard, but
neither the implementation nor the test actually enforced that: the
listener had no local guard and the test aborted only once.
Signed-off-by: cliffhall <cliff@futurescale.com>
Both SSE handlers installed their subscriber, primed the stream so
Firefox's fetch() resolves (#1858), and only then registered onAbort.
Hono's onAbort has no already-aborted replay and abort() fires only the
subscribers registered at that moment, so a client disconnecting while
the priming write was in flight fired nothing: the session event
consumer / server-list subscriber stayed installed, the mcp.json watcher
was never stopped, and the handler parked forever on a promise nothing
would resolve.
Add primeAndHoldSseStream, which registers one listener before the first
await — it both cleans up and releases the hold, collapsing the previous
duplicate onAbort registrations. Priming still happens after the
subscriber is installed, since callers treat the stream's first bytes as
proof they are subscribed.
Closes#1999
Signed-off-by: cliffhall <cliff@futurescale.com>
clients/web emits two bundles — dist (the Vite SPA) and build (the tsup
prod-server runner) — but only dist was in globalIgnores, so `npm run lint`
parsed ~1.2MB of generated output and reported an unused-eslint-disable
warning from inside the vendored undici source. Its three sibling clients
already ignore their build/ output.
Closes#2043
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
A resume snapshot written before this change carries `selectedToolName`
where `ToolsUiState` now expects `selectedToolKey`. The blind cast in
`restoreTabUiFromSnapshot` carried it through as a stray field that would be
re-serialized on the next redirect. Route the Tools entry through a
`normalizeToolsUi` that drops it explicitly instead.
The name is deliberately not migrated to a row key: a snapshot lives for one
redirect (so this only arises when the app is upgraded mid-flight), and the
tools list is fetched after reconnect, long after restore — there is nothing
to resolve the name against. The selection is lost; search, form values, and
the run-as-task toggle restore intact.
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>
#1961 keyed the sidebar's React children by source position, but everything
downstream still identified a tool by `tool.name`. With a `tools/list` that
repeats a name, both copies highlighted at once, the click guard made the
other copy a no-op, and `tools.find((t) => t.name === …)` always resolved to
the first — so a later copy rendered but could never be opened.
Thread the row key through selection instead:
- `utils/toolUtils.ts` now owns `toolRowKey` (moved out of ToolControls) and
adds `findToolByRowKey`, which compares computed keys rather than parsing
one so the two can't drift.
- `ToolControls`: `selectedName` → `selectedKey`; the highlight and the click
guard compare keys, and `onSelectTool` emits the row key.
- `ToolsScreen`: `ToolsUiState.selectedToolName` → `selectedToolKey`, with
both lookups going through `findToolByRowKey`. Renamed through
`screenUiState.ts` and `App.tsx`'s `onToolsUiChange`.
`onCallTool` still sends `selectedTool.name` — the duplicated name is the
genuine wire identity; only the UI identity needed to be unique.
Also prefixes the excluded (SEP-2243) rows' keys: they render into the same
`Stack` as the list rows but index within their own list, so an excluded tool
sharing a name and index with a listed one collided in the key space.
Closes#2001
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>
Server Settings already has a Custom Headers editor, so adding one to the
manual add form created a second place to edit the same field. The reporter
had also conditionally withdrawn #1915 — "editing that information from the
server settings dialog is sufficient" once #1848 closed, which it has.
Reverts the add-form editor, the shared KeyValueRows element, the optional
`settings` argument on addServer / updateServer, and the App-level merge
helper. What remains are two defects found while working in
ServerSettingsForm, which this PR now closes instead (#2040): the Custom
Headers hint misstating OAuth precedence, and the key/value rows' missing
accessible names.
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>
Round-4 review follow-ups on #2038.
The Custom Headers hint claimed a custom `Authorization` value is "ignored"
once OAuth is configured. Verified against the installed
@modelcontextprotocol/client: both SSE and streamable-HTTP `_commonHeaders()`
set the bearer first and then spread `requestInit.headers`, so a custom
value *overrides* the OAuth token. The hint told users the opposite of what
would happen, and could have them silently break their own auth. Corrected in
both places — the modal's new hint and the pre-existing one in
ServerSettingsForm, which carried the same wrong claim.
`utils/serverSettingsPatch.ts` imported both its shared types from
components, reversing the one-way `components -> utils` direction AGENTS.md
sets. `KeyValuePair` moves to `utils/keyValuePairs.ts` (KeyValueRows
re-exports it), and the mode is spelled as a literal union — still checked,
since `ServerConfigModalMode` must stay assignable to it at the call site.
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>
Round-3 review follow-ups on #2038.
`buildHeaderSettingsPatch` treated the mere presence of stored headers as a
change, so saving an id or URL edit re-sent the modal's settings snapshot —
taken when it opened — and could overwrite a metadata or OAuth change made
in the settings form since. It now compares the submitted headers against
the stored ones and omits `settings` when they match, which is what makes
the PUT route preserve the node. Order counts as a difference: it is what
the form round-trips and what the user sees.
Row labels carried the key alone, so two rows sharing a key — mid-edit, or a
duplicate a server persisted — gave both rows' controls identical accessible
names, the thing the labelling exists to prevent. The row number is now
always part of the label rather than a blank-key fallback.
Signed-off-by: cliffhall <cliff@futurescale.com>
