Skip to content

Core/v1.5-port cleanup follow-ups (#1306 deferred items) #1309

@cliffhall

Description

@cliffhall

Background

Combined tracker for five small, mechanical cleanup items deferred from the #1306 review (the v1.5 InspectorClient runtime port). All are direct v1.5 carry-overs that work correctly today but have small consistency or strictness issues worth fixing. Each is independent and the whole set is roughly a single afternoon's work.

Best done after #1307 (the integration-test follow-up) so the OAuth subsystem in particular has real test coverage before any of it gets refactored.

Items

1. Consolidate the three OAuthStorage implementations

core/auth/browser/storage.ts, core/auth/node/storage-node.ts, and core/auth/remote/storage-remote.ts are each ~130 lines and differ only in their constructors. All three delegate to the same createOAuthStore factory. They can collapse into a single class parameterized on storage adapter with zero behavior change.

  • Replace the three classes with one parameterized class (e.g. OAuthStorage taking an adapter)
  • Keep the three adapters (browser/node/remote) as the distinguishing types
  • Update core/mcp/types.ts InspectorClientEnvironment.oauth.storage consumers
  • Verify ported v1.5 OAuth tests still pass

2. Re-enable erasableSyntaxOnly in tsconfig.app.json

#1306 disabled erasableSyntaxOnly: true because the ported v1.5 code uses TS parameter properties (constructor(private foo: T)) in seven places. Convert them and flip the flag back on.

Sites (from #1306 review):

  • core/auth/providers.ts:55
  • core/auth/providers.ts:110
  • core/auth/state-machine.ts:260
  • core/mcp/oauthManager.ts:48
  • core/mcp/inspectorClient.ts:179
  • core/mcp/messageTrackingTransport.ts:27
  • core/mcp/remote/remoteClientTransport.ts:115
  • Remove the erasableSyntaxOnly: false override + TODO comment in clients/web/tsconfig.app.json

3. Switch console.errorthis.logger.error in inspectorClient.ts

The rest of the file uses this.logger; two sites use console.error directly.

  • core/mcp/inspectorClient.ts:1965 (roots/list_changed notification failure)
  • Audit the rest of the file for any other console.* sites and convert them too

4. Standardize local ID generation on crypto.randomUUID()

Five sites build entry IDs from ${Date.now()}-${Math.random()}. These are local-only identifiers (never sent to the server), so there's no security concern, but crypto.randomUUID() is already used by other paths (fetchTracking, task) and would be more consistent.

  • core/mcp/samplingCreateMessage.ts:38
  • core/mcp/elicitationCreateMessage.ts:39
  • core/mcp/inspectorClient.ts:312
  • core/mcp/inspectorClient.ts:323
  • core/mcp/inspectorClient.ts:332

5. Pin the hono Context generic at new Hono<Env>() and drop the streamSSE cast

core/mcp/remote/node/server.ts:458 has streamSSE(c as unknown as Parameters<typeof streamSSE>[0], …) to work around a hono generic mismatch. The clean fix is to pin the Context generic when the Hono app is created so the cast becomes unnecessary.

Acceptance criteria

  • All five item sections above are checked off
  • npm run validate + npm run test:storybook both pass
  • No new TODOs added; the corresponding TODOs in vite.config.ts / tsconfig.app.json / inspector source removed

Out of scope

Metadata

Metadata

Assignees

Labels

v2Issues and PRs for v2

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions