fix(detector): replace n.Current()'s silent-download call site

CHANGELOG.md

@@ -146,6 +146,37 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   for the full rationale.
 
 ### Fixed
+- `internal/detector/n.go`: `N.Current()` no longer shells out
+  to `n current` — an undocumented subcommand that upstream
+  `tj/n` resolves as a label equivalent to "latest" and
+  side-effects a download of the newest available Node.js
+  version. Pre-fix, every `Current()` invocation would have
+  silently mutated the user's machine by running the
+  catch-all arm `install "$1"` for "current", which fetches
+  the latest semver from nodejs.org, downloads the tarball,
+  extracts it, and activates it. Since callers treat
+  `Current()` errors as "active version unknown, don't
+  exclude it" (the safe-by-convention default per the
+  `Manager` interface doc), every n-using machine ran the
+  cleanup step without active-version exclusion on every
+  invocation — and would now also have the latest version
+  installed behind the user's back. The replacement reads
+  the active version off `$N_PREFIX/bin/node --version`
+  instead: n's `activate` function copies the active
+  node binary into that path, so its `--version` is the
+  authoritative source for "what's active". Side-effect-free,
+  matches every supported n install, and the parser
+  (`parseNNodeVersion`) handles both `vX.Y.Z` and bare
+  `X.Y.Z` shapes. Doc comments at the top of `n.go` and
+  the mutation-methods block both spell out the "do not
+  use `n current`" reasoning. New tests in `n_test.go` pin
+  the new path (`TestN_Current_InvokesNodeVersionNotNCurrent`
+  explicitly fails if the literal `n current` request re-
+  appears), parser cases (`TestParseNNodeVersion_*`), and
+  error propagation (`TestN_Current_PropagatesRunShellError`,
+  `TestN_Current_PropagatesParseError`). The pre-fix tests
+  (`TestParseNCurrent_*`, `TestNCurrent_InvokesShell`) are
+  deleted — they codified the buggy behavior. Closes #59.
 - `internal/cli` (`upgrade.go`): `Manager.Current()` failure no
   longer leaves the active Node.js version unprotected from
   cleanup deletion. Pre-fix, the post-upgrade cleanup step

internal/detector/n.go

@@ -238,7 +238,8 @@ func parseNInstalled(stdout string) ([]semver.Version, error) {
 // --- Mutation methods ----------------------------------------------------
 //
 // Install, Uninstall, Use, SetDefault, GlobalNpmPrefix, and Current
-// for n all shell out to the `n` binary through runShell.
+// for n all shell out to either the `n` binary or to the active
+// node binary through runShell.
 //
 // Important n specifics:
 //   - Install / Use take a bare `<v>`: `n install <v>`, `n <v>`.
@@ -250,9 +251,13 @@ func parseNInstalled(stdout string) ([]semver.Version, error) {
 //   - GlobalNpmPrefix follows n's on-disk layout: each version is
 //     in $N_PREFIX/n/versions/node/<v>/ with the global prefix
 //     at .../lib/node_modules.
-//   - Current is derived from `n current` (added in n 8+); older
-//     n versions return a non-zero exit, which we propagate as an
-//     error.
+//   - Current is derived from `<N_PREFIX>/bin/node --version` —
+//     n's `activate` copies the active node binary into that
+//     path, so its `--version` reports the active version. We
+//     deliberately do NOT use `n current`: that string falls
+//     through to upstream's default dispatch arm which calls
+//     `install`, side-effecting a download of the latest
+//     version. See #59.
 
 // Install runs `n install <v>`. n auto-uses the newly-installed
 // version, so there's no separate "default" step.
@@ -338,29 +343,60 @@ func nPrefix() string {
 }
 
 // Current returns the version n currently has active for the user.
-// Source: `n current` (added in n 8+). The output is a bare semver
-// like "22.11.0". On older n versions that don't have `n current`,
-// the call returns a non-zero exit code and we propagate that error.
+//
+// Strategy: invoke `<N_PREFIX>/bin/node --version` and parse the
+// output. n's `activate` function (bin/n) copies the active
+// version's `bin/node` into `$N_PREFIX/bin/node`, so the active
+// node binary's `--version` IS the active version — without
+// requiring a separate "what's active?" subcommand.
+//
+// Why not `n current`? That subcommand doesn't exist as a
+// first-class dispatch arm in upstream `tj/n` (bin/n's case
+// statement). `n current` falls through to the default `*)`
+// arm which calls `install "$1"`, which resolves "current" as
+// a label equivalent to "latest" and **downloads and installs
+// the newest available Node.js version**. So calling
+// `N.Current()` would have silently mutated the user's machine
+// on every invocation — defeating the cleanup safety net on
+// every n install, every run. See #59.
+//
+// On platforms where `$N_PREFIX` is empty (the helper returned
+// ""), or `$N_PREFIX/bin/node` is missing, or the subprocess
+// fails, we return an error — the caller treats that as
+// "active version unknown, don't exclude it" (the safe-by-
+// convention default per the Manager interface doc).
 func (n *N) Current() (semver.Version, error) {
-	res, err := runShell(context.Background(), "n", "current")
+	prefix := nPrefix()
+	if prefix == "" {
+		return semver.Version{}, errors.New("n: cannot resolve N_PREFIX or /usr/local")
+	}
+	nodeBin := filepath.Join(prefix, "bin", "node")
+	// We don't pre-Stat nodeBin here — let the shell-out error
+	// surface the "not installed" case in a single round-trip
+	// (the runShell error message will already name the missing
+	// path), rather than paying for a stat that we'd race
+	// against a concurrent uninstall.
+	res, err := runShell(context.Background(), nodeBin, "--version")
 	if err != nil {
-		return semver.Version{}, fmt.Errorf("n current: %w", err)
+		return semver.Version{}, fmt.Errorf("n current: %s --version: %w", nodeBin, err)
 	}
-	return parseNCurrent(res.Stdout)
+	return parseNNodeVersion(res.Stdout)
 }
 
-// parseNCurrent extracts the active version from `n current` output.
-// Exposed (lowercase) for direct unit testing.
+// parseNNodeVersion extracts the active version from
+// `<N_PREFIX>/bin/node --version` output. Exposed (lowercase)
+// for direct unit testing.
 //
-// Real observed output (n 10.2.0):
+// Real observed output (node 22.11.0):
 //
-//	22.11.0
+//	v22.11.0
 //
 // We take the first non-empty line, trim whitespace, strip an
-// optional "v" prefix, and feed the remainder to semver.NewVersion.
-// Older n versions that don't support `current` exit non-zero
-// before stdout — that error is propagated by runShell.
-func parseNCurrent(stdout string) (semver.Version, error) {
+// optional "v" prefix, and feed the remainder to
+// semver.NewVersion. node(1) has been emitting the
+// `vX.Y.Z` form since its earliest public releases, so this is
+// stable across every n-supported Node version.
+func parseNNodeVersion(stdout string) (semver.Version, error) {
 	for _, raw := range strings.Split(stdout, "\n") {
 		line := strings.TrimSpace(raw)
 		if line == "" {
@@ -372,5 +408,5 @@ func parseNCurrent(stdout string) (semver.Version, error) {
 		}
 		return *v, nil
 	}
-	return semver.Version{}, errors.New("n current returned empty output")
+	return semver.Version{}, errors.New("n current: <node --version> returned empty output")
 }

internal/detector/n_test.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"strings"
 	"testing"
 
 	"github.com/Masterminds/semver/v3"
@@ -530,10 +531,30 @@ func TestN_Uninstall_PropagatesError(t *testing.T) {
 	}
 }
 
-// --- parseNCurrent ----------------------------------------------------
+// --- parseNNodeVersion (the new Current() parser) ---------------------
+//
+// These tests cover the parser that reads `<N_PREFIX>/bin/node
+// --version` output. The pre-fix code parsed `n current` output,
+// which no longer exists in the upstream-detector flow — that
+// subcommand was undocumented and side-effecting. See #59.
+
+func TestParseNNodeVersion_VPrefixedReal(t *testing.T) {
+	// Real observed output of `node --version`:
+	//   $ node --version
+	//   v22.11.0
+	v, err := parseNNodeVersion("v22.11.0\n")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if v.String() != "22.11.0" {
+		t.Errorf("got %q, want %q", v.String(), "22.11.0")
+	}
+}
 
-func TestParseNCurrent_Bare(t *testing.T) {
-	v, err := parseNCurrent("22.11.0\n")
+func TestParseNNodeVersion_Bare(t *testing.T) {
+	// Defensive: some node builds / forks may omit the "v"
+	// prefix. The parser must still produce a valid semver.
+	v, err := parseNNodeVersion("22.11.0\n")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -542,8 +563,8 @@ func TestParseNCurrent_Bare(t *testing.T) {
 	}
 }
 
-func TestParseNCurrent_WithPrefix(t *testing.T) {
-	v, err := parseNCurrent("v22.11.0\n")
+func TestParseNNodeVersion_LeadingTrailingWhitespace(t *testing.T) {
+	v, err := parseNNodeVersion("   v22.11.0   \n")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -552,23 +573,45 @@ func TestParseNCurrent_WithPrefix(t *testing.T) {
 	}
 }
 
-func TestParseNCurrent_Empty(t *testing.T) {
-	_, err := parseNCurrent("")
+func TestParseNNodeVersion_Empty(t *testing.T) {
+	_, err := parseNNodeVersion("")
 	if err == nil {
 		t.Error("expected error on empty input")
 	}
 }
 
-func TestNCurrent_InvokesShell(t *testing.T) {
+func TestParseNNodeVersion_Unparseable(t *testing.T) {
+	// Anything other than a bare semver (with or without "v")
+	// must error, not silently return zero value.
+	_, err := parseNNodeVersion("not-a-version\n")
+	if err == nil {
+		t.Error("expected error on unparseable input, got nil")
+	}
+}
+
+// --- N.Current() tests ------------------------------------------------
+//
+// These tests pin that Current() (a) does NOT call `n current`
+// — the silent-download foot-gun — and (b) does call the active
+// node binary at $N_PREFIX/bin/node --version. The fixture stubs
+// `runShell` so we can assert the EXACT shell-out string without
+// needing a real n install in CI.
+
+func TestN_Current_InvokesNodeVersionNotNCurrent(t *testing.T) {
+	// The bug: pre-fix code called `n current`, which falls
+	// through to upstream's default dispatch arm and downloads
+	// the latest Node. A regression that re-introduced that
+	// would be caught HERE: the literal `n current` request is
+	// the one thing this test asserts MUST NOT appear.
 	var captured string
 	withStubShell(t,
 		nil,
 		func(req string) (*platform.RunResult, error) {
 			captured = req
-			if req != "n current" {
-				t.Errorf("unexpected runShell call: %q", req)
+			if req == "n current" {
+				t.Fatalf("Current() must not invoke `n current` — it side-effects a download; got %q", req)
 			}
-			return &platform.RunResult{Stdout: "22.11.0\n"}, nil
+			return &platform.RunResult{Stdout: "v22.11.0\n"}, nil
 		},
 	)
 
@@ -580,6 +623,44 @@ func TestNCurrent_InvokesShell(t *testing.T) {
 		t.Errorf("got %q, want %q", got.String(), "22.11.0")
 	}
 	if captured == "" {
-		t.Error("expected n current to be invoked")
+		t.Error("expected a runShell call (the new path resolves <N_PREFIX>/bin/node --version)")
+	}
+	// Sanity: the captured request should end in `--version`
+	// (we don't pin the absolute path because N_PREFIX is
+	// platform-dependent — /usr/local on Linux/macOS, whatever
+	// the test env sets, etc.).
+	if !strings.HasSuffix(captured, "--version") {
+		t.Errorf("captured request = %q, want one that ends in `--version`", captured)
+	}
+}
+
+func TestN_Current_PropagatesRunShellError(t *testing.T) {
+	// If <N_PREFIX>/bin/node is missing or fails to exec, we
+	// must propagate the error so callers treat Current() as
+	// "active version unknown" — the safe default.
+	wantErr := errors.New("simulated missing node binary")
+	withStubShell(t, nil, func(req string) (*platform.RunResult, error) {
+		return nil, wantErr
+	})
+
+	_, err := NewN().Current()
+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+	if !errors.Is(err, wantErr) {
+		t.Errorf("error %v should wrap %v", err, wantErr)
+	}
+}
+
+func TestN_Current_PropagatesParseError(t *testing.T) {
+	// The shell-out succeeded but the body is unparseable. We
+	// must return an error, not the zero semver.Version.
+	withStubShell(t, nil, func(req string) (*platform.RunResult, error) {
+		return &platform.RunResult{Stdout: ""}, nil
+	})
+
+	_, err := NewN().Current()
+	if err == nil {
+		t.Error("expected parsing error from blank --version output, got nil")
 	}
 }