refactor(auth): remove Zustand from OAuth persistence, clean up storage IO (#1549)

[BobDickinson]

Depends on #1609 / Blocked by #1609 (retarget to v2/main when #1609 merged)

Closes #1549

---

Summary

Replace the Zustand persist + adapter stack for OAuth runtime state with a direct OAuthStorageBase + persist backend model. OAuth state is written as plain JSON { servers, idpSessions } to ~/.mcp-inspector/storage/oauth.json (file, remote HTTP, or sessionStorage for tests). All OAuthStorage getters are async and auto-load; setters auto-persist. load() remains optional preload (OAuth callback fail-fast on web). Removes zustand from the repo and deletes core/storage/adapters/*.

---

Why

Zustand persist added an extra in-memory layer, { state, version } envelopes on write, and awkward semantics (sync getters vs async HTTP/file I/O) that complicated sign-out, callback resume, and multi-consumer sharing. #1548 wired web through RemoteOAuthStorage; this PR completes the storage refactor so production code no longer depends on Zustand for OAuth at all.

---

Architecture (after)

OAuthStorage (interface)
    └── OAuthStorageBase
            ├── OAuthMemoryStore (in-memory { servers, idpSessions })
            └── OAuthPersistBackend (oauth-persist.ts)
                    ├── createFileOAuthPersistBackend      → NodeOAuthStorage
                    ├── createRemoteOAuthPersistBackend    → RemoteOAuthStorage
                    └── createSessionOAuthPersistBackend     → BrowserOAuthStorage (tests/reference)

• Write: plain { servers, idpSessions }
• Read: accepts legacy { state, version } envelope (old Zustand persist); promotes inner payload; migrate-on-write on next save
• SDK bridge: BaseOAuthClientProvider.prepareForAuth() caches scope for sync clientMetadata.scope; codeVerifier() is async (SDK already awaits it)

Client	Store
Web	getWebRemoteOAuthStorage() → RemoteOAuthStorage → /api/storage/oauth → oauth.json
CLI / TUI	NodeOAuthStorage → oauth.json

---

Key changes

Core

• Add core/auth/oauth-persist.ts — parse/serialize blob, file / remote / session backends
• Refactor OAuthStorageBase — all getters async + ensureLoaded(); mutators await persist()
• Refactor OAuthMemoryStore — plain mutable store (no Zustand)
• BaseOAuthClientProvider — prepareForAuth(), async codeVerifier() / getServerMetadata()
• oauthManager, connection-state, idpOidc, EMA paths — await storage getters
• Remove getOAuthStore() from public API; add @internal resetNodeOAuthStorageCache() for tests
• clearAllOAuthClientState() reads server URLs from file backend (not Zustand store)
• Delete core/storage/adapters/{file-storage,remote-storage,index}.ts

Dependencies / build

• Remove zustand from root, web, cli, tui package.json and lockfiles
• Drop Zustand aliases from vitest.shared.mts, tsup/tsconfig paths

Tests

• New oauth-persist.test.ts (legacy envelope promotion, plain JSON serialize)
• Storage unit/integration tests: async getters, auto-load, no getOAuthStore
• Provider / oauthManager / connection-state / EMA mocks → mockResolvedValue
• Remote storage test: getters load automatically (no sync-before-load() pattern)

Docs

• AGENTS.md — OAuth persist backends, no Zustand in tree
• specification/v2_auth_ema.md — OAuth persistence (Remove Zustand file-store wrappers; auto-convert to plain JSON on read #1549 done), checklist updated
• specification/v2_storage.md, v2_servers_file.md, v2_scope.md, and related auth specs aligned

---

API notes for reviewers

Before	After
Sync getCodeVerifier, getScope, …	All getters return Promise<…>; call await
Must await load() before sync reads	Getters/setters load automatically; load() optional at boundaries
getOAuthStore()	Removed (tests use NodeOAuthStorage + resetNodeOAuthStorageCache)
Zustand { state, version } on write	Plain JSON only; legacy envelope still read

Web OAuth callback still calls await webOAuthStorage.load() before resumeAfterOAuth for fail-fast if /api/storage/oauth is unreachable — not because getters require preload.

---

Migration / compatibility

• On disk: existing oauth.json files with { state, version } continue to work; first save rewrites as plain JSON
• Browser sessionStorage: no automatic migration from pre-Web: migrate auth store to shared /store API (RemoteOAuthStorage parity with TUI/CLI) #1548 session blobs; web uses shared file-backed store (Web: migrate auth store to shared /store API (RemoteOAuthStorage parity with TUI/CLI) #1548)
• No user action required for typical CLI/TUI/web local dev paths

[BobDickinson]

@claude review

[claude]

Claude finished @BobDickinson's task in 2m 38s —— View job

---

Code review — remove Zustand from OAuth persistence (#1612)

Reviewed against origin/v2/1548-web-remote-oauth-storage. Overall this is a clean, well-structured refactor: the OAuthStorageBase + OAuthPersistBackend split is much clearer than the Zustand persist stack, the async-getter/auto-load contract is applied consistently, and the legacy-envelope read path plus test coverage (oauth-persist.test.ts) are nicely done. The load-once memoization (loadPromise) and pre-mutation replace() correctly avoid clobbering in-memory writes. A few things worth addressing before merge, none of them blocking correctness for the common sequential flow.

---

1. Remote backend has no write serialization → possible lost update (medium)

core/storage/store-io.ts chains file writes per path (pendingWrites), so concurrent saveX() calls to the file backend are serialized and last-write-wins with full state. The remote backend (core/auth/oauth-persist.ts:136) has no equivalent — each write() is a bare POST.

OAuthStorageBase.persist() captures this.memory.snapshot() at call time (oauth-storage.ts:57-59). If two mutators overlap:

saveA: mutate(A) → POST body {A}      (captured first)
saveB: mutate(B) → POST body {A,B}    (captured second)

If {A,B} lands and then the older {A} POST lands after it (network reordering), server state reverts to {A} and B is lost. This can't happen on the file backend but is unguarded for remote.

Since the fix belongs at a backend-agnostic layer, consider serializing persist() in OAuthStorageBase with a small promise chain (a persistQueue mirroring pendingWrites), so ordering holds regardless of backend. In practice OAuth ops within one flow are sequential, so this is an edge case — but the file backend already defends against it, so the remote path should too. Fix this →

2. clients/launcher/package-lock.json still pins zustand (minor, verify CI)

The PR removes zustand from root/web/cli/tui but the launcher lockfile still lists "zustand": "^5.0.13" in its embedded sibling-package entries (lines 41, 70, 113). Launcher's own package.json never depended on zustand, so there's no runtime impact — but the lock is now out of sync with the web/cli/tui package.json it mirrors. Re-run npm install at the root and commit the regenerated launcher lock so a strict npm ci doesn't flag it. Fix this →

3. Redundant double JSON round-trip in remote read (nit)

oauth-persist.ts:130-134: res.json() parses the body, then parseOAuthPersistBlob(JSON.stringify(store)) re-serializes and parseStore() re-parses it. Consider making parseOAuthPersistBlob accept unknown (it already type-guards with isRecord) so the remote path can pass the parsed object directly and skip the stringify+reparse. The file/session paths naturally have a string.

4. clearAllOAuthClientState reads the file twice (minor)

storage-node.ts:48-56 opens a fresh createFileOAuthPersistBackend(...).read() to enumerate server URLs, then calls storage.clear(url) — which auto-loads (another read) and persists per URL. Functionally correct, but you could enumerate from the already-cached storage (e.g. expose a keys/snapshot accessor) to avoid the extra file read and the two independent views of the file. Test-only path, so low priority.

---

Minor observations (no action required)

• NodeOAuthStorage's constructor is public and pulls shared memory from the module cache while keeping a per-instance loaded flag. The cached getNodeOAuthStorage() is the intended entry; a directly-constructed second instance would re-read() on first access and replace() shared memory — safe only because persistence is always awaited. Consider a doc note that direct construction is for tests.
• prepareForAuth() → cachedScope bridge for the SDK's sync clientMetadata.scope is a clean solution; saveScope() correctly keeps cachedScope in sync (providers.ts:211-214).
• The enterpriseManaged: true merge in saveTokens (oauth-storage.ts:145-148) only ever sets the flag, never clears it — matches the "sticky until server state cleared" semantics used by clearEnterpriseManagedResourceServers, so this is intentional.

---

Nice cleanup overall — the storage layer is materially simpler and the async contract is applied uniformly across oauthManager, connection-state, and idpOidc. Items #1 and #2 are the ones I'd resolve before merge.
· branch v2/1549-oauth-storage-no-zustand

[BobDickinson]

Addressed review items 1–3:

1. Persist serialization — OAuthStorageBase now chains persist() via a per-instance promise queue (mirrors pendingWrites in store-io.ts), so concurrent mutators write in order on remote as well as file backends.
2. Launcher lockfile — regenerated clients/launcher/package-lock.json; stale zustand entries in the extraneous sibling-package snapshots are gone.
3. Remote read round-trip — parseOAuthPersistBlob accepts string | unknown; remote backend passes the parsed res.json() object directly. Added a unit test for the object path.

Deferred

4. clearAllOAuthClientState double read — test-only helper, low priority.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 37s —— View job

---

Code review — remove Zustand from OAuth persistence (#1612)

Re-reviewed against origin/v2/main, focusing on the three items from the first review and scanning the refactor for new issues. The follow-up commit (af174666) resolves all the actionable items cleanly. This is a solid, well-scoped refactor — the storage layer is materially simpler and the async/auto-load contract is applied uniformly.

---

Verification of prior review items

• Progress notifications #1 Remote write serialization ✅ OAuthStorageBase.persist() now chains through a per-instance persistQueue (oauth-storage.ts:59-67). The snapshot is captured synchronously at persist() entry, before awaiting the prior write — so because mutations are synchronous, each queued write carries the full cumulative state and the last write wins in order on both file and remote backends. Rejections are safe: the caller awaits tracked, and each subsequent persist() attaches its own .catch(() => {}) to the prior promise, so no unhandled rejection and one failed write doesn't wedge the queue. Correct.
• Request logging #2 Launcher lockfile ✅ grep -rn zustand across all five lockfiles now returns nothing.
• Completion requests #3 Remote read round-trip ✅ parseOAuthPersistBlob accepts string | unknown and the remote backend passes the parsed res.json() object directly (oauth-persist.ts:41-53, 138-142). No more stringify+reparse.

---

New observations

1. Redundant Object.keys guard in remote read() can throw on a null body (minor / robustness)

oauth-persist.ts:138-142:

const store = await res.json();
if (Object.keys(store as object).length === 0) {
  return null;
}
return parseOAuthPersistBlob(store);

This guard is now redundant and slightly fragile:

• parseOAuthPersistBlob({}) already returns null (no state/version, no servers/idpSessions), so the empty-object case is handled downstream — the server's missing-file response (c.json({}, 200), server.ts:922) doesn't need a special case here.
• If the store file ever contains literal null (corruption, or a hand-edited file), the GET returns null and Object.keys(null as object) throws a TypeError before reaching parseOAuthPersistBlob — which would otherwise have returned null safely (it guards raw === null).

Dropping the guard and passing store straight to parseOAuthPersistBlob(store) is both simpler and strictly more robust. Fix this →

2. clearAllOAuthClientState still double-reads the file (deferred #4 — fine to leave)

storage-node.ts:48-56 reads server URLs via a fresh createFileOAuthPersistBackend(...).read(), then calls storage.clear(url) per URL (each auto-loads + persists). Correct because persistence is always awaited, so disk is authoritative; it's a test-only helper. Agree with deferring — noting only that a snapshot/keys accessor on OAuthStorageBase would let it enumerate from the already-cached instance and avoid the second read + the two independent file views.

---

Confirmed correct (spot checks)

• Snapshot immutability: OAuthMemoryStore.snapshot() is a shallow copy, but setServerState/setIdpSession do immutable replacement ({...prev, ...updates} into a new map), so a captured snapshot never observes later mutations — the shallow copy is safe for the queued-write pattern.
• Async fan-out: connection-state.ts, idpOidc.ts, and oauthManager.ts correctly await every getter that became a Promise; getEmaFlowConfig() was promoted to async and all call sites await it. The removed storage.load() calls in idpOidc/oauthManager are safe since getters auto-load, and the web callback keeps its explicit load() for fail-fast.
• SDK bridge: prepareForAuth() → cachedScope for the SDK's sync clientMetadata.scope, with saveScope() keeping it in sync, is a clean solution. oauthManager.ts:142-143 uses provider.prepareForAuth() + provider.scope rather than re-reading storage — good.
• No dangling references to the removed getOAuthStore / barrel getStateFilePath exports.

---

Nice cleanup. Item 1 is the only thing I'd consider before merge, and it's minor — everything from the first review is resolved.
· branch v2/1549-oauth-storage-no-zustand

[cliffhall]

Addressed review item #1 (redundant empty-object guard in remote read()):

• Removed the Object.keys(store).length === 0 check in createRemoteOAuthPersistBackend.read() and now pass the parsed res.json() straight to parseOAuthPersistBlob(store). parseOAuthPersistBlob already returns null for {} (the server's missing-file c.json({}, 200) response) and guards raw === null, so the empty-object case is handled downstream and the fragile Object.keys(null) TypeError is gone. Simpler and strictly more robust. (54d5b18b)

#2 (clearAllOAuthClientState double-read) — leaving as deferred per your note; it's a test-only helper and correct as-is. Can revisit with a snapshot/keys accessor on OAuthStorageBase in a follow-up if worthwhile.

@claude review

[claude]

Claude finished @cliffhall's task in 52s —— View job

---

Code review — remove Zustand from OAuth persistence (#1612)

Re-reviewed commit 54d5b18b (drop redundant empty-object guard in remote read()) against origin/v2/main.

• Read the changed code
• Verify prior review item Progress notifications #1 (empty-object guard)
• Scan for regressions

---

Verification

#1 — Redundant empty-object guard removed ✅ createRemoteOAuthPersistBackend.read() (core/auth/oauth-persist.ts:135-139) now passes the parsed res.json() straight to parseOAuthPersistBlob(store). This is both simpler and strictly more robust:

• parseOAuthPersistBlob guards raw === null || raw === undefined up front (:46-48), so a literal null body no longer hits Object.keys(null) → TypeError.
• {} (the server's c.json({}, 200) missing-file response) falls through the record check with no state/version and no servers/idpSessions, returning null (:56-64).
• The 404 short-circuit (:129-130) is preserved.

The doc comment accurately explains the reasoning. Good, focused fix.

#2 — clearAllOAuthClientState double-read — Agree with leaving deferred; it's a test-only helper and correct because persistence is always awaited (disk is authoritative). A snapshot/keys accessor on OAuthStorageBase remains a reasonable future cleanup.

Note (non-blocking)

The oauth-persist.test.ts unit tests cover null, plain JSON, the already-parsed object path, and the legacy envelope — but there's no explicit assertion that parseOAuthPersistBlob({}) returns null, which is exactly the branch this fix now leans on for the remote missing-file path. It's covered transitively (the remote unit tests exercise the object path, and the servers/idpSessions absent branch is hit), but a one-line expect(parseOAuthPersistBlob({})).toBeNull() would document the contract this fix depends on. Optional.

---

All actionable items from both prior reviews are now resolved. The refactor is clean, the async/auto-load contract is applied uniformly, and this last change makes the remote read path both simpler and more defensive. LGTM. ✅
· branch v2/1549-oauth-storage-no-zustand

[cliffhall]

Added the optional test from your note (cdeb2316): expect(parseOAuthPersistBlob({})).toBeNull() in oauth-persist.test.ts, with a comment tying it to the server's c.json({}, 200) missing-file response that the remote read() path now leans on. That closes out both prior reviews.

@claude review

[claude]

Claude finished @cliffhall's task in 49s —— View job

---

Code review — remove Zustand from OAuth persistence (#1612)

Re-reviewed commit cdeb2316 (assert parseOAuthPersistBlob({}) returns null) against origin/v2/main.

• Read the new test assertion
• Verify it matches the documented contract
• Scan for regressions

---

Verification

Optional note from prior review addressed ✅ oauth-persist.test.ts:12-18 now asserts expect(parseOAuthPersistBlob({})).toBeNull(), with a comment tying it to the server's c.json({}, 200) missing-file response.

I traced the assertion against the implementation (core/auth/oauth-persist.ts:43-65) and it exercises exactly the branch the remote read() path depends on:

• {} is not null/undefined, so it passes the early guard (:46).
• It's a non-string, so parsed = raw — the already-parsed-object path from res.json() (:50).
• isRecord({}) is true (:52), so it doesn't bail there.
• No state/version envelope (:56) and no servers/idpSessions keys (:60), so it falls through to return null (:64). ✅

This is the precise fall-through that 54d5b18b leaned on when it dropped the Object.keys(store).length === 0 guard, so the test correctly documents and locks in the contract. Good, focused addition.

Regression scan

No production code touched — the commit is test-only (+7 lines). No impact on the async/auto-load contract, the persistQueue serialization, or the remote read path.

---

All items from all three prior reviews are now resolved:

• Progress notifications #1 Remote write serialization → persistQueue in OAuthStorageBase ✅
• Request logging #2 Launcher lockfile zustand entries removed ✅
• Completion requests #3 Remote read double round-trip eliminated ✅
• Progress notifications #1 (2nd pass) Redundant/fragile empty-object guard removed ✅
• Optional note parseOAuthPersistBlob({}) contract now explicitly tested ✅

The refactor is clean, well-scoped, and the storage layer is materially simpler than the Zustand persist stack. LGTM. ✅
· branch v2/1549-oauth-storage-no-zustand

[cliffhall]

Ran a browser smoke test of OAuth persistence (Playwright + headless Chromium, against a local DCR OAuth MCP test server) and it surfaced a runtime regression: the browser's RemoteOAuthStorage went through oauth-persist.ts, which value-imported the Node-only store-io.js (atomically → stubborn-fs → node:process.getuid). That pulled Node built-ins into the browser bundle and the web app crashed to a blank page at runtime. It slipped past CI because unit/integration tests run in node/happy-dom and smoke:web only checks the served HTML — nothing executes the app in a real browser.

Fixed in 1e6a2825, following the existing store-id.ts precedent:

• Pure JSON (de)serializers split into Node-free core/storage/store-serialize.ts (store-io.ts re-exports them, so existing importers are unaffected).
• Node-only createFileOAuthPersistBackend moved to core/auth/node/oauth-persist-file.ts; storage-node.ts + the adapters test updated.
• oauth-persist.ts now imports only the Node-free serializers → browser bundle no longer pulls node:fs/atomically.

Tests added: a source-level guard that oauth-persist.ts never imports store-io/node:/atomically; direct unit tests for the isomorphic remote + session backends; OAuthMemoryStore unit tests (incl. replace() fallbacks); and the clearAllOAuthClientState no-file path. Full local coverage gate is green again.

Verified end-to-end in a real browser: DCR + PKCE flow completes, tokens persist to the remote store, and after a full page reload the server reconnects with no re-authentication (GET /api/storage/oauth shows the same access_token before and after reload).

@claude review

[claude]

Claude finished @cliffhall's task in 1m 25s —— View job

---

Code review — keep oauth-persist browser-safe (1e6a2825)

• Read the fix commit diff (1e6a2825)
• Verify oauth-persist.ts no longer pulls in Node built-ins
• Check the module split (store-serialize.ts, oauth-persist-file.ts)
• Review the new/updated tests
• Scan for regressions

---

Reviewed 1e6a2825 against origin/v2/main. This is a correct, minimal fix for a real runtime regression, and it follows the established store-id.ts precedent cleanly. LGTM. ✅

Root cause & fix verified

The break was a value import: oauth-persist.ts value-imported createFileOAuthPersistBackend (and the backend value-imported store-io.js → node:fs/atomically → stubborn-fs → node:process.getuid). Because the browser's RemoteOAuthStorage chains through oauth-persist.ts, Vite pulled Node built-ins into the browser bundle → blank page. The fix removes that edge:

• core/storage/store-serialize.ts (serializeStore/parseStore) — pure, Node-free; store-io.ts re-exports them so existing importers are unaffected (store-io.ts:17). Same pattern already used for validateStoreId/store-id.ts.
• createFileOAuthPersistBackend moved to core/auth/node/oauth-persist-file.ts; oauth-persist.ts now imports only ../storage/store-serialize.js (oauth-persist.ts:12).

Full browser chain confirmed Node-free

I traced every value import reachable from the browser's RemoteOAuthStorage, not just the one file the guard test scans:

• remote/storage-remote.ts → oauth-storage.ts, store.ts, oauth-persist.ts (all value imports)
• oauth-storage.ts → only type imports + oauth-persist.ts
• store.ts → only a type import from oauth-persist.ts
• oauth-persist.ts → store-serialize.ts only

grep confirms the only remaining reference to store-io under core/auth/ outside node/ is a doc comment, and oauth-persist-file.ts is imported solely by the Node storage-node.ts. No Node built-in is reachable from the browser entry.

Tests

• oauth-persist-browser-safe.test.ts — source-level regex guard that oauth-persist.ts doesn't import store-io.js/node:*/atomically and does import store-serialize.js. Cheap and pins exactly the regression that occurred.
• Restored isomorphic remote+session backend unit coverage in oauth-persist.test.ts, plus OAuthMemoryStore (store.test.ts) and the clearAllOAuthClientState no-file path — good, since coverage of oauth-persist.ts dropped when the file backend moved out.

Observations (non-blocking)

1. The source guard is direct-only, not transitive. It scans oauth-persist.ts's own import list, so a Node dep pulled in through store-serialize.ts, store.ts, or oauth-storage.ts (or a future edge added to them) would slip past it — the same class of bug, one hop removed. The chain is clean today (verified above), so this is a hardening idea, not a defect. If you want defense-in-depth, an actual browser/jsdom import() of RemoteOAuthStorage (or a Vite/rollup bundle assertion) would catch transitive regressions that regex can't. Given the e2e Playwright verification and the direct guard, fine to leave. Strengthen guard →

2. CI gap acknowledged in the commit message is the real lesson here — unit/integration run in node/happy-dom and smoke:web only asserts the served HTML, so nothing executes the app in a real browser. The regex guard closes this hole; a headless-browser boot smoke would close the category. Worth a follow-up issue if the team wants it, but out of scope for this PR.

The refactor is well-scoped and the browser-safety boundary is now explicit and documented in each module header. Nice fix.
· branch v2/1549-oauth-storage-no-zustand

[cliffhall]

Addressed the non-blocking notes:

• Progress notifications #1 (guard is direct-only) — Done in 4c30dc6e. The browser-safe guard is now transitive: it scans every module reachable from RemoteOAuthStorage (storage-remote.ts, oauth-storage.ts, store.ts, oauth-persist.ts, store-serialize.ts) for store-io/node:*/atomically imports, so a Node dep added one hop away is caught too — not just a direct import in oauth-persist.ts.
• Request logging #2 (headless-browser boot smoke in CI) — Agree it's the real category fix and out of scope here; I'll open a follow-up issue on the v2 board for a browser boot smoke so the whole class is covered in CI.

@claude review

[claude]

Claude finished @cliffhall's task in 1m 32s —— View job

---

Code review — transitive browser-safe guard (4c30dc6e)

• Read the new transitive guard test
• Verify it covers the full RemoteOAuthStorage import chain
• Scan for regressions

Reviewed 4c30dc6e against origin/v2/main. The commit is test-only (+42/-23, single file) — it extends oauth-persist-browser-safe.test.ts from scanning one file to scanning every module reachable from the browser's RemoteOAuthStorage. LGTM. ✅

---

Verification

I traced the value-import graph from the web entry (lib/remoteOAuthStorage.ts → remote/storage-remote.ts) and confirmed the BROWSER_REACHABLE list is complete for the current graph:

Module	Value imports reachable from it
auth/remote/storage-remote.ts	oauth-storage.js, store.js, oauth-persist.js
auth/oauth-storage.ts	none (storage.js, store.js, oauth-persist.js are all type-only)
auth/store.ts	none (only a type import of oauth-persist.js)
auth/oauth-persist.ts	../storage/store-serialize.js
storage/store-serialize.ts	none (leaf; pure JSON.stringify/JSON.parse, no imports)

Every value-reachable module is in the list, and store-serialize.ts is genuinely Node-free. The it.each runs each file through the FORBIDDEN matchers (store-io, ^node:, ^atomically), and the extra assertion pins that oauth-persist.ts sources its (de)serializers from store-serialize.js. The regex matches import type … from too, so it's conservative (a type-only Node import — erased at build — would still be flagged), which is fine.

Regression scan

No production code touched — no impact on the async/auto-load contract, persistQueue, or the remote read path.

One observation (non-blocking, matches the note being addressed)

The guard is transitive by manual enumeration, not automatically transitive. It walks a hardcoded list that someone traced from the current graph — it does not follow imports itself. So if a new value import is added to, say, oauth-storage.ts pointing at a new foo.ts that pulls node:fs, the guard wouldn't catch it because foo.ts isn't in BROWSER_REACHABLE. That's the same class of gap pushed one hop further out, and it's an inherent limitation of a static list rather than a real defect. As noted, the true category fix is a headless-browser boot smoke in CI (the planned follow-up) — a static list can't replace actually loading the bundle. Fine to leave; the e2e Playwright verification is the real backstop today.

If you ever want the list to be self-maintaining, the guard could instead start from storage-remote.ts, parse its value imports (skipping import type), and recurse — but that's more machinery than this hardening step warrants.

---

All items from all four prior review passes are now resolved:

• Progress notifications #1 Remote write serialization → persistQueue ✅
• Request logging #2 Launcher lockfile zustand entries removed ✅
• Completion requests #3 Remote read double round-trip eliminated ✅
• Progress notifications #1 (2nd) Redundant empty-object guard removed ✅
• parseOAuthPersistBlob({}) contract test added ✅
• Browser-safety regression (1e6a2825) fixed + now guarded transitively ✅

Clean, well-scoped refactor. LGTM. ✅
· branch v2/1549-oauth-storage-no-zustand

[cliffhall]

Follow-up issue for the headless-browser boot smoke (observation #2): #1615 — added to the v2 board (Backlog). Thanks for the thorough passes — all items resolved. ✅

[cliffhall]

Pushed 98517c27 to fix the CI failure (a tsc -b type error in the remote-backend write() test — mock.calls[0][1].body on an untyped mock; now captured via a typed vi.fn<typeof fetch> implementation). CI is green again (Validate + integration + smokes + Storybook all pass).

@claude review

[claude]

Claude finished @cliffhall's task in 51s —— View job

---

Code review — fix type error in remote backend write() test (98517c27)

• Read the fix commit diff (98517c27)
• Verify it resolves the tsc -b type error correctly
• Confirm the assertion still exercises the same behavior
• Check CI status

---

Reviewed 98517c27 against origin/v2/main. The commit is test-only (+7/-4, single file — oauth-persist.test.ts) and correctly fixes the CI tsc -b failure. LGTM. ✅

Verification

The root cause is accurate: ok.mock.calls[0][1].body indexes into an untyped mock's call tuple. With the project's strict indexing, an untyped vi.fn(async () => …) gives a call-args tuple of [], so [0] is possibly-undefined and [1] is out of range — a compile error under tsc -b (which CI's validate runs, but vitest's transpile-only path does not, which is why it slipped through locally).

The fix is the idiomatic one:

• vi.fn<typeof fetch>((_input, init) => …) types the mock against the real fetch signature, so init is RequestInit | undefined and the fetchFn: ok assignment no longer needs the as unknown as typeof fetch double-cast.
• Capturing init?.body inside the implementation (capturedBody) sidesteps mock.calls indexing entirely.
• init?.body as string | undefined is a sound narrowing — RequestInit.body is BodyInit | null | undefined, and the backend always passes a serialized string (oauth-persist.ts write()), so the downstream JSON.parse is exercising exactly what it did before.

The behavioral assertions are unchanged: it still verifies the POST method via toHaveBeenCalledWith and that the request body round-trips to SNAPSHOT. JSON.parse(capturedBody ?? "") would throw on "" if the body were never captured, but the preceding await backend.write(SNAPSHOT) guarantees the implementation ran, so capturedBody is set — the ?? "" is just there to satisfy the string type for JSON.parse.

Observation (non-blocking)

The other backends in this file still use the as unknown as typeof fetch cast on inline arrow mocks (:110, :119-128, :154-155, :167-176, etc.). Those don't index mock.calls, so they compile fine and don't need the fix — but if you wanted uniformity, vi.fn<typeof fetch>(…) would drop the casts there too. Purely cosmetic; fine to leave.

CI

Green — the single workflow run (Validate + integration + smokes + Storybook) completed success.

---

All items from all prior review passes remain resolved, and this last commit clears the CI type error without touching any production code. LGTM. ✅
· branch v2/1549-oauth-storage-no-zustand