fix(cli): make post-upgrade cleanup confirmation sticky, no silent override

CHANGELOG.md

@@ -607,6 +607,45 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   `<goos>/<goarch>` platform pattern, and `--check` flag is a
   no-op not an error) so a future refactor can't silently drop
   a field. Closes #68.
+- `internal/cli/cleanup.go`: confirmation at the post-upgrade
+  cleanup's all-or-nothing prompt is now sticky — answering
+  `y` (delete all) or a specific version at the "What would you
+  like to do?" prompt no longer triggers a second, separate
+  per-version `Delete vX? [y/N]` loop on top of the explicit
+  confirmation. Pre-fix, the per-version loop fired unconditionally
+  for every candidate in `toOffer`, because `cfg.PerVersion`
+  defaults to `true` from `cfg.Cleanup.Prompt`'s config default —
+  so a user who answered `y` once at the all-or-nothing prompt
+  saw nothing deleted if their terminal session's input stream
+  ended before they re-answered for every candidate (the
+  `default:` branch of `promptPerVersion` returns `false`, and
+  empty / non-`y` input landed each version in `result.Skipped`
+  with no visible error). User reported live: answered `y` at
+  the cleanup prompt during a real `nodeup upgrade`, then
+  `fnm list` afterward still showed the old versions — no
+  deletion occurred, with no surface indication of why.
+  Fix: set `cfg.PerVersion = false` for the per-version loop
+  once a higher-level confirmation has been recorded. Three
+  classes of higher-level confirmation trigger this:
+  1. **`decision.deleteAll`** — `y` / `yes` at the all-or-nothing
+     prompt.
+  2. **`decision.deleteOne`** — a specific version typed at the
+     all-or-nothing prompt (the user's choice IS the per-version
+     confirmation).
+  3. **`AutoDeleteAll` (without `ForcePerVersion`)** — set by
+     `--cleanup`, `--yes`, or `cfg.Cleanup.Auto`. The user's
+     pre-flight opt-in counts; the per-version prompt is redundant.
+  The `ForcePerVersion` downgrade from #58 is the only path that
+  keeps `cfg.PerVersion = true` after Step 1b, because it
+  represents an inability to safely exclude the active version
+  — see `TestCleanupPrompt_ForcePerVersionDowngradesAutoDeleteAll`,
+  `TestCleanupPrompt_ForcePerVersionIgnoresPerVersionFalse`, and
+  `TestCleanupPrompt_ForcePerVersionWithNonInteractive` for the
+  preservation. `TestCleanupPrompt_PerVersionConfirm` and
+  `TestCleanupPrompt_SpecificVersion` (the tests that pinned
+  the buggy double-prompt behavior) are updated to assert the
+  new correct behavior; `TestCleanupPrompt_DeleteAllSkipsPerVersionPrompt`
+  is the explicit regression test for #76. Closes #76.
 
 ## [0.0.0] - 2024-07-01
 

internal/cli/cleanup.go

@@ -231,8 +231,36 @@ func runCleanupPrompt(cfg cleanupConfig, toInstall, installed []semver.Version,
 	}
 
 	// Step 4: Per-version loop.
+	//
+	// Per-version confirmation is "sticky-up" only — once the user
+	// has explicitly opted into deletion at a higher level (the
+	// all-or-nothing prompt's `deleteAll` or `deleteOne`, or a
+	// pre-flagged `--cleanup` / `--yes` / `--cleanup-version` /
+	// `cfg.Cleanup.Auto`), we MUST NOT re-prompt per version. The
+	// old behavior (a second `Delete vX? [y/N]` for each candidate,
+	// where empty / non-y input silently skipped the deletion) was
+	// a real regression: users who answered `y` once at the
+	// all-or-nothing prompt would see nothing deleted if their
+	// terminal session's input stream ended before they re-answered
+	// for every candidate. The fix is to set `cfg.PerVersion = false`
+	// for the loop once a higher-level confirmation has been
+	// recorded. The ForcePerVersion downgrade (Step 1b) is the only
+	// path that keeps per-version on, because it represents an
+	// inability to safely exclude the active version — see #58.
+	if !cfg.ForcePerVersion {
+		// Higher-level confirmation → no per-version prompt.
+		// ForcePerVersion explicitly overrides this in Step 1b
+		// by setting `cfg.PerVersion = true`, so by the time we
+		// reach here, the only configs that still have
+		// PerVersion=true are (a) a default-true cfg.Cleanup.Prompt
+		// without any higher-level confirmation (e.g. an empty
+		// cfg.Prefiltered + a user who answered `n`/`skip` — but
+		// that case returns before reaching Step 4) or (b) the
+		// ForcePerVersion downgrade. Path (b) is the one we want
+		// to preserve; path (a) can't reach Step 4.
+		cfg.PerVersion = false
+	}
 	for _, v := range toOffer {
-		confirm := true
 		if cfg.PerVersion {
 			answer, err := promptPerVersion(v, streams)
 			if err != nil {
@@ -243,7 +271,6 @@ func runCleanupPrompt(cfg cleanupConfig, toInstall, installed []semver.Version,
 				result.Skipped = append(result.Skipped, v)
 				continue
 			}
-			_ = confirm
 		}
 
 		if err := m.Uninstall(v); err != nil {

internal/cli/cleanup_test.go

@@ -179,10 +179,16 @@ func TestCleanupPrompt_YesDeletesAll(t *testing.T) {
 }
 
 func TestCleanupPrompt_PerVersionConfirm(t *testing.T) {
-	// With PerVersion=true, "y" to all-or-nothing still requires a
-	// per-version "y" to actually delete. User says y, y, n — only
-	// the first should be deleted.
-	streams, _ := newCleanupIO("y\ny\nn\n")
+	// Once the user has explicitly opted into mass-delete at the
+	// all-or-nothing prompt ("y"), that confirmation is sticky: no
+	// per-version re-prompt, no chance for a non-`y` default to
+	// silently override the explicit `y` from the previous step.
+	// This is the regression test for #76 — the pre-fix behavior
+	// was: `y` at all-or-nothing, then `Delete v18.20.4? [y/N]`,
+	// then `Delete v20.18.0? [y/N]`, with empty/non-y input silently
+	// skipping each version. The user reported live: they answered
+	// `y` once, then `fnm list` showed nothing was deleted.
+	streams, _ := newCleanupIO("y\n")
 	mgr := &stubManager{name: "fnm"}
 	cfg := cleanupConfig{PerVersion: true}
 
@@ -194,17 +200,69 @@ func TestCleanupPrompt_PerVersionConfirm(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	if len(result.Deleted) != 1 {
-		t.Errorf("expected 1 deleted, got %v", result.Deleted)
+	if len(result.Deleted) != 2 {
+		t.Errorf("expected 2 deleted (y at all-or-nothing is sticky), got %v", result.Deleted)
 	}
-	if len(result.Skipped) != 1 {
-		t.Errorf("expected 1 skipped, got %v", result.Skipped)
+	if len(result.Skipped) != 0 {
+		t.Errorf("expected 0 skipped, got %v", result.Skipped)
 	}
-	if result.Deleted[0].String() != "18.20.4" {
-		t.Errorf("expected 18.20.4 deleted, got %s", result.Deleted[0])
+	if len(mgr.uninstalls) != 2 {
+		t.Errorf("expected 2 Uninstall calls, got %v", mgr.uninstalls)
 	}
-	if result.Skipped[0].String() != "20.18.0" {
-		t.Errorf("expected 20.18.0 skipped, got %s", result.Skipped[0])
+}
+
+// TestCleanupPrompt_DeleteAllSkipsPerVersionPrompt is the explicit
+// regression test for issue #76. It pins that when the user
+// answers `y` at the all-or-nothing prompt, NO per-version
+// `Delete vX? [y/N]` prompt appears afterward — and crucially,
+// that a short input stream (e.g. just `y\n` with no further
+// answers) still results in every candidate being deleted.
+//
+// The pre-fix behavior was: `y\n` at the all-or-nothing prompt
+// set `decision.deleteAll = true` and `toOffer = candidates`, but
+// the per-version loop unconditionally fired `promptPerVersion`
+// for each candidate because `cfg.PerVersion` defaulted to true
+// from `cfg.Cleanup.Prompt`. Empty / non-`y` input to those
+// per-version prompts fell into the `default:` branch of
+// `promptPerVersion` and each version landed in `result.Skipped`
+// — so a user who answered `y` once saw nothing deleted, with no
+// visible error. The fix sets `cfg.PerVersion = false` for the
+// per-version loop once a higher-level confirmation has been
+// recorded (all-or-nothing `deleteAll` / `deleteOne`, `--cleanup`,
+// `--cleanup-version`, `--yes`, or `cfg.Cleanup.Auto`). The
+// only path that keeps `cfg.PerVersion = true` is the
+// ForcePerVersion downgrade (#58), which is set when
+// `Manager.Current()` errors — see TestCleanupPrompt_ForcePerVersion*
+// for that.
+func TestCleanupPrompt_DeleteAllSkipsPerVersionPrompt(t *testing.T) {
+	// Two candidates. Only `y\n` is supplied — no per-version
+	// answers. Pre-fix this would result in 0 deletes (every
+	// candidate skipped due to the `default:` fallback in
+	// `promptPerVersion`); post-fix it must result in 2 deletes.
+	streams, out := newCleanupIO("y\n")
+	mgr := &stubManager{name: "fnm"}
+	cfg := cleanupConfig{PerVersion: true} // would normally re-prompt
+
+	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) != 2 {
+		t.Fatalf("deleteAll must skip per-version prompt: expected 2 deleted, got %d (%v)", len(result.Deleted), result.Deleted)
+	}
+	if len(result.Skipped) != 0 {
+		t.Errorf("deleteAll must skip per-version prompt: expected 0 skipped, got %d (%v)", len(result.Skipped), result.Skipped)
+	}
+	// The output must NOT contain a per-version `Delete v` prompt
+	// — that's the visible signal that we re-prompted. Only the
+	// all-or-nothing prompt + the per-version "Deleted" lines
+	// should be present.
+	if strings.Contains(out.String(), "Delete v") {
+		t.Errorf("deleteAll must skip per-version prompt; output contains a 'Delete v' prompt:\n%s", out.String())
 	}
 }
 
@@ -228,9 +286,12 @@ func TestCleanupPrompt_NoSkips(t *testing.T) {
 
 func TestCleanupPrompt_SpecificVersion(t *testing.T) {
 	// User picks "20.18.0" by typing it. We should delete only that one.
+	// Note: with PerVersion=true (the default), the explicit
+	// specific-version pick is itself the per-version confirmation
+	// — no further `Delete v20.18.0? [y/N]` re-prompt. See #76.
 	streams, _ := newCleanupIO("20.18.0\n")
 	mgr := &stubManager{name: "fnm"}
-	cfg := cleanupConfig{PerVersion: false} // no per-version confirm
+	cfg := cleanupConfig{PerVersion: true} // explicit pick is sticky
 
 	candidates := []semver.Version{
 		mustVer(t, "18.20.4"),