Add npm installation for macOS and Linux - #30
Open
hsliuustc0106 wants to merge 5 commits into
Open
hsliuustc0106 wants to merge 5 commits into
hsliuustc0106 wants to merge 5 commits into
Conversation
Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com>
Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com>
Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com>
hsliuustc0106
commented
Oct 2, 2026
hsliuustc0106
left a comment
Contributor
Author
There was a problem hiding this comment.
Review: high quality, locally verified — approve-with-nits
I ran all three npm test suites locally on Linux, plus a trial merge against current main:
npm test— 8/8 launcher tests passnpm run test:package— pack,--ignore-scriptsglobal install,npx, payload/version checks passnpm run test:bootstrap— real uv + Python 3.12 download, cached restart, GitHub TLS check passnpm view nanodot→ 404: the package name is free on the registry, no squatting risk
Verified as solid (no action needed)
- Import-hijack protection: probes use
python -I; the CLI runs withPYTHONSAFEPATH=1plus an explicitsys.path.insert(0, bundled-src), so neither cwd nor a caller'sPYTHONPATHcan shadow the bundlednanodotpackage. The decoynanodot.pyin package-smoke exercises exactly this. - No shell injection surface: everything goes through
spawnwithout a shell;owner/repo#1; echo unsafepasses through literally (tested). - Supply-chain hygiene on the download path: uv pinned at 0.12.21 via the versioned official URL,
curl -qdisables.curlrc,--failcatches HTTP errors, install into a temp dir with an atomic rename publish; the concurrent-first-runEEXISTrace is handled and tested. - System cleanliness:
UV_UNMANAGED_INSTALL+UV_NO_MODIFY_PATH=1+python install --no-bin— no shell profile edits, no global Python shims (the fake installer asserts both env vars). - CLI semantics pass through: argv, stdin (secret entry), cwd, cooperative SIGINT/SIGTERM, exit codes incl. signal→128+n; second run has clean stderr.
- Version sync is gated across
package.json/pyproject.toml/__init__.pyby package-smoke. filesglob is complete:srcon both this branch and currentmain(27 files post-#28) contains only.py, nothing to miss.node >=22is the oldest maintained LTS line, a fair floor.
Required before merge
- README.md conflicts with
main. The PR is based on82eb79a;mainsince merged #28 and expanded the README. I verified in a scratch worktree that README.md is the only conflict — resolve by folding the npm install section into the current README structure rather than taking the PR version wholesale. Mergingmainin will also activate the conditional runner check in package-smoke (see below).
Suggestions
- CI double-runs: the workflow triggers on both
pushandpull_request, so same-repo PRs run every push twice (visible as two check sets). Suggestpush: branches: [main]. - No LICENSE file: both manifests declare MIT but the repo has no LICENSE file, so
npm packships without one. Worth adding before publishing an npm package. - Hard
curldependency: bootstrap requirescurlon PATH. Node 22 shipsfetch— downloading the installer with it would drop the extra requirement on minimal Linux images. Also, whencurlis missing the current error ("retry setup with internet access") is misleading for what is reallyspawn curl ENOENT.
Nits
stdio: ['inherit', check ? 2 : 'inherit', 'inherit']— both branches are effectively identical (fd 2 direct vs inherited); simplifiable.UV_CACHE_DIRretains the ~50MB download archive indefinitely;uv python install --no-cachewould avoid it.- SIGHUP isn't forwarded, though the child shares the launcher's process group so tty-generated HUP reaches it; only a targeted non-tty HUP to the launcher PID is affected.
Note on coverage
The runner start/stop section of package-smoke is conditional on runner_control.py, which only enters the package after merging main (#28). It didn't execute on this branch. The PR description covers it via the MVP snapshot validation, but please merge main into this branch before merging so that block actually runs in CI.
hsliuustc0106
marked this pull request as ready for review
October 2, 2026 09:53
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Users currently need a Python checkout and a manually managed environment to run nanodot. Add an npm package for macOS and Linux, exposing the existing CLI through
npm install -g nanodotornpx nanodotafter the npm release.The package bundles the Python source from the same checkout and requires Node.js 22+. The launcher reuses Python 3.11+ or prepares a private Python 3.12 runtime on first use with the official uv 0.12.21 installer. Runtime setup preserves shell profiles; npm installation works with
--ignore-scripts. Arguments, stdin, working directory, signals, and CLI exit status pass through, including the bundled source path for the background runner.Validation:
--ignore-scripts,npxexecution, payload checks, and version checks passed.661f4bapassed all 515 tests; its installed npm package also completed a real anonymous watch on already-merged PR Implement #14: end-to-end PR-watch validation suite, all fakes #27 with one persistent inbox entry and no duplicate, then started/stopped the background runner. No runner remains active.Integration: based on fetched main
82eb79a. The full MVP CLI release depends on #28. npm publication is a separate maintainer release step; this PR includes packaging/release instructions.Final CI on
bc6481f: PR npm checks, push npm checks, and Python CI all passed.