KeyringSecretStore throws from the AsyncEntry constructor, defeating its own degradation contract (500 on GET /api/servers)

[dan-kwiat]

Version: v2 (main)
Affected client: core (observed via Web; CLI/TUI catalog loads hit the same path)

Follow-up to #1845 with a root cause and prompt to fix. Happy to fold this into that issue instead if you'd prefer it tracked there.

Prompt

I fixed this locally with the following prompt:

---

In core/auth/node/secret-store.ts, KeyringSecretStore constructs its AsyncEntry outside the try in get, set and delete. The @napi-rs/keyring constructor performs the platform-store setup and throws when no keychain is reachable, so the documented degradation contract (:79-89) never engages: the raw error escapes instead, 500ing every GET /api/servers and breaking CLI/TUI catalog loads on any box without a Secret Service.

Move the construction inside the existing try blocks in all three methods so the documented contract holds when the failure is at construction time: get returns null, delete silently no-ops, and set is the only operation that throws — as KeychainUnavailableError, so the routes' 503 translation and the migratePlaintextSecrets skip branch in core/mcp/remote/node/server.ts both engage. Update the doc comment if the wording no longer matches.

Extend the vi.mock in clients/web/src/test/integration/auth/node/secret-store.test.ts: its stub constructor cannot fail today, which is why this gap survived the coverage gate. Add a constructorThrows hook alongside the existing method-level failures flags and cover all three methods under it.

---

Detail

AsyncEntry::new in @napi-rs/keyring does the platform-store setup (on Linux, the Secret Service connect with a keyutils fallback) and returns a Result — so it throws when neither backend is reachable. Because construction sits outside the try in get/set/delete, the documented behavior at secret-store.ts:79-89 ("get returns null, destructive ops no-op, only set hard-fails") never engages for a construction-time failure.

Two consequences beyond the 500 itself:

• expectedSecretFields always includes the OAuth slot, so rehydrateConfig constructs an entry for every server. The default seeded catalog is enough to 500 the first GET /api/servers, before any server is added.
• The escaping error isn't a KeychainUnavailableError, so it bypasses both the routes' 503 translation and the migratePlaintextSecrets skip branch in core/mcp/remote/node/server.ts — a generic 500 instead of the actionable message.

Before: GET /api/servers → 500 Failed to read server list: Couldn't access platform storage: PermissionDenied (note the unwrapped keyring message — proof it escaped the try).
After: 200 with the server list; secret-free entries work with no keychain present.

Verification

Reproduced and confirmed fixed in the published container, which has no D-Bus session:

docker build -t mcp-inspector .
docker run --rm -p 6274:6274 mcp-inspector

Before the change, adding a server appeared to succeed (POST → {"ok":true}) but the list never loaded; after, the round-trip works.

Judgment call worth flagging

Having set wrap a constructor failure as KeychainUnavailableError changes what callers see from a raw throw to a typed one. That's what makes the 503 ton-skip branch fire, and it matches the documented contract — but it is a behavior change for anything matching on the raw error, so it's worth a deliberate decision rather than being assumed.

Related, separate question

This restores non-secret flows in a container but doesn't answer what secret persistence should mean there. Since SecretStore is already an interface (with InMemorySecretStore alongside), an explicit file-backed or in-memory store may be a better container story than shipping a keyring daemon. Happy to open that separately if useful.