fix: load @napi-rs/keyring lazily so an unsupported platform degrades instead of crashing (#1905)

[cliffhall]

Closes #1905
Closes #1852

Stacked on #1945 (v2/fix/keyring-construct-degradation, which closes #1848). Base is that branch, not v2/main, so this diff shows only the lazy-import change. Merge #1945 first; GitHub will retarget this to v2/main automatically. The two fix adjacent failure modes in the same file and the second builds on the first's test scaffolding, which is why they're stacked rather than independent.

@napi-rs/keyring ships one prebuilt binary per platform triple and throws on import where it has none. On Android / Termux there is no @napi-rs/keyring-android-arm64, so the static top-level import at core/auth/node/secret-store.ts:16 threw during module evaluation — the Inspector exited at startup with Cannot find native binding, before any of the keychain-unavailable handling in that same file could run.

This is one layer earlier than #1848: there the package loaded and AsyncEntry::new threw; here the package never loads at all.

Also closes #1852 ("unable to run the web UI on Windows"), which is the same root cause reached a different way: npm's optional-deps bug (npm/cli#4828) drops the platform package on a supported triple — typically via an in-place npx cache upgrade — and the static import then throws the identical Cannot find native binding at startup. That was previously being fixed in #1943, which bundled this change together with #1848's; #1943 is closed as superseded by this stack, and its one unique piece is ported below.

The change

The static import becomes a cached dynamic one. Two details are deliberate:

• The outcome is cached, not just the module. A box without a binary must not re-attempt (and re-throw) resolution on every secret operation — expectedSecretFields always includes the OAuth slot, so that would be once per server per GET /api/servers.
• The failure is cached alongside the success so set can name the underlying cause in its KeychainUnavailableError rather than reporting a bare "unavailable".

An unloadable package now folds into the same degradation contract as an unreachable keychain: get returns null, delete / deleteAllForServer no-op, set throws KeychainUnavailableError — and is not double-wrapped, since the load failure is already typed by the time the catch sees it.

The class doc comment now enumerates all three ways "unavailable" can arrive (package won't load / constructor throws / operation throws), because each of them escaped the contract at some point and the contract is only as good as its narrowest funnel. Also updated the @napi-rs/keyring note in clients/web/server/vite-base-config.ts to say the load is lazy.

The KeychainUnavailableError message now branches on the cause (ported from #1943). A missing platform binary has nothing to do with a keyring daemon, so telling that user to install libsecret sends them down the wrong path — when the cause carries the loader's Cannot find native binding phrasing, the error instead points at a reinstall and, for npx, clearing the npx cache. Every other cause keeps the Linux libsecret / gnome-keyring advice.

There is deliberately no cache-reset seam — a platform does not grow a native binary mid-run. Tests reach the unloadable path through a fresh module instead (below).

Tests

The shared vi.mock stub can't express this — it models a keyring that loads. The new describe builds on vi.resetModules() + vi.doMock with a throwing factory and re-imports the module, covering: the module still evaluating at all (the regression itself), get, set (typed, with the cause appended, not double-wrapped), delete / deleteAllForServer, and the load-once caching.

One assertion is deliberately by shape rather than literal text: vitest substitutes its own "error when mocking a module" message for a throwing doMock factory, so the simulated Cannot find module ... string never reaches the store. The test asserts the cause is appended (/Underlying error: .+/) rather than swallowed; the comment records why. In production the real "Cannot find native binding" text lands in that slot.

Verified red-without / green-with: all six new tests fail against the static import and pass after.

The two hint branches get their own cases, asserted by direct construction rather than through the store — for the same doMock reason as above, the real loader text can't reach the constructor from the unloadable-package path, so these pin the wording against the message napi-rs actually produces (reinstall hint present and libsecret advice absent for a native-binding cause; the reverse for any other).

Bundling check — because the fix only works if the dynamic import survives bundling, I confirmed all three built clients keep it as a dynamic import rather than inlining it back to a static one:

clients/cli/build/index.js:import("@napi-rs/keyring")
clients/tui/build/index.js:import("@napi-rs/keyring")
clients/web/build/index.js:import("@napi-rs/keyring")

Verification

npm run ci green, run in stages:

• validate — pass on re-run. The first pass showed two failures in AppsScreen.test.tsx and useServers.test.tsx (both timing-sensitive, one at 5.9s under full-suite load); both pass in isolation and neither touches the keyring path.
• coverage — 4818 passed; secret-store.ts at 98.3 / 96.42 / 100 / 100. Same two pre-existing inspectorClient.test.ts unhandled rejections noted on fix: construct AsyncEntry inside the try so keychain degradation engages (#1848) #1945, which reproduce identically in a sibling worktree on unmodified code whose CI is green.
• verify:build-gate — pass
• smoke — all six pass
• ci:storybook — 462 passed

No UI change, so no screenshots.

Not covered here

This makes an unsupported platform start and work for non-secret flows; it does not give it secret persistence. The reporter on #1848 raised the same question for containers. Since SecretStore is already an interface with InMemorySecretStore alongside, a file-backed or explicitly in-memory store is worth considering as its own issue — out of scope for this fix.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F

[cliffhall]

Code review

Reviewed the full diff (+208/-23, 3 files) against its base branch, and checked the runtime behavior of the real package rather than reasoning only from the mocked tests.

Overview

Replaces the static top-level import { AsyncEntry, findCredentialsAsync } from "@napi-rs/keyring" with a cached dynamic import, so a platform with no prebuilt binary degrades through the existing keychain-unavailable contract instead of crashing during module evaluation.

Correctness

The mechanism is sound, and a few details are right in ways that are easy to get wrong:

• keyringLoad ??= … is assigned synchronously, before any await, so N concurrent gets from readKeychainEntriesFor's Promise.all share one in-flight import rather than racing N of them.
• .then(onFulfilled, onRejected) means the cached promise never rejects — it always resolves to a KeyringLoad. No unhandled-rejection hazard from a cached failure nobody awaits.
• The double-wrap guard in set is a real catch, not a hypothetical one. Throwing KeychainUnavailableError from inside the try and then re-entering its own catch would otherwise nest the prefix twice and push the actual cause a level deeper. Good that it has a dedicated test asserting exactly one prefix.
• typeof import("@napi-rs/keyring") is type-only and erases cleanly; the bundle check in the description confirms only the dynamic import(...) survives in all three clients.

I verified the CJS→ESM interop against the real package rather than trusting the mock:

keys: AsyncEntry,Entry,default,findCredentials,findCredentialsAsync,module.exports
AsyncEntry: function | findCredentialsAsync: function

So mod.AsyncEntry resolves correctly today — same named-export resolution the static import was already relying on. Not a live bug.

The one thing I'd change

Validate the module shape in loadKeyring, and treat a bad shape as unavailable.

The namespace check above is reassuring for today, but consider the failure mode if it ever stops holding — a package version that moves to a default-only export, a bundler that changes interop handling, or a platform where cjs-module-lexer doesn't detect the named exports:

• keyring.mod.AsyncEntry is undefined
• new undefined(...) throws a TypeError
• that TypeError is caught by the very catch that implements graceful degradation
• get returns null, so every server's secrets silently vanish and the UI shows an empty list with no error at all

Nothing in the suite would catch that. The unit tests all run against the stub, so they never exercise the real namespace. And the smokes don't help either: smoke:web:browser asserts the "Add Servers" control renders — which is the empty state — so a total secret-rehydration failure and a healthy first run look identical to it.

That's a nasty shape for a bug: silent, data-shaped, and invisible to every gate. Cheap fix, in loadKeyring:

keyringLoad ??= import("@napi-rs/keyring").then(
  (mod): KeyringLoad =>
    typeof mod.AsyncEntry === "function" &&
    typeof mod.findCredentialsAsync === "function"
      ? { ok: true, mod }
      : {
          ok: false,
          err: new Error(
            "@napi-rs/keyring loaded but did not expose AsyncEntry / findCredentialsAsync",
          ),
        },
  (err: unknown): KeyringLoad => ({ ok: false, err }),
);

That converts a silent empty-list into the same actionable 503 on set that every other unavailability produces — which is exactly the contract this PR is about. It's also the one new risk the refactor introduces, so it seems worth closing in the same change.

If you'd rather not, the alternative is an unmocked round-trip test (set → get → delete against the real keyring, skipped when unavailable) so at least one thing in the suite touches the real namespace. The guard is cheaper and fails more usefully.

Test coverage

Strong. Six cases, and the vi.resetModules() + vi.doMock approach is the right call — a flag on the shared stub genuinely cannot express "the import itself rejects," and the comment says so rather than leaving the reader wondering why this describe is shaped differently.

• Asserting the module still evaluates at all as its own first test is the right framing: that, not any downstream behavior, is the regression.
• The load-once test is a genuine caching assertion, not a tautology — three operations, one attempt.
• The honesty about vitest substituting its own message for a throwing doMock factory, with the assertion relaxed to /Underlying error: .+/ and the reason recorded inline, is better than the alternative of quietly asserting something weaker with no explanation. It does mean the real "Cannot find native binding" text is never exercised end to end; acceptable, and now documented.

secret-store.ts at 98.3 / 96.42 / 100 / 100.

Nits

• Comment run-on in the test file: the block explaining the re-imported module's distinct class identity runs straight into "The real-world message on Termux…" with no separator, so the two read as one comment. The second belongs to LOAD_ERROR — a blank line, or moving it directly above the constant, fixes it.
• LOAD_ERROR is now thrown but never matched against. That's fine and deliberate, but a reader may go looking for the assertion that uses it; the comment mostly covers this.

Conventions, performance, security

• TypeScript rules honored — the discriminated KeyringLoad union is the right shape here, no any, no casts.
• The vite-base-config.ts comment update is the kind of doc-sync the project asks for and is easy to forget; good that it's in the same PR.
• Performance: one extra module resolution on the first secret operation of the process, cached thereafter. Negligible, and it removes the resolution from startup.
• The deliberate absence of a cache-reset seam is correct and well-argued — a platform doesn't grow a native binary mid-run, and the reasoning is recorded where the next person will look. Note this contrasts with the keychain-reachability case, which genuinely can change mid-process; the doc comment could arguably say why the two are cached differently, but it's a fine distinction and the current text isn't wrong.
• Security: no new surface. Secrets still never touch disk on this path.

Verdict

Approve modulo the shape guard, which I'd fold in before merge — it's a few lines and it closes the only new silent-failure path this refactor opens. Everything else is a nit.

Note: I authored this PR, so treat the review as a structured self-check rather than independent sign-off — the shape-guard finding in particular came out of testing the real package against the mock's assumptions, which the original work didn't do.

[Copilot]

Pull request overview

This PR updates the Node-side secret store implementation to lazy-load @napi-rs/keyring so that platforms (or installs) where the native binding cannot be resolved degrade gracefully instead of crashing the Inspector at startup.

Changes:

• Replace the static top-level keyring import with a cached dynamic import that memoizes both success and failure outcomes.
• Add a module “shape check” so missing/changed exports are treated as keychain unavailability with an actionable error.
• Expand integration tests to cover unloadable module and wrong-shape module scenarios, and update Vite’s optimizeDeps exclude comment to reflect the lazy-load behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
core/auth/node/secret-store.ts	Implements cached lazy import + shape check and routes keyring-load failures into the existing degradation contract.
clients/web/src/test/integration/auth/node/secret-store.test.ts	Adds coverage for unloadable keyring imports and wrong-shape exports; asserts error-message branching behavior.
clients/web/server/vite-base-config.ts	Updates the @napi-rs/keyring optimizeDeps exclusion comment to reflect lazy-loading and startup crash avoidance.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Fixed in 0fb3e1c — good catch, and it was a real inconsistency rather than a cosmetic one: secret-store.ts's own enumeration was updated to four when the shape check landed, but this header was left at three. An exhaustive-sounding list that omits the case a describe further down the same file covers is exactly the kind of comment that misleads.

It now reads four, names the wrong-shape case, and explains why the last two need their own describes rather than the shared stub — one needs the import to reject, the other needs it to resolve to a namespace the stub cannot express.

I also swept both files plus vite-base-config.ts for any other stale count; that header was the only one.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

core/auth/node/secret-store.ts:149

• The fallback hint in KeychainUnavailableError only special-cases the native-binding loader error; other non-libsecret causes (notably the new “wrong module shape” guard) will still incorrectly tell users to install libsecret/gnome-keyring. Consider adding a dedicated hint branch for the shape-check error so the advice matches the actual underlying cause.

    const hint = message.includes("Cannot find native binding")
      ? `The @napi-rs/keyring platform package for this OS is missing or unavailable — reinstall the Inspector (for npx, clear the npx cache under your npm cache directory first).`
      : `On Linux, install libsecret / gnome-keyring.`;

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

• This test title says it avoids “returning null secrets”, but the asserted behavior still includes get() returning null (by contract). Rewording the title to emphasize that set() hard-fails (the important distinction) would make the intent clearer for future readers.

  it("treats a namespace without a callable AsyncEntry as unavailable rather than returning null secrets", async () => {

[cliffhall]

Both suppressed comments from the last Copilot pass were right and are fixed in 1917435.

1. Wrong-shape cause got the libsecret advice. This was the sharper of the two: the shape check I added introduced a cause that fell straight through to On Linux, install libsecret / gnome-keyring — advice that cannot fix a packaging mismatch, and that on Windows or macOS points at something which does not exist. It is the same wrong-advice bug the native-binding branch was added to fix, reintroduced by the guard itself one commit later.

Hint selection now lives in a hintFor function where the libsecret line is the explicit fallback rather than the default, so a new cause landing there is a visible choice instead of a silent inheritance. The shape failure is now a distinct KeyringModuleShapeError matched by type rather than by message: it is our own error, so re-parsing text we just wrote would be pointless indirection — the native-binding branch string-matches only because that phrasing comes from the napi-rs loader. New test asserts the shape cause gets the reinstall advice and explicitly not libsecret.

2. Test title overstated what the guard does. Also correct, and it is a claim I had already walked back elsewhere: the guard does not prevent get from returning null — that is the read-tolerance contract and it behaves identically with or without the check. What changes is that set names the real cause instead of reporting AsyncEntry is not a constructor. The comments and commit message said so; the title still said "rather than returning null secrets", so the file contradicted itself. Retitled to hard-fails set ... (get still returns null by contract).

Also updated the KeychainUnavailableError doc to enumerate all three causes and why each needs different advice.

Verification: validate:core + validate:web pass (3728 unit), coverage 4827 passed with secret-store.ts at 98.63 / 97.22 / 100 / 100, tsc -b clean. The nonzero exit on coverage:web is the two pre-existing CONNECTION_CLOSED unhandled rejections in inspectorClient.test.ts, which reproduce identically on the base commit — zero test failures and no threshold errors in the run itself.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

core/auth/node/secret-store.ts:110

• When checkKeyringShape hits the catch (err) path (e.g. a Proxy-based namespace throwing on missing export access), the resulting KeychainUnavailableError will currently fall through to the libsecret/gnome-keyring hint because the cause is not a KeyringModuleShapeError. That produces misleading remediation for a packaging/interop problem, which conflicts with the intent of hintFor to avoid wrong advice. Consider wrapping this catch-path in KeyringModuleShapeError while preserving the original error via cause/message so the hint correctly points to reinstall/packaging.

export class KeyringModuleShapeError extends Error {
  constructor() {
    super(
      "@napi-rs/keyring loaded but did not expose AsyncEntry / findCredentialsAsync",
    );

[cliffhall]

Right again, and fixed in dc65363 — this is the third instance of one mistake, which is the more useful signal than the individual bug.

The shape check fails two ways: the members are absent, or reading them throws (a Proxy-backed namespace, which is what vitest's module mock does). Only the first carried KeyringModuleShapeError; the catch path returned the raw error, so it fell through to install libsecret — the identical wrong-remedy bug as the two commits before it, one branch further along.

Both paths now carry the type. KeyringModuleShapeError takes a detail string plus a standard cause, so the throwing path reads its exports could not be read: <original> — the packaging hint is earned without losing the underlying failure.

What I take from three in a row: the shape was wrong, not just the branch. Making libsecret an explicit fallback in hintFor last round did not stop me from adding a second exit that skipped it. So the invariant is now pinned by tests rather than by structure — one asserts both construction paths produce the same hint, another asserts the throwing-namespace case reaches the user as a packaging problem and explicitly not as libsecret. The next cause that forgets its remediation fails a test instead of needing a reviewer to spot it.

I also verified the negative assertion actually asserts — rejects.not.toThrow can pass vacuously if chained wrong, so I inverted the regex, confirmed the test fails, and restored it.

Verification: validate:core + validate:web pass (3728 unit), coverage 4828 passed, zero test failures, secret-store.ts at 98.63 / 94.73 / 100 / 100 (branches down from 97.22, still well over the 90 gate — the uncovered arm is the non-Error cause stringification). tsc -b clean. The nonzero coverage:web exit is still the two pre-existing CONNECTION_CLOSED rejections that reproduce on the base commit.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.