feat(detector): warn on system Node and sandboxed installations#30
Merged
dipto0321 merged 3 commits intoJul 1, 2026
Merged
Conversation
Add a path-based classifier that identifies how the `node` binary on PATH was installed: OS-package, snap, flatpak, homebrew-core, or manager-managed. `nodeup upgrade` prints a warning to stderr when the binary is one nodeup will not (and cannot) replace, with a platform-specific hint for what to use instead. `nodeup check` surfaces the same classification in both JSON and table output so the user can diagnose the situation before running an upgrade. The classifier is path-based and pure (no subprocesses except a single `which node` / `where node` probe), with three package-level seams (`whichNode`, `getenv`, `userHomeDir`) that tests stub. Manager detection wins over path patterns: if the resolved binary lives inside a known manager's data directory, it is classified as `manager` regardless of the on-disk layout. Refs: #27
There was a problem hiding this comment.
Pull request overview
This PR adds a “system Node” detector that classifies the node binary found on PATH (OS package manager vs snap/flatpak/Homebrew vs manager-managed vs unknown) and surfaces user-facing warnings so nodeup doesn’t silently treat non-upgradeable installs as upgradeable (closes #27).
Changes:
- Introduces a path-based
ResolveSystemNodeclassifier andWarnSystemNoderenderer ininternal/detector/, plus test seams and a comprehensive test suite. - Wires warnings into
nodeup upgrade(stderr warning before mutations) and extendsnodeup checkoutput/JSON to include system-node classification. - Updates docs and changelog to describe the new classifier behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/detector/system_node.go | Adds system-node classification + warning text generation. |
| internal/detector/system_node_helpers.go | Adds test seams for env/home-dir access used by the classifier. |
| internal/detector/system_node_test.go | Adds tests for classification, manager-root attribution, warnings, and seams. |
| internal/cli/upgrade.go | Emits a system-node warning to stderr prior to upgrade actions. |
| internal/cli/check.go | Adds system-node classification to table and --json output. |
| docs/managers.md | Documents how nodeup behaves when node on PATH isn’t manager-owned. |
| CHANGELOG.md | Records the new system-node classifier feature and wiring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+54
to
+57
| // String returns a human-readable label for the kind, suitable for | ||
| // the `nodeup upgrade` and `nodeup check` output. Empty string | ||
| // for SystemNodeUnknown is deliberate — callers should switch on | ||
| // the value rather than stringify it. |
Comment on lines
+105
to
+127
| var whichNode = func(ctx context.Context) (string, error) { | ||
| // Windows `where` and unix `which` have different output shapes; | ||
| // we want "the first line" from either. Both exit 0 when found | ||
| // and non-zero when not. | ||
| var name string | ||
| var args []string | ||
| if runtime.GOOS == "windows" { | ||
| name = "where" | ||
| args = []string{"node"} | ||
| } else { | ||
| name = "which" | ||
| args = []string{"node"} | ||
| } | ||
| out, err := exec.CommandContext(ctx, name, args...).Output() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| line := strings.TrimSpace(strings.SplitN(string(out), "\n", 2)[0]) | ||
| if line == "" { | ||
| return "", fmt.Errorf("%w: empty output from %s", ErrNoNodeOnPATH, name) | ||
| } | ||
| return line, nil | ||
| } |
Comment on lines
+214
to
+218
| case strings.HasPrefix(s, "/usr/local/bin/") && looksLikeHomebrewCoreLayout(s): | ||
| // `/usr/local/bin/node` is a wrapper that ultimately points | ||
| // at /usr/local/Cellar/node/<v>/bin/node. Some Homebrew | ||
| // installs put the wrapper directly inside /usr/local/bin | ||
| // without the symlink chain showing the Cellar prefix; the |
Comment on lines
+303
to
+305
| // The actual detection: if the path contains "/.nvm/versions/node/" | ||
| // anywhere, it's an nvm install regardless of where NVM_DIR points. | ||
| // Same for fnm, volta, asdf, mise, n, nodenv. |
Comment on lines
+320
to
+321
| cleanParent := filepath.Clean(parent) | ||
| rel, err := filepath.Rel(cleanParent, child) |
Comment on lines
+360
to
+363
| // We keep this conservative: an empty slice with ok=true means | ||
| // "this manager exists, but we don't know its root — fall through | ||
| // to path-based classification". An ok=false result is reserved | ||
| // for "manager is nil, please don't try to attribute". |
Comment on lines
+77
to
+89
| // Probe `node` on PATH and classify how it's installed. We pass | ||
| // nil for the manager here — `check` doesn't pick a manager | ||
| // (that's `upgrade`'s job) and the classifier still works from | ||
| // path patterns alone. The warning text is captured for both | ||
| // the JSON envelope and the table renderer. | ||
| var sysNode *systemNodeJSON | ||
| if info, err := detector.ResolveSystemNode(cmd.Context(), nil); err == nil { | ||
| sysNode = &systemNodeJSON{ | ||
| Path: info.Path, | ||
| Kind: info.Kind.String(), | ||
| Manager: info.Manager, | ||
| } | ||
| } |
| | `os-package` | `/usr/bin/node`, `/bin/node`, `/opt/node/...`, `C:\Program Files\nodejs\node.exe`, `~/scoop/apps/nodejs/...` | Prints a warning to stderr (upgrade) or table (check). The platform-specific hint names the right upgrade tool: `sudo apt upgrade nodejs`, `winget upgrade Node.js`, etc. | | ||
| | `snap` | `/snap/bin/node`, `/snap/node/<rev>/bin/node` | Warns. Run `snap refresh node`. | | ||
| | `flatpak` | `/var/lib/flatpak/runtime/node/...`, `/usr/libexec/flatpak/...` | Warns. Run `flatpak update` (or uninstall the flatpak and let nodeup manage a manager install instead). | | ||
| | `homebrew-core` | `/usr/local/bin/node`, `/opt/homebrew/bin/node`, `~/homebrew/Cellar/node/...`, `/home/linuxbrew/.linuxbrew/bin/node` | Warns. Run `brew upgrade node`, or `brew uninstall node` and let nodeup take over. | |
Comment on lines
+613
to
+614
| // Look up `go` (always present in any Go test environment) and | ||
| // assert that whichNode returns a non-empty absolute path. |
Comment on lines
+456
to
+465
| // Returns ("", false) when no warning is warranted: | ||
| // | ||
| // - info.Kind == SystemNodeManaged: this is exactly what nodeup | ||
| // upgrades; no warning needed. | ||
| // - info.Kind == SystemNodeUnknown and info.Path == "": nothing | ||
| // detected; that's a different problem (no node at all) and is | ||
| // handled elsewhere (ErrNoNodeOnPATH). | ||
| // - info.Kind == SystemNodeUnknown and info.Path != "": path | ||
| // didn't match any known layout; we emit a soft warning so the | ||
| // user can decide. |
- whichNode now uses exec.LookPath instead of shelling out to which/where, removing a dependency on those tools being present on minimal images. - Homebrew wrapper branch is restricted to darwin; on Linux, /usr/local/bin/node is overwhelmingly a manual make install and must classify as OS-package. - isInside cleans both child and parent paths so paths with intermediate "."/".." segments (e.g. /a/b/c/..) classify as inside /a. - WarnSystemNode/managerManagedRoots/String doc comments now match actual behavior (SystemNodeUnknown is "unknown", not empty; ok=false covers unknown managers too). - looksLikeNVMInstall comment no longer claims to detect fnm/ volta/asdf/mise — only nvm. - runCheck now passes the detected manager to ResolveSystemNode when exactly one is installed so a normal fnm/nvm/volta node on PATH classifies as "manager" rather than "unrecognized". - docs/managers.md Homebrew Cellar example uses the canonical /usr/local/Cellar and /opt/homebrew/Cellar paths. - TestWhichNode renamed and updated to reflect LookPath semantics. Co-Authored-By: Sonnet 4.6 <noreply@puku.sh>
|
✔️ 366b412...6fe3975 - Conventional commits check succeeded. |
dipto0321
deleted the
fix/platform/warn-on-system-node-and-sandboxed-installations
branch
15 tasks
dipto0321
added a commit
that referenced
this pull request
Jul 1, 2026
…ep (#33) The CHANGELOG was missing Phase 6 bullets for PRs #28 (QuotePath for paths with spaces), #29 (interrupted-upgrade sentinel + --from restore), and #30 (system-node classifier with warnings). It also had a duplicate `### Added` header from a prior merge and still listed the scripts/issue-workflow.sh script that PR #32 deleted. The README's Project status table said "Phase 1-5 OK" with no mention of Phase 6 cross-platform polish. The Phase 7 status (GoReleaser config + brew/scoop taps + npm wrapper) was also missing. CHANGELOG.md: - Single Added / Changed / Removed block under [Unreleased] (was two Added blocks). - Add Phase 6 bullets: system-node classifier (#30), interrupted- upgrade sentinel + replay (#29), QuotePath for paths with spaces (#28). - Drop the stale `scripts/issue-workflow.sh` bullet; add a Removed entry pointing at the squash-merged PR #32 commit for context. README.md: - Project status table marks Phase 6 complete, with PR refs and a note that Phase 7 (issue #17) is the remaining work blocking the v1.0.0 tag. - Trailing paragraph updated to reflect Phase 6 in the phase order. Refs #18 Co-authored-by: dipto0321 <dipto@local>
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.
Summary
Linked issues
Closes #27
Type of change
feat— see the title; check the matching box below- [ ]
fix- [ ]
docs- [ ]
style- [ ]
refactor- [ ]
perf- [ ]
test- [ ]
build- [ ]
ci- [ ]
chore- [ ]
revertChecklist
feat(detector): subject)make cilocally and it passesdocs/, inline godoc)Scope notes / things reviewers may want to look at
Screenshots / output