fix(cli): fail closed when Manager.Current() can't detect active Node#87
Merged
Conversation
The post-upgrade cleanup step silently swallowed `Current()`
errors and proceeded with the zero-value `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: 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. Combined with --cleanup / --yes /
cfg.Cleanup.Auto, that path would auto-delete the Node.js
version the user is actively running.
Fail closed instead:
- New `cleanupConfig.ForcePerVersion` flag. When set,
`runCleanupPrompt` overrides AutoDeleteAll=false and
PerVersion=true at step 1b (after NonInteractive wins, before
any prompt fires), so per-version y/N is forced regardless of
how the user configured cleanup auto-confirm.
- `runUpgrade` tracks `currentErr` from `m.Current()` and
sets `cleanupCfg.ForcePerVersion = true` when it fails.
A warning ("could not determine the currently-active Node
version (...); cleanup will require per-version confirmation
for safety") is printed so the user knows why a --cleanup /
--yes run suddenly turned per-version.
- `cleanupCandidates` (the active-version exclusion logic) is
intentionally unchanged — the safety net is at the prompt
layer, not the candidate-set layer, because failing the whole
upgrade would be too aggressive and the per-version y/N is
the correct UX when we can't see what's active.
- New tests 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 (NonInteractive wins over ForcePerVersion).
docs/managers.md updated: the candidates section now describes
the fail-closed behavior, and the --yes row notes it's downgraded
to per-version y/N when Current() fails.
Refs #58
|
✔️ 561dac0 - 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
1 participant
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
If
Manager.Current()failed during the post-upgrade cleanup step (e.g.,nvmafternvm deactivate, or any transientnode -pparse hiccup), the version currently powering the user's shell became an ordinary, deletable cleanup candidate. Combined with--cleanup/--yes/cfg.Cleanup.Auto, that path would auto-delete the active Node.js version.This PR fails closed: when
Current()errors, the cleanup step forces per-version y/N confirmation (downgrading every auto-confirm path), and surfaces a warning explaining why the prompt is firing.Linked issues
Fixes #58
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 (tidy/fmt/vet/test all green;make lintfails locally with the documented build(lint): .golangci.yml v1 schema incompatible with golangci-lint v2 (brew installs v2, make lint fails to even parse config) #62 golangci v1/v2 schema issue — CI uses the pinned v1.64.8 which is unaffected)docs/, inline godoc)Screenshots / output
Pre-fix behavior on a transient
Current()failure with--cleanup:Post-fix behavior on the same
Current()failure:--no-cleanupstill wins (the strongest fail-closed stance):Files touched
internal/cli/cleanup.go— newForcePerVersion boolfield oncleanupConfig.runCleanupPromptapplies it at step 1b (right after theNonInteractiveshort-circuit, before any prompt fires), settingAutoDeleteAll = falseandPerVersion = true. Doc comment carries the rationale and references bug(cli): Current() failure leaves the active Node version unprotected from cleanup deletion #58.internal/cli/upgrade.go— trackscurrentErrfromm.Current(). On error, setscleanupCfg.ForcePerVersion = trueand prints a stderr-style warning (cmd.Printf) explaining the downgrade.internal/cli/cleanup_test.go— three new test functions pinning the fail-closed contract:TestCleanupPrompt_ForcePerVersionDowngradesAutoDeleteAll—AutoDeleteAll=true + ForcePerVersion=truerequires per-version y/N (input: all-or-nothing "y" + per-version "y" + per-version "n" → 1 deleted, 1 skipped). A regression that ran withAutoDeleteAll=truewould auto-delete both, never reading the per-version answers.TestCleanupPrompt_ForcePerVersionIgnoresPerVersionFalse—PerVersion=false + ForcePerVersion=truestill requires per-version y/N, even though the user explicitly setPrompt: falsein their config.TestCleanupPrompt_ForcePerVersionWithNonInteractive—NonInteractive=true + ForcePerVersion=truestill skips entirely (theNonInteractiveshort-circuit fires first). A regression that flipped the order of the two checks would print prompts even when--no-cleanupwas passed.docs/managers.md— the candidates section now describes the fail-closed behavior (per-version y/N whenCurrent()fails), and the--yesrow notes it's downgraded to per-version y/N in that case.CHANGELOG.md—### Fixedentry under[Unreleased]documenting the change with the bug(cli): Current() failure leaves the active Node version unprotected from cleanup deletion #58 reference.