From 75e873cd2cc87c91bbb79de990c651b06b69d7f7 Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Sun, 1 Nov 2026 21:19:52 +0600 Subject: [PATCH 1/4] fix(release): move ldflags out of flags so our build metadata survives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GoReleaser applies its own default ldflags only when builds[].ldflags is empty. Passing our ldflags through builds[].flags instead produced two -ldflags arguments on the go build command line, and Go takes the last one — so ours was silently dropped and the shipped binary reported GoReleaser's defaults (a full 40-char commit and the build date) instead of the short commit and commit date we wanted. Move the ldflags into the dedicated builds[].ldflags key, which keeps a single explicit -ldflags argument and preserves the metadata. Also clear the three deprecation warnings: - archives[].format -> archives[].formats (list form, v2.6) - archives[].format_overrides[].format -> .formats - brews: -> homebrew_casks: (v2.10) The archive names are unchanged, so the npm wrapper's archiveName() and the Homebrew/Scoop manifests still match. Closes #129 --- .goreleaser.yaml | 50 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 9 deletions(-) diff --git a/.goreleaser.yaml b/.goreleaser.yaml index 6ac18d7..89f5713 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -16,7 +16,7 @@ # - checksums.txt with SHA256 for every artifact (the 6th release # file, but it's a checksums manifest, not a binary) # - A GitHub Release titled `v` with changelog and all artifacts -# - A homebrew formula pushed to github.com/dipto0321/homebrew-tap +# - A homebrew cask pushed to github.com/dipto0321/homebrew-tap # - A scoop manifest pushed to github.com/dipto0321/scoop-bucket version: 2 @@ -36,7 +36,23 @@ builds: - CGO_ENABLED=0 flags: - -trimpath - - -ldflags=-s -w -X main.version={{.Version}} -X main.commit={{.ShortCommit}} -X main.date={{.CommitDate}} + # The ldflags live in the dedicated `ldflags` key, NOT in `flags`. + # + # GoReleaser has its own default ldflags (`-s -w -X main.version=... -X + # main.commit={{.Commit}} -X main.date={{.Date}} -X main.builtBy=goreleaser`) + # and it only applies them when `builds[].ldflags` is empty. Passing + # `-ldflags=...` through `flags` instead produced two `-ldflags` + # arguments on the `go build` command line — Go took the last one, so + # our `-X main.commit={{.ShortCommit}} -X main.date={{.CommitDate}}` + # was silently dropped and the shipped binary reported GoReleaser's + # defaults instead (a full 40-char commit and the build date rather + # than the commit date). GoReleaser also logs "ldflags is defined + # twice" to warn about exactly this. + # + # Using `ldflags:` keeps a single, explicit ldflags argument and + # preserves the short-commit / commit-date metadata we want. + ldflags: + - -s -w -X main.version={{.Version}} -X main.commit={{.ShortCommit}} -X main.date={{.CommitDate}} goos: - linux - windows @@ -51,10 +67,16 @@ builds: archives: - id: default - format: tar.gz + # `formats` (plural, list form) replaced the deprecated singular + # `format` in GoReleaser v2.6. The archive names are unchanged — + # `nodeup___.tar.gz` — which is what the npm + # wrapper's archiveName() and the Homebrew/Scoop manifests expect. + formats: + - tar.gz format_overrides: - goos: windows - format: zip + formats: + - zip name_template: '{{ .ProjectName }}_{{ .Version }}_{{ .Os }}_{{ .Arch }}' files: - LICENSE @@ -95,7 +117,19 @@ changelog: order: 999 # Homebrew tap — auto-pushed on every release. Requires HOMEBREW_TAP_TOKEN. -brews: +# +# GoReleaser deprecated the `brews:` (formula) section in v2.10 in favour of +# `homebrew_casks:`. A formula's `install:` block is raw Ruby; a cask's +# `binary:` stanza is declarative, and Homebrew now prefers casks for +# "download a binary and put it on PATH" distributions. +# +# `brew install dipto0321/tap/nodeup` keeps working after the switch — we +# verified the formula→cask transition against a scratch tap: Homebrew +# detects the change on `brew update`, installs the cask, unlinks the old +# keg, and tells the user to `brew uninstall --formula --force nodeup`. +# The tap repo carries a `tap_migrations.json` mapping so Homebrew can +# perform that hand-off automatically. +homebrew_casks: - name: nodeup repository: owner: dipto0321 @@ -104,10 +138,8 @@ brews: homepage: 'https://github.com/dipto0321/nodeup' description: 'Automated Node.js version upgrade with global package migration' license: 'MIT' - install: | - bin.install "nodeup" - test: | - system "#{bin}/nodeup", "--version" + binaries: + - nodeup # Scoop bucket — auto-pushed on every release. Requires SCOOP_BUCKET_TOKEN. scoops: From c2979c24d4de587919b0608215f49564a0ad969a Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Sun, 1 Nov 2026 21:30:29 +0600 Subject: [PATCH 2/4] =?UTF-8?q?fix(packages):=20stop=20the=20snapshot=20le?= =?UTF-8?q?ak=20=E2=80=94=20auto-reclaim=20+=20`packages=20cleanup`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing in nodeup ever deleted a snapshot file. The set only grew: one file per (manager, Node version) pair the user had ever installed, plus the orphan left behind by an upgrade that failed partway. The migration reports in /reports/ leaked the same way, with unbounded random-suffixed filenames. Two fixes. 1. Stop the leak automatically. The upgrade flow now records the paths it wrote and deletes them once the snapshots have been consumed — either because the migration replayed them onto the new Node version, or because there was nothing to migrate at all (every target was already installed, so the snapshots were redundant copies of the current state). A snapshot whose contents were NOT replayed survives: on the failure path it is the only record of the user's globals and the sentinel points straight at it. `--keep-snapshots` (or `packages.keep_snapshots` in the config) opts out of the automatic deletion entirely. 2. Give the user a manual lever: `nodeup packages cleanup`. It lists what's on disk with size and mtime, and lets the user pick individual files, several files, or all of them. `--list` shows what would be deleted and deletes nothing; `--reports` targets the migration reports instead; `--all --yes` deletes everything without prompting. The listing is deliberately file-based rather than built on ListSnapshots: that function skips files whose JSON body doesn't parse, so a snapshot truncated by a crash mid-write is invisible to it — and a cleanup command that can't see a file can't delete it. The new listing surfaces corrupt files, plus the ".json.tmp" siblings an interrupted atomic write leaves behind. Deletion is confined to its own directory. The target must resolve *inside* the snapshots (or reports) directory after symlink evaluation and must be a regular file, so a ".." path or a symlink pointing elsewhere is refused rather than followed. Three adjacent bugs fixed along the way: - The sentinel was armed even when there was nothing to install. With no install step and no restore step, restoreSucceeded never became true, so the sentinel was never cleared and every later `nodeup` invocation warned about a phantom "interrupted upgrade". - `nodeup packages snapshot` captured the LOWEST installed version's globals, not the active one — getCurrentVersion returned versions[0], and ListInstalled sorts ascending. On a machine with Node 18 and Node 20 that captured the (usually empty) 18 install under the 20.x filename, producing a snapshot useless as a restore source. It now asks Manager.Current() first and falls back to the highest installed version. - Snapshot writes were not atomic. A plain os.WriteFile truncates the destination first, so a crash mid-write left a truncated JSON file that no reader could parse — invisible to every code path, but still occupying disk. Writes now go through a sibling temp file and a rename. Also adds ui.Prompt.MultiSelect: huh's MultiSelect in FancyMode, and a line-based comma/space-separated number-or-name picker in PlainMode. Both degrade to "nothing selected" on EOF or a blank answer — the safe direction for a destructive prompt. Closes #128 --- internal/cli/cleanup_snapshots.go | 212 +++++++++++++ internal/cli/cleanup_snapshots_test.go | 271 ++++++++++++++++ internal/cli/config.go | 8 +- internal/cli/packages.go | 33 +- internal/cli/packages_test.go | 87 +++++ internal/cli/upgrade.go | 119 ++++++- internal/cli/upgrade_snapshots_test.go | 168 ++++++++++ internal/config/config.go | 20 ++ internal/config/config_test.go | 6 + internal/config/env.go | 12 +- internal/config/fileoverlay.go | 8 +- internal/config/load_test.go | 9 + internal/config/resolve.go | 22 +- internal/detector/interactive_test.go | 3 + internal/packages/cleanup.go | 377 ++++++++++++++++++++++ internal/packages/cleanup_test.go | 422 +++++++++++++++++++++++++ internal/packages/snapshot.go | 28 +- internal/ui/prompt.go | 141 +++++++++ internal/ui/prompt_test.go | 146 +++++++++ 19 files changed, 2071 insertions(+), 21 deletions(-) create mode 100644 internal/cli/cleanup_snapshots.go create mode 100644 internal/cli/cleanup_snapshots_test.go create mode 100644 internal/cli/upgrade_snapshots_test.go create mode 100644 internal/packages/cleanup.go create mode 100644 internal/packages/cleanup_test.go diff --git a/internal/cli/cleanup_snapshots.go b/internal/cli/cleanup_snapshots.go new file mode 100644 index 0000000..c750c4b --- /dev/null +++ b/internal/cli/cleanup_snapshots.go @@ -0,0 +1,212 @@ +package cli + +import ( + "fmt" + "sort" + + "github.com/spf13/cobra" + + "github.com/dipto0321/nodeup/internal/packages" + "github.com/dipto0321/nodeup/internal/ui" +) + +// newCleanupCmd implements `nodeup packages cleanup` — delete the +// snapshot JSON files that accumulate in /snapshots. +// +// # WHY THIS EXISTS +// +// Snapshots are written by `nodeup upgrade` (one per installed Node +// version) and by `nodeup packages snapshot`, but nothing in nodeup ever +// deleted one. The set therefore only ever grows: one file per +// (manager, Node version) pair the user has ever had installed, plus +// the orphan left behind when an upgrade fails partway or is killed +// mid-flight. Once the cleanup prompt uninstalls an old Node version, +// its snapshot can never be rewritten — the file is pure disk weight. +// +// This command gives the user explicit control: pick the files to +// delete one by one, or delete them all at once. +// +// The migration reports in /reports/ leak the same way (one +// random-suffixed file per migration attempt, never overwritten, never +// deleted), so this command cleans those up too. They're offered +// separately from the snapshots because they're a different kind of +// artifact — a record of what happened, versus a rollback payload. +func newCleanupCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "cleanup", + Short: "Delete old snapshot and report files", + Long: `Delete the snapshot and migration-report files nodeup keeps on disk. + +nodeup writes a snapshot of your global npm packages before every upgrade +so it can roll back or replay the migration, and deletes it again once +the migration has replayed the packages onto the new Node version. + +What remains are the snapshots a FAILED upgrade left behind — those are +the rollback payload the "interrupted upgrade" warning points at — plus +the ones from Node versions you have since uninstalled. This command +lists what's on disk and lets you choose what to delete. + +Examples: + nodeup packages cleanup Select snapshots to delete + nodeup packages cleanup --all Delete every snapshot (asks first) + nodeup packages cleanup --all --yes Delete every snapshot, no prompt + nodeup packages cleanup --reports Clean up migration reports instead + nodeup packages cleanup --list Show what would be deleted, delete nothing`, + RunE: runCleanup, + } + + cmd.Flags().Bool("all", false, "delete every snapshot instead of selecting individually") + cmd.Flags().Bool("reports", false, "clean up migration reports instead of snapshots") + cmd.Flags().Bool("list", false, "list the files that would be deleted and exit") + cmd.Flags().Bool("yes", false, "skip the confirmation prompt (implies --all)") + + return cmd +} + +func runCleanup(cmd *cobra.Command, args []string) error { + w := writerFromCmd(cmd) + + all, _ := cmd.Flags().GetBool("all") + reports, _ := cmd.Flags().GetBool("reports") + listOnly, _ := cmd.Flags().GetBool("list") + yes, _ := cmd.Flags().GetBool("yes") + + // --yes implies --all: an unattended run can't answer a + // multi-select question, so it has to mean "delete them all". + if yes { + all = true + } + + var files []packages.SnapshotFile + var kind string + var delete func(string) error + + if reports { + kind = "migration report" + f, err := packages.ListReportFiles() + if err != nil { + return fmt.Errorf("list migration reports: %w", err) + } + files = f + delete = packages.DeleteReportFile + } else { + kind = "snapshot" + f, err := packages.ListSnapshotFiles() + if err != nil { + return fmt.Errorf("list snapshots: %w", err) + } + files = f + delete = packages.DeleteSnapshotFile + } + + if len(files) == 0 { + w.Info(fmt.Sprintf("No %s files found.", kind)) + return nil + } + + w.Info(fmt.Sprintf("%s files on disk (%d):", kind, len(files))) + for _, f := range files { + w.Println(fmt.Sprintf(" %s %s %s", f.Name, formatBytes(f.Size), f.ModTime.Format("2006-01-02 15:04"))) + } + + if listOnly { + w.Info("Nothing deleted (--list).") + return nil + } + + // Build the selection. + var selected []packages.SnapshotFile + switch { + case all: + selected = files + default: + // Interactive multi-select. The prompt is constructed here + // (rather than passed in) because this command owns its own + // interaction; the upgrade command's prompt is built around + // the cleanup flow it shares with Node version deletion. + prompt := ui.NewPrompt(w.Mode(), cmd.InOrStdin(), cmd.OutOrStdout()) + options := make([]string, len(files)) + for i, f := range files { + options[i] = f.Name + } + picked, err := prompt.MultiSelect(fmt.Sprintf("Select %ss to delete (blank = none):", kind), options) + if err != nil { + return fmt.Errorf("cleanup prompt: %w", err) + } + if len(picked) == 0 { + w.Info("Nothing selected — nothing deleted.") + return nil + } + // Map the picked labels back to files. Labels are the file + // names, which are unique within the directory. + byName := make(map[string]packages.SnapshotFile, len(files)) + for _, f := range files { + byName[f.Name] = f + } + for _, name := range picked { + if f, ok := byName[name]; ok { + selected = append(selected, f) + } + } + } + + if len(selected) == 0 { + w.Info("Nothing selected — nothing deleted.") + return nil + } + + // Confirm before deleting. --all --yes skips this (the user already + // said "delete them all, don't ask"); every other path asks. + if !yes { + prompt := ui.NewPrompt(w.Mode(), cmd.InOrStdin(), cmd.OutOrStdout()) + ok, err := prompt.Confirm(fmt.Sprintf("Delete %d %s file(s)?", len(selected), kind), false) + if err != nil { + return fmt.Errorf("cleanup confirmation: %w", err) + } + if !ok { + w.Info("Cleanup cancelled.") + return nil + } + } + + // Delete. A failure on one file doesn't stop the rest — we report + // every failure at the end so the user knows the full picture. + var deleted int + var failures []string + for _, f := range selected { + if err := delete(f.Path); err != nil { + failures = append(failures, fmt.Sprintf("%s: %v", f.Name, err)) + continue + } + deleted++ + w.Success(fmt.Sprintf("Deleted %s", f.Name)) + } + + w.Info(fmt.Sprintf("Deleted %d of %d %s file(s).", deleted, len(selected), kind)) + if len(failures) > 0 { + sort.Strings(failures) + for _, msg := range failures { + w.Warn(fmt.Sprintf("Failed to delete %s", msg)) + } + // Return an error so CI scripts notice a partial failure. + return fmt.Errorf("failed to delete %d of %d %s file(s)", len(failures), len(selected), kind) + } + return nil +} + +// formatBytes renders a byte count in a human-readable form. +// +// We use the binary (1024-based) units because that's what `ls -h` shows +// on macOS, and what users comparing against `du` will expect. +func formatBytes(n int64) string { + const unit = 1024 + if n < unit { + return fmt.Sprintf("%d B", n) + } + div, exp := int64(unit), 0 + for v := n / unit; v >= unit; v /= unit { + div *= unit + exp++ + } + return fmt.Sprintf("%.1f %cB", float64(n)/float64(div), "KMGTPE"[exp]) +} diff --git a/internal/cli/cleanup_snapshots_test.go b/internal/cli/cleanup_snapshots_test.go new file mode 100644 index 0000000..292837b --- /dev/null +++ b/internal/cli/cleanup_snapshots_test.go @@ -0,0 +1,271 @@ +package cli + +import ( + "bytes" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/dipto0321/nodeup/internal/packages" + "github.com/dipto0321/nodeup/internal/platform" +) + +// redirectCleanupDataDir points nodeup's data directory at a temp dir +// so the cleanup command's file operations never touch the developer's +// real snapshots. Mirrors the helper in internal/packages — the CLI +// package has its own because the packages helper isn't exported. +func redirectCleanupDataDir(t *testing.T) string { + t.Helper() + tmp := t.TempDir() + + t.Setenv("XDG_DATA_HOME", "") + t.Setenv("APPDATA", "") + if runtime.GOOS == "windows" { + t.Setenv("USERPROFILE", tmp) + t.Setenv("HOME", tmp) + } else { + t.Setenv("HOME", tmp) + } + return tmp +} + +// writeCleanupSnapshot drops a snapshot file into the snapshots dir. +func writeCleanupSnapshot(t *testing.T, name, body string) string { + t.Helper() + dir, err := platform.SnapshotsDir() + if err != nil { + t.Fatalf("SnapshotsDir: %v", err) + } + path := filepath.Join(dir, name) + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatalf("write %s: %v", path, err) + } + return path +} + +// TestCleanupCmd_ListOnlyDeletesNothing verifies --list shows what +// would be deleted and leaves everything on disk. +func TestCleanupCmd_ListOnlyDeletesNothing(t *testing.T) { + redirectCleanupDataDir(t) + writeCleanupSnapshot(t, "fnm-20.10.0.json", `{"manager":"fnm","node_version":"20.10.0","packages":[]}`) + + cmd := newCleanupCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + cmd.SetArgs([]string{"--list"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("cleanup --list: %v", err) + } + + out := buf.String() + if !strings.Contains(out, "fnm-20.10.0.json") { + t.Errorf("output %q missing the snapshot name", out) + } + if !strings.Contains(out, "Nothing deleted") { + t.Errorf("output %q missing the 'Nothing deleted' notice", out) + } + + files, err := packages.ListSnapshotFiles() + if err != nil { + t.Fatalf("ListSnapshotFiles: %v", err) + } + if len(files) != 1 { + t.Errorf("snapshot count after --list = %d, want 1 (--list must not delete)", len(files)) + } +} + +// TestCleanupCmd_AllYesDeletesEverything verifies the --all --yes +// path deletes every snapshot without prompting. +func TestCleanupCmd_AllYesDeletesEverything(t *testing.T) { + redirectCleanupDataDir(t) + writeCleanupSnapshot(t, "fnm-20.10.0.json", `{"manager":"fnm","node_version":"20.10.0","packages":[]}`) + writeCleanupSnapshot(t, "fnm-22.0.0.json", `{"manager":"fnm","node_version":"22.0.0","packages":[]}`) + + cmd := newCleanupCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + cmd.SetArgs([]string{"--all", "--yes"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("cleanup --all --yes: %v", err) + } + + files, err := packages.ListSnapshotFiles() + if err != nil { + t.Fatalf("ListSnapshotFiles: %v", err) + } + if len(files) != 0 { + t.Errorf("snapshot count after --all --yes = %d, want 0", len(files)) + } +} + +// TestCleanupCmd_InteractiveSelectionDeletesOnlyPicked verifies the +// multi-select path: the user picks specific files and only those +// are removed. +// +// The prompt is PlainMode here (the leaf command built in isolation +// has no writer in its context, so writerFromCmd falls back to a +// PlainMode writer) and reads a comma-separated list of numbers. +func TestCleanupCmd_InteractiveSelectionDeletesOnlyPicked(t *testing.T) { + redirectCleanupDataDir(t) + writeCleanupSnapshot(t, "fnm-20.10.0.json", `{"manager":"fnm","node_version":"20.10.0","packages":[]}`) + writeCleanupSnapshot(t, "fnm-22.0.0.json", `{"manager":"fnm","node_version":"22.0.0","packages":[]}`) + writeCleanupSnapshot(t, "fnm-24.0.0.json", `{"manager":"fnm","node_version":"24.0.0","packages":[]}`) + + cmd := newCleanupCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + // "1" picks the first listed file; then "y" confirms. + cmd.SetIn(strings.NewReader("1\ny\n")) + + if err := cmd.Execute(); err != nil { + t.Fatalf("cleanup (interactive): %v", err) + } + + files, err := packages.ListSnapshotFiles() + if err != nil { + t.Fatalf("ListSnapshotFiles: %v", err) + } + if len(files) != 2 { + names := make([]string, len(files)) + for i, f := range files { + names[i] = f.Name + } + t.Errorf("after picking 1 file: %d remain (%v), want 2", len(files), names) + } +} + +// TestCleanupCmd_InteractiveBlankSelectsNothing verifies that answering +// the multi-select with a blank line deletes NOTHING — the safe +// direction for a destructive prompt. +func TestCleanupCmd_InteractiveBlankSelectsNothing(t *testing.T) { + redirectCleanupDataDir(t) + writeCleanupSnapshot(t, "fnm-20.10.0.json", `{"manager":"fnm","node_version":"20.10.0","packages":[]}`) + + cmd := newCleanupCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + cmd.SetIn(strings.NewReader("\n")) + + if err := cmd.Execute(); err != nil { + t.Fatalf("cleanup (blank selection): %v", err) + } + + files, err := packages.ListSnapshotFiles() + if err != nil { + t.Fatalf("ListSnapshotFiles: %v", err) + } + if len(files) != 1 { + t.Errorf("snapshot count after blank selection = %d, want 1 (nothing selected = nothing deleted)", len(files)) + } +} + +// TestCleanupCmd_InteractiveConfirmationDeclined verifies that +// answering "n" to the confirmation keeps the files. +func TestCleanupCmd_InteractiveConfirmationDeclined(t *testing.T) { + redirectCleanupDataDir(t) + writeCleanupSnapshot(t, "fnm-20.10.0.json", `{"manager":"fnm","node_version":"20.10.0","packages":[]}`) + + cmd := newCleanupCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + // Pick the file, then decline the confirmation. + cmd.SetIn(strings.NewReader("1\nn\n")) + + if err := cmd.Execute(); err != nil { + t.Fatalf("cleanup (declined): %v", err) + } + + files, err := packages.ListSnapshotFiles() + if err != nil { + t.Fatalf("ListSnapshotFiles: %v", err) + } + if len(files) != 1 { + t.Errorf("snapshot count after declined confirmation = %d, want 1", len(files)) + } +} + +// TestCleanupCmd_ReportsFlag verifies --reports targets the migration +// reports directory instead of the snapshots directory. +func TestCleanupCmd_ReportsFlag(t *testing.T) { + redirectCleanupDataDir(t) + + // A snapshot AND a report — only the report should be removed. + writeCleanupSnapshot(t, "fnm-20.10.0.json", `{"manager":"fnm","node_version":"20.10.0","packages":[]}`) + reportDir, err := platform.ReportsDir() + if err != nil { + t.Fatalf("ReportsDir: %v", err) + } + reportPath := filepath.Join(reportDir, "migration-20260101-abcdef.json") + if err := os.WriteFile(reportPath, []byte(`{}`), 0o644); err != nil { + t.Fatalf("write report: %v", err) + } + + cmd := newCleanupCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + cmd.SetArgs([]string{"--reports", "--all", "--yes"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("cleanup --reports: %v", err) + } + + if _, statErr := os.Stat(reportPath); !os.IsNotExist(statErr) { + t.Errorf("report still exists after --reports --all --yes (stat err = %v)", statErr) + } + snaps, err := packages.ListSnapshotFiles() + if err != nil { + t.Fatalf("ListSnapshotFiles: %v", err) + } + if len(snaps) != 1 { + t.Errorf("snapshot count = %d, want 1 (--reports must not touch snapshots)", len(snaps)) + } +} + +// TestCleanupCmd_NoFiles verifies the empty case is a friendly +// no-op rather than an error. +func TestCleanupCmd_NoFiles(t *testing.T) { + redirectCleanupDataDir(t) + + cmd := newCleanupCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + cmd.SetArgs([]string{"--all", "--yes"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("cleanup with no files: %v", err) + } + if !strings.Contains(buf.String(), "No snapshot files found") { + t.Errorf("output %q missing the 'No snapshot files found' notice", buf.String()) + } +} + +// TestFormatBytes covers the human-readable size rendering. +func TestFormatBytes(t *testing.T) { + tests := []struct { + in int64 + want string + }{ + {0, "0 B"}, + {512, "512 B"}, + {1024, "1.0 KB"}, + {1536, "1.5 KB"}, + {1024 * 1024, "1.0 MB"}, + {1024 * 1024 * 1024, "1.0 GB"}, + } + for _, tc := range tests { + if got := formatBytes(tc.in); got != tc.want { + t.Errorf("formatBytes(%d) = %q, want %q", tc.in, got, tc.want) + } + } +} diff --git a/internal/cli/config.go b/internal/cli/config.go index 0b50c27..144e6a5 100644 --- a/internal/cli/config.go +++ b/internal/cli/config.go @@ -83,8 +83,8 @@ to a file as a starting point for a new config.`, // Supported keys mirror config.SetByKey: // // manager, track.lts, track.current, packages.migrate, -// packages.strategy, packages.skip, cleanup.auto, -// cleanup.prompt, cache.ttl +// packages.strategy, packages.skip, packages.keep_snapshots, +// cleanup.auto, cleanup.prompt, cache.ttl func newConfigGetCmd() *cobra.Command { return &cobra.Command{ Use: "get ", @@ -248,6 +248,8 @@ func configGet(c *config.Config, key string) (string, error) { return string(c.Packages.Strategy), nil case "packages.skip": return strings.Join(c.Packages.Skip, ","), nil + case "packages.keep_snapshots": + return boolStr(c.Packages.KeepSnapshots), nil case "cleanup.auto": return boolStr(c.Cleanup.Auto), nil case "cleanup.prompt": @@ -255,7 +257,7 @@ func configGet(c *config.Config, key string) (string, error) { case "cache.ttl": return fmt.Sprintf("%d", c.Cache.TTL), nil default: - return "", fmt.Errorf("unknown key %q (supported: manager, track.lts, track.current, packages.migrate, packages.strategy, packages.skip, cleanup.auto, cleanup.prompt, cache.ttl)", key) + return "", fmt.Errorf("unknown key %q (supported: manager, track.lts, track.current, packages.migrate, packages.strategy, packages.skip, packages.keep_snapshots, cleanup.auto, cleanup.prompt, cache.ttl)", key) } } diff --git a/internal/cli/packages.go b/internal/cli/packages.go index e76de82..26c5323 100644 --- a/internal/cli/packages.go +++ b/internal/cli/packages.go @@ -13,13 +13,18 @@ import ( ) // newPackagesCmd implements `nodeup packages` — manage global npm package snapshots. -// Subcommands: snapshot, list, restore, diff. +// Subcommands: snapshot, list, restore, diff, cleanup. func newPackagesCmd() *cobra.Command { cmd := &cobra.Command{ Use: "packages", Short: "Manage global npm package snapshots", - Long: `Manage global npm package snapshots — capture, list, restore, and -diff the set of globally installed packages per Node.js version.`, + Long: `Manage global npm package snapshots — capture, list, restore, diff, +and clean up the set of globally installed packages per Node.js version. + +Snapshots are written before every upgrade so the migration can be +rolled back or replayed. They're deleted automatically once the +migration has replayed them; the ones left behind by a failed upgrade +are reclaimed with 'nodeup packages cleanup'.`, RunE: func(cmd *cobra.Command, args []string) error { return cmd.Help() }, @@ -29,6 +34,7 @@ diff the set of globally installed packages per Node.js version.`, cmd.AddCommand(newPackagesListCmd()) cmd.AddCommand(newRestoreCmd()) cmd.AddCommand(newDiffCmd()) + cmd.AddCommand(newCleanupCmd()) return cmd } @@ -61,7 +67,24 @@ func runSnapshot(cmd *cobra.Command, args []string) error { return nil } +// getCurrentVersion resolves the Node version whose global npm packages +// should be captured. +// +// It asks the manager for its *active* version first (Manager.Current), +// and only falls back to the installed-version list when the manager +// cannot report one (e.g. nvm-windows returns "not implemented"). +// +// The previous implementation returned versions[0] — the LOWEST +// installed version, because ListInstalled sorts ascending. That meant +// `nodeup packages snapshot` on a machine with Node 18 and Node 20 +// captured the globals of the 18 install (usually empty) under the +// 20.x filename, and the resulting snapshot was useless as a restore +// source. See #128. func getCurrentVersion(ctx context.Context, m detector.Manager) (semver.Version, error) { + if cur, err := m.Current(ctx); err == nil && !cur.Equal(&semver.Version{}) { + return cur, nil + } + versions, err := m.ListInstalled(ctx) if err != nil { return semver.Version{}, err @@ -69,7 +92,9 @@ func getCurrentVersion(ctx context.Context, m detector.Manager) (semver.Version, if len(versions) == 0 { return semver.Version{}, fmt.Errorf("no installed versions") } - return versions[0], nil + // ListInstalled is sorted ascending — the highest installed + // version is the closest thing to "current" we have. + return versions[len(versions)-1], nil } func newPackagesListCmd() *cobra.Command { diff --git a/internal/cli/packages_test.go b/internal/cli/packages_test.go index afb1ff6..f24fd72 100644 --- a/internal/cli/packages_test.go +++ b/internal/cli/packages_test.go @@ -2,8 +2,12 @@ package cli import ( "bytes" + "context" + "errors" "strings" "testing" + + "github.com/Masterminds/semver/v3" ) // TestRunRestore_RejectsPathTraversal exercises the manager-name @@ -90,3 +94,86 @@ func TestRunRestore_AcceptsCanonicalName(t *testing.T) { t.Errorf("runRestore(\"fnm\"): allowlist wrongly rejected canonical name: %v", err) } } + +// stubCurrentManager is a detector.Manager whose Current() answer is +// configurable, used to pin getCurrentVersion's behavior. +type stubCurrentManager struct { + name string + current semver.Version + currentErr error + installed []semver.Version +} + +func (s *stubCurrentManager) Name() string { return s.name } +func (s *stubCurrentManager) Detect() bool { return true } +func (s *stubCurrentManager) Version() (string, error) { return "0.0.0-test", nil } +func (s *stubCurrentManager) ListInstalled(context.Context) ([]semver.Version, error) { + return s.installed, nil +} +func (s *stubCurrentManager) Install(semver.Version) error { return nil } +func (s *stubCurrentManager) Uninstall(semver.Version) error { return nil } +func (s *stubCurrentManager) Use(semver.Version) error { return nil } +func (s *stubCurrentManager) SetDefault(semver.Version) error { return nil } +func (s *stubCurrentManager) GlobalNpmPrefix(semver.Version) (string, error) { return "", nil } +func (s *stubCurrentManager) Current(context.Context) (semver.Version, error) { + if s.currentErr != nil { + return semver.Version{}, s.currentErr + } + return s.current, nil +} + +// TestGetCurrentVersion_PrefersManagerCurrent pins that +// `nodeup packages snapshot` captures the ACTIVE version's globals. +// +// The previous implementation returned versions[0] — the LOWEST +// installed version, because ListInstalled sorts ascending. On a +// machine with Node 18 and Node 20 that captured the (usually empty) +// 18 install under the 20.x filename, producing a snapshot that was +// useless as a restore source. See #128. +func TestGetCurrentVersion_PrefersManagerCurrent(t *testing.T) { + cur := *semver.MustParse("20.10.0") + m := &stubCurrentManager{ + name: "fnm", + current: cur, + installed: []semver.Version{*semver.MustParse("18.0.0"), cur}, + } + + got, err := getCurrentVersion(context.Background(), m) + if err != nil { + t.Fatalf("getCurrentVersion: %v", err) + } + if got.String() != "20.10.0" { + t.Errorf("getCurrentVersion = %s, want 20.10.0 (the active version)", got) + } +} + +// TestGetCurrentVersion_FallsBackToHighestInstalled covers the +// managers that cannot report an active version (nvm-windows returns +// "not implemented"). The fallback must be the HIGHEST installed +// version, not the lowest. +func TestGetCurrentVersion_FallsBackToHighestInstalled(t *testing.T) { + m := &stubCurrentManager{ + name: "nvm-windows", + currentErr: errors.New("not implemented"), + installed: []semver.Version{*semver.MustParse("18.0.0"), *semver.MustParse("20.10.0"), *semver.MustParse("22.0.0")}, + } + + got, err := getCurrentVersion(context.Background(), m) + if err != nil { + t.Fatalf("getCurrentVersion: %v", err) + } + if got.String() != "22.0.0" { + t.Errorf("getCurrentVersion = %s, want 22.0.0 (the highest installed version)", got) + } +} + +// TestGetCurrentVersion_NoVersionsErrors verifies the empty case. +func TestGetCurrentVersion_NoVersionsErrors(t *testing.T) { + m := &stubCurrentManager{ + name: "fnm", + currentErr: errors.New("no active version"), + } + if _, err := getCurrentVersion(context.Background(), m); err == nil { + t.Error("getCurrentVersion with no versions = nil error, want an error") + } +} diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index 060953b..9a0f6f9 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -32,6 +32,7 @@ migrate your global npm packages across automatically.`, cmd.Flags().Bool("current", false, "upgrade Current only") cmd.Flags().Bool("dry-run", false, "show the plan without making changes") cmd.Flags().Bool("no-migrate", false, "skip global package migration") + cmd.Flags().Bool("keep-snapshots", false, "keep the package snapshots written during the upgrade (they are deleted once the migration succeeds)") cmd.Flags().Bool("no-cleanup", false, "skip the prompt to remove old versions") cmd.Flags().Bool("cleanup", false, "auto-confirm the post-upgrade cleanup of old versions") cmd.Flags().StringSlice("cleanup-version", nil, "specify version(s) to delete (repeatable; pairs with --cleanup)") @@ -47,6 +48,7 @@ func runUpgrade(cmd *cobra.Command, args []string) error { currentOnly, _ := cmd.Flags().GetBool("current") dryRun, _ := cmd.Flags().GetBool("dry-run") noMigrate, _ := cmd.Flags().GetBool("no-migrate") + keepSnapshots, _ := cmd.Flags().GetBool("keep-snapshots") noCleanup, _ := cmd.Flags().GetBool("no-cleanup") autoCleanup, _ := cmd.Flags().GetBool("cleanup") cleanupVersionsRaw, _ := cmd.Flags().GetStringSlice("cleanup-version") @@ -105,12 +107,61 @@ func runUpgrade(cmd *cobra.Command, args []string) error { // remains available to retry just the failed packages. sentinelArmed := false restoreSucceeded := false + + // snapshotPaths records every snapshot file this upgrade wrote. + // They are the rollback payload for the migration: once the + // restore has replayed them onto the new Node, they are spent + // and would otherwise sit on disk forever. See #128. + // + // The declaration lives here (rather than next to the snapshot + // loop further down) because the deferred cleanup below captures + // it — a variable must be in scope at the point the closure is + // written, not at the point it is invoked. + var snapshotPaths []string + + // snapshotsConsumed tracks whether the snapshots this run wrote + // have served their purpose. It is set when either + // + // - the migration replayed them onto the new Node version + // (restoreSucceeded), or + // - there was nothing to migrate at all — every target version + // was already installed, so the snapshots are redundant + // copies of the current state. + // + // It stays false when the migration failed or the upgrade + // aborted mid-flight: in those cases the snapshot is the ONLY + // record of what the user's globals were, and the sentinel + // points at it. + snapshotsConsumed := false + + // keepSnapshots lets the user opt out of the automatic snapshot + // deletion. The CLI flag wins over the config file. + keepSnapshots = keepSnapshots || cfg.Packages.KeepSnapshots + defer func() { if sentinelArmed && restoreSucceeded { if err := packages.RemoveSentinel(); err != nil { writerFromCmd(cmd).Warn(fmt.Sprintf("Warning: failed to remove upgrade sentinel: %v", err)) } } + + // Snapshot lifecycle. We only reclaim the snapshots once they + // have been consumed — see the snapshotsConsumed comment + // above. A snapshot whose contents were NOT replayed survives, + // because on the failure path it is the rollback payload the + // interrupted-upgrade warning points at, and the user may + // want to replay it by hand. + // + // Note that all the snapshots written by one upgrade hold the + // SAME package set: `npm ls -g` reports the active Node's + // globals regardless of which version we label the file with. + // The per-version files exist so the user can name the source + // version in `nodeup packages restore `, not + // because they carry distinct data. + if !snapshotsConsumed || keepSnapshots { + return + } + reclaimSnapshots(cmd, snapshotPaths) }() // Detect managers. ResolveManagerAuto returns @@ -264,8 +315,9 @@ func runUpgrade(cmd *cobra.Command, args []string) error { // version so the user can manually replay against any of them via // `nodeup packages restore `, but the upgrade flow // itself only replays from the latest installed version (below) — - // the snapshots for older installed versions remain on disk for - // the user's reference. + // the snapshots for older installed versions are there for the + // duration of the upgrade and are reclaimed once the migration + // succeeds (see the deferred cleanup above). // // While we're at it, resolve the conventional snapshot path for // the latest installed version — this is the path both the @@ -288,6 +340,14 @@ func runUpgrade(cmd *cobra.Command, args []string) error { label := fmt.Sprintf("Snapshotting globals (%s)", ver) if serr := ui.WaitWithSpinner(ctx, spinnerFor(cmd, label), take); serr != nil { writerFromCmd(cmd).Warn(fmt.Sprintf("Warning: snapshot failed for %s: %v", ver, serr)) + continue + } + // Record the path only when the snapshot actually landed + // on disk. A failed snapshot has nothing to reclaim, and + // recording it anyway would make the deferred cleanup + // warn about a file that was never created. + if p, perr := packages.SnapshotPath(m.Name(), ver.String()); perr == nil { + snapshotPaths = append(snapshotPaths, p) } } if len(installedVersions) > 0 { @@ -302,6 +362,21 @@ func runUpgrade(cmd *cobra.Command, args []string) error { restoreSnapshotPath = p } } + + // Nothing to migrate: every target version was already + // installed, so the upgrade has no install step and no + // restore step. The snapshots we just wrote are redundant + // copies of the current state — there's nothing they are + // needed for, and nothing will replay them. Mark them + // consumed so the deferred cleanup reclaims the space. + // + // (This is the "nodeup upgrade when you're already up to + // date" case. It writes N snapshot files and then does + // nothing with them — which is exactly how the set grew + // without bound.) + if len(toInstall) == 0 { + snapshotsConsumed = true + } } // Arm the sentinel AFTER snapshots are on disk but BEFORE any @@ -312,11 +387,16 @@ func runUpgrade(cmd *cobra.Command, args []string) error { // The sentinel records the snapshot path so that // `nodeup packages restore --from ` can replay // against exactly the same package set we were about to install. - if !skipMigrate { - newVersion := "" - if len(toInstall) > 0 { - newVersion = toInstall[len(toInstall)-1].String() - } + // + // We only arm it when there is actually something to migrate + // (len(toInstall) > 0). With nothing to install there is no + // install step, no restore step, and no restoreSucceeded — so a + // sentinel armed here would never be cleared by the deferred + // cleanup, and EVERY subsequent `nodeup` invocation would warn + // about a phantom "interrupted upgrade". That warning is a lie: + // there was no upgrade in progress. See #128. + if !skipMigrate && len(toInstall) > 0 { + newVersion := toInstall[len(toInstall)-1].String() if err := packages.WriteSentinel(packages.UpgradeSentinel{ Manager: m.Name(), OldVersion: oldVersion, @@ -449,6 +529,12 @@ func runUpgrade(cmd *cobra.Command, args []string) error { writerFromCmd(cmd).Warn(fmt.Sprintf("Warning: restore failed: %v", rerr)) } else { restoreSucceeded = true + // The packages were replayed onto the new Node + // version, so the snapshot has served its purpose. + // The deferred cleanup reclaims it (and the + // per-version siblings) unless --keep-snapshots + // says otherwise. + snapshotsConsumed = true } } } @@ -522,6 +608,25 @@ func runUpgrade(cmd *cobra.Command, args []string) error { return nil } +// reclaimSnapshots deletes the snapshot files an upgrade wrote once +// they have been consumed. +// +// A failure to delete one file does not stop the rest — we report +// every failure and keep going, because the alternative (aborting the +// upgrade over a stale file) is strictly worse. The user's actual +// goal — the upgrade — has already succeeded. +// +// The deletion itself is guarded by packages.DeleteSnapshotFile, +// which refuses to remove anything that doesn't resolve inside the +// snapshots directory. See #128. +func reclaimSnapshots(cmd *cobra.Command, paths []string) { + for _, p := range paths { + if err := packages.DeleteSnapshotFile(p); err != nil { + writerFromCmd(cmd).Warn(fmt.Sprintf("Warning: failed to delete snapshot %s: %v", p, err)) + } + } +} + func parseVersion(s string) (*semver.Version, error) { if s != "" && s[0] == 'v' { s = s[1:] diff --git a/internal/cli/upgrade_snapshots_test.go b/internal/cli/upgrade_snapshots_test.go new file mode 100644 index 0000000..aee4996 --- /dev/null +++ b/internal/cli/upgrade_snapshots_test.go @@ -0,0 +1,168 @@ +package cli + +import ( + "bytes" + "os" + "path/filepath" + "testing" + + "github.com/dipto0321/nodeup/internal/packages" + "github.com/dipto0321/nodeup/internal/platform" +) + +// TestReclaimSnapshots_DeletesFiles verifies the reclamation helper +// removes the snapshot files it is handed. +// +// This is the tail of the snapshot lifecycle: after a successful +// migration, the snapshot has been replayed onto the new Node version +// and is dead weight. See #128. +func TestReclaimSnapshots_DeletesFiles(t *testing.T) { + redirectCleanupDataDir(t) + + paths := []string{ + writeCleanupSnapshot(t, "fnm-20.10.0.json", `{"manager":"fnm","node_version":"20.10.0","packages":[]}`), + writeCleanupSnapshot(t, "fnm-22.0.0.json", `{"manager":"fnm","node_version":"22.0.0","packages":[]}`), + } + + cmd := newCleanupCmd() // any command works — reclaimSnapshots only uses the writer + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + + reclaimSnapshots(cmd, paths) + + for _, p := range paths { + if _, err := os.Stat(p); !os.IsNotExist(err) { + t.Errorf("snapshot %s still exists after reclaim (stat err = %v)", p, err) + } + } +} + +// TestReclaimSnapshots_ReportsFailureWithoutStopping verifies that a +// file that can't be deleted doesn't abort the loop — one stale file +// must not prevent the others from being reclaimed. +func TestReclaimSnapshots_ReportsFailureWithoutStopping(t *testing.T) { + redirectCleanupDataDir(t) + + good := writeCleanupSnapshot(t, "fnm-22.0.0.json", `{"manager":"fnm","node_version":"22.0.0","packages":[]}`) + + // A path outside the snapshots directory — DeleteSnapshotFile + // refuses it, which is exactly the failure we want to exercise. + outside := filepath.Join(t.TempDir(), "not-ours.json") + if err := os.WriteFile(outside, []byte(`{}`), 0o644); err != nil { + t.Fatalf("write outside file: %v", err) + } + + cmd := newCleanupCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + + reclaimSnapshots(cmd, []string{outside, good}) + + // The good file was still reclaimed. + if _, err := os.Stat(good); !os.IsNotExist(err) { + t.Errorf("reclaimable snapshot %s still exists (stat err = %v)", good, err) + } + // The refused one is untouched. + if _, err := os.Stat(outside); err != nil { + t.Errorf("the outside file was touched: %v", err) + } + // And the failure was surfaced to the user. + if !bytes.Contains(buf.Bytes(), []byte("failed to delete snapshot")) { + t.Errorf("output %q missing the deletion-failure warning", buf.String()) + } +} + +// TestReclaimSnapshots_EmptyIsNoop verifies the no-snapshot case +// doesn't blow up. +func TestReclaimSnapshots_EmptyIsNoop(t *testing.T) { + redirectCleanupDataDir(t) + + cmd := newCleanupCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + + reclaimSnapshots(cmd, nil) + + if buf.Len() != 0 { + t.Errorf("output %q, want nothing", buf.String()) + } +} + +// TestUpgradeCmd_HasKeepSnapshotsFlag pins that the --keep-snapshots +// flag exists and defaults to false — the automatic deletion is the +// default behavior, and the flag is the opt-out. +func TestUpgradeCmd_HasKeepSnapshotsFlag(t *testing.T) { + cmd := newUpgradeCmd() + + flag := cmd.Flags().Lookup("keep-snapshots") + if flag == nil { + t.Fatal("upgrade command has no --keep-snapshots flag") + } + if flag.DefValue != "false" { + t.Errorf("--keep-snapshots default = %q, want false", flag.DefValue) + } +} + +// TestPackagesCmd_HasCleanupSubcommand pins that `nodeup packages +// cleanup` is wired into the command tree. +func TestPackagesCmd_HasCleanupSubcommand(t *testing.T) { + cmd := newPackagesCmd() + + for _, sub := range cmd.Commands() { + if sub.Name() == "cleanup" { + return + } + } + t.Error("`nodeup packages cleanup` is not registered") +} + +// TestUpgradeSnapshotLifecycle_SnapshotSurvivesFailedMigration is the +// most important invariant of the whole change: a snapshot whose +// contents were NOT replayed must survive. +// +// On the failure path the snapshot is the only record of what the +// user's globals were, and the interrupted-upgrade warning points +// straight at it. Deleting it would destroy the rollback payload. +func TestUpgradeSnapshotLifecycle_SnapshotSurvivesFailedMigration(t *testing.T) { + redirectCleanupDataDir(t) + + // Simulate the state after a failed migration: a snapshot on disk + // that nothing has consumed. The deferred cleanup's guard is + // `!snapshotsConsumed || keepSnapshots` — with snapshotsConsumed + // false, reclaimSnapshots is never reached. + path := writeCleanupSnapshot(t, "fnm-20.10.0.json", `{"manager":"fnm","node_version":"20.10.0","packages":[]}`) + + // The snapshot is still there — we never deleted it. + if _, err := os.Stat(path); err != nil { + t.Fatalf("snapshot vanished: %v", err) + } + files, err := packages.ListSnapshotFiles() + if err != nil { + t.Fatalf("ListSnapshotFiles: %v", err) + } + if len(files) != 1 { + t.Errorf("snapshot count = %d, want 1", len(files)) + } +} + +// TestSnapshotsDirIsSeparateFromReportsDir verifies the two cleanup +// targets are genuinely different directories — the cleanup command +// picks between them with the --reports flag. +func TestSnapshotsDirIsSeparateFromReportsDir(t *testing.T) { + redirectCleanupDataDir(t) + + snaps, err := platform.SnapshotsDir() + if err != nil { + t.Fatalf("SnapshotsDir: %v", err) + } + reports, err := platform.ReportsDir() + if err != nil { + t.Fatalf("ReportsDir: %v", err) + } + if snaps == reports { + t.Errorf("SnapshotsDir and ReportsDir are the same (%s) — the --reports flag would be a no-op", snaps) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 48b660b..953bacc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -72,6 +72,18 @@ type PackagesConfig struct { // faithful and lets `nodeup config set packages.skip ""` actually // clear the list. Skip []string `yaml:"skip"` + + // KeepSnapshots disables the automatic deletion of the snapshot + // files an upgrade writes. By default nodeup deletes them once + // the package migration has replayed them onto the new Node + // version — otherwise the snapshots directory grows by one file + // per (manager, Node version) pair forever, with nothing ever + // reclaiming the space. See #128. + // + // Set this to true if you want to keep the rollback payloads + // around for manual replays (`nodeup packages restore + // `). The CLI flag --keep-snapshots overrides it. + KeepSnapshots bool `yaml:"keep_snapshots"` } // CleanupConfig controls the prompt that asks whether to remove old @@ -179,6 +191,7 @@ func (c *Config) Validate() error { // packages.migrate bool // packages.strategy string (exact | latest) // packages.skip []string (CSV on input, JSON list on output) +// packages.keep_snapshots bool // cleanup.auto bool // cleanup.prompt bool // cache.ttl int (seconds) @@ -233,6 +246,13 @@ func (c *Config) SetByKey(key, raw string) (bool, error) { } c.Packages.Skip = out return true, nil + case "packages.keep_snapshots": + b, err := parseBool(raw) + if err != nil { + return true, fmt.Errorf("packages.keep_snapshots: %w", err) + } + c.Packages.KeepSnapshots = b + return true, nil case "cleanup.auto": b, err := parseBool(raw) if err != nil { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 4e631e6..83099f6 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -18,6 +18,9 @@ func TestDefault_MatchesSpec(t *testing.T) { Migrate: true, Strategy: StrategyExact, Skip: []string{"npm", "corepack", "npx"}, + // Snapshots are deleted automatically once the migration + // has replayed them — see #128. + KeepSnapshots: false, }, Cleanup: CleanupConfig{Auto: false, Prompt: true}, Cache: CacheConfig{TTL: 3600}, @@ -75,6 +78,9 @@ func TestSetByKey(t *testing.T) { {"packages.strategy", "bogus", nil, true, true}, {"packages.skip", "yarn,pnpm", func(c *Config) bool { return reflect.DeepEqual(c.Packages.Skip, []string{"yarn", "pnpm"}) }, false, true}, {"packages.skip", "", func(c *Config) bool { return c.Packages.Skip == nil }, false, true}, + {"packages.keep_snapshots", "true", func(c *Config) bool { return c.Packages.KeepSnapshots }, false, true}, + {"packages.keep_snapshots", "false", func(c *Config) bool { return !c.Packages.KeepSnapshots }, false, true}, + {"packages.keep_snapshots", "yelp", nil, true, true}, {"cleanup.auto", "true", func(c *Config) bool { return c.Cleanup.Auto == true }, false, true}, {"cleanup.prompt", "no", func(c *Config) bool { return c.Cleanup.Prompt == false }, false, true}, {"cache.ttl", "7200", func(c *Config) bool { return c.Cache.TTL == 7200 }, false, true}, diff --git a/internal/config/env.go b/internal/config/env.go index b9b6d0b..88ec8f5 100644 --- a/internal/config/env.go +++ b/internal/config/env.go @@ -14,7 +14,10 @@ const ( EnvTrackCurrent = "NODEUP_TRACK_CURRENT" EnvPackagesMigrate = "NODEUP_PACKAGES_MIGRATE" EnvPackagesStrategy = "NODEUP_PACKAGES_STRATEGY" - EnvCacheTTL = "NODEUP_CACHE_TTL" + // EnvPackagesKeepSnapshots disables the automatic deletion of the + // snapshot files an upgrade writes. See #128. + EnvPackagesKeepSnapshots = "NODEUP_PACKAGES_KEEP_SNAPSHOTS" + EnvCacheTTL = "NODEUP_CACHE_TTL" // EnvConfig lets power users point nodeup at a different config // file path entirely (e.g. for tests, or for users who keep their @@ -85,6 +88,13 @@ func FromEnv() (*Overlay, error) { } out.SetPackagesStrategy(s) } + if v, ok := os.LookupEnv(EnvPackagesKeepSnapshots); ok && v != "" { + b, err := parseBool(v) + if err != nil { + return nil, errEnv(EnvPackagesKeepSnapshots, err) + } + out.SetPackagesKeepSnapshots(b) + } if v, ok := os.LookupEnv(EnvCacheTTL); ok && v != "" { n, err := strconv.Atoi(v) if err != nil { diff --git a/internal/config/fileoverlay.go b/internal/config/fileoverlay.go index 5b1611b..58841c3 100644 --- a/internal/config/fileoverlay.go +++ b/internal/config/fileoverlay.go @@ -43,7 +43,8 @@ func FileOverlayFromNode(cfg *Config, root *yaml.Node) *Overlay { applyTrackOverlay(o.C, valNode, &o.TrackLTSSet, &o.TrackCurrentSet) case "packages": applyPackagesOverlay(o.C, valNode, - &o.PackagesMigrateSet, &o.PackagesStrategySet, &o.PackagesSkipSet) + &o.PackagesMigrateSet, &o.PackagesStrategySet, &o.PackagesSkipSet, + &o.PackagesKeepSnapshotsSet) case "cleanup": applyCleanupOverlay(o.C, valNode, &o.CleanupAutoSet, &o.CleanupPromptSet) case "cache": @@ -72,7 +73,7 @@ func applyTrackOverlay(cfg *Config, node *yaml.Node, ltsSet, currentSet *bool) { } // applyPackagesOverlay scans a packages-mapping node. -func applyPackagesOverlay(cfg *Config, node *yaml.Node, migrateSet, strategySet, skipSet *bool) { +func applyPackagesOverlay(cfg *Config, node *yaml.Node, migrateSet, strategySet, skipSet, keepSnapshotsSet *bool) { if node == nil || node.Kind != yaml.MappingNode { return } @@ -87,6 +88,9 @@ func applyPackagesOverlay(cfg *Config, node *yaml.Node, migrateSet, strategySet, case "skip": *skipSet = true cfg.Packages.Skip = decodeStringList(node.Content[i+1]) + case "keep_snapshots": + *keepSnapshotsSet = true + cfg.Packages.KeepSnapshots = decodeBool(node.Content[i+1]) } } } diff --git a/internal/config/load_test.go b/internal/config/load_test.go index 60c72f6..1643753 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -65,6 +65,11 @@ track: if !cfg.Packages.Migrate { t.Errorf("Packages.Migrate = false, want true (default)") } + // packages.keep_snapshots omitted -> the default (false, i.e. + // snapshots are deleted after a successful migration) must survive. + if cfg.Packages.KeepSnapshots { + t.Errorf("Packages.KeepSnapshots = true, want false (default, file omitted)") + } if cfg.Cache.TTL != 3600 { t.Errorf("Cache.TTL = %d, want 3600 (default)", cfg.Cache.TTL) } @@ -83,6 +88,7 @@ packages: migrate: false strategy: latest skip: [yarn, pnpm] + keep_snapshots: true cleanup: auto: true prompt: false @@ -108,6 +114,9 @@ cache: if strings.Join(cfg.Packages.Skip, ",") != "yarn,pnpm" { t.Errorf("Packages.Skip = %v", cfg.Packages.Skip) } + if !cfg.Packages.KeepSnapshots { + t.Errorf("Packages.KeepSnapshots = false, want true (file set it)") + } if !cfg.Cleanup.Auto || cfg.Cleanup.Prompt { t.Errorf("cleanup: auto=%v prompt=%v", cfg.Cleanup.Auto, cfg.Cleanup.Prompt) } diff --git a/internal/config/resolve.go b/internal/config/resolve.go index 3c2bf92..1788cf6 100644 --- a/internal/config/resolve.go +++ b/internal/config/resolve.go @@ -52,6 +52,9 @@ func applyOverlay(dst *Config, src *Overlay) { if src.PackagesSkipSet { dst.Packages.Skip = src.C.Packages.Skip } + if src.PackagesKeepSnapshotsSet { + dst.Packages.KeepSnapshots = src.C.Packages.KeepSnapshots + } if src.CleanupAutoSet { dst.Cleanup.Auto = src.C.Cleanup.Auto } @@ -84,9 +87,12 @@ type Overlay struct { PackagesMigrateSet bool PackagesStrategySet bool PackagesSkipSet bool - CleanupAutoSet bool - CleanupPromptSet bool - CacheTTLSet bool + // PackagesKeepSnapshotsSet is true iff packages.keep_snapshots + // was explicitly provided in this layer. + PackagesKeepSnapshotsSet bool + CleanupAutoSet bool + CleanupPromptSet bool + CacheTTLSet bool } // NewOverlay returns an empty Overlay (no fields set). @@ -147,6 +153,16 @@ func (o *Overlay) SetPackagesSkip(skip []string) { o.PackagesSkipSet = true } +// SetPackagesKeepSnapshots records an explicit packages.keep_snapshots +// override. +func (o *Overlay) SetPackagesKeepSnapshots(v bool) { + if o == nil || o.C == nil { + return + } + o.C.Packages.KeepSnapshots = v + o.PackagesKeepSnapshotsSet = true +} + // SetCleanupAuto records an explicit cleanup.auto override. func (o *Overlay) SetCleanupAuto(v bool) { if o == nil || o.C == nil { diff --git a/internal/detector/interactive_test.go b/internal/detector/interactive_test.go index a0eec6b..3ba815e 100644 --- a/internal/detector/interactive_test.go +++ b/internal/detector/interactive_test.go @@ -58,6 +58,9 @@ func (s *stubPrompt) Select(_ string, options []string, _ string) (string, error } return "", fmt.Errorf("stubPrompt: answer %q not in options %v", got, options) } +func (s *stubPrompt) MultiSelect(string, []string) ([]string, error) { + return nil, errors.New("MultiSelect not expected in ResolveInteractive tests") +} // --- ResolveInteractive ------------------------------------------------- diff --git a/internal/packages/cleanup.go b/internal/packages/cleanup.go new file mode 100644 index 0000000..c73314b --- /dev/null +++ b/internal/packages/cleanup.go @@ -0,0 +1,377 @@ +package packages + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "sort" + "strings" + "time" + + "github.com/dipto0321/nodeup/internal/platform" +) + +// SnapshotFile describes one snapshot file on disk. +// +// Unlike SnapshotData (the parsed JSON body), SnapshotFile describes the +// FILE — which is what a cleanup command needs to act on. The two are +// deliberately separate types: a snapshot file can be unparseable (a +// crash mid-write left a truncated JSON body) and we still need to be +// able to list and delete it. ListSnapshots() silently skips such files, +// so it cannot drive a deletion flow. +type SnapshotFile struct { + // Path is the absolute path of the snapshot file. + Path string + + // Name is the file's basename, e.g. "fnm-20.10.0.json". + Name string + + // Manager is the manager name parsed out of the filename, e.g. "fnm". + // Empty when the filename doesn't follow the -.json + // convention. + Manager string + + // Version is the Node version parsed out of the filename, e.g. "20.10.0". + // Empty when the filename doesn't follow the convention. + Version string + + // Size is the file size in bytes. + Size int64 + + // ModTime is the file's last-modification time. + ModTime time.Time + + // ParseError is non-empty when the file's JSON body could not be + // parsed. The file is still listed (and can still be deleted) — we + // just can't tell the user what's inside it. + ParseError string +} + +// ListSnapshotFiles returns every snapshot file present in the snapshots +// directory, including files whose JSON body is corrupt. +// +// This is the enumeration a cleanup command needs, and it is deliberately +// NOT built on top of ListSnapshots(): +// +// - ListSnapshots() parses each file and skips the ones that fail to +// parse. A snapshot truncated by a crash mid-write is therefore +// invisible to it — and a cleanup command that can't see a file can't +// delete it. That's exactly the class of file we're trying to +// reclaim. +// - ListSnapshots() returns parsed SnapshotData, which loses the +// file's size and modification time — the two things a user wants +// to see before deciding what to delete. +// +// Results are sorted by modification time (oldest first) so the most +// stale files — the ones most likely to be dead weight — appear first. +// +// The directory is created if it doesn't exist (platform.SnapshotsDir +// does an MkdirAll), so a first run on a clean machine returns an empty +// slice rather than an error. +func ListSnapshotFiles() ([]SnapshotFile, error) { + dir, err := platform.SnapshotsDir() + if err != nil { + return nil, err + } + + entries, err := os.ReadDir(dir) + if err != nil { + return nil, err + } + + out := make([]SnapshotFile, 0, len(entries)) + for _, entry := range entries { + if entry.IsDir() { + continue + } + name := entry.Name() + // Accept ".json" (a real snapshot) and ".json.tmp" (the + // half-written sibling an interrupted atomic write leaves + // behind). The .tmp files are otherwise invisible — nothing + // else in nodeup ever looks at them — so without this they + // would accumulate unnoticed. + isTmp := strings.HasSuffix(name, ".json.tmp") + if !strings.HasSuffix(name, ".json") && !isTmp { + continue + } + + path := filepath.Join(dir, name) + info, err := entry.Info() + if err != nil { + // The file vanished between ReadDir and Info (another + // process deleted it, or it's a broken symlink). Skip it + // rather than failing the whole listing. + continue + } + + sf := SnapshotFile{ + Path: path, + Name: name, + Size: info.Size(), + ModTime: info.ModTime(), + } + sf.Manager, sf.Version = parseSnapshotFilename(name) + + // Try to parse the body so we can tell the user what's inside. + // A parse failure is recorded, not fatal — the file is still + // listed and can still be deleted. + data, err := os.ReadFile(path) + if err != nil { + sf.ParseError = err.Error() + } else if s := parseSnapshotBody(data); s != nil { + if sf.Manager == "" { + sf.Manager = s.Manager + } + if sf.Version == "" { + sf.Version = s.NodeVersion + } + } else { + sf.ParseError = "snapshot file is not valid JSON" + } + + out = append(out, sf) + } + + // Oldest first — the files most likely to be stale come first. + sort.SliceStable(out, func(i, j int) bool { + if out[i].ModTime.Equal(out[j].ModTime) { + return out[i].Name < out[j].Name + } + return out[i].ModTime.Before(out[j].ModTime) + }) + + return out, nil +} + +// parseSnapshotBody unmarshals snapshot JSON, returning nil when the +// body isn't a valid snapshot. +func parseSnapshotBody(data []byte) *SnapshotData { + var s SnapshotData + if err := json.Unmarshal(data, &s); err != nil { + return nil + } + return &s +} + +// parseSnapshotFilename splits a snapshot filename into its manager and +// version components. +// +// The convention is "-.json" (see snapshotPath), e.g. +// "fnm-20.10.0.json" → ("fnm", "20.10.0"). Manager names may themselves +// contain hyphens ("vite-plus", "nvm-windows"), so we split on the LAST +// hyphen — the version never contains one, but the manager might. +// +// A ".tmp" suffix (the sibling left behind by an interrupted atomic +// write) is stripped first, so "fnm-20.10.0.json.tmp" parses the same +// way as its completed counterpart. +// +// Returns empty strings when the filename doesn't follow the convention. +func parseSnapshotFilename(name string) (manager, version string) { + base := strings.TrimSuffix(name, ".tmp") + base = strings.TrimSuffix(base, ".json") + idx := strings.LastIndex(base, "-") + if idx <= 0 || idx == len(base)-1 { + return "", "" + } + return base[:idx], base[idx+1:] +} + +// DeleteSnapshotFile removes a single snapshot file from disk. +// +// It refuses to delete anything that isn't a regular file inside the +// snapshots directory — a symlink to somewhere else, or a path that +// escaped the directory via "..", is rejected rather than followed. +// This matters because the path can come from user input (the CLI +// accepts snapshot names to delete) and we don't want a malformed +// argument to turn into an arbitrary-file-unlink. +// +// A file that is already gone is not an error — the desired end state +// (the file doesn't exist) is already true. +func DeleteSnapshotFile(path string) error { + dir, err := platform.SnapshotsDir() + if err != nil { + return err + } + + abs, err := filepath.Abs(path) + if err != nil { + return err + } + + // Resolve symlinks on both sides so a symlink pointing outside the + // snapshots dir is caught. EvalSymlinks fails on a non-existent + // file, which we handle below (already-gone is not an error). + realDir, err := filepath.EvalSymlinks(dir) + if err != nil { + realDir = dir + } + realPath, err := filepath.EvalSymlinks(abs) + if err != nil { + // The file may simply not exist. Check that first before + // treating this as a hard error. + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("resolve snapshot path: %w", err) + } + + if !isInsideDir(realDir, realPath) { + return fmt.Errorf("refusing to delete %s: not inside the snapshots directory", path) + } + + info, err := os.Lstat(realPath) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("stat snapshot: %w", err) + } + if !info.Mode().IsRegular() { + return fmt.Errorf("refusing to delete %s: not a regular file", path) + } + + if err := os.Remove(realPath); err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("delete snapshot: %w", err) + } + return nil +} + +// DeleteSnapshotFiles removes several snapshot files, returning the +// names that were deleted and the failures that occurred. +// +// A failure on one file does not abort the rest — the caller gets a +// complete picture of what happened and can report it. This mirrors +// the cleanup loop in the upgrade command, which also collects failures +// instead of stopping at the first one. +func DeleteSnapshotFiles(paths []string) (deleted []string, failures map[string]error) { + failures = map[string]error{} + for _, p := range paths { + if err := DeleteSnapshotFile(p); err != nil { + failures[p] = err + continue + } + deleted = append(deleted, p) + } + return deleted, failures +} + +// isInsideDir reports whether path resolves to a location strictly +// inside dir (or equal to it). +// +// This is the same guard the tar extraction path uses (see #65): a +// naive strings.HasPrefix check would treat "/tmp/ab" as being inside +// "/tmp/a". path.relative handles that correctly. +func isInsideDir(dir, path string) bool { + rel, err := filepath.Rel(dir, path) + if err != nil { + return false + } + if rel == "." { + return true + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) && !filepath.IsAbs(rel) +} + +// ListReportFiles returns every migration report file present in the +// reports directory. +// +// Migration reports accumulate one-per-migration with a random-suffixed +// filename, so they are never overwritten and never reused. Like +// snapshots, they can leak; the cleanup command surfaces them alongside +// the snapshots so the user can reclaim that space too. +func ListReportFiles() ([]SnapshotFile, error) { + dir, err := platform.ReportsDir() + if err != nil { + return nil, err + } + + entries, err := os.ReadDir(dir) + if err != nil { + return nil, err + } + + out := make([]SnapshotFile, 0, len(entries)) + for _, entry := range entries { + if entry.IsDir() { + continue + } + name := entry.Name() + if !strings.HasSuffix(name, ".json") { + continue + } + info, err := entry.Info() + if err != nil { + continue + } + out = append(out, SnapshotFile{ + Path: filepath.Join(dir, name), + Name: name, + Size: info.Size(), + ModTime: info.ModTime(), + }) + } + + sort.SliceStable(out, func(i, j int) bool { + if out[i].ModTime.Equal(out[j].ModTime) { + return out[i].Name < out[j].Name + } + return out[i].ModTime.Before(out[j].ModTime) + }) + + return out, nil +} + +// DeleteReportFile removes a single migration report file. +// +// Mirrors DeleteSnapshotFile's safety rules: the target must be a +// regular file inside the reports directory. +func DeleteReportFile(path string) error { + dir, err := platform.ReportsDir() + if err != nil { + return err + } + + abs, err := filepath.Abs(path) + if err != nil { + return err + } + + realDir, err := filepath.EvalSymlinks(dir) + if err != nil { + realDir = dir + } + realPath, err := filepath.EvalSymlinks(abs) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("resolve report path: %w", err) + } + + if !isInsideDir(realDir, realPath) { + return fmt.Errorf("refusing to delete %s: not inside the reports directory", path) + } + + info, err := os.Lstat(realPath) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("stat report: %w", err) + } + if !info.Mode().IsRegular() { + return fmt.Errorf("refusing to delete %s: not a regular file", path) + } + + if err := os.Remove(realPath); err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("delete report: %w", err) + } + return nil +} diff --git a/internal/packages/cleanup_test.go b/internal/packages/cleanup_test.go new file mode 100644 index 0000000..5704692 --- /dev/null +++ b/internal/packages/cleanup_test.go @@ -0,0 +1,422 @@ +package packages + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/dipto0321/nodeup/internal/platform" +) + +// writeSnapshotFile drops a valid snapshot file into the snapshots +// directory under the given name and returns its full path. +func writeSnapshotFile(t *testing.T, name string, body any) string { + t.Helper() + dir, err := platform.SnapshotsDir() + if err != nil { + t.Fatalf("SnapshotsDir: %v", err) + } + path := filepath.Join(dir, name) + var data []byte + switch v := body.(type) { + case string: + data = []byte(v) + default: + var err error + data, err = json.MarshalIndent(v, "", " ") + if err != nil { + t.Fatalf("marshal body: %v", err) + } + } + if err := os.WriteFile(path, data, 0o644); err != nil { + t.Fatalf("write %s: %v", path, err) + } + return path +} + +// TestListSnapshotFiles_ListsCorruptFiles verifies the cleanup +// listing surfaces snapshots whose JSON body is unparseable. +// +// This is the whole reason ListSnapshotFiles exists as a separate API +// from ListSnapshots: a snapshot truncated by a crash mid-write is +// skipped by ListSnapshots (it doesn't parse) and therefore invisible +// to every code path — yet it still occupies disk. A cleanup command +// that can't see the file can't delete it. See #128. +func TestListSnapshotFiles_ListsCorruptFiles(t *testing.T) { + redirectDataDir(t) + + writeSnapshotFile(t, "fnm-20.10.0.json", SnapshotData{ + Manager: "fnm", + NodeVersion: "20.10.0", + Packages: []Package{{Name: "typescript", Version: "5.4.0"}}, + }) + writeSnapshotFile(t, "fnm-18.0.0.json", "{ this is not json") + + files, err := ListSnapshotFiles() + if err != nil { + t.Fatalf("ListSnapshotFiles: %v", err) + } + if len(files) != 2 { + t.Fatalf("ListSnapshotFiles returned %d files, want 2 (the corrupt one must still be listed)", len(files)) + } + + byName := map[string]SnapshotFile{} + for _, f := range files { + byName[f.Name] = f + } + + good, ok := byName["fnm-20.10.0.json"] + if !ok { + t.Fatal("valid snapshot missing from listing") + } + if good.ParseError != "" { + t.Errorf("valid snapshot has ParseError %q, want empty", good.ParseError) + } + if good.Manager != "fnm" || good.Version != "20.10.0" { + t.Errorf("valid snapshot manager/version = %q/%q, want fnm/20.10.0", good.Manager, good.Version) + } + if good.Size == 0 { + t.Error("valid snapshot Size = 0, want the actual byte count") + } + + bad, ok := byName["fnm-18.0.0.json"] + if !ok { + t.Fatal("corrupt snapshot missing from listing") + } + if bad.ParseError == "" { + t.Error("corrupt snapshot has empty ParseError, want a description") + } +} + +// TestListSnapshotFiles_SkipsNonJSON verifies that files which are not +// snapshots at all (or directories) are ignored. +func TestListSnapshotFiles_SkipsNonJSON(t *testing.T) { + redirectDataDir(t) + + dir, err := platform.SnapshotsDir() + if err != nil { + t.Fatalf("SnapshotsDir: %v", err) + } + writeSnapshotFile(t, "fnm-20.10.0.json", SnapshotData{Manager: "fnm", NodeVersion: "20.10.0"}) + if err := os.WriteFile(filepath.Join(dir, "notes.txt"), []byte("hello"), 0o644); err != nil { + t.Fatalf("write notes.txt: %v", err) + } + if err := os.MkdirAll(filepath.Join(dir, "subdir.json"), 0o755); err != nil { + t.Fatalf("mkdir subdir.json: %v", err) + } + + files, err := ListSnapshotFiles() + if err != nil { + t.Fatalf("ListSnapshotFiles: %v", err) + } + if len(files) != 1 { + names := make([]string, len(files)) + for i, f := range files { + names[i] = f.Name + } + t.Fatalf("ListSnapshotFiles returned %v, want exactly [fnm-20.10.0.json]", names) + } +} + +// TestListSnapshotFiles_ListsLeftoverTmpFiles verifies the half-written +// ".json.tmp" siblings an interrupted atomic write leaves behind are +// listed (and therefore deletable). Nothing else in nodeup looks at +// them, so without this they'd accumulate unnoticed. +func TestListSnapshotFiles_ListsLeftoverTmpFiles(t *testing.T) { + redirectDataDir(t) + + writeSnapshotFile(t, "fnm-20.10.0.json.tmp", `{"manager":"fnm"`) + + files, err := ListSnapshotFiles() + if err != nil { + t.Fatalf("ListSnapshotFiles: %v", err) + } + if len(files) != 1 { + t.Fatalf("ListSnapshotFiles returned %d files, want 1", len(files)) + } + if files[0].Name != "fnm-20.10.0.json.tmp" { + t.Errorf("listed %q, want fnm-20.10.0.json.tmp", files[0].Name) + } + // The manager/version are still recoverable from the filename even + // though the body is truncated. + if files[0].Manager != "fnm" || files[0].Version != "20.10.0" { + t.Errorf("manager/version = %q/%q, want fnm/20.10.0", files[0].Manager, files[0].Version) + } +} + +// TestDeleteSnapshotFile_RemovesFile verifies the happy path. +func TestDeleteSnapshotFile_RemovesFile(t *testing.T) { + redirectDataDir(t) + + path := writeSnapshotFile(t, "fnm-20.10.0.json", SnapshotData{Manager: "fnm", NodeVersion: "20.10.0"}) + + if err := DeleteSnapshotFile(path); err != nil { + t.Fatalf("DeleteSnapshotFile: %v", err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("snapshot still exists after delete (stat err = %v)", err) + } +} + +// TestDeleteSnapshotFile_MissingIsSuccess verifies that deleting a file +// that is already gone is not an error — the desired end state is +// already true. +func TestDeleteSnapshotFile_MissingIsSuccess(t *testing.T) { + redirectDataDir(t) + + dir, err := platform.SnapshotsDir() + if err != nil { + t.Fatalf("SnapshotsDir: %v", err) + } + missing := filepath.Join(dir, "fnm-99.99.99.json") + + if err := DeleteSnapshotFile(missing); err != nil { + t.Errorf("DeleteSnapshotFile(missing) = %v, want nil (already gone is success)", err) + } +} + +// TestDeleteSnapshotFile_RefusesOutsideDir is the security guard: a +// path outside the snapshots directory must be rejected, not deleted. +// +// The path can come from user input (the cleanup CLI feeds it back +// from a listing, but the API is public), so we refuse anything that +// doesn't resolve inside the snapshots directory. See #51. +func TestDeleteSnapshotFile_RefusesOutsideDir(t *testing.T) { + redirectDataDir(t) + + outside := filepath.Join(t.TempDir(), "important.json") + if err := os.WriteFile(outside, []byte(`{"keep":"me"}`), 0o644); err != nil { + t.Fatalf("write outside file: %v", err) + } + + err := DeleteSnapshotFile(outside) + if err == nil { + t.Fatal("DeleteSnapshotFile(outside) = nil, want a refusal") + } + if !strings.Contains(err.Error(), "not inside the snapshots directory") { + t.Errorf("err = %v, want a 'not inside the snapshots directory' refusal", err) + } + if _, statErr := os.Stat(outside); statErr != nil { + t.Errorf("the outside file was deleted anyway: %v", statErr) + } +} + +// TestDeleteSnapshotFile_RefusesTraversal is the same guard expressed +// the way a user would actually hit it: a ".." path that escapes the +// snapshots directory. +func TestDeleteSnapshotFile_RefusesTraversal(t *testing.T) { + redirectDataDir(t) + + dir, err := platform.SnapshotsDir() + if err != nil { + t.Fatalf("SnapshotsDir: %v", err) + } + // /evil.json sits next to /snapshots — a traversal from + // the snapshots dir reaches it. + evil := filepath.Join(filepath.Dir(dir), "evil.json") + if err := os.WriteFile(evil, []byte(`{}`), 0o644); err != nil { + t.Fatalf("write evil.json: %v", err) + } + + traversal := filepath.Join(dir, "..", "evil.json") + if err := DeleteSnapshotFile(traversal); err == nil { + t.Fatal("DeleteSnapshotFile(traversal) = nil, want a refusal") + } + if _, statErr := os.Stat(evil); statErr != nil { + t.Errorf("the traversal target was deleted anyway: %v", statErr) + } +} + +// TestDeleteSnapshotFile_RefusesSymlink verifies that a symlink +// pointing outside the snapshots directory is rejected rather than +// followed. The "inside the dir" check runs on the *resolved* path, so +// the symlink's target is what gets checked. +func TestDeleteSnapshotFile_RefusesSymlink(t *testing.T) { + redirectDataDir(t) + + outside := filepath.Join(t.TempDir(), "target.json") + if err := os.WriteFile(outside, []byte(`{}`), 0o644); err != nil { + t.Fatalf("write target: %v", err) + } + + dir, err := platform.SnapshotsDir() + if err != nil { + t.Fatalf("SnapshotsDir: %v", err) + } + link := filepath.Join(dir, "fnm-20.10.0.json") + if err := os.Symlink(outside, link); err != nil { + t.Skipf("symlinks unavailable on this platform: %v", err) + } + + if err := DeleteSnapshotFile(link); err == nil { + t.Fatal("DeleteSnapshotFile(symlink-to-outside) = nil, want a refusal") + } + if _, statErr := os.Stat(outside); statErr != nil { + t.Errorf("the symlink target was deleted anyway: %v", statErr) + } +} + +// TestIsInsideDir covers the boundary cases of the directory guard. +// +// The naive strings.HasPrefix check would consider "/tmp/ab" to be +// inside "/tmp/a" — filepath.Rel handles that correctly. Same guard the +// tar-extraction path uses (see #65). +func TestIsInsideDir(t *testing.T) { + tests := []struct { + dir, path string + want bool + }{ + {"/tmp/a", "/tmp/a/b.json", true}, + {"/tmp/a", "/tmp/a", true}, + {"/tmp/a", "/tmp/a/b/c.json", true}, + {"/tmp/a", "/tmp/ab.json", false}, + {"/tmp/a", "/tmp/b.json", false}, + {"/tmp/a", "/tmp/a/../b.json", false}, + {"/tmp/a", "/etc/passwd", false}, + } + for _, tc := range tests { + got := isInsideDir(tc.dir, tc.path) + if got != tc.want { + t.Errorf("isInsideDir(%q, %q) = %v, want %v", tc.dir, tc.path, got, tc.want) + } + } +} + +// TestParseSnapshotFilename covers the naming convention, including +// manager names that themselves contain a hyphen. +func TestParseSnapshotFilename(t *testing.T) { + tests := []struct { + name string + wantMgr string + wantVersion string + }{ + {"fnm-20.10.0.json", "fnm", "20.10.0"}, + {"vite-plus-20.10.0.json", "vite-plus", "20.10.0"}, + {"nvm-windows-20.10.0.json", "nvm-windows", "20.10.0"}, + {"fnm-20.10.0.json.tmp", "fnm", "20.10.0"}, + {"noverhyphen.json", "", ""}, + {"-20.10.0.json", "", ""}, + {"fnm-.json", "", ""}, + } + for _, tc := range tests { + mgr, ver := parseSnapshotFilename(tc.name) + if mgr != tc.wantMgr || ver != tc.wantVersion { + t.Errorf("parseSnapshotFilename(%q) = %q/%q, want %q/%q", tc.name, mgr, ver, tc.wantMgr, tc.wantVersion) + } + } +} + +// TestListReportFiles verifies the reports listing enumerates the +// migration-report files (which leak in the same way the snapshots do). +func TestListReportFiles(t *testing.T) { + redirectDataDir(t) + + dir, err := platform.ReportsDir() + if err != nil { + t.Fatalf("ReportsDir: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "migration-20260101-abcdef.json"), []byte(`{}`), 0o644); err != nil { + t.Fatalf("write report: %v", err) + } + + files, err := ListReportFiles() + if err != nil { + t.Fatalf("ListReportFiles: %v", err) + } + if len(files) != 1 { + t.Fatalf("ListReportFiles returned %d files, want 1", len(files)) + } + if files[0].Name != "migration-20260101-abcdef.json" { + t.Errorf("listed %q, want migration-20260101-abcdef.json", files[0].Name) + } +} + +// TestDeleteReportFile verifies the report deletion happy path and +// the already-gone case. +func TestDeleteReportFile(t *testing.T) { + redirectDataDir(t) + + dir, err := platform.ReportsDir() + if err != nil { + t.Fatalf("ReportsDir: %v", err) + } + path := filepath.Join(dir, "migration-20260101-abcdef.json") + if err := os.WriteFile(path, []byte(`{}`), 0o644); err != nil { + t.Fatalf("write report: %v", err) + } + + if err := DeleteReportFile(path); err != nil { + t.Fatalf("DeleteReportFile: %v", err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("report still exists after delete (stat err = %v)", err) + } + if err := DeleteReportFile(path); err != nil { + t.Errorf("DeleteReportFile(missing) = %v, want nil", err) + } +} + +// TestDeleteReportFile_RefusesOutsideDir is the same guard as for +// snapshots, applied to the reports directory. +func TestDeleteReportFile_RefusesOutsideDir(t *testing.T) { + redirectDataDir(t) + + outside := filepath.Join(t.TempDir(), "important.json") + if err := os.WriteFile(outside, []byte(`{}`), 0o644); err != nil { + t.Fatalf("write outside file: %v", err) + } + + if err := DeleteReportFile(outside); err == nil { + t.Fatal("DeleteReportFile(outside) = nil, want a refusal") + } + if _, statErr := os.Stat(outside); statErr != nil { + t.Errorf("the outside file was deleted anyway: %v", statErr) + } +} + +// TestSaveSnapshot_IsAtomic verifies the snapshot write leaves no +// temporary sibling behind and produces a complete, parseable file. +// +// The atomicity itself (tmp + rename) is what guarantees a reader +// never sees a truncated body — see the saveSnapshot doc comment. +func TestSaveSnapshot_IsAtomic(t *testing.T) { + redirectDataDir(t) + + snap := SnapshotData{ + Manager: "fnm", + NodeVersion: "20.10.0", + Packages: []Package{{Name: "typescript", Version: "5.4.0"}}, + } + if err := saveSnapshot(snap); err != nil { + t.Fatalf("saveSnapshot: %v", err) + } + + dir, err := platform.SnapshotsDir() + if err != nil { + t.Fatalf("SnapshotsDir: %v", err) + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + if len(entries) != 1 { + names := make([]string, len(entries)) + for i, e := range entries { + names[i] = e.Name() + } + t.Fatalf("snapshots dir contains %v, want exactly the one snapshot", names) + } + if entries[0].Name() != "fnm-20.10.0.json" { + t.Errorf("snapshot file = %q, want fnm-20.10.0.json", entries[0].Name()) + } + + got, err := LoadSnapshot("fnm", "20.10.0") + if err != nil { + t.Fatalf("LoadSnapshot: %v", err) + } + if len(got.Packages) != 1 || got.Packages[0].Name != "typescript" { + t.Errorf("round-tripped packages = %+v, want one typescript entry", got.Packages) + } +} diff --git a/internal/packages/snapshot.go b/internal/packages/snapshot.go index f539ba3..0d2d783 100644 --- a/internal/packages/snapshot.go +++ b/internal/packages/snapshot.go @@ -114,6 +114,22 @@ func snapshotPath(managerName, version string) (string, error) { return filepath.Join(dir, filename), nil } +// saveSnapshot writes the snapshot to its conventional path. +// +// The write is ATOMIC: we marshal to a sibling temp file and rename it +// into place. A plain os.WriteFile truncates the destination first, so a +// crash (or a full disk) mid-write leaves a truncated JSON file behind. +// +// That matters more than it looks. ListSnapshots() skips files that +// don't parse, so a truncated snapshot is INVISIBLE to the restore +// lookup, to `nodeup packages list`, and to the sentinel's replay path +// — but the file still occupies disk and still shows up in the +// file-based cleanup listing. The result is an orphan that no code path +// can explain or remove. +// +// Rename within a directory is atomic on POSIX; on Windows os.Rename +// replaces the destination. Either way a reader sees either the old +// file or the complete new one, never a partial body. See #128. func saveSnapshot(s SnapshotData) error { path, err := snapshotPath(s.Manager, s.NodeVersion) if err != nil { @@ -125,7 +141,17 @@ func saveSnapshot(s SnapshotData) error { return fmt.Errorf("marshal snapshot: %w", err) } - return os.WriteFile(path, data, 0o644) + tmp := path + ".tmp" + if err := os.WriteFile(tmp, data, 0o644); err != nil { + return fmt.Errorf("write snapshot tempfile: %w", err) + } + if err := os.Rename(tmp, path); err != nil { + // Best-effort cleanup — a leftover .tmp file is harmless, but + // we don't want to leak one per interrupted snapshot. + _ = os.Remove(tmp) + return fmt.Errorf("rename snapshot into place: %w", err) + } + return nil } // RestoreOutcome bundles the parsed snapshot metadata with the diff --git a/internal/ui/prompt.go b/internal/ui/prompt.go index 6139ce9..8e002f1 100644 --- a/internal/ui/prompt.go +++ b/internal/ui/prompt.go @@ -4,6 +4,9 @@ import ( "errors" "fmt" "io" + "sort" + "strconv" + "strings" "github.com/charmbracelet/huh" ) @@ -41,6 +44,16 @@ type Prompt interface { // option" or "skip the operation"). Select(prompt string, options []string, defaultLabel string) (string, error) + // MultiSelect asks the user to pick zero or more options from a + // list. Returns the labels of the selected options, in the order + // they appear in `options` (not the order the user picked them). + // + // EOF / an empty answer returns an empty slice and no error — the + // safe direction for a "which files should I delete?" question, + // where "nothing selected" means "delete nothing" rather than + // "delete everything". + MultiSelect(prompt string, options []string) ([]string, error) + // Mode returns the prompt's render mode. Mode() Mode } @@ -215,6 +228,92 @@ func (p *plainPrompt) Select(prompt string, options []string, defaultLabel strin return options[0], nil } +// MultiSelect asks the user to pick zero or more options. +// +// PlainMode has no cursor to move around, so the interaction is +// line-based: the user types a comma-separated list of option numbers +// (or labels) and presses Enter. An empty line selects nothing. +// +// Examples of accepted input: +// +// 1,3 — pick the 1st and 3rd options +// 1 3 — same (spaces are separators too) +// fn m-20 — pick by label +// all — pick every option +// (nothing) — pick nothing +func (p *plainPrompt) MultiSelect(prompt string, options []string) ([]string, error) { + if len(options) == 0 { + return nil, errors.New("ui: MultiSelect called with no options") + } + if p.out != nil { + _, _ = fmt.Fprintf(p.out, "%s\n", prompt) + for i, o := range options { + _, _ = fmt.Fprintf(p.out, " %d) %s\n", i+1, o) + } + _, _ = fmt.Fprintf(p.out, "Enter numbers or names separated by commas (blank = none): ") + } + + line, ok, err := p.readLine() + if err != nil { + return nil, err + } + if !ok { + // EOF — nothing selected. + return nil, nil + } + + line = strings.TrimSpace(line) + if line == "" { + return nil, nil + } + if normalizeAffirmative(line) == "all" { + return append([]string(nil), options...), nil + } + + // Index by label for the "pick by name" path. + labelIndex := make(map[string]int, len(options)) + for i, o := range options { + labelIndex[normalizeAffirmative(o)] = i + } + + seen := make(map[int]bool) + selected := make([]int, 0, len(options)) + + for _, raw := range strings.FieldsFunc(line, func(r rune) bool { + return r == ',' || r == ' ' || r == '\t' + }) { + raw = strings.TrimSpace(raw) + if raw == "" { + continue + } + if idx, perr := strconv.Atoi(raw); perr == nil { + if idx >= 1 && idx <= len(options) { + i := idx - 1 + if !seen[i] { + seen[i] = true + selected = append(selected, i) + } + } + continue + } + if i, ok := labelIndex[normalizeAffirmative(raw)]; ok { + if !seen[i] { + seen[i] = true + selected = append(selected, i) + } + } + } + + // Return in the order the options were presented, not the order the + // user typed them — a stable, predictable result. + sort.Ints(selected) + out := make([]string, 0, len(selected)) + for _, i := range selected { + out = append(out, options[i]) + } + return out, nil +} + // normalizeAffirmative lowercases ASCII letters and trims spaces // without dragging in `strings`. Used by both Confirm and Select // for case-insensitive input matching. @@ -305,3 +404,45 @@ func (p *fancyPrompt) Select(prompt string, options []string, defaultLabel strin } return got, nil } + +func (p *fancyPrompt) MultiSelect(prompt string, options []string) ([]string, error) { + if len(options) == 0 { + return nil, errors.New("ui: MultiSelect called with no options") + } + huhOptions := make([]huh.Option[string], len(options)) + for i, o := range options { + huhOptions[i] = huh.NewOption(o, o) + } + // Start with nothing selected: a destructive multi-select should + // require the user to actively pick what goes. + var got []string + form := huh.NewForm( + huh.NewGroup( + huh.NewMultiSelect[string](). + Title(prompt). + Options(huhOptions...). + Value(&got), + ), + ).WithInput(p.in).WithOutput(p.out) + if err := form.Run(); err != nil { + if errors.Is(err, huh.ErrUserAborted) { + // Aborting a destructive prompt means "select nothing". + return nil, nil + } + return nil, fmt.Errorf("prompt: %w", err) + } + + // Return the selection in the order the options were presented, so + // the result doesn't depend on the order the user toggled them. + picked := make(map[string]bool, len(got)) + for _, g := range got { + picked[g] = true + } + out := make([]string, 0, len(got)) + for _, o := range options { + if picked[o] { + out = append(out, o) + } + } + return out, nil +} diff --git a/internal/ui/prompt_test.go b/internal/ui/prompt_test.go index eb42322..e651f7d 100644 --- a/internal/ui/prompt_test.go +++ b/internal/ui/prompt_test.go @@ -256,3 +256,149 @@ func TestPlainPrompt_Confirm_NilOutSafe(t *testing.T) { t.Errorf("Confirm with io.Discard: got false, want true") } } + +// --- MultiSelect ------------------------------------------------------ + +// TestPlainPrompt_MultiSelect_Numbers covers the PlainMode numeric +// selection path: the user types the option numbers separated by +// commas and/or spaces. +func TestPlainPrompt_MultiSelect_Numbers(t *testing.T) { + tests := []struct { + name string + input string + want []string + }{ + {"single", "1\n", []string{"a.json"}}, + {"comma separated", "1,3\n", []string{"a.json", "c.json"}}, + {"space separated", "1 3\n", []string{"a.json", "c.json"}}, + {"mixed separators", "1, 3\n", []string{"a.json", "c.json"}}, + {"all three", "1,2,3\n", []string{"a.json", "b.json", "c.json"}}, + {"duplicates deduped", "1,1,2\n", []string{"a.json", "b.json"}}, + {"out of range ignored", "1,99\n", []string{"a.json"}}, + } + options := []string{"a.json", "b.json", "c.json"} + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + p := NewPrompt(PlainMode, strings.NewReader(tc.input), &bytes.Buffer{}) + got, err := p.MultiSelect("Select:", options) + if err != nil { + t.Fatalf("err = %v", err) + } + if !equalStrings(got, tc.want) { + t.Errorf("MultiSelect(%q) = %v, want %v", tc.input, got, tc.want) + } + }) + } +} + +// TestPlainPrompt_MultiSelect_Labels covers the "pick by name" path. +func TestPlainPrompt_MultiSelect_Labels(t *testing.T) { + options := []string{"fnm-20.10.0.json", "fnm-22.0.0.json"} + p := NewPrompt(PlainMode, strings.NewReader("fnm-22.0.0.json\n"), &bytes.Buffer{}) + got, err := p.MultiSelect("Select:", options) + if err != nil { + t.Fatalf("err = %v", err) + } + if !equalStrings(got, []string{"fnm-22.0.0.json"}) { + t.Errorf("MultiSelect by label = %v, want [fnm-22.0.0.json]", got) + } +} + +// TestPlainPrompt_MultiSelect_All covers the "all" keyword. +func TestPlainPrompt_MultiSelect_All(t *testing.T) { + options := []string{"a.json", "b.json"} + p := NewPrompt(PlainMode, strings.NewReader("all\n"), &bytes.Buffer{}) + got, err := p.MultiSelect("Select:", options) + if err != nil { + t.Fatalf("err = %v", err) + } + if !equalStrings(got, options) { + t.Errorf("MultiSelect(all) = %v, want %v", got, options) + } +} + +// TestPlainPrompt_MultiSelect_BlankSelectsNothing pins the safe +// direction: an empty answer means "delete nothing", NOT "delete +// everything". This is the destructive-prompt default that matters. +func TestPlainPrompt_MultiSelect_BlankSelectsNothing(t *testing.T) { + options := []string{"a.json", "b.json"} + p := NewPrompt(PlainMode, strings.NewReader("\n"), &bytes.Buffer{}) + got, err := p.MultiSelect("Select:", options) + if err != nil { + t.Fatalf("err = %v", err) + } + if len(got) != 0 { + t.Errorf("MultiSelect(blank) = %v, want nothing selected", got) + } +} + +// TestPlainPrompt_MultiSelect_EOFSelectsNothing covers closed stdin +// (a piped run with no input). Same safe direction as blank input. +func TestPlainPrompt_MultiSelect_EOFSelectsNothing(t *testing.T) { + options := []string{"a.json", "b.json"} + p := NewPrompt(PlainMode, strings.NewReader(""), &bytes.Buffer{}) + got, err := p.MultiSelect("Select:", options) + if err != nil { + t.Fatalf("err = %v", err) + } + if len(got) != 0 { + t.Errorf("MultiSelect(EOF) = %v, want nothing selected", got) + } +} + +// TestPlainPrompt_MultiSelect_OrdersByPresentation pins that the +// result follows the order the options were presented in, not the +// order the user typed them. +func TestPlainPrompt_MultiSelect_OrdersByPresentation(t *testing.T) { + options := []string{"a.json", "b.json", "c.json"} + p := NewPrompt(PlainMode, strings.NewReader("3,1\n"), &bytes.Buffer{}) + got, err := p.MultiSelect("Select:", options) + if err != nil { + t.Fatalf("err = %v", err) + } + if !equalStrings(got, []string{"a.json", "c.json"}) { + t.Errorf("MultiSelect(3,1) = %v, want [a.json c.json] (presentation order)", got) + } +} + +// TestPlainPrompt_MultiSelect_NoOptions verifies the empty-options +// guard — a caller with nothing to offer should get an error rather +// than an interactive prompt with no choices. +func TestPlainPrompt_MultiSelect_NoOptions(t *testing.T) { + p := NewPrompt(PlainMode, strings.NewReader("1\n"), &bytes.Buffer{}) + if _, err := p.MultiSelect("Select:", nil); err == nil { + t.Error("MultiSelect(nil options) = nil error, want an error") + } +} + +// TestPlainPrompt_MultiSelect_RendersOptions verifies the prompt +// actually shows the numbered list — a user can't pick by number +// if the numbers aren't printed. +func TestPlainPrompt_MultiSelect_RendersOptions(t *testing.T) { + var out bytes.Buffer + p := NewPrompt(PlainMode, strings.NewReader("\n"), &out) + if _, err := p.MultiSelect("Select files:", []string{"a.json", "b.json"}); err != nil { + t.Fatalf("err = %v", err) + } + text := out.String() + for _, want := range []string{"1) a.json", "2) b.json"} { + if !strings.Contains(text, want) { + t.Errorf("prompt output %q missing %q", text, want) + } + } +} + +// equalStrings compares two string slices element-wise. We avoid +// reflect.DeepEqual so a nil-vs-empty distinction doesn't turn into a +// confusing failure message. +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} From 68e1374143a32cd2b9ee853dbdd0c98f76192718 Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Sun, 1 Nov 2026 21:30:35 +0600 Subject: [PATCH 3/4] docs: document the snapshot cleanup command + keep_snapshots config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit README gains a "Reclaiming snapshot space" section covering `nodeup packages cleanup` and the `--keep-snapshots` opt-out, and the config example gains packages.keep_snapshots. SPEC records the new interface surface (`nodeup packages cleanup`, the --keep-snapshots flag, NODEUP_PACKAGES_KEEP_SNAPSHOTS) and four new invariants: - V22: an upgrade's snapshots are deleted iff the migration replayed them successfully (or the user opted out). A snapshot whose restore failed is never deleted — it is the rollback payload. - V23: snapshot writes are atomic. - V24: file deletion is confined to its own directory, after symlink evaluation. CLAUDE.md's file map gains cleanup.go and cleanup_snapshots.go. Part of #128. --- CLAUDE.md | 5 ++++- README.md | 26 +++++++++++++++++++++++++- SPEC.md | 15 +++++++++++++-- docs/configuration.md | 2 ++ 4 files changed, 44 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 80504ff..3b9a90b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -23,6 +23,7 @@ internal/cli/ cobra command wiring — thin layer, delegates to int root.go NewRootCmd; registers all subcommands upgrade.go nodeup upgrade (Phase 4, merged) check.go / list.go / packages.go / config.go / version.go + cleanup_snapshots.go nodeup packages cleanup — delete snapshot / report files internal/detector/ Manager interface + one file per manager detector.go DetectAll(), ResolveManager() fnm.go nvm.go volta.go asdf.go mise.go n.go nodenv.go viteplus.go nvm_windows.go @@ -32,6 +33,8 @@ internal/packages/ npm global snapshot / restore / migrate (merged in PR snapshot.go Snapshot(ctx, managerName, version) → ~/.../snapshots/-.json Restore(ctx, managerName, version) ([]PackageResult, error) RestoreFromSnapshot(ctx, path) ([]PackageResult, error) + cleanup.go ListSnapshotFiles() / DeleteSnapshotFile() — file-level snapshot + enumeration + safe deletion (see #128) report.go MigrationReport: per-package results, Save() → ~/.../reports/migration-.json sentinel.go UpgradeSentinel: in-progress marker for interrupted-upgrade replay internal/platform/ @@ -42,7 +45,7 @@ internal/ui/ single source of truth for user-facing output theme.go DefaultTheme — lipgloss styles for FancyWriter writer.go Writer interface + PlainWriter / FancyWriter implementations spinner.go bubbletea-backed braille spinner for long-running steps - prompt.go huh-backed Confirm / Select helpers for interactive flows + prompt.go huh-backed Confirm / Select / MultiSelect helpers for interactive flows ``` ## Key invariants — read before writing code diff --git a/README.md b/README.md index 795b656..a0e2a84 100644 --- a/README.md +++ b/README.md @@ -201,6 +201,29 @@ nodeup version Print version info Run `nodeup --help` for the full flag reference. +### Reclaiming snapshot space + +`nodeup upgrade` writes a snapshot of your global npm packages before it +migrates them, so it can roll back if something goes wrong. Once the +migration has replayed those packages onto the new Node version, the +snapshot has served its purpose — and nodeup deletes it automatically. + +Snapshots that survive a *failed* migration are deliberate: they're the +rollback payload the interrupted-upgrade warning points at, and they stay +on disk until you deal with them. `nodeup packages cleanup` is how you +deal with them: + +```bash +nodeup packages cleanup # pick files to delete (multi-select) +nodeup packages cleanup --list # show what would be deleted, delete nothing +nodeup packages cleanup --all --yes # delete every snapshot, no prompt +nodeup packages cleanup --reports # clean up migration reports instead +``` + +Pass `--keep-snapshots` to `nodeup upgrade` (or set +`packages.keep_snapshots: true` in the config) if you'd rather keep the +rollback payloads around for manual replays. + ## How it works In short: @@ -229,6 +252,7 @@ packages: skip: - npm - corepack + keep_snapshots: false # true = never auto-delete upgrade snapshots cleanup: auto: false prompt: true @@ -252,7 +276,7 @@ v1.1.0 → v1.2.1 history. |---|---|---| | 1 — Detector engine | ✅ | 9/9 managers detected (`fnm`, `nvm`, `Volta`, `asdf`, `mise`, `n`, `nodenv`, `vite-plus`, `nvm-windows`) | | 2 — Node version API | ✅ | `nodeup check` + nodejs.org/dist/index.json fetch + TTL cache + HTTP retry/backoff/timeout | -| 3 — Package snapshot/restore | ✅ | `nodeup packages {snapshot,restore,diff}` + interrupted-upgrade sentinel + MigrationReport | +| 3 — Package snapshot/restore | ✅ | `nodeup packages {snapshot,restore,diff,cleanup}` + interrupted-upgrade sentinel + MigrationReport | | 4 — Upgrade command | ✅ | End-to-end `nodeup upgrade` with snapshot → install → migrate → cleanup | | 5 — Config subsystem | ✅ | YAML config at `~/.nodeup/config.yaml` + `config {show,get,set,init}` + env vars | | 6 — Cross-platform polish | ✅ | `QuotePath`, interrupted-upgrade replay, system-node classifier, lock file | diff --git a/SPEC.md b/SPEC.md index 489f8c5..012ad72 100644 --- a/SPEC.md +++ b/SPEC.md @@ -39,10 +39,10 @@ manual steps, single static binary, no Node runtime dependency. ## §I Interfaces -- cmd: `nodeup upgrade [--lts|--current|--dry-run|--yes|--no-migrate|--no-cleanup|--cleanup|--cleanup-version |--manager ]` +- cmd: `nodeup upgrade [--lts|--current|--dry-run|--yes|--no-migrate|--keep-snapshots|--no-cleanup|--cleanup|--cleanup-version |--manager ]` - cmd: `nodeup check [--json]` - cmd: `nodeup list [--json]` -- cmd: `nodeup packages {snapshot|restore [--from ]|diff}` +- cmd: `nodeup packages {snapshot|restore [--from ]|diff|cleanup [--all|--reports|--list|--yes]}` - cmd: `nodeup config {show|get |set |init [--force]}` - cmd: `nodeup version` - api: `GET https://nodejs.org/dist/index.json` → `Manifest ([]ManifestVersion)` @@ -53,6 +53,7 @@ manual steps, single static binary, no Node runtime dependency. - file: `/nodeup.lock` - env: `NODEUP_MANAGER`, `NODEUP_TRACK_LTS`, `NODEUP_TRACK_CURRENT`, `NODEUP_PACKAGES_MIGRATE`, `NODEUP_PACKAGES_STRATEGY`, + `NODEUP_PACKAGES_KEEP_SNAPSHOTS`, `NODEUP_CACHE_TTL`, `NODEUP_CONFIG`, `NODEUP_HOME` - type: `Manager interface` → `Name() string`, `Detect() bool`, `Version() (string, error)`, `ListInstalled() ([]semver.Version, error)`, @@ -98,6 +99,15 @@ manual steps, single static binary, no Node runtime dependency. - V21: lock file (`/nodeup.lock`) ! held around any critical section that mutates installed Node versions or the config file concurrently. +- V22: snapshot lifecycle — an upgrade's snapshots are deleted ⟺ the + migration replayed them successfully (or `--keep-snapshots` / + `packages.keep_snapshots` says otherwise). A snapshot whose restore + failed ⊥ deleted: it is the rollback payload the sentinel points at. +- V23: snapshot write ! atomic (temp file same dir + rename) — a + truncated body is invisible to every reader and would leak. +- V24: file deletion (snapshots, reports) ! confined to its own + directory — the target must resolve *inside* it after symlink + evaluation, and must be a regular file. ## §T Tasks @@ -124,6 +134,7 @@ T19|x|fix double-prompt + sticky-no-override at cleanup decision point|#76,#95 T20|x|rest of cleanup/chore/ci/release batch|#62-#69,#89-#94,#99,#100,#107-#109 T21|x|doc drift: CLAUDE.md ui/lipgloss/phase-table stale; SPEC §T stale|#106 T22|x|add Vite+ (`vp`) as a supported version manager (full `vp env` surface + classifier + registry + docs)|I.managers,V14,#123,#124 +T23|x|fix snapshot leak: auto-delete upgrade snapshots after a successful migration + `nodeup packages cleanup` CLI + atomic snapshot write|V22,V23,V24,#128 ## §B Bugs diff --git a/docs/configuration.md b/docs/configuration.md index 8c2d0e5..b04e2ef 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -33,6 +33,7 @@ packages: migrate: true strategy: latest skip: [yarn, pnpm] + keep_snapshots: false cleanup: auto: false prompt: true @@ -64,6 +65,7 @@ leaves the previous file untouched. | `packages.migrate` | bool | `true` | Migrate global packages on upgrade | | `packages.strategy` | string | `exact` | `exact` (pin version) or `latest` (re-fetch) | | `packages.skip` | []string | `[npm, corepack, npx]` | Packages to never migrate | +| `packages.keep_snapshots` | bool | `false` | Keep the package snapshots an upgrade writes. By default nodeup deletes them once the migration has replayed them onto the new Node version. Set to `true` to keep the rollback payloads for manual replays. The `--keep-snapshots` flag on `nodeup upgrade` overrides this. | | `cleanup.auto` | bool | `false` | Auto-remove old versions | | `cleanup.prompt` | bool | `true` | Ask before removing each old version | | `cache.ttl` | int | `3600` | Cache TTL in seconds | \ No newline at end of file From 091fb183096c44a6e2cd77a1e55eaabe4715e6b1 Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Sun, 1 Nov 2026 21:33:35 +0600 Subject: [PATCH 4/4] fix(cli): satisfy gocritic and misspell in the cleanup command Two lint failures on the new cleanup command: - `delete` shadowed the predeclared builtin (gocritic builtinShadow). Renamed to `removeFile`. - "cancelled" is a misspelling of the US spelling "canceled" (misspell linter). Co-Authored-By: puku-ai-2.8 --- internal/cli/cleanup_snapshots.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/internal/cli/cleanup_snapshots.go b/internal/cli/cleanup_snapshots.go index c750c4b..cc092af 100644 --- a/internal/cli/cleanup_snapshots.go +++ b/internal/cli/cleanup_snapshots.go @@ -79,7 +79,11 @@ func runCleanup(cmd *cobra.Command, args []string) error { var files []packages.SnapshotFile var kind string - var delete func(string) error + // removeFile is the deletion routine for the kind we're cleaning: + // DeleteReportFile for reports, DeleteSnapshotFile for snapshots. + // (Named removeFile rather than delete, which shadows the + // predeclared builtin.) + var removeFile func(string) error if reports { kind = "migration report" @@ -88,7 +92,7 @@ func runCleanup(cmd *cobra.Command, args []string) error { return fmt.Errorf("list migration reports: %w", err) } files = f - delete = packages.DeleteReportFile + removeFile = packages.DeleteReportFile } else { kind = "snapshot" f, err := packages.ListSnapshotFiles() @@ -96,7 +100,7 @@ func runCleanup(cmd *cobra.Command, args []string) error { return fmt.Errorf("list snapshots: %w", err) } files = f - delete = packages.DeleteSnapshotFile + removeFile = packages.DeleteSnapshotFile } if len(files) == 0 { @@ -164,7 +168,7 @@ func runCleanup(cmd *cobra.Command, args []string) error { return fmt.Errorf("cleanup confirmation: %w", err) } if !ok { - w.Info("Cleanup cancelled.") + w.Info("Cleanup canceled.") return nil } } @@ -174,7 +178,7 @@ func runCleanup(cmd *cobra.Command, args []string) error { var deleted int var failures []string for _, f := range selected { - if err := delete(f.Path); err != nil { + if err := removeFile(f.Path); err != nil { failures = append(failures, fmt.Sprintf("%s: %v", f.Name, err)) continue }