refactor(core): apply v1.5-port cleanups deferred from #1306

[cliffhall]

Closes #1309.

Five mechanical cleanups deferred from the v1.5 runtime port (#1306). Each is independent; all together drop ~210 lines and re-tighten TS strictness.

Summary

1. Consolidate OAuthStorage implementations. Browser/Node/RemoteOAuthStorage were three ~130-line copies that differed only in their adapter. Body lives once in a new OAuthStorageBase (core/auth/oauth-storage.ts); each environment file is now a 5-line subclass that wires its adapter and calls super. Public API (new BrowserOAuthStorage(), new NodeOAuthStorage(path?), new RemoteOAuthStorage(opts)) is unchanged, so all existing call sites and tests work untouched.

2. Re-enable erasableSyntaxOnly. Rewrote the seven constructor(private foo: T) sites enumerated in the issue, plus two extras the issue missed (samplingCreateMessage.ts / elicitationCreateMessage.ts), as explicit field declarations + assignments. The temporary erasableSyntaxOnly: false override + TODO in tsconfig.app.json is gone.

3. Logger consistency in inspectorClient.ts. Swapped the one console.error (roots/list_changed failure path) to this.logger.error({ error }, "…") to match the rest of the file.

4. crypto.randomUUID() everywhere. The five message-tracking + sampling/elicitation ID sites that used ${Date.now()}-${Math.random()} now use crypto.randomUUID() directly (with semantic prefixes preserved on the sampling/elicit cases).

5. Drop the streamSSE cast in core/mcp/remote/node/server.ts. Pinned the hono generic at new Hono<Env>() per the issue. Root cause of the streamSSE(c as unknown as …) cast turned out to be a tsconfig path-alias bug, not a hono generic mismatch: the "hono/*": ["./node_modules/hono/*"] wildcard did not honor hono's subpath exports map, so hono/streaming fell through to the root-hoisted hono@4.12.12 (peerDep of the SDK) while hono itself resolved to clients/web/hono@4.12.18. Two physically separate packages → TS rejected the (otherwise identical) Context types. Replacing the wildcard with an explicit "hono/streaming" entry in both tsconfig.app.json and tsconfig.test.json makes both imports resolve to the same package; the cast is no longer needed.

Test plan

• npm run validate from clients/web/ (format + lint + build + unit + integration coverage)
• npm run test:storybook (67 stories / 298 play-tests)
• OAuth unit & integration suites still pass after the storage consolidation
• remote-transport.test.ts (47 tests) still passes after the streamSSE cast removal

Out of scope

The acceptance-criteria "out of scope" items from #1309 (BaseOAuthClientProvider dispatcher refactor, generated core/mcp/version.ts, vite alias trimming) remain deferred.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

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

---

Claude PR Review

• Read the PR diff vs origin/v2/main
• Examine OAuthStorageBase consolidation
• Examine erasableSyntaxOnly rewrites
• Examine logger + randomUUID changes
• Examine hono streamSSE + tsconfig path-alias fix
• Post review feedback

---

Summary

Clean, focused PR. Five independent mechanical cleanups, each easy to verify in isolation. The OAuthStorageBase extraction is the clear highlight — it removes ~390 lines of near-identical boilerplate across the three storage files without changing any public API, so the existing per-subclass test suites continue to exercise the full behavior. The erasableSyntaxOnly re-enablement is a real win — it restores a TS-strictness knob that was lowered explicitly as a temporary measure in #1306, with a TODO pointing at this issue. The path-alias root-causing of the hono streamSSE type problem is nice detective work; the explicit "hono/streaming" entry in both tsconfig.app.json and tsconfig.test.json is the right fix.

Approving in spirit (bot can't formally approve). A few small observations and nits below — none block merge.

---

Observations

1. clearClientInformation has a stranded call that was copied into the base. core/auth/oauth-storage.ts:60 — this.store.getState().getServerState(serverUrl); with the return value discarded. Carried over verbatim from the three originals, so not introduced by this PR, but consolidation is the natural moment to delete it. Either remove it, or (if it's load-bearing for some Zustand side effect I'm missing) add a one-line comment explaining why.

2. crypto.randomUUID() usage is now inconsistent with an existing guarded call site. The five new sites call crypto.randomUUID() unguarded. But core/mcp/inspectorClient.ts:421-423 already uses a guarded fallback:

const taskId =
  typeof crypto !== "undefined" && crypto.randomUUID
    ? crypto.randomUUID()
    : `task-${Date.now()}-${Math.random().toString(36).slice(2, 11)}`;

Either the guard is genuinely needed (in which case the new sites should match), or — more likely, given the project's runtime targets — the guard is dead code that should be dropped for consistency. A one-line cleanup PR later is fine; flagging so it doesn't get lost.

3. PR description slightly overstates the streamSSE fix. The description says "Drop the streamSSE cast" — but core/mcp/remote/node/server.ts:456 still has streamSSE(c as Context, …). What the PR actually does is collapse the earlier c as unknown as Parameters<typeof streamSSE>[0] double-cast (plus explanatory comment) down to a single as Context. Worth tightening the description, and worth a quick check whether the residual as Context can be eliminated entirely now that both hono imports resolve to the same package — if not, the code deserves a short comment explaining why a cast is still needed (route-typed Context<Env, P, I> vs. generic Context, I'd guess).

4. Field placement nit in core/mcp/remote/remoteClientTransport.ts. The new private readonly options / config declarations landed at lines 114-115, between the sessionId getter and the constructor. The other fields are grouped at the top (lines 97-101). Moving these two up with the rest matches the pattern used in inspectorClient.ts, oauthManager.ts, messageTrackingTransport.ts, and state-machine.ts (all rewritten in this same PR). Purely stylistic.

---

Things that look right

• OAuthStorageBase is a proper implements OAuthStorage class; the three subclasses are minimal constructor-only wrappers — exactly the right shape for this kind of consolidation.
• The erasable-syntax rewrites are consistent across all six files — field declaration at class top (except the nit above), assignment in constructor body, no drift into alternate styles.
• inspectorClient.ts:1967 logger swap matches the call style used in the rest of the file (this.logger.error({ error }, "…")).
• tsconfig.test.json gets the same "hono/streaming" entry as tsconfig.app.json — easy to forget the sibling config, good catch.
• Using new Hono<Env>() instead of new Hono() means the context type is actually deterministic in the route handler, which is the right direction even if one cast remains.
• The core/auth/state-machine.ts rewrite correctly preserves the optional fetchFn?: typeof fetch — easy place to drop the ? by accident.
  · branch v2/issue-1309-port-cleanups

[cliffhall]

Thanks for the review. All four observations addressed in 75fab8d.

1. Stranded getServerState read. Removed. Verified pure (returns get().servers[serverUrl] || {} with no setter side effect) — was just copy-paste residue from the three originals.

2. crypto.randomUUID() guard. Dropped. Project's minimum runtimes (Node ≥18, modern browsers) all support it unconditionally; the fallback branch in inspectorClient.ts was dead code. Six call sites now consistent.

3. streamSSE cast. Good catch — the as Context was diagnostic residue I forgot to clean up. With both hono imports resolving to the same package via the new hono/streaming alias, the route-typed Context flows through with no cast at all. PR description tightened in spirit by the deletion. Build + 47 integration tests still green.

4. Field placement. Moved options and config in RemoteClientTransport up with the rest of the field block. Matches the other five rewritten classes in this PR.

Re-ran npm run validate and npm run test:storybook — both still pass.

[cliffhall]

@claude review

[claude]

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

---

Claude PR Re-Review

• Fetch latest branch state
• Verify observation Progress notifications #1 fix (stranded getServerState removed)
• Verify observation Request logging #2 fix (crypto.randomUUID guard dropped)
• Verify observation Completion requests #3 fix (streamSSE cast gone)
• Verify observation SSE transport support #4 fix (field placement in RemoteClientTransport)
• Check for any new issues introduced by the cleanup
• Post updated review

---

Summary

All four observations from the previous review are cleanly addressed in 75fab8d. Each change is minimal and surgical — 4 files, +4/-9 lines total, no collateral edits. CI is green. Ship it.

---

Verification

1. Stranded getServerState read — fixed. core/auth/oauth-storage.ts:59-69 — clearClientInformation now goes straight to building updates. The surrounding updates → setServerState flow is identical to the three originals minus the no-op read. Clean.

2. crypto.randomUUID() guard — fixed. core/mcp/inspectorClient.ts:420 is now just const taskId = crypto.randomUUID();. Ran a repo-wide grep over core/ — all 7 randomUUID call sites are now unguarded and consistent (3 in inspectorClient.ts, 1 for task IDs, plus samplingCreateMessage, elicitationCreateMessage, server.ts sessionId). The one remaining typeof crypto !== "undefined" check in core/auth/utils.ts:43 is for getRandomValues, a different API, and predates this PR — out of scope.

3. streamSSE cast — fixed. core/mcp/remote/node/server.ts:456 is now streamSSE(c, async (stream) => { ... }) with zero casts. Confirmed the Context import (line 20) is still used for the two middleware signatures (c: Context, next: Next at lines 82 and 140), so the import stays. The new Hono<Env>() + "hono/streaming" tsconfig alias together make the route-typed Context flow through to streamSSE with no assistance — exactly the end state the PR description promised.

4. Field placement — fixed. core/mcp/remote/remoteClientTransport.ts:97-103 — options and config now sit with _sessionId, eventStreamReader, eventStreamAbort, and closed in a single field block at the top of the class. The sessionId getter and constructor follow. Matches the layout of the other five rewritten classes.

---

No new issues

Nothing introduced by the fixup commit. No new call sites, no drift in style, no stray console.error reintroduced, and the integration suite is still green per CI run 26004052372.

LGTM.
· branch v2/issue-1309-port-cleanups