fix(auth): address Copilot review round 1 on the secrets-file lock (#2082)

- 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>

clients/web/src/test/integration/auth/node/file-lock.test.ts

@@ -72,12 +72,32 @@ const warnings = (): string =>
 async function holdLockInChildProcess(
   target: string,
   holdMs: number,
+  /**
+   * Secrets the child writes *while holding the lock*, before it announces
+   * itself. A parent that honours the lock therefore reads a map that already
+   * contains them, which is what lets the caller assert on the merged result
+   * rather than only on timing.
+   */
+  writeWhileHeld?: Record<string, string>,
 ): Promise<{ ready: Promise<void>; done: Promise<void> }> {
   const script = `
     const lockfile = require(${JSON.stringify(LOCKFILE_MODULE)});
+    const fs = require("node:fs");
+    const path = require("node:path");
+    // A second Inspector reaches the lock through \`withSecretFileLock\`, which
+    // creates the storage directory first. Mirror that, or the fresh-install
+    // case below would be testing the child's omission rather than the parent.
+    fs.mkdirSync(path.dirname(${JSON.stringify(target)}), { recursive: true });
     lockfile
       .lock(${JSON.stringify(target)}, { realpath: false, stale: 10000 })
       .then(async (release) => {
+        const secrets = ${JSON.stringify(writeWhileHeld ?? null)};
+        if (secrets) {
+          fs.writeFileSync(
+            ${JSON.stringify(target)},
+            JSON.stringify({ version: 1, encryption: "none", secrets }),
+          );
+        }
         process.stdout.write("acquired\\n");
         await new Promise((r) => setTimeout(r, ${holdMs}));
         await release();
@@ -148,32 +168,83 @@ describe("withSecretFileLock across processes", () => {
     expect(warnings()).toBe("");
   });
 
-  it("serializes two FileSecretStores in different processes", async () => {
+  it("makes FileSecretStore.set wait on a lock another process holds", async () => {
     // The end-to-end shape from the issue: a CLI run beside a web session.
-    // The child holds the lock while the parent's `set` is in flight, so the
-    // parent's whole read-modify-write happens after the child is gone.
+    //
+    // Deliberately asserts that the parent's `set` has *not finished* while
+    // the child holds the lock. A test that only checks both keys survive
+    // afterwards passes with the lock removed from `mutate` entirely — the
+    // optimistic verify would repair the clobber and hide the regression.
+    // Not-yet-resolved is the observation only a real lock can produce.
     const target = filePath();
-    const store = new FileSecretStore({ filePath: target });
-    await store.set("srv", "env:FIRST", "1");
-
-    const { ready, done } = await holdLockInChildProcess(target, 300);
+    const { ready, done } = await holdLockInChildProcess(target, 700, {
+      "srv:env:FROM_CHILD": "1",
+    });
     await ready;
-    await store.set("srv", "env:SECOND", "2");
+
+    let settled = false;
+    const store = new FileSecretStore({ filePath: target });
+    const pending = store.set("srv", "env:FROM_PARENT", "2").then(() => {
+      settled = true;
+    });
+
+    // Comfortably inside the child's hold, and comfortably outside the few
+    // milliseconds an unlocked read-modify-write would take.
+    await new Promise((r) => setTimeout(r, 300));
+    expect(settled).toBe(false);
+
     await done;
+    await pending;
+
+    // Having waited, the parent read the map the child left, so its own entry
+    // landed *on top of* the child's rather than replacing it.
+    const reader = new FileSecretStore({ filePath: target });
+    expect(await reader.get("srv", "env:FROM_CHILD")).toBe("1");
+    expect(await reader.get("srv", "env:FROM_PARENT")).toBe("2");
+    expect(warnings()).toBe("");
+  }, 20_000);
+
+  it("creates the storage directory so the very first save is locked too", async () => {
+    // `writeStoreFile` creates the parent directory, but from *inside* the
+    // locked section — so without the `mkdir` in `withSecretFileLock` the
+    // first save on a fresh install fails `ENOENT` on the lock and degrades
+    // to an unlocked write. That is the save most likely to be racing
+    // another, since two Inspectors started together both reach it.
+    const target = path.join(tmpDir, "fresh-install", "secrets.json");
+    const { ready, done } = await holdLockInChildProcess(target, 700);
+    await ready;
+
+    let settled = false;
+    const store = new FileSecretStore({ filePath: target });
+    const pending = store.set("srv", "env:FIRST_EVER", "1").then(() => {
+      settled = true;
+    });
+
+    await new Promise((r) => setTimeout(r, 300));
+    expect(settled).toBe(false);
+
+    await done;
+    await pending;
 
     const reader = new FileSecretStore({ filePath: target });
-    expect(await reader.get("srv", "env:FIRST")).toBe("1");
-    expect(await reader.get("srv", "env:SECOND")).toBe("2");
+    expect(await reader.get("srv", "env:FIRST_EVER")).toBe("1");
+    // No degrade warning: the lock was genuinely held, not skipped.
+    expect(warnings()).toBe("");
   }, 20_000);
 });
 
 describe("withSecretFileLock degrades rather than failing", () => {
   it("runs the body anyway when the lock cannot be created, and says so once", async () => {
-    // A directory that does not exist stands in for every real variant —
+    // A path whose parent is a *file* stands in for every real variant —
     // read-only `$HOME`, a mount owned by another uid, a filesystem without
-    // `mkdir` semantics. This store exists for boxes where the usual
-    // mechanism is missing, so it must not gain a new way to be unavailable.
-    const target = path.join(tmpDir, "no-such-dir", "secrets.json");
+    // `mkdir` semantics — and unlike a permissions-based setup it fails the
+    // same way for root, so it cannot pass locally and flake in a container.
+    // Note a merely *missing* directory is no longer this case:
+    // `withSecretFileLock` creates it. This store exists for boxes where the
+    // usual mechanism is missing, so it must not gain a new way to be
+    // unavailable.
+    await fs.writeFile(path.join(tmpDir, "not-a-dir"), "", "utf-8");
+    const target = path.join(tmpDir, "not-a-dir", "secrets.json");
 
     let ran = 0;
     await withSecretFileLock(target, async () => {

core/auth/node/file-lock.ts

@@ -35,6 +35,7 @@
  * underneath it, and says so once.
  */
 
+import * as fs from "node:fs/promises";
 import * as path from "node:path";
 // CJS-only package. A default import is the shape that survives every
 // bundler this repo runs core/ through (tsup for cli/tui, vite's SSR/node
@@ -140,6 +141,19 @@ export async function withSecretFileLock<T>(
   fn: () => Promise<T>,
 ): Promise<T> {
   const target = path.resolve(filePath);
+  // Create the parent directory before locking, not after. `writeStoreFile`
+  // creates it on the way to writing the secrets file, but that runs *inside*
+  // the locked section — so on a fresh install, where `~/.mcp-inspector` does
+  // not exist yet, `proper-lockfile` would fail `ENOENT` and every first save
+  // would degrade to an unlocked write with a warning. That is the one save
+  // most likely to be racing another: two Inspectors started together both
+  // reach it, and it is exactly the interleaving this lock exists to close.
+  //
+  // Failure is deliberately swallowed rather than reported here. A directory
+  // that cannot be created is the same condition as a lock that cannot be
+  // taken, and the catch below already says so with the right message — one
+  // that mentions the lock rather than a `mkdir` the caller never asked for.
+  await fs.mkdir(path.dirname(target), { recursive: true }).catch(() => {});
   let release: (() => Promise<void>) | undefined;
   try {
     release = await properLockfile.lock(target, {

core/auth/node/secret-store-selection.ts

@@ -666,10 +666,18 @@ async function anythingToMigrate(filePath: string): Promise<boolean> {
     return (await fs.readdir(path.dirname(filePath))).some(
       (name) => name === base || name.startsWith(`${base}.migrating-`),
     );
-  } catch {
-    // No storage directory yet — the first run on a fresh install, and the
-    // path this check exists to keep quiet.
-    return false;
+  } catch (err) {
+    const code = (err as NodeJS.ErrnoException).code;
+    // Only "there is no directory" proves there is nothing to migrate — the
+    // first run on a fresh install, and the path this check exists to keep
+    // quiet. Every other failure (EACCES, EPERM, EMFILE) means the directory
+    // is there and could not be *listed*, which is not the same as empty: a
+    // directory can deny listing while still permitting access to the known
+    // `secrets.json` path, so returning false would select the keychain and
+    // leave those secrets invisible with nothing said. Fall through instead
+    // and let the under-lock `existsSync` and claim decide, which is what
+    // happened before this fast path existed.
+    return code !== "ENOENT" && code !== "ENOTDIR";
   }
 }
 

vitest.shared.mts

@@ -101,6 +101,17 @@ export function vitestSharedPaths(clientDir: string) {
       find: /^yaml$/,
       replacement: path.resolve(repoRoot, "node_modules/yaml"),
     },
+    // Same reasoning, one layer in: `proper-lockfile` is reached only through
+    // `core/` (the secrets file's cross-process lock, #2082), which is the
+    // other root-owned tree with no manifest of its own. Resolution finds the
+    // root copy on its own today — nothing declares it in a client — and this
+    // pin is what keeps that from depending on nothing ever arriving as some
+    // client's transitive dependency, which would otherwise give a test two
+    // copies of a module whose whole job is a single registry of held locks.
+    {
+      find: /^proper-lockfile$/,
+      replacement: path.resolve(repoRoot, "node_modules/proper-lockfile"),
+    },
   ];
 
   const projectResolve = {