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: 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 diff --git a/internal/cli/cleanup_snapshots.go b/internal/cli/cleanup_snapshots.go new file mode 100644 index 0000000..cc092af --- /dev/null +++ b/internal/cli/cleanup_snapshots.go @@ -0,0 +1,216 @@ +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 + // 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" + f, err := packages.ListReportFiles() + if err != nil { + return fmt.Errorf("list migration reports: %w", err) + } + files = f + removeFile = packages.DeleteReportFile + } else { + kind = "snapshot" + f, err := packages.ListSnapshotFiles() + if err != nil { + return fmt.Errorf("list snapshots: %w", err) + } + files = f + removeFile = 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 canceled.") + 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 := removeFile(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 +}