feat(auth): give the secrets file a real cross-process lock

.github/copilot-instructions.md

@@ -80,6 +80,7 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.ts`) custo
 - Dependencies reached only through **root-owned code with no manifest** (`test-servers/src`, `core/`) are declared at the root and aliased to the **repo root** in `vitest.shared.mts` — as `express` and `yaml` are — not to `<client>/node_modules` like the other pins there.
 - **A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed it in the *consumer's* tree, beside a React satisfying *its* peer range — looser than ours, which is all it takes to split React. `ink-form`/`ink-scroll-view` declare `">=18"`, so a consumer's React 18 satisfies them, the TUI ends up with two React instances, and it crashes on the first hook (#1952). Both are inlined via `noExternal` in `clients/tui/tsup.config.ts`. **`ink` is the one exemption, justified by cost (~1.4MB) — never by a peer range**: flag any claim that `">=19"` keeps npm from misplacing it, which is false and was in this repo once. What keeps it safe is the **root `react` range staying open to the whole major (`^19.0.0`)** so npm can dedupe with a consumer's pinned React 19; treat narrowing that range as reopening the bug. `clients/tui/__tests__/tsupConfig.test.ts` enforces the split, the root-declaration of exempt packages, and that range.
 - **Which section is a separate question from which manifest.** A package `core/` imports at runtime must be in root **`dependencies`**: client builds externalize npm packages, so a published install resolves them from the root manifest and devDependencies are absent there. Only test/build-only packages (`express`) belong in `devDependencies`. Flag a runtime `core/` import added to `devDependencies` — it passes every local check and breaks the published package.
+- **A root-declared package that `core/` imports at runtime must also be named in each client's bundler `external` list** — `clients/{cli,tui}/tsup.config.ts` and `clients/web/tsup.runner.config.ts`, all three. Bundlers externalize what the *client's* manifest declares, and these packages are root-only by rule, so omitting them means they get bundled. For a CJS package inlined into an ESM bundle that is fatal: esbuild's `Dynamic require of "path" is not supported` shim throws at import time and the binary dies before parsing a flag (#2082, `proper-lockfile`). Flag a new root runtime dependency that is not added to all three.
 
 ## Tests and the coverage gate
 

AGENTS.md

@@ -68,7 +68,28 @@ v2/main/
 │   │   │                               #   throws and the routes turn into a 503, and the keychain
 │   │   │                               #   probe), file-secret-store.ts (0600 JSON, AES-256-GCM when
 │   │   │                               #   MCP_INSPECTOR_SECRET_KEY is set — refuses to overwrite a
-│   │   │                               #   file it cannot decrypt rather than destroying it), and
+│   │   │                               #   file it cannot decrypt rather than destroying it),
+│   │   │                               #   file-lock.ts (withSecretFileLock: the cross-process
+│   │   │                               #     mutual exclusion #2082 settled on — proper-lockfile,
+│   │   │                               #     borrowed rather than hand-rolled. Read its header before
+│   │   │                               #     citing it: it makes two LIVE Inspectors exclusive, and
+│   │   │                               #     does NOT make stale takeover single-winner. proper-lockfile
+│   │   │                               #     detects a takeover only on its 5s refresh tick, which an
+│   │   │                               #     ordinary sub-second mutation never reaches, and its release
+│   │   │                               #     is an unconditional rmdir (as is its signal-exit handler) —
+│   │   │                               #     so withSecretFileLock passes a GUARDED options.fs, the one
+│   │   │                               #     seam both removal paths share, refusing to delete a lock
+│   │   │                               #     that is no longer ours (inode+birthtime). That NARROWS the
+│   │   │                               #     window, it does not close it — still check-then-act across
+│   │   │                               #     processes. BEST-EFFORT throughout: do not write that a
+│   │   │                               #     compromised holder is always told, or that the winner's
+│   │   │                               #     lock is always preserved.
+│   │   │                               #     DEGRADES when no lock CAN be taken (read-only $HOME etc),
+│   │   │                               #     since this store exists for boxes missing the usual
+│   │   │                               #     mechanism; but THROWS on ELOCKED — a lock held by a live
+│   │   │                               #     writer is evidence the lock works, not licence to bypass
+│   │   │                               #     it — after waiting past the stale window),
+│   │   │                               #   and
 │   │   │                               #   secret-store-selection.ts (the POLICY: explicit
 │   │   │                               #   MCP_INSPECTOR_SECRET_STORE wins, else probe the keychain,
 │   │   │                               #   else fall back LOUDLY — to memory in a container with
@@ -228,6 +249,8 @@ The same **placement** rule covers anything reached only through **root-owned co
 - A package only the tests, the test servers, or the build tooling need belongs in **`devDependencies`** — `express`, added there by #1970.
 - `yaml` is in `dependencies` today even though its only importer is `test-servers/src/load-config.ts`. Left as-is deliberately (moving it changes what ships, which is not a docs change); if you touch it, confirm no published path reads YAML first.
 
+**A root-declared package that `core/` imports at runtime must also be named in each client's bundler `external` list.** tsup and Vite externalize what the *client's* `package.json` declares, and a root-only dependency is in none of them — so it gets **bundled**, silently, and the placement rule above is what guarantees every such package is root-only. For a CJS package inlined into an ESM bundle that is fatal rather than merely wasteful: esbuild leaves a `Dynamic require of "path" is not supported` shim that throws at *import* time, so the binary dies before it parses a flag. `proper-lockfile` hit exactly that in #2082; `@napi-rs/keyring` is listed in all three for the same reason. The three lists are `clients/cli/tsup.config.ts`, `clients/tui/tsup.config.ts`, and `clients/web/tsup.runner.config.ts` — add a new package to **all** of them, since which client reaches it is a function of what `core/` imports, not of what the client's own code names.
+
 **A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed **it** in the consumer's tree, and npm places a package beside a React satisfying *that package's* peer range — looser than ours in every case here, which is all it takes to split React. `ink-form` and `ink-scroll-view` declare `">=18"`, satisfied by a consumer's React 18 while our React 19 nests underneath: the bundle renders through one React, those packages call hooks on another, and the TUI crashes on the first hook (#1952). Both are inlined by `clients/tui/tsup.config.ts` (`noExternal`) and declared only in `clients/tui/package.json`, where the build resolves them — declaring an inlined package at the root would just make consumers install a second, unused copy.
 
 **`ink` is the single exemption, and it is justified by cost, not by safety.** Bundling it works but adds ~1.4MB (`react-reconciler` + `yoga-layout`, plus a `createRequire` banner, since inlined CJS calls `require` at runtime and esbuild's ESM interop rejects that without a real `require` in scope). **Never justify an exemption by a peer range** — `ink` briefly carried "its `">=19"` peer keeps npm honest", which is false: a consumer pinning React 19.0 satisfies `">=19"` while a narrower range of ours nests underneath. What actually makes the exemption safe is a *different* lever: the **root `react` range stays open to the whole major (`^19.0.0`)**, so npm can dedupe our React with whatever React 19 a consumer pins and an external `ink` lands on the same copy the bundle uses. Narrowing it (e.g. back to `^19.2.4`) silently reopens the crash for the renderer itself, which breaks TUI *startup*, not just its forms. `clients/tui/__tests__/tsupConfig.test.ts` enforces all of it: React-rendering deps inlined, each exempt package both external and root-declared, and the root range pinned to `ink`'s peer floor.

README.md

@@ -504,9 +504,17 @@ Setting the passphrase later is safe — the next write upgrades an existing pla
 
 The Inspector writes the file `0600` and re-tightens it at startup if something loosened it. If it _cannot_ — the file belongs to another user, or the mount is read-only — it says so in the log rather than continuing to describe the file as protected, since on that box the mode claim above is not true.
 
-**Two Inspectors, one file.** Within a process, mutations are serialized per file path, so a web session's own concurrent saves cannot lose each other. Across processes — a CLI run beside a web session — there is deliberately **no lock**: writers are allowed to collide and the loser is made to notice. Each mutation reads the file, applies its change, writes, then reads back and compares the whole map; if another process wrote in between, it re-applies onto what they left and retries, and after five lost rounds it fails loudly rather than returning as though the value were saved.
+**Two Inspectors, one file.** Within a process, mutations are serialized per file path, so a web session's own concurrent saves cannot lose each other. Across processes — a CLI run beside a web session — each mutation takes an exclusive lock on `secrets.json.lock` for the whole read-modify-write, using [`proper-lockfile`](https://github.com/moxystudio/node-proper-lockfile) (the same library npm itself locks with). The lock expires 10 seconds after its holder stops refreshing it, so an Inspector that is killed mid-save does not leave the file unwritable.
 
-This is **not mutual exclusion**, and the residual case is worth stating: the verify only catches a clobber that has already landed, so if one Inspector reads back *before* the other's write arrives, both report success and one value is gone. That needs two Inspectors writing the same file within the gap between one's write and its read-back — narrow, but not only a crash. An earlier build did take a lock (`secrets.json.lock`); it was removed because making a `mkdir` lock single-winner on a stale takeover needs a compare-and-swap on a directory entry that Node does not expose, so it had the same class of failure with several hundred more lines and no way to close it.
+Two running Inspectors are therefore genuinely serialized. What a lock file cannot make single-winner is the *takeover of a lock whose holder died* — that needs a compare-and-swap on a directory entry (`renameat2`) which Node does not expose, and it is what an earlier hand-rolled attempt failed three review rounds on. `proper-lockfile` does not close that race either. The window opens only after a holder dies without releasing.
+
+The Inspector adds one thing on top: every lock-directory removal the library makes on its behalf — on release, and from its exit handler — is guarded by a check that the directory is still the one it created (by inode and birth time, which survive the library's own refresh but not a delete-and-recreate). That matters because those removals are otherwise unconditional, so a holder whose lock had been replaced would delete the *winner's* lock on the way out, turning one compromised writer into two unprotected ones. It also surfaces the takeover as a warning. Treat all of this as **best-effort**: the guard is still a check followed by an act, so it makes the destructive case rare rather than impossible, and it rests on filesystem metadata that not every filesystem reports.
+
+Which is why, underneath the lock, each mutation still reads the file, applies its change, writes, then reads back and compares the whole map; if something wrote in between it re-applies onto what was left and retries, failing loudly after five lost rounds rather than returning as though the value were saved. That check is what still catches a clobber inside that window — and it covers what no lock can, since a lock only orders the writers that *take* it: an editor, a restored backup, or an Inspector older than this release.
+
+If another process holds the lock and will not let go, the save **fails** rather than going ahead unlocked — waiting past the stale window first, so a crashed Inspector resolves itself rather than failing everyone else's saves. Writing alongside a writer you can see is the one case where degrading would lose the secret it was trying to protect.
+
+It is also what covers the lock being unavailable. This store exists for boxes where the usual mechanism isn't there, so a directory that can't hold a lock file — a read-only `$HOME`, a mount owned by another uid — makes the save proceed unlocked with a warning, rather than turning every `set` into a failure on exactly the deployments the store was written for.
 
 Three env vars affect where the file lands. `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` picks the store outright, bypassing the probe. `MCP_INSPECTOR_SECRET_FILE` names the file. Failing both, the file follows `MCP_STORAGE_DIR` — the same variable that relocates OAuth tokens and `client.json` — so mounting a volume at your configured storage directory is enough to make secrets durable there.
 

clients/cli/tsup.config.ts

@@ -21,6 +21,13 @@ export default defineConfig({
   noExternal: [/^@inspector\/core/],
   external: [
     "@napi-rs/keyring",
+    // Root-declared (see the repo's dependency-placement rule) and CJS, which
+    // is the combination that bites: tsup externalizes what the *client's*
+    // package.json declares, so a root-only dependency is bundled unless named
+    // here — and inlining a CJS module into an ESM bundle leaves esbuild's
+    // `Dynamic require of "path" is not supported` shim, which throws at
+    // import time and takes the whole binary down before it parses a flag.
+    "proper-lockfile",
     "@modelcontextprotocol/client",
     "@modelcontextprotocol/core",
     "commander",

clients/tui/tsup.config.ts

@@ -120,6 +120,13 @@ export default defineConfig({
     "@modelcontextprotocol/client",
     "@modelcontextprotocol/core",
     "@napi-rs/keyring",
+    // Root-declared (see the repo's dependency-placement rule) and CJS, which
+    // is the combination that bites: tsup externalizes what the *client's*
+    // package.json declares, so a root-only dependency is bundled unless named
+    // here — and inlining a CJS module into an ESM bundle leaves esbuild's
+    // `Dynamic require of "path" is not supported` shim, which throws at
+    // import time and takes the whole binary down before it parses a flag.
+    "proper-lockfile",
   ],
   esbuildPlugins: [inkFormLabelPatch],
   esbuildOptions(options) {

clients/web/server/vite-base-config.ts

@@ -34,6 +34,14 @@ const NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE = [
   // excluding it keeps Vite's dep scanner from chasing into the
   // platform-specific binaries during dev startup.
   "@napi-rs/keyring",
+  // `proper-lockfile` is reached only through `core/auth/node/file-lock.ts`
+  // — the secrets file's cross-process lock (#2082) — which the Hono
+  // `/api/servers` handlers pull in via `core/auth/node/file-secret-store.ts`.
+  // Same node-only import chain as `atomically` above, and the same reason:
+  // it is CJS with a `graceful-fs`/`signal-exit`/`retry` graph that Vite's
+  // dev scanner has no business walking. Note the tsup `external` lists do
+  // **not** cover this — they configure the production bundles, not `vite dev`.
+  "proper-lockfile",
 ] as const;
 
 export function getViteBaseConfig() {