fix(mcp-server): point types metadata to emitted declarations#146
Conversation
|
Warning Review limit reached
More reviews will be available in 49 minutes and 10 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 switches published typings to 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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@mcp-server/test/npm-exec.ts`:
- Around line 12-15: The current branch uses npmCli when truthy without
verifying the path actually exists, so a non-existent npm_execpath will throw
before falling back to "npm"; modify the logic around the npmCli usage in
mcp-server/test/npm-exec.ts (the block that calls execFileSync(process.execPath,
[npmCli, ...args], options)) to first verify the file exists (e.g.,
fs.existsSync or fs.access) similar to spawnNpx’s check, and only call
execFileSync with process.execPath + npmCli if the file check passes; otherwise
fall back to execFileSync("npm", args, options).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a90f8ac6-6dd0-4984-a188-ac23700d6107
⛔ Files ignored due to path filters (1)
mcp-server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
mcp-server/package.jsonmcp-server/src/cli-bin.integration.test.tsmcp-server/src/package-published-esm.integration.test.tsmcp-server/test/npm-exec.ts
✅ Files skipped from review due to trivial changes (1)
- mcp-server/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- mcp-server/src/cli-bin.integration.test.ts
- mcp-server/src/package-published-esm.integration.test.ts
Fall back to plain npm when npm_execpath is set but the file is missing, matching spawnNpx behavior.
cursor
Bot
force-pushed
the
codex/fix-currents-mcp-types
branch
from
e57a432 to
26e0294
Compare
3 participants
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