fix(core): lazy-load @napi-rs/keyring so a missing native binding can't kill the launcher

clients/web/src/test/integration/auth/node/secret-store.test.ts

@@ -8,7 +8,7 @@
  * (`get` returns null on failure, destructive ops no-op, `set` is the
  * one operation that hard-fails with `KeychainUnavailableError`).
  */
-import { describe, it, expect, beforeEach, vi } from "vitest";
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
 
 // The mock must be hoisted above the `await import` of secret-store
 // inside the `KeyringSecretStore` describe block. Use `vi.hoisted` so
@@ -24,6 +24,7 @@ const keyringMocks = vi.hoisted(() => {
     deleteThrows: false,
     findThrows: false,
     deleteThrowsNoEntry: false,
+    constructorThrows: false,
   };
   const credentials = (): Array<{ account: string; password: string }> => {
     const out: Array<{ account: string; password: string }> = [];
@@ -35,6 +36,12 @@ const keyringMocks = vi.hoisted(() => {
   class AsyncEntry {
     private readonly key: string;
     constructor(_service: string, username: string) {
+      // `AsyncEntry::new` performs the platform-store setup and throws
+      // when no secret service is reachable (#1848) — the stub must be
+      // able to fail here, not just in the methods.
+      if (failures.constructorThrows) {
+        throw new Error("Couldn't access platform storage: PermissionDenied");
+      }
       this.key = username;
     }
     async getPassword(): Promise<string | undefined> {
@@ -215,6 +222,7 @@ describe("KeyringSecretStore (mocked native bindings)", () => {
     keyringMocks.failures.deleteThrows = false;
     keyringMocks.failures.findThrows = false;
     keyringMocks.failures.deleteThrowsNoEntry = false;
+    keyringMocks.failures.constructorThrows = false;
     store = new KeyringSecretStore();
   });
 
@@ -311,13 +319,47 @@ describe("KeyringSecretStore (mocked native bindings)", () => {
     ).toBe("p");
   });
 
+  it("get returns null when the AsyncEntry constructor throws (#1848)", async () => {
+    keyringMocks.failures.constructorThrows = true;
+    expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe(
+      null,
+    );
+  });
+
+  it("set throws KeychainUnavailableError when the AsyncEntry constructor throws (#1848)", async () => {
+    keyringMocks.failures.constructorThrows = true;
+    await expect(
+      store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v"),
+    ).rejects.toBeInstanceOf(KeychainUnavailableError);
+  });
+
+  it("delete silently no-ops when the AsyncEntry constructor throws (#1848)", async () => {
+    keyringMocks.failures.constructorThrows = true;
+    await expect(
+      store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET),
+    ).resolves.toBeUndefined();
+  });
+
   it("KeychainUnavailableError stringifies a non-Error cause", () => {
     const err = new KeychainUnavailableError("plain string cause");
     expect(err).toBeInstanceOf(KeychainUnavailableError);
     expect(err.message).toContain("plain string cause");
     expect(err.message).toMatch(/libsecret/);
   });
 
+  it("KeychainUnavailableError steers a missing-native-binding cause to a reinstall hint (#1852)", () => {
+    const err = new KeychainUnavailableError(
+      new Error(
+        "Cannot find native binding. npm has a bug related to optional dependencies " +
+          "(https://github.com/npm/cli/issues/4828).",
+      ),
+    );
+    expect(err.message).toMatch(/reinstall the Inspector/);
+    expect(err.message).toMatch(/Cannot find native binding/);
+    // The Linux keyring-daemon advice is irrelevant to a broken install.
+    expect(err.message).not.toMatch(/libsecret/);
+  });
+
   it("KeychainUnavailableError carries the underlying error message", async () => {
     keyringMocks.failures.setThrows = true;
     try {
@@ -330,3 +372,63 @@ describe("KeyringSecretStore (mocked native bindings)", () => {
     }
   });
 });
+
+describe("KeyringSecretStore (module load failure — #1852)", () => {
+  // `loadKeyring` caches its result per module instance, so each test
+  // takes a fresh copy of secret-store whose dynamic import of
+  // `@napi-rs/keyring` rejects the way the napi-rs loader does when the
+  // platform binary package is missing (npm's optional-deps bug,
+  // npm/cli#4828). The statically-imported copy at the top of this file
+  // is untouched — these tests only use the fresh module's exports.
+  const importWithLoadFailure = async () => {
+    vi.resetModules();
+    vi.doMock("@napi-rs/keyring", () => {
+      throw new Error(
+        "Cannot find native binding. npm has a bug related to optional dependencies " +
+          "(https://github.com/npm/cli/issues/4828). Please try `npm i` again after removing " +
+          "both package-lock.json and node_modules directory.",
+      );
+    });
+    return import("@inspector/core/auth/node/secret-store.js");
+  };
+
+  afterEach(() => {
+    vi.doUnmock("@napi-rs/keyring");
+    vi.resetModules();
+  });
+
+  it("get returns null instead of propagating the load error", async () => {
+    const mod = await importWithLoadFailure();
+    const store = new mod.KeyringSecretStore();
+    expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe(
+      null,
+    );
+    // The failed load is cached — a second call degrades the same way.
+    expect(await store.get("alpha", envSecretField("KEY"))).toBe(null);
+  });
+
+  it("delete and deleteAllForServer silently no-op", async () => {
+    const mod = await importWithLoadFailure();
+    const store = new mod.KeyringSecretStore();
+    await expect(
+      store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET),
+    ).resolves.toBeUndefined();
+    await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined();
+  });
+
+  it("set throws KeychainUnavailableError built from the cached load error", async () => {
+    // (vitest substitutes its own error text when a mock factory throws,
+    // so the loader's exact message can't be asserted here — the
+    // reinstall-hint wording is covered by the direct-construction test
+    // above. What matters here is the *typed* rejection: it's what the
+    // routes' 503 translation and the migratePlaintextSecrets skip
+    // branch dispatch on.)
+    const mod = await importWithLoadFailure();
+    const store = new mod.KeyringSecretStore();
+    await expect(
+      store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v"),
+      // Instance check against the fresh module's class — the statically
+      // imported KeychainUnavailableError is a different module instance.
+    ).rejects.toBeInstanceOf(mod.KeychainUnavailableError);
+  });
+});

core/auth/node/secret-store.ts

@@ -13,10 +13,32 @@
  * browser side never imports this; it gets values rehydrated into the
  * `/api/servers` response by the Hono handler.
  */
-import { AsyncEntry, findCredentialsAsync } from "@napi-rs/keyring";
 
 const SERVICE_NAME = "mcp-inspector";
 
+type KeyringModule = typeof import("@napi-rs/keyring");
+
+/**
+ * `@napi-rs/keyring` is loaded lazily because its top-level `require`
+ * throws when the platform binary package (an npm optionalDependency,
+ * e.g. `@napi-rs/keyring-win32-x64-msvc`) is missing — npm's
+ * optional-deps bug (npm/cli#4828) drops those on install, and the npx
+ * cache hits it on in-place upgrades. A static import here would take
+ * down the whole launcher at boot (#1852); loading on first use instead
+ * lets the load failure degrade through the same availability contract
+ * as an unreachable keychain (see `KeyringSecretStore`).
+ */
+let keyringLoad: Promise<KeyringModule | null> | undefined;
+let keyringLoadError: unknown;
+
+function loadKeyring(): Promise<KeyringModule | null> {
+  keyringLoad ??= import("@napi-rs/keyring").catch((err: unknown) => {
+    keyringLoadError = err;
+    return null;
+  });
+  return keyringLoad;
+}
+
 export {
   SECRET_FIELD_OAUTH_CLIENT_SECRET,
   SECRET_FIELD_IDP_CLIENT_SECRET,
@@ -39,18 +61,23 @@ const buildAccount = (serverId: string, field: string): string =>
   `${serverId}:${field}`;
 
 /**
- * Thrown when the OS keychain is unavailable — typically Linux without
- * libsecret / gnome-keyring installed. Surfaced as a 503 by the API
- * handlers so the UI can show an actionable error rather than a generic
- * 500. macOS and Windows always have a working keychain, so this only
- * realistically fires on minimal Linux installs.
+ * Thrown when the OS keychain is unavailable. Two realistic causes:
+ * Linux without libsecret / gnome-keyring installed, and a missing
+ * `@napi-rs/keyring` platform binary package (npm's optional-deps bug,
+ * npm/cli#4828 — the module itself fails to load, any OS). Surfaced as
+ * a 503 by the API handlers so the UI can show an actionable error
+ * rather than a generic 500.
  */
 export class KeychainUnavailableError extends Error {
   constructor(cause: unknown) {
-    super(
-      `OS keychain is not available. On Linux, install libsecret / gnome-keyring. ` +
-        `Underlying error: ${cause instanceof Error ? cause.message : String(cause)}`,
-    );
+    const message = cause instanceof Error ? cause.message : String(cause);
+    // The napi-rs loader's throw for a missing platform package starts
+    // with this phrase — steer those users to a reinstall instead of
+    // the (irrelevant) Linux keyring-daemon advice.
+    const hint = message.includes("Cannot find native binding")
+      ? `The @napi-rs/keyring platform package for this OS is missing — reinstall the Inspector (for npx, clear the npx cache under your npm cache directory first).`
+      : `On Linux, install libsecret / gnome-keyring.`;
+    super(`OS keychain is not available. ${hint} Underlying error: ${message}`);
     this.name = "KeychainUnavailableError";
   }
 }
@@ -79,19 +106,29 @@ export interface SecretStore {
  * can use `=== null` rather than truthiness (an empty-string secret is
  * a real value and must round-trip).
  *
- * **Availability behavior.** When the keychain is unavailable (the
- * typical case is Linux without libsecret / gnome-keyring), `set` is
- * the only operation that throws `KeychainUnavailableError` — that's
- * the moment where data would actually be lost. `get` returns `null`
- * (as if no entry existed) and the destructive operations silently
- * no-op (there's nothing to delete anyway). This keeps non-secret
- * flows working on a stock CI runner / minimal Linux box; the user
- * only hits a hard error when they actually try to save a secret.
+ * **Availability behavior.** When the keychain is unavailable — the
+ * module failed to load (missing platform binary, #1852), the entry
+ * constructor threw (no reachable secret service, #1848), or the
+ * operation itself failed (Linux without libsecret / gnome-keyring) —
+ * `set` is the only operation that throws `KeychainUnavailableError`:
+ * that's the moment where data would actually be lost. `get` returns
+ * `null` (as if no entry existed) and the destructive operations
+ * silently no-op (there's nothing to delete anyway). This keeps
+ * non-secret flows working on a stock CI runner / minimal Linux box /
+ * broken npx-cache install; the user only hits a hard error when they
+ * actually try to save a secret.
  */
 export class KeyringSecretStore implements SecretStore {
   async get(serverId: string, field: string): Promise<string | null> {
-    const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field));
+    const keyring = await loadKeyring();
+    if (!keyring) return null;
     try {
+      // Constructed inside the try: `AsyncEntry::new` performs the
+      // platform-store setup and throws when no backend is reachable.
+      const entry = new keyring.AsyncEntry(
+        SERVICE_NAME,
+        buildAccount(serverId, field),
+      );
       const v = await entry.getPassword();
       return v ?? null;
     } catch {
@@ -104,8 +141,13 @@ export class KeyringSecretStore implements SecretStore {
   }
 
   async set(serverId: string, field: string, value: string): Promise<void> {
-    const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field));
+    const keyring = await loadKeyring();
+    if (!keyring) throw new KeychainUnavailableError(keyringLoadError);
     try {
+      const entry = new keyring.AsyncEntry(
+        SERVICE_NAME,
+        buildAccount(serverId, field),
+      );
       await entry.setPassword(value);
     } catch (err) {
       // The only operation that hard-fails — if we can't persist the
@@ -116,8 +158,13 @@ export class KeyringSecretStore implements SecretStore {
   }
 
   async delete(serverId: string, field: string): Promise<void> {
-    const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field));
+    const keyring = await loadKeyring();
+    if (!keyring) return;
     try {
+      const entry = new keyring.AsyncEntry(
+        SERVICE_NAME,
+        buildAccount(serverId, field),
+      );
       await entry.deleteCredential();
     } catch {
       // Both reasons for a throw collapse to the same desired outcome
@@ -131,9 +178,11 @@ export class KeyringSecretStore implements SecretStore {
   }
 
   async deleteAllForServer(serverId: string): Promise<void> {
+    const keyring = await loadKeyring();
+    if (!keyring) return;
     let creds: Array<{ account: string; password: string }>;
     try {
-      creds = await findCredentialsAsync(SERVICE_NAME);
+      creds = await keyring.findCredentialsAsync(SERVICE_NAME);
     } catch {
       // Same reasoning as `delete`: nothing was written, nothing to sweep.
       return;