Skip to content

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

Closed

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 outputSchema → JsonSchemaType 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.

Activity

  1. self-assigned this
    on Jul 15, 2026
  2. changed the title [-]Refactor core/mcp/serverList.ts object↔Record 'as unknown as' casts into a typed helper[/-] [+]Refactor / justify codebase-wide 'as unknown as' casts (starting with serverList.ts)[/+] on Jul 24, 2026
  3. cliffhall commented on Jul 24, 2026

    @cliffhall
    MemberAuthor

    Merged via #1756 (squashed into v2/main). Codebase-wide as unknown as audit complete: removable double casts removed, unavoidable ones justified in place.

  4. added this to the v2.0.0 milestone on Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

v2Issues and PRs for v2

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions