refactor(mcp): back the MCP client with the official SDK - #1
Open
HeavenllyDemon wants to merge 7 commits into
Open
refactor(mcp): back the MCP client with the official SDK#1HeavenllyDemon wants to merge 7 commits into
HeavenllyDemon wants to merge 7 commits into
Conversation
…l SDK, then background tasks Two stacked PRs. PR 1 migrates McpStdioClient's internals to @modelcontextprotocol/sdk@1.30.0 behind the existing seam (manager.ts and its tests untouched in commit 1), then adds non-text tool content and stderr logging. PR 2 adds server-directed background tasks via client.experimental.tasks, delivered through the existing task_notification path as a third registry alongside bg-registry and bg-agent-registry. v1 over v2 because v2 ships task types with no runtime and fails the conformance tasks suite by design; v1 also bundles smaller (0.51 vs 0.76 MB) since the server stack tree-shakes out. Records three measured behaviour deltas that must be neutralised explicitly: the SDK's 60s default request timeout, its 6-key default stdio environment, and its stricter parsing of the NORMA_FAKE_NULL fixture.
- the bare-null-line delta does not exist. The SDK surfaces it as a
non-fatal zod error via onerror and the connection survives, so both the
fixture and its test stay unchanged.
- commit 2 does NOT change the registry contract. ToolRunResult stays
string-shaped and images attach via ctx.attachImage, so callToolContent
is additive.
- new delta found: the SDK's stdio stderr defaults to "inherit", which
would leak server stderr into the daemon's own stderr.
(The implementation plan lives at docs/superpowers/plans/ and is untracked —
.gitignore:33 already excludes docs/superpowers/.)
Pinned exactly, not caret-ranged: PR 2 uses the experimental tasks API, which can move under a minor bump. Both lockfiles move together — bun install updates bun.lock, and pnpm install --lockfile-only updates pnpm-lock.yaml (bun install alone does not touch it).
Same public surface; the private JSON-RPC plumbing (request/notify/onData/die
framing, pending map, read buffer, id counter) is gone. Every existing MCP test
assertion passes UNMODIFIED — 22 pass / 61 expects across client+manager, 40 /
107 including engine-mcp and mcp-resources, byte-identical to the pre-change
baseline. manager.ts is untouched.
Three SDK defaults are overridden deliberately; each would otherwise be a
silent behaviour change:
- timeout: the SDK defaults to 60s, the old client had none. A tool legally
running longer works today, so NORMA_MCP_CALL_TIMEOUT_MS (default 10min)
with resetTimeoutOnProgress. Ironic but real: "no request timeout" was a
defect, yet silently GAINING a 60s one is a regression.
- env: the SDK's getDefaultEnvironment() passes 6 keys; we keep passing the
full parent env so servers relying on inherited credentials keep working.
- stderr: the SDK defaults to "inherit", which would leak server stderr into
the daemon's own stderr. Piped, and CONSUMED — the SDK pipes the child's
stderr into a PassThrough, which keeps the kernel pipe drained but grows
in memory unboundedly if nothing reads it. The old client attached a no-op
drain for the same reason; this restores it and pre-wires the logging that
lands with the manager's logger.
Gained: notifications/cancelled on abort, -32601 replies to unknown
server->client requests, negotiation to 2025-11-25, zod-validated responses.
Pagination is NOT free — v1's listTools issues exactly one request and never
follows nextCursor — so listAllTools/listResources loop explicitly. Proof
follows in the next commit.
v1's listTools issues exactly one request and does not follow nextCursor, so the loop in listAllTools is our code and needs its own proof — this is the one defect the SDK does not fix for us. Fixture gated behind NORMA_FAKE_PAGES so every pre-existing test still sees a single unpaginated page: 6 -> 7 in client.test.ts, 22 -> 23 across the MCP suite, exactly the one added test.
Closes the '/* could log to daemon log */' TODO the hand-rolled client left behind — a misbehaving MCP server was undebuggable because its stderr was captured and discarded. The client-side consumer landed with the transport swap (it had to: an unread PassThrough grows unboundedly). This wires the manager's existing logger into the constructor so those lines actually go somewhere, and proves it with a chatty fixture gated behind NORMA_FAKE_STDERR. 23 -> 24 in the MCP suite, exactly the one added test.
An image block from an MCP tool call previously became the literal string "[non-text content omitted]" — the user-visible defect of the old client (a screenshot server returned nothing usable to the model). Image blocks now go through attachImageGuarded, the same path read_mcp_resource already uses, so the IMAGE_MAX_BYTES guard and the "[image omitted: …]" fallback behave identically across both surfaces. The registry contract is UNCHANGED. ToolRunResult stays string-shaped and images travel out-of-band on ctx.attachImage, so callToolContent is purely additive and callTool() keeps its exact signature for every existing caller. Both register() call sites now share one toolRunner() helper rather than duplicating the closure, mirroring how startOne already shares bring-up. resource_link and audio degrade to honest one-line summaries instead of the blanket omitted-content string. 24 -> 25 in the MCP suite, exactly the one added test.
HeavenllyDemon
added a commit
that referenced
this pull request
Aug 19, 2026
Fix round 1's first pass covered trap #1 (indexRange overflow) and trap #2's non-trapping "huge but representable, exceeds the cap" variant over the live helper, but not trap #2's own genuine multiplication-overflow input -- caught on advisor review of the report before sending it back. testOverflowingViewportAtTheFinestValidZoomIsRefusedAndTheHelperSurvives drives it: zoomPPT at TileMath.maxZoomPPT (span 5 twips/tile), a length just under Int64.max/2 (indexRange itself stays representable, ~4.6e18 < Int64.max), producing a per-axis tile count around 9.2e17 whose SQUARE overflows Int64 inside estimatedTileCount's own multiplication -- refused as viewportTooLarge, helper survives a ping afterward. Same shape as TileMathTests' pure testEstimatedTileCountAllFourOutcomes case (d), now also proven live. All three trap inputs are now each driven over the real wire, per the review's original instruction. Targeted re-run (TileMath/TileCache/codec + the live classes): 92 tests, 0 failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces
McpStdioClient's hand-rolled JSON-RPC-over-stdio internals with@modelcontextprotocol/sdk@1.30.0, behind an unchanged public seam.This is PR 1 of 2. PR 2 adds server-directed background MCP tasks (
client.experimental.tasks) delivered through the existingtask_notificationpath; it depends on thecallToolContentseam introduced here. Design:docs/research/2026-08-18-mcp-sdk-migration.md.Why v1 and not v2
v2 (
@modelcontextprotocol/client@2.0.0) has a far smaller dependency tree, but it ships the task types marked@deprecated … no SDK runtimeand fails the conformance tasks suite by design —Clienthas nogetTask/callToolStreamat runtime. PR 2 needs that runtime, so v1 it is.The usual objection to v1 is its 17 dependencies. Measured, that objection doesn't survive: the server half never reaches the binary.
sdk@1.30.0client@2.0.0express/hono/cors/express-rate-limitin bundleVerified on the real shipped artifact, not a probe:
strings dist/norma-core | grep -c node_modules/express/→ 0, same for hono, cors, express-rate-limit.Defects closed
tools/list/resources/listignorednextCursor, silently exposing only the first page. Not fixed by the SDK: v1'slistToolsissues exactly one request. The loop inlistAllTools/listResourcesis ours, with its own test.AbortSignalrace that left the server working and leaked the pending entry. The SDK now sendsnotifications/cancelledupstream.[non-text content omitted]. A screenshot server returned nothing usable to the model.Also: server stderr was captured and discarded (
/* could log to daemon log */), making a misbehaving server undebuggable.Three SDK defaults overridden deliberately
Each would otherwise be a silent behaviour change:
NORMA_MCP_CALL_TIMEOUT_MS, default 10 min,resetTimeoutOnProgressgetDefaultEnvironment())process.env"inherit""pipe"+ consumed, routed to the daemon logWorth stating plainly: "no request timeout" is listed above as a defect, but silently gaining a 60s one is a regression — a tool legally running longer works today. The 10-minute bound is chosen, not inherited. Once PR 2 lands, genuinely long work should be a task, not a sync call holding a turn open.
The stderr default matters more than it looks. The SDK pipes child stderr into a
PassThrough(client/stdio.js), which keeps the kernel pipe drained — so a chatty server does not block — but an unreadPassThroughgrows in memory for the life of the process. The old client attached a no-op drain for the same reason.Gained for free
-32601replies to unknown server→client requests, protocol negotiation2024-11-05→2025-11-25, zod-validated responses.Verification
manager.tswas untouched through the transport swap, and every pre-existing MCP test assertion passed unmodified — that was the regression proof.client+manager)engine-mcp,mcp-resources)main)@norma/protocol@norma/plugin-sdkbun run verify:workflowPASSES against the realbun build --compileartifact.tsc --noEmit: unchanged at 6 pre-existing errors, all intest/agent/approvals.test.ts, none inagent/mcp.Two pre-existing failures are not from this branch; both reproduce on a pristine worktree at
4d981b51:tools-bash.test.tssandbox test — this machine deniesmktempin/var/folders. Baseline fails 2 there; this branch fails 1. (The core row is deliberately left without a "before" figure: the pre-change total was never measured in isolation, only mid-branch, so quoting one would overstate the rigor. The comparison that was measured against pristine4d981b51is the per-suite MCP and CLI rows.)@norma/cliInk fullscreen tests — need a real TTY. 1151 / 30 on both baseline and branch, identical.Not in scope
tools/list_changedlive registry updates; elicitation/sampling wiring; Streamable HTTP / SSE transports.agent/lsp/client.tsis the same hand-rolled pattern against a different protocol and is the obvious next candidate.