Describe the bug
Small cleanup items found while reviewing the post-upgrade cleanup feature (PR #56 / issue #41), bundled since each is a one-line-to-small fix:
- Dead code in
internal/cli/cleanup.go:166,177: confirm := true is declared and then only ever referenced via _ = confirm — it never actually gates anything (the real control flow is a continue elsewhere). Leftover from a refactor; delete both lines.
- Dead code in
internal/cli/upgrade.go:110: _ = yes immediately after yes has already been fully consumed by the cleanup-flag resolution block above it (lines 94-109). Remove.
internal/detector/fnm.go:247 (GlobalNpmPrefix's final error path) drops the underlying os.Stat error entirely:
return "", fmt.Errorf("fnm global npm prefix for %s not found at %s or %s", v, prefix, filepath.Join(dir, "installation", "lib", "node_modules"))
No %w, no err2 included, even though every one of its 6 sibling implementations (asdf, mise, n, nodenv, nvm, volta) correctly wraps the underlying OS error with %w in the equivalent function. Low severity (message quality/debuggability only), but worth making consistent.
internal/detector/nvm.go's Current() method has zero test coverage at the method level. nvm_test.go only tests parseNVMCurrent (the pure output parser) — there's no TestNVMCurrent_InvokesShell-style test asserting NVM.Current() actually builds and runs the source ... && nvm current script, unlike every other manager (fnm, mise, n, nodenv, asdf, volta all have this test). Given Current() is the safety-critical gate protecting the active version from deletion (see the related "Current() error leaves active version unprotected" issue), this is worth closing.
Severity
Low individually — bundled as one chore PR since none needs its own review cycle.
Describe the bug
Small cleanup items found while reviewing the post-upgrade cleanup feature (PR #56 / issue #41), bundled since each is a one-line-to-small fix:
internal/cli/cleanup.go:166,177:confirm := trueis declared and then only ever referenced via_ = confirm— it never actually gates anything (the real control flow is acontinueelsewhere). Leftover from a refactor; delete both lines.internal/cli/upgrade.go:110:_ = yesimmediately afteryeshas already been fully consumed by the cleanup-flag resolution block above it (lines 94-109). Remove.internal/detector/fnm.go:247(GlobalNpmPrefix's final error path) drops the underlyingos.Staterror entirely:%w, noerr2included, even though every one of its 6 sibling implementations (asdf, mise, n, nodenv, nvm, volta) correctly wraps the underlying OS error with%win the equivalent function. Low severity (message quality/debuggability only), but worth making consistent.internal/detector/nvm.go'sCurrent()method has zero test coverage at the method level.nvm_test.goonly testsparseNVMCurrent(the pure output parser) — there's noTestNVMCurrent_InvokesShell-style test assertingNVM.Current()actually builds and runs thesource ... && nvm currentscript, unlike every other manager (fnm, mise, n, nodenv, asdf, volta all have this test). GivenCurrent()is the safety-critical gate protecting the active version from deletion (see the related "Current() error leaves active version unprotected" issue), this is worth closing.Severity
Low individually — bundled as one
chorePR since none needs its own review cycle.