fix(auth): persist OAuth pre-redirect Network log across the redirect (#1384)

[cliffhall]

Closes #1384.

Stacked on #1383 (#1379). Base is v2/wire-oauth-flow so the diff is just this fix. Rebase onto v2/main after #1383 (and #1382) merge.

Problem

After the OAuth flow landed (#1379), the Network tab showed only the post-redirect auth HTTP (discovery re-run + POST /token). The pre-redirect half — protected-resource + auth-server discovery and the DCR POST /register that run during authenticate() — was lost when the page navigated to the authorization server. (And if a server's tokens are already cached, connect does zero auth fetches, so "only transport" shows.)

Root causes (all three had to be fixed)

1. Ordering — BrowserNavigation set window.location.href before the client's saveSession event fired (OAuthManager calls onBeforeOAuthRedirect after auth() already navigated). The save raced the unload and was dropped. → BrowserNavigation now runs a synchronous beforeNavigate(url) hook immediately before location.href; App wires it through createWebEnvironment to flush the active fetch log to RemoteInspectorClientStorage (keyed by the authId parsed from the auth URL) with a keepalive POST that outlives the unload.
2. Illegal invocation — RemoteInspectorClientStorage defaulted to this.fetchFn = globalThis.fetch and called this.fetchFn(...), which re-binds this so native fetch throws "Illegal invocation" (swallowed by the .catch). This silently broke all session save/load. → default to a wrapper that preserves the global receiver (same gotcha already handled in environmentFactory.ts).
3. Restore race — hydrateFetchRequests replaced the list, so a load resolving after the resuming connect appended live entries would clobber them. → merge restored (older) entries ahead of live ones, dedupe by id.

saveSession also now uses keepalive: true.

Verification

End-to-end against the MCP SDK demo OAuth server (resource :3000, auth :3001, DCR). Before: 10 requests, 3 auth (post-redirect only). After: 14 requests, 5 auth — pre-redirect discovery + POST /register now merged in with post-redirect discovery + /token, alongside transport. The pre-redirect session is confirmed persisted to ~/.mcp-inspector/storage/inspector-session-<authId>.json.

New unit tests: BrowserNavigation beforeNavigate ordering; FetchRequestLogState hydrate merge/dedupe (including the live-append race). npm run validate (1855), test:integration (491), test:storybook (322) all green.

[cliffhall]

Auth and Transport Network Events

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 4s —— View job

---

Claude PR Review

• Read PR diff vs origin/v2/wire-oauth-flow
• Review BrowserNavigation beforeNavigate change in core/auth/browser/providers.ts
• Review RemoteInspectorClientStorage fetch-binding fix in core/mcp/remote/sessionStorage.ts
• Review FetchRequestLogState.hydrateFetchRequests merge/dedupe in core/mcp/state/fetchRequestLogState.ts
• Review App.tsx + environmentFactory.ts wiring
• Review test coverage
• Post final review

---

Summary

Three independent bugs that each had to be fixed for the pre-redirect Network log to survive — ordering, fetch-binding, restore race — are all addressed with surgical, well-justified changes. The diagnostic work in the PR description is excellent, and inline comments explain the why of each non-obvious choice (keepalive lifetime, illegal-invocation, prepend-merge). I'd ship it.

A few small observations below; none are blockers.

---

Strengths

• Each fix is the minimum needed. No collateral refactor; the beforeNavigate hook is purely additive on BrowserNavigation, the storage default is one expression, and hydrateFetchRequests keeps its previous contract for the empty case.
• Comments capture the load-bearing rationale. core/auth/browser/providers.ts:14-23 (why synchronous before location.href), core/mcp/remote/sessionStorage.ts:34-39 (why a wrapper, not the bare reference), and core/mcp/state/fetchRequestLogState.ts:159-164 (why merge, not replace) all answer the obvious "why is this written this way?" follow-up.
• Tests target the exact regression surface: BrowserNavigation runs beforeNavigate before location.href (providers.test.ts:83-105); hydrate merges and dedupes against a live append (fetchRequestLogState.test.ts:205-244); hydrate is a no-op when destroy fires first (fetchRequestLogState.test.ts:272-296). Good coverage of the race semantics.

---

Observations / minor suggestions

1. Double-save path is now live but undocumented

InspectorClient's OAuthManager still fires saveSession via its onBeforeOAuthRedirect callback (core/mcp/inspectorClient.ts:239-243), and FetchRequestLogState still subscribes to that event (core/mcp/state/fetchRequestLogState.ts:106-119). With this PR, App.tsx's new onBeforeOAuthRedirect also writes the same payload under the same authId synchronously before navigation. Both paths now run on every OAuth redirect.

It's harmless (last-writer-wins, payload identical) and the comment-thread already notes the original saveSession event arrived "too late." But a future reader will wonder why both paths exist. A one-line note on either FetchRequestLogState.onSaveSession or App.tsx's onBeforeOAuthRedirect ("BrowserNavigation's beforeNavigate is the primary flush; this listener is the backstop for non-redirect save points like token refresh") would close the loop.

2. Keepalive 64KB comment is path-specific, but saveSession is general

core/mcp/remote/sessionStorage.ts:81-84 says "the payload is the pre-redirect log only (a few small auth entries, no captured bodies), comfortably under keepalive's 64KB cap." That's true for the new direct call from onBeforeOAuthRedirect. But the same method is also called from FetchRequestLogState's saveSession listener, where the payload is the entire current fetchRequests array — which in a long session could plausibly exceed 64KB (request URLs + headers add up). The comment will read as a guarantee, but it only holds for one of the call sites.

Suggested rewording: keep the "why keepalive" rationale; soften the size claim ("the pre-redirect log payload is small; longer sessions may exceed keepalive's 64KB cap and silently drop — acceptable since the log is non-essential").

3. No direct test for the fetchFn default wrapper

All clients/web/src/test/core/mcp/remote/sessionStorage.test.ts tests pass an explicit fetchFn, so the default-wrapper branch added at core/mcp/remote/sessionStorage.ts:39 isn't exercised. Since the original bug ("Illegal invocation") was silently swallowed by .catch(() => {}) in callers, a regression here would again be invisible. A one-liner that constructs new RemoteInspectorClientStorage({ baseUrl: "..." }) without fetchFn, stubs globalThis.fetch, and asserts the stub is called with the expected URL would lock this in. Fix this →

4. onBeforeOAuthRedirect swallows save errors

App.tsx:499-501 and core/mcp/state/fetchRequestLogState.ts:115-117 both .catch(() => {}) the save. Before this PR, losing the pre-redirect log was the user-visible bug. The codebase clearly prefers silent failures (pino is silent by default), but it might be worth at least a logger.warn so a future regression surfaces somewhere instead of being completely invisible. Optional.

5. Nit: setupClientForServer deps churn

App.tsx:615-627 has the full set of state managers in the dep list, so setupClientForServer's identity changes on essentially every state setter. That in turn re-runs the OAuth-callback effect at App.tsx:638-703 — but the oauthCallbackHandledRef.current guard short-circuits it. So functionally fine, just a bit wasteful. Not this PR's problem (pre-existing pattern); flagging only because the new onBeforeOAuthRedirect dep extends the chain slightly. The new callback itself is stable (useCallback([sessionStorageAdapter]) and sessionStorageAdapter is memoized with []), so it doesn't add new churn.

---

Verification

PR description reports npm run validate (1855), test:integration (491), test:storybook (322) all green — combined with the new tests targeting the exact regression points, that's adequate.
• Branch: v2/oauth-network-persist

[cliffhall]

Thanks for the review — addressed the actionable items in 89c06ec.

1. Double-save path undocumented — Added a comment on FetchRequestLogState's saveSession listener spelling it out: BrowserNavigation's beforeNavigate hook is the primary flush for the redirect case (synchronous before navigation → keepalive survives the unload); this listener is the backstop that may lose the navigation race, and is harmless when it duplicates (last-writer-wins on an identical payload under the same authId). It also remains the save path for any future non-redirect saveSession caller (e.g. token refresh).

2. Keepalive 64KB comment over-promised — Reworded. Kept the "why keepalive" rationale but made the size caveat general: the method is also reachable from the listener with the full session log, so a long session could exceed the 64KB cap and drop silently — acceptable since the persisted log is best-effort, not load-bearing.

3. No test for the fetchFn default wrapper — Added one: constructs RemoteInspectorClientStorage without a fetchFn, stubs globalThis.fetch, and asserts the default wrapper calls it with the expected URL and resolves. This locks in the Illegal-invocation fix that callers otherwise swallow via .catch(() => {}).

4. Swallowed save errors (optional) — Left as-is. The codebase convention is silent failures (pino silent by default), and item 3's wrapper test now guards the specific invisible-regression risk that motivated this. Happy to add a logger.warn if you'd prefer it tracked.

5. setupClientForServer dep churn (nit) — Acknowledged; pre-existing pattern, guarded by the ref/path short-circuits, and the new onBeforeOAuthRedirect callback is stable (useCallback([sessionStorageAdapter]), memoized []). No change.

Note: base is now v2/main (#1383 merged), changes propagated down to #1387. Full suite green on this branch (validate 1861) and the tip (validate 1870, integration 495, storybook 322).