feat(mt#4854): Migrate the MCP surface to the SDK v2 packages at current-protocol parity - #3547
Merged
Merged
Conversation
…t current-protocol parity
@modelcontextprotocol/sdk (v1) never implements the MCP 2026-07-28 revision at any
published version — npm carries one dist-tag, latest -> 1.30.0, whose
LATEST_PROTOCOL_VERSION is 2025-11-25. Support ships in a separate v2 package family.
This migrates every SDK import site to it at current-protocol parity: no wire change,
no serving-entry change. Adopting the 2026-07-28 revision stays with the parent,
mt#4608, gated on a decision ask#11232 deliberately deferred.
Applied the vendor codemod (bunx @modelcontextprotocol/codemod v1-to-v2), whose
dry-run scope matched the planned file set exactly: 34 changes across 10 files plus
package.json. Imports resolve per the SDK's own importMap — Server and the spec types
to /server, StdioServerTransport to /server/stdio, StreamableHTTPServerTransport to
NodeStreamableHTTPServerTransport from /node (Express hands us Node req/res, so the
Node variant is correct), JSONRPCMessageSchema to /core, Client + InMemoryTransport
both from /client (the guide requires one package per linked pair), and
McpError/ErrorCode to ProtocolError/ProtocolErrorCode.
Three manual changes the codemod could not make:
- diagnostic-capture.ts: v1's flat `extra` became v2's structured `ctx`, and the two
fields this capture reads moved DIFFERENTLY — sessionId stayed top-level, _meta
moved to ctx.mcpReq._meta. Reading ctx._meta would not throw, it would silently
capture undefined forever, and nothing asserts on this output.
- server.ts tools/list: v2 types Tool.inputSchema as { type: "object"; ... } where v1
took a bare object. The `{}` fallback becomes `{ type: "object" }` — NOT a wire
change, because the only production addTool caller (command-mapper.ts:508) always
supplies inputSchema, so the fallback is unreachable there. Corrected rather than
cast away: a bare `{}` was never spec-valid.
- server.test.ts: v2 routes tools/call through _invokeInputRequiredCapableHandler,
which reads ctx.mcpReq.requestState() before delegating, so the bare `{}` these
tests passed as `extra` now throws inside the SDK. Eight sites reaching the SDK's
private _requestHandlers map collapse into one getToolsCallHandler helper, which
also shrinks mt#4844's migration surface from eight copies to one.
Corrects the planning enumeration: `grep -l` for the package string conflated mentions
with imports. src/commands/mcp/start-command.ts and
services/reviewer/src/mcp-client.ts only NAME the SDK in comments — the reviewer
client is explicitly "plain fetch only, no @modelcontextprotocol/sdk dependency". The
real import surface is 10 files, and the coexistence SC3 planned for is moot: v1 is
removed outright.
Typecheck clean across 8 projects, lint clean across 4292 files, 169 tests pass across
the 6 related suites.
… pre-push gate caught The first commit fixed the eight `_requestHandlers` reaches in server.test.ts and passed a targeted run of the six suites related to the files I changed. The pre-push gated suite then failed three MORE src/mcp files with the identical `ctx.mcpReq.requestState()` TypeError: drift-gate.test.ts (2), server-response-size-guard.test.ts (3), server-in-flight-tool-calls.test.ts (1). These are exactly the other three files mt#4844 enumerates as reaching the SDK's private handler map. They break under the SDK upgrade without being edited by it, so a related-test selection keyed on CHANGED files could not reach them — the gate is what caught it, working as intended. Rather than copy the wrapper a fourth time, the helper moves to src/mcp/test-support/tools-call-handler.ts and all four suites import it. That turns fourteen copies of the private-map reach into one, so mt#4844's migration becomes a single edit; the module's docblock carries the v2 context rationale and drift-gate's two sites lose a now-redundant hand-written cast. Evidence, per file and in its own process (multiple src/mcp files in one bun test invocation hit the Bun defect CLAUDE.md documents — no summary, exit 0, which is not a pass and is recorded here as the non-answer it is): src/mcp/server.test.ts 46 pass 0 fail src/mcp/server-tool-name-resolution.test.ts 4 pass 0 fail src/mcp/client-capabilities.test.ts 23 pass 0 fail src/mcp/drift-gate.test.ts 28 pass 0 fail (was 26 pass / 2 fail) src/mcp/server-in-flight-tool-calls.test.ts 1 pass 0 fail (was 0 pass / 1 fail) src/mcp/server-response-size-guard.test.ts 3 pass 0 fail (was 0 pass / 3 fail) Plus 96 pass / 0 fail across mcp-structured-errors.test.ts, start-command.test.ts and deploy-minsky-mcp.test.ts. Typecheck clean across 8 projects; lint clean across 4293 files.
Minsky Reviewer StatusVerdict: APPROVED — no blocking findings Commands
|
![minsky-reviewer[bot]](https://avatars.githubusercontent.com/u/52695?s=60&v=4){kind=small}
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Key block: The replacement criterion for SC3/AT4 requires no @modelcontextprotocol/sdk in bun.lock; however, bun.lock still contains multiple v1 SDK entries (including an explicit @modelcontextprotocol/sdk@1.17.0 and nested inspector-related sections). This contradicts the spec/AT and must be resolved or the spec updated. Non-blocking notes: (1) test helper’s injected ctx lacks mcpReq.notify, which could break future tests using progressToken — suggest adding a no-op notify; (2) centralizing private _requestHandlers access is better, but migrating tests to a supported seam (connectTransport + InMemoryTransport) remains advisable (mt#4844). Aside from the lockfile inconsistency, the code migration appears coherent and parity-preserving. I did not re-grep the entire tree; verification of SC2/AT1-AT3 depended on live runs outside the diff.
Findings
- [BLOCKING] bun.lock:1 — Replacement criterion (SC3/AT4) not met:
@modelcontextprotocol/sdkstill present in bun.lock
The spec’s replacement for SC3/AT4 explicitly requires that@modelcontextprotocol/sdkappear in neitherpackage.jsonnorbun.lock. Whilepackage.jsonremoves the dependency,bun.lockstill contains multiple@modelcontextprotocol/sdkentries via nested dependencies (e.g., an entry for"@modelcontextprotocol/sdk@1.17.0"and numerous@modelcontextprotocol/sdk/*subtrees associated with the inspector packages). See the diff hunks showing additions for"@modelcontextprotocol/sdk": ["@modelcontextprotocol/sdk@1.17.0", …]and related sections (@modelcontextprotocol/inspector-*/…/@modelcontextprotocol/sdk). This violates the criterion’s stated check that the lockfile not contain any@modelcontextprotocol/sdkentries. Either (a) purge the v1 package from the lock by updating/removing the transitive inspector dependencies that pull it in, or (b) update the task spec/criteria to accept transitive lockfile presence and adjust AT4 accordingly. As written, this is a blocking mismatch against the Success Criteria. - [NON-BLOCKING] src/mcp/test-support/tools-call-handler.ts:48 — Helper-supplied ctx lacks
mcpReq.notify, risking future failures when tests set aprogressToken
buildProgressReporterinsrc/mcp/server.tsnow expects asendNotificationfunction compatible with v2’sctx.mcpReq.notifywhen a caller provides_meta.progressToken. The test helper’s wrapper context sets onlymcpReq: { requestState: () => undefined }(see lines 53-62) and omitsnotify. Current tests likely don’t exercise progress (no_meta.progressToken), but adding one in a future test would cause a runtime error when the server attemptsctx.mcpReq.notify(...). Suggestion: include a no-opnotify: async () => {}on the helper’smcpReqto fully satisfy the v2 context shape and make tests robust to progress-token use. - [NON-BLOCKING] src/mcp/server.test.ts:546 — Tests still reach SDK private
_requestHandlers(now via helper) — consider migrating to a supported seam
The newgetToolsCallHandlerhelper centralizes accesses to the SDK Server’s private_requestHandlersmap, which is an improvement over ~14 scattered sites. However, this still couples tests to an internal detail of the SDK. Pertesting-standards.mdc §Testable Design, prefer exercising behavior through supported seams. The code already exposesMinskyMCPServer.connectTransport(...)and v2 providesInMemoryTransportin@modelcontextprotocol/client— together these offer a public, supported path to drive requests. Suggest migrating these tests to construct a linked pair and call through the client, or add a designed observable that exposes the handler invocation without peering into private state (tracked in mt#4844). Marking as non-blocking advisory for test design.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 — every in-scope SDK import site is on v2. The 10 sites that actually IMPORT the SDK … Verified by: grep … returns only v2 specifiers, and no @modelcontextprotocol/sdk import remains anywhere. | Met | package.json removes "@modelcontextprotocol/sdk" and adds v2 packages (package.json: dependencies diff). Code imports updated across touched files: e.g., src/mcp/server.ts top imports now from "@modelcontextprotocol/server", "@modelcontextprotocol/node"; src/mcp/server-tool-name-resolution.test.ts imports { Client, InMemoryTransport } from "@modelcontextprotocol/client"; scripts/deploy-minsky-mcp.ts and scripts/verify-mcp-hidden-params.ts import LATEST_PROTOCOL_VERSION from "@modelcontextprotocol/server". No v1 imports are present in the shown diff hunks. |
| SC2 — no wire change, measured not assumed. For each client in AT1’s control set, the negotiated protocol version and header are byte-identical before and after. | Unverifiable | This criterion depends on live-run measurements (AT1) that are outside the diff. The PR body includes sample output, but as reviewers we cannot reproduce or verify from code alone. |
| SC3 — WITHDRAWN, premise false. Replacement criterion: @modelcontextprotocol/sdk is removed from package.json entirely and no file imports it; validate_* and build are clean afterward. | Not Met | While package.json removes the v1 SDK and code imports are migrated, bun.lock still contains multiple entries for "@modelcontextprotocol/sdk" (e.g., entries for "@modelcontextprotocol/sdk@1.17.0" and many nested @modelcontextprotocol/inspector-* → sdk references). AT4 replacement also requires bun.lock not to contain v1. This violates the replacement criterion as written; follow-up needed to remove or reconcile transitive v1 lock entries. |
| SC4 — the codemod's output is reviewed, not merged blind. State in the PR body which files the codemod rewrote and which were hand-edited. | Met | PR Description details the codemod vs hand-edited changes, including specific unresolved codemod warnings and how they were addressed (diagnostic-capture ctx mapping, inputSchema typing, and test helper extraction). |
| SC5 — no serving-entry change. src/mcp/server.ts keeps constructing a hand-built Server; createMcpHandler / serveStdio are NOT introduced here. | Met | src/mcp/server.ts constructs Server directly and uses StdioServerTransport or NodeStreamableHTTPServerTransport; there is no import or usage of createMcpHandler/serveStdio. See src/mcp/server.ts:1-20 and constructor/setups. |
| validate_typecheck and validate_lint clean; bun scripts/run-tests-gated.ts passes; the bundle-boot smoke passes. | Unverifiable | These are CI/runtime results not visible in the diff. The PR body claims they passed, but we cannot verify from repository contents alone. |
| AT1 — negative control, then comparison. BEFORE any edit, record the negotiated protocol version… After the migration, all three are identical. | Unverifiable | Live-run evidence outside the diff; cannot be verified here. |
| AT2 — the bundle boots (build and start, health 200 with identity). | Unverifiable | Requires building and running the bundle; outside the diff. |
| AT3 — no v1 import remains, anywhere (grep). Residual non-import mentions are fine. | Unverifiable | Repo-wide grep cannot be performed within this review context beyond the shown hunks. The visible code changes remove v1 imports in the touched files, but a full-tree assertion is out of scope here. |
| AT4 — REPLACED (coexistence withdrawn). Replacement asserts removal is complete and consistent — @modelcontextprotocol/sdk appears in neither package.json nor bun.lock, and the four v2 packages resolve from node_modules at 2.0.0. | Not Met | bun.lock in the diff still contains multiple @modelcontextprotocol/sdk entries (e.g., an explicit mapping for "@modelcontextprotocol/sdk@1.17.0" and numerous nested sections). This contradicts the replacement acceptance test as written. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| getToolsCallHandler | function | src/mcp/server.test.ts — imports and uses to obtain tools/call handler, src/mcp/server-response-size-guard.test.ts — imports and uses, src/mcp/drift-gate.test.ts — imports and uses, src/mcp/server-in-flight-tool-calls.test.ts — imports and uses | Adopted | New test-support export scoped to tests under src/mcp; consumers present in four suites as shown in the diff. |
Documentation impact
- no-update-needed — This PR migrates internal MCP SDK imports to the v2 package family at protocol parity without changing the serving entry or documented behavior. No new user-facing commands, flags, or behavior changes are introduced; server remains hand-constructed (no createMcpHandler/serveStdio). Documentation updates are not included in the PR and do not appear necessary for parity-only internal dependency changes.
…ect the criterion that was wrong
BLOCKING: "@modelcontextprotocol/sdk still present in bun.lock" — AT4 said it should
appear in neither package.json nor bun.lock. The finding is correct and the criterion
was wrong: @modelcontextprotocol/inspector@0.16.2 (a devDependency, out of scope) and
its three sub-packages all declare "@modelcontextprotocol/sdk": "^1.17.0", so a v1
lockfile entry is not removable by this task at all. package.json itself declares no
v1 dependency.
But the finding pointed at something real that the wording hid: bun hoists the
transitive v1 to node_modules/@modelcontextprotocol/sdk, so a stray
`import "@modelcontextprotocol/sdk"` would still RESOLVE and typecheck. The migration
had no regression guard — typecheck, tests and the existing lint set would all pass on
a file that reverted to v1.
So this does not just reword the criterion. eslint.config.js gains a
no-restricted-imports pattern banning "@modelcontextprotocol/sdk" and its subpaths,
which is now what actually enforces SC1. Negative control, run and observed:
$ printf 'import { Server } from "@modelcontextprotocol/sdk/server/index.js";
import { McpError } from "@modelcontextprotocol/sdk";' > control.ts
$ bunx eslint control.ts
1:1 error '@modelcontextprotocol/sdk/server/index.js' import is restricted ... no-restricted-imports
2:1 error '@modelcontextprotocol/sdk' import is restricted ... no-restricted-imports
✖ 2 problems (2 errors, 0 warnings)
Both the bare specifier and the subpath form fire. Control file deleted after the run.
NON-BLOCKING (1) — helper ctx lacks mcpReq.notify: correct, and more than
forward-looking. server.ts:1471 passes ctx.mcpReq.notify to buildProgressReporter
whenever a request carries a progressToken (mt#2677). No test sets one today, so the
gap is latent, but the first that did would fail on an undefined callee rather than on
its own assertion. Added an async no-op default — the spec makes progress delivery
optional — with the spread staying caller-first so a test asserting on progress
overrides it without touching the helper.
NON-BLOCKING (2) — tests still reach private _requestHandlers via the helper:
acknowledged, no change. That is mt#4844's scope, and this PR reduced the surface from
fourteen reaches to one to make that migration a single edit. The helper's docblock
records it.
AT4 amended in the spec to its achievable form: no direct dependency, v2 resolving at
2.0.0, every remaining bun.lock v1 entry transitive via the inspector, and the lint
rule failing any source file that imports v1.
Verification: typecheck clean across 8 projects; lint clean across 4295 files; the six
src/mcp suites pass individually (46/28/1/3/4/23, 0 fail).
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Re-verification of R1 blocking found it addressed: the v1 SDK remains in bun.lock only as a transitive devDependency via @modelcontextprotocol/inspector, and the PR added an ESLint no-restricted-imports rule to prevent any source from importing v1. The migration to v2 packages is reflected in package.json and src imports, and no serving-entry changes were introduced. I found one minor issue in the new test helper: the context merge order overwrites a caller-provided ctx.mcpReq despite the comment promising caller-first; suggested a small merge fix. Otherwise, changes look coherent and low-risk, with codemod vs hand-edits clearly documented. Approving — please consider the non-blocking helper fix.
Findings
- [NON-BLOCKING] src/mcp/test-support/tools-call-handler.ts:59 — Caller override order is inverted: helper overwrites a provided
ctx.mcpReqdespite comment claiming caller-first
The returned wrapper builds the context as{ ...(ctx as Record<string, unknown>), mcpReq: { requestState: () => undefined, notify: async () => {} } }. This places the defaultmcpReqAFTER the spread, so anymcpReqthe caller supplies is overwritten. The block comment above states the opposite — that the spread is caller-first so tests can override defaults — but the code does not match it.
Suggested fix: move the defaults before the spread, or merge at the field level so caller wins, e.g. handler(request, { mcpReq: { requestState: () => undefined, notify: async () => {}, ...(ctx as any).mcpReq }, ...(ctx as Record<string, unknown>) }). This keeps today's behavior and allows future tests to inject a custom mcpReq without editing the helper.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 — every in-scope SDK import site is on v2; no @modelcontextprotocol/sdk imports remain and package.json dependency entry is updated. | Met | package.json: dependencies include @modelcontextprotocol/{server,client,node,core} at ^2.0.0 and contain no @modelcontextprotocol/sdk entry. ESLint rule added to forbid importing v1: eslint.config.js:746-757 adds no-restricted-imports pattern banning "@modelcontextprotocol/sdk" and subpaths. |
| SC2 — no wire change, measured not assumed (protocol negotiation identical before/after). | Unverifiable | Live-run negotiation evidence is not present in the repo diff. The criterion depends on execution output (AT1); this review cannot observe external run results. |
| SC3 — WITHDRAWN; replacement: v1 removed outright (no package.json entry; build/lint/tests clean). | Met | package.json has no @modelcontextprotocol/sdk dependency and v2 packages are present. Lint enforcement added in eslint.config.js to block any future v1 imports. |
| SC4 — the codemod's output is reviewed, not merged blind; PR body states which files were codemodded vs hand-edited. | Met | PR description includes sections "Key changes — what the codemod did vs. what was hand-edited" and enumerates codemod rewrites and manual fixes (diagnostic-capture.ts ctx mapping; tools/list inputSchema; test harness helper). |
| SC5 — no serving-entry change (no createMcpHandler/serveStdio/versionNegotiation introduced). | Met | src/mcp/server.ts continues to construct a hand-built Server via new Server(...) and StdioServerTransport/NodeStreamableHTTPServerTransport. No references to createMcpHandler/serveStdio/versionNegotiation are present. |
| validate_typecheck/lint clean; gated tests pass; bundle-boot smoke passes. | Unverifiable | These are CI/run-time outcomes and not encoded in the repo files. The diff shows no failing test artifacts; execution results are not observable from the code alone. |
Documentation impact
- no-update-needed — This PR migrates internal SDK imports from v1 to the v2 package family at protocol parity with no serving-entry changes. No new CLI/API surface or behavior is introduced; docs are unaffected. I checked for any docs specifically referencing the server construction entry (createMcpHandler/serveStdio) and none are added in this diff.
…nd cover it
NON-BLOCKING, and correct: the docblock claimed the caller's ctx was spread
caller-first, while the literal `mcpReq` key sat AFTER the outer spread and therefore
overwrote whatever the caller passed. A comment asserting behaviour the code does not
have is worse than no comment, so this fixes the code rather than the sentence.
The outer spread cannot express the intent — it sets sibling keys, so any literal
`mcpReq` after it discards the caller's. The defaults now live INSIDE `mcpReq` with the
caller's own members spread over them, which gives the documented semantics: supply one
member and it wins, supply none and both defaults stand, supply `notify` and
`requestState` still survives.
Added `src/mcp/test-support/tools-call-handler.test.ts` (4 tests) so the contract is
enforced rather than asserted — the helper is now load-bearing for four suites and
mt#4844 will build on it, and its own behaviour had no coverage at all. That absence is
what let the inverted ordering ship in the first place.
Negative control, run and observed: with the caller spread removed from the helper,
(fail) getToolsCallHandler (mt#4854) > a caller-supplied mcpReq member WINS over the
default, and the others survive
3 pass / 1 fail
Exactly the discriminating test fails; the other three are insensitive to the ordering
by construction, which is stated in the file so a later reader does not mistake their
passing for coverage of this property.
Verification: typecheck clean across 8 projects; lint clean across 4296 files; the new
suite 4 pass / 0 fail; the four suites that consume the helper unchanged at 46/28/1/3,
0 fail.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Re-verified the latest commit and scoped diff. The prior BLOCKING concern about lingering v1 SDK imports is addressed elsewhere in the PR (package.json shows only v2 packages); nothing in this fix commit regresses that. The new getToolsCallHandler helper correctly supplies the v2-required ctx.mcpReq members with sensible defaults and now spreads caller overrides last, matching the stated contract; accompanying tests exercise both defaulting and override order. I find no critical new defects introduced by these additions. Some process assertions (wire-parity measurements, codemod provenance) are inherently unverifiable from source, but do not block merge. Verdict: APPROVE.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 — every in-scope SDK import site is on v2. | Met | server tests and helpers import only v2 packages: e.g., src/mcp/server.test.ts:7-8 import from "@modelcontextprotocol/node" and "@modelcontextprotocol/server" (v2 family). No v1 package appears in package.json dependencies; only v2 packages are declared (package.json: dependencies). |
| SC2 — no wire change, measured not assumed. | Unverifiable | This criterion depends on live negotiation measurements (AT1) outside the diff. The PR body claims these were run, but that evidence is not a repo artifact. Unable to verify negotiated protocol-version parity from files alone. |
| SC3 — WITHDRAWN, premise false. Replaced by: v1 is removed outright. | Met | package.json shows no @modelcontextprotocol/sdk dependency. Only v2 packages (@modelcontextprotocol/server|client|node|core) are present (package.json: dependencies). Import sites in changed tests also reference v2 packages (src/mcp/server.test.ts:7-8). |
| SC4 — the codemod's output is reviewed, not merged blind. | Unverifiable | This is a process assertion documented in the PR body rather than a code artifact. The repo changes are consistent with both codemod and hand edits, but the review-vs-codemod provenance itself cannot be verified from source files. |
SC5 — no serving-entry change. src/mcp/server.ts keeps constructing a hand-built Server; createMcpHandler / serveStdio are NOT introduced here. |
Met | src/mcp/server.test.ts continues to exercise a hand-constructed SDK Server via stdio/HTTP transports, and no imports of createMcpHandler/serveStdio or version-negotiation helpers are present in visible imports (src/mcp/server.test.ts:6-13). The package.json dependencies include only core v2 packages and do not add @modelcontextprotocol/hono or web-entry adapters that would imply a serving-entry change. |
| validate_typecheck/lint/tests/bundle smoke clean | Unverifiable | These are CI-run commands whose outputs are not in-repo artifacts. While the PR body lists passing runs, we cannot verify from source alone in this review context. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| getToolsCallHandler | function | src/mcp/server.test.ts:13 — imported and used to drive tools/call via SDK server, src/mcp/test-support/tools-call-handler.test.ts:12 — imported for unit tests of the helper | Adopted |
Documentation impact
- no-update-needed — This PR introduces internal test-support helper code and migrates SDK imports to the v2 package family at protocol parity, with no serving-entry change and no user-facing CLI or API surface changes. package.json shows dependency swaps only (no new commands or flags). No docs files are touched in the diff, and no behavior changes are introduced per the task spec (SC5).
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
The prior BLOCKING concern about lingering v1 MCP SDK usage is addressed: package.json removes the v1 SDK, v2 packages are added, and an ESLint no-restricted-imports rule now enforces against regressions. All in-scope import sites have been migrated to v2, server handlers updated for v2’s context/typing, and a new test helper consolidates fragile private-map access while providing the required v2 ctx members; accompanying tests validate override order and defaults. I found no new critical issues introduced by the fixes. One advisory, non-blocking check remains to verify LATEST_PROTOCOL_VERSION is re-exported by @modelcontextprotocol/server. Overall, the change meets the spec’s criteria within what is verifiable from the diff, and is ready to merge.
Findings
- [NON-BLOCKING] scripts/deploy-minsky-mcp.ts:67 — NEEDS VERIFICATION:
LATEST_PROTOCOL_VERSIONsource package in v2
The import was changed from@modelcontextprotocol/sdk/types.jsto@modelcontextprotocol/server(scripts/deploy-minsky-mcp.ts:67andscripts/verify-mcp-hidden-params.ts:23). In v2, some constants moved to@modelcontextprotocol/coreand may or may not be re-exported byserver. IfLATEST_PROTOCOL_VERSIONis not re-exported by@modelcontextprotocol/server, this will break typecheck/runtime. If CI passed, this is fine — otherwise, consider importing from@modelcontextprotocol/coredirectly. (This is advisory; I could not verify the package’s actual exports from the diff.)
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 — every in-scope SDK import site is on v2. | Met | package.json: removes "@modelcontextprotocol/sdk" and adds v2 packages (@modelcontextprotocol/server|client|node|core). Updated imports across touched files reference only v2 packages, e.g., src/mcp/server.ts:1-7 (now imports from "@modelcontextprotocol/server", "@modelcontextprotocol/server/stdio", and "@modelcontextprotocol/node"); src/mcp/server.test.ts:9-14; src/mcp/server-tool-name-resolution.test.ts:25-26 (now "@modelcontextprotocol/client"); scripts/* now import LATEST_PROTOCOL_VERSION from "@modelcontextprotocol/server". |
| SC2 — no wire change, measured not assumed. | Unverifiable | This criterion requires live negotiation/version checks (AT1). The diff alone cannot demonstrate wire identity. No in-repo artifact records the before/after capture. Marking Unverifiable based on diff-only review. |
| SC3 — WITHDRAWN … Replacement: v1 is removed outright. | Met | package.json:168-176 — removes "@modelcontextprotocol/sdk". eslint.config.js:746-758 adds a no-restricted-imports ban for "@modelcontextprotocol/sdk" and subpaths to prevent regressions. bun.lock shows v2 packages added and any remaining v1 entries are transitive via inspector (not direct deps). |
| SC4 — the codemod's output is reviewed, not merged blind. | Unverifiable | The requirement is a PR-body statement of codemod vs hand edits. This cannot be verified from source files alone; the code changes do reflect targeted hand edits (e.g., diagnostic-capture ctx mapping, handler schema-to-string changes), but the presence of a PR-body declaration is not a repo artifact. |
| SC5 — no serving-entry change (no createMcpHandler/serveStdio). | Met | src/mcp/server.ts continues to construct a hand-built Server and Stdio/Node transports; no imports or references to createMcpHandler/serveStdio/versionNegotiation appear in the diff. The entry remains the custom class MinskyMCPServer. |
| validate_typecheck/lint/tests pass; bundle-boot smoke passes. | Unverifiable | Build/test execution status is not observable from the diff. While eslint.config.js was updated and tests were added/updated, CI/run outputs are not part of repo content. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| src/mcp/test-support/tools-call-handler.getToolsCallHandler | function | src/mcp/server.test.ts:547 — retrieves tools/call handler via helper, src/mcp/server-response-size-guard.test.ts:65 — uses helper to fetch handler (3 sites), src/mcp/drift-gate.test.ts:386, 422 — uses helper to fetch handler, src/mcp/server-in-flight-tool-calls.test.ts:48 — uses helper to fetch handler, src/mcp/test-support/tools-call-handler.test.ts — direct tests of the helper | Adopted |
Documentation impact
- no-update-needed — This PR migrates internal MCP SDK imports from v1 to the v2 package family at protocol-parity, with no serving-entry change (no createMcpHandler/serveStdio) and no intended wire-level behavior change. The only user-facing surface touched is test-only helpers and internal server wiring; CLI commands, routes, and documented behavior remain the same. I did not find docs referencing the specific v1 import paths; therefore no docs addition or invalidation is required.
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.
Summary
@modelcontextprotocol/sdk(v1) never implements the MCP 2026-07-28 revision at any published version — npm carries exactly one dist-tag,latest→1.30.0(published 2026-07-27, one day before the spec), whoseLATEST_PROTOCOL_VERSIONis2025-11-25, with noinputResponses/resultTypeanywhere in it. Support ships in a separate v2 package family (@modelcontextprotocol/core|client|server|node, all2.0.0).This migrates every SDK import site to v2 at current-protocol parity: no wire change, no serving-entry change. Adopting 2026-07-28 stays with the parent mt#4608, gated on a decision ask#11232 deliberately deferred. This is the vendor's own staging —
upgrade-to-v2.mdandsupport-2026-07-28.mdare two separate guides.Key changes — what the codemod did vs. what was hand-edited (SC4)
Codemod (
bunx @modelcontextprotocol/codemod v1-to-v2), reviewed not merged blind. Its dry run matched the planned file set exactly — 34 changes across 10 files +package.json— and every rewrite was checked against the SDK's ownimportMap.ts:Server/spec types →/server,StdioServerTransport→/server/stdio,StreamableHTTPServerTransport→NodeStreamableHTTPServerTransportfrom/node(Express hands us Nodereq/res, matching the guide's decision rule),JSONRPCMessageSchema→/core,Client+InMemoryTransportboth from/client(the guide requires one package per linked pair),McpError/ErrorCode→ProtocolError/ProtocolErrorCode.Hand-edited — three things the codemod flagged but could not resolve (8 warnings, 6
@mcp-codemod-errormarkers, all discharged):diagnostic-capture.ts— v1's flatextrabecame v2's structuredctx, and the two fields this capture reads moved differently:sessionIdstayed top-level,_metamoved toctx.mcpReq._meta(per the SDK'scontextPropertyMap.ts). Readingctx._metawould not throw — it would silently captureundefinedforever, and nothing asserts on this research output.server.tstools/list— v2 typesTool.inputSchemaas{ type: "object"; … }where v1 took a bareobject. The{}fallback became{ type: "object" }. Not a wire change: the only productionaddToolcaller (command-mapper.ts:508) always supplies aninputSchema, so the fallback is unreachable there. Corrected rather than cast away — a bare{}was never spec-valid.tools/callthrough_invokeInputRequiredCapableHandler, which readsctx.mcpReq.requestState()before delegating, so the bare{}these tests passed asextranow throws inside the SDK. Fourteen reaches into the SDK's private_requestHandlersmap collapse into onesrc/mcp/test-support/tools-call-handler.tshelper — which also reduces mt#4844's migration from fourteen sites to one.Review response
R1 BLOCKING — "
@modelcontextprotocol/sdkstill present in bun.lock". The finding is correct and my criterion was wrong.@modelcontextprotocol/inspector@0.16.2(a devDependency, out of scope) and its three sub-packages all declare"@modelcontextprotocol/sdk": "^1.17.0", so a v1 lockfile entry is not removable by this task at all.package.jsonitself declares none.It pointed at something real that the wording hid: bun hoists the transitive v1 to
node_modules/@modelcontextprotocol/sdk, so a strayimport "@modelcontextprotocol/sdk"would still resolve and typecheck — the migration had no regression guard. Fixed with an ESLintno-restricted-importsban on the package and its subpaths, which is now what enforces SC1. AT4 amended to its achievable form.R1 NON-BLOCKING (notify) — correct, and latent rather than hypothetical:
server.ts:1471passesctx.mcpReq.notifytobuildProgressReporterwhenever a request carries aprogressToken. Added an async no-op default. Class scan run:grep -rn 'ctx\.mcpReq\.'oversrc/packages/scriptsshowsnotifyis the only other member our source reaches, so the helper now supplies exactly the reachable set.R1 NON-BLOCKING (private
_requestHandlers) — acknowledged, no change. That is mt#4844's scope; this PR shrank the surface from fourteen reaches to one to make that a single edit.R2 NON-BLOCKING — "caller override order is inverted". Correct, and it was a comment contradicting its own code: the literal
mcpReqsat after the outer spread, so a caller-suppliedmcpReqwas silently discarded while the docblock claimed caller-first. Fixed by spreading the caller'smcpReqover the defaults, and covered by a new test file so the contract is enforced rather than asserted.Spec deviations recorded
grep -lon the package string, which counts mentions as imports.src/commands/mcp/start-command.ts,services/reviewer/src/mcp-client.tsandscripts/mt2677-live-verify-progress-notifications.tsonly name the SDK in comments — the reviewer client's matched line literally reads "Plain fetch only — no@modelcontextprotocol/sdkdependency". The codemod independently confirmed 10.Known overlap, acknowledged rather than discovered at merge
PR #3412 (mt#4639, open since 2026-08-27) touches 3 of this PR's files:
src/mcp/server.ts(its import block),scripts/deploy-minsky-mcp.ts,scripts/verify-mcp-hidden-params.ts. Proceeding deliberately: #3412 already owes a rebase regardless of this PR (#3532 merged and modified two of its files), the edits are semantically independent (catch bodies vs. import specifiers), and blocking a 10-file migration on a 313-file PR inverts the cost.Testing
Execution evidence:
SC1 — no v1 import remains anywhere:
Every remaining specifier is v2:
/server(Server, spec types, ProtocolError/ProtocolErrorCode, LATEST_PROTOCOL_VERSION),/server/stdio(StdioServerTransport),/client(Client, InMemoryTransport),/node(NodeStreamableHTTPServerTransport),/core(JSONRPCMessageSchema).SC2 — no wire change, measured live rather than assumed:
SC5 — the serving entry is untouched; the 2026-07-28 opt-in is NOT introduced:
Tests — each
src/mcpfile in its own process. Passing several to onebun testinvocation reproduces the Bun defect CLAUDE.md documents (no summary, exit 0), which is a non-answer, not a pass, and is not counted here:validate_typecheckclean across 8 projects;validate_lintclean across 4296 files.Bundle-boot smoke — asserting the health body's service identity, not just the status code (per mt#3148):
Negative control — three, each run and observed rather than asserted:
Round 2 is worth naming for the reviewer: those three files break from the dependency change while the related-test selector keys on the files the diff changed, so it could not reach them by construction. A dependency upgrade's blast radius is its dependents, not its diff. Filed as mt#4862.
Deploy verification:
src/mcp/server.tsships inminsky-mcp, so this is deploy surface. After merge I will rundeployment_wait-for-latestwithnotBefore= the merge timestamp andexpectCommitSha= the merge SHA, readbuildIdentity, and assert the/healthbody'sservicefield, per §10.