codemod iterations#2386
Conversation
🦋 Changeset detectedLatest commit: f2b3bc8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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: |
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
Needs a CI check - seems to fail a build
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| Published specs are resolved to concrete versions via `npm view` at startup (a failure aborts the run). Each `@modelcontextprotocol/*` package is resolved **independently** — they are not released in lockstep. Results are written to a per-run directory keyed on the resolved |
There was a problem hiding this comment.
🟡 The Modes section states 'Published specs are resolved to concrete versions via npm view at startup (a failure aborts the run)', but the per-package --sdk-version resolution loop in batchTest.ts deliberately tolerates a miss with a warning and continues using the codemod-written range — only the codemod-version and the no---sdk-version representative-label resolutions actually abort. Consider qualifying this sentence (and the matching code comment 'Any npm-view failure aborts before work begins.') to describe the tolerate-with-warning behavior, or making per-package misses abort.
Extended reasoning...
What the doc claims vs. what the code does
The README's new Modes section says: "Published specs are resolved to concrete versions via npm view at startup (a failure aborts the run). Each @modelcontextprotocol/* package is resolved independently…" Because the abort claim is immediately followed by the per-package-resolution sentence, a reader naturally takes "a failure aborts the run" to cover the per-package --sdk-version resolution as well.
In main() (packages/codemod/src/bin/batchTest.ts), only two resolution paths can actually reach the outer catch → process.exit(1):
- The codemod-version resolution (
--codemod=published). - The representative-label resolution of
@modelcontextprotocol/serverwhen--sdk=publishedwith no--sdk-versionand--codemod=local.
The per-package --sdk-version loop, by contrast, wraps each resolvePublishedVersion(pkg, opts.sdkVersion) call in its own try/catch that only emits console.warn("Warning: <pkg>@<spec> did not resolve; leaving its codemod-written range in place.") and continues. The inline comment explicitly documents this as intentional ("Tolerate a per-package miss with a warning + continue — mirroring packLocalPackages' per-pack tolerance"), since the SDK packages are not released in lockstep. The code comment a few lines above ("Any npm-view failure aborts before work begins.") carries the same over-claim as the README.
Concrete walk-through
- Run
batch-test -- --sdk=published --sdk-version=2.0.0-alpha.3 --codemod=published. @modelcontextprotocol/codemod@latestresolves fine → no abort.- The per-package loop hits
@modelcontextprotocol/core@2.0.0-alpha.3, which doesn't exist (core's latest is alpha.1).resolvePublishedVersionthrows, the innercatchprints a warning, and the loop moves on. - The run proceeds:
coreis absent fromsdkVersions, sorewriteToPublishedVersionleaves whatever range the codemod wrote for it, while the other packages are pinned to alpha.3. - An operator who fat-fingers
--sdk-version(e.g. a typo'd spec that resolves for nothing) gets a full run with all packages on codemod-written ranges and ansdk-from-codemod-style mixed result, instead of the abort the README promised — easy to miss in a long log.
Why this matters and how to fix
This is purely a doc/code mismatch in internal dev-tooling, and the tolerate-with-warning behavior itself is reasonable and intentionally chosen. But the prose promises stricter behavior than the implementation ships, which can mislead someone diagnosing a run with unexpectedly mixed pins. The fix is a one-sentence clarification in the README (e.g. "…a failure resolving the codemod version or the representative SDK label aborts the run; a per-package --sdk-version miss only warns and falls back to the codemod-written range") plus the same tweak to the code comment — or, alternatively, making per-package misses abort if strictness is preferred. The flag table's --sdk-version row could also mention the fallback.
| // Restore any shebang a transform dropped. Done after comment insertion so the inserted | ||
| // comments (positioned against the post-transform text) shift down with the code uniformly. | ||
| for (const [filePath, shebang] of shebangs) { | ||
| const sf = project.getSourceFile(filePath); | ||
| if (sf && !sf.getFullText().startsWith('#!')) { | ||
| sf.insertText(0, shebang); | ||
| } | ||
| } | ||
| project.saveSync(); |
There was a problem hiding this comment.
🟡 When a transform drops a leading shebang, the diagnostic line numbers in RunnerResult are resolved against the shebang-stripped text and never adjusted after the shebang (plus captured blank lines) is re-inserted at position 0, so the file:line values printed by the CLI and recorded in batch-test report.json point N lines too high (N = 2 in the new fixture) relative to the saved file. Bumping diag.line for files in the shebangs map (or restoring the shebang before line resolution) would keep them accurate.
Extended reasoning...
What happens
Diagnostic line numbers are finalized in the per-file loop via d.resolveCurrentLine() (runner.ts ~193-201), i.e. against the post-transform text. For a file where the imports transform consumed the line-1 shebang as leading trivia of the first import — the exact case this PR addresses — that post-transform text no longer contains the shebang line(s). The new restoration loop (runner.ts:228-235) then runs after insertDiagnosticComments() and re-inserts the captured shebang (plus any trailing blank lines) at position 0 before project.saveSync(). Every line of the saved file shifts down by N, but Diagnostic.line is never bumped to match.
Where it's visible
The numbers aren't purely internal: formatDiagnostic (utils/diagnostics.ts:43) prints them as ${d.file}:${d.line} in CLI verbose/diagnostics output, and the batch test records them in report.json → codemod.diagnostics. The inserted in-file /* CODEMOD */ comments are not affected — they were positioned against the same stripped text and shift down uniformly with the code, so they stay adjacent to the right statements. Only the numeric line values drift.
Concrete walk-through
Take the new integration-test fixture shape with a diagnostic added:
- Input: line 1
#!/usr/bin/env node, line 2 blank, line 3 the v1 import, line 4 a statement that emits a diagnostic. - The imports transform rewrites the import and drops the shebang + blank line (leading trivia). Post-transform text: line 1 = import, line 2 = the diagnostic statement.
resolveCurrentLine()runs against this text →diag.line = 2.insertDiagnosticComments()inserts the comment above line 2 of this text — correct relative position.- The restoration loop re-inserts
"#!/usr/bin/env node\n\n"at position 0 and saves. In the saved file the flagged statement (and its comment) now sit ~2 lines lower, but the returned diagnostic still says line 2 — pointing at the import (or blank line) instead.
A user following src/cli.ts:2 … from the CLI output, or tooling reading report.json, lands on the wrong line.
Why nothing else catches it
Nothing downstream re-resolves lines: the restoration loop only mutates the source files, and the new shebang integration test only asserts on the saved text, not on result.diagnostics line numbers. This skew is newly introduced by the restoration approach (pre-PR the shebang was simply lost, so the numbers happened to match the broken saved file).
Why it's a nit
It only triggers for shebang files that both lose the shebang and emit diagnostics; the offset is small (typically 1-2 lines); and the primary UX — the inserted in-file comments — remains correctly placed. Line numbers were also already best-effort relative to the final file (they don't account for previously inserted comment lines either).
Fix
In the restoration loop, count the newlines in the restored shebang string and bump diag.line for every diagnostic on that file (in allDiagnostics/fileResults); or alternatively restore the shebang before the resolveCurrentLine() pass / comment insertion so all positions are computed against the final text.
| // Record the actually-installed SDK versions (best-effort) so summary.sdkVersions is truthful for | ||
| // every mode: published-pinned, sdk-from-codemod (versions only known post-install), and local (the | ||
| // workspace versions packed into the tarballs the clone depends on). Done AFTER the checks because a | ||
| // monorepo target's deps are installed into <pkg.dir>/node_modules by the check command itself — the | ||
| // Step-6 reinstall runs at the clone root and, under --ignore-workspace, never touches a subdirectory | ||
| // package. Resolve against the package's node_modules first, then fall back to the clone root for | ||
| // hoisted single-package layouts; for a root package (pkg.dir = '.') the two coincide (deduped). The | ||
| // summary's `config` records the SDK source, so a bare version here is unambiguous. | ||
| for (const sdkPkg of PUBLISHABLE_V2_PACKAGES) { | ||
| const candidates = [ | ||
| ...new Set([ | ||
| path.join(fullPkgDir, 'node_modules', sdkPkg, 'package.json'), | ||
| path.join(clonePath, 'node_modules', sdkPkg, 'package.json') | ||
| ]) | ||
| ]; | ||
| for (const candidate of candidates) { | ||
| try { | ||
| const installed = JSON.parse(readFileSync(candidate, 'utf8')) as { version: string }; | ||
| sdkVersions[sdkPkg] = installed.version; | ||
| break; | ||
| } catch { | ||
| // not installed at this location — try the next candidate | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 The sdkVersions map is shared between the startup pinning logic and the post-install "record actually-installed versions" loop, so in --sdk=published --sdk-version=<spec> mode a package that failed to resolve at the spec (and is intentionally left unpinned) gets hard-pinned for every subsequent repo to whatever version the first repo happened to install. Recording into a separate map used only for summary.sdkVersions would keep the pinning behavior consistent across repos.
Extended reasoning...
What the bug is. main() uses a single const sdkVersions: Record<string, string> object for two distinct purposes: (1) at startup it is populated from --sdk-version resolution and later passed to rewriteToPublishedVersion(pkgJsonPath, sdkVersions) in Step 6 to pin each clone's v2 deps, and (2) after every package's post-codemod checks, the "Record the actually-installed SDK versions" loop (lines ~710–726) writes the version found in node_modules for every package in PUBLISHABLE_V2_PACKAGES back into that same object. There is no copy or separate map, so the recording loop mutates the pinning input for all later iterations.
The code path that triggers it. Run with --sdk=published --sdk-version=2.0.0-alpha.3 against a manifest with two repos, where one package (say @modelcontextprotocol/core) has no 2.0.0-alpha.3 release. At startup, the resolution loop's catch warns and deliberately leaves core out of sdkVersions — the inline comment promises: "A skipped package is left out of sdkVersions, so rewriteToPublishedVersion keeps whatever range the codemod wrote for it."
Step-by-step:
- Repo Better validation of data coming over the wire #1, Step 6:
rewriteToPublishedVersionpinsserver/client/etc. to2.0.0-alpha.3and correctly leavescoreat the codemod-written range^2.0.0-alpha.1. ✅ matches the comment. - Repo Better validation of data coming over the wire #1, post-checks: the recording loop reads
node_modules/@modelcontextprotocol/core/package.jsonand writes e.g.sdkVersions['@modelcontextprotocol/core'] = '2.0.0-alpha.1'into the same map. - Repo Race condition between transport start and server connection #2, Step 6:
rewriteToPublishedVersionnow sees an entry forcoreand hard-pins repo Race condition between transport start and server connection #2'scoredependency to2.0.0-alpha.1— the concrete version repo Better validation of data coming over the wire #1 happened to install — instead of keeping the codemod-written range as documented.
Why nothing prevents it. The recording loop's own comment shows it exists only to make summary.sdkVersions truthful across all modes; nothing scopes its writes away from the pinning path, and rewriteToPublishedVersion just uses whatever entries exist in the map at call time. There is also a secondary effect: a version found via the clone-root fallback (e.g. a hoisted/transitive copy, or a leftover when the pinned install failed) can overwrite an explicitly resolved pin, so later repos can be pinned to something other than what --sdk-version resolved to.
Impact. Behavior becomes repo-order-dependent within a single run and contradicts the documented invariant. In practice the leaked value is usually the same version the range would have resolved to anyway, and this is the internal batch-test harness rather than shipped SDK code, so the practical blast radius is small — hence non-blocking.
Fix. Record the installed versions into a separate map (e.g. installedSdkVersions) used only for summary.sdkVersions (optionally seeded from the resolved pins), leaving the pinning map immutable after startup. That keeps rewriteToPublishedVersion's behavior identical for every repo and keeps the summary truthful.
| // A leading `#!` shebang is leading trivia of the first import; some transforms drop it when | ||
| // they rewrite that import, silently breaking CLI packages whose `bin` points at the compiled | ||
| // entry. Capture it now and restore it after transforms, before saving. Include any blank lines | ||
| // that followed it (also part of the same dropped trivia) so the original spacing round-trips. | ||
| const shebangMatch = originalText.match(/^#![^\n]*\n(?:[ \t]*\n)*/); | ||
| if (shebangMatch) { |
There was a problem hiding this comment.
🟡 The shebang capture/restore in runner.ts is a user-facing bugfix to the published @modelcontextprotocol/codemod package, but the PR ships no .changeset/*.md entry, so the fix won't produce a version bump or reach npm consumers. Adding a patch changeset for @modelcontextprotocol/codemod (as #2383 did for the previous codemod fix) would let this ship; the batch-test harness changes are dev-only and don't need one.
Extended reasoning...
What's missing
This PR contains two distinct kinds of changes: (1) dev-only batch-test harness work (batchTest.ts, README, repos.json, harness tests), and (2) a genuine user-facing bugfix in packages/codemod/src/runner.ts — capturing a leading #! shebang (plus its trailing blank lines) before transforms run and restoring it before project.saveSync(). The PR's own integration test describes the behavior it fixes as a "Regression: the imports transform consumed the line-1 shebang … silently breaking CLI packages whose bin points at the compiled entry." That is a behavior change in what the published codemod writes to users' files.
Why a changeset is needed
@modelcontextprotocol/codemod is a published, public package (currently 2.0.0-alpha.1, no "private": true, ships a bin, and pkg-pr-new builds it for this very PR). It is not in .changeset/config.json's ignore list, so under this repo's changesets-based release flow, behavior fixes to it only ship to npm when a changeset bumps the package. This PR adds no .changeset/*.md file, and changeset-bot has flagged "No Changeset found" on the PR.
What happens without it (step-by-step)
- The PR merges with the runner.ts shebang fix but no changeset.
- The changesets release workflow sees no pending changeset for
@modelcontextprotocol/codemod, so no version bump is created and no new version is published. - A user runs
npx @modelcontextprotocol/codemod(the latest published version) on a CLI project whose entry file starts with#!/usr/bin/env node. - They still get the old behavior: the imports transform drops the shebang, the compiled
binentry loses its interpreter line, and their CLI silently breaks after migration — even though the fix is already merged onmain. - The fix only reaches npm whenever some other change to the codemod happens to carry a changeset, with no release note crediting this fix.
Why nothing else covers it
The new integration test guards the behavior in CI, but tests don't trigger releases. Repo precedent confirms the convention: the immediately preceding codemod fix (#2383, commit 9f8ba61) shipped with .changeset/lucky-poets-refuse.md declaring a patch for @modelcontextprotocol/codemod, and earlier codemod-iteration PRs (e.g. #2354) included codemod-*.md changesets for their transform changes.
How to fix
Add a one-file changeset, e.g. .changeset/codemod-preserve-shebang.md:
---
'@modelcontextprotocol/codemod': patch
---
Preserve a leading #! shebang (and its trailing blank lines) on migrated files instead of dropping it with the first import's leading trivia.This is release-process hygiene rather than a code defect, and the maintainer may intentionally batch it into a later changeset — hence non-blocking — but as written the user-facing fix will not ship.
| export function parseNpmViewVersion(jsonText: string): string { | ||
| const parsed = JSON.parse(jsonText) as string | string[]; | ||
| if (Array.isArray(parsed)) { | ||
| if (parsed.length === 0) throw new Error('npm view returned an empty version array'); | ||
| return parsed.at(-1)!; // highest match for a range | ||
| } | ||
| return parsed; | ||
| } |
There was a problem hiding this comment.
🟡 parseNpmViewVersion returns parsed.at(-1) with the comment "highest match for a range", but npm view <pkg>@<range> version --json returns matching versions in publish order, not semver order — so this picks the most-recently-published match, which is lower than the highest whenever a backport is published after a newer matching release. The mis-resolved version would flow into the results-dir label, summary.json, and the pinned versions when --sdk-version is a range; a small semver sort/max before picking (or softening the comment) would fix it.
Extended reasoning...
What the bug is. parseNpmViewVersion (packages/codemod/src/bin/batchTest.ts:364-371) handles the array case of npm view <pkg>@<spec> version --json by returning parsed.at(-1)! with the inline comment // highest match for a range. The npm CLI builds that array by filtering Object.keys(packument.versions) with semver.satisfies, and the registry packument's versions object is keyed in publish order, not semver order. So .at(-1) is the most-recently-published matching version, not the highest one — the comment over-promises.
Concrete proof (real registry data). Run npm view \"node-fetch@>=2.6.7 <4\" version --json today: the returned array ends with \"2.6.13\", \"2.7.0\" because the 2.x backports were published after the 3.x line, even though 3.3.2 is the semver-highest match in that range. parseNpmViewVersion on that output returns 2.7.0, not 3.3.2. The same shape applies to any package the moment a patch backport lands after a newer matching release — e.g. with spec ^2.0.0, publishing 2.0.5 after 2.1.0 makes .at(-1) return 2.0.5.
The code path that triggers it. resolvePublishedVersion calls this helper, and it's hit with a range spec on two paths: (1) when the operator passes a range as --sdk-version/--codemod-version — the README's flag table explicitly advertises "npm version/tag/range" as accepted values; and (2) on the default no---sdk-version published-SDK path, where main() resolves V2_PACKAGE_VERSIONS[SERVER_PKG] (a caret range from generated/versions) for the representative directory label, which routinely produces the array branch. Exact versions and dist-tags (latest) return a JSON string and bypass .at(-1), so those common cases are unaffected.
Why nothing else prevents it. Nothing downstream re-checks the resolution: the value returned here becomes sdkVersionResolved/codemodVersionResolved and sdkVersions[pkg], and from there flows verbatim into computeResultsDirName (the results/codemod-…__sdk-… directory), summary.json → config/sdkVersions, and — when --sdk-version is a range — the concrete versions rewriteToPublishedVersion pins into each clone's package.json. The unit test for the array case (['2.0.0-alpha.1','2.0.0-alpha.2']) happens to be in both publish and semver order, so it doesn't catch the distinction.
On the refutation. A verifier argued the trigger is implausible because the @modelcontextprotocol/* packages are released via changesets and have so far only moved forward, and that exact-version/dist-tag specs bypass the array branch. Both points are fair on impact — which is why this is filed as a non-blocking nit, not a bug to hold the PR on — but the array branch is not exotic (the default no---sdk-version label resolution uses a caret range), the helper is exported as part of the module surface, backport patches are routine on npm generally, and the comment makes a specific claim ("highest match") that the implementation does not deliver. The fix costs one line.
How to fix. Either pick the semver-max instead of the last element — e.g. sort with a small prerelease-aware comparator (or semver.rcompare if a dependency on semver is acceptable) before taking the last/first element — or, if publish order is considered good enough for this internal harness, change the comment to say "most recently published match" so it doesn't mislead future readers.
| 4. Runs the codemod using the programmatic API | ||
| 5. Packs local SDK packages as tarballs and rewrites `package.json` deps to use them (so the test runs against the current SDK branch, not published npm versions) | ||
| 5. Packs local SDK packages as tarballs and rewrites `package.json` deps to use them, so the test runs against the current SDK branch — only when `--sdk=local` (the default); with `--sdk=published` the v2 deps are installed from npm instead (see [Modes](#modes)) |
There was a problem hiding this comment.
🟡 Step 4 of the "How it works" list still unconditionally says the codemod "runs ... using the programmatic API", but with the new --codemod=published mode it is instead executed by shelling out to the published CLI via npx (runCodemod() in batchTest.ts). Consider qualifying step 4 the same way step 5 was qualified for the --sdk split, e.g. "using the programmatic API (--codemod=local, the default) or the published CLI via npx (--codemod=published)".
Extended reasoning...
What the issue is
The "How it works" list in packages/codemod/batch-test/README.md describes the per-repo pipeline. Step 4 still reads:
- Runs the codemod using the programmatic API
This was accurate before this PR, when the batch test always called run() in-process. The PR introduces a --codemod flag with a published value, and the new runCodemod() helper in packages/codemod/src/bin/batchTest.ts branches on it: local keeps the in-process run() call, while published shells out with npx -y -p @modelcontextprotocol/codemod@<resolved> mcp-codemod v1-to-v2 <sourceDir> --verbose and captures raw stdout/stderr. So the unconditional "programmatic API" claim in step 4 is now wrong for one of the two supported modes.
Why this is an inconsistency introduced by this PR rather than a pre-existing gap
The same diff updated the adjacent step 5 to qualify the analogous SDK-source split ("only when --sdk=local (the default); with --sdk=published the v2 deps are installed from npm instead"), and even updated the in-code comment for this step from "Step 5: Run codemod (programmatic API)" to "Step 5: Run codemod (local in-process API, or published CLI)". Step 4 of the README is the only entry in the list that still ignores the new modes.
Why the distinction matters
It isn't purely cosmetic: the README's own Limitation paragraph (added in this PR) explains that in published-codemod mode the CLI emits text rather than structured diagnostics, so codemod.diagnostics is empty and the raw output lands under codemod.cli instead. Whether the codemod ran via the programmatic API or via npx therefore changes what data shows up in report.json, which is exactly what someone reading "How it works" before analyzing results would care about.
Step-by-step proof
- Run
pnpm --filter @modelcontextprotocol/codemod batch-test -- --codemod=published. parseArgs()setsopts.codemod = 'published';main()resolves@modelcontextprotocol/codemod@latestvianpm viewand stores it inresolved.codemodVersionResolved.- For each package, the "Step 5: Run codemod" block calls
runCodemod(resolved.codemodSource, ...)withsource === 'published', which builds thenpx -y -p @modelcontextprotocol/codemod@<ver> mcp-codemod v1-to-v2 ...command and runs it viashell()fromtmpdir()— the in-processrun()API is never invoked. - The resulting
report.jsonhascodemod.diagnostics: []and acodemod.cliblob, contradicting what a reader of step 4 ("programmatic API") would expect.
Why the impact is small / how to fix
The Modes table and the Limitation paragraph further down the same README describe the published-CLI path correctly, so a careful reader won't stay misled — this is a doc-consistency nit in internal dev tooling, not a functional bug. The fix is a one-phrase qualification of step 4, mirroring step 5, e.g.:
- Runs the codemod — using the programmatic API (
--codemod=local, the default) or the published CLI vianpx(--codemod=published; see Modes)
- typedoc: exclude src/bin from codemod docs so the batch-test harness's
exported types no longer trip treatWarningsAsErrors (fixes check:all)
- runner: shift diagnostic lines by the restored shebang's line count so
reported file:line matches the saved file
- batchTest: record post-install versions in a separate map so a
startup-unresolved package isn't retroactively pinned for later repos
- batchTest/README: correct npm-view abort wording, the parseNpmViewVersion
'highest match' comment, and 'How it works' step 4
- changeset: patch @modelcontextprotocol/codemod for the shebang fix
…tprotocol/typescript-sdk into feature/codemod-iterations-4
2 participants
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context