Copilot review round 5 on #2114 — a hole in last round's fix.
When the persisted scope was `offline_access` alone,
`scopeForDeclinedRefreshGrant` returned undefined.
`OAuthManager.createOAuthProvider` reads that as "nothing stored" and
seeds storage with the configured scope, so the request-only filter
silently overwrote the value it promised to leave alone — and
re-enabling the grant could no longer restore it.
It now falls back to the configured scope, which is both the right
thing to request and inert to that branch. The previous "storage is
never rewritten" test could not see this: it ran at the provider level,
while the seeding branch lives in the manager. The new regression runs
there, and fails when the fallback is removed.
Also corrects the EMA rationale, which was wrong even though its
conclusion held. `createOAuthProviderForTransport` does build a
`BaseOAuthClientProvider` and wrap it, and `EmaTransportOAuthProvider`
forwards `clientMetadata` — so the setting is not bypassed outright.
What it cannot change is the IdP authorization leg's fixed
`openid offline_access`, which is the leg the user signs in through and
therefore the reason the control cannot affect EMA's consent behavior.
Fixed in the docs, the code comment, the UI string and the test.
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 4 on #2114 — a real gap, not a wording one.
Declining the grant stops the SDK *adding* `offline_access`, but the
Inspector's own storage puts it back. A successful default-on
authorization persists the scope the AS granted
(`resolvePersistedScopeAfterGrant`), `offline_access` included;
`prepareForAuth` reloads it and `authenticate` hands it to the SDK as
`scope`, which sees the token in scope and appends `prompt=consent`
again. So any server that had authorized once could uncheck the box and
still hit AADSTS90094 — the users most likely to reach for the setting
were the ones it did not help.
`scopeForDeclinedRefreshGrant` filters it out of the *request* only,
applied in the provider's `scope` getter — the one seam both
`clientMetadata.scope` and `authenticate`'s `scope:` argument read
through. An `offline_access` the user configured is honored; storage is
never rewritten, so re-checking the box restores the old behavior
instead of finding the value destroyed, and scope-satisfaction checks
still see the real grant. Verified by reverting the filter: the new e2e
regression fails.
Also from that round, both previously suppressed:
The checkbox looked effective under enterprise-managed authorization,
which never builds this provider at all — it authorizes against the
enterprise IdP with a fixed `openid offline_access`
(`ema/constants.ts:16`). It now annotates itself the way the endpoint
overrides do. The existing count assertion covered two such notices and
was named for the overrides alone; it is renamed and expects three, with
a dedicated case pinning the notice to this control so the count cannot
be met by three copies elsewhere.
And "obtains no new refresh token" was another overstatement — the AS
decides issuance, and a preconfigured client is reused with whatever its
registration already permits. Replaced with what the setting actually
controls.
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 2 on #2114. The setting was described as
guaranteeing "no offline_access, no forced prompt", which is not what
the SDK does.
`startAuthorization` appends `prompt=consent` for any `offline_access`
in the effective scope and never consults `grant_types`, and
`determineScope` skips its augmentation when the scope already carries
it. So the prompt still goes out when the user configured
`offline_access` in Scopes, or left Scopes blank and the resource
advertises it in `scopes_supported`. The flag removes the SDK's
*automatic* augmentation and nothing more; the docs, the help text and
the type say that now.
The form also flags the case a user can act on: opted out while
`offline_access` is still listed in Scopes. Whole-token match, so
`offline_access_extra` does not trip it.
Corrects the Clear stored OAuth state claim too. It clears the local
tokens and client information — it is not a revocation and cannot
reach the AS. A dynamically registered client re-registers on the next
connect (the new registration declaring only `authorization_code`); a
preconfigured `oauth.clientId` is reused unchanged.
Adds the two missing seam tests, both confirmed to fail without the
code they cover: App's connect wiring (deleting the spread fails two of
the three) and an /api/servers POST followed by a read-back, since a
200 alone never proved the opt-out persisted.
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 on #2114. The setting was described as taking effect
"immediately", which is wrong twice over.
`applyLiveServerSettings` treats OAuth as connection-time-only, so a
connected client keeps the metadata it was built with — deliberately,
and identically to every other field in that section (client id,
secret, scopes, authorizationParams, endpoint overrides). Not wiring
this one field live; saying "next connect" instead, in the docs, the
checkbox description, and the type.
And two pieces of state outlive the setting: the registration already
held at the authorization server still lists the grant, and a refresh
token issued earlier stays usable, because the SDK's
`refreshAuthorization()` posts the stored token without consulting
`grant_types`. So the stated cost is not guaranteed until stored OAuth
state is cleared. Documented, rather than revoking credentials as a
side effect of a checkbox — Clear stored OAuth state already does that
explicitly.
Also adds the manager->provider seam test the other bridges have:
without it, deleting `requestRefreshToken:` from `createOAuthProvider`
would leave the feature dead with every other test green.
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>
The Inspector always declared `refresh_token` in its OAuth client
metadata. The SDK's `determineScope()` then appends `offline_access`
whenever the authorization server advertises it and the client declares
that grant, and `startAuthorization()` appends `prompt=consent` whenever
`offline_access` is in scope. Against Microsoft Entra ID that forced
consent prompt routes a non-admin user into the admin-consent workflow
and fails with AADSTS90094 — even after a tenant admin has consented.
Adds a per-server "Request refresh token" setting (default on) that
drops the grant from the client metadata, breaking that chain at its
source. Persisted as `oauth.requestRefreshToken` and written only when
off, so entries that never touch it keep a byte-stable round-trip. Web
edits it in Server Settings → Authorization; the CLI and TUI pick it up
from the same per-server settings via `buildRunnerClientAuthOptions`.
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 4 on #2109. A `tools/list` may repeat a name (#1957) and
ToolsScreen already disambiguates those rows with a key of its own, but the
detail panel handed SchemaForm only the name — so switching between two
same-named tools looked like the same entity and left the first one's enlarged
fields (and its in-progress number/JSON text, which predates this PR) in place.
ToolDetailPanel now takes an optional `resetKey`, defaulting to the name for
callers whose list cannot repeat one, and ToolsScreen passes the row key.
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 3 on #2109.
- The focus hand-off ran in a passive effect, so the browser presented at least
one frame with focus on the document body. useLayoutEffect instead.
- The right-section widths were pixel totals. They are now built from the
measured button widths plus the gap and inset, and emitted through `rem` so
they track a user's root font size rather than pinning to CSS pixels.
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 2 on #2109. A disabled form (a tool call in flight) left
the enlarge button live, and activating it swapped in a text area that mounts
disabled — so the focus hand-off targeted an unfocusable element and keyboard
focus was dropped to the document.
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 1 on #2109.
- Activating Enlarge from the keyboard unmounted the focused button and left
focus on the document body, so the next Tab restarted at the top of the page.
The text area now takes focus on mount, caret after the text already typed.
- Every field's enlarge button carried the same "Enlarge" name, so a form with
several string arguments exposed indistinguishable controls. Each now reads
"Enlarge <field label>". Queries that matched a field by its label alone are
scoped to the textbox role, since the button's name now contains it too.
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>
A string argument renders as a single-line `<input>`, which swallows Enter —
so a value containing newlines could not be typed at all.
Each string field now carries an enlarge button (ImEnlarge2) to the left of its
clear button. Clicking it swaps the input for an autosizing text area that keeps
the field's value, label, and length constraints. It is deliberately one-way:
shrinking back would have to either discard newlines the user typed or keep a
value the field can no longer show, and both are worse than staying enlarged.
Enlarging is per-field and resets with `resetKey`, so it cannot carry across to
another tool's same-named field.
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 copy said `progressToken` may be "an integer" while the implementation
validates with `ProgressTokenSchema` — zod's `.int()`, which rejects anything
past `Number.MAX_SAFE_INTEGER`. This repo's own tests assert that
`MAX_SAFE_INTEGER + 1` is dropped, so the documentation was promising a value
the client discards.
Corrected in all three places, so the code comment, the docs callout and the UI
hint agree: `dropInvalidReservedMeta`'s doc block (which review did not flag),
the `metadata` reserved-keys callout, and the Request Metadata hint.
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>
`progressToken` was not the only reserved key. The pinned SDK's
`RequestMetaSchema` also constrains `io.modelcontextprotocol/related-task` to
`{ taskId: string }`, and a conforming server rejects the *whole request* over
one bad reserved member rather than ignoring that key. Before #1910 every value
was a string, so only `progressToken` could be wrong; the widening made all of
them reachable.
Replace both hand-rolled checks with the SDK's schemas — `RequestMetaSchema`
for the payload, `ProgressTokenSchema` for the single value. The schema is
loose, so unknown keys pass through untouched, and zod reports the offending
top-level key as `issue.path[0]`, so only that member is dropped and its
siblings survive. A member the SDK constrains in a later release is then
covered without anyone remembering to add it — which is the drift that produced
the `isInteger`/`isSafeInteger` bug one round ago.
`name` was hard-coded, and react-ace uses it as the editor container's DOM id,
so two instances emitted duplicate ids. Derived from the per-instance id, and
deliberately distinct from the id given to the textarea — that one is what the
wrapper's `<label for>` targets, so reusing it would just move the collision.
The docs table, the docs prose and the form hint all claimed "any JSON" without
qualification, presenting a configuration as valid that the client then drops.
All three now name the reserved keys and say what happens to a bad one.
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>
A textarea is a poor place to hand-write a nested `_meta` payload. Ace closes a
brace when you open one, folds nested objects, and — through its JSON worker —
marks the offending line in the gutter instead of reporting one message for the
whole document.
Three things the swap turned up:
- Ace fires *two* change events for a replace (a remove, then an insert), so
select-all-and-retype briefly leaves an empty document. Acting on the first
emitted `{}` and wiped the user's saved metadata the instant they selected
it. `handleChange` now coalesces the pair in a microtask, and the component
tracks what it last emitted so the parent echoing that value back is not
mistaken for an external reset.
- Ace's own `github` themes fail WCAG AA on the gutter — `#AAA` on `#e8e8e8` is
1.89:1, and the dark theme is 4.13:1. Folding needs the gutter, so the colors
are overridden with palette tokens (5.82:1 and 4.88:1) rather than hiding it.
- `textInputAriaLabel` alone never reaches the DOM: Ace composes the label as
"<label>, Cursor at row N" and only recomputes it when the cursor moves, so
it is forced once on load. Until then the control's only accessible name is a
position readout.
Testing is split because Ace's input path does not work under happy-dom at all
— `userEvent.type` reaches the textarea and produces no edit, so a test written
that way asserts nothing. Unit tests drive the editor instance through a shared
`src/test/aceEditor.ts` helper; the real keyboard behaviour (`{` then Enter
giving `{\n \n}`) is asserted by Storybook play functions in Chromium.
`react-ace`/`ace-builds` are declared in `clients/web` rather than the root, per
the rule for a dependency that renders React components.
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>
`react-refresh/only-export-components` rejects a non-component export from a
component module, so `parseObjectDraft` could not live beside
`JsonObjectInput`. It is a pure transform, which puts it in `utils` under the
`lib`/`utils` rule anyway.
Also render the metadata hint's inline literals through `<Code>` rather than
markdown backticks, which a Mantine `Text` shows verbatim.
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>
Closes#1910
`_meta` was typed `Record<string, string>` end to end in the Inspector —
on disk as a `{ key, value }[]` pair array, in the Server Settings form as
key/value rows, and in every request verb's per-call metadata parameter.
Nothing in the MCP spec restricts a `_meta` value to a string, and the SDK
models the field as a `z.looseObject`, so the restriction was ours alone.
Widen the whole path to `RequestMetadata` (`Record<string, JsonValue>`):
- Per-server default metadata is a JSON **object** on disk and in memory.
The pre-#1910 pair array is still read (`normalizeStoredMetadata`) so an
existing `mcp.json` keeps working; it is never written back, so a save
migrates the entry.
- The Server Settings → Request Metadata section is a JSON editor
(`JsonObjectInput`) instead of key/value rows — rows cannot express an
object value. It keeps the typed text as the display source and emits
only parsed JSON, the same draft split `SchemaJsonField` uses, so the box
stays typeable; invalid text is flagged inline and not applied.
- The CLI stops flattening `--metadata` / `--tool-metadata` through
`metaValueToString`. Those values were already JSON-parsed, so
`trace={"id":"abc"}` used to reach the wire as the *string* `{"id":"abc"}`
and now reaches it as an object.
- `servers/show` redaction replaces a sensitive key's whole value rather
than a string, so a structured value cannot leak through its members.
- `progressTokenOf` narrows the reserved `progressToken` key back to
`string | number` before it reaches the SDK; any other JSON still travels
inside `_meta` but wires up no progress callback.
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>
Round 20, and almost all of it is fallout from round 19's two changes. Both
were right in direction and wrong in the detail, in the same way: each solved
its case by treating a broad thing as a proxy for a narrow one.
**The durability guard wrote newly entered secrets to disk in plaintext.**
Preserving legacy plaintext for a session-scoped store is correct — stripping
it moves the only durable copy into RAM. Using *the whole submitted entry* as
the proxy for "legacy" was not: it turned
`MCP_INSPECTOR_SECRET_STORE=memory` into "write every new secret to
`mcp.json` in the clear", while the footer for that very store says secrets
are written nowhere. A security regression introduced by a data-loss fix, and
the same overshoot in `client.json`.
Provenance decides now. A secret field is kept on disk only when it was
already there, unchanged, in the file being overwritten; a newly entered or
changed value is stripped and lives in the session store, which is exactly
what the footer promises. Where provenance cannot be established — an
unparseable prior file — the value is treated as new, because writing a
secret to disk on a guess is the failure that matters.
**The atomic claim was not as safe as its comments said.** Four ways:
- A crash between the claim and the copy left only
`secrets.json.migrating-<pid>`. The next run checked the canonical path,
found nothing, selected the keychain, and every stored credential vanished
with no warning. Orphans are now adopted at startup.
- Restoring used `rename`, which on POSIX **replaces** its destination — so
the restore path could overwrite a `secrets.json` a concurrent writer had
recreated, destroying the later successful write that claiming exists to
protect. The comment beside it asserted the opposite. `link` + `unlink` is
the no-clobber primitive this needed.
- Every claim failure was read as a lost race. `EACCES`/`EROFS` leave the
file exactly where it is, and returning quietly selected the keychain while
file-backed secrets sat there invisible. Only `ENOENT` is benign now.
- A failed cleanup left a plaintext-capable snapshot on disk with a silent
catch, while the app reported the keychain as the only store. It names the
path now.
**A third secret-entry surface had no disclosure at all.** `ServerConfigModal`
takes stdio `env` values, which are extracted into the store exactly as
Server Settings' are — and the footer's own doc comment claimed there were
only two such dialogs. It has the footer now, and every server write that can
carry a secret (config submit, both import flows) refreshes the descriptor,
not just the settings save.
Full `npm run ci` green, staged tree verified first.
Signed-off-by: cliffhall <cliff@futurescale.com>
The footer's file states now read "Secrets: Plaintext file. Owner-only
permissions." and "Secrets: Encrypted file. Owner-only permissions.",
and the band itself became the copy affordance for the path.
Three things worth reviewing:
- The path is no longer printed on the band. It goes to the clipboard
instead, which means the only place it stays *readable* is the
button's accessible name — so that carries it in full
("Copy secrets file path: /home/node/.mcp-inspector/secrets.json")
rather than a generic "Copy". A clipboard is no use to a screen-reader
user who cannot inspect it.
- "Owner-only permissions." is a claim of fact, so it is made only when
the mode was verified as 0600. `looseMode` is set exactly when it is
something else and could not be tightened, and the message says
"Mode 0644 — not owner-only." there. A footer that asserted the
opposite of the truth about a secrets file is the one failure this
band exists to prevent, so it has its own test.
- The footer phrases its own copy rather than reusing
`secretStorageLabel` for the file case: that label is parenthetical
("File (unencrypted)") because the startup banner appends
" at <path>" to it. The tone and the underlying facts still come from
the shared helpers, so the two surfaces cannot disagree about
anything that matters. Memory's line is still derived from the shared
caveat.
The click target is an `UnstyledButton` filling the band edge to edge —
a real control, reachable by keyboard and announced as a button, rather
than a click handler on a div. Padding moved from the band to the inner
row so the button has no dead margins.
Signed-off-by: cliffhall <cliff@futurescale.com>
Weights the label so the band reads as a labelled fact rather than one
run of prose — the eye lands on what the line is about before reading
which store it names.
Rendered as a `span` inside the existing `Text` rather than as a sibling
in the `Group`: `Text` renders a `<p>`, so a nested one would be a `<p>`
inside a `<p>` (invalid, and the browser closes the outer paragraph
early), and a sibling would take the Group's `gap` and sit too far from
the value it labels. `inherit` keeps it at the label's own size.
Splitting the text is why the assertions move off `getByText`: it matches
a string against an element's *direct* text nodes, so "Secrets: OS
keychain" now reads as two fragments even though it renders as one line.
The unit tests, both modal tests, and the four stories assert
`toHaveTextContent` on the band instead, which reads the full subtree.
Verified in the running dev server: prefix 700, store name 500, both at
the same 12px.
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>
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-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>
#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-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>
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>
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>
Two review findings on the fetch wrapper:
The content-type test was a substring match for "json", which also
matches `application/x-ndjson` — how the streamable-HTTP transport can
serve its long-lived server-push channel (`isLongLivedStreamResponse`).
Awaiting `.json()` on a clone of an unbounded stream never resolves, so
the MCP connection would hang for as long as an override was configured.
It now matches the media type exactly (`application/json` or a `+json`
suffix), with a regression test driven by a stream that never closes.
The overrides also leave `issuer` as discovery returned it, so pointing
one at an authorization server that advertises a different issuer is
rejected on callback by the RFC 9207 / SEP-2352 mix-up defense. That
check is deliberately not weakened; the limit is now stated on the
module, in the field description, and in the configuration guide.
Signed-off-by: cliffhall <cliff@futurescale.com>
Round-2 review follow-ups on #2038, all three from the suppressed set.
The header rows stayed editable while a save was in flight, unlike the rest
of the modal — a slow request let a user edit a row whose value had already
been captured, and the modal then closed having persisted the earlier one.
`KeyValueRows` takes a `disabled` prop and the modal passes `submitting`.
Each row's clear buttons were still named the bare "Clear", so a screen
reader saw six indistinguishable buttons across three rows. They now name
the field they empty ("Clear header name, Cookie").
The App-level headers-into-settings merge — the seam that distinguishes an
edit from a clone, and where the credential copy lived — moves into
`utils/serverSettingsPatch.ts` with its own tests. App.tsx is outside the
coverage gate, so inline it could not be covered at all.
Signed-off-by: cliffhall <cliff@futurescale.com>
The overrides are per-server configuration for the *resource* server's
authorization server, but the fetch wrapper that applies them is shared
with the enterprise-managed (EMA) flow, whose IdP OIDC discovery runs
through the same fetch and satisfies the metadata predicate. An
authorization override could therefore redirect the IdP login to the
resource AS, and a token override could send the IdP code or refresh
token there.
OAuthManager.getEndpointOverrides() now returns nothing when the server
is enterprise-managed — the same boundary redirectToExternalAuthorization
draws for the custom authorization parameters (#2018) — with a regression
test, the note surfaced on both form fields, and the rule documented.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-4 review on #2035 — again reported as "no new
comments" while carrying three suppressed ones. All three reproduced.
1. The TUI form still named its fields from the SDK. `uriTemplateToForm`
read `UriTemplate.variableNames`, which mangles a name: `{;id}` yields
";id" and `{id:3}` yields "id:3". The form therefore submitted
`{ ";id": "7" }` while the shared expander looked up `id`, found
nothing, and dropped the expression — the value vanished silently.
This is the finding that mattered most, because it falsified the claim
this change is built on. Moving the expander into core only makes the
clients agree if each client's FORM derives its names from the same
parser: a form submits under the names it rendered. The TUI now reads
`templateVariables`, and the SDK template is constructed only to
validate, preserving the existing malformed-template diagnostic.
Its `required` flag comes from `requiredGroups`, and only a variable
that is the sole member of a non-omittable expression is marked: ink-
form cannot express "any one of these", so marking every member of
`{a,b}` required would refuse input the expander accepts.
2. `encodeURIComponent` is not RFC 3986's unreserved set — it leaves the
sub-delims !'()* bare, which RFC 6570 requires encoded for every
operator except + and #.
Fixing that settled a design question left open in the previous round.
With two encoders, the SAME value encoded differently depending on
whether its expression happened to carry a modifier, since only the
modifier pushed it onto our path. So delegation is gone entirely: one
expander, one set of rules. The SDK's `UriTemplate` is still
constructed, but only to validate a template.
3. The "Any one of: a, b" hint could contradict the disabled submit
button. A name can sit in a singleton required group AND a shared one
(`x://{a}/{a,b}`), where the singleton demands that exact field. The
hint is suppressed in that case.
Signed-off-by: cliffhall <cliff@futurescale.com>
A floated promise is invisible at review time — the call reads like an
awaited one minus four characters — and the unhandled rejection it
produces surfaces in a different test, in a different file, as a stack
pointing at SDK internals. In #1947 two un-held `callTool` calls made
`npm run ci` unpassable and took a full investigation to attribute; the
fix was two lines. `@typescript-eslint/no-floating-promises` catches the
class outright, so enable it at `error` everywhere.
The rule is type-aware and `tseslint.configs.recommended` is not, so each
scope's config gains a parser project. The four clients name their own
tsconfig projects (both of them — `src` and `__tests__`/`server` live in
different ones). The root scope has no tsconfig at all, so add
`tsconfig.lint.json`: a lint-only project covering `core/**`,
`test-servers/src/**`, and `vitest.shared.mts`, emitting nothing and
gating nothing. It sets `moduleResolution: bundler` deliberately — under
NodeNext `core/`'s extensionless imports fail to resolve, every import
degrades to `any`, and the rule silently stops seeing promises at all.
Measured surface, and the fixes:
- core (2) — `stream.close()` in two Hono `onAbort` listeners. Hono
invokes subscribers with a bare `subscriber()`, so a promise returned
from the listener is floated one layer up; `closeAbortedStream` owns
the rejection where it can actually be owned.
- tui (5) — `handleDisconnect` had no catch of its own and its only
caller is a key handler that cannot await, so a failing disconnect was
a live unhandled rejection; it now surfaces the message the way
`handleConnect` does. The rest are `void` at callers that genuinely
cannot await, over callees that already own their failures.
- web (92) — 83 are Storybook `expect(...)`, which is instrumented and
returns a promise, now awaited (`scrollAreaStoryAssertions` is async
for the same reason). Two tests were asserting against a provider call
they never awaited. `open(url)` in both web servers now warns instead
of escaping a synchronous callback.
- cli, launcher — already clean.
Type-aware linting costs time: `clients/web`'s `eslint .` goes from ~8s
to ~19s, inside `validate`. Narrowing the loaded projects is the lever if
that needs reducing later; dropping the rule is not.
Closes#1959
Signed-off-by: cliffhall <cliff@futurescale.com>
Adding a server by hand offered no way to set custom HTTP headers, so the
only route to a cookie or a routing header was importing a config file.
The manual "Add server" form now carries a Custom Headers key/value editor
for the sse / streamable-http transports, pre-populated when editing or
cloning and submitted alongside the config.
Headers are not part of `MCPServerConfig` — they live on the entry's
`settings` — so they travel as `onSubmit`'s third argument rather than
folded into the config, and `addServer` / `updateServer` gained an optional
`settings` parameter to carry them to the backend (`POST /api/servers`
already accepted one). `updateServer`'s omission semantics are unchanged:
with no settings passed, the route still preserves the node on disk.
`KeyValueRows` moves out of ServerSettingsForm into a shared element so the
two editors cannot drift, and each row's controls gain a row-scoped
`aria-label` ("header value, Cookie", "Remove header, Cookie") — previously
every remove button announced only "X", indistinguishable across rows.
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>
The Resources screen discovered and substituted template variables with a
bare `/\{(\w+)\}/g` regex, which is wrong two ways: it cannot see an
expression carrying an operator, so `foobar://events{?topic}` rendered no
input at all; and it splices the raw value in, so a `/`, `?`, `#`, `%`,
space or non-ASCII character in a simple `{topic}` landed unencoded and
changed the URI's structure -- `foo/bar` produced an extra path segment
that a conforming matcher rejects with `-32602 Resource not found`.
Delegate expansion to the SDK's `UriTemplate`, the same implementation
`InspectorClient.readResourceFromTemplate` (and so the TUI) already
expands through, so the two clients cannot disagree about what a template
means. The new `utils/uriTemplate` supplies only what that class does
not: which variables to render an input for, which of them a read cannot
proceed without, and a partially-expanded preview.
Required-ness follows the operator. Under `?`, `&`, `.` or `/` the whole
expression is omitted when the variable is undefined, so those fields are
marked Optional and reading with them blank is a legitimate request for
the unfiltered resource; under `""`, `+` or `#` the variable sits mid-URI,
so it stays required. Blank fields are dropped before expanding so an
untouched optional field reads as undefined rather than as the empty
string, which would expand to a valueless `?topic=`.
Adds the `rfc6570_templates` preset and a `rfc6570-templates-http.json`
showcase server serving the two templates from the issue.
Closes#1919
Signed-off-by: cliffhall <cliff@futurescale.com>
The Tools screen mounts the Results panel only once a call has produced a
result, but the panel's zero-content branch rendered "No results yet" — the
same wording it would use before anything had run. An empty `content` array
with no `structuredContent` is a legal `CallToolResult`, so a call that had
plainly completed reported itself as a call that never happened.
That reads worst after a multi-round-trip (MRTR) sequence, which is how it was
reported: the user answers one or more embedded elicitations, watches the
Protocol tab group the exchange and mark it COMPLETE, and is then told there
are no results yet. Nothing was lost — the result was stored and rendered; the
panel simply had no wording for "completed, returned nothing".
The panel now renders an "Empty result" alert saying the call completed and
returned no content. This is the remaining half of the same gap #1908 closed
for `structuredContent`-only results.
test-servers: new `mrtr_empty` preset (`createMrtrEmptyResultTool`) — one
elicitation round, then a complete result with `content: []` and no
`structuredContent` — added to `mrtr-showcase-http.json` so the shape is
reproducible by hand. README documents it.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
`ServerTasksCapability`'s `list`/`cancel` are objects, not booleans, so
the legacy-tasks fixture failed `tsc -b` in CI. A stale incremental
build cache hid it locally.
Signed-off-by: cliffhall <cliff@futurescale.com>
SEP-2663 moved task support from `capabilities.tasks` to the negotiated
`capabilities.extensions["io.modelcontextprotocol/tasks"]`, but the
Connection Info modal's capability grid tested only for the top-level
key. A modern server that advertised the extension in `server/discover`
therefore showed "Tasks ✗" while the very same extension id was listed
under "Server Extensions" two sections below.
Server capability rows now fall back to an extension id for the
capabilities the modern era expresses that way (tasks is the only one
today), gated on the negotiated era so a legacy connection is still
judged purely on its `initialize` capabilities. Keyed the same way
`InspectorClient.isTasksExtensionNegotiated()` gates the Tasks tab, so
the checkmark and the tab can't disagree.
Signed-off-by: cliffhall <cliff@futurescale.com>
