codemod improvements#2156
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the previous round — the CRLF offset handling, the hasMcpImports gate in handlerRegistration, the grep quoting, and the same-line merge test all look fixed in the latest commit, and I found no new bugs in this revision. Since the PR adds a new user-file-mutating behavior (writing @mcp-codemod-error comments into source) plus import-map changes, a human maintainer should still take a pass; one small residual from my earlier comment is that comment insertion still has no guard against landing inside a multi-line template literal, and the changeset bot is flagging a missing changeset.
Extended reasoning...
Overview
This PR ("codemod improvements") touches 17 files in packages/codemod. The main feature is a new actionRequired diagnostic kind that causes the runner to physically insert /* @mcp-codemod-error ... */ comments into user source files at the diagnostic location, plus a CLI hint pointing users at a grep command. Supporting changes include a shared isKeyPositionIdentifier helper extracted into astUtils, an import-map simplification for client/stdio.js, expanded mock-method coverage (unmock, requireActual, etc.), removal of the incorrect registerResource schema-wrap path, namespace-import guards in symbolRenames, shorthand-property expansion in contextTypes, and a new commentInsertion.test.ts suite.
Status relative to prior review
My previous review (2026-05-27 05:57 UTC) raised four issues. The latest commit (3e50511, pushed after that review) addresses them: the runner now computes offsets from the un-normalized text and preserves the file's line ending (with a CRLF test added), handlerRegistrationTransform now gates on hasMcpImports (with a test), the grep hint quotes the path, and the same-line merge test now drives run() end-to-end and asserts the merged comment. The only residual from that round is the lack of a safe-boundary check when the insertion line falls inside a multi-line template literal or JSX text — a narrow edge case, noted but not blocking on its own.
Security risks
None of significance. The codemod is a developer-run CLI tool operating on the user's own source tree; it does not touch the SDK runtime, transports, or auth. The grep hint is printed, not executed, and is now quoted. The main risk class is unwanted mutation of user files, which the new hasMcpImports gate and dry-run behavior mitigate.
Level of scrutiny
Moderate. This is tooling code rather than protocol/transport code, so the blast radius is limited to the migration experience, but the comment-insertion feature is a deliberate design choice (writing annotations into user code) that a maintainer should consciously sign off on, and the PR bundles several semi-independent transform fixes. The PR description is empty and there is no changeset, which a maintainer may want filled in before merge.
Other factors
Test coverage is reasonable: the new comment-insertion suite covers insertion, dry-run, idempotency, indentation, line-shift placement, same-line merging, and CRLF, and the transform test updates match the behavior changes. The bug hunting system found no bugs in this revision. Given the feature scope and the missing changeset/description, I am deferring rather than shadow-approving.
There was a problem hiding this comment.
Thanks — the remaining open issue from my last pass (no safe-boundary check before splicing the comment into multi-line template literals) is now addressed by the isInsideLiteral guard and the new template-literal/CRLF tests, and I found no new issues in this revision. Given the breadth of behavior changes here (warning→actionRequired escalation that physically writes comments into user files, the importMap stdio simplification, removal of registerResource uriSchema wrapping, expanded mock methods), this still warrants a human maintainer pass before merge — also note the missing changeset.
Extended reasoning...
Overview
This PR spans 17 files in packages/codemod: it introduces an actionRequired diagnostic kind that physically inserts /* @mcp-codemod-error */ comments into users' source files via a new insertDiagnosticComments() step in runner.ts, escalates many existing warnings to that level across the v1→v2 transforms, simplifies the client/stdio.js import mapping to target @modelcontextprotocol/client/stdio directly, removes the registerResource/uriSchema wrapping path in mcpServerApi, expands the recognized vi/jest mock methods, hardens import-merge logic against namespace imports, and adds a substantial commentInsertion.test.ts suite.
Status of prior feedback
All issues raised in my earlier reviews have been addressed in the current revision: the CRLF offset computation and the template-literal boundary guard (isInsideLiteral before sf.insertText), the missing hasMcpImports() gate in handlerRegistrationTransform, the unquoted path in the grep hint, and the same-line merge test now genuinely exercises the merge path through run(). I verified the stdio import-map simplification against packages/client/src/stdio.ts, whose subpath export covers exactly the four symbols previously listed in symbolTargetOverrides, so the simplification is behavior-preserving. The bug-hunting system found no new issues in this revision.
Security risks
None of significance — this is a developer-side migration tool, not runtime SDK/transport/auth code. The main risk class is unwanted mutation of user source files by the comment-insertion feature, which the new boundary/idempotency/CRLF tests now cover.
Level of scrutiny
Moderate. The codemod package is not protocol- or security-critical, but the new behavior of writing annotations into users' files and several intentional behavior changes (e.g. dropping the registerResource uriSchema wrapping, the diagnostic-level escalations) are design-level decisions a maintainer should sign off on rather than something to shadow-approve. The PR description is empty and there is no changeset despite the package being published, which a maintainer should also weigh.
Other factors
Test coverage for the new functionality is good (comment insertion, dry-run, idempotency, indentation, line-shift, template-literal skip, CRLF), and the transform-level test updates match the behavior changes. The remaining open questions are intent-level (scope of the escalation, the registerResource removal) rather than correctness bugs, hence deferring rather than approving.
1 participant
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context