fix(detector): replace n.Current()'s silent-download call site#88
Merged
Conversation
Pre-fix, N.Current() ran `n current` — an undocumented subcommand in upstream tj/n (bin/n). `n current` is not a first-class case arm in the dispatch block; it falls through to the default `*` arm which calls `install "$1"`. Inside `install`, the string "current" matches the version-label branch in display_remote_versions (same branch as "latest"), and the function downloads the latest Node.js tarball, extracts it, and calls activate. So every Current() invocation would silently side-effect-install the newest available Node version behind the user's back — every run, on every n-using machine. Callers treat Current() errors as "active version unknown, don't exclude it" (the safe-by-convention default per the Manager interface doc), so this also silently defeated the cleanup safety net on every n install. Replacement: shell out to `$N_PREFIX/bin/node --version` and parse the output. n's `activate` function copies the active node binary into $N_PREFIX/bin/node, so that binary's --version IS the active version. Side-effect-free, matches every supported n install, and the parser handles both vX.Y.Z and bare X.Y.Z output shapes (the latter is defensive against n forks). Tests: - TestParseNNodeVersion_* (4): parser correctness for v-prefixed, bare, whitespace-padded, empty, and unparseable input. The pre-fix parser's name was parseNCurrent — the rename is the most visible signal that the contract changed. - TestN_Current_InvokesNodeVersionNotNCurrent: pins the new behavior at the integration level. The literal "n current" request is the ONE THING the fixture asserts MUST NOT appear; any regression that re-introduces the bug fails immediately with a t.Fatalf citing the request. - TestN_Current_PropagatesRunShellError: missing node binary or exec failure surfaces as an error so callers treat it as "active unknown". - TestN_Current_PropagatesParseError: blank --version output errors rather than returning the zero semver. The pre-fix tests (TestParseNCurrent_*, TestNCurrent_InvokesShell) are deleted — they codified the buggy behavior. Refs #59
|
✔️ 01c528a - Conventional commits check succeeded. |
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
internal/detector/n.go'sN.Current()shells out ton current— an undocumented subcommand in upstreamtj/n(bin/n). The stringcurrentis not a first-class case arm in the script's dispatch block; it falls through to the default*)arminstall "$1"; exit ;;. Insideinstall, "current" matches the version-label branch indisplay_remote_versions(same as "latest"), which downloads the latest Node.js tarball, extracts it, and activates it. So everyCurrent()invocation would silently side-effect-install the newest available Node version on the user's machine — every run, every n-using machine.Callers treat
Current()errors as "active version unknown, don't exclude it" (the safe-by-convention default per theManagerinterface doc). That meant the cleanup safety net never fired on anyninstall: the active version was always "unknown," and--cleanup/--yes/cfg.Cleanup.Autocould happily auto-delete whatever was actually powering the user's shell.This PR replaces the call site with
<N_PREFIX>/bin/node --version. n'sactivatefunction (bin/n) copies the active node binary into that path, so its--versionIS the active version — side-effect-free, and matches every supported n install. The pre-fix tests (TestParseNCurrent_*,TestNCurrent_InvokesShell) codified the buggy behavior; they're deleted.Linked issues
Fixes #59
Type of change
fix— bug fix (PATCH bump)feat— new feature (MINOR bump)refactor— no behavior changeperf— performance improvementdocs— documentation onlytest— tests onlychore— maintenance / dependency bumpci— CI/CD onlybuild— build system onlyChecklist
feat(scope): subject)make cilocally and it passes (tidy/fmt/vet/test all green;make lintfails locally with the documented build(lint): .golangci.yml v1 schema incompatible with golangci-lint v2 (brew installs v2, make lint fails to even parse config) #62 golangci v1/v2 schema issue — CI uses the pinned v1.64.8 which is unaffected)docs/, inline godoc)Screenshots / output
Pre-fix behavior on an
n-using machine, everynodeupinvocation:Post-fix behavior:
Files touched
internal/detector/n.go—N.Current()rewritten to shell out to<N_PREFIX>/bin/node --versionvia the existingnPrefix()helper. The pre-fixrunShell(ctx, "n", "current")is gone.parseNCurrent(the pre-fixn currentparser) is renamed toparseNNodeVersionand its doc comment is rewritten to explain<node --version>'s output format and whyn currentis not used.Currentstrategy) is updated to spell out the reasoning and link fix(detector): n manager's Current() relies on unverified 'n current' subcommand #59.internal/detector/n_test.go—TestParseNCurrent_Bare,TestParseNCurrent_WithPrefix,TestParseNCurrent_Empty,TestNCurrent_InvokesShell(which all pinnedn currentinvocation / parsing): deleted.TestParseNNodeVersion_VPrefixedReal,TestParseNNodeVersion_Bare,TestParseNNodeVersion_LeadingTrailingWhitespace,TestParseNNodeVersion_Empty,TestParseNNodeVersion_Unparseable— parser coverage for both common shapes (vX.Y.Z) and defensive shapes (bareX.Y.Z, whitespace, garbage).TestN_Current_InvokesNodeVersionNotNCurrent— integration test that fails the test if the literaln currentrequest re-appears in any future regression. The fixture'st.Fatalfcites the request so the diagnosis is immediate.TestN_Current_PropagatesRunShellError,TestN_Current_PropagatesParseError— error-path coverage."strings".CHANGELOG.md—### Fixedentry under[Unreleased]documenting the change with the fix(detector): n manager's Current() relies on unverified 'n current' subcommand #59 reference.