No behavior change.
Round 7 removed the loose-mode `console.warn` from selection — it was
encryption-blind and duplicated the caveat printed moments later. This test
kept asserting `warn` was called with "0644" and kept passing, because the
caveat also contains the mode. So it was green while describing a code path
that no longer exists: named "warns once at selection", commented as if the
selection warning were the thing under test, and pinned to a substring loose
enough that either line would satisfy it.
Asserts the caveat's full sentence now, so it is fastened to the surviving
mechanism rather than to whichever line happens to mention the mode.
Also records what it took to reach this state at all, since it is not
obvious and I had it wrong: as the file's owner you *cannot* stage a
loose-mode descriptor, because `tightenSecretFilePermissions` repairs a 0644
file to 0600 during selection and the report is then correctly "owner-only".
It is only real for a file owned by another user or on a read-only mount,
which is why the chmod is mocked — standing in for a privilege a test cannot
drop.
Found by @mcp-inspector-v2-f1, who tried to screenshot the state for the PR
body and got "Owner-only permissions." instead.
Signed-off-by: cliffhall <cliff@futurescale.com>
Five taken; one rejected with evidence, below.
- **The keychain hand-off ran outside the file lock.** Read → copy → delete
has to be one transaction: another Inspector completing a `set` between
the read and the delete had its brand-new secret removed without ever
being copied — a write that reported success and then vanished, which is
the loss the lock exists to prevent, reached through the migration meant
to preserve things. The whole sequence now runs under the same
cross-process lock every mutation takes.
- **An unmigratable entry was skipped and the file deleted anyway.** A key
that is not `serverId:field` cannot be addressed through the store API,
so it cannot be copied — which makes the hand-off incomplete by
definition, and the complete-success rule then forbids removing the
source. It was discarding that value permanently in order to tidy up a
migration that had failed. The copyable entries still go across; the file
stays, and says why.
- **`readOnDiskEncryption` accepted any format version.** A version-2 file
was reported as a usable plaintext or encrypted store even though
`readMap` refuses it and every save fails. Checked before the mode now,
matching `readMap`.
- **The loose-mode warning at selection was encryption-blind**, claiming a
reader could read the secrets of a file that is ciphertext, and it
duplicated the caveat printed moments later by `warnAboutSecretStorage`.
Selection now only repairs; the descriptor path does the reporting,
because it is the one that knows whether the file is encrypted.
- **The footer's tooltip offered to copy the path without showing it.** The
band stopped printing it and the accessible name carries it for
screen-reader users, which left a sighted user unable to see where a
custom destination points without pasting it somewhere else.
**Not taken: "`writeStoreFile` is not atomic."** It is. The finding reads
`store-io.ts` line 84 as a direct write, but `writeFile` there is imported
from the `atomically` package, not `node:fs`. Verified rather than argued:
writing over an existing file changes its inode (so a rename happened) and
leaves mode `0600`. I had started implementing the suggested temp+rename
before checking the import, which would have replaced a tested library with
a hand-rolled duplicate.
That check corrected something of mine, though: `tightenSecretFilePermissions`
justified itself with "a rename over a 0644 file keeps the loose mode
forever", which is false — the temp carries `mode: 0o600` and the rename
replaces the inode, so every *write* re-establishes it. The function is
still needed for the case it actually addresses: a file that exists and is
never written. Comment corrected to say so.
Full `npm run ci` green — 5,753 tests under the coverage gate.
Signed-off-by: cliffhall <cliff@futurescale.com>
Four findings, all suppressed, all real. Three are the same mistake in
different places: a state that was never established being reported as a
confident answer.
- **`readOnDiskEncryption` conflated "no file" with "unreadable file".**
Both answered `null`, and the descriptor falls back to the configured
write policy on `null` — so a passphrase-configured install reported
"File (encrypted)" about a file whose envelope had never parsed, and
which `set` was in fact about to refuse rather than overwrite. It now
returns a four-state result, and only `absent` falls back to the policy
(honestly: the first write really will create it that way). `unreadable`
produces `encryptionUnknown` on the descriptor, `plaintext` is omitted
entirely so nothing can read a default out of it, and the label, caveat,
tone and footer all carry "unreadable" rather than guessing a mode.
- **The loose-mode warning overstated exposure for an encrypted file.**
"Anyone who can read it can read the secrets in it" is false when the
file is ciphertext — a reader gets the bytes and still needs the
passphrase. Both the caveat and the footer tooltip now branch on
encryption: the passphrase becomes the only protection, which is the
real consequence. Overstating it is not harmless, because a warning that
cries wolf is discounted on the occasion it is literally true.
- **The keychain hand-off could invert its own keychain-wins rule.**
`SecretStore.get` answers `null` both for a missing entry and for a
keychain it could not read, and the migration *writes* on `null` — so a
transient read failure would overwrite a newer keychain value with the
older file copy. Added an optional `getStrict` (throws instead of
swallowing) implemented by `KeyringSecretStore`, with
`secretStoreGetStrict` falling back to `get` for stores whose reads
cannot fail. A throw now aborts the hand-off with the file left in
place, so the next run retries.
Writing the tests turned up one more, in code the review had passed:
`parseOwner` recorded a garbled pid as `-1`, and `process.kill(-1, 0)` is
a *wildcard* asking about every process this user can signal — it answers
"alive" almost always, which would have made a lock with a corrupt stamp
permanently unstealable. Only a positive pid is usable now; anything else
is treated as no pid at all and the mtime decides.
Tests: the four envelope states kept distinct; the descriptor refusing to
guess, with the genuine no-file case as the control; strict-read abort
without a write and with the file preserved, plus a store whose `get` and
`getStrict` disagree so the test proves which one the migration consults;
encryption-aware wording asserted in both directions in the caveat and the
band; three unusable owner stamps and a wildcard pid; and the descriptor
shapes that must now be rejected — neither half of the encryption question
answered, or both. Full `npm run ci` green — 5,750 tests under the
coverage gate.
Signed-off-by: cliffhall <cliff@futurescale.com>
Repairs origin as well as answering the review. 2d2c787e committed the
`absorbFileSecretsIntoKeyring` tests without the implementation they
import, so the branch has been failing `tsc` since; this commit brings the
source across.
Round 5's four findings (two inline, two suppressed):
- **The stale-lock takeover was still racy.** Round 4 made *stealing*
liveness-aware but left the takeover as remove-then-recreate, which is
not an election: two waiters can both judge the same lock stale, the
first removes it and takes a fresh one, and the second then removes
*that* — both inside the section, which is the lost update the takeover
exists to prevent. Claiming is now an atomic `rename` to a
token-unique name, so exactly one waiter can take a given stale lock and
the losers get ENOENT. A failed claim also falls through to the deadline
instead of `continue`ing, so a takeover that can never succeed times out
rather than spinning forever on a swallowed error.
- **Every `stat` failure was read as "no file yet".** An existing file we
cannot inspect (EACCES on the directory, a mode-less filesystem) then
produced no warning at all, and the footer went on stating owner-only
permissions having verified nothing. There is now a third state,
`unknown`, carried end to end: `permissionsUnknown` on the descriptor, a
caveat that says the permissions could not be checked, a `warn` tone, and
its own branch in the footer sentence — which otherwise would have
claimed "Owner-only permissions." by omission, the same bug one layer up.
- **mountinfo decoded only `\040`.** The kernel escapes tab, newline and
backslash too (`\011`, `\012`, `\134`), so a mounted path containing any
of them was compared in its escaped form, matched nothing, and read as
the container's writable layer — `memory` selected and persistence
silently lost on a box that had arranged for it. Decoded as octal in
general now, since the rule is octal-in-general and a future addition
would otherwise reintroduce exactly this.
- **File-backed secrets vanished once a keychain appeared.** Save secrets
without libsecret, install libsecret, and the next run selects the
keychain and stops seeing them: still on disk, read by nothing, and
nothing looks broken. `absorbFileSecretsIntoKeyring` hands them over on
the run that finds a keychain, under the rules the two existing plaintext
migrations already use — keychain wins on conflict, the file is removed
only on complete success, and a file that cannot be read is never
deleted, because that is precisely the case where the values would be
lost.
Tests: a two-waiter race over one stale lock asserting a single holder
throughout; `unknown` from `readSecretFilePermissions` through the
descriptor to the footer sentence and tone; the hand-off's happy path,
keychain-wins, write-failure, undecryptable-file, empty-file,
malformed-key, and explicitly-configured-keyring cases; and six more
rejected descriptor shapes, including each file-only field on a non-file
kind. Full `npm run ci` green — 5,732 tests under the coverage gate.
Signed-off-by: cliffhall <cliff@futurescale.com>
Reads "Memory (this session only): Secrets are not written anywhere and
are lost on exit." — shorter tail than "lost when the Inspector exits",
and a colon separating the store from what follows it.
The colon is added at the footer's composition, not inside
`secretStorageLabel`. The label is also consumed alone by
`secretStorageSummary`, which appends " at <path>" for a file-backed
store, so a colon baked into the label would print
"File (encrypted): at /path" in the startup banner. It renders only when
a detail actually follows.
The caveat string is shared with the banner's warning line, so the
terminal picks up the shorter wording too.
Signed-off-by: cliffhall <cliff@futurescale.com>
Found on a re-read of the round-4 liveness work, not by review.
The heartbeat refreshed the lock directory's mtime unconditionally. After
a steal — possible when this process looked dead, or across hosts where
the pid check cannot apply — the directory belongs to someone else, and
touching it keeps *their* lock looking alive. If they then die, the stale
timestamp nobody is maintaining is being maintained by us, and a third
writer can never break in: a stale lock turned unbreakable, which is the
permanent outage the stealing exists to avoid.
The heartbeat now re-reads the owner stamp and clears itself when the
token no longer matches, so it only ever refreshes a lock we still hold.
The release path already checked the token; this closes the same hole on
the other side.
Tested by stamping a different owner onto a held lock and aging it: the
mtime stays old across several heartbeat intervals, and release leaves the
thief's lock intact.
Signed-off-by: cliffhall <cliff@futurescale.com>
Four findings (two inline, two suppressed), all taken. Three are the lock,
which is where the remaining sharp edges were.
- **A lock could be created and then never stamped.** If `mkdir` succeeded
but writing the owner token failed, the catch treated it as "cannot lock
here", returned the no-op release, and left the directory behind. The
mutation then ran unlocked *and* every later writer queued behind an
orphan nobody could deliberately release, until it aged out. The
acquisition now tracks whether it created the directory, removes it on a
failed stamp, and refuses rather than entering the section unlocked.
- **An old mtime was being treated as proof the owner was dead.** It is
not: a holder that is SIGSTOPed, starved by a blocked event loop, or on a
machine that suspended is alive and not running, and expiring it on
elapsed time alone takes the lock out from under a live critical section
— the lost update the lock exists to prevent, arrived at through the
machinery meant to prevent it. The heartbeat narrowed the window; it
could not close it, because the holder that most needs protecting is
precisely the one not running its timers.
The owner stamp now carries `pid` and `host`, and staleness only *opens*
the question — on the same host a waiter asks the OS whether that pid
still exists (`kill(pid, 0)`, which sends no signal; `EPERM` counts as
alive, so a holder running as another user is never stolen from). Only a
genuinely absent process is stolen from. Where the stamp cannot be
trusted — another machine over a shared filesystem, or a missing/corrupt
owner file — the mtime remains the answer, because the alternative is a
lock nothing can ever break, turning one dead process into a permanent
outage.
- **Podman was classified as a host.** `/.dockerenv` is Docker's marker and
rootless Podman has no equivalent of it, while its cgroup paths say
`libpod-<id>.scope` rather than the literal "podman" the pattern looked
for. Such a box fell back to a file in the ephemeral writable layer and
reported it as durable — the wrong way round to be wrong.
`/run/.containerenv` and `libpod` are both recognized now.
- **Two tests floated a second `acquireLock`**, so the test could finish and
`afterEach` could remove the temp directory while the acquisition was
still running. Both hold and settle it now.
Separately, a defect of my own found while re-reading round 3:
`describeFileStore` called `tightenSecretFilePermissions`, so the function
whose job is to *describe* the store issued a `chmod` on every
`GET /api/config` — once per page load, on a file another process may be
mid-write on. Split out a read-only `readSecretFilePermissions`; the repair
stays at startup, where it happens once and its outcome is announced.
Tests: a stale lock whose owner is alive is not stolen (and survives the
attempt), one whose pid is gone is, one stamped by another host is; a
failed ownership stamp reports and leaves nothing behind; Podman via both
signals; and the read-only permission check leaves a 0644 file at 0644.
Full `npm run ci` green — 5,711 tests under the coverage gate.
Signed-off-by: cliffhall <cliff@futurescale.com>
Nine findings (six inline, three suppressed), all taken. Two were serious.
**The memory fallback made the migrations destructive.** Both migrations —
mcp.json in the remote server, client.json in `node-persistence` — lift a
plaintext secret off disk, write it to the secret store, and then delete it
from the file. That trade is safe only while the store is at least as
durable as the disk, which held by construction while `InMemorySecretStore`
was a test double. Making it a production fallback broke it: on an
unmounted container a plain `GET /api/servers` would have moved a user's
existing secrets into RAM and lost them at exit — destroying data on a
read.
`SecretStore` gains an optional `isDurable()`, absent meaning durable, and
both migrations withhold the disk delete when it answers false (the value
is still loaded, so the session behaves normally). The production fallback
is now a named `SessionSecretStore` rather than the bare
`InMemorySecretStore`, so the suite's many doubles keep standing in for a
*working keychain* — which is what they are there to be — and only code
that deliberately chooses RAM in production says so.
**Round 2's per-call descriptor never reached anyone.** Both web servers
resolved it once at startup and baked it into `initialConfig`, which
`/api/config` serves unchanged — so the freshness fix was inert for its
only consumer. The route now takes a `secretStorageResolver` and calls it
per request.
The lock needed three more fixes, and they compound:
- **Release removed whatever directory was at the path.** A holder whose
work outran the stale threshold would delete the *next* owner's lock,
letting a third writer into a section two processes believed they held.
Ownership is now stamped with a random token and release only removes a
lock still carrying it.
- **The timeout fell through to an unlocked write** — under contention,
the one moment another writer is provably active. It now throws
`SecretFileLockTimeoutError`, which `set` is allowed to do.
- **The first write of every process bypassed the lock**, because the
secrets directory does not exist yet and `mkdir` failed ENOENT. The
parent is created first. First writes are exactly when two processes are
most likely to start together.
Fixing the timeout exposed a fourth: with stale (10s) below timeout, a
waiter stole from a holder that was merely *slow* rather than dead —
reinstating the lost update. A holder now heartbeats its lock, so
"stale" means the holder is gone, and the timeout is what a live-but-slow
holder produces. Lock timings are injectable so the tests can exercise
contention, staleness, and timeout without spending the real intervals.
Also: the loose-permission state is carried into `SecretStorageInfo` as
`looseMode` and rendered as the caveat (it outranks the encryption caveat —
a file others can read is a live exposure, while "unencrypted" is a
property of a file only its owner can open), rather than living in a
startup log the browser cannot see; and `usableSecretStorage` validates
`pendingEncryption` and `looseMode` as the optional booleans/numbers they
are (`pendingEncryption: "false"` is truthy and would have printed advice
that is the opposite of the truth) and rejects file-only fields on a
keychain or memory descriptor.
Tests: both migrations against a session store with a durable control
alongside, so the guard is visibly the difference; `/api/config` serving a
changing resolver and falling back without one; lock ownership after a
steal, the timeout under a live holder, no-steal from a slow holder, and a
locked first write; `looseMode` through the descriptor, the caveat
ordering, and the tone; and five more rejected payload shapes. Full
`npm run ci` green — 5,704 tests under the coverage gate.
Signed-off-by: cliffhall <cliff@futurescale.com>
Seven findings, all taken.
- **The descriptor went stale after the upgrading write.** Round 1 made it
read the file instead of the policy, but it was still computed once and
cached beside the selection — so the first `set` under a newly-set
passphrase encrypted the file while `/api/config` kept serving "still
unencrypted" for the rest of the session, describing bytes this very
process had just changed. The *selection* stays cached (the banner, the
payload, and the store doing the writing must agree on which store it
is); the file descriptor is re-derived per call, which costs one read of
a small file per config fetch.
- **The mount check asked the wrong question.** Comparing a directory's
`st_dev` against its parent's detects a mount *at* that path, but a
volume mounted at `/home/node` leaves `/home/node/.mcp-inspector` an
ordinary subdirectory sharing its parent's device — so a durable
container setup was judged unmounted and demoted to session-only
`memory`. It now reads `/proc/self/mountinfo` and asks whether the
directory is *under* any mount other than `/`, which also catches the
same-device bind mount no `st_dev` test can see. Where mountinfo is
unreadable (macOS, Windows, a `/proc`-less container) it falls back to
the device comparison, now against `/` rather than the parent.
- **A decrypted payload was cast, not checked.** GCM proves the bytes are
authentic, not that they parse to the right shape. An array passes
`typeof === "object"`, takes the named assignment, and then
`JSON.stringify` drops every named property of an array — so `set` would
resolve having written a file without the secret it was handed. Silent
loss on the one operation whose entire contract is that it does not lose
things. Both branches now validate a non-array object of string values,
and a shape refusal is deliberately not rethrown as a key mismatch,
which would send the user to fix a working passphrase.
- **The read-modify-write cycle was only serialized in-process.** A
durable file is shared state and a second Inspector on the same box (a
CLI run beside a web session) could read the same map and overwrite the
first one's save, both reporting success. Mutations now take a
`mkdir`-based lock. A lock left by a killed process is stolen after 10s
— one `docker kill` must not brick secret writes permanently, which
would be a worse failure than the race. If the lock cannot be created at
all, the write proceeds unlocked rather than denying a real user a real
secret to prevent a race with a process that may not exist. Reads do not
lock: `writeStoreFile` is atomic, so a reader sees the old file or the
new one, never a torn one.
- **The permission repair was fire-and-forget while the docs promised
0600.** A file owned by another user, or on a read-only mount, stayed
exposed while the footer described it as protected. It now verifies the
resulting mode and warns, naming the actual mode, when it could not be
tightened.
- **`Group`'s theme carried a domain-named `secretStorageFooter`
variant**, which AGENTS.md reserves for app-wide primitive styling.
Renamed to the structural `stickyModalFooter`; the secret-storage
meaning stays in the element that owns it.
- The PR description still claimed the salt made a short passphrase safe,
contradicting the corrected comment and README. Updated.
Tests: both mount layouts that motivated the change (mount-at and
mount-above) plus the sibling-prefix, space-escaped, first-run, and
no-mountinfo cases; the array and non-string refusals on both the
plaintext and the authentic-ciphertext paths (the latter through a
hand-built fixture, since the store's own writer cannot produce that
state); lock exclusion, stale-lock stealing, the un-lockable fallback, and
two independent store instances racing; the permission report in all three
states; and the descriptor no longer going stale within one process. Full
`npm run ci` green — 5,688 tests under the coverage gate.
Signed-off-by: cliffhall <cliff@futurescale.com>
Six findings, all taken.
- The descriptor reported the *write policy*, not the file. Adding
`MCP_INSPECTOR_SECRET_KEY` to an install that already has a plaintext
file flipped `store.encrypted` immediately, so the banner and footer
claimed "File (encrypted)" while the existing secrets sat readable
until the next `set`. `FileSecretStore.readOnDiskEncryption()` now
reads the envelope (never the payload, so it needs no passphrase) and
the descriptor is built from that. The transitional state gets its own
`pendingEncryption` flag, which changes the advice without softening
the verdict — "set a passphrase" is the wrong thing to tell someone who
already has, so the caveat names the pending write instead.
- The secrets file ignored the configured storage directory. A container
mounting only that directory was judged unmounted, fell back to
`memory`, and lost secrets the user had arranged to keep.
`defaultSecretFilePath()` now follows `MCP_STORAGE_DIR` — the variable
that already relocates OAuth tokens and `client.json` — between the
explicit `MCP_INSPECTOR_SECRET_FILE` and the `~/.mcp-inspector`
default. (The issue comment called it `MCP_INSPECTOR_STORAGE_DIR`; no
such variable exists.)
- `version` was never checked before the plaintext branch, so a
`{ version: 2, encryption: "none" }` file was read and silently
rewritten as version 1, discarding whatever the newer format added —
the same destroy-to-satisfy-an-additive-request case the encrypted
branch already refuses. Checked first now, so the answer no longer
depends on which encryption mode the newer writer chose.
- The doc comment claimed a per-file salt made "a short, memorable value
safe to use". It does not: the salt defeats precomputation, not
guessing, and the scrypt cost is deliberately a few milliseconds
because it runs on every read and write. Both the comment and the
README now say to use a high-entropy value, generated rather than
chosen.
- `usableSecretStorage` validated only `kind`, so a partial
`{ kind: "file" }` was cast through and — because a missing `plaintext`
is falsy — rendered as the quiet, neutral *encrypted* file. That is the
confident half-answer the guard exists to reject, produced by the
absence of information rather than by any claim the backend made.
`reason`, `durable`, and each kind's own required fields are checked.
- The footer's inline `Tooltip` carried four static props; extracted to a
`CaveatTooltip` constant per the `.withProps()` rule.
Tests cover each: the policy/file disagreement in both directions and the
no-file-yet fallback, `MCP_STORAGE_DIR` and its precedence against
`MCP_INSPECTOR_SECRET_FILE`, the version refusal (asserting the bytes
survive it) split from the unrecognized-cipher refusal it used to be
conflated with, `readOnDiskEncryption` including the keyless and
unparseable cases, five new rejected descriptor shapes, and the changed
caveat. Full `npm run ci` green.
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>
* fix: load @napi-rs/keyring lazily so an unsupported platform degrades
`@napi-rs/keyring` ships one prebuilt binary per platform triple and
throws on import where it has none. On Android / Termux there is no
`@napi-rs/keyring-android-arm64`, so the static top-level import in
`core/auth/node/secret-store.ts` threw during module evaluation and the
Inspector exited at startup with "Cannot find native binding" — before
any of the keychain-unavailable handling in that same file could run.
Replaces the static import with a cached dynamic one. The outcome is
cached, not just the module, so a box without a binary doesn't
re-attempt (and re-throw) resolution on every secret operation —
`expectedSecretFields` means that would otherwise be once per server per
`GET /api/servers` — and so `set` can name the underlying cause in its
`KeychainUnavailableError`.
An unloadable package now folds into the same degradation contract as an
unreachable keychain: `get` returns null, `delete` and
`deleteAllForServer` no-op, and `set` throws `KeychainUnavailableError`
(not double-wrapped — the load failure is already typed by the time the
catch sees it). The class doc comment now enumerates all three ways
"unavailable" can arrive, since each escaped the contract at some point.
The shared `vi.mock` stub can't express this — it models a keyring that
loads — so the new tests build on `vi.resetModules()` + `vi.doMock` with
a throwing factory and re-import the module, covering the load failure,
each method under it, and the load-once caching.
Verified the dynamic import survives bundling in all three clients
(cli, tui, and the web runner) rather than being inlined back to a
static one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F
* fix: steer a missing native binding to a reinstall hint
`KeychainUnavailableError` gave the Linux libsecret / gnome-keyring
advice for every cause, including the one where the keychain is fine and
the @napi-rs/keyring platform package is what's missing — an unsupported
triple (#1905) or npm's optional-deps bug dropping it on a supported one
(npm/cli#4828, the Windows report on #1852). Detect the loader's "Cannot
find native binding" phrasing and point those users at a reinstall /
npx-cache clear instead.
Ported from the superseded #1943, which fixed this alongside the same
lazy-load change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
* fix: name a bad keyring namespace instead of "is not a constructor"
The lazy import accepted whatever the resolution produced. The named
exports arrive through CJS interop, so a resolution that stopped
yielding them — a default-only export upstream, a bundler changing
interop, a platform where named-export detection fails — would hand us
`undefined`, and `new mod.AsyncEntry(...)` would throw a TypeError
inside the very try that implements graceful degradation.
Shape-check the resolved module and treat a bad shape as unavailable.
Scope, because it is narrower than it first appears: this does NOT
prevent a bad shape from emptying the secret list. `get` returns null
either way — that is its read-tolerance contract, and the TypeError
lands in the same catch a dead keychain does. What changes is the
diagnosis. Without the check the only signal is `set` reporting
"keyring.mod.AsyncEntry is not a constructor", which reads like an
Inspector bug; with it, `set` names the real problem once, at the load
boundary, in the same actionable 503 as every other unavailability.
Catching the silent-empty-list case itself would need a round-trip
against the unmocked package, which nothing in the suite does today.
The member access sits inside the try because reading a missing export
is not always a harmless undefined — a Proxy-backed namespace can throw,
as vitest's module mocks do — and letting that escape would reject the
cached promise, which is otherwise guaranteed to always resolve.
Of the four new tests only the cause-message one fails without the
guard; the rest pin the surrounding contract.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
* docs: correct the test header's count of unavailability entry points
The shape check added a fourth way "unavailable" can arrive, and the
enumeration in secret-store.ts was updated to match, but the test file's
header still said three — so it read as an exhaustive list that silently
omitted the case a whole describe below it covers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
* fix: give the wrong-shape cause its own remediation hint
The shape check added a cause that fell through to the libsecret
advice — so a packaging mismatch told the user to install a keyring
daemon, which cannot help, and on Windows or macOS points somewhere that
does not exist. That is the same wrong-advice bug the native-binding
branch was added to fix, reintroduced by the guard itself.
Hint selection moves into `hintFor`, where libsecret is the explicit
fallback rather than the default, and the shape failure becomes a
distinct `KeyringModuleShapeError` matched by type — it is our own error,
so there is no reason to re-parse text we wrote (the native-binding
branch matches a string only because that message comes from napi-rs).
Also retitles the wrong-shape test: it claimed the guard avoids
"returning null secrets", but `get` returns null either way by the
read-tolerance contract. The title now says what actually differs —
`set` hard-fails — matching the correction already made in the comments.
Both raised by Copilot review.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
* fix: give both shape-check failure paths the packaging hint
The shape check can fail two ways — the members are absent, or reading
them throws — and only the first carried KeyringModuleShapeError. The
catch path returned the raw error, so a Proxy-backed namespace throwing
on access fell through to the libsecret advice: the same wrong-remedy
bug as the previous two commits, one branch further along.
Both paths now carry the type. KeyringModuleShapeError takes a detail
string plus a standard `cause`, so the throwing path reads "its exports
could not be read: <original>" and keeps the underlying failure legible
while still earning the packaging hint.
Three instances of one mistake in a row (a new cause added without
deciding which remediation it inherits) says the shape was wrong, not
just the branch: making libsecret an explicit fallback in `hintFor` did
not stop me adding a second exit that skipped it. The tests now pin the
invariant directly — one asserts both construction paths produce the
same hint — so the next cause that forgets is a failing test rather than
a reviewer's catch.
Raised by Copilot review.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: construct AsyncEntry inside the try so keychain degradation engages
`KeyringSecretStore` built its `AsyncEntry` outside the `try` in `get`,
`set`, and `delete`. `AsyncEntry::new` performs the platform-store setup
(on Linux, the Secret Service connect with a keyutils fallback) and
throws when no backend is reachable, so the documented degradation
contract never engaged for a construction-time failure: the raw keyring
error escaped, 500ing every `GET /api/servers` on a box without a Secret
Service (the published container, which has no D-Bus session).
`expectedSecretFields` always includes the OAuth slot, so `rehydrateConfig`
constructs an entry for every server — the default seeded catalog was
enough to 500 the first list load, before any server was added. And
because the escaping error wasn't a `KeychainUnavailableError`, it
bypassed both the routes' 503 translation and the `migratePlaintextSecrets`
skip branch, yielding a generic 500 instead of the actionable message.
Moving construction inside the existing `try` restores the contract for
that failure mode: `get` returns null, `delete` no-ops, and `set` is the
only operation that throws — as `KeychainUnavailableError`, which is what
the 503 translation and the migration skip both match on.
The test stub's constructor could not fail, which is why the coverage
gate never saw the gap. Adds a `constructorThrows` hook alongside the
existing method-level `failures` flags and covers all three methods plus
`deleteAllForServer` under it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F
* test: pin why the OAuth secret field is unconditional
`expectedSecretFields` always includes the OAuth slot, and #1848's report
frames that as why the 500 fired so broadly — which invites a follow-up
that skips the keychain read for servers with "no OAuth config". That
change would lose data.
`extractSecretsFromStored` deletes the `oauth` block outright when
`clientSecret` was its only property, so such a server carries NO marker
on disk that a secret exists — the keychain is the sole record, and the
unconditional slot is what finds it again. Gating the read on a
disk-visible `oauth` block would silently stop rehydrating exactly that
shape.
The existing tests do fail if the slot is made conditional (verified by
applying the change: 4 red), but only under names that explain nothing
about the consequence — `always lists the OAuth slot first` reads like a
tautology worth updating rather than a trap. Add a round-trip case that
states the consequence, so the next reader sees the field is load-bearing
rather than defensive.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Migrate all three clients + core + test-servers from
@modelcontextprotocol/sdk@1.29 to SDK v2 (2.0.0-beta.4), keeping behavior
identical against legacy (2025-11-25) servers. Foundation for the SDK V2 +
New Spec stack.
Packages
- Split @modelcontextprotocol/sdk -> @modelcontextprotocol/client (Client,
transports, OAuth, error taxonomy, protocol types) + core (Zod schemas);
stdio -> client/stdio, Ajv -> client/validators/ajv. Test-servers ->
@modelcontextprotocol/server (+ server-legacy/sse). Deterministic import
rewrite (schemas -> core, everything else -> client); the codemod misroutes
client code to /server so it was not used for path routing.
- new Client(..., { versionNegotiation: { mode: 'legacy' } }) — nothing
2026-era goes on the wire.
Runtime (core/mcp/inspectorClient.ts)
- Error taxonomy: McpError -> ProtocolError, ErrorCode -> ProtocolErrorCode;
RequestTimeout -> SdkError/SdkErrorCode.
- Handlers take method strings; task methods (tasks/*, notifications/tasks/*)
use the 3-arg custom form (excluded from v2's spec-method set).
- Legacy tasks kept working (per project requirement): requestor tasks re-
implemented over raw client.request + explicit deprecated schemas;
experimental.tasks.callToolStream replaced by a local pollTaskToolCall
async generator (with _progressHandlers revival so task-execution progress
still reaches the caller). Receiver (server-initiated) tasks: the v2 Client
validates a sampling/elicit handler result and rejects the legacy { task }
response with -32602 — installReceiverTaskResponseBypass swaps the wrapped
entry in the private _requestHandlers map for the task-augmented branch so
{ task } reaches the wire (fixes server-initiated sampling-via-task).
- Pagination: list verbs use raw client.request({method}, List*ResultSchema)
for single-page behavior (v2's client.listX() auto-aggregate all pages).
Web / auth / apps
- Rename local JsonSchemaType -> InspectorFormSchema + add toFormSchema()
structural narrowing at the SDK-schema -> form boundary.
- ext-apps still peers on SDK v1: one compat boundary with documented casts
(remove when ext-apps#702 lands a v2 peer).
- OAuth provider saveClientInformation reconciled with v2's
OAuthClientInformationContext; InvalidClientMetadataError re-homed as a
local class (server-legacy/auth is not browser-safe).
- vitest.shared.mts dedupes @modelcontextprotocol/client+core to one copy so
vi.mock intercepts the SUT's import.
Accepted v2 behavior changes (per #1624 / spec §9.2): unknown-tool call
rejects with -32602; cancellation is stream-abort (no guaranteed
notifications/cancelled frame); output-schema strictness restored via the
Inspector's own Ajv check on the default path.
npm run ci green: validate (web 3087 / cli 212 / tui 274 / launcher 5),
coverage (>=90 per-file, all clients), smoke (launcher/cli/web), storybook
(410).
Closes#1624
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(cli): programmatic ergonomics — initialize, tool-args-json, connect-timeout, env-var fixes
Adds the #1573 programmatic-ergonomics surface to the CLI, re-implemented
informed by PR #1510 and adapted to the v2/main OAuth-wired cli.ts:
- `--method initialize` — connect-only probe emitting
`{serverInfo, protocolVersion, capabilities, instructions}`.
- `--tool-args-json '{"k":"v"}'` — tool args passed verbatim as JSON (no
key=value coercion); mutually exclusive with `--tool-arg`, with clear
errors for combined use, malformed JSON, and non-object values.
- `--connect-timeout <ms>` — default 15000 for ad-hoc `--server-url`/target
runs, file-level timeout otherwise; `0` disables. Backed by the exported
`withConnectTimeout()` helper.
- MCP_CATALOG_PATH is ignored when an ad-hoc target is given, so a shell that
exports it can still run one-off ad-hoc invocations.
- MCP_STORAGE_DIR is honored by core `getStateFilePath` (`<dir>/oauth.json`),
below the per-file MCP_INSPECTOR_OAUTH_STATE_PATH override.
Tests: new programmatic-ergonomics.test.ts (CLI, in-process runCli + direct
withConnectTimeout unit tests) and getStateFilePath resolution cases in the
web storage integration suite. Per-file coverage gate stays ≥90.
README: documents the new flags and env-var semantics.
Closes#1573
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
* feat(cli): --app-info probe + --format json envelope
Adds the #1574 app-probing and machine-readable output surface, re-implemented
informed by PR #1510 and adapted to the v2/main OAuth-wired cli.ts.
- `--method tools/call --tool-name <t> --app-info` — prints one JSON line
describing the tool's app posture (hasApp, resourceUri, csp, permissions,
domain, …) WITHOUT invoking the tool; exit 0 when it has an app, 2 (no_app)
when it doesn't. A missing tool exits 5 (tool_not_found) — distinct from the
no-app exit so a typo isn't mistaken for "no app".
- `--method tools/list --app-info` — one NDJSON line per tool over a single
connection (`| jq -c 'select(.hasApp)'`).
- `--format json` — a single stdout envelope (`{result}` plus `{appInfo}` as a
sibling key for App tools), no banner, so any method pipes into jq.
- A tools/call returning isError:true now exits 5 (tool_is_error) after
printing its payload, so && chains stop on a failed call.
Implementation: `collectAppInfo()` (core `extractAppInfo` + a tolerated
resource read that reports `resourceError`), an NDJSON loop in tools/list, and
a single `emitResult()` that owns format + exit-code handling. `runMethod` now
returns a `MethodOutcome` discriminated union so the NDJSON path can signal
"already emitted".
Tests: app-info.test.ts (exit-0/2/5 + NDJSON + no-invoke), format-json.test.ts
(envelope shapes + bad --format), emit-result.test.ts (direct unit coverage of
the isError→exit-5 and resourceError branches via exported emitResult/
collectAppInfo). Updated the tools.test "nonexistent tool" case to the new
exit-5 tool_not_found contract. Per-file coverage ≥90 on all four dimensions
(cli.ts 100% stmts/funcs/lines). README documents the flags, NDJSON shape, and
exit semantics.
Closes#1574
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
* feat(cli): stored-auth suite — --use-stored-auth, --wait-for-auth, --list-stored-auth, --print-handoff
Adds the #1575 web→CLI OAuth handoff surface, re-implemented informed by PR
#1510 and adapted to v2/main.
- `--use-stored-auth` — read the shared oauth.json and inject
`Authorization: Bearer <token>` for --server-url; exit 3 (no_stored_token),
listing stored keys, when none matches.
- `--wait-for-auth <sec>` — poll oauth.json (500ms) until a token for
--server-url appears, then proceed; timeout → exit 3 (auth_wait_timeout).
- `--list-stored-auth` — print `{oauthStatePath, storedServerUrls}` and exit
(short-circuit, no connect).
- `--print-handoff` — emit `{serverUrl, deepLink, portForwardCmd,
oauthStatePath, apiToken, note}` for --server-url and exit.
Adaptations vs. the reference:
- State-path resolution reuses core `getStateFilePath()` (the
MCP_INSPECTOR_OAUTH_STATE_PATH → <MCP_STORAGE_DIR>/oauth.json → default
precedence added in #1573) instead of a CLI-local resolver.
- The state file is read through the shared isomorphic
`parseOAuthPersistBlob`, so both the plain `{servers,idpSessions}` layout the
web backend writes and the legacy `{state,version}` envelope are accepted.
- `parseArgs` now returns a ParseResult union so the short-circuit modes return
without a server connection.
Blind injection: the token is passed as-is; a stale one surfaces as HTTP 401 →
exit 3. Documented in the README along with all four flags, the state-path
precedence, and a follow-up note that --print-handoff's deepLink shape is
interim pending #1576's canonical handoff format.
Tests: stored-auth.test.ts drives injection, --header coexistence, missing
--server-url, no-token exit-3, URL-normalisation matching, MCP_STORAGE_DIR
resolution, listing, handoff (+note), wait-for-auth polling/timeout, and
normalizeServerUrl units — all against a real in-process HTTP test server.
Per-file coverage ≥90 on all four dimensions (cli.ts 100% stmts/funcs/lines).
Closes#1575
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
* docs(core): scope MCP_STORAGE_DIR comment in getStateFilePath to the OAuth backend
Address review of PR #1651: the doc comment claimed MCP_STORAGE_DIR was
honoured "across the app, so setting it once points every persist backend at
the same directory", but the branch only relocates oauth.json. Narrow the
comment to state it is OAuth-backend-only (client.json/mcp.json still resolve
from getDefaultStorageDir → HOME) and record why the resolution lives here
rather than in getDefaultStorageDir (the default-path constant is evaluated at
module load, so a runtime-set MCP_STORAGE_DIR — as the CLI stored-auth /
--wait-for-auth flows set it — must be read at call time). Comment-only; no
behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
* review(cli): per-tool-tolerant app-info listing + drop dead helper (PR #1653)
Address review of PR #1653:
- collectAppInfo now never throws: a malformed `_meta.ui.resourceUri`
(extractAppInfo throws) folds into `{hasApp:false, resourceError}` just like a
resources/read failure. This makes `tools/list --app-info` per-tool tolerant —
one bad tool no longer aborts the whole NDJSON listing with a generic exit-1.
Guarding inside collectAppInfo (vs. a loop-local try/catch) keeps the branch
unit-testable; added an emit-result.test.ts case for the malformed-URI path.
- Documented that `tools/list --app-info` always emits NDJSON regardless of
`--format` (the per-tool list shape is fixed; --format json only reshapes the
single-result paths) — in both the code comment and the README.
- Removed the now-unused `expectJsonError` helper (its only caller moved to the
exit-5 tool_not_found contract).
Coverage stays ≥90 on all four dimensions (cli.ts 100% stmts/funcs/lines).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
* review(cli): stored-auth follow-ups — TODO(#1576), wait-for-auth guard test, docs (PR #1655)
Address review of PR #1655:
- Added an explicit TODO(#1576) on buildHandoff's deepLink: `transport:"http"`
is hardcoded even for SSE, and `autoConnect=<token>` is not one of the three
token sources the web app reads (per CLAUDE.md) — both to be reconciled with
#1576's canonical handoff format.
- Added a test for `--wait-for-auth` without `--server-url`, covering the
previously-untested `--wait-for-auth` arm of the shared missing-server-url
guard's message ternary.
- Documented that `--list-stored-auth` / `--print-handoff` are mutually
exclusive short-circuit modes (list takes precedence) and that a stored
refresh_token is not yet used (noted as a follow-up in code + README).
No behavior change; coverage stays ≥90 on all four dimensions (cli.ts 100%
stmts/funcs/lines).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The refactor routed the browser's RemoteOAuthStorage through
`oauth-persist.ts`, which value-imported the Node-only `store-io.js`
(`atomically` -> `stubborn-fs` -> `node:process.getuid`, plus
`node:fs/promises`). That dragged Node built-ins into the browser bundle
and the web app crashed to a blank page at runtime — a regression from
v2/main, where browser OAuth storage used a Zustand JSON adapter with no
Node imports. Unit/integration tests run in node/happy-dom and
`smoke:web` only checks the served HTML, so none of them execute the app
in a real browser and the break slipped through.
Fix, following the existing `store-id.ts` precedent:
- Split the pure JSON (de)serializers into Node-free
`core/storage/store-serialize.ts`; `store-io.ts` re-exports them so
existing importers are unaffected.
- Move the Node-only `createFileOAuthPersistBackend` into
`core/auth/node/oauth-persist-file.ts`; update `storage-node.ts` and
the storage adapters test to import from there.
- `oauth-persist.ts` now imports only the Node-free serializers, so the
browser bundle no longer pulls in `node:fs`/`atomically`.
Tests:
- oauth-persist-browser-safe.test.ts: source-level guard that
`oauth-persist.ts` never imports store-io/node:/atomically.
- Direct unit tests for the isomorphic remote + session backends
(restoring oauth-persist.ts coverage after the file backend moved out).
- store.test.ts: OAuthMemoryStore unit coverage incl. replace() fallbacks.
- storage.test.ts: clearAllOAuthClientState null-snapshot (no file) path.
Verified end-to-end in a real browser (Playwright): DCR OAuth flow
completes, tokens persist via the remote store, and after a full page
reload the server reconnects with no re-authentication.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(web): update OAuth E2E tests to use new authentication method
- Replaced `runGuidedAuth` with `authenticate` in the InspectorClient OAuth end-to-end tests to support quick authentication with CIMD pre-registration.
- Updated comments to reflect the change in authentication method and its support for HTTP test metadata URLs.
- Ensured that the tests continue to validate the expected authorization URL after the change.
Additionally, imported `ensureCimdClientRegistration` in the OAuthManager to streamline client registration during the authentication process.
* Guided-auth removal, OAuth smoke testing, and TUI/CLI support
* Added CLI OAuth runner flags tests
* Added clarifying comment on port 0 binding
* README notes on TUI oauth callback port, prettier fixes for CLI tests
* CIMD validation uses inline like EMA IdP
* Fixed TUI test failures from merge
* test: bring 5 OAuth files over the 90 per-file coverage gate
The guided-auth-removal work added/touched several files that fell below
the repo's >=90 per-file coverage gate (npm run coverage). CI doesn't run
the gate (it's local-only), so validate + test:integration passed while
these gaps slipped through. Bring them all back over the line:
- core/auth/node/runner-oauth-callback.ts: test bad-URL/non-http-scheme
throws, no-port->80 default, and IPv6 hostname bracketing. Annotate the
provably-dead guards (empty host, out-of-range port, String(err)
fallback, pathname "/" fallback) -- all rejected by new URL() upstream --
with justified v8 ignore comments instead of lowering the gate.
- core/auth/cimd.ts: test the AS-doesn't-support-CIMD early return.
- core/react/useClientSettingsDraft.ts: test the functional-updater path,
the stale-onChange-after-close prev===null guard, and the
timer-fires-after-close value===null guard.
- ClientSettingsForm.tsx: test the CIMD checkbox toggle and CIMD URL clear.
- clients/tui/src/utils/oauthDisplay.ts: test static/dcr registration
kinds, the expired IdP session, and the configured-scope/empty-scope
branches of formatScopes.
npm run coverage now passes (exit 0) across web, cli, tui, launcher.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: cliffhall <cliff@futurescale.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(web): audit branch-coverage outliers, raise per-file gate to 70 (#1271)
The web per-file coverage gate had drifted red: the recently-landed EMA
auth feature (core/auth/ema/*) and ClientSettings components shipped
undertested, and the prior 50% branch floor let business-logic branches
go untested. CI does not run the coverage gate, so this accumulated.
Add tests to bring every failing file above the gate and push branch
coverage well past 70 across the board, then raise the per-file branches
threshold 50 -> 70 in clients/web/vite.config.ts.
- EMA core (emaFlow, resourceContext, jwt, wire): 50-88% -> 86-100% branch
- EMA OIDC/transport (idpOidc, tokenEndpoint, transportProvider): -> 97-100%
- oauthManager / discovery: 73-83% -> 88-92%
- ClientSettingsModal/Form, ConnectionInfoContent: 0-88% -> 96-100%
- useClientSettingsDraft / useEmaIdpLoginState hooks: 20% -> 100%
- TaskStatusBadge / SortableServerCard outliers: 50% -> 100%
Coverable branches reached by mocking useComputedColorScheme (dark theme),
dnd-kit useSortable (isDragging), and fetch/OIDC paths. Remaining sub-100
branches are genuinely-unreachable defensive guards or happy-dom-inherent
(portal / useMediaQuery / typeof window) and documented in test comments.
No source files changed. Global web branch coverage 88.73% -> 90.63%.
AGENTS.md updated; cli/tui/launcher stay at 50 (out of scope).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: raise per-file coverage gate to 90/90/90/90 across all clients
Follows the #1271 web branch-coverage audit (50 -> 70) by raising the
whole per-file gate to >=90 on all four dimensions — lines, statements,
functions, branches — for web, cli, tui, and launcher (core/ is measured
under the web suite). A codebase-wide audit added real tests for every
outlier and brought global coverage up across the board.
Config: all four vitest/vite coverage configs set to 90/90/90/90.
Two pure-type modules (core/mcp/import/types.ts, clients/web/server/
types.ts) added to the web coverage exclude list, matching the existing
type-only exclusions.
Tests: ~50 test files extended/created across core (auth, ema, mcp,
remote, state, storage, react), web components/screens/server, and the
cli/tui clients. Highlights: full EMA OAuth flow + transport coverage,
inspectorClient/remote server backfill, every Mantine control's handler
branches, and the cli unsupported-method / argv-fallback paths.
Genuinely-unreachable branches are annotated at the source with justified
`/* v8 ignore -- <reason> */` comments rather than lowering the gate:
happy-dom-inherent paths (Mantine portal, useMediaQuery, typeof window
SSR guards), React StrictMode effect replay, and provably-dead defensive
guards (?? fallbacks the types make impossible, Select.onChange values
outside the allowed list). No runtime logic changed; the only structural
edits are two behavior-preserving refactors (ResourceTemplatePanel hoists
the field value to a local so the ignore applies in JSX; ToolsScreen
swaps a ternary for an if) and prettier line-wrapping.
Also fixed a flaky real-timer debounce assertion in PromptArgumentsForm
(asserted an exact sibling-typing interleaving that raced the 300ms
debounce; now asserts the load-bearing fact — a non-empty fire-time
sibling capture).
AGENTS.md updated to describe the uniform 90 gate and the v8-ignore
policy. All four client coverage gates, root validate, storybook (370),
and smoke pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(web): make connect-crash SSE reader stop on the terminal event
`readSseEvents` looped until the server closed the stream (`done`). The
server closes the events stream immediately after writing the terminal
`transport_error`, but under the heavy parallel scheduler pressure of a
full `coverage` run (v8 instrumentation across all workers) that close
can lag far enough behind the event to trip the 30s test timeout — a
hang, not a wrong result. This made the test intermittently fail under
`npm run coverage` (it passed in isolation and under CI's lighter
`test:integration`).
Stop reading as soon as the terminal `transport_error` event arrives
(cancelling the reader) instead of waiting only for `done`. That event is
always the last one the session queue emits, so nothing is lost, and the
test no longer depends on stream-close timing. The `done` break is kept
for any stream that closes first.
Verified green across two consecutive full `npm run test:coverage` runs
(3094 tests) where it previously hung, plus isolation (~0.5s).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds enterprise-managed authorization (EMA): browser-side IdP login + RFC 8693 token-exchange and JWT-bearer resource-token mint, per-server EMA opt-in in Server Settings, IdP config in Client Settings, and IdP clientSecret keychain storage. Includes the v2/main merge and review follow-ups (400 on malformed client config; normalizeIdpIssuer import consolidation).
* fix(storage): await store-file writes instead of polling (#1412)
OAuth persistence is Zustand `persist` → file-storage adapter →
writeStoreFile, which the persist middleware invokes fire-and-forget:
saveTokens updates the in-memory store synchronously while the file
write lags. Tests verified persistence by polling the state file with a
2s/50ms `waitForStateFile()` budget, which flaked under full-suite load
(the file was written, the poll just didn't observe it in time).
Make the write completion awaitable instead:
- Track in-flight writeStoreFile() promises per resolved path in
store-io and add flushStoreFileWrites(filePath?) to await them (also
useful for graceful shutdown). Writes to the same path are chained so a
flush awaits every queued write.
- Convert the OAuth E2E, node storage, and file-storage-adapter tests
from poll loops (waitForStateFile / vi.waitFor on existsSync /
setTimeout) to `await flushStoreFileWrites(path)` + a single read.
- Remove the now-unused waitForStateFile() test helper.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore(storage): doc load-bearing sync registration; rename stale debug env
Address @claude review on #1413:
- Document that writeStoreFile()'s synchronous pendingWrites registration is
load-bearing for flushStoreFileWrites(), in both the storage adapter's
setItem callsite and the pendingWrites docblock, so future refactors don't
introduce an await before registration and silently re-flake the tests.
- Rename the stale DEBUG_WAIT_FOR_STATE_FILE test env var to DEBUG_PERSIST_PATH
(waitForStateFile was removed in this PR).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(servers): move oauthClientSecret and stdio env values to OS keychain (#1356)
mcp.json is designed to be tool-shareable (symlinked from Claude Desktop's
config, pasted into bug reports, synced via dotfiles), so storing OAuth
client secrets and stdio env values in plaintext there meant any of those
flows could leak them. Lift both into the OS keychain via @napi-rs/keyring
(active replacement for the archived keytar). The wire shape is unchanged:
the GET /api/servers handler rehydrates from the keychain so browser code
sees the same JSON; the on-disk file no longer contains the secret
material. Includes an idempotent migration that lifts plaintext from older
mcp.json files (or hand-edited ones) into the keychain on first read.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(servers): tolerate keychain unavailability on non-set ops (#1356)
CI on Linux failed because `@napi-rs/keyring` hard-errors without
libsecret, and the route handlers were touching the keychain even for
no-secret flows (defensive `deleteAllForServer` on POST/DELETE, sweep
on PUT). The user's intent for the Linux fallback was "hard fail when a
secret is actually involved" — not on every keychain touch.
Make `KeyringSecretStore.get / delete / deleteAllForServer` silently
degrade when the keychain is unavailable (return null / no-op); `set`
remains the one operation that hard-fails with
`KeychainUnavailableError`. The migration path catches the error and
preserves the disk plaintext rather than losing the secret.
Also inject `InMemorySecretStore` in `useServers.test.tsx` and
`servers-events.test.ts` so tests never touch a developer's real
keychain even when libsecret is present.
Adds `KeyringSecretStore` tests (vi.mock'd native bindings) and an
integration suite that simulates Linux-without-libsecret to verify the
tolerance contract end-to-end.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(servers): close GET migration race; reverse keychain/disk order (#1356)
Addresses code review:
1. **Concurrency**: GET /api/servers was reading + migrating outside the
write lock, then writing inside it — a concurrent POST/PUT/DELETE
between the read and the lock acquisition could be clobbered by the
migration write. Also affected the first-launch seed write. Fix:
take a fast unlocked peek; if the file is absent (seed branch) or
carries plaintext to migrate (migration branch), re-read inside the
write lock and decide there. Added `hasPlaintextSecrets()` as a
pure predicate so the common no-migration path stays lock-free.
2. **Write ordering**: POST and PUT wrote disk first, then keychain.
A keychain `set` failure mid-write left a half-configured entry on
disk and trapped retries at 409. Reversed to keychain-first so a
503 leaves both stores in their pre-write state. PUT in-place now
sets new fields, writes disk, then deletes obsolete fields (a
failed disk write leaves orphan keychain entries — recoverable —
rather than premature deletions of fields the user still expects
to see).
3. **Tolerance simplification**: dropped the no-entry regex in
`KeyringSecretStore.delete` — both "no entry" and "keychain
unavailable" collapse to the same desired outcome (the entry isn't
there anymore), so a uniform swallow is clearer.
4. **Parallelism**: `readKeychainEntriesFor` now `Promise.all`s the
per-field `get` calls. macOS Keychain round-trips are 10-50ms;
serializing 5+ env vars per server × N servers is a meaningful
wall-clock cost on GET.
5. **Spec doc**: reflects the tolerance contract (only `set`
hard-fails) and the keychain-before-disk write ordering rationale.
New regression test in servers-route.test.ts covers the retry-after-503
contract directly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(servers): parallel keychain writes + migration partial-state comment (#1356)
Two nice-to-have observations from the re-review:
- `writeKeychainEntriesFor` now `Promise.all`s its per-field sets,
matching the read side. No ordering requirement among distinct
(id, field) keys; on unavailability every set rejects and Promise.all
surfaces the first rejection (still 503 from the route handler).
- `migratePlaintextSecrets` catch block now spells out the
partial-migration semantics: we return the original config so the
disk plaintext stays intact, and the next successful GET retries
with the keychain-wins branch absorbing already-set entries.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(servers): parallel keychain deletes for symmetry (#1356)
Picks up the symmetry observation from the third review: `deleteKeychainFields`
now uses `Promise.all` like its read and write siblings. Distinct (id, field)
deletes have no ordering requirement, and `secretStore.delete` is already a
silent no-op on unavailability so Promise.all has no failure-mode surprise.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test: switch integration project to folder-based manifest (#1314)
Replace the `integrationTests` enumeration in vite.config.ts with a
folder glob (`src/test/integration/**/*.test.ts`). Files under that
folder run in the integration vitest project (node env, 30s timeouts);
everything else runs in the unit project (happy-dom).
This removes the silent misclassification trap from #1307: a new
integration-style test no longer falls into the unit project (and
happy-dom) just because someone forgot to add it to the array.
The 13 ported v1.5 tests + their helpers move under src/test/integration/
in a strict mirror of core/ source layout (mcp/, auth/, auth/node/,
storage/). Renames use git mv so history is preserved.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test: address PR review on #1318 (glob extensions + AGENTS doc)
- vite.config.ts: widen integrationGlob from `*.test.ts` to
`*.test.{ts,tsx}` so the integration folder fully owns its contents.
Without this, a `.test.tsx` placed under src/test/integration/ would
match the unit project's `{ts,tsx}` include, fail the unit `.ts`-only
exclude, and silently land under happy-dom — the exact misclassification
trap this PR set out to close.
- AGENTS.md: update the line describing where integration tests live;
it still pointed at the old `src/test/core/` location and the removed
`integrationTests` enumeration.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cliffhall
Cliff Hall
Bob Dickinson