codemod: rewrite moved error-code semantics, fix StreamableHTTPError target, preserve file headers#2240
codemod: rewrite moved error-code semantics, fix StreamableHTTPError target, preserve file headers#2240felixweinberger wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 0298cfc 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/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find correctness bugs and verified the SdkHttpError target and SdkError-raised timeout/disconnect claims against docs/migration.md and packages/core, but the new ~250-line heuristic error-code-semantics transform rewrites user code semantics, so it deserves a human design pass.
Extended reasoning...
Overview
This PR is confined to packages/codemod: a new error-code-semantics transform (registered third in the v1→v2 pipeline), a corrected StreamableHTTPError → SdkHttpError rewrite in removedApis.ts, and leading-trivia (shebang/banner) preservation in importUtils.ts plus the three transforms that remove first imports. Three changesets (one minor, two patch) and ~30 new tests including an end-to-end pipeline test.
Verification
The substantive claims check out against the repo: docs/migration.md documents SdkHttpError (typed .status/.statusText) as the StreamableHTTPError replacement, and packages/core/src/shared/protocol.ts raises RequestTimeout/ConnectionClosed as SdkError — so the guard retargeting the new transform performs matches the documented v2 behavior, and the previous SdkError/error.data?.status guidance was indeed wrong. The trivia capture/restore helpers are simple and idempotent (restore is a no-op if the text still starts with the trivia).
Security risks
None. The codemod runs locally on the user's own source files; no auth, network, or transport code is touched.
Level of scrutiny
Moderate, but human-warranted. This is not runtime SDK code, so the blast radius is limited to migration output quality. However, the new transform makes heuristic AST rewrites of user code (findInstanceofGuard matches any subject instanceof X anywhere in an enclosing condition by subject text, regardless of && vs || structure). Edge cases exist where a rewrite could silently change semantics — e.g. a single guard protecting both a ProtocolErrorCode comparison and a moved-member comparison (e instanceof ProtocolError && (e.code === ProtocolErrorCode.InvalidParams || e.code === SdkErrorCode.RequestTimeout)), or an ||-joined instanceof that isn't actually a guard. The transform handles the analogous mixed cases for switches and object literals with diagnostics, but not for boolean expressions under one guard. Whether that residual risk is acceptable for a best-effort codemod, and whether the SDK wants this much heuristic complexity in the codemod at all, is a maintainer design call (Guiding Principles 1–2).
Other factors
Test coverage is strong (unit tests for each rewrite/diagnostic path, idempotence, an integration test exercising the full transform order), the ordering comment in transforms/index.ts was updated consistently, and the changeset prose matches the implementation. No outstanding reviewer comments; only the changeset bot has commented.
ErrorCode.RequestTimeout and ErrorCode.ConnectionClosed moved to SdkErrorCode (a string enum raised on SdkError) in v2. The symbol rename alone leaves guards like 'e instanceof ProtocolError && e.code === SdkErrorCode.RequestTimeout' compiling but never matching at runtime. The new transform rewrites such guards to SdkError where that is unambiguous, and emits diagnostics for switches and maps that mix the two enums.
Removing the first import declaration in a file (during import consolidation, the ErrorCode split, or removed-API handling) also removed the leading trivia attached to it, silently deleting shebangs and file-header comments. Capture the trivia before mutating and restore it afterwards.
The v2 replacement for StreamableHTTPError is SdkHttpError, which extends SdkError and exposes the HTTP status as typed .status/.statusText accessors. The codemod previously rewrote references to plain SdkError and told users the status lives in error.data?.status, leaving migrated code one subclass too general and the guidance pointing at an untyped field.
…st usage Retargeting an instanceof guard to SdkError can leave the renamed ProtocolError (or McpError) import specifier unused, which consumer linters flag in migrated output. Remove the specifier when no other references remain, keeping the import when the class is still used. removeUnusedImport now preserves file-leading shebangs and banner comments when it removes an emptied first-statement import, and reports whether it removed anything.
felixweinberger
force-pushed
the
fweinberger/codemod-migration-fixes
branch
from
585c2c8 to
0298cfc
Compare
| const comparison = getComparisonContext(access); | ||
| if (comparison) { | ||
| const guard = findInstanceofGuard(access, comparison.subjectText); | ||
| if (guard) { | ||
| const className = guard.getRight().getText(); | ||
| if (V1_ERROR_CLASSES.has(className)) { | ||
| guardsToRewrite.add(guard); |
There was a problem hiding this comment.
🔴 Rewriting an instanceof ProtocolError/McpError guard to instanceof SdkError ignores sibling comparisons under the same guard against ProtocolErrorCode (or non-moved ErrorCode members), so a v1 condition like e instanceof McpError && (e.code === ErrorCode.RequestTimeout || e.code === ErrorCode.InvalidParams) migrates to a guard under which the ProtocolErrorCode.InvalidParams branch can never match — the exact silent never-matching failure this transform exists to eliminate, with no diagnostic. Mirror the existing mixed-enum handling for switches/object literals: detect ProtocolErrorCode (or non-moved ErrorCode) comparisons in the guarded condition and emit the error-level "split the condition" diagnostic instead of rewriting.
Extended reasoning...
The bug. When a single instanceof guard protects comparisons against both a moved SdkErrorCode member and a ProtocolErrorCode member, the transform decides to rewrite the guard based only on the moved-member comparison. In errorCodeSemantics.ts (lines 70–76), once findInstanceofGuard returns a guard whose right side is in V1_ERROR_CLASSES (ProtocolError/McpError), the guard is unconditionally added to guardsToRewrite — there is no check whether the same condition also contains comparisons against ProtocolErrorCode or non-moved ErrorCode members. Those sibling comparisons remain protocol-code comparisons, but the guard they depend on becomes instanceof SdkError, so they can never match.\n\nStep-by-step proof. Take very common v1 retry/classification code:\n\nts\nif (e instanceof McpError && (e.code === ErrorCode.RequestTimeout || e.code === ErrorCode.InvalidParams)) { ... }\n\n\n1. symbolRenames (handleErrorCodeSplit) rewrites each member individually: RequestTimeout → SdkErrorCode.RequestTimeout (it is in ERROR_CODE_SDK_MEMBERS), InvalidParams → ProtocolErrorCode.InvalidParams, and McpError → ProtocolError. Result: e instanceof ProtocolError && (e.code === SdkErrorCode.RequestTimeout || e.code === ProtocolErrorCode.InvalidParams).\n2. errorCodeSemantics collects only the SdkErrorCode.RequestTimeout access (ProtocolErrorCode is not in MOVED_MEMBER_ENUMS, so the sibling is invisible to it).\n3. getComparisonContext matches (the other side is e.code), and findInstanceofGuard walks up through the || and && binaries to e instanceof ProtocolError.\n4. ProtocolError is in V1_ERROR_CLASSES, so the guard goes into guardsToRewrite with no inspection of the rest of the condition, and is replaced with e instanceof SdkError.\n5. Output: e instanceof SdkError && (e.code === SdkErrorCode.RequestTimeout || e.code === ProtocolErrorCode.InvalidParams). In v2, an InvalidParams error is raised as a ProtocolError, never an SdkError, so the InvalidParams half of the condition is now unreachable. Before this PR (and after symbolRenames alone), that half still worked at runtime.\n\nWhy nothing catches it. No diagnostic is emitted for this case — the guard rewrite path has no mixed-enum detection, unlike the switch handler (which emits the error "switch mixes SdkErrorCode and ProtocolErrorCode cases … Split into …") and the object-literal handler (which flags mixed keys). There is also no test covering a mixed comparison chain under one guard, so the gap is invisible to the suite. This contradicts the PR's own stated principle: where a rewrite is not safely automatable, prefer an error-level diagnostic over guessing, because the failure mode being targeted is silent.\n\nImpact. Realistic error-classification/retry code that distinguishes timeouts from protocol errors under one McpError guard gets silently corrupted: the protocol-code branch (e.g. reporting InvalidParams) stops firing after migration, with no compile error under loose typing and no codemod diagnostic. That is precisely the silent never-matching breakage the new transform was added to eliminate, just shifted from the SDK member to the protocol member.\n\nFix. Before adding a guard to guardsToRewrite, scan the guarded condition (the chain between the guard and the comparison, or simply the enclosing if/ternary condition) for <subject>.code comparisons against ProtocolErrorCode or non-moved ErrorCode members. If any are found, skip the rewrite and emit the same error-level diagnostic used for mixed switches, telling the user to split the condition into an instanceof SdkError branch and an instanceof ProtocolError branch. Add a test for the mixed-comparison-under-one-guard pattern.
| function findInstanceofGuard(access: Node, subjectText: string): BinaryExpression | undefined { | ||
| let scope: Node | undefined = access; | ||
| while (scope) { | ||
| if (Node.isBinaryExpression(scope) || Node.isIfStatement(scope) || Node.isConditionalExpression(scope)) { | ||
| const searchRoot = Node.isIfStatement(scope) | ||
| ? scope.getExpression() | ||
| : Node.isConditionalExpression(scope) | ||
| ? scope.getCondition() | ||
| : scope; | ||
| for (const candidate of [searchRoot, ...searchRoot.getDescendantsOfKind(SyntaxKind.BinaryExpression)]) { | ||
| if (!Node.isBinaryExpression(candidate)) continue; | ||
| if (candidate.getOperatorToken().getKind() !== SyntaxKind.InstanceOfKeyword) continue; | ||
| if (candidate.getLeft().getText() !== subjectText) continue; | ||
| return candidate; | ||
| } | ||
| } | ||
| if ( | ||
| Node.isFunctionDeclaration(scope) || | ||
| Node.isFunctionExpression(scope) || | ||
| Node.isArrowFunction(scope) || | ||
| Node.isSourceFile(scope) | ||
| ) { | ||
| return undefined; | ||
| } | ||
| scope = scope.getParent(); | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
🔴 findInstanceofGuard() returns any subject instanceof Class found in an enclosing condition without checking that the instanceof actually guards the comparison conjunctively (&&) on the truthy path, so realistic shapes like else-if chains (if (e instanceof ProtocolError) {...} else if (e.code === SdkErrorCode.RequestTimeout) {...}), || disjunctions, and negated guards (!(e instanceof ProtocolError) && ...) get their unrelated instanceof silently rewritten to SdkError, corrupting the user's ProtocolError handling with no diagnostic. The rewrite should only fire when the instanceof is an && conjunct guarding the comparison (or the access sits in the then/whenTrue branch); otherwise fall back to the existing warning/error diagnostics.
Extended reasoning...
What the bug is
findInstanceofGuard() (errorCodeSemantics.ts:217-244) walks up from the moved-member comparison through every enclosing BinaryExpression / IfStatement / ConditionalExpression and returns the first subject instanceof <Class> found anywhere in those conditions. The candidate loop only checks the operator kind and the left-operand text — it never verifies that (a) the instanceof is a logical-AND conjunct on the path to the comparison, or (b) the comparison sits in the guarded (truthy) branch of the if/ternary. Any matching instanceof in an enclosing condition is then added to guardsToRewrite and unconditionally rewritten to instanceof SdkError.
Code path / step-by-step proof (else-if case)
Take this realistic v1-style error-classification code (post-rename spelling shown, but the pre-rename spelling triggers the same path):
if (e instanceof ProtocolError) {
handleProtocolError(e);
} else if (e.code === SdkErrorCode.RequestTimeout) {
retry();
}- The transform finds the moved-member access
SdkErrorCode.RequestTimeoutandgetComparisonContext()returnssubjectText = 'e'(the other side ise.code). findInstanceofGuard()walks up: the===BinaryExpression — no instanceof among its descendants; the innerIfStatement(the else-if), whose expression is the===— still no instanceof.- The walk continues to the parent of the else-if, which in the AST is the outer
IfStatement(an else-if is the outer if'selseStatement). ItssearchRootise instanceof ProtocolError; the operator isinstanceofand the left text is'e', so it is returned as the "guard". ProtocolErroris inV1_ERROR_CLASSES, so the outer guard is added toguardsToRewriteand rewritten toe instanceof SdkError— retargeting the protocol-error branch, which has nothing to do with the timeout comparison. The else-if comparison itself remains unguarded, and because a "guard" was found, the no-guard warning diagnostic is also suppressed. Output:
if (e instanceof SdkError) { // was: e instanceof ProtocolError — silently corrupted
handleProtocolError(e);
} else if (e.code === SdkErrorCode.RequestTimeout) {
retry(); // still effectively unreachable, and no warning emitted
}The same over-matching hits two more shapes:
||chains —if (e instanceof ProtocolError || e.code === SdkErrorCode.RequestTimeout): the||BinaryExpression is itself a scope,getDescendantsOfKind(BinaryExpression)finds the instanceof disjunct (which is not a guard at all), and it is rewritten — silently dropping the user's ProtocolError handling from the disjunction.- Negations —
!(e instanceof ProtocolError) && e.code === ...: the instanceof inside thePrefixUnaryExpressionis still a descendant of the&&chain, so it is rewritten to!(e instanceof SdkError) && ..., inverting the intended check.
Why nothing prevents it
guardsToRewrite is consumed unconditionally — every guard collected is rewritten with no further validation, and finding a "guard" suppresses the warning that an unguarded comparison would otherwise receive. The new tests only cover the conjunctive &&-chain and nested-if happy paths; none cover else-branches, ||, or negated forms.
Impact
The codemod silently mutates user error-handling code whose semantics it has not verified, with no diagnostic — exactly the silent-failure class the PR description says this transform must never reintroduce ("it prefers an error-level diagnostic … over guessing; the codemod should never reintroduce silence"). In the else-if case it actively corrupts the other branch (protocol-error handling stops matching protocol errors) while leaving the timeout branch broken and unflagged.
How to fix
Only treat an instanceof as a guard when it conjunctively (&&) dominates the comparison: while walking up, accept an instanceof only if it is a conjunct of an && chain on the path to the access, or the condition of an if/ternary whose then/whenTrue branch contains the access — and skip candidates wrapped in a negation. When no such guard exists, fall through to the existing no-guard warning (or the foreign-class error), which is the behavior the transform already has for genuinely unguarded comparisons.
| const mixesProtocolCodes = switchStmt | ||
| .getDescendantsOfKind(SyntaxKind.PropertyAccessExpression) | ||
| .some(p => Node.isIdentifier(p.getExpression()) && p.getExpression().getText() === 'ProtocolErrorCode'); |
There was a problem hiding this comment.
🟡 The mixed-enum detection for switches (lines 108-110) and object-literal keys (lines 141-143) only looks for the literal identifier ProtocolErrorCode, so when the transform runs standalone (--transforms error-code-semantics) on un-renamed v1 code, a switch/map mixing ErrorCode.RequestTimeout with ErrorCode.InvalidParams is not flagged as mixed — it gets the milder "verify instanceof SdkError" warning (which is misleading for a mixed switch) instead of the error-level "Split into ..." diagnostic. To honor the header's claim that the pre-rename spelling is recognized and the transform is safe standalone, the mixesProtocolCodes checks should also treat ErrorCode.<member not in ERROR_CODE_SDK_MEMBERS> accesses as protocol-code cases.
Extended reasoning...
The bug. The transform's header comment (and the ordering note in index.ts) states that both the post-rename (SdkErrorCode.X) and pre-rename (ErrorCode.X) spellings are recognized, so the transform "composes with the symbol rename transform in either order" and is safe standalone. The moved-member collection honors this — MOVED_MEMBER_ENUMS contains both 'SdkErrorCode' and 'ErrorCode' — and so does the guard rewrite (V1_ERROR_CLASSES includes McpError). But the two mixed-enum checks do not: both the switch path (lines 108-110) and the object-literal path (lines 141-143) compute mixesProtocolCodes by looking only for property accesses whose expression text is the literal identifier ProtocolErrorCode.\n\nCode path that triggers it. Run --transforms error-code-semantics standalone on un-renamed v1 code:\n\nts\nimport { ErrorCode } from '@modelcontextprotocol/sdk/types.js';\ndeclare const e: { code: unknown };\nswitch (e.code) {\n case ErrorCode.RequestTimeout: // moved member → SdkErrorCode/SdkError in v2\n retry();\n break;\n case ErrorCode.InvalidParams: // protocol member → ProtocolErrorCode/ProtocolError in v2\n report();\n break;\n}\n\n\nStep-by-step:\n1. The descendant scan collects ErrorCode.RequestTimeout (ErrorCode ∈ MOVED_MEMBER_ENUMS, RequestTimeout ∈ ERROR_CODE_SDK_MEMBERS); ErrorCode.InvalidParams is correctly not collected as a moved member.\n2. The access is a CaseClause expression, so control reaches the switch branch and the switch is added to flaggedSwitches.\n3. mixesProtocolCodes scans the switch's property accesses for an expression whose text is exactly 'ProtocolErrorCode'. In pre-rename code the protocol member is still spelled ErrorCode.InvalidParams, so nothing matches and mixesProtocolCodes is false.\n4. The transform emits the milder warning — "switch case on SdkErrorCode.RequestTimeout: ... Verify the switch subject is checked with instanceof SdkError" — instead of the error-level "switch mixes ... Split into an instanceof SdkError branch and an instanceof ProtocolError branch" diagnostic. The same gap applies to the object-literal path: a map mixing [ErrorCode.RequestTimeout] and [ErrorCode.InvalidParams] keys gets the "Update any Record<number, ...> typing" tail instead of "split it into separate maps".\n\nWhy existing code doesn't prevent it. The moved-member detection and guard rewrite were both extended for the pre-rename spelling, but the two mixesProtocolCodes checks were not, and the test suite only covers mixed switches/maps in the post-rename spelling (ProtocolErrorCode.InvalidParams), so nothing exercises a pre-rename mixed switch/map.\n\nImpact. The milder guidance is actively wrong for a mixed switch: if the user follows "verify the switch subject is checked with instanceof SdkError" and wraps the whole switch accordingly, the InvalidParams (protocol-error) case becomes unreachable in v2 — exactly the silent never-matches failure mode this transform exists to prevent. If the user instead runs the symbols transform later, the rename produces the impossible mixed-enum switch and no transform re-emits the split-the-switch error unless error-code-semantics is re-run.\n\nWhy this is only a nit (addressing the refutation). One verifier argued this isn't worth flagging because no code is rewritten incorrectly, a diagnostic is still emitted at the right location, the default pipeline (symbols → error-code-semantics) sees the post-rename spelling and works correctly, and the transform is idempotent so a re-run after the rename catches the mix. Those mitigations are all real and they cap the severity: this is a diagnostic-precision gap in a secondary execution mode, not a code-mutation bug, and it should not block the PR. But it is still a genuine gap relative to the transform's own documented standalone-composability claim, and the substituted warning's guidance is misleading for the mixed case rather than merely weaker — so it's worth a small fix rather than dropping entirely.\n\nFix. In both mixesProtocolCodes computations, also count a property access as a protocol-code occurrence when its expression is the pre-rename enum and the member is not a moved member, e.g.:\n\nts\n.some(p => {\n const expr = p.getExpression();\n if (!Node.isIdentifier(expr)) return false;\n const name = expr.getText();\n if (name === 'ProtocolErrorCode') return true;\n return MOVED_MEMBER_ENUMS.has(name) && !ERROR_CODE_SDK_MEMBERS.has(p.getName());\n});\n\n\nplus a test for a pre-rename mixed switch and map mirroring the existing post-rename ones.
| } else if (className !== 'SdkError') { | ||
| diagnostics.push( | ||
| error( | ||
| filePath, | ||
| line, | ||
| `\`${comparison.subjectText}.code === SdkErrorCode.${memberName}\` is guarded by ` + | ||
| `\`instanceof ${className}\`, but v2 raises ${memberName} on SdkError. ` + | ||
| `Manual fix: \`${comparison.subjectText} instanceof SdkError && ` + | ||
| `${comparison.subjectText}.code === SdkErrorCode.${memberName}\`.` | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 The unrecognized-class branch in errorCodeSemantics.ts emits an error-level diagnostic (which makes the CLI exit 1) for guards like e instanceof Error && e.code === SdkErrorCode.RequestTimeout, even though SdkError extends Error so that guard works correctly in v2 — the "Manual fix" message claims a problem that doesn't exist. Consider treating Error (and SdkError subclasses like SdkHttpError) as acceptable guard classes, or downgrading this specific case to a warning so a correct migration isn't reported as a failure.
Extended reasoning...
What happens. errorCodeSemantics.ts lines 77–88: when a comparison against a moved member (SdkErrorCode.RequestTimeout / ConnectionClosed) is guarded by an instanceof check whose class is not in {ProtocolError, McpError, SdkError}, the transform emits a DiagnosticLevel.Error diagnostic: \X.code === SdkErrorCode.RequestTimeout` is guarded by `instanceof `, but v2 raises RequestTimeout on SdkError. Manual fix: .... The CLI treats any error-level diagnostic as a failed run — cli.ts:90-97filtersDiagnosticLevel.Errorand setsprocess.exitCode = 1.\n\n**Why this is a false positive for instanceof Error.** SdkError extends Error (packages/core/src/errors/sdkErrors.ts:59), and SdkHttpError extends SdkError(line 95). So a defensive v1 guard likee instanceof Error && e.code === ErrorCode.RequestTimeoutmigrates (after the symbols rename) toe instanceof Error && e.code === SdkErrorCode.RequestTimeout, which is fully correct v2 runtime behavior: every SdkErrorinstance passesinstanceof Error, and .codecarries theSdkErrorCodestring. Yet the codemod tells the user this guard is broken and needs a manual fix, and the run exits 1.\n\n**Step-by-step proof.**\n1. Input v1 file (a.jsfile or loosely-typed catch block — the codemod processes.jsper theprocesses .js filesintegration test):\n ```js\n import { ErrorCode } from '@modelcontextprotocol/sdk/types.js';\n try { await call(); } catch (e) {\n if (e instanceof Error && e.code === ErrorCode.RequestTimeout) { retry(); }\n }\n ```\n2.symbolsrewrites the member toSdkErrorCode.RequestTimeout; the guard stays e instanceof Error.\n3. error-code-semanticsfinds the moved-member comparison,getComparisonContextreturns subjecte, findInstanceofGuardmatchese instanceof Errorin the same&&chain.\n4.Erroris not inV1_ERROR_CLASSESand!== 'SdkError', so the else ifbranch at line 80 fires: error-level diagnostic with the "Manual fix" text.\n5.cli.ts:90-97sees an error-level diagnostic →process.exitCode = 1. The migration was actually correct; nothing needed fixing.\n\n**On the intentional-design objection.** One reviewer pass argued this is the documented conservative policy ("instanceof guard against an unrecognized class → error with the exact manual fix") and therefore not a bug. That policy makes sense for genuinely unknown classes (e.g. CustomError), where the guard might silently never match — and the existing test for that case is fine. But the policy's purpose is to catch never-matching guards, and instanceof Errorcannot never-match: it is a strict superclass ofSdkError. For this case the diagnostic text is factually wrong (it asserts the check is broken when it isn't), it's emitted at error level rather than as a review note, and it fails the run. The trigger surface is admittedly narrow — in strictly-typed TS the comparison is usually spelled (e as any).code, whose subject text doesn't match the guard's left operand and falls into the warning path instead — so this mainly bites JS files and any-typed subjects, which is why this is a nit rather than a blocker.\n\n**How to fix.** Either add 'Error'(and arguably'SdkHttpError', a subclass of SdkErrorthat also passes the comparison when the error is an HTTP SdkError) to the set of acceptable guard classes that take the no-diagnostic path, or keep the message but emit it as awarning(...)when the guard class isError`/an SdkError subclass so the run isn't marked failed. No code is mutated by this branch, so the change is purely about diagnostic level/wording.
1 participant
Three codemod fixes for backwards-compatibility breaks that surface when migrating a real v1 codebase: a new
error-code-semanticstransform for the relocatedRequestTimeout/ConnectionClosederror codes, theStreamableHTTPErrorrewrite corrected toSdkHttpError, and preservation of file-leading shebangs/banner comments during import rewrites.Motivation and Context
All three were observed while migrating a large production MCP host application (client + server + custom transports) from SDK v1 to v2 using the codemod.
1. Silent never-matching error checks (the dangerous one). In v1,
ErrorCode.RequestTimeoutandErrorCode.ConnectionClosedwere numeric members of the protocol error enum, raised onMcpError. In v2 they moved toSdkErrorCode— a string enum — raised onSdkError. The existing symbol-rename transform updates the enum reference but not the surrounding semantics, so this v1 code:migrates to:
which compiles (under loose typing it even runs) and never matches — timeout and disconnect handling silently stops working. The new
error-code-semanticstransform rewrites theinstanceofguard toSdkErrorwhere the subject is unambiguous, and emits diagnostics for the cases it cannot safely rewrite: guards against unrecognized classes,switchstatements mixingSdkErrorCodeandProtocolErrorCodecases (impossible in v2 — the enums live on different error classes), and maps keyed by the moved members (previously numeric keys, now strings).2. Wrong replacement class for
StreamableHTTPError. The codemod rewroteStreamableHTTPErrortoSdkErrorand told users "HTTP status is now inerror.data?.status". Perdocs/migration.md, the actual replacement isSdkHttpError(extendsSdkError) with typed.status/.statusTextaccessors. Migrated code ended up one subclass too general, and the emitted guidance pointed at an untyped field:3. Deleted file headers. When import consolidation, the
ErrorCodesplit, or removed-API handling removed the first import declaration in a file, the leading trivia attached to it —#!/usr/bin/env nodeshebangs and file-header banner comments — was silently deleted with it. Across a large migration this stripped dozens of documentation headers and broke directly-executable scripts. The trivia is now captured before mutation and restored afterwards.How Has This Been Tested?
error-code-semantics(guard rewrite in&&chains and nested ifs, pre- and post-rename spellings, import insertion, no-guard/foreign-guard/mixed-switch/keyed-map diagnostics, idempotence).ErrorCode-split path).StreamableHTTPErrortests updated to assert theSdkHttpErrorrewrite.try/catchclassifyingMcpError/ErrorCode.RequestTimeout/ConnectionClosed) run through the full registered transform order, asserting the composed result — guards retargeted toSdkError, header intact, no v1 imports left (including the guard class import once its last usage is rewritten). This is the ordering interaction (symbolsthenerror-code-semantics) the transform's registration comment relies on.SdkHttpErrortarget fails 7 (incl. an integration test), disabling trivia restoration fails 4.Breaking Changes
None — the codemod produces strictly more correct output. Code already migrated with the old rewrite can be re-run; the new transform is idempotent and recognizes both pre- and post-rename spellings.
Types of changes
Checklist
Additional context
The
error-code-semanticstransform runs aftersymbolsin the pipeline (it consumes the post-rename spelling) but also recognizes the pre-rename spelling so it composes when run standalone via--transforms. Where a rewrite is not safely automatable it prefers an error-level diagnostic with the exact manual fix over guessing — the failure mode it targets is silent, so the codemod should never reintroduce silence.