fix: MCP server fails to start via npx (#58)#63
Conversation
1. isMain guard: process.argv[1] (symlink path) never matched import.meta.url (real path). Fixed with realpathSync comparison. 2. skills.json not in npm package: prebuild copies marketplace/skills.json to data/ dir, included in package files. 3. Version string bumped from 1.12.0 to 1.16.0. Closes #58
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughImplements symlink-aware entry-point detection using realpathSync and expands skills.json discovery paths; adds a prebuild step to include Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (process argv)"
participant FS as "Filesystem (realpathSync)"
participant Loader as "MCP loader"
participant Server as "SkillKit MCP Server"
CLI->>FS: realpath(process.argv[1])
FS->>CLI: resolved CLI path (rgba(100,150,250,0.5))
Loader->>FS: compute pkgDir and candidate paths
Loader->>FS: check `pkgDir/../data/skills.json`, `pkgDir/../../marketplace/skills.json`, `process.cwd()/marketplace/skills.json`
FS-->>Loader: return first existing skills.json path (rgba(150,200,100,0.5))
CLI->>FS: realpath(import.meta.url)
FS->>CLI: resolved module path (rgba(200,100,150,0.5))
CLI->>Loader: compare resolved paths -> if equal, invoke main()
Loader->>Server: start server with loaded skills and version 1.16.0
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/mcp/src/index.ts`:
- Around line 218-220: The comparison using realpathSync(process.argv[1]) can
throw when process.argv[1] is undefined or points to a non-existent path; wrap
the resolution in a try/catch and treat failures as non-equal so import as a
library does not crash. Specifically, in the block that defines resolvedArgv and
resolvedModule (using realpathSync, process.argv, fileURLToPath,
import.meta.url), catch errors from realpathSync and set resolvedArgv to a safe
fallback (e.g., undefined or an empty string) so the subsequent if (resolvedArgv
=== resolvedModule) check simply evaluates false instead of throwing; ensure you
do not change the semantics when argv is valid.
🧹 Nitpick comments (2)
packages/mcp/package.json (1)
22-22: Cross-platform compatibility:mkdir -pandcpare POSIX-only.The
prebuildscript uses shell commands that won't work natively on Windows (cmd.exe). If Windows developer support matters, consider using a cross-platform alternative likecpy-clior a small Node script. If the team is Unix-only, this is fine to leave as-is.packages/mcp/src/index.ts (1)
4-4: Note:realpathSyncis imported at the top level but also dynamically imported insideloadSkills(Line 23).Minor inconsistency —
readFileSyncis dynamically imported inloadSkillswhilerealpathSyncfrom the samenode:fsmodule is statically imported here. Not blocking, but worth being aware of.
- Wrap in try/catch so library imports don't crash - Check process.argv[1] exists before calling realpathSync - Also resolve import.meta.url with realpathSync for pnpm/hoisted setups
Summary
isMainguard usedprocess.argv[1] === fileURLToPath(import.meta.url)which fails when npx creates a.bin/symlink. Fixed withrealpathSync()to resolve both paths before comparison.marketplace/skills.jsonwasn't included in the npm package ("files": ["dist"]). Addedprebuildscript to copy it todata/and includeddatain files array.Closes #58
Test plan
pnpm --filter @skillkit/mcp buildsucceedspnpm --filter @skillkit/mcp test— 9/9 tests passnode packages/mcp/dist/index.js— starts and loads 15,065 skillsln -sf .../dist/index.js /tmp/test && node /tmp/test— starts correctlySummary by CodeRabbit