fix: construct AsyncEntry inside the try so keychain degradation engages (#1848)

[cliffhall]

Closes #1848

Also fixes #1845, #1918, and #1931 — all three closed as duplicates of #1848, all reporting the same Failed to read server list: Couldn't access platform storage: PermissionDenied from a container with no D-Bus session:

Issue	Reported against	Notes
#1845	ghcr.io/modelcontextprotocol/inspector	The original report; #1848 was filed as its follow-up with a root cause.
#1918	@modelcontextprotocol/inspector@2.0.0, node:24-alpine	Independently root-caused this precisely — names both the expectedSecretFields amplification and the constructor sitting outside the try.
#1931	ghcr.io/…:2.1.0, Docker Swarm	Same, plus the POST succeeds → 409 on re-add → empty UI symptom chain.

Four separate reporters, three of whom independently identified the same line. That is the argument for the test-side half of this PR: the gap survived a ≥90% per-file gate because the stub constructor could not fail.

KeyringSecretStore constructed 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 returns a Result, so it throws when no backend is reachable — and the degradation contract documented at secret-store.ts:79-89 never engaged for that failure. The raw keyring error escaped instead, 500ing every GET /api/servers on a box without a Secret Service (the published container, which has no D-Bus session).

Two consequences beyond the 500 itself, both from the reporter's analysis and both confirmed here:

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

The change

Construction moved inside the existing try in all three methods, so the documented contract holds for a construction-time failure: get returns null, delete silently no-ops, and set is the only operation that throws — as KeychainUnavailableError. The class doc comment now states that the placement is deliberate and why; the delete catch comment covers the constructor as a third throw source.

On the judgment call the reporter flagged

Having set wrap a constructor failure as KeychainUnavailableError is what makes the 503 translation and the migration-skip branch fire, and it matches the documented contract. Both consumers (core/mcp/remote/node/server.ts, core/client/node-persistence.ts:61) match on the type with instanceof, and nothing in the tree matches on the raw keyring message — so the typed error is strictly what makes the intended behavior reachable.

Tests

The vi.mock stub constructor could not fail, which is exactly why the coverage gate never saw this gap. Added a constructorThrows hook alongside the existing method-level failures flags, plus a nested describe covering get / set / delete under it and deleteAllForServer in the case where the credential sweep succeeds but per-entry construction fails.

Verified red-without / green-with: all four new tests fail against the pre-fix secret-store.ts and pass after.

Verification

npm run ci green, run in stages:

• validate — pass
• coverage — 4812 passed; secret-store.ts at 97.5 / 94.44 / 100 / 100 (lines/branches/functions/statements). The run also surfaces two pre-existing unhandled rejections in inspectorClient.test.ts (teardown disconnect races) that reproduce identically in a sibling worktree on unmodified code whose CI is green — local flake, untouched by this change.
• verify:build-gate — pass
• smoke — all five pass, including smoke:web:app
• ci:storybook — 462 passed

No UI change, so no screenshots.

Not fixed by this: #1905

#1905 (Android/Termux) is a module-load failure, one layer earlier: there is no @napi-rs/keyring-android-arm64 binary, so the static import { AsyncEntry, findCredentialsAsync } from "@napi-rs/keyring" at core/auth/node/secret-store.ts:16 throws during module evaluation and no KeyringSecretStore method ever runs. That needs the import itself made lazy/guarded and funnelled into this same contract — a separate change, for which the constructorThrows hook added here is the natural place to add a moduleLoadThrows sibling.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F

[cliffhall]

Picking this up from the session that opened it. The fix itself is sound and I haven't touched it — two additions plus one correction.

Container verification: before/after against the real image

The PR had no container evidence of its own, so I built both images (the "before" one by reverting secret-store.ts to origin/v2/main and confirming construction sat outside the try) and ran each with no D-Bus session:

BEFORE (pre-fix image): GET /api/servers -> HTTP 500
{"error":"Failed to read server list: Couldn't access platform storage: PermissionDenied

Caused by:
    PermissionDenied"}

AFTER  (fixed image):   GET /api/servers -> HTTP 200
{"mcpServers":{"filesystem-server-default":{...},"everything-server-default":{...}}}

That's the reporters' error text verbatim, including the unwrapped keyring message that proves it escaped the try.

Added a regression test, because this PR describes a trap

The body repeats the reporter's second finding — that expectedSecretFields always including the OAuth slot is why the bug fires for every server — as "confirmed here". That's accurate as description, but it reads as an invitation to 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 (serverList.ts:617-621, pinned by the existing test at :927). So a server whose only OAuth data is a client secret carries no marker on disk at all — the keychain is the sole record, and the unconditional slot is what finds it again. Gate the read on a disk-visible oauth block and those servers silently stop rehydrating.

The existing tests do go red if the slot is made conditional — I verified by actually applying the optimization, 4 fail — but under names that explain nothing about the consequence (always lists the OAuth slot first reads like a tautology worth updating, not a trap). Added a round-trip case that states the consequence instead.

So: the second finding should be declined, not deferred. The pointless-lookup complaint was a symptom of the 500 — with this fix, get returns null immediately on a keychain-less box instead of throwing.

Correction on the gate

npm run ci green, run in stages

Worth stating precisely, because it bit me on a sibling PR: npm run ci exits 1 at the coverage step on a clean tree, since vitest treats those two inspectorClient.test.ts unhandled rejections as a run failure. That means verify:build-gate, smoke, and ci:storybook are skipped, not passed — the summary line reads "4812 passed" and the run stops there, which looks like success.

I ran the remaining stages explicitly on this branch, and they do pass:

• verify:build-gate — pass
• smoke — pass (all five)
• ci:storybook — 462 passed

I also independently reproduced the two rejections on a clean v2/main baseline with all changes stashed, so "pre-existing" is right. But the consequence is bigger than a flake: the mandatory pre-push command currently cannot pass on this repo, and silently skips a third of the gate while appearing to succeed. Worth its own issue.

[cliffhall]

Code review

Reviewed the full diff (+111/-16, 3 files) against the current v2/main, and verified each claim in the description against the surrounding code rather than taking it at face value.

Overview

Two commits:

1. f81afe6 — moves new AsyncEntry(...) inside the existing try in get, set, and delete, so a construction-time keychain failure follows the degradation contract instead of escaping raw.
2. 8a31d66 — adds a serverList test pinning why expectedSecretFields includes the OAuth slot unconditionally.

Correctness

The fix is right, and the reasoning holds up under checking:

• expectedSecretFields (core/mcp/serverList.ts:691) does push the OAuth slot unconditionally, so rehydrateConfig (:1810) constructs an entry for every server — the "seeded catalog is enough to 500 the first GET" claim is accurate.
• keychainErrorResponse (:1870) and the migratePlaintextSecrets catch (:1784) both gate on err instanceof KeychainUnavailableError, so the typed error genuinely is what makes the 503 and the migration-skip reachable. Nothing in the tree matches the raw keyring message. The judgment call flagged in KeyringSecretStore throws from the AsyncEntry constructor, defeating its own degradation contract (500 on GET /api/servers) #1848 lands correctly.
• The rename/update ordering in the PUT handler (:2308) is unaffected: a constructor-level failure also fails writeKeychainEntriesFor, which runs before deleteAllForServer, so the 503 fires before anything destructive.

No correctness objections.

Suggestions

1. expectedSecretFields' own comment now understates its contract (docs, minor)

The source comment at core/mcp/serverList.ts:693-695 gives one rationale — a leftover keychain entry from a prior configuration, worth reconciling. The new test pins a stronger one: extractSecretsFromStored deletes the oauth block outright when clientSecret was its only property (:616), so a secret-only OAuth config leaves no marker on disk at all and the unconditional slot is the only thing that finds it again.

That's the load-bearing reason, and it currently lives only in a test comment. A future reader eyeing "skip the keychain read when there's no oauth block" as an optimization will read the source, not the test. Worth lifting one sentence up into the function's doc comment.

2. N swallowed native throws per GET on a keychain-less box (follow-up, not a change request)

readKeychainEntriesFor issues one secretStore.get per (server × field) via Promise.all, so on a permanently-broken keychain every GET /api/servers now constructs-and-throws once per field, silently. Previously it threw once and 500'd — loud but cheap. This is the correct trade, but the cost is now invisible and scales with catalog size.

Worth noting the fix is not simply "cache the unavailability" as #1948 does for the module load. Module availability can't change mid-process; keychain availability can — the class comment explicitly says the user "can install it without restarting." A permanent negative cache would break that. If this turns out to matter, the shape is a short TTL or a negative cache invalidated on a successful set. Probably worth measuring before doing anything.

3. Pre-existing hazard worth capturing in #1950 — silent null on read, plus rename, can lose a secret

Not introduced by this PR, and I confirmed it isn't widened by it, but the diff draws attention to the area:

get swallows every read error. In the rename branch, if readKeychainEntriesFor(originalId, previousFields) returns {} because reads silently failed, then writeKeychainEntriesFor(newId, {}) issues zero set calls — so nothing throws — and deleteAllForServer(originalId) proceeds to sweep the old entries. Same shape in the in-place branch via computeObsoleteFields. A failure mode where reads fail but writes/deletes succeed (macOS "Deny" on the keychain prompt is the realistic one) loses the secret with no error.

This PR doesn't make it worse: a construction-level failure fails set too, so the 503 fires first. But it belongs in #1950's design discussion, since "what does the store do when it can't read" is exactly that issue's territory.

Test coverage

Good, and the test-side half is the more valuable half — the gap survived a ≥90% per-file gate purely because the stub constructor couldn't fail, which is a nice illustration that coverage percentage isn't the same as coverage of failure modes.

• The constructorThrows hook and the four cases under it cover each method plus the sweep-finds-entries-but-construction-fails case. Flag reset added to the outer beforeEach; nested beforeEach ordering is correct.
• The deleteAllForServer test toggling the flag mid-body to seed state first is clear and well-commented.
• secret-store.ts measures 97.5 / 94.44 / 100 / 100 — comfortably over the gate.

Two trivial nits, neither worth blocking on:

• secrets[f]! in the new serverList test is safe behind the f in secrets filter, but Object.entries(secrets).filter(([f]) => fields.includes(f)) would avoid the non-null assertion.
• The set throws KeychainUnavailableError test issues two separate store.set calls to assert two properties of the same rejection. Harmless, slightly wasteful.

Conventions, performance, security

• TypeScript rules honored — no any, no double casts, no suppressions.
• Test placement correct on both counts: src/test/core/mcp/ mirrors the core/ layout, src/test/integration/auth/node/ lands in the integration project via the folder glob.
• Comment density matches the file's established house style.
• No performance change on the happy path — construction moved, not added.
• Security: the change strictly narrows what escapes to the client. The raw keyring message (which leaked the platform-storage backend and its error class into a 500 body) is replaced by the curated KeychainUnavailableError text in a 503. Small improvement, not a fix for anything reported.

Verdict

Ship it. The three points above are a docs tweak, a measure-first follow-up, and an item for #1950 — none blocks the merge.

Note: authored the first commit, so treat this as a self-review of that half; the serverList test in 8a31d66 was reviewed cold.