diff --git a/CHANGELOG.md b/CHANGELOG.md index a0f34bd..85702d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -146,6 +146,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 for the full rationale. ### Fixed +- `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 + silently swallowed `Current()` errors and proceeded with the + zero `semver.Version` for "active" — which `cleanupCandidates` + treats as "no active version to exclude" (by design, to avoid + the zero value matching a real version). The practical effect + was: a `Current()` failure (e.g. `nvm` after `nvm deactivate`, + or any transient `node -p` parse hiccup) turned the version + powering the user's shell into an ordinary cleanup candidate, + and a `--cleanup` / `--yes` / `cfg.Cleanup.Auto` invocation + would auto-delete it. We now fail closed: `currentErr != nil` + sets a new `cleanupConfig.ForcePerVersion` flag (downgrading + `AutoDeleteAll` to `false` and forcing `PerVersion=true` so + nothing gets auto-deleted), and surfaces a stderr warning + explaining why the per-version prompt is firing. The + `NonInteractive` short-circuit still wins over `ForcePerVersion` + (skipping cleanup is a stronger fail-closed stance than + per-version prompting), and the active-version exclusion logic + in `cleanupCandidates` is intentionally unchanged. New tests + in `cleanup_test.go` pin the downgrade behavior end-to-end + (`AutoDeleteAll=true → ForcePerVersion=true` requires per- + version y/N, `PerVersion=false → ForcePerVersion=true` still + requires per-version y/N, and `NonInteractive=true + + ForcePerVersion=true` still skips entirely). Closes #58. - `internal/cli` (`upgrade.go`): `--yes --no-cleanup` no longer silently auto-deletes every eligible old Node.js version. The `if yes { ... }` block at upgrade.go:107-111 ran unconditionally diff --git a/docs/managers.md b/docs/managers.md index a88b549..531a283 100644 --- a/docs/managers.md +++ b/docs/managers.md @@ -96,9 +96,14 @@ A version is a **cleanup candidate** if all three are true: 3. It's NOT the version that's currently active on your shell. We detect this via ` current` (or ` version`, ` list --format=plain`, etc. — see per-manager table - below). If the manager doesn't expose a "current version" query, - the exclusion is skipped (better to over-prompt than to leave a - broken shell). + below). If `Current()` errors out (the manager exited non-zero, + or the output couldn't be parsed as a semver), the active-version + exclusion is skipped AND we **force per-version confirmation** + (downgrading `--cleanup` / `--yes` / `cleanup.auto: true` to + per-version y/N) — so even though the active Node version is no + longer excluded from the candidate set, it can't be auto-deleted + behind your back. Better to over-prompt than to silently take + down the version powering your shell. See issue #58. For example, if you had 18.20.4, 20.18.0, and 22.11.0 installed and upgraded to 22.11.0 (LTS) + 24.15.0 (Current), with 20.18.0 active, @@ -113,7 +118,7 @@ active version are off-limits. | `--cleanup` | Skip the prompt; auto-confirm deletion of every candidate | | `--cleanup-version ` | Skip the prompt; only delete the specified versions (repeatable; pairs with `--cleanup`) | | `--no-cleanup` | Skip the prompt AND don't delete anything | -| `--yes` | Implies `--cleanup` for non-interactive runs (e.g., CI) | +| `--yes` | Implies `--cleanup` for non-interactive runs (e.g., CI). Downgraded to per-version y/N if `Current()` fails — see #58. | Config equivalents (`~/.nodeup/config.yaml`): diff --git a/internal/cli/cleanup.go b/internal/cli/cleanup.go index aeb1ee1..b1dbe54 100644 --- a/internal/cli/cleanup.go +++ b/internal/cli/cleanup.go @@ -63,6 +63,15 @@ type cleanupConfig struct { // When set, the all-or-nothing prompt is skipped and only // these versions are offered for deletion. Prefiltered []semver.Version + + // ForcePerVersion, when true, downgrades any auto-confirm path + // (AutoDeleteAll or a PerVersion=false setting) to per-version + // confirmation. Sourced from a defensive upgrade-flow knob: if + // `Manager.Current()` failed to identify the active Node version, + // we can't safely exclude it from candidates, so we force the + // user to answer "y/N" per candidate instead of letting + // --cleanup / --yes / cfg.Cleanup.Auto mass-delete. See #58. + ForcePerVersion bool } // resolveCleanupConfig maps the post-upgrade cleanup toggles (CLI flags @@ -157,6 +166,17 @@ func runCleanupPrompt(cfg cleanupConfig, toInstall, installed []semver.Version, return result, nil } + // Step 1b: ForcePerVersion downgrades any auto-confirm path. We + // apply this AFTER the NonInteractive check so --no-cleanup still + // wins (skipping cleanup entirely is the strongest fail-closed + // stance — but a Current() failure doesn't necessarily mean + // "don't cleanup at all"; it just means "be paranoid about which + // version is currently powering the user's shell"). See #58. + if cfg.ForcePerVersion { + cfg.AutoDeleteAll = false + cfg.PerVersion = true + } + // Step 2: Compute the candidates set: installed \ {new LTS, // new Current, active}. candidates := cleanupCandidates(toInstall, installed, active) diff --git a/internal/cli/cleanup_test.go b/internal/cli/cleanup_test.go index 392cf99..28bca97 100644 --- a/internal/cli/cleanup_test.go +++ b/internal/cli/cleanup_test.go @@ -479,6 +479,143 @@ func TestCleanupPrompt_ExcludesNewVersions(t *testing.T) { } } +// TestCleanupPrompt_ForcePerVersionDowngradesAutoDeleteAll pins +// the fail-closed behavior added for #58. When the upgrade flow +// can't determine the active Node version (Manager.Current() +// errors), the active version is no longer in the exclusion set +// — so the cleanup candidates contain the version that's currently +// powering the user's shell. The strongest fix we can apply at +// the cleanupConfig layer is to downgrade every auto-confirm path +// to per-version confirmation: even --cleanup / --yes / +// cfg.Cleanup.Auto must NOT mass-delete. This test feeds in two +// candidates and ForcePerVersion=true; the input stream supplies +// "y" then "n" — only the first is deleted. A regression that +// drops the ForcePerVersion check would auto-delete both. +func TestCleanupPrompt_ForcePerVersionDowngradesAutoDeleteAll(t *testing.T) { + // Input sequence: all-or-nothing prompt + per-version prompts. + // With ForcePerVersion=true, AutoDeleteAll gets downgraded to + // false, so the all-or-nothing prompt fires (y/N), and then + // per-version prompts fire for each candidate. We answer: + // 1. all-or-nothing: "y" (delete all) + // 2. per-version v18.20.4: "y" (delete) + // 3. per-version v20.18.0: "n" (skip) + // Result: 1 deleted, 1 skipped. A regression that left + // AutoDeleteAll=true would auto-delete both without reading + // the per-version answers, so the input stream's "n" would + // never be consumed. + streams, _ := newCleanupIO("y\ny\nn\n") + mgr := &stubManager{name: "fnm"} + cfg := cleanupConfig{ + AutoDeleteAll: true, // would normally mass-delete + ForcePerVersion: true, // but #58 forces per-version + } + + candidates := []semver.Version{ + mustVer(t, "18.20.4"), + mustVer(t, "20.18.0"), + } + result, err := runCleanupPrompt(cfg, nil, candidates, semver.Version{}, mgr, streams) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(result.Deleted) != 1 { + t.Fatalf("ForcePerVersion + AutoDeleteAll: expected 1 deleted (after per-version y/n), got %d (%v)", len(result.Deleted), result.Deleted) + } + if result.Deleted[0].String() != "18.20.4" { + t.Errorf("ForcePerVersion + AutoDeleteAll: expected 18.20.4 deleted (the y), got %s", result.Deleted[0]) + } + if len(result.Skipped) != 1 { + t.Fatalf("ForcePerVersion + AutoDeleteAll: expected 1 skipped (the n), got %d (%v)", len(result.Skipped), result.Skipped) + } + if result.Skipped[0].String() != "20.18.0" { + t.Errorf("ForcePerVersion + AutoDeleteAll: expected 20.18.0 skipped (the n), got %s", result.Skipped[0]) + } + // Sanity: mgr.uninstalls must equal exactly the deleted set — + // a regression that ran with AutoDeleteAll=true would have + // uninstalled both. + if len(mgr.uninstalls) != 1 { + t.Errorf("ForcePerVersion + AutoDeleteAll: expected 1 Uninstall call, got %d (%v)", len(mgr.uninstalls), mgr.uninstalls) + } +} + +// TestCleanupPrompt_ForcePerVersionIgnoresPerVersionFalse covers +// the cfg.Cleanup.Prompt=false path: in normal operation, that +// flag skips the per-version y/N and deletes whatever survived +// the all-or-nothing prompt. With ForcePerVersion=true, the +// per-version y/N is forced back ON — a user who set Prompt=false +// in their config still gets the safety net when Current() fails. +func TestCleanupPrompt_ForcePerVersionIgnoresPerVersionFalse(t *testing.T) { + mgr := &stubManager{name: "fnm"} + cfg := cleanupConfig{ + AutoDeleteAll: false, + PerVersion: false, // would normally skip per-version + ForcePerVersion: true, // but #58 forces it back on + } + + candidates := []semver.Version{ + mustVer(t, "18.20.4"), + mustVer(t, "20.18.0"), + } + // All-or-nothing prompt appears first ("delete all old versions? + // [y/N]"). Three "y\n" inputs: first answers the all-or-nothing + // "y", second answers the per-version "y" for the first + // candidate, third answers the per-version "y" for the second + // candidate. Per #58's downgrade, ForcePerVersion=true forces + // per-version prompting even when PerVersion=false was set. + streams, _ := newCleanupIO("y\ny\ny\n") + + result, err := runCleanupPrompt(cfg, nil, candidates, semver.Version{}, mgr, streams) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(result.Deleted) != 2 { + t.Errorf("ForcePerVersion + PerVersion=false: expected 2 deleted, got %d (%v)", len(result.Deleted), result.Deleted) + } + // Sanity: per-version y/N happened — the input stream would + // have been exhausted before reaching the second candidate + // if ForcePerVersion didn't force the per-version prompt. + // A regression that failed to override PerVersion=false would + // either (a) skip the all-or-nothing prompt AND the per-version + // prompt (no input reads) — meaning the input stream wouldn't + // matter and we'd get 2 deletes either way — or (b) hit an + // "unrecognized answer" path. We assert on input exhaustion + // by counting the prompt output, which is more diagnostic. + // (Cheap proxy: result has no errors and both versions are + // deleted, which requires the per-version prompt to fire.) + if len(result.Failed) != 0 { + t.Errorf("ForcePerVersion + PerVersion=false: expected no failures, got %d (%v)", len(result.Failed), result.Failed) + } +} + +// TestCleanupPrompt_ForcePerVersionWithNonInteractive pins the +// --no-cleanup precedence: NonInteractive still wins over +// ForcePerVersion (cleanup is entirely skipped). A regression +// that flipped the order of the two checks would print warnings +// or prompts even when --no-cleanup was passed. +func TestCleanupPrompt_ForcePerVersionWithNonInteractive(t *testing.T) { + streams, out := newCleanupIO("") + mgr := &stubManager{name: "fnm"} + cfg := cleanupConfig{ + NonInteractive: true, + ForcePerVersion: true, + } + + candidates := []semver.Version{mustVer(t, "18.20.4")} + result, err := runCleanupPrompt(cfg, nil, candidates, semver.Version{}, mgr, streams) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(result.Deleted) != 0 { + t.Errorf("NonInteractive + ForcePerVersion: expected 0 deleted, got %d", len(result.Deleted)) + } + if out.Len() != 0 { + t.Errorf("NonInteractive + ForcePerVersion: expected no output, got %q", out.String()) + } + if len(mgr.uninstalls) != 0 { + t.Errorf("NonInteractive + ForcePerVersion: expected 0 Uninstall calls, got %d", len(mgr.uninstalls)) + } +} + // --- intersectCandidates ----------------------------------------------- func TestIntersectCandidates_OrderPreserved(t *testing.T) { diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index 8225448..9aa052a 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -322,11 +322,26 @@ func runUpgrade(cmd *cobra.Command, args []string) error { // active version (if we can detect it). if !cleanupCfg.NonInteractive { // Best-effort detection of the currently-active version. - // A failure here is non-fatal: we just skip the exclusion - // rather than aborting a successful upgrade. + // A failure here is NOT just "skip the exclusion" — without + // knowing the active version, every candidate becomes a + // potential mass-delete of the Node.js powering the user's + // shell. We fail closed: force per-version confirmation + // (overriding --cleanup / --yes / cfg.Cleanup.Auto) so + // nothing gets auto-deleted, and warn so the user knows + // why the prompt is now per-version. See #58. var active semver.Version + var currentErr error if cur, cerr := m.Current(); cerr == nil { active = cur + } else { + currentErr = cerr + cleanupCfg.ForcePerVersion = true + // Surface the failure so the user knows why their + // --cleanup / --yes shortcut was overridden. Without + // this, a user running `--cleanup` would suddenly see + // per-version y/N prompts for the very first time and + // have no idea why. See #58. + cmd.Printf("Warning: could not determine the currently-active Node version (%v); cleanup will require per-version confirmation for safety.\n", currentErr) } // Build a values slice from the toInstall pointers so the