Copilot review round 7 — two findings, both upheld.
**An in-progress migration snapshot could be adopted by another startup.**
The fast path counts every `*.migrating-*` as work and
`recoverOrphanedSnapshots` adopts them — including one whose owner is still
reading it. It link/unlinks the snapshot back to the live path and re-claims
it under a new name, the owner's hand-off then fails `ENOENT`, and if the
adopting process exits before copying, that healthy session starts with none
of those secrets, recoverable only on some later run.
Fixed the second way the reviewer suggested, not the first. Holding the main
lock across the hand-off also closes it and was tried — it broke "claims the
file atomically, so a later write is not deleted", because it blocks every
ordinary writer for the whole migration and fails their save past the retry
budget. That is a worse regression than the race, since #1950 guarantees a
write completing after the claim survives. The existing test caught it.
So the hand-off takes a lock on the **snapshot** instead, and
`recoverOrphanedSnapshots` skips an orphan whose lock is held. Liveness comes
from the lock rather than from the pid already in the filename: a pid both
outlives its process and recurs — pid 1 on every container start, which
#1950 documents as the reason the name carries a nonce — whereas a lock
expires by itself if its owner dies, so a genuinely abandoned snapshot
becomes adoptable with nothing to clean up. `isFileLockHeld` answers `false`
when it cannot tell, since the alternative to a wrong `false` is never
recovering an abandoned file.
**A compromised lock arrives as `ERELEASED`, and we told the operator to
delete the lock.** `setLockAsCompromised` marks the lock released and drops
its registry entry *before* calling `onCompromised`, so `release()` answers
`ERELEASED` without touching the filesystem — and the directory at that path
is by then the **winner's live lock**. "Remove it by hand" would destroy the
exclusion of a process that did nothing wrong. Handled separately and
silently; `onCompromised` has already reported what happened. A conditional
rather than an early return, since a `return` inside `finally` discards
whatever the body was returning or throwing.
Both new tests verified by mutation.
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 6 — five findings (two inline, three suppressed), all
valid.
**A stale lock that cannot be cleared caused unlocked writes.** `acquireLock`
does not only *create* directories: on finding a stale one it removes it and
retries, and that removal can fail — `ENOTEMPTY` for a lock with anything
inside it, `EACCES`/`EROFS` for one we may not touch. Those surface as
ordinary non-`ELOCKED` errors, which the probe read as "locks do not work
here" and degraded on. So every Inspector on the box quietly bypassed the
*same* stuck lock and raced its writes — while the release-failure message
was telling the operator saves would keep failing until they cleared it.
`isStuckOrHeld` now discriminates on **whether the lock directory exists**
rather than on an errno taxonomy: if it is there, something holds it and we
refuse; if it is not, we genuinely could not create one and degrading is the
documented trade (#1848, #1905). Reading the state the decision is about
avoids enumerating error codes per platform — and an enumeration is exactly
what let `ENOTEMPTY` through.
**Identity was captured after the acquire promise settled.** That span
includes the library's own `utimes`/`stat` probe, so a waiter replacing our
directory inside it would have us record *the winner's* identity as our own,
after which the removal guard would accept and delete their lock. It is now
recorded in the `mkdir` callback — the moment the directory becomes ours, and
synchronous with respect to this process, so there is no window.
**The expiry advice was still wrong for `EACCES`/`EPERM`/`EROFS`.** Since the
guard `stat`s before removing, everything reaching the release catch is an
`rmdir` that was refused — and stale takeover reclaims through that same
`rmdir`. The `ENOENT` arm was unreachable for the same reason, so the branch
was dead code: removed, and the advice is now unconditional.
**`proper-lockfile` was missing from `NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE`.** A
fourth externalization surface — the tsup lists configure the production
bundles, not `vite dev`, whose dep scanner would otherwise walk its
CJS/`graceful-fs`/`signal-exit` graph. Added, and `vite-base-config.test.ts`
now asserts it along with `chokidar` and `@napi-rs/keyring`, which were also
absent from that assertion.
**A test was named for something it did not exercise.** "does not treat an
unlistable directory as an empty one" was driving `ENOTDIR`, which is
deliberately treated *as* the fresh-install case, so the actual new behaviour
was untested. Split in two: one for `ENOTDIR`, and a real `EACCES` one using
a `--x` directory — `readdir` denied, a known name still reachable, which is
the asymmetry that makes "unlistable" different from "empty". Driven with a
real mode rather than a stub, since `vi.spyOn` cannot redefine an ESM
namespace export; skipped as root and on Windows, where the mode would not
bite and the test would assert nothing.
Both new tests verified by mutation.
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 5. Both comments upheld; the first is a real hole in
round 4's own fix.
**The ownership guard was in the wrong place, and overclaimed.**
Round 4 checked ownership and then called `release()`. Two problems:
- Check-then-act. The `stat` and the `rmdir` were separate async steps, so a
waiter could replace the directory in between and we deleted the winner's
fresh lock anyway — the very thing the check was added to prevent.
- Skipping `release()` on a mismatch left proper-lockfile's record registered
in its `locks` map, and its `signal-exit` handler `rmdirSync`s every
registered lock with **no ownership check of its own**. An exit during that
window deleted the winner too.
The guard now lives in `options.fs`, which is the single seam *both* removal
paths route through — `removeLock` on release, and the exit handler. It
refuses to remove a directory whose inode and birth time are not the ones
recorded at acquire, and reports the refusal as success so the library's
bookkeeping forgets the lock either way (leaving it registered is what hands
the exit handler a record pointing at the winner's directory).
The check is a `statSync` immediately followed by an `rmdirSync`, with no
`await` between them, so nothing in this process can interleave. **It still
does not close the race** — it is check-then-act across processes, which needs
the compare-and-swap Node does not expose — and saying it "closes the
destructive half outright" was wrong. file-lock.ts, README.md,
specification/v2_servers_file.md and AGENTS.md now all say it narrows the
window rather than closing it. AGENTS.md carries the negative instruction
alongside round 4's, since this is the third round spent on an overclaim.
**"It expires on its own after 10s" is false for the ENOTEMPTY case** the new
test exercises. Stale takeover reclaims through the same `rmdir`, which also
cannot remove a non-empty directory — so nothing clears it, and every later
save fails ELOCKED against it. `releaseAdvice` now branches: by-hand removal
for ENOTEMPTY, expiry for everything else.
Tests: a real child process demonstrates the exit handler removing a
replacement lock (with an **empty** directory — a non-empty one makes
`rmdirSync` fail ENOTEMPTY, which is how the first draft of this test passed
for the wrong reason). The release-failure test now asserts the message says
"by hand" and does not promise expiry. The shim's `rmdirSync` arm carries a
justified `v8 ignore`: it is reachable only from a real process exit, and its
logic is `removeIfMine`, covered through the async arm.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4 — four comments, all the same finding against the four
files carrying the claim, and it was more than a wording problem.
The comment said a holder whose lock was replaced is told. Checked against
proper-lockfile@4.1.2, that is false in the case that actually happens:
- `updateLock` compares mtime only on its refresh tick — `stale / 2`, so 5s
here — while an ordinary mutation is a read, an scrypt derivation and an
atomic write, comfortably under a second. The tick never runs, so nothing
fires.
- `release` → `unlock` → `removeLock` is an unconditional `rmdir` with **no
ownership check**. So the compromised holder goes on to delete the
*winner's* lock on the way out, ending the winner's exclusion too: one
compromised writer silently becomes two unprotected ones.
The second half is a correctness bug, not a documentation defect, and it is
fixable here even though the takeover race itself is not.
`withSecretFileLock` now records the lock directory's inode and birth time at
acquire and re-checks them before releasing. On a mismatch it **declines to
release** — leaving the new holder's lock alone costs nothing, ours is
already gone — and warns.
Inode and birth time rather than mtime, deliberately: both survive the
library's own `utimes` refresh, so a lock held past one tick is not accused of
being compromised, and both change on a delete-and-recreate, which is exactly
the event to detect. Where a filesystem reports neither, the two reads agree
and the check concludes "ours", degrading to the library's unaided behaviour
rather than to a false alarm. An unreadable baseline at acquire likewise
answers "ours" — with nothing to compare against, accusing a healthy lock
would be worse than releasing it.
Detection is therefore **best-effort**, and file-lock.ts, README.md,
specification/v2_servers_file.md and AGENTS.md now all say so rather than
promising the loser is told. AGENTS.md carries the negative instruction too,
since this is the second round spent on the same overclaim.
Tests: "does not delete the winner's lock after being taken over" replaces the
lock inside the body and asserts the replacement survives with the *same*
inode and birth time — it was left alone, not deleted and recreated. It waits
for nothing, being the fast case the library's tick misses. Verified by
mutation: forcing `stillOurs` to always answer true fails it. A second test
covers a release that fails while the lock genuinely *is* ours (a stray file
makes `rmdir` fail ENOTEMPTY), so the ownership check cannot swallow a real
release failure.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3.
- **`absorbFileSecretsIntoKeyring` must never throw, and round 2 broke that.**
Making `withSecretFileLock` reject on a held lock was right for `set` — a
user is waiting on that value — and wrong here: this is awaited directly by
both `resolveSecretStore` branches, so a stuck writer elsewhere on the box
would have failed store resolution and with it the whole session. The lock
failure is now caught, warned, and the file left for the next run. Covered
by a test that holds the lock from another handle and asserts the hand-off
resolves, the file survives, and the lock-specific warning is emitted (it
asserts that message rather than the shared "left in place" tail, which the
pre-existing claim-failure path also prints and would have let it pass
without the lock ever being reached).
- **The refusal message overstated how long it waited.** `retries *
maxTimeout` reads 20s for a schedule that sums to 15.26s — the early
attempts are the exponential ramp, not the cap. Replaced with
`RETRY_BUDGET_MS`, computed from the schedule rather than written down, so
it cannot drift from the thing it describes. `retry` applies no jitter by
default, so it is exact rather than an estimate.
- **The PR description still described the pre-round-2 design** — the
single-winner claim and the ~3s backoff. Rewritten to match the code.
Coverage: the round-3 changes cost `secret-store-selection.ts` two branches
and it fell to 89.62%. Recovered honestly rather than by annotation — the
`ENOTDIR` arm of `anythingToMigrate` now has a real test (a path whose parent
is a file, the other shape of "nothing there"), and the new catch types its
parameter `Error` instead of re-narrowing, since `withSecretFileLock` rejects
only with the `SecretStoreUnavailableError` it constructs one call away.
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, both comments upheld.
**ELOCKED must not degrade.** Exhausting the retries on a held lock means the
lock is *working* and something else demonstrably holds it — so running the
mutation anyway entered the exact interleaving the lock exists to prevent,
and entered it knowing a concurrent writer was there. `set` now fails
instead. Two supporting changes:
- The retry budget went from ~3.3s to ~15s so it outlasts the 10s stale
window. Refusing is only defensible because a *crashed* holder resolves by
takeover first; with the shorter budget, one Inspector killed mid-save
would have failed every later save on the box until someone deleted the
lock by hand. Pinned by a new test that stages a dead holder's lock
directly (mkdir + backdated mtime) and asserts the save succeeds.
- Acquire is now probe-then-wait. `proper-lockfile` drives its whole acquire
through `retry`, which re-attempts on *any* error — so with a 20-retry
budget a read-only `$HOME` would have spent the full 15s re-issuing an
identical failing `mkdir` on every save before degrading. One retry-less
probe separates "held" (worth waiting out) from "unavailable" (not).
**The comment overclaimed what proper-lockfile provides, and the reviewer was
right about the mechanism.** Verified against lib/lockfile.js@4.1.2: on
EEXIST it stats, and if stale it rmdirs and re-mkdirs *without* checking the
directory it removed is the one it found stale — so a slow waiter can still
delete a fast waiter's fresh lock and both proceed. That is the same race the
hand-rolled version could not close, and it is not closable without
renameat2(RENAME_EXCHANGE).
What the library actually adds is that the loser is *detected*: its refresh
tick compares the lock's mtime against the value recorded at acquire, so a
compromised holder is told rather than proceeding silently. And what is
genuinely exclusive is the case that matters — mkdir is atomic and a live
holder refreshes its mtime, so its lock never goes stale and two running
Inspectors are serialized. The residual window opens only after a holder dies
without releasing, and the optimistic verify still covers it.
file-lock.ts, README.md, specification/v2_servers_file.md and AGENTS.md all
carried the overclaim; all four now state the narrower, true version.
Also fixes a test that was quietly asserting the opposite of its name: its
holder used stale: 60_000 while the waiter used 10_000, and staleness is
judged by the *waiter's* threshold — so the holder was declared stale after
10s and the save succeeded by takeover.
Not done: a two-process stale-takeover race test. It would be testing the
library rather than this code, and is inherently nondeterministic; the honest
response to that finding was to stop claiming the race is closed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
- Create the storage directory before locking. `writeStoreFile` creates it,
but from inside the locked section — so on a fresh install, where
`~/.mcp-inspector` does not exist, proper-lockfile failed ENOENT and every
*first* save degraded to an unlocked write with a warning. That is the save
most likely to be racing another (two Inspectors started together both
reach it), so the lock was absent from precisely the interleaving it exists
to close.
- `anythingToMigrate` no longer reads every `readdir` failure as "nothing to
migrate". Only ENOENT/ENOTDIR proves the fresh-install case; a directory
that denies listing can still permit access to the known `secrets.json`,
and returning false there selected the keychain and left those secrets
invisible with nothing said. Other errors fall through to the under-lock
checks, which is what happened before the fast path existed.
- Pin `proper-lockfile` to the repo-root install in `vitest.shared.mts`,
beside express and yaml: it is reached only through root-owned `core/`.
Resolution already finds the root copy since no client declares it; the pin
is what stops that depending on it never arriving as some client's
transitive dependency, which would give a test two copies of a module whose
whole job is one registry of held locks.
- Make the cross-process test prove `mutate` takes the lock. The previous one
asserted only that both secrets survived, which the optimistic verify
delivers with the lock removed entirely. It now asserts the parent's `set`
has *not* settled while a child holds the lock — an observation only a real
lock produces. Verified by removing `withSecretFileLock` from `mutate`:
three tests fail, where none did before.
The degrade test's "lock cannot be created" case is now a path whose parent
is a *file* rather than a missing directory, since a missing directory is no
longer that case. It also fails identically for root, so it cannot pass
locally and flake in a container.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
#1950 shipped FileSecretStore without cross-process mutual exclusion, by
decision: an earlier revision hand-rolled a `mkdir` election with an owner
stamp, a heartbeat and a stale-takeover, and three consecutive review rounds
found a real race in it. The last one is not closable with what Node exposes
— claiming a stale lock atomically needs compare-and-swap on a directory
entry (`renameat2(RENAME_EXCHANGE)`) — so it was replaced with optimistic
verify-and-retry and the residual documented.
#2082 settles that as "borrow, don't hand-roll". `proper-lockfile` is what
npm itself locks with, and stale-takeover is precisely the problem it has
already solved: it re-stats the lock directory after claiming it and gives
the lock up when the mtime is not the one it wrote, so the loser of a
takeover race releases instead of proceeding.
- core/auth/node/file-lock.ts: `withSecretFileLock`. `realpath: false` so a
file can be locked into existence (the very first `set` has no
`secrets.json`, and the library's default resolves through `fs.realpath`);
a warning in place of the library's `onCompromised`, which throws from a
timer and would take the session down; and a degrade-with-one-warning path
rather than a throw when no lock can be taken — this store exists for boxes
missing the usual mechanism (#1848, #1905) and must not gain a new way to
be unavailable.
- FileSecretStore.mutate holds it across the whole read-modify-write. The
optimistic verify stays underneath and is not redundant: a lock is advisory
between the processes that take it, so the verify covers a writer outside
this codebase and covers the degrade path. The in-process queue stays too,
and gains a second job — proper-lockfile is not reentrant, so serializing
per path keeps ELOCKED meaning "another process".
- absorbFileSecretsIntoKeyring takes the same lock around orphan adoption and
the atomic claim, behind a lock-free `readdir` fast path so the common
startup (keychain available, no file ever written) neither creates a lock
directory nor warns about one it could not create.
proper-lockfile is a root `dependency` per the placement rule, and is named
in all three bundler `external` lists: tsup externalizes what the *client's*
manifest declares, so a root-only CJS package was being inlined into the ESM
bundles, leaving esbuild's `Dynamic require of "path" is not supported` shim
that killed `--cli` at import time. That rule was undocumented; it is now in
AGENTS.md beside the placement rule that creates it, and mirrored into
.github/copilot-instructions.md.
Tests: 7 new in file-lock.test.ts driving a real second process (the existing
suite structurally cannot — `serialize` orders in-process callers before the
lock sees them), plus two for the migration fast path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 23 produced **no inline comments** — nothing wrong in the code it had
not already seen. The five suppressed ones are all in unchanged code: one real
misdiagnosis and four pieces of drift.
**A decrypted-but-corrupt payload was reported as a wrong passphrase.** By the
time `JSON.parse` runs, GCM has verified the tag — so the passphrase is
*proven correct*, and a `SecretFileKeyMismatchError` there tells the user to
restore a secret they never lost. `asSecretMap` was already outside that catch
for exactly this reason; `JSON.parse` was not, so the same class of mistake
survived one line above the fix for it. The decrypt now has its own narrow
catch and everything after it reports a corrupt file.
The test fixture could not express this case — it `JSON.stringify`s its
payload, so it can only produce valid JSON. Split into a raw-plaintext variant
so a fixture can carry bytes that are authentic under the passphrase and still
unparseable, which is the only shape that reaches the branch.
The rest is documentation that had drifted from the code it describes:
- `handOff`'s doc still described the re-read-and-compare protocol that the
atomic claim replaced two rounds ago — an obsolete concurrency invariant is
worse than none, because the next reader takes it as the contract.
- The spec still described that same hand-off, and still called Client and
Server Settings "the only two dialogs that accept a secret". Both now match
the code, and the dialog count is called out as load-bearing: the one that
was missing from it was the one taking secrets with no disclosure at all.
- The `void load()` in `refresh` carries the justification the repo's
no-floating-promises rule requires — `load` owns its failures, and `refresh`
is deliberately synchronous because `refreshingPersist` calls it from a
`finally`.
Full `npm run ci` green, staged tree verified first.
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 22. Three findings, all in code I wrote, and the first two are both
cases of a stated rationale being wrong rather than a line being wrong.
**`refreshingPersist` skipped the refresh on failure, and I had argued for
that explicitly.** The doc and its test both said "a persist that threw did
not write, so there is nothing new to describe". That is false for this write
order: both persistence paths write the secret store *before* the file, so a
rejected disk write can follow a `set` that has already upgraded
`secrets.json` from plaintext to encrypted. The wrapper then left the footer
describing a file that no longer exists in that form — the stale-descriptor
bug it exists to prevent, reached through its own error path.
It refreshes in a `finally` now. The asymmetry is what settles it: refreshing
after a failure that changed nothing costs one idempotent GET, while not
refreshing after a failure that changed something leaves a security statement
wrong until reload. The test asserting the old behavior is inverted, with the
reasoning recorded so it is not "corrected" back.
**The KDF parameters were validated for well-formedness, not for cost.**
scrypt's work is proportional to `N * r * p`, and all three come off disk — so
a file declaring a modest `N` with `p: 1_000_000` burns unbounded CPU on every
read *and* on merely describing the store for `/api/config`, which is a page
load. Anyone who can write that file can already read its secrets, so this is
not a privilege boundary; it is a footgun and a hang, and the fix is free
because this build writes exactly the constants it now enforces as maxima.
**Two secret-store mutations still bypassed the refresh.** The pagination
toggle resends the server's rehydrated secrets, and deletion sweeps them —
both are writes, and a write is what performs the pending upgrade. Routed
through the same wrapper as the rest.
Full `npm run ci` green, staged tree verified first.
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>
Round 19 found real data loss rather than process noise, in two places.
**An unrelated settings edit destroyed the secret the durability gate had
just preserved.** The GET migration withholds its strip for a session-scoped
store, so `mcp.json` stays the durable copy — but the POST/PUT paths always
wrote the stripped shape. The GET also returns the rehydrated secret to the
settings form, which resends the whole object when the user changes anything,
so editing a timeout wrote the stripped entry, moved the only durable copy
into RAM, and lost it at exit. Every operation reported success. The client
config had the identical shape: the read-path guard was there, the write path
had none, and saving a CIMD URL did the same thing to the IdP client secret.
Both write paths now agree with their read paths: while the store cannot
outlive the process, the file stays the durable copy.
**The hand-off's delete is no longer a check-to-delete race.** Re-reading
immediately before `rm` caught writes that had already landed and was blind to
the one arriving between the check and the delete — destroying a *later*,
already-successful write, which is worse than the optimistic-write residual
and, unlike it, fixable without a lock. The file is claimed by an atomic
rename before anything reads it, so the migration operates on a snapshot
nobody else can reach and a writer that recreates `secrets.json` is simply
untouched.
Claiming introduced a failure mode the existing tests caught immediately: a
migration that aborts must not leave the secrets stranded at the staging path.
The snapshot is restored on any incomplete outcome, and when the live path has
been recreated in the meantime it is left in place with its location logged —
an operator-visible orphan is recoverable; clobbering someone's write is not.
Also:
- **`setMany`**, the write-side twin of `getMany`. `set` costs a whole-file
read-decrypt-encrypt-write-verify cycle per call, and the settings form
resends a server's full `env` map on any edit, so a 10-variable stdio server
spent ~30 scrypt derivations saving an unrelated change. One cycle now; the
keychain keeps its parallel individual writes.
- **A doc that contradicted its function.** `readOnDiskEncryption` still said
it "never decrypts, so it needs no passphrase" — round 12 made it
authenticate precisely because structure is not access.
Full `npm run ci` green, staged tree verified before running it.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three findings. The first is the fourth consecutive one in the refresh path,
so it is answered by making that path testable rather than by patching it
again.
**The refresh-after-persist wiring was covered by nothing.** It lived as two
inline `await …; refresh()` pairs in `App.tsx`, which is deliberately outside
the coverage `include` — so the hook's tests and the modals' tests could both
pass while the callback joining them was broken, leaving the security footer
stale after exactly the write that changed it. Three of the last four rounds
found defects in this path; none of them could have been caught by a test,
because there was nowhere for one to live.
Extracted to `utils/refreshingPersist`, which is inside the gate and has its
own tests: refresh runs after the persist resolves (not before — refreshing
first re-reads the descriptor the pending write is about to invalidate),
arguments pass through unchanged (the server-settings caller is
`(id, settings)`, and a transparent wrapper is the difference between saving
the entry and dropping it), and a failed persist refreshes nothing while
propagating the error.
**A tmpfs-style false promise, in the path fallback.** With neither `HOME`
nor `USERPROFILE` set — an ordinary service environment —
`defaultSecretFilePath` returned `.mcp-inspector/secrets.json`, a *relative*
path, while `FileSecretStoreOptions.filePath` and the `SecretStorageInfo.path`
the footer offers to copy both promise an absolute one. Resolved like the two
override branches above it.
**A warning that was usually false.** The session-store notice fired on every
`/api/servers` read whenever the store was in-memory — including for the
default empty catalog, where "plaintext values are left on disk" is not true
of anything, repeated on every list refresh. It is now emitted lazily, at most
once per server instance, and only when the loop actually preserves a
non-empty secrets set. A log line that is usually untrue is one people learn
to skip, which costs it the occasion it matters.
The remaining suppressed comment is the hand-off's check-to-delete race,
tracked in #2082.
Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three findings. The first is the third consecutive round in which my previous
fix introduced the next defect, all in the same ~15 lines of `useInitialConfig`.
**The request token did not subsume the unmount guard, and I said it did.**
Bumping the generation on effect teardown invalidates loads already *in
flight*; a `refresh()` called after unmount claims the next token, is
therefore current, and commits into a dead hook. That is a live path — the
callers fire `refresh()` from an async settings persist that can outlive
`App`. Mounted state is tracked separately again and `stale()` checks both.
The test for this was also wrong, in the way that matters: it called
`refresh()` after unmount but never settled the fetch, so it could not observe
the commit it existed to prevent. It settles the promise now.
**A tmpfs mount was reported as durable.** `docker run --tmpfs
/home/node/.mcp-inspector` puts a real entry in mountinfo, so a mount-table
lookup answered "durable", selected the file store, and published
`durable: true` for a file that disappears when the container stops — the
exact false promise the in-memory fallback exists to avoid, reached by
trusting the mount table over the filesystem type. `tmpfs`/`ramfs` are
excluded now, read from the field after the ` - ` separator rather than by
position, since the optional fields before it are variable in number.
**A malformed envelope was diagnosed as a changed passphrase.** A truncated
payload, a wrong-length IV or tag, or an out-of-range KDF parameter all threw
`SecretFileKeyMismatchError`, whose 503 tells the user to restore a passphrase
that cannot repair a structurally broken file. The read path now runs the same
`encryptedEnvelopeProblem` check the descriptor path already ran, and reserves
the key-mismatch error for an envelope that is well-formed and fails
authentication — which is the case where that advice is right. Two existing
tests asserted the old behavior and encoded the bug; both updated, and a new
one pins that a well-formed envelope with the wrong key still gets the
passphrase advice.
The remaining suppressed comment is the hand-off's check-to-delete race,
tracked in #2082.
Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
One inline and three of the four suppressed. Two of the four are cases where
an earlier fix of mine was applied to one branch and not its twin.
**The footer went stale after the write that changed it.** `useInitialConfig`
fetched once, and `secretStorage` is the one field it carries that describes
state *this app mutates*: the first save under a newly-set passphrase
re-encrypts a pre-existing plaintext file. The backend re-derives the
descriptor per request — deliberately, and there is a test for that — but the
client had no way to ask again, so the footer said "Plaintext file" for the
rest of the session about a file that no longer was. Exactly the misreport
this band exists to prevent, arrived at from the client side.
The hook now exposes `refresh()`, and both persist paths that can carry a
secret call it: server settings (OAuth client secret, stdio `env:` values)
and client settings (the enterprise IdP client secret). The refresh tracks
its own liveness through a ref, since unlike the mount fetch it has no effect
teardown to cancel against.
**The plaintext branch skipped the payload validation the encrypted branch
does.** Round 12 tightened the encrypted envelope check and left this one
behind, so `{ "encryption": "none", "secrets": [] }` — trivial to hand-write —
described a healthy plaintext store whose next save `readMap` refuses. Both
branches validate now.
**`secretStoreGetStrict`'s doc contradicted the code it was written for.** It
said falling back to `get` is right "for the in-memory and file stores —
neither can fail in a way that masquerades as absence". `FileSecretStore.get`
does exactly that masquerade, which is why round 9 gave that store its own
`getStrict`. The fallback stays (the seam is optional and doubles must
compile) but the comment no longer claims it is equivalent for a store that
can fail.
**The spec documented the performance bug rather than the fix.** It still said
`getMany` decrypts once per server, which was the shape round 13 replaced —
and describing the earlier bug as the design is worse than saying nothing.
The remaining suppressed comment is the hand-off's TOCTOU delete: the
cross-process guarantee, decided against for this PR and tracked in #2082.
Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
No new inline comments this round, but two of the four suppressed ones were
real defects rather than restatements — both in code I added, both silently
wrong in the safe-looking direction.
**A symlinked secrets directory read as unmounted.** `isOnMountPoint` walks
to the nearest existing ancestor *lexically* and then string-prefixes it
against mountinfo, so `~/.mcp-inspector -> /data/inspector` with `/data`
mounted matched no mount point. The container then selected the session-only
store on a box that had explicitly arranged for persistence, and nothing said
so — the mount was there, the volume was there, the secrets evaporated on
exit. Resolved through symlinks now, tolerantly: a path that cannot be
resolved falls back to the lexical comparison, which is what this did before
and is correct whenever no symlink is involved. The `statSync` fallback branch
already followed links, so this also makes the two paths agree.
**The bulk read did not fix the case it was written for.** `getMany` batched
*one server's* fields, but both rehydration callers iterate servers — so a
catalog of 20 servers holding one secret each still paid 20 serialized scrypt
derivations. The same ~450ms stall the seam was introduced to remove, reached
one server at a time instead of one field at a time. Measuring the wrong unit
and then optimizing it is worse than not optimizing, because the number in the
commit message says it is fixed.
The seam is cross-server now — `getMany(requests: SecretBulkRequest[])` — and
both callers pass the whole catalog in one call. `FileSecretStore` decrypts
once for the entire rehydration; the keychain fallback still issues
independent parallel `get`s, which is what it wants. A test asserts one
derivation for four servers, next to the existing one asserting one for three
fields, so the property is pinned at both axes.
The other two suppressed comments are the cross-process guarantee — `mutate`'s
residual and the hand-off's TOCTOU delete — which remain decided against for
this PR and tracked in #2082.
Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three inline findings and one of the two suppressed. The other suppressed
one — that the hand-off's delete is still a TOCTOU — is the cross-process
guarantee, decided against for this PR and tracked as #2082.
**My convergence tests did not converge anything.** The reviewer caught that
the "two stores racing" tests cannot race: `serialize` keys its queue on the
resolved path *process-wide*, which I made it do in the previous commit
precisely so two instances would take turns. So the tests passed by
serialization while their names and comments claimed they were exercising
the optimistic retry, and the retry's successful path had no coverage at all.
A fix I was pleased with silently disabled the tests written to prove it
worked.
Split in two now, both honest about what they drive:
- `in-process mutations are serialized per path` keeps the multi-instance
cases, which pin a real property — it is what makes the in-process case
correct *without* relying on the retry.
- `cross-process convergence` injects one external write between the store's
write and its verifying read, via raw `fs` so it bypasses the queue the way
another process would. The surviving entry can only be explained by the
re-apply, so these pin the mechanism rather than the outcome. Covers a
set, a delete, and the encrypted path where every attempt re-derives.
**A well-formed envelope is not an openable one.** `readOnDiskEncryption`
classified by structure alone, so a wrong or unset `MCP_INSPECTOR_SECRET_KEY`
produced the quiet, healthy "Encrypted file" footer for a file whose every
`get` returns `null` and every `set` is refused — the exact state the
unreadable footer promises to warn about. It now authenticates: decrypt and
discard, costing one scrypt derivation (~23ms) per descriptor build, once per
`/api/config`. A test asserted the old behavior and has been rewritten; the
comment on it explains why "describable" has to mean *accurately* describable.
**The copy button's `aria-label` was eating the warning.** `aria-label`
replaces descendant text, so a screen-reader user heard "Copy secrets file
path …" and never heard "Plaintext file" — the one sentence that should
change their mind about typing a secret. The status now leads the accessible
name and the path follows, which also keeps the name a superset of the
visible text (WCAG 2.5.3).
**The spec still described the pre-#1950 invariant** in its migration
bullets: every plaintext secret lifted into the keychain and removed from
`mcp.json`, with only `KeychainUnavailableError` named. Both bullets now
describe the selected store, the durability gate that withholds the disk
delete for a session-scoped store, the strict read behind the store-wins
lookup, and `SecretStoreUnavailableError` as the family the handler catches.
Full `npm run ci` green, including the per-file ≥90 gate — the new validator
and retry branches needed six more cases to clear it.
Signed-off-by: cliffhall <cliff@futurescale.com>
The headline is a correction to my own claim rather than a new defect.
**The optimistic verify is weaker than the comment said it was.** It
described the residual as a crash between the write and the verifying read.
The reviewer pointed out that no crash is needed: order two writers as
write-A, verify-A, write-B, verify-B and both report success while A's entry
is gone — A's verify simply ran before there was anything to see, and B did
nothing wrong. A crash is one instance of that shape, not the whole of it.
The trace in the old comment quietly assumed A verifies *after* B writes,
which nothing guarantees.
Corrected in the doc comment, the README and the spec, and the honest
summary is now stated rather than implied: this converges in every
interleaving where the clobber lands before the verify, it is not mutual
exclusion, and a deployment that needs the guarantee wants an OS-backed lock
from a dedicated library — not another hand-rolled election. It remains
better than what it replaced, which lost updates across a wider set of
interleavings and could not be closed at all with what Node exposes.
The other findings:
- **An envelope naming the cipher still is not an openable one.** Round 9
added a `kdf`/`data` presence check; three dot-separated strings satisfied
it while an empty tag, a 4-byte IV or `N: 3` still made every decrypt
throw — so the quiet "Encrypted file" footer went to a file whose next
save was guaranteed to fail. Now validates part sizes, a non-empty
ciphertext, and the scrypt cost parameters (node rejects a non-power-of-two
`N` synchronously, out of the derivation rather than the decrypt). A
positive control asserts a file we actually wrote still reads as encrypted.
- **`encryptionUnknown` + `looseMode` claimed an exposure it had not
established.** With both set, `plaintext` is absent, so a `!== false` test
landed on "anyone who can read it can read the secrets in it" — about a
file that may well hold ciphertext we simply could not classify. Three-way
now, in `secretStorageCaveat` and mirrored in the footer's tooltip, with a
test on each.
- **The README described a lock that no longer exists**, which is the worst
kind of stale doc: it stated a durability guarantee the code had stopped
providing. Rewritten to describe the actual design, including the residual.
- **The private-method access in the concurrency test** now carries the
justification the repo's TypeScript rules require: the asserted shape is
`writeMap`'s real signature, so a rename breaks the line instead of
silently detaching the stub, and a production injection seam would let the
real write path drift from the tested one.
Also updates `specification/v2_servers_file.md`, which still described #1356's
world: one keychain-only store, and unavailability as a hard error. It now
covers store selection and the probe, the file format and lazy encryption
upgrade, the concurrency design and its residual, the `getStrict`/`getMany`
seams and the two traps `getStrict` hit, the durability gate on migration,
the keychain hand-off and its conditional delete, and how the active store is
surfaced.
Full `npm run ci` green. (One unrelated `AppRenderer` theme-flip test failed
once under full-suite parallelism and passes in isolation with and without
these changes — a flake, not a regression.)
Signed-off-by: cliffhall <cliff@futurescale.com>
Two changes, interleaved in the same files because both are about the
same read-modify-write path.
## The lock is gone (maintainer decision on the round-5/7/8 finding)
Three consecutive review rounds found a real race in the `mkdir` lock, and
the last one is not patchable with what Node exposes: claiming a stale lock
needs compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`).
Without it, `canSteal` inspects one lock but `rename` acts on whatever is at
that path *later*, so a waiter that lost the first race can move the
winner's fresh lock aside and enter alongside it.
Rather than patch it a fourth time, writers now collide and the loser
notices. `mutate` reads `M0`, applies the mutation to get `M1`, writes it,
then reads back `M2`; if they differ, someone wrote in between, so it
re-applies onto what they left and retries, bounded.
The comparison is over the **whole map**, and that is the load-bearing
detail. Checking only the entry we touched passes in exactly the case that
loses data — ours is present, and it is the *other* writer's that is gone.
With A and B from the same `M0`: A writes `MA`, B writes `MB` and clobbers
A; B verifies and correctly sees `MB`; A verifies, sees `M2 !== MA`,
re-applies onto `MB`, writes again. Both survive, and the writer that was
clobbered is the one that repairs it.
Two things this deliberately does not claim:
- It is **not mutual exclusion**. A process that crashes between its write
and its verifying read cannot notice it was clobbered, and nothing else
will. That window is strictly narrower than the lock's — which lost
updates with every participant alive — and needs no primitive Node lacks.
Stated in the code rather than left to be rediscovered in review.
- Non-convergence **fails loudly**. After five lost rounds `set` throws
rather than returning, because returning would reintroduce the silent
loss with extra steps. `delete` stays silent, per the interface contract.
`serialize` is now keyed on the resolved path **process-wide** rather than
per instance. Two `FileSecretStore`s on one file are ordinary — the resolved
store holds one and `absorbFileSecretsIntoKeyring` builds another — and a
per-instance queue does not order them against each other at all. The new
convergence test caught this on its first run: two instances racing `set`
lost an entry. In-process is now correct by construction; the verify covers
what it cannot see, a second Inspector process.
The hand-off's read → copy → delete was the lock's other user. Its delete is
now conditional: the file is re-read immediately before removal and compared
against what was copied, and it declines to delete when they differ. Leaving
the file costs nothing (keychain-wins makes the next run idempotent) while
deleting on a stale read costs a secret.
## Round 9 (two inline, two suppressed)
- **`KeyringSecretStore.getStrict` let raw binding errors escape.** Both
plaintext migrations keep their source file only when they catch
`SecretStoreUnavailableError`, so a transient keychain failure 500'd
`GET /api/servers` instead of abandoning the migration. Wrapped, as `set`
already does.
- **`FileSecretStore` had no strict read at all**, so `secretStoreGetStrict`
fell back to the tolerant `get` and mapped every read failure to `null` —
leaving the exact inversion the seam was introduced to prevent open on the
store it was introduced for.
- **`readOnDiskEncryption` reported "encrypted" for an envelope it cannot
open.** Naming the cipher is not the same as being openable: a file with no
`kdf`/`data`, or a `data` that is not `iv.tag.ciphertext`, produced the
quiet reassuring footer for a file whose next save was guaranteed to fail.
- **`/api/config` kept a stale descriptor when the resolver returned
`undefined`**, because the conditional spread left the startup value
standing. Assigned unconditionally now; `c.json` omits the key.
Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
The lock finding from this round is deliberately not addressed here — see
below.
- **My round-6 strict-read fix was inert in production.** `DeferredSecretStore`
is what `defaultSecretStore()` returns, and it did not forward `getStrict`
— so `secretStoreGetStrict` fell through to the tolerant `get` for every
real caller, and the strictness existed only in tests that injected a
concrete store. Forwarded now, along with `getMany`.
- **The same tolerant-read bug remained in both pre-existing migrations.**
I added `getStrict` for the new hand-off in round 6 and never applied it to
the mcp.json and client.json migrations, which have the identical
shape: decide from `get`, then write on `null`. A transient keychain read
failure therefore let an older plaintext value overwrite a newer keychain
one — and the disk copy was then stripped, so the newer value was gone. Both
use the strict read now; the throw is a `SecretStoreUnavailableError`, which
both call sites already handle as "abandon the migration, keep the file".
- **The permission check made a false statement on Windows.** `stat` returns
synthetic bits there, and interpreting them yields a confident falsehood
either way — "0666 — not owner-only" about a file the ACL may restrict, or
"owner-only" about one it does not. The ACL is the real answer and this does
not inspect it, so Windows now reports `unknown`.
- **Rehydration cost one full decryption per field.** `get` reads and decrypts
the entire file, and both rehydration paths ask field by field, serialized
behind the store's own queue. Measured on this machine, scrypt at N=16384 is
~23ms per derivation, so a 20-secret encrypted catalog spent **~450ms of
pure key derivation on every `GET /api/servers`** — a visible stall, not a
micro-optimization. Added an optional `getMany` seam: `FileSecretStore`
decrypts once per server, everything else falls back to the parallel
per-field `get` that already suited it. A test counts derivations directly:
1 for a three-field `getMany`, 3 for the three `get`s it replaced.
Also broadens a log message that still said "Keychain unavailable" for a
store that may not be a keychain.
Full `npm run ci` green — 5,762 tests under the coverage gate.
Signed-off-by: cliffhall <cliff@futurescale.com>
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