fix: resolve symlinked bin path before entry-point comparison - #31
fix: resolve symlinked bin path before entry-point comparison#31autojack-bot[bot] wants to merge 1 commit into
Conversation
npx/npm install this package's `mcp-pirsch` bin as a symlink to dist/index.js. When invoked through that symlink, process.argv[1] is the symlink path while import.meta.url is already resolved to the real target, so the isDirectExecution comparison always evaluated false. main() never ran and the process exited 0 with zero output — surfacing in AutoHub as "Server mcp-pirsch process exited unexpectedly (code: 0, signal: null)" on every npx-based load. Running node dist/index.js directly worked fine, which masked the bug. Resolve process.argv[1] with realpathSync before comparing against import.meta.url so symlinked bin invocations are detected correctly. Added a regression test that spawns the built entry point through a symlink (matching npm's bin shim) and asserts the server starts.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b0115c4a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // so process.argv[1] is the symlink path while import.meta.url is already resolved to | ||
| // the real target. Resolve the symlink before comparing or this always evaluates false | ||
| // under npx, main() never runs, and the process exits cleanly with no output. | ||
| return pathToFileURL(realpathSync(process.argv[1])).href === import.meta.url; |
There was a problem hiding this comment.
Accept preserved symlink mains in the entry guard
When users run the bin with NODE_OPTIONS=--preserve-symlinks-main (node --help describes this as preserving symbolic links when resolving the main module), Node keeps import.meta.url at the symlink path; this code always realpaths argv[1], so node node_modules/.bin/mcp-pirsch evaluates false again and exits 0 without starting. The guard should accept either the direct argv[1] file URL or the realpath URL so the symlink fix does not regress environments using that Node option.
Useful? React with 👍 / 👎.
| it('starts the server when invoked directly (dist/index.js exists)', () => { | ||
| expect(existsSync(distEntry)).toBe(true); |
There was a problem hiding this comment.
Avoid requiring dist before the test script builds it
In a clean checkout, dist/ is gitignored and package.json's npm test script only runs vitest run, so this new test fails before exercising the fix whenever tests are run without a preceding npm run build (the README documents npm test as the standalone test command). Either have the test setup build the entry or run the source through the TS runner so the suite is not dependent on an untracked build artifact.
Useful? React with 👍 / 👎.
Summary
Fixes the MCP server exiting cleanly with no output whenever it's launched via
npxor an installedmcp-pirschbin — reported by AutoHub asServer mcp-pirsch process exited unexpectedly (code: 0, signal: null).Root cause
npm/npxinstall this package's bin as a symlink:node_modules/.bin/mcp-pirsch -> ../@verygoodplugins/mcp-pirsch/dist/index.js. The entry-point guard insrc/index.tswas:When Node executes the file through that symlink,
process.argv[1]is the symlink path, but ESM'simport.meta.urlis already resolved to the real target path. The two never match, soisDirectExecutionwas alwaysfalseunder npx/bin invocation,main()was never called, and the process fell off the end of the module with nothing pending — a cleanexit(0)with zero output (no error, no "Pirsch MCP server running" log). Runningnode dist/index.jsdirectly worked fine (no symlink involved), which is why this went unnoticed — the credential guard'sprocess.exit(1)path was also ruled out since the observed exit code is 0, not 1.Fix
Resolve
process.argv[1]withrealpathSyncbefore comparing it toimport.meta.url(which is already realpath-resolved by Node's ESM loader):Testing
src/index.spawn.test.ts: spawns the builtdist/index.js(a) directly and (b) through a freshly created symlink (mirroring npm's bin shim), and asserts the server logsPirsch MCP server runningand stays alive in both cases. This test fails onmain(symlink case hangs with zero stderr output) and passes on this branch.npm run lint,npm run typecheck,npm run build,npm testall green locally (55/55 tests passing).dist/index.jsand invoking via the symlink — confirmed silentexit 0with no stdout/stderr before the fix, and correct startup log after.Test plan
npm run lintnpm run typechecknpm run buildnpm test(55 passed, including the new regression test)npx -y @verygoodplugins/mcp-pirsch@latestload succeeds against the published fix