Copilot review round 2 on #2100, plus a collision the review did not find.
The default app-origin port was 6276 — which is already
RUNNER_OAUTH_CALLBACK_DEFAULT_PORT, the CLI/TUI loopback OAuth callback.
That port is fixed precisely so an OAuth app can pre-register
http://127.0.0.1:6276/oauth/callback, which makes it the one listener here
that cannot move when something takes it first. A running --web would hold
it and break a later --cli/--tui OAuth flow at its registered redirect URI,
and docs/mcp-app-review.md has those two running side by side. This
listener has a documented dynamic fallback, so this listener yields: the
default is now 6278 (6277 skipped — v1's retired proxy port, which the
migration guide tells people to stop forwarding). A test asserts the two
defaults stay distinct.
Review comments:
- POST /api/app-document now applies hono's bodyLimit BEFORE parsing. The
8MiB check ran after c.req.json() had buffered and parsed the whole
request, and bounded only `html` — so bulk in `csp`, or in keys the route
never reads, was fully materialized before the 413.
- `csp` is validated as an HTTP header value and length-bounded. The
controller hands it to res.writeHead() verbatim, where Node throws
ERR_INVALID_CHAR synchronously — inside the handler for a later,
unrelated request, outside this route's error handling.
- The app-origin port now propagates everywhere the other two do: the CLI
--print-handoff port-forward command (and its test), the Dockerfile's ENV
and EXPOSE, and the remote-review SSH recipe in docs/mcp-app-review.md.
Without it an app declaring _meta.ui.domain is unreachable through every
documented remote workflow.
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>
An MCP App is rendered by handing its HTML to the sandbox proxy as
`srcdoc`, in a frame sandboxed without `allow-same-origin` — the #1565
isolation model. That gives the app document an opaque origin, so every
request it makes carries `Origin: null` and no CORS / OAuth-callback /
API-key allowlist can admit it. `_meta.ui.domain` is the spec field a
server uses to ask its host for a stable, dedicated origin, and it was
being dropped on the floor.
The field is explicitly host-dependent and the Inspector owns no domain
infrastructure, so it treats any non-empty value as a request rather than
an address, and answers with a real loopback origin on its own listener
(MCP_APP_ORIGIN_PORT, default 6276). One shared origin, path-keyed per
document: that delivers what the field is for without minting a port or
DNS name per app, and is therefore not a per-app isolation boundary.
The inner frame is granted `allow-same-origin` on this path only, which
is what makes the origin real. It is not a #1565 regression: the listener
is on its own port, so the app stays cross-origin to both the proxy and
the Inspector. The proxy refuses the grant if the URL's origin equals its
own, and the grant is never reachable from the server-supplied `sandbox`
string, which is still stripped. The per-app CSP is delivered as a real
response header there rather than only as a <meta>.
Every failure path falls back to the default srcdoc render with a console
warning naming the field, rather than blanking the app.
Also fixes a pre-existing `tsc -b` break on v2/main (useServers.test.tsx
passed `metadata: []` where `RequestMetadata` is expected), which has had
CI red since #2087 and blocks the pre-push gate.
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>
Addresses review round 4 on #2099 — three posted comments and three suppressed
ones, all valid.
- A rebuilt Response reported `url: ""`, `redirected: false`, and
`type: "default"` even after undici followed a redirect: a fetch-contract
regression visible only to proxy users, and one `core/auth/endpointOverrides.ts`
explicitly guards against by declining to rebuild at all. This adapter has no
such choice — the response arrives from the wrong realm — so it now reinstates
the three with `defineProperty`.
- Normalizing a `Request` input dropped every option the Request itself carried
beyond method/headers/body/signal. Most visibly `redirect`: a
`new Request(url, { redirect: "manual" })` started following redirects instead
of surfacing them. All of them are now carried through, with an explicit
`init` still winning per the fetch contract.
Both are covered by tests driving a real redirect through a real proxy, and
both were confirmed to fail without the fix.
- `@modelcontextprotocol/ext-apps` was added only to the CLI's `external` list.
AGENTS.md requires every root-declared runtime dependency in all three, since
which client reaches it is a function of what `core/` imports rather than of
the client's own code. Added to the web runner and TUI lists too.
- The `BUNDLED_CLIENTS` test compared the hard-coded list against a hard-coded
copy of itself, so a new tsup-bundled client would have left it green while
going unchecked. It now discovers clients from disk (a client is tsup-bundled
iff it ships a tsup config) and compares. Verified it fails when a stub client
is added.
- AGENTS.md's project tree still described `core/mcp/node/` as only the stdio
transport factory; it now carries `proxyFetch.ts` and its role.
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>
Addresses the suppressed review comment on #2099, which was the sharpest of the
three rounds: the guard stopped looking at a package the moment its `external`
entry was removed — so deleting `"undici"` from the web config would have made
the pre-fix bundle pass. That is self-referential, and #2067 was precisely a
MISSING declaration, so the guard covered its regression but not the bug.
Candidates are now the union of two independently-derived lists: each client's
`external` array, and the root manifest's `dependencies` — root-declared by
rule, reached through `core/`, and required to be external in every client
bundle. Removing a package from an `external` list can no longer remove it from
scrutiny.
That immediately surfaced one latent instance of the same rule violation:
`@modelcontextprotocol/ext-apps` is root-declared and reached through
`core/mcp/apps.ts`, but was absent from the CLI's `external` list and was being
inlined into `index.js` — dragging the transitive v1 SDK and zod-to-json-schema
in with it. It is ESM, so it was not failing the way `undici` did; what it
violated was the rule. Adding it drops the CLI bundle from 590KB to 518KB.
It is also a real example of the shape the previous revision could not see: it
was inlined into the entry, emitting no `<pkg>-HASH.js` chunk at all.
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>
Addresses review round 2 on #2099.
- The /api/fetch proxy test set only uppercase HTTP_PROXY, so an ambient
lowercase `http_proxy` (which undici's EnvHttpProxyAgent prefers) or a NO_PROXY
could route the request elsewhere and fail the assertion. All six proxy vars
are now saved and cleared for the whole /api/fetch suite and restored in
afterAll — suite-level rather than per-test because the agent is memoized
process-wide once built. Verified the test still passes under a deliberately
hostile ambient env (`http_proxy=http://127.0.0.1:9 NO_PROXY='*'`).
- `verify:bundle-externals` reported success when `parseExternals` returned
nothing, so an ordinary tsup refactor — hoisting the array to a constant, or a
line break after `external:` — would silently disable the guard. It now fails
closed: every bundled client has externals, so "none parsed" means the parser
lost track of the config. Verified against exactly that refactor. The
per-client verdict moved into a pure `evaluateClient()` so all three failure
modes are unit-tested, and the failure epilogue is now printed only for a real
inlining, since "add the package to `external`" is the wrong instruction for
the other two.
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>
Addresses the Copilot review on #2099.
- `/api/fetch` used the bare global `fetch`. That route is the browser's only
way out to the network — the web client's `environment.fetch` is
`createRemoteFetch()`, which forwards OAuth discovery and token requests
there — so a corporate-proxy user could connect to a server but never
authorize against it, and the PR's "OAuth is proxied now" claim held for the
CLI and TUI but not for web. It now uses the same proxy-aware base fetch, with
an integration test driving a real forwarding proxy (confirmed failing without
the fix).
- `verify:bundle-externals` only recognized an inlining that produced a
`<pkg>-HASH.js` chunk. A *statically* imported package is folded into
`index.js` and emits no chunk, so it would have passed. Detection now keys off
esbuild's `// <path>` module banners, covering both shapes. Because that
depends on unminified output, a bundle with no banners now fails as such
rather than reporting clean — enabling `minify` cannot silently retire the
check. Tests cover the entry-inlined case, the same-prefix case
(`undici-types` must not read as `undici`), and the no-banner case.
- The pre-push pipeline summaries in AGENTS.md, README.md, and
.github/copilot-instructions.md still described `verify:build-gate → smoke`
and omitted the new gate.
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>
Corporate-proxy users could not connect to any remote MCP server from v2.
Two independent defects, both reported in #2067.
1. The web and TUI bundles inlined undici. It was declared in the root and
`clients/cli` manifests but named in no client's tsup `external` list, and
tsup auto-externalizes only what the *nearest* manifest declares — so
esbuild inlined 1.05MB of it into `clients/{web,tui}/build/`. undici is
CommonJS, so the emitted chunk's `require("assert")` hit esbuild's ESM
`__require` shim and threw `Dynamic require of "assert" is not supported`.
The rejected `import()` surfaced as "the `undici` package is not available",
telling users to install a package that was already present — and could not
have helped, because the specifier had been rewritten to a relative chunk at
build time.
2. `withProxyDispatcher` injected a *userland* undici `EnvHttpProxyAgent` into
Node's *built-in* fetch. That couples two different copies of undici at the
dispatcher handler interface, which is not stable across majors: Node 22
embeds undici 6, Node 24 embeds 7, Node 26 embeds 8. A userland undici 8
agent handed a built-in undici 7 handler is rejected with
`fetch failed: invalid onRequestStart method`, and the request never leaves
the process. So fixing the bundling alone would only have converted the
web/TUI failure into the CLI's.
Both halves of the pair now come from userland undici — its `fetch` and its
`EnvHttpProxyAgent` — which keeps the handler inside one copy and works
unchanged from the Node 22.19 engine floor through Node 26. Deleting the
wrapper in favor of Node's native `NODE_USE_ENV_PROXY` was not an option:
measured, it is unsupported on 22.19 (the floor) and only lands mid-22.x.
Because undici's `Response` is a different class from `globalThis.Response`,
the proxied fetch re-wraps each response as a genuine global `Response`,
streaming preserved. Without that, `res instanceof Response` is false for
callers that test it — including the SDK's OAuth error formatter, which would
degrade every message to `Raw body: [object Response]`.
The proxy now sits at the bottom of the fetch stack (`environment.fetch` for
the Node clients, and the `createTransportNode` default for the web backend,
its one caller that supplies no `fetchFn`) rather than wrapping the transport's
fetch. Proxying means *substituting* the fetch, not decorating one, so wrapping
a supplied `fetchFn` would discard it; installing it underneath lets
InspectorClient's own wrappers compose over it. A side effect worth having:
OAuth discovery and token requests go through `environment.fetch` too, and were
never proxied before.
Guarding it:
- `undici` is added to all three tsup `external` lists and removed from
`clients/cli/package.json`, so it is root-declared only, per the placement
rule in AGENTS.md. The two installs had already drifted (root 8.5.0 vs cli
8.9.0).
- New `npm run verify:bundle-externals` fails if any package a client declares
external was inlined into that client's bundle anyway. It reads the *built
output*, not the config — those two disagreed for four releases — and derives
what to check from each client's own `external` array, so a newly
externalized package is covered without editing the guard. Verified against
the real pre-fix artifact, which it flags.
- The proxy tests now drive a real forwarding proxy rather than a mock. That
matters: defect 2 was a runtime interface mismatch, which no mock can catch
because no mock ever dispatches.
AGENTS.md's must-not-bundle rule (added by #2082) is extended with why undici
slipped past it, and notes the `node -e` trap — probing the bundled chunk with
`node -e` falsely succeeds, because it exposes a global `require` that
satisfies esbuild's guard.
Closes#2067
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>
Two review findings from round 2, both correct.
1. The regression suite killed its child only after the assertions. If
`assert.rejects` or either assertion failed, execution never reached the
kill, leaving a 60-second child attached to the runner — a suite guarding a
process leak would leak one itself, precisely on the run where the
regression had come back. Teardown is now registered from `onSpawn` via
`t.after`, so it runs on every path out of every case. That is the helper's
own ownership rule applied to the tests; killing an already-exited child is
a no-op, so the early-exit cases pay nothing for it.
2. Document the new shared helper, per the maintenance rules. AGENTS.md's
scripts/lib tree entry now carries announced-child.mjs and what its contract
buys, and the smoke:web:app paragraph records why the spawn/readiness step
moved out of the smoke (same reason teardownWebServer left prod-web-server's
stop()). README's test:scripts row names the new suite alongside
resolve-node-bin's.
Not mirrored into .github/copilot-instructions.md: that file is a distillation
of the rules a reviewer cites against a diff, and deliberately excludes the
project-structure tree and tooling inventory.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 5. Both comments upheld; the first is a real hole in
round 4's own fix.
**The ownership guard was in the wrong place, and overclaimed.**
Round 4 checked ownership and then called `release()`. Two problems:
- Check-then-act. The `stat` and the `rmdir` were separate async steps, so a
waiter could replace the directory in between and we deleted the winner's
fresh lock anyway — the very thing the check was added to prevent.
- Skipping `release()` on a mismatch left proper-lockfile's record registered
in its `locks` map, and its `signal-exit` handler `rmdirSync`s every
registered lock with **no ownership check of its own**. An exit during that
window deleted the winner too.
The guard now lives in `options.fs`, which is the single seam *both* removal
paths route through — `removeLock` on release, and the exit handler. It
refuses to remove a directory whose inode and birth time are not the ones
recorded at acquire, and reports the refusal as success so the library's
bookkeeping forgets the lock either way (leaving it registered is what hands
the exit handler a record pointing at the winner's directory).
The check is a `statSync` immediately followed by an `rmdirSync`, with no
`await` between them, so nothing in this process can interleave. **It still
does not close the race** — it is check-then-act across processes, which needs
the compare-and-swap Node does not expose — and saying it "closes the
destructive half outright" was wrong. file-lock.ts, README.md,
specification/v2_servers_file.md and AGENTS.md now all say it narrows the
window rather than closing it. AGENTS.md carries the negative instruction
alongside round 4's, since this is the third round spent on an overclaim.
**"It expires on its own after 10s" is false for the ENOTEMPTY case** the new
test exercises. Stale takeover reclaims through the same `rmdir`, which also
cannot remove a non-empty directory — so nothing clears it, and every later
save fails ELOCKED against it. `releaseAdvice` now branches: by-hand removal
for ENOTEMPTY, expiry for everything else.
Tests: a real child process demonstrates the exit handler removing a
replacement lock (with an **empty** directory — a non-empty one makes
`rmdirSync` fail ENOTEMPTY, which is how the first draft of this test passed
for the wrong reason). The release-failure test now asserts the message says
"by hand" and does not promise expiry. The shim's `rmdirSync` arm carries a
justified `v8 ignore`: it is reachable only from a real process exit, and its
logic is `removeIfMine`, covered through the async arm.
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>
Copilot review round 4 — four comments, all the same finding against the four
files carrying the claim, and it was more than a wording problem.
The comment said a holder whose lock was replaced is told. Checked against
proper-lockfile@4.1.2, that is false in the case that actually happens:
- `updateLock` compares mtime only on its refresh tick — `stale / 2`, so 5s
here — while an ordinary mutation is a read, an scrypt derivation and an
atomic write, comfortably under a second. The tick never runs, so nothing
fires.
- `release` → `unlock` → `removeLock` is an unconditional `rmdir` with **no
ownership check**. So the compromised holder goes on to delete the
*winner's* lock on the way out, ending the winner's exclusion too: one
compromised writer silently becomes two unprotected ones.
The second half is a correctness bug, not a documentation defect, and it is
fixable here even though the takeover race itself is not.
`withSecretFileLock` now records the lock directory's inode and birth time at
acquire and re-checks them before releasing. On a mismatch it **declines to
release** — leaving the new holder's lock alone costs nothing, ours is
already gone — and warns.
Inode and birth time rather than mtime, deliberately: both survive the
library's own `utimes` refresh, so a lock held past one tick is not accused of
being compromised, and both change on a delete-and-recreate, which is exactly
the event to detect. Where a filesystem reports neither, the two reads agree
and the check concludes "ours", degrading to the library's unaided behaviour
rather than to a false alarm. An unreadable baseline at acquire likewise
answers "ours" — with nothing to compare against, accusing a healthy lock
would be worse than releasing it.
Detection is therefore **best-effort**, and file-lock.ts, README.md,
specification/v2_servers_file.md and AGENTS.md now all say so rather than
promising the loser is told. AGENTS.md carries the negative instruction too,
since this is the second round spent on the same overclaim.
Tests: "does not delete the winner's lock after being taken over" replaces the
lock inside the body and asserts the replacement survives with the *same*
inode and birth time — it was left alone, not deleted and recreated. It waits
for nothing, being the fast case the library's tick misses. Verified by
mutation: forcing `stillOurs` to always answer true fails it. A second test
covers a release that fails while the lock genuinely *is* ours (a stray file
makes `rmdir` fail ENOTEMPTY), so the ownership check cannot swallow a real
release failure.
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>
`lint` failed only on errors, so a warn-level rule was a channel nothing
consumed. `react-hooks/exhaustive-deps` reported two stale-closure
`useCallback` deps in App.tsx on every run, `npm run ci` passed anyway,
and the defect was caught only by a review round on #2076.
Add `--max-warnings 0` to all six lint scopes — each of
clients/{web,cli,tui,launcher}'s `eslint .`, plus the root's `lint:core`
and `lint:shared` — so a warning fails `lint`, and therefore `validate`
and `ci`. All six were already warning-free, so no suppressions, rule
downgrades, or ignore-list widening were needed.
Also promote `react-hooks/exhaustive-deps` to `error` in both React
scopes (web, tui). The flag makes severity irrelevant to the gate; it
does not make it read correctly in an editor.
Document the invariant in AGENTS.md as a new "Lint has no warning tier"
section, note it in the README beside the type-aware-linting paragraph,
and mirror it into .github/copilot-instructions.md.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2, both comments upheld.
**ELOCKED must not degrade.** Exhausting the retries on a held lock means the
lock is *working* and something else demonstrably holds it — so running the
mutation anyway entered the exact interleaving the lock exists to prevent,
and entered it knowing a concurrent writer was there. `set` now fails
instead. Two supporting changes:
- The retry budget went from ~3.3s to ~15s so it outlasts the 10s stale
window. Refusing is only defensible because a *crashed* holder resolves by
takeover first; with the shorter budget, one Inspector killed mid-save
would have failed every later save on the box until someone deleted the
lock by hand. Pinned by a new test that stages a dead holder's lock
directly (mkdir + backdated mtime) and asserts the save succeeds.
- Acquire is now probe-then-wait. `proper-lockfile` drives its whole acquire
through `retry`, which re-attempts on *any* error — so with a 20-retry
budget a read-only `$HOME` would have spent the full 15s re-issuing an
identical failing `mkdir` on every save before degrading. One retry-less
probe separates "held" (worth waiting out) from "unavailable" (not).
**The comment overclaimed what proper-lockfile provides, and the reviewer was
right about the mechanism.** Verified against lib/lockfile.js@4.1.2: on
EEXIST it stats, and if stale it rmdirs and re-mkdirs *without* checking the
directory it removed is the one it found stale — so a slow waiter can still
delete a fast waiter's fresh lock and both proceed. That is the same race the
hand-rolled version could not close, and it is not closable without
renameat2(RENAME_EXCHANGE).
What the library actually adds is that the loser is *detected*: its refresh
tick compares the lock's mtime against the value recorded at acquire, so a
compromised holder is told rather than proceeding silently. And what is
genuinely exclusive is the case that matters — mkdir is atomic and a live
holder refreshes its mtime, so its lock never goes stale and two running
Inspectors are serialized. The residual window opens only after a holder dies
without releasing, and the optimistic verify still covers it.
file-lock.ts, README.md, specification/v2_servers_file.md and AGENTS.md all
carried the overclaim; all four now state the narrower, true version.
Also fixes a test that was quietly asserting the opposite of its name: its
holder used stale: 60_000 while the waiter used 10_000, and staleness is
judged by the *waiter's* threshold — so the holder was declared stale after
10s and the save succeeded by takeover.
Not done: a two-process stale-takeover race test. It would be testing the
library rather than this code, and is inherently nondeterministic; the honest
response to that finding was to stop claiming the race is closed.
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>
#1950 shipped FileSecretStore without cross-process mutual exclusion, by
decision: an earlier revision hand-rolled a `mkdir` election with an owner
stamp, a heartbeat and a stale-takeover, and three consecutive review rounds
found a real race in it. The last one is not closable with what Node exposes
— claiming a stale lock atomically needs compare-and-swap on a directory
entry (`renameat2(RENAME_EXCHANGE)`) — so it was replaced with optimistic
verify-and-retry and the residual documented.
#2082 settles that as "borrow, don't hand-roll". `proper-lockfile` is what
npm itself locks with, and stale-takeover is precisely the problem it has
already solved: it re-stats the lock directory after claiming it and gives
the lock up when the mtime is not the one it wrote, so the loser of a
takeover race releases instead of proceeding.
- core/auth/node/file-lock.ts: `withSecretFileLock`. `realpath: false` so a
file can be locked into existence (the very first `set` has no
`secrets.json`, and the library's default resolves through `fs.realpath`);
a warning in place of the library's `onCompromised`, which throws from a
timer and would take the session down; and a degrade-with-one-warning path
rather than a throw when no lock can be taken — this store exists for boxes
missing the usual mechanism (#1848, #1905) and must not gain a new way to
be unavailable.
- FileSecretStore.mutate holds it across the whole read-modify-write. The
optimistic verify stays underneath and is not redundant: a lock is advisory
between the processes that take it, so the verify covers a writer outside
this codebase and covers the degrade path. The in-process queue stays too,
and gains a second job — proper-lockfile is not reentrant, so serializing
per path keeps ELOCKED meaning "another process".
- absorbFileSecretsIntoKeyring takes the same lock around orphan adoption and
the atomic claim, behind a lock-free `readdir` fast path so the common
startup (keychain available, no file ever written) neither creates a lock
directory nor warns about one it could not create.
proper-lockfile is a root `dependency` per the placement rule, and is named
in all three bundler `external` lists: tsup externalizes what the *client's*
manifest declares, so a root-only CJS package was being inlined into the ESM
bundles, leaving esbuild's `Dynamic require of "path" is not supported` shim
that killed `--cli` at import time. That rule was undocumented; it is now in
AGENTS.md beside the placement rule that creates it, and mirrored into
.github/copilot-instructions.md.
Tests: 7 new in file-lock.test.ts driving a real second process (the existing
suite structurally cannot — `serialize` orders in-process callers before the
lock sees them), plus two for the migration fast path.
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>
The command reference still ended `npm run smoke` at `smoke:web:app` and
credited the shared prod-web-server helper with three consumers. Adds the new
smoke to both, plus its own entry alongside the sibling smokes: what it drives,
why the not-negotiated half is the load-bearing one, and the two mechanics that
are easy to get wrong (the tabs are a SegmentedControl with no `role="tab"`,
and the prompt string also appears in the hidden Protocol payload).
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>
Closes#1950
Before this, a box with no reachable OS keychain — the published
container (no D-Bus session, #1848), Android/Termux (no platform binary,
#1905), a minimal Linux install without libsecret — could not persist an
OAuth client secret or a stdio `env:` value at all. `KeyringSecretStore`
degraded into a store that silently holds nothing: `get` returned null,
`delete` no-opped, and `set` hard-failed with a 503. That is the correct
failure behavior, but it left the documented, supported way to run the
Inspector with no secret persistence whatsoever.
Adds the two missing implementations and, more importantly, the policy
that picks one and the surfaces that say which was picked.
- `FileSecretStore` — one `0600` JSON document at
`~/.mcp-inspector/secrets.json`, encrypted with AES-256-GCM whenever
`MCP_INSPECTOR_SECRET_KEY` is set (scrypt, per-file random salt). The
whole map is encrypted as a unit rather than value-by-value so the
account names — `${serverId}:${field}` — are hidden too; a per-value
scheme would leave a readable index of which servers you hold a client
secret for. A file that can no longer be decrypted reads as empty
(matching the interface's read tolerance) but *refuses to be written*,
because replacing a file of still-valid secrets to satisfy an additive
request destroys data.
- `secret-store-selection.ts` — the policy.
`MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright;
otherwise probe the keychain and, if unreachable, fall back loudly. The
fallback is `memory` in a container whose secrets directory is not on a
mount, and `file` otherwise — so mounting the volume the README already
recommends for the catalog flips the same run to durable storage with
no configuration, and an unmounted container gets the honest answer
rather than a file `--rm` will discard. Cached per process so the
banner, `/api/config`, and the store doing the writing cannot disagree.
- Automatic, but never silent. The downgrade prints at startup, rides
`GET /api/config`, and is named in a permanent 32px footer at the
bottom of both dialogs that accept a secret. A startup banner is seen
once by whoever started the process; a toast is seen once; a
dismissible banner is the thing a user dismisses before doing the work
it describes. The plaintext and memory cases render their caveat inline
in a warning tone — a warning you must hover to discover is not one.
- `SecretStoreUnavailableError` is now the base every store's `set`
throws, so the routes translate one condition into a 503 instead of
matching `KeychainUnavailableError` by name.
Docs: the container section of the root README gains a table of which
store you get and how to make secrets survive, plus the
`MCP_INSPECTOR_SECRET_KEY` upgrade/mismatch rules.
Tests: the existing interface contract block is extracted to
`secretStoreContract.ts` and run against all three implementations, so
none can drift on the availability contract. `FileSecretStore` is tested
against a real temp filesystem — mode, cross-instance round-trip, that
the ciphertext does not contain the plaintext, the read-modify-write
race, and every refusal path. Full `npm run ci` passes, per-file gate
included.
Signed-off-by: cliffhall <cliff@futurescale.com>
verify:dep-lockstep is a durable guard but not a coverage guard — the two
coverage guards are verify:format-coverage and verify:typecheck-coverage,
which is the terminology the rest of the repo uses. The sentence said three
and then named two.
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds an AGENTS.md rule under the pre-push gate naming the gated surface
(first-party source) and everything excluded from it (each client's build/,
web's dist/ and storybook-static/, coverage/, test-servers/build,
core/**/{build,dist}, *.tsbuildinfo), why gating generated output is harmful
even when it only warns, and the fact that the three coverage guards assert
the converse and so cannot catch this drift. Mirrored into
.github/copilot-instructions.md per the maintenance rules.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3, all three real:
- `require.resolve.paths()` also appends Node's GLOBAL FOLDERS
($HOME/.node_modules, $HOME/.node_libraries, $PREFIX/lib/node) and any
NODE_PATH entries, so round 2's lookup could satisfy a missing repo
install from a GLOBALLY installed typescript/vite — measuring the
programs with the wrong compiler instead of producing the actionable
"run npm install" this helper promises. Filter to the node_modules
directories on fromDir's own ancestor chain.
- The quoting predicate missed characters cmd.exe still parses, notably
parentheses (`C:\Temp(1)`), and quoting cannot suppress `%VAR%` at all
— expansion happens before quotes are processed. Extracted to
lib/win-shell-args.mjs with a broad predicate and a hard error on `%`
rather than a silently different path. `platform` is a parameter so the
Windows behavior is exercised on the POSIX machines that run the suite.
- AGENTS.md claimed pack-and-verify spawns via `process.execPath`; it does
not, and cannot — its children are `npm` and the installed `.bin` shim.
State the exception and why.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01387r3MotGk52hrkfjzHFsN
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2 (suppressed comments), all three real:
- `require.resolve("<pkg>/package.json")` is governed by the package's
`exports`, and Node keeps no special case for `./package.json` — so a
package declaring an `exports` map without it threw
ERR_PACKAGE_PATH_NOT_EXPORTED even though its bin was installed and
spawnable, a false "not installed" pointing at `npm install` for a
package already on disk. Walk `resolve.paths()` instead, which never
consults `exports`. The docblock claiming the opposite is corrected.
- `shell: true` (needed at all on Windows to start the `.cmd` shims) makes
Node hand `cmd.exe` one space-joined string, so an argument holding a
path with a space splits in two. Every generated path in
pack-and-verify lives under `tmpdir()`, which sits beneath the user
profile. Quote them via `shellArgs()` at all four spawn sites, the
command path included — the hazard predates this PR, so fix the class.
- Document the new module: `scripts/lib/resolve-node-bin.mjs` in AGENTS.md's
scripts/ tree, its tests in the README's `test:scripts` row.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01387r3MotGk52hrkfjzHFsN
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>
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 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>
Addresses Copilot's round-5 review on #2035 — eight suppressed comments,
all of them real.
The consequential one overturns behavior I copied from the SDK and
defended in two earlier rounds. RFC 6570 expands each expression
independently; it carries no cross-expression state. The SDK tracks
whether a query expression has already emitted and rewrites a later
`{?two}`'s `?` to `&`. That looks friendlier and is wrong, and its own
matcher says so: for `x{?one}{?two}` it expands to `x?one=1&two=2`, and
`match("x?one=1&two=2")` on that same template returns **null**, while
`match("x?one=1?two=2")` returns both variables. So the rewrite emitted a
URI the advertised template cannot match — #1919 one level up. A server
that wants a continuation advertises `{?one}{&two}`, which expands to `&`
and matches; producing that shape is the server's choice to declare, not
ours to infer.
Also:
- `{id:abc}` was silently treated as `{id}`, and a test codified it. RFC
6570's max-length is `%x31-39 0*3DIGIT`, so `{id:}`, `{id:0}`,
`{id:abc}` and `{id:10000}` are invalid templates rather than templates
with an ignorable modifier — and the SDK's constructor accepts them all,
so nothing else rejects them. Strict expansion now throws; lenient
returns the raw template. Discovery stays lenient so the panel still
renders rather than going blank.
- The TUI could submit `{a,b}` completely blank while the web panel
blocked the same request: ink-form has no way to express "any one of
these", so its members are left optional and the modal needs its own
group check. Added, with tests — it was also below the coverage gate
without them (functions 76.92%).
- Added the integration test the config never had: it resolves the
checked-in config and drives all four reads over a real transport, so a
misspelt preset fails there rather than only when someone runs the repro
by hand.
- "All three clients" was false. The CLI has no template form and its
`resources/read` passes the already-expanded `--uri` straight to
`readResource`, so nothing here runs for it. Corrected in the README,
AGENTS.md, and the module comment, and dropped a docblock still
describing the SDK-delegation path removed last round.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-4 review on #2035 — again reported as "no new
comments" while carrying three suppressed ones. All three reproduced.
1. The TUI form still named its fields from the SDK. `uriTemplateToForm`
read `UriTemplate.variableNames`, which mangles a name: `{;id}` yields
";id" and `{id:3}` yields "id:3". The form therefore submitted
`{ ";id": "7" }` while the shared expander looked up `id`, found
nothing, and dropped the expression — the value vanished silently.
This is the finding that mattered most, because it falsified the claim
this change is built on. Moving the expander into core only makes the
clients agree if each client's FORM derives its names from the same
parser: a form submits under the names it rendered. The TUI now reads
`templateVariables`, and the SDK template is constructed only to
validate, preserving the existing malformed-template diagnostic.
Its `required` flag comes from `requiredGroups`, and only a variable
that is the sole member of a non-omittable expression is marked: ink-
form cannot express "any one of these", so marking every member of
`{a,b}` required would refuse input the expander accepts.
2. `encodeURIComponent` is not RFC 3986's unreserved set — it leaves the
sub-delims !'()* bare, which RFC 6570 requires encoded for every
operator except + and #.
Fixing that settled a design question left open in the previous round.
With two encoders, the SAME value encoded differently depending on
whether its expression happened to carry a modifier, since only the
modifier pushed it onto our path. So delegation is gone entirely: one
expander, one set of rules. The SDK's `UriTemplate` is still
constructed, but only to validate a template.
3. The "Any one of: a, b" hint could contradict the disabled submit
button. A name can sit in a singleton required group AND a shared one
(`x://{a}/{a,b}`), where the singleton demands that exact field. The
hint is suppressed in that case.
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
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>
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>
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>
Real deployments gate IdP selection, prompt behavior, or audience on a query
parameter the OAuth/OIDC core specs never standardized (Keycloak's
`kc_idp_hint`, OIDC's `login_hint` / `prompt` / `acr_values`, Auth0's
`audience`), and none of them could be driven from the Inspector.
The Inspector never builds the authorize URL — the SDK's `auth()` does, and it
exposes no hook for extra parameters — so the merge happens at the one seam
that sees the finished URL: `BaseOAuthClientProvider.redirectToAuthorization`.
It lives in `core/`, so web, CLI, and TUI all inherit it.
- `core/auth/authorizationParams.ts`: the reserved-key list (`client_id`,
`code_challenge`, `code_challenge_method`, `redirect_uri`, `resource`,
`response_type`, `scope`, `state`), the key validator the form renders
inline, and the merge itself. Reserved keys are dropped with a warning
rather than silently overriding PKCE / the state binding / RFC 8707;
blank-key rows are skipped; the caller's URL is never mutated.
- Applied to the authorization request only — nothing wires it into the token
request.
- Persisted as `oauth.authorizationParams` (a string record) on the per-server
config beside `scopes`, edited as key/value rows in Server Settings ->
Authorization ("Additional authorization parameters"), with an inline error
per offending row plus a section-level warning.
- The enterprise-managed (EMA) leg redirects to a *different* authorization
server (the enterprise IdP), so it goes through the explicit
`redirectToExternalAuthorization` entry point, which skips the merge — a
`kc_idp_hint` or `audience` meant for the MCP server's AS must never be
appended to the IdP's authorize URL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#2015, folded into #1928 rather than left as a follow-up.
The TUI's `schemaToForm` has the same defect the web `SchemaForm` had: its
local `JsonSchemaProperty` declares only `type?: string` — no `anyOf`, no
array-valued `type` — so a `.nullish()` argument has no top-level `type` or
`enum` for the dispatcher to read and falls to the `switch` default, losing
its select. Milder than the web bug (a plain text field, not a corrupting
JSON textarea), but the same class.
Fixing it in a second place is the argument for not having the logic in a
first place. So the collapse moves to `core/json/nullableUnion.ts` as
`normalizeNullableUnion`, generic over a minimal `NullableUnionSchema`
shape — the least a caller must expose to be *recognized* as a nullable
union — so neither client has to adopt the other's schema type. Both form
builders now share one copy and cannot drift on which schemas they render.
- `core/json/nullableUnion.ts` — the shared collapse, plus tests at
`clients/web/src/test/core/nullableUnion.test.ts` (the web suite is what
gates `core/`).
- `clients/web`: `schemaUtils.normalizeUnionType` is gone; `SchemaForm` and
`resolveRefsInMessage` call the core function directly.
- `clients/tui`: `schemaToForm` normalizes each property before dispatch,
and `JsonSchemaProperty` grows the `anyOf` / array-`type` fields the
encoding uses.
Verified by driving the built TUI under a pty against the
`nullable-fields-http.json` showcase server. Before, every argument of
`record_shipment` rendered as a plain text field; after, `direction` is a
select ("No value"), `quantity` an integer, `express` a boolean
("[Not set]").
Signed-off-by: cliffhall <cliff@futurescale.com>
The SDK validates a list result as a whole, so a single non-conforming entry
rejected the entire response and the panel showed "Couldn't load resources"
with zero items — the exact opposite of what a debugging tool should do with a
server that is 90% right. Reported against a PHP server whose empty
`annotations` object reaches the wire as `[]` rather than `{}`.
`listAll*` now falls back to a lenient re-walk when — and only when — the SDK
rejects a result it received (`InvalidResult` / `UnsupportedResultType`), keeps
the entries that validate, and reports the rest. A conforming server takes the
strict, cache-aware path exactly as before.
Salvaging is not accepting: the dropped entries are named with their reason in
a warning above the list, and the Protocol entry is still marked rejected, so
the wire truth survives. When every entry validates, the original error is
rethrown rather than swallowed — the rejection was about something the
per-item pass cannot explain.
`isClientDecodeRejection` moves to the new module and is now shared with
managedListState, which had the only copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review:
- `<x.y.z>` / `<major|minor|patch>` inside a bash block is shell redirection,
so both documented commands were syntax errors if copied. Verified: `bash -n`
rejects `git tag <x.y.z> && ...` and accepts `git tag 2.3.0 && ...`. Use a
concrete 2.2.0 -> 2.3.0 example and say what to substitute.
- "agree on the version at every point" / "never disagree" overstated it. The
procedure deliberately bumps v2/main first, so between the bump and the
milestone merge the branches differ — v2/main reads the version being built,
main the one released. Describe the real invariant: they converge when the
release merge lands, and v2/main is never left behind main.
Signed-off-by: cliffhall <cliff@futurescale.com>
v2/main has been at 2.0.0 since the v2.0.0 release while main and npm `latest`
are at 2.2.0. The bump was created on the milestone-merge branch, which is cut
from main, so it only ever existed downstream of v2/main and nothing carried it
back — `chore: 2.1.0` and `chore: 2.2.0` are both on main and neither is on
v2/main.
That is not cosmetic. A branch cut from a milestone-merge branch silently
carries the bump into an unrelated PR — #2009 arrived with a 2.0.0 -> 2.2.0 diff
on a container bugfix — and anything reading the version in development
(readInspectorVersion, --version, GET /api/config) reported a version two
releases old.
Move the bump to v2/main, before the milestone merge, so it flows into main with
the rest of the milestone's work and the branches never disagree. The release
tag is applied to the resulting main commit instead. The publish workflow needs
no change: its tag-vs-package.json assertion runs at the release commit on main,
which still carries the bump.
Deliberately not a back-merge of main into v2/main: main holds the whole pre-v2
v1 history (~230 commits v2/main lacks, retained through `chore: replace main's
tree with v2`), so merging would graft all of it into the develop branch's log
permanently to deliver a two-file change.
Also sets v2/main to 2.2.0 as a one-time catch-up. Nothing in the repo asserts a
specific version value, so this is a safe two-file change.
Closes#2010
Signed-off-by: cliffhall <cliff@futurescale.com>
Two defaults that each break the Inspector inside a container.
#1951 — the web UI never loads in a VS Code dev container. `HOST` defaulted
to the NAME `localhost`, and `server.listen(port, host)` resolves a name
through `dns.lookup` and binds the SINGLE address it gets back. Since Node 17
the default result order is `verbatim`, so on a glibc Linux host — whose
/etc/hosts maps localhost to both 127.0.0.1 and ::1 — that is `::1`. The
server bound IPv6 loopback only and refused every IPv4 client, including the
dev container port forwarder. It presents as an endless spinner rather than a
connection error because the forwarder accepts on the host side before failing
to connect inward. Default to the ADDRESS 127.0.0.1 instead; an explicit
HOST=localhost is still honored as typed.
#2008 — the Apps tab is unreachable in any container. The MCP Apps sandbox
listened on an OS-assigned port, so it changed every run and could never be
named in a devcontainer `forwardPorts`, a `docker run -p`, or a tunnel. Default
it to a fixed 6275, mirroring CLIENT_PORT's fixed 6274, and retry once on an
OS-assigned port (loudly) if it is taken — the collision a dynamic default
never had.
Verified in the reporter's exact image
(mcr.microsoft.com/devcontainers/typescript-node:4-24-trixie): both listeners
come up on 127.0.0.1 and answer 200 on 6274 and 6275/sandbox.
Also EXPOSE 6275 and set MCP_SANDBOX_PORT in the image, and document both
defaults in the web README, the migration guide, and the root README's Docker
section.
Signed-off-by: cliffhall <cliff@futurescale.com>
Review caught a second-order effect of the fix: making the leaked-timer net
global invalidated the rationale of a neighbouring rule.
"Do not hand-roll a bare MantineProvider — that reintroduces the leak class"
stopped being true the moment the net moved into setup.ts, which covers every
unit test regardless of how it renders. The rule itself is still right, so keep
it and replace the justification: a hand-rolled provider skips the project theme
and the helper's options and drifts from every other test. Both copies say
explicitly that the old reason no longer holds, so nobody re-derives it.
Mirrored into .github/copilot-instructions.md in the same PR, as AGENTS.md
requires for review-relevant guidance — it carried the identical stale claim.
That makes four places this one belief had propagated to. A fix can create stale
docs by making a previously-true statement false, which is easy to miss when
only the changed lines get read.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
