From 73bfce20f666b15d3676822989f081b7707284a3 Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Fri, 3 Jul 2026 11:50:54 +0600 Subject: [PATCH] fix(cli): make post-upgrade cleanup confirmation sticky, no silent override MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #76 Pre-fix, a user who answered `y` at the all-or-nothing cleanup prompt ("What would you like to do?") saw nothing deleted if their terminal session's input stream ended before they re-answered for every candidate — the per-version loop fired unconditionally for every version in `toOffer` because `cfg.PerVersion` defaults to `true` from `cfg.Cleanup.Prompt`'s config default, the `default:` branch of `promptPerVersion` returned `false` for empty / non-`y` input, and every version landed 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: confirmation is now sticky at the level the user chose to operate at. Three classes of higher-level confirmation set `cfg.PerVersion = false` for the per-version loop: 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 from the candidate set — when `Manager.Current()` errors, we fall back to per-version y/N to avoid mass-deletion. All three `TestCleanupPrompt_ForcePerVersion*` tests confirm this preservation. `TestCleanupPrompt_PerVersionConfirm` and `TestCleanupPrompt_SpecificVersion` (the tests that pinned the buggy double-prompt behavior) are rewritten to assert the new correct behavior. `TestCleanupPrompt_DeleteAllSkipsPerVersionPrompt` is the explicit regression test for #76 — two candidates, only `y\n` supplied, both must be deleted and no "Delete v" prompt may appear in the output. CHANGELOG: ### Fixed entry under [Unreleased]. Co-Authored-By: puku-ai-2.8 --- CHANGELOG.md | 39 ++++++++++++++++ internal/cli/cleanup.go | 31 ++++++++++++- internal/cli/cleanup_test.go | 87 ++++++++++++++++++++++++++++++------ 3 files changed, 142 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f0f579d..ff6182a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -607,6 +607,45 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `/` 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 diff --git a/internal/cli/cleanup.go b/internal/cli/cleanup.go index b1dbe54..e174583 100644 --- a/internal/cli/cleanup.go +++ b/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 { diff --git a/internal/cli/cleanup_test.go b/internal/cli/cleanup_test.go index 28bca97..5bdbed9 100644 --- a/internal/cli/cleanup_test.go +++ b/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"),