fix(mcp-server): point types metadata to emitted declarations#146
fix(mcp-server): point types metadata to emitted declarations#146itosa-kazu wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 49 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR updates the MCP server package to publish ESM-compatible type declarations via a new ChangesESM type declarations and npm CLI integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
mcp-server/src/cli-bin.integration.test.ts (2)
24-25: ⚡ Quick winNpx CLI path derivation assumes a specific npm structure.
The regex replacement
/npm-cli\.js$/assumes thatprocess.env.npm_execpathalways ends withnpm-cli.js. If npm's internal structure changes or the path format differs across platforms or npm versions, this derivation will fail silently, falling back to the systemnpxcommand without indication.🛡️ More robust derivation
-const npmCli = process.env.npm_execpath; -const npxCli = npmCli?.replace(/npm-cli\.js$/, "npx-cli.js"); +const npmCli = process.env.npm_execpath; +const npxCli = npmCli + ? path.join(path.dirname(npmCli), "npx-cli.js") + : undefined;This approach derives the npx path by replacing the filename within the same directory, which is more resilient to path variations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/src/cli-bin.integration.test.ts` around lines 24 - 25, The current npx path derivation assumes npm_execpath ends with "npm-cli.js" by using a fixed regex; update the logic to safely compute npxCli from npmCli by taking the directory portion of process.env.npm_execpath (reference: npmCli and process.env.npm_execpath) and joining it with "npx-cli.js" so filename replacement is not dependent on exact string endings and works across platforms (handle null/undefined npmCli and Windows path separators), and ensure a clear fallback/log message if the computed npxCli does not exist.
24-39: ⚡ Quick winExtract shared npm execution helpers to eliminate duplication.
The
npmCli,npxCli,execNpm, andspawnNpxdefinitions (lines 24–39) are duplicated verbatim inpackage-published-esm.integration.test.ts(lines 46–53 for the npm subset). Extract these helpers into a shared test utility module to maintain a single source of truth and simplify future updates.♻️ Suggested structure
Create
mcp-server/test/helpers/npm-exec.ts:import { execFileSync, spawnSync } from "node:child_process"; import { existsSync } from "node:fs"; import path from "node:path"; const npmCli = process.env.npm_execpath; const npxCli = npmCli?.replace(/npm-cli\.js$/, "npx-cli.js"); export function execNpm(args: string[], options: Parameters<typeof execFileSync>[2]) { if (npmCli) { return execFileSync(process.execPath, [npmCli, ...args], options); } return execFileSync("npm", args, options); } export function spawnNpx(args: string[], options: Parameters<typeof spawnSync>[2]) { if (npxCli && existsSync(npxCli)) { return spawnSync(process.execPath, [npxCli, ...args], options); } return spawnSync("npx", args, options); }Then import in both test files:
-const npmCli = process.env.npm_execpath; -const npxCli = npmCli?.replace(/npm-cli\.js$/, "npx-cli.js"); - -function execNpm(args: string[], options: Parameters<typeof execFileSync>[2]) { - if (npmCli) { - return execFileSync(process.execPath, [npmCli, ...args], options); - } - return execFileSync("npm", args, options); -} - -function spawnNpx(args: string[], options: Parameters<typeof spawnSync>[2]) { - if (npxCli && existsSync(npxCli)) { - return spawnSync(process.execPath, [npxCli, ...args], options); - } - return spawnSync("npx", args, options); -} +import { execNpm, spawnNpx } from "../test/helpers/npm-exec.js";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/src/cli-bin.integration.test.ts` around lines 24 - 39, Extract the duplicated helpers npmCli, npxCli, execNpm, and spawnNpx into a single shared test helper module (e.g., npm-exec.ts) and export execNpm and spawnNpx; then remove the duplicate declarations from mcp-server/src/cli-bin.integration.test.ts and package-published-esm.integration.test.ts and import the exported execNpm and spawnNpx from the new helper instead, preserving the exact behavior and signatures (Parameters<typeof execFileSync>[2] and Parameters<typeof spawnSync>[2]).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@mcp-server/src/cli-bin.integration.test.ts`:
- Around line 24-25: The current npx path derivation assumes npm_execpath ends
with "npm-cli.js" by using a fixed regex; update the logic to safely compute
npxCli from npmCli by taking the directory portion of process.env.npm_execpath
(reference: npmCli and process.env.npm_execpath) and joining it with
"npx-cli.js" so filename replacement is not dependent on exact string endings
and works across platforms (handle null/undefined npmCli and Windows path
separators), and ensure a clear fallback/log message if the computed npxCli does
not exist.
- Around line 24-39: Extract the duplicated helpers npmCli, npxCli, execNpm, and
spawnNpx into a single shared test helper module (e.g., npm-exec.ts) and export
execNpm and spawnNpx; then remove the duplicate declarations from
mcp-server/src/cli-bin.integration.test.ts and
package-published-esm.integration.test.ts and import the exported execNpm and
spawnNpx from the new helper instead, preserving the exact behavior and
signatures (Parameters<typeof execFileSync>[2] and Parameters<typeof
spawnSync>[2]).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65007382-f2db-4bee-ac8b-cd4fc14de924
📒 Files selected for processing (3)
mcp-server/package.jsonmcp-server/src/cli-bin.integration.test.tsmcp-server/src/package-published-esm.integration.test.ts
itosa-kazu
force-pushed
the
codex/fix-currents-mcp-types
branch
from
204015f to
6d6df1b
Compare
1 participant
Summary
typesand exportedtypesto the emitteddist/api.d.mtsValidation
npm ci --ignore-scriptsnpx tsdownnpx vitest run src/package-published-esm.integration.test.tsnpx tsdown;npx vitest run(12 files / 385 tests)npm pack --ignore-scriptsplus clean temp install metadata smokeNote:
npm run buildrunstsdownsuccessfully here, then fails on Windows at the existing POSIXchmod 755 dist/index.mjsstep.Summary by CodeRabbit
Chores
Tests