Skip to content

feat(cli): post-upgrade cleanup prompt + native mutation commands#56

Merged
dipto0321 merged 2 commits into
mainfrom
feat/upgrade-cleanup-prompt
Jul 1, 2026
Merged

feat(cli): post-upgrade cleanup prompt + native mutation commands#56
dipto0321 merged 2 commits into
mainfrom
feat/upgrade-cleanup-prompt

Conversation

@dipto0321

Copy link
Copy Markdown
Owner

Summary

After a successful upgrade, nodeup upgrade now asks whether to delete the old Node.js versions left behind. The prompt offers three options — y deletes every candidate, typing a specific version (e.g. 20.18.0) deletes only that one, and N (or empty enter) skips. Candidates are computed as installed \ {new LTS, new Current, currently active}.

To make the prompt actually reachable, this PR also fills in the Install, Uninstall, Use, SetDefault, GlobalNpmPrefix, and Current methods for all 7 working managers (fnm, nvm, Volta, asdf, mise, n, nodenv). The pre-existing stubs were returning ErrXxxNotImplemented, which made nodeup upgrade itself fail before even getting to migration or cleanup. Volta's and n's SetDefault remain intentional no-ops (those managers pin per-project, not per-machine). GlobalNpmPrefix returns the per-version npm global modules directory for each manager's on-disk layout. Current runs <manager> current (or per-manager equivalent) so we can exclude the active version from cleanup candidates.

nvm-windows stays unsupported and returns ErrNVMWindowsNotImplemented from Current; the prompt treats that error as "active version unknown" and proceeds without exclusion.

Pre-existing problem this PR also fixes

PR #20 merged end-to-end nodeup upgrade with m.Install(*v) and m.SetDefault(*latest) on lines 221/229 of internal/cli/upgrade.go, but every Manager implementation had those methods as stubs returning ErrXxxNotImplemented. This meant:

  • nodeup upgrade would fail immediately at line 221 with ErrFNMNotImplemented (or whichever manager is active).
  • Snapshot, migration, and the new cleanup prompt never executed.
  • The stale TODO comments at fnm.go:155-169 said "will be filled in when the upgrade command (Phase 4) needs to mutate state", but Phase 4 was feat(cli): implement end-to-end upgrade command #20 itself.

Without implementing the mutation methods, the cleanup prompt is unreachable. So this PR expands scope: it ships the user-requested cleanup feature AND wires the mutations that make it actually work.

Manager interface change

New method on Manager:

Current() (semver.Version, error)

Used to detect the currently-active version so the cleanup prompt can exclude it. Returns ErrNVMWindowsNotImplemented for nvm-windows (honestly documenting the gap).

New CLI flags

  • --cleanup — auto-confirm deletion of every candidate
  • --cleanup-version <v> — only delete these versions (repeatable; pairs with --cleanup)

Existing --no-cleanup flag stays as the "skip everything" toggle. --yes implies --cleanup for non-interactive runs (e.g., CI).

Config equivalents

~/.nodeup/config.yaml:

cleanup:
  auto: false   # set true to skip the all-or-nothing prompt
  prompt: true  # set false to skip per-version confirm

Precedence (highest first): --no-cleanup > --cleanup (or cleanup.auto: true) > --cleanup-version <v> > cleanup.prompt: false > default interactive prompt.

Per-manager behavior

Manager Install cmd Uninstall cmd Current query Notes
fnm fnm install <v> fnm uninstall <v> fnm current Refuses to uninstall active
nvm source nvm.sh && nvm install -s <v> … nvm uninstall <v> … nvm current system / none → "unknown"
Volta volta install node@<v> volta uninstall node@<v> First node@<v> row of volta list --format=plain SetDefault is a no-op
asdf asdf install nodejs <v> asdf uninstall nodejs <v> asdf current nodejs Plugin name is nodejs
mise mise install node@<v> mise uninstall node@<v> mise current node Writes to ~/.config/mise/config.toml
n n install <v> n uninstall <v> n current (n ≥ 8) SetDefault is a no-op
nodenv nodenv install <v> nodenv uninstall <v> nodenv version Writes to ~/.nodenv/version
nvm-windows unsupported unsupported returns ErrNVMWindowsNotImplemented Documented limitation

Tests

  • New internal/cli/cleanup_test.go — table-driven prompt tests covering: empty stdin (skip), y/n (delete-all vs skip), per-version y/n confirm, --cleanup (auto), --cleanup-version <v> (prefiltered), cleanup.auto config-driven, no candidates (skip with friendly note), exclusion logic (active version + new versions excluded), uninstall error handling (one failure doesn't block others).
  • Updated internal/detector/<manager>_test.go for each of the 7 working managers + nvm-windows: replaced the "stub returns ErrXxxNotImplemented" tests with real ones that capture the shell-out argv and assert it matches the per-manager command shape.
  • All tests pass with -race and 53.8% coverage.

Docs

  • README.md — added a paragraph to Quickstart explaining the post-upgrade prompt.
  • docs/managers.md — added a "Post-upgrade cleanup" section with the candidate-set formula, the flag table, the per-manager behavior table, and the failure modes.
  • CHANGELOG.md — added entries under Unreleased → Added.

Closes

🤖 Generated with puku-cli

dipto0321 and others added 2 commits July 2, 2026 03:23
After a successful upgrade, nodeup upgrade now asks whether to delete
the old Node.js versions left behind. The prompt offers three options
- y deletes every candidate, typing a specific version (e.g. 20.18.0)
deletes only that one, and N (or empty enter) skips. Candidates are
computed as installed \ {new LTS, new Current, currently active}.

To make that prompt reachable, this PR also fills in the Install,
Uninstall, Use, SetDefault, GlobalNpmPrefix, and Current methods for
all 7 working managers (fnm, nvm, Volta, asdf, mise, n, nodenv). The
pre-existing stubs were returning ErrXxxNotImplemented, which made
nodeup upgrade itself fail before even getting to migration or
cleanup. Volta and n's SetDefault remain intentional no-ops (those
managers pin per-project, not per-machine). GlobalNpmPrefix returns
the per-version npm global modules directory for each manager's
on-disk layout. Current runs <manager> current (or per-manager
equivalent) so we can exclude the active version from cleanup
candidates. nvm-windows stays unsupported and returns
ErrNVMWindowsNotImplemented from Current; the prompt treats that
error as "active version unknown" and proceeds without exclusion.

New Manager interface method: Current() (semver.Version, error).
New upgrade flags: --cleanup, --cleanup-version <v>. The existing
--no-cleanup flag stays. Config equivalents (cleanup.auto /
cleanup.prompt) under the documented names; precedence is
--no-cleanup > --cleanup / cleanup.auto > --cleanup-version >
cleanup.prompt > default prompt.

Closes #41.

Co-Authored-By: Sonnet 4.6 <noreply@puku.sh>
- paramTypeCombine (×3): combine adjacent []semver.Version params
  into a single declaration (runCleanupPrompt, cleanupCandidates,
  intersectCandidates).
- emptyStringTest: replace `len(s) > 0` with `s != ""` in
  trimNewline.
- SA4006 (×2): drop the unused `streams, _ := newCleanupIO("")`
  calls in TestCleanupPrompt_AutoDeleteAll and
  TestCleanupPrompt_PrefilteredOnly — both tests immediately
  reassign `streams` from a multi-line buffer anyway.

Co-Authored-By: Sonnet 4.6 <noreply@puku.sh>
@dipto0321 dipto0321 force-pushed the feat/upgrade-cleanup-prompt branch from e80d24e to 8169e0b Compare July 1, 2026 21:27
@cocogitto-bot

cocogitto-bot Bot commented Jul 1, 2026

Copy link
Copy Markdown

✔️ 0be7a8a...8169e0b - Conventional commits check succeeded.

@dipto0321 dipto0321 changed the title feat(upgrade): post-upgrade cleanup prompt + native mutation commands feat(cli): post-upgrade cleanup prompt + native mutation commands Jul 1, 2026
@dipto0321 dipto0321 merged commit 5ddbe43 into main Jul 1, 2026
10 checks passed
@dipto0321 dipto0321 deleted the feat/upgrade-cleanup-prompt branch July 1, 2026 21:33
dipto0321 pushed a commit that referenced this pull request Jul 5, 2026
…pkg-doc collision + dead FileOverlay

Residual stale-comment / dead-code cleanups from the 2026-07-03 re-review
that didn't belong in any of the active fix-wave PRs. All deletions /
docstring edits — no behavior change.

- internal/detector/fnm.go: delete ErrFNMNotImplemented (zero call
  sites, no tests, mutations have been real since PR #56); rewrite
  the FNM struct doc to describe the actual implemented surface
  (detection + mutations + layout queries) instead of "Phase 1 only,
  mutations not implemented"; drop the "left as stubs in Phase 1"
  paragraph from the mutation section.

- internal/packages/sentinel.go: demote `// Package sentinel
  implements…` to `// Sentinel file handling: …` — godoc attributes
  package-doc comments to the actual package name (`packages`), so
  the rendered godoc was wrong. Verified no canonical `// Package
  packages …` comment exists elsewhere in the package, so this was
  the sole package-level doc and the move doesn't leave the package
  undocumented.

- internal/config/resolve.go: delete superseded FileOverlay() (the
  coarse all-fields overlay that marked explicit-zero values as set
  — exactly the bug FileOverlayFromNode was introduced to fix in
  #85). Update the Overlay type's doc comment to point at
  FileOverlayFromNode as the file-layer builder.

Closes #104.

Co-Authored-By: puku-ai-2.8 <noreply@puku.sh>
dipto0321 pushed a commit that referenced this pull request Jul 5, 2026
Residual stale-comment / dead-code cleanups from the 2026-07-03
re-review that didn't belong in any of the active fix-wave PRs. All
deletions / docstring edits — no behavior change.

- internal/detector/fnm.go: delete ErrFNMNotImplemented (zero call
  sites, no tests, mutations real since PR #56); rewrite the FNM
  struct doc to describe the actual surface (detection + mutations
  + layout queries) instead of "Phase 1 only, not implemented"; drop
  the "left as stubs in Phase 1" paragraph from the mutation section.

- internal/packages/sentinel.go: demote "// Package sentinel
  implements..." to "// Sentinel file handling: ..." — godoc
  attributes package-doc comments to the actual package name
  (`packages`), so the rendered godoc was wrong. Verified no
  canonical "// Package packages ..." comment exists elsewhere, so
  the package isn't left undocumented.

- internal/config/resolve.go: delete superseded FileOverlay() (the
  coarse all-fields overlay that marked explicit-zero values as set
  — exactly the bug FileOverlayFromNode was introduced to fix in
  #85). Update the Overlay type's doc comment to point at
  FileOverlayFromNode as the file-layer builder.

Closes #104.

Co-Authored-By: puku-ai-2.8 <noreply@puku.sh>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant