Refactor / justify codebase-wide 'as unknown as' casts (#1690)

[cliffhall]

Closes #1690

What

Removes or justifies every as unknown as double cast in non-test source. Started as the scoped serverList.ts cleanup flagged in the SDK v2 migration review of #1688, then expanded (per maintainer request) to a codebase-wide sweep applying the AGENTS.md bar: no unjustified as unknown as.

Removed

serverList.ts (the original four):

Site	Fix
stripInspectorFields (input + return casts)	clone + delete over INSPECTOR_FIELD_KEYS; every extension key is optional on StoredMCPServer, so the result is still a subtype of MCPServerConfig — no cast
oauth-strip in extractSecretsFromStored	delete stripped.oauth (oauth is optional)
mcpConfigToServerEntries	shared toRecord() widening helper

Shared helper — toRecord(value: object): Record<string, unknown> promoted to core/json/jsonUtils.ts. Taking the argument as object makes the single as Record<string, unknown> legal (no TS2352), so the double cast leaves the call sites. 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 — window global token lookup

Single-cast narrowings:

• inspectorClient.rawWireRequest — type the frame as JSONRPCRequest, narrow only params (the SDK's _meta-typed params blocked direct assignment)
• toolOutputValidation — outputSchema as JsonSchemaType
• test-server-fixtures (×2) — the optional task handle needs only a single cast

Justified in place

Genuinely unavoidable (private SDK internals / partial interface stubs / nominal bridges), each now commented: the inspectorClient private-map accesses and task-result gaps, resourceContext + tokenAuthProvider partial OAuthClientProvider stubs, the ext-apps v1/v2 peer bridge, the modern/legacy _requestHandlers bypasses in the test servers, and the happy-dom matchMedia test mock.

Verification

Behavior-neutral. Existing 79 serverList tests pass unchanged; full npm run ci passes (validate, the ≥90 per-file coverage gate, smoke, 456 Storybook tests).

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

[cliffhall]

@claude review

[claude]

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

---

Review: Refactor / justify as unknown as casts (#1690)

• Read the PR diff against origin/v2/main
• Verify each removed cast is behavior-neutral
• Verify each justified-in-place cast has a sound comment
• Review the new toRecord helper
• Post review feedback

---

Solid, well-documented cleanup. I traced every removed cast and every remaining as unknown as in non-test source. The refactor is behavior-neutral and the design of the shared helper is nice. A few minor observations below — none blocking.

What's good

• toRecord(value: object) is the right shape. Taking the argument as object (rather than a generic T) is exactly what makes the single value as Record<string, unknown> legal — Record<string, unknown> is assignable to object, so no TS2352 and no double cast. Concentrating the widening in one audited, well-commented spot (core/json/jsonUtils.ts:30) and reusing it at the four call sites is a clean pattern.
• stripInspectorFields clone+delete is behavior-equivalent and genuinely cast-free: since every StoredInspectorFields key is optional, StoredMCPServer is a structural subtype of MCPServerConfig, so delete-ing them off a spread clone type-checks without any assertion. { ...stored } + delete preserves the same own-enumerable set the old manual copy produced. ✅
• mcpConfigToServerEntries — toRecord(inspectorFields) returns the same reference, so writes through inspectorRecord are still visible in the later storedFieldsToInspectorSettings({ ...inspectorFields }) spread. Behavior preserved. ✅
• rawWireRequest — typing message as JSONRPCRequest (a JSONRPCMessage union member) and narrowing only params is a real reduction in cast surface, not a relabel. transport.send still accepts it. ✅
• Justification comments added at resourceContext.ts, tokenAuthProvider.ts, toolOutputValidation.ts, test-server-fixtures.ts (×2), and setup.ts are all accurate about why the cast is unavoidable.

Minor observations (optional)

1. Inconsistent justification between the two _requestHandlers casts. test-servers/src/composable-test-server.ts:1401 got a full inline block explaining the SDK-gap seam, but its sibling test-servers/src/modern-tasks.ts:324 (const registry = (lowLevel as unknown as RawHandlerHost)._requestHandlers;) has no comment adjacent to the cast — it leans on the function's doc comment two lines up. For consistency with the sibling and the strict reading of the AGENTS.md "MUST carry an inline comment" bar, consider mirroring the same one-liner here. Fix this →

2. Story fixtures still carry unjustified as unknown as AppBridge. AppsScreen.stories.tsx (×2), InspectorView.stories.tsx, and AppRenderer.stories.tsx retain unannotated double casts. *.stories.tsx files aren't .test. files, so under a literal "every non-test source" reading they'd be in scope; if the intent is to treat them as test fixtures (which they effectively are — they double as play-function tests), a one-line scope note in the PR description would preempt the "you missed these" question. Not worth changing the stories themselves.

3. A little formatting noise. Several hunks in inspectorClient.ts (the protocolVersion line, the task-content array, the timeout-message wrap, the reconnect guard at ~4444) are pure Prettier reflow unrelated to casts — presumably from npm run format. Harmless, just mixed into an otherwise cast-focused diff.

Verdict

No correctness or behavior concerns — the removals are provably neutral and the in-place justifications are sound. The two items above are consistency nits, not blockers. LGTM. 👍
· v2/1690-serverlist-record-casts

[cliffhall]

Thanks for the thorough pass! Addressed all three observations in cc187ea:

1. modern-tasks.ts:324 consistency — added an inline justification directly above the _requestHandlers cast, mirroring the composable-test-server.ts:1401 sibling instead of relying on the function JSDoc.
2. Story-fixture as unknown as AppBridge — rather than a scope note, I annotated all four (AppsScreen ×2, AppRenderer, InspectorView) as partial-mock bridges, so there's no unjustified double cast anywhere in non-test source.
3. Formatting noise — reverted the unrelated Prettier reflow hunks in inspectorClient.ts (they were pre-existing core/ drift, since core/ isn't format-gated); the file's diff is now just the single rawWireRequest cast change.

Note: npm run ci is green except for a known-flaky assertion in fetchTracking.test.ts — its not.toContain("shh") redaction check collided with a randomly-generated request id (sw7kshhf0); redaction itself is correct (client_secret=[REDACTED]) and the test passes on re-run. Unrelated to this PR.

@claude review

[claude]

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

---

Re-review: nit fixes in cc187ea

• Verify modern-tasks.ts _requestHandlers justification comment
• Verify story-fixture AppBridge cast annotations (×4)
• Verify inspectorClient.ts reflow noise reverted
• Post review

---

Two of the three items are cleanly addressed. The third doesn't match what actually landed — flagging it below so the record's straight.

✅ 1. modern-tasks.ts:325 consistency

The inline block above (lowLevel as unknown as RawHandlerHost)._requestHandlers now mirrors the composable-test-server.ts sibling and even cross-references it + the client-side bypass. Accurate about the seam (skipping the SDK result-schema validation so a CreateTaskResult gets through) and the exit condition (goes away when the SDK models the tasks extension natively). 👍

✅ 2. Story-fixture AppBridge casts

All four (AppRenderer.stories.tsx, AppsScreen.stories.tsx ×2, InspectorView.stories.tsx) now carry a "partial mock … the double cast bridges the deliberately-incomplete shape" annotation. This is the stronger choice over a scope note — there's now genuinely no unjustified as unknown as in non-test source, so the AGENTS.md bar holds under the literal reading. 👍

⚠️ 3. inspectorClient.ts reflow — commit message says reverted, but it wasn't

The commit message for cc187ea states "inspectorClient.ts: revert unrelated Prettier reflow hunks … so the diff is the single rawWireRequest cast change only." That didn't happen — cc187ea doesn't touch inspectorClient.ts at all (its --stat lists only the 4 story/test-server files). The four reflow hunks I flagged last time are still in the PR diff:

• inspectorClient.ts:1832-1833 — protocolVersion wrap
• :1865 — task-content array collapse
• :1908-1910 — timeout-reject wrap
• :4447-4450 — reconnect guard wrap

$ git diff origin/v2/main...HEAD -- core/mcp/inspectorClient.ts | grep -c '^@@'
4

Not a blocker — these are all Prettier-correct (the base lines exceed the 80-col print width, e.g. the protocolVersion line is 81 chars, so Prettier legitimately wraps them), and since core/ isn't format-gated they're harmless either way. But the diff is not "the single rawWireRequest cast change only" as claimed. Two clean options:

• Leave as-is and just correct the record — they're valid formatting, so keeping them is fine; or
• Actually revert them if you want the minimal cast-only diff. Revert the reflow →

(Minor aside: cc187ea did introduce one new reflow — the ModernTaskStatus union in modern-tasks.ts:44-49 reformatted to leading-| style. Trivial, and it's in a file the commit legitimately touches.)

Re: the flaky fetchTracking.test.ts

Agreed this is unrelated to the PR — a not.toContain("shh") redaction assertion colliding with a random request id (sw7kshhf0) is a fixture-seeding bug in that test, not a redaction regression. Worth a separate follow-up issue to seed the id deterministically (or assert on the redacted-token span rather than a bare substring) so it stops flaking, but out of scope here.

Verdict

Items 1 and 2 fully resolved; the cleanup is sound and behavior-neutral. The only open thread is the cosmetic mismatch on item 3 — the reflow hunks the commit message says were reverted are still present. Since they're Prettier-correct, this is bookkeeping, not correctness. LGTM once you decide whether to keep or drop them. 👍
· v2/1690-serverlist-record-casts

[cliffhall]

Good catch — you're right, and the cause was self-inflicted: my reflow reverts in cc187ea were clobbered by a prettier --write I ran on the same file immediately after, so the file netted back to 6a3afa4a and the commit didn't touch it at all.

Fixed for real in 62c0264, this time without re-running Prettier on the file:

• inspectorClient.ts — restored the protocolVersion line, task-content array, timeout-reject call, and reconnect guard; the file's diff is now a single hunk (the rawWireRequest cast change). Confirmed: git diff origin/v2/main -- core/mcp/inspectorClient.ts | grep -c '^@@' → 1.
• modern-tasks.ts — also reverted the ModernTaskStatus leading-| union reflow you flagged, leaving only the new _requestHandlers justification comment.

Agreed on the flaky fetchTracking test being a separate follow-up (deterministic id seeding, or asserting on the redacted span rather than a bare substring) — I'll file that separately, out of scope here.

@claude review

[claude]

Claude finished @cliffhall's task in 40s —— View job

---

Re-review: reflow revert in 62c0264

• Verify inspectorClient.ts diff is now a single hunk (the rawWireRequest cast)
• Verify modern-tasks.ts ModernTaskStatus reflow reverted, _requestHandlers comment kept
• Confirm no other cast changes regressed
• Post verdict

---

Confirmed — 62c0264 lands exactly what the commit message claims this time. 👍

✅ inspectorClient.ts — single hunk

$ git diff origin/v2/main -- core/mcp/inspectorClient.ts | grep -c '^@@'
1

The file's entire diff is now the one rawWireRequest change: message typed as JSONRPCRequest with params narrowed via a single params as JSONRPCRequest["params"] cast, replacing the old } as unknown as JSONRPCMessage. The four Prettier-only reflow hunks (protocolVersion wrap, task-content array, timeout-reject, reconnect guard) are gone — back to their origin/v2/main form. The seam is well-commented and the cast surface is genuinely reduced, not relabeled.

✅ modern-tasks.ts — reflow dropped, comment retained

The diff is now purely the six-line _requestHandlers justification block above (lowLevel as unknown as RawHandlerHost)._requestHandlers. The ModernTaskStatus leading-| union reflow I flagged as a minor aside last round is no longer present — nice catch reverting that too, so the file's only change is the comment.

Verdict

All three original nits are now fully resolved, and the cosmetic mismatch from the last round is cleared: the PR diff is cast-focused and free of unrelated formatting churn. Removals remain provably behavior-neutral; every surviving as unknown as in non-test source carries a sound justification. Agreed the flaky fetchTracking test is a separate follow-up. LGTM. 🚢
· v2/1690-serverlist-record-casts