fix(cli): --yes no longer overrides --no-cleanup precedence

CHANGELOG.md

@@ -146,6 +146,46 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   for the full rationale.
 
 ### Fixed
+- `internal/cli` (`upgrade.go`): `--yes --no-cleanup` no longer
+  silently auto-deletes every eligible old Node.js version. The
+  `if yes { ... }` block at upgrade.go:107-111 ran unconditionally
+  AFTER the `switch` that was supposed to make `--no-cleanup` win,
+  so it flipped `NonInteractive` back to `false` and set
+  `AutoDeleteAll=true`, regardless of `noCleanup`. A CI script
+  that habitually passes `-y`/`--yes` (the documented
+  `--no-cleanup` contract is "never prompt, never delete") would
+  therefore lose Node installs it never asked to remove. The
+  cleanup-toggle resolution is now factored into a pure helper
+  `resolveCleanupConfig(noCleanup, autoCleanup, yes, versions, cfg)`
+  in `cleanup.go` so it's testable in isolation; the `--yes`
+  block is guarded by `&& !noCleanup`, restoring `--no-cleanup`'s
+  precedence. New tests in `cleanup_config_test.go` pin the
+  precedence table (including the negative-path matrix where
+  `--no-cleanup` beats every other knob). Closes #57.
+- `internal/cli/packages.go` (`runRestore` and `runDiff`): the
+  `<manager>` positional argument is now validated against a
+  canonical allowlist (`detector.IsAllowedManagerName`) before any
+  filesystem path is constructed. Pre-fix, the manager name was
+  interpolated verbatim into a snapshot filename
+  (`fmt.Sprintf("%s-%s.json", managerName, version)`) and the
+  result was `filepath.Join`'d into the snapshots dir — and
+  `filepath.Join` collapses `..` segments, so a manager name like
+  `../../tmp/evil` resolved outside the snapshots directory. An
+  attacker with a local file-placement primitive (a shared temp
+  directory, a cloned repo containing a payload filename like
+  `<prefix>../../tmp/evil-1.0.0.json`) could have the resulting
+  snapshot's `Packages` list piped straight into
+  `npm install -g <name>@<version>` with no validation. The
+  allowlist is built from `detector.AllowedManagerNames()` (a new
+  helper that derives the list from `detector.All()`), so the
+  set stays in sync with the per-platform build files; the match
+  is byte-for-byte case-sensitive (matching the `--manager` flag),
+  and the error message surfaces the offender and the allowlist
+  so typos remain user-fixable. `internal/detector/manager_names.go`
+  (new file) holds the helpers; `manager_names_test.go` pins
+  AllowedManagerNames ↔ All() parity and IsAllowedManagerName
+  behaviour (incl. traversal payloads, case-fold negativity, and
+  near-miss strings). Closes #51.
 - `internal/cli`: upgrade loop's restore step was looking up its
   snapshot by the **new** installed Node version
   (`packages.Restore(ctx, mgr, *v)` where `v` came from `toInstall`),

internal/cli/cleanup.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/Masterminds/semver/v3"
 
+	"github.com/dipto0321/nodeup/internal/config"
 	"github.com/dipto0321/nodeup/internal/detector"
 )
 
@@ -64,6 +65,54 @@ type cleanupConfig struct {
 	Prefiltered []semver.Version
 }
 
+// resolveCleanupConfig maps the post-upgrade cleanup toggles (CLI flags
+// + config-file values) into the cleanupConfig struct that
+// runCleanupPrompt consumes.
+//
+// Precedence, highest first:
+//
+//	--no-cleanup             → NonInteractive=true; nothing else applies.
+//	                          --no-cleanup's documented contract is
+//	                          "never prompt, never delete" and it beats
+//	                          every other toggle, including --yes. A CI
+//	                          script that always passes `-y`/`--yes`
+//	                          must NOT silently delete Node installs
+//	                          the user told it to keep. See #57.
+//	--cleanup                → AutoDeleteAll=true.
+//	cfg.Cleanup.Auto         → AutoDeleteAll=true.
+//	--yes (and !noCleanup)   → AutoDeleteAll=true, PerVersion=false,
+//	                          NonInteractive=false. So a non-interactive
+//	                          run still makes progress without hanging
+//	                          on an unserviced prompt.
+//
+// PerVersion defaults to cfg.Cleanup.Prompt (true by default), and
+// Prefiltered is whatever --cleanup-version passed.
+func resolveCleanupConfig(noCleanup, autoCleanup, yes bool, cleanupVersions []semver.Version, cfg config.CleanupConfig) cleanupConfig {
+	out := cleanupConfig{
+		NonInteractive: noCleanup,
+		PerVersion:     cfg.Prompt,
+		Prefiltered:    cleanupVersions,
+	}
+	switch {
+	case noCleanup:
+		// Already set; no other knobs apply. --no-cleanup wins.
+	case autoCleanup:
+		out.AutoDeleteAll = true
+	case cfg.Auto:
+		out.AutoDeleteAll = true
+	}
+	if yes && !noCleanup {
+		// --yes implies auto-delete-all so non-interactive runs don't
+		// hang on an unserviced prompt. Guarded by !noCleanup so a user
+		// who explicitly says "never delete" doesn't have that contract
+		// silently overridden by a CI script's `-y` flag. See #57.
+		out.NonInteractive = false
+		out.AutoDeleteAll = true
+		out.PerVersion = false
+	}
+	return out
+}
+
 // cleanupResult summarizes what happened during a cleanup run. The
 // upgrade command reports this to the user after the prompt loop
 // completes (success or partial-failure).

internal/cli/cleanup_config_test.go

@@ -0,0 +1,182 @@
+package cli
+
+import (
+	"testing"
+
+	"github.com/Masterminds/semver/v3"
+
+	"github.com/dipto0321/nodeup/internal/config"
+)
+
+// TestResolveCleanupConfig_NoCleanupBeatsYes pins the precedence fix
+// for #57. Before the fix, --yes ran unconditionally AFTER the switch
+// that was supposed to make --no-cleanup win — the body flipped
+// NonInteractive back to false and set AutoDeleteAll=true, so
+// `nodeup upgrade --yes --no-cleanup` silently deleted every old
+// Node.js version. After the fix, --no-cleanup's documented contract
+// ("never prompt, never delete") survives any combination of the
+// other flags.
+func TestResolveCleanupConfig_NoCleanupBeatsYes(t *testing.T) {
+	cfg := config.CleanupConfig{Auto: false, Prompt: true}
+
+	got := resolveCleanupConfig(
+		/*noCleanup*/ true,
+		/*autoCleanup*/ false,
+		/*yes*/ true,
+		nil,
+		cfg,
+	)
+
+	if !got.NonInteractive {
+		t.Errorf("--no-cleanup + --yes: NonInteractive = false, want true (--no-cleanup must win)")
+	}
+	if got.AutoDeleteAll {
+		t.Errorf("--no-cleanup + --yes: AutoDeleteAll = true, want false (--no-cleanup must win)")
+	}
+	// Sanity: a regression that flipped the `&& !noCleanup` guard
+	// back to a bare `yes` would also need to leave the per-version
+	// prompt flag alone. Pin both.
+	if !got.PerVersion {
+		t.Errorf("--no-cleanup + --yes: PerVersion = false, want true (cfg.Cleanup.Prompt default)")
+	}
+}
+
+// TestResolveCleanupConfig_YesAutoDeletesWithoutNoCleanup verifies
+// that --yes still works as documented when --no-cleanup is NOT set.
+// A regression that guarded the block with `!yes` instead of `&& !noCleanup`
+// would silently break every CI script relying on --yes.
+func TestResolveCleanupConfig_YesAutoDeletesWithoutNoCleanup(t *testing.T) {
+	cfg := config.CleanupConfig{Auto: false, Prompt: true}
+
+	got := resolveCleanupConfig(
+		/*noCleanup*/ false,
+		/*autoCleanup*/ false,
+		/*yes*/ true,
+		nil,
+		cfg,
+	)
+
+	if got.NonInteractive {
+		t.Errorf("--yes alone: NonInteractive = true, want false (we're not skipping cleanup, just auto-confirming)")
+	}
+	if !got.AutoDeleteAll {
+		t.Errorf("--yes alone: AutoDeleteAll = false, want true")
+	}
+	if got.PerVersion {
+		t.Errorf("--yes alone: PerVersion = true, want false (auto-confirm should skip per-version y/N)")
+	}
+}
+
+// TestResolveCleanupConfig_CleanupFlagBeatsYesAndCfgAuto documents
+// that --cleanup and cfg.Cleanup.Auto both map to AutoDeleteAll=true.
+// Both are subsumed under --yes's "skip the per-version confirm"
+// behavior — they're the same end state.
+func TestResolveCleanupConfig_CleanupFlagSetsAutoDeleteAll(t *testing.T) {
+	cfg := config.CleanupConfig{Auto: false, Prompt: true}
+
+	got := resolveCleanupConfig(false, true, false, nil, cfg)
+	if !got.AutoDeleteAll {
+		t.Errorf("--cleanup: AutoDeleteAll = false, want true")
+	}
+	if got.NonInteractive {
+		t.Errorf("--cleanup: NonInteractive = true, want false")
+	}
+}
+
+func TestResolveCleanupConfig_ConfigAutoSetsAutoDeleteAll(t *testing.T) {
+	cfg := config.CleanupConfig{Auto: true, Prompt: true}
+
+	got := resolveCleanupConfig(false, false, false, nil, cfg)
+	if !got.AutoDeleteAll {
+		t.Errorf("cfg.Cleanup.Auto=true: AutoDeleteAll = false, want true")
+	}
+}
+
+// TestResolveCleanupConfig_PrefilteredPropagates covers --cleanup-version:
+// the parsed semver slice must pass through unchanged.
+func TestResolveCleanupConfig_PrefilteredPropagates(t *testing.T) {
+	cfg := config.CleanupConfig{Auto: false, Prompt: true}
+
+	pre := []semver.Version{
+		mustVer(t, "20.18.0"),
+		mustVer(t, "18.20.4"),
+	}
+
+	got := resolveCleanupConfig(false, false, false, pre, cfg)
+	if len(got.Prefiltered) != len(pre) {
+		t.Fatalf("Prefiltered length = %d, want %d", len(got.Prefiltered), len(pre))
+	}
+	for i := range pre {
+		if got.Prefiltered[i].String() != pre[i].String() {
+			t.Errorf("Prefiltered[%d] = %s, want %s", i, got.Prefiltered[i], pre[i])
+		}
+	}
+}
+
+// TestResolveCleanupConfig_DefaultsAreInteractive covers the default
+// state: no flags, default config (Auto=false, Prompt=true). Cleanup
+// should run interactively with per-version y/N prompts.
+func TestResolveCleanupConfig_DefaultsAreInteractive(t *testing.T) {
+	cfg := config.CleanupConfig{Auto: false, Prompt: true}
+
+	got := resolveCleanupConfig(false, false, false, nil, cfg)
+	if got.NonInteractive {
+		t.Errorf("defaults: NonInteractive = true, want false")
+	}
+	if got.AutoDeleteAll {
+		t.Errorf("defaults: AutoDeleteAll = true, want false")
+	}
+	if !got.PerVersion {
+		t.Errorf("defaults: PerVersion = false, want true (cfg.Cleanup.Prompt default)")
+	}
+	if len(got.Prefiltered) != 0 {
+		t.Errorf("defaults: Prefiltered = %v, want empty", got.Prefiltered)
+	}
+}
+
+// TestResolveCleanupConfig_CfgPromptFalseSkipsPerVersion covers
+// cfg.Cleanup.Prompt=false: per-version y/N is skipped but we still
+// hit the all-or-nothing prompt (unless Auto is also true).
+func TestResolveCleanupConfig_CfgPromptFalseSkipsPerVersion(t *testing.T) {
+	cfg := config.CleanupConfig{Auto: false, Prompt: false}
+
+	got := resolveCleanupConfig(false, false, false, nil, cfg)
+	if got.PerVersion {
+		t.Errorf("cfg.Cleanup.Prompt=false: PerVersion = true, want false")
+	}
+	if got.AutoDeleteAll {
+		t.Errorf("cfg.Cleanup.Prompt=false: AutoDeleteAll = true, want false (only Prompt was false, not Auto)")
+	}
+}
+
+// TestResolveCleanupConfig_NoCleanupBeatsEverythingElse is the
+// negative-path matrix for #57. Each row sets one knob that would
+// otherwise drive AutoDeleteAll=true and asserts --no-cleanup
+// wins on every one. The bug from #57 was a single unguarded
+// `if yes` block; this test pins all the other places the same
+// regression could re-appear.
+func TestResolveCleanupConfig_NoCleanupBeatsEverythingElse(t *testing.T) {
+	cases := []struct {
+		name        string
+		autoCleanup bool
+		yes         bool
+		cfg         config.CleanupConfig
+	}{
+		{"noCleanup + autoCleanup", true, false, config.CleanupConfig{Auto: false, Prompt: true}},
+		{"noCleanup + yes", false, true, config.CleanupConfig{Auto: false, Prompt: true}},
+		{"noCleanup + cfg.Auto", false, false, config.CleanupConfig{Auto: true, Prompt: true}},
+		{"noCleanup + every knob", true, true, config.CleanupConfig{Auto: true, Prompt: true}},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := resolveCleanupConfig(true, tc.autoCleanup, tc.yes, nil, tc.cfg)
+			if !got.NonInteractive {
+				t.Errorf("NonInteractive = false, want true (--no-cleanup must always win)")
+			}
+			if got.AutoDeleteAll {
+				t.Errorf("AutoDeleteAll = true, want false (--no-cleanup must always win)")
+			}
+		})
+	}
+}

internal/cli/upgrade.go

@@ -79,37 +79,11 @@ func runUpgrade(cmd *cobra.Command, args []string) error {
 	skipMigrate := noMigrate || !cfg.Packages.Migrate
 	_ = skipMigrate // referenced below in snapshot/restore sections
 
-	// Cleanup behavior toggles. Resolution order (highest first):
-	//   --no-cleanup           : never prompt, never delete
-	//   --cleanup              : auto-delete all (no all-or-nothing prompt)
-	//   --cleanup-version <v>  : delete only these versions
-	//   cfg.Cleanup.Auto       : auto-delete all (no prompt)
-	//   cfg.Cleanup.Prompt=false: skip the per-version confirm
-	//   default                : prompt, one y/N per cleanup action
-	//
-	// The cleanupConfig struct is what runCleanupPrompt consumes.
-	cleanupCfg := cleanupConfig{
-		NonInteractive: noCleanup,
-		PerVersion:     cfg.Cleanup.Prompt,
-		Prefiltered:    cleanupVersions,
-	}
-	switch {
-	case noCleanup:
-		// already set; no other knobs apply
-	case autoCleanup:
-		cleanupCfg.AutoDeleteAll = true
-		cleanupCfg.Prefiltered = cleanupVersions // combine with --cleanup-version if both set
-	case cfg.Cleanup.Auto:
-		cleanupCfg.AutoDeleteAll = true
-		cleanupCfg.Prefiltered = cleanupVersions
-	}
-	// --yes implies auto-delete-all so non-interactive runs don't hang.
-	if yes {
-		cleanupCfg.NonInteractive = false
-		cleanupCfg.AutoDeleteAll = true
-		cleanupCfg.PerVersion = false
-	}
-	_ = yes
+	// Resolve the cleanup behavior. The full precedence table lives
+	// on resolveCleanupConfig; the key contract is that --no-cleanup
+	// beats --yes ("never prompt, never delete" must not be silently
+	// overridden by a CI script's -y). See #57.
+	cleanupCfg := resolveCleanupConfig(noCleanup, autoCleanup, yes, cleanupVersions, cfg.Cleanup)
 
 	// From here on, anything that mutates disk in a way that needs
 	// replay-by-sentinel bookkeeping is wrapped in a sentinel lifecycle.