Skip to content

Refactor / justify codebase-wide 'as unknown as' casts (starting with serverList.ts) #1690

Description

@cliffhall

Context

Flagged during human review of the SDK v2 migration (#1688): core/mcp/serverList.ts had several pre-existing as unknown as casts that widen a typed config object to Record<string, unknown> (and back) to do key-level manipulation.

They are safe — StoredMCPServer / MCPServerConfig have no index signature, so a single as Record<string, unknown> is a TS2352 error and the unknown step is required — but they read as a code smell. This issue originally tracked removing them in serverList.ts.

Scope (expanded)

While in the area we swept the whole codebase for as unknown as double casts (outside test files) and applied the review bar from AGENTS.md: no unjustified as unknown as. Each site is either removed or carries a comment explaining why it's unavoidable.

Removed (widened via a shared helper, or narrowed to a single cast)

  • serverList.ts — the original four:
    • stripInspectorFields: rewritten as clone + delete over INSPECTOR_FIELD_KEYS (all extension keys are optional on StoredMCPServer, so the result is still a subtype of MCPServerConfig — no cast).
    • oauth-strip in extractSecretsFromStored: oauth is optional → plain delete stripped.oauth.
    • mcpConfigToServerEntries: routed through a documented toRecord(value: object) widening helper.
  • Shared helper toRecord(value: object): Record<string, unknown> promoted to core/json/jsonUtils.ts and reused at:
    • core/mcp/node/config.ts (the normalizeServerType loop)
    • core/mcp/remote/node/server.ts (pino log-level dispatch)
    • clients/web/src/App.tsx (the window global token lookup)
  • inspectorClient.rawWireRequest — type the frame as JSONRPCRequest and narrow only params with a single structural cast (the SDK's _meta-typed request params is what blocked direct assignment).
  • toolOutputValidation.ts — SDK outputSchemaJsonSchemaType collapses to a single cast.
  • test-server-fixtures.ts (×2) — the optional task handle is reachable with a single cast (the field is optional).

Justified in place (genuinely unavoidable)

Private SDK internals and structural bridges that can't be removed without SDK changes, now each carrying a comment: the inspectorClient private-map accesses (_requestHandlers / _responseHandlers / _progressHandlers) and task-result gaps, resourceContext + tokenAuthProvider partial OAuthClientProvider stubs, the ext-apps v1/v2 peer bridge in createAppBridgeFactory, the modern/legacy _requestHandlers bypasses in the test servers, and the happy-dom matchMedia test mock.

Notes

Behavior-neutral throughout — good existing coverage on serverList.ts (79 tests) plus the full npm run ci gate (validate, ≥90 per-file coverage, smoke, Storybook). Net goal met: zero unjustified as unknown as in non-test source.

Ref: review comments 3589589773 / 3589591281 on #1688.

Metadata

Metadata

Assignees

Labels

v2Issues and PRs for v2

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions