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.
HeavenllyDemon
added a commit
that referenced
this pull request
Aug 25, 2026
…ive verdict)
slides-lok-research.md's own R1 ("the single biggest risk to reorder
being implementable at this pin"): no arbitrary-index move UNO command
exists, only selection-based MovePageUp/Down/First/Last, and whether
setPart drives that selection in a genuinely headless session (no Slide
Sorter panel ever shown) was undeterminable from source. Live-tested,
first attempt: it does.
testProbeInvestigatesWhetherReorderIsReachableHeadless deliberately
targets slide 2 (index 1), never slide 1 (index 0) — a fresh doc's
plausible default selection at page 0 would make an index-0 move
unable to distinguish "setPart drove it" from "page 0 was already
selected." The three-position readout (all of index 0/1/2, not just
the target) answers both reachability and the setPart-selection
question in one run: before [One,Two,Three], after moving 1->2:
[One,Three,Two], exactly as predicted for a real adjacent swap.
slidesReorderOnDedicatedThread implements the real single-step (and,
untested beyond one step, N-step) mechanism:
- Dispatched on the PRIMARY view, destroyAgentViewIfAnyOnDedicatedThread
first, NOT the agent-view isolation this file's read path uses.
Deliberate, not an oversight: sheetsManageSheetOnDedicatedThread's own
header carries two rounds of live evidence that structural page/sheet-
list mutations on the agent view either hang (30s, .uno:Remove,
whenever an agent view merely existed) or never converge
(.uno:Add/.uno:Name, full 20-attempt budget burned every run).
MovePage* is implemented on SlideSorterViewShell, the same structural
class, so this starts from the already-proven-safe shape rather than
re-earning the hang empirically. Sheets' own disclosed residual
(primary-view setPart can move an adopted document's own visible
slide) carries over unchanged, not closed here either.
- Verified by re-reading placeholder TITLE text at from/to, never
getPartName (research's own positional-recompute trap) and, contrary
to the controller's own resume-message instruction ("verified by
getPartInfo.hash"), never getPartInfo's hash field either — a
DISCLOSED substitution: research §7 states neither of LOK's two
in-session identity primitives survives save+reload, which is fatal
for this task's own mandatory save+reopen two-part-discriminator
proof reorder's real drill will need anyway. Title text is this
bridge's own already-proven, reload-durable content primitive
(Probe A) — reusing it avoids a second mechanism AND avoids adding
getPartInfo as new wire surface for what would otherwise be a
probe-only read.
- New notifyWhenFinished: true call sites (both MovePage* dispatch and
none other), per the controller's own instruction #1.
- Own dedicated slidesManageVerificationAttempts budget (20, matching
its closest analogue sheetsManageVerificationAttempts's own value,
not shared with it).
Multi-step composition (abs(to - from) > 1) is implemented as the same
primitive repeated but NOT independently live-verified — only the
single-step case the probe actually ran. Keep/excise for `reorder`
remains the coordinator's call; this is the verdict artifact for that
decision, not a unilateral ship.
HeavenllyDemon
added a commit
that referenced
this pull request
Aug 25, 2026
…ssertions F-1 (Important, "arc's defect class #1, 4th occurrence") -- reorder's and add_slide's filesystem seals asserted three titles the pristine fixture already has, and String.contains is order-blind, so both passed on a file that was never written at all: - reorder: replaced the three contains() checks with byte-offset order comparisons (Two's range.lowerBound < Three's < One's) -- the seal's actual subject is ORDER, which contains() cannot observe at all. - add_slide: added a <draw:page> count assertion (== 4) to all three scenarios' own saved bytes, not just scenario 2. Scenarios 1 and 3 previously never reopened or checked saved bytes at all (the reviewer's own evidence line named this explicitly) -- now sealed via the same readODFEntry helper, no independent client needed since it's a raw zip-entry read. F-2 (Minor) -- scenario 1's tautological second assertion (hasPrefix(...) || contains(...), whose right disjunct the line above already proved true, so it could never fail regardless of what the text actually started with) replaced with a single real hasPrefix check, pinning formatSlidesInfo's own documented output format. F-3 (Minor) -- scenario 3 asserted the three original titles landed at positions 2/3/4 but never asserted the resulting slide COUNT, so a double-insert leaving five slides with the originals still at 2/3/4 would have passed. Added the same "4 slides" check scenarios 1 and 2 already have. Both fixed tests pass live.
HeavenllyDemon
added a commit
that referenced
this pull request
Aug 25, 2026
…stdout
The stdout assertion was not fragile, it was STRUCTURALLY IMPOSSIBLE.
main.ts:659 renders a tool result as output.split('\n')[0].slice(0,120) — first
line, 120 chars — so a multi-row sheets read prints only its header and the grid
never reaches stdout at all. toolResultFor also admitted column-0 '|' lines, and
the model's narration in this product IS a column-0 markdown table: the string it
matched came from the model, in a session carrying that same string in context
from the sheets.set prompt ninety events earlier. The step advertised as the
independent fresh-open leg was satisfied by prose — inside the gate built to make
that impossible.
Worse, the red I recorded as proof was hollow: expectedA4 was '' so the red came
from the length guard, never from the tool result. That guard is now a distinct
INCONCLUSIVE outcome that can never again be mistaken for the assertion.
Now reads the session JSONL's ToolResultEvent (callId-paired, output unbounded) —
the authoritative record, sidestepping the CLI layer entirely. toolResultFor is
deleted rather than left as a trap.
Also: the file-evidence denominator is PINNED (deleting a step turned 8/8 into a
green 7/7 — the tally itself going vacuous, this arc's #1 class one level up);
SIGINT/SIGTERM tear down; before.found is checked before the vacuity guard (an
unreadable pristine cell read as unstyled); the font alternation puts the
self-closing form first (a <font/> was swallowed by the greedy paired form,
shifting every later index — latent, zero present today).
CORRECTION: 'the daemon reports listening on a socket it never created' is FALSE
and withdrawn. At 127 bytes the socket IS created (srw-------) while running; I
listed the directory after killing the daemon, and it removes its socket on
shutdown. Wrong conclusion and wrong supporting fact — measured after the fact
instead of during it. sun_path=103 stands.
Claude-Session: https://claude.ai/code/session_019pqCwL5mTcMEPHer8TK5D8
HeavenllyDemon
added a commit
that referenced
this pull request
Aug 27, 2026
…y occurrence"
Eight saved-bytes drills against the real spawned helper, plus a new pristine fixture. All 24 tests
in the file pass.
LT-1 IS ANSWERED: A FIND-SCOPED FORMAT REACHES EVERY OCCURRENCE
The research could not settle this from source — whether SwWrtShell::SetAttrSet applies across every
cursor in a FIND_ALL's multi-range ring was untraceable at its budget — and the tool description
could not be written honestly until it was. The fixture holds the literal in three separate
paragraphs, so the saved content.xml distinguishes the answers directly: three bold runs means every
occurrence, one would have meant the first only. It is three. The drill also asserts WHICH text is
bold, not just how many runs there are — a format that bolded whole paragraphs instead of the
matched words would produce the right count and the wrong result.
TWO INSTRUMENT FAILURES, BOTH CAUGHT BY CROSS-CHECKING AGAINST RAW BYTES
The bold-counting helper was wrong twice, both times reporting NO bold on a file that had it, and
both times the product was right. Recording this because the near-miss is the useful part: the first
failure reported "0 bold runs" while the verb's own sentence said "Confirmed by reading the
formatting back out of the document." Reading that as a false-positive verification and "fixing" the
verification would have broken working code. Dumping the actual content.xml is what settled it — all
three MARKERs were correctly wrapped in a bold span.
1. The regex read `\\b` where it needed `\b`, so the style scan matched nothing.
2. It counted only text SPANS. ODF carries bold on two different things: a word-scoped format
produces a span referencing a bold text style, while a whole-paragraph format produces no span
at all — the paragraph gets a bold paragraph style. So it reported zero on a correctly
whole-document-bolded file.
The helper now scans both carriers and treats fo:font-weight="normal" as the explicit clear it is,
and it carries a self-check: if the bytes declare a bold weight anywhere and the scan found no
carrier, it fails ITSELF rather than the product.
WHAT THE DRILLS COVER
A pristine-fixture assertion that exists so the other drills cannot pass by construction (the arc's
#1 defect class, four prior occurrences). bold:false CLEARING rather than toggling — the drill that
catches the toggle-hazard class, since a subtly wrong payload passes a bold-it-on test and fails
this one. A no-match find refusing with the document left byte-identical. Alignment and line spacing
reaching the saved paragraph properties, asserted against a fixture that declares neither. A heading
style applying. The result sentence never asserting bare success — it must either say what it
confirmed or say it could not check. And a mistyped `bold` refused on four arms (string, number,
array, object) starting FROM a bold document, so a coerced-to-false value would show as a real
change rather than hiding as a no-op.
The fixture is committed as both .fodt (readable source) and .odt (what the drills use). The bridge
cannot save flat ODF — measured, not assumed: the first live run failed with "saving is not
supported for this document's format", which is a property of the save path, not of the fixture.
Claude-Session: https://claude.ai/code/session_019pqCwL5mTcMEPHer8TK5D8
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.