fix(cli): make post-upgrade cleanup confirmation sticky, no silent override#95
Merged
Merged
Conversation
…erride 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 <noreply@puku.sh>
|
✔️ 73bfce2 - 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
The post-upgrade cleanup step's "delete all" confirmation is no longer silently overridden by an unprompted per-version re-confirmation. Pre-fix, a user who answered
yat the all-or-nothing prompt 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 intoOffer(becausecfg.PerVersiondefaults totruefromcfg.Cleanup.Prompt's config default), thedefault:branch ofpromptPerVersionreturnedfalsefor empty / non-yinput, and every version landed inresult.Skippedwith no visible error.User-reported live: answered
yat the cleanup prompt during a realnodeup upgrade, thenfnm listafterward still showed the old versions — no deletion occurred, with no surface indication of why.This PR makes confirmation "sticky" at the level the user chose to operate at:
decision.deleteAll—y/yesat the all-or-nothing prompt.decision.deleteOne— a specific version typed at the all-or-nothing prompt (the user's choice IS the per-version confirmation).AutoDeleteAll(withoutForcePerVersion) — set by--cleanup,--yes, orcfg.Cleanup.Auto. The user's pre-flight opt-in counts; the per-version prompt is redundant.Fix: in
runCleanupPromptStep 4, setcfg.PerVersion = falsefor the per-version loop once a higher-level confirmation has been recorded. TheForcePerVersiondowngrade from #58 is the only path that keepscfg.PerVersion = trueafter Step 1b, because it represents an inability to safely exclude the active version from the candidate set — whenManager.Current()errors, we can't be sure which version is powering the user's shell, so we fall back to per-version y/N to avoid mass-deletion. All threeTestCleanupPrompt_ForcePerVersion*tests confirm this preservation.Linked issues
Closes #76
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 (Go-side tidy/fmt/vet/lint/test all green)TestCleanupPrompt_DeleteAllSkipsPerVersionPromptis the explicit regression test for bug(cli): cleanup double-prompts — confirming 'delete all' still requires a second per-version 'y', silently skips if missed #76;TestCleanupPrompt_PerVersionConfirmandTestCleanupPrompt_SpecificVersionupdated to assert the new correct behavior;TestCleanupPrompt_AutoDeleteAllandTestCleanupPrompt_PrefilteredOnlycontinue to pass unchanged because their input streams already provided extray\ns)runCleanupPromptStep 4 explains the "sticky-up" semantics and theForcePerVersionexception)make lintgreen)Screenshots / output
Pre-fix:
Post-fix:
--cleanup(no per-version prompt, even withcleanup.prompt: truein config):--cleanup-version 18.20.0(no all-or-nothing prompt, no per-version prompt):Why this isn't breaking
The new behavior is strictly more permissive: any user who set
cfg.Cleanup.Prompt: falsealready had per-version prompts off (no change). Users withcleanup.prompt: truewho want to delete all will now succeed with oneyinstead of needingyonce per candidate. Users who want to keep the per-version safety net can keep using--no/nat the all-or-nothing prompt to skip cleanup entirely, or--cleanup-version <v>to pick specific versions. The only path that still asks for per-version confirmation is theForcePerVersiondowngrade from #58, which is set whenManager.Current()errors — a real safety mechanism that protects against accidentally deleting the version that's currently powering the user's shell.Files touched
internal/cli/cleanup.go—runCleanupPromptadds a single block before the per-version loop:if !cfg.ForcePerVersion { cfg.PerVersion = false }. This makes the higher-level confirmation sticky. ForcePerVersion (set byupgrade.gowhenManager.Current()errors) explicitly overrides this in Step 1b by settingcfg.PerVersion = true, so the ForcePerVersion safety net from bug(cli): Current() failure leaves the active Node version unprotected from cleanup deletion #58 still works.confirmvariable from the previous Step 4 (it was assigned but never read).ForcePerVersionexception.internal/cli/cleanup_test.go—TestCleanupPrompt_PerVersionConfirm: rewritten. Was:cfg.PerVersion: true+ "y\ny\nn\n" → 1 deleted, 1 skipped (pinned the bug). Now: same config + "y\n" → 2 deleted (the new correct behavior). Comment explains why.TestCleanupPrompt_SpecificVersion: rewritten. Was:cfg.PerVersion: false. Now:cfg.PerVersion: true+ "20.18.0\n" → 1 deleted, the explicit pick is its own confirmation. The test now also pins the "specific-version pick is sticky" behavior.TestCleanupPrompt_DeleteAllSkipsPerVersionPrompt: new. The explicit regression test for bug(cli): cleanup double-prompts — confirming 'delete all' still requires a second per-version 'y', silently skips if missed #76 — two candidates, onlyy\nsupplied, both must be deleted and no "Delete v" prompt may appear in the output.TestCleanupPrompt_AutoDeleteAll,TestCleanupPrompt_PrefilteredOnly,TestCleanupPrompt_NoSkips, allTestCleanupPrompt_ForcePerVersion*: unchanged and still green. They prove that the fix doesn't accidentally re-prompt under--cleanup,--cleanup-version,n, orForcePerVersion.CHANGELOG.md—### Fixedentry under[Unreleased]documenting the sticky-confirmation semantics + the three higher-level confirmation classes + the ForcePerVersion exception. References bug(cli): cleanup double-prompts — confirming 'delete all' still requires a second per-version 'y', silently skips if missed #76.