fix(cli): use pathToFileURL for portable main-module guard - #67
fix(cli): use pathToFileURL for portable main-module guard#67advancedresearcharray wants to merge 1 commit into
Conversation
|
Ready for re-review — this is the follow-up to the portable main-module guard requested on #46. Changes
Verification (re-run on current head)
Test coverage for @theDakshJaitly — whenever you have a moment. |
theDakshJaitly
left a comment
There was a problem hiding this comment.
Thanks — the pathToFileURL switch is the right call for Windows/unicode paths. But the guard regresses the common case: it compares import.meta.url (which Node resolves through symlinks) against an unresolved process.argv[1], so it evaluates false for any symlinked bin — global npm i -g, npx, node_modules/.bin — and even for direct runs under a symlinked dir (macOS /var -> /private/var). When it's false, program.parse() never runs and the CLI silently no-ops. CI misses it because the tests invoke node dist/cli.js from the real checkout path.
Verified locally on Node 20: the current guard is false for npx/global-bin invocation; resolving the symlink fixes all cases (and is a no-op on Windows, where npm uses .cmd shims):
import { realpathSync } from "node:fs";
import { pathToFileURL } from "node:url";
// ...
if (process.argv[1] && import.meta.url === pathToFileURL(realpathSync(process.argv[1])).href) {
program.parse();
}Could you also add a test that runs the built CLI through a symlink (mirroring how npm installs the bin) so this path is covered going forward? Happy to merge once that's in.
|
Addressed the symlink regression from the review. Changes
Verification
|
|
Re-review requested — symlink regression fix is on head (21855cb). What changed since the review
Verification (re-run on 21855cb)
@theDakshJaitly — ready when you are. |
realpathSync(process.argv[1]) so npm global/npx/node_modules/.bin symlinks match import.meta.url. Add a symlinked-bin integration test and guard against missing argv paths in test imports. Co-authored-by: Cursor <cursoragent@cursor.com>
21855cb to
6df8998
Compare
Gate program.parse() on import.meta.url === pathToFileURL(realpathSync(process.argv[1])).href so the CLI runs correctly when invoked through a symlinked bin (npm global, npx, node_modules/.bin), with a try/catch when argv[1] is absent. Adds an integration test that invokes the built CLI through a symlink. Supersedes #67.
Addresses the remaining review feedback on #46.
Context
CLI-level test coverage for
mex log/mex timelineoption parsing already merged via #47. PR #46's review requested a portable main-module guard; this PR lands it with symlink-safe resolution for npm global / npx /node_modules/.binlayouts.Change
realpathSyncfromnode:fsandpathToFileURLfromnode:urlprogram.parse()onimport.meta.url === pathToFileURL(realpathSync(process.argv[1])).href(try/catch whenargv[1]is absent, e.g. test imports)--version) and import-without-auto-parsepatterns/cli-option-parsing-tests.mdto document the guardVerification
npx vitest run test/cli.test.ts— 13/13 passingnpm test— 177/177 passingnpm run typecheck— passingCloses the actionable items from @theDakshJaitly's review on #46. #46 itself can be closed as superseded by #47 + this follow-up.