fix(cli): replay snapshot from latest-installed version, not new version

CHANGELOG.md

@@ -123,6 +123,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   `chore(ci): replace issue-workflow.sh shell script with an AI skill`
   for the full rationale.
 
+### Fixed
+- `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`),
+  but the upgrade flow only writes snapshots for the **previously**
+  installed versions. The restore then read a non-existent
+  `<DataDir>/snapshots/<mgr>-<newVersion>.json`, hit
+  `read snapshot: open …: no such file or directory`, and the loop's
+  blanket `cmd.Printf("Warning: restore failed: …")` swallowed the
+  error — silently no-op'ing the package migration for every newly
+  installed Node. The restore step now resolves the snapshot path the
+  same way the sentinel does (latest-installed-version key) and
+  replays via `packages.RestoreFromSnapshot(ctx, snapshotPath)`,
+  carrying the user's globals forward to the new default exactly
+  once. Added a regression test pinning the
+  `RestoreFromSnapshot`-reads-source-path contract. Closes #42.
+
 ## [0.0.0] - 2024-07-01
 
 ### Added

internal/cli/upgrade.go

@@ -210,50 +210,58 @@ func runUpgrade(cmd *cobra.Command, args []string) error {
 		return nil
 	}
 
-	// Snapshot current packages
+	// Snapshot current packages. We record one snapshot per installed
+	// version so the user can manually replay against any of them via
+	// `nodeup packages restore <mgr> <version>`, 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.
+	//
+	// While we're at it, resolve the conventional snapshot path for
+	// the latest installed version — this is the path both the
+	// sentinel arms and the restore step reads from, so we compute it
+	// once and pass it down.
+	var restoreSnapshotPath string
+	var oldVersion string
 	if !skipMigrate {
 		ctx := cmd.Context()
 		for _, v := range installedVersions {
 			if err := packages.Snapshot(ctx, m.Name(), v); err != nil {
 				cmd.Printf("Warning: snapshot failed for %s: %v\n", v, err)
 			}
 		}
+		if len(installedVersions) > 0 {
+			last := installedVersions[len(installedVersions)-1]
+			oldVersion = last.String()
+			// Resolve the snapshot path the conventional way. We
+			// tolerate a failure here — without a snapshot path the
+			// sentinel is still useful (it tells the user a
+			// migration was in flight) and we don't want to abort
+			// the upgrade over a path-resolution glitch.
+			if p, perr := packages.SnapshotPath(m.Name(), oldVersion); perr == nil {
+				restoreSnapshotPath = p
+			}
+		}
 	}
 
 	// Arm the sentinel AFTER snapshots are on disk but BEFORE any
 	// install mutation. If we crash between here and the deferred
 	// cleanup, the sentinel is the "this upgrade was interrupted"
 	// breadcrumb that the next `nodeup` invocation will pick up.
 	//
-	// We point the sentinel at the latest installed version's snapshot
-	// since that is the most likely one we want to replay against —
-	// it contains the package set the user had right before we started
-	// installing the new versions.
+	// The sentinel records the snapshot path so that
+	// `nodeup packages restore --from <sentinel path>` 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()
 		}
-		oldVersion := ""
-		var snapshotPath string
-		if len(installedVersions) > 0 {
-			last := installedVersions[len(installedVersions)-1]
-			oldVersion = last.String()
-			// Resolve the snapshot path the conventional way so the
-			// warning message can hand it to the user verbatim. We
-			// tolerate a failure here — without a snapshot path the
-			// sentinel is still useful (it tells the user a migration
-			// was in flight) and we don't want to abort the upgrade
-			// over a path-resolution glitch.
-			if p, perr := packages.SnapshotPath(m.Name(), oldVersion); perr == nil {
-				snapshotPath = p
-			}
-		}
 		if err := packages.WriteSentinel(packages.UpgradeSentinel{
 			Manager:      m.Name(),
 			OldVersion:   oldVersion,
 			NewVersion:   newVersion,
-			SnapshotPath: snapshotPath,
+			SnapshotPath: restoreSnapshotPath,
 		}); err != nil {
 			cmd.Printf("Warning: failed to write upgrade sentinel: %v\n", err)
 		} else {
@@ -277,13 +285,23 @@ func runUpgrade(cmd *cobra.Command, args []string) error {
 		}
 	}
 
-	// Restore packages
+	// Restore packages under the NEW default. We replay the snapshot
+	// of the latest previously-installed Node version's global npm
+	// packages against the freshly-set-default Node — the user's
+	// globals carry forward to the new install exactly once.
+	//
+	// We deliberately do NOT loop per-new-version here: each newly
+	// installed Node has its own `npm install -g` environment, and
+	// the right package set is the one from the user's most-recent
+	// active Node, not from any freshly-installed (empty) one. The
+	// sentinel already records this snapshot path for
+	// replay-after-interrupt (`nodeup packages restore --from
+	// <sentinel path>`); here we read the same path directly.
 	if !skipMigrate && len(toInstall) > 0 {
-		ctx := cmd.Context()
-		for _, v := range toInstall {
-			if err := packages.Restore(ctx, m.Name(), *v); err != nil {
-				cmd.Printf("Warning: restore failed: %v\n", err)
-			}
+		if restoreSnapshotPath == "" {
+			cmd.Printf("Warning: no snapshot path available to restore from; skipping package migration\n")
+		} else if err := packages.RestoreFromSnapshot(cmd.Context(), restoreSnapshotPath); err != nil {
+			cmd.Printf("Warning: restore failed: %v\n", err)
 		}
 	}
 

internal/packages/snapshot_test.go

@@ -0,0 +1,138 @@
+package packages
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"io/fs"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/Masterminds/semver/v3"
+)
+
+// TestSnapshot_StoredVersionKeyMatchesArg verifies the on-disk path
+// convention used by Snapshot() / SnapshotPath(): a snapshot taken for
+// Node 20.10.0 lives at <DataDir>/snapshots/fnm-20.10.0.json — the
+// file name is keyed on the version argument, not on the new version
+// the upgrade flow is about to install.
+//
+// This test pins down the contract that the upgrade-loop fix in #42
+// relies on: the upgrade flow snapshots the *previously installed*
+// versions, then on restore uses RestoreFromSnapshot(<that snapshot
+// path>) rather than looking up a snapshot file keyed on the
+// newly-installed version (which would silently miss and no-op the
+// migration).
+func TestSnapshot_StoredVersionKeyMatchesArg(t *testing.T) {
+	redirectDataDir(t)
+
+	// No installed npm, no packages — Snapshot() reads
+	// `npm ls -g --json --depth=0` and will fail in a test
+	// environment. We bypass Snapshot() and call saveSnapshot()
+	// directly with a known SnapshotData struct so the test stays
+	// hermetic.
+	snap := SnapshotData{
+		Manager:     "fnm",
+		NodeVersion: "20.10.0",
+		Packages:    []Package{},
+	}
+	if err := saveSnapshot(snap); err != nil {
+		t.Fatalf("saveSnapshot: %v", err)
+	}
+
+	// Conventional path should exist for the OLD-installed version.
+	wantPath, err := SnapshotPath("fnm", "20.10.0")
+	if err != nil {
+		t.Fatalf("SnapshotPath old-version: %v", err)
+	}
+	if _, err := os.Stat(wantPath); err != nil {
+		t.Fatalf("snapshot file not created at conventional path %s: %v", wantPath, err)
+	}
+
+	// The version-key the upgrade loop used to look up (BEFORE the
+	// #42 fix) was the NEW installed version (20.11.0). That key
+	// deliberately misses — verifying the miss here pins down the
+	// regression that the fix prevents.
+	newVerPath, err := SnapshotPath("fnm", "20.11.0")
+	if err != nil {
+		t.Fatalf("SnapshotPath new-version: %v", err)
+	}
+	if _, statErr := os.Stat(newVerPath); statErr == nil {
+		t.Fatalf("new-version snapshot file should NOT exist at %s — if it does, the version-key mismatch regression has resurfaced", newVerPath)
+	}
+}
+
+// TestRestore_LooksUpByArgVersion covers the existing behavior of
+// Restore(ctx, manager, version): it loads the conventional
+// <DataDir>/snapshots/<mgr>-<version>.json file. We don't exercise
+// installPackages' actual `npm install -g` call (that needs a working
+// npm binary), but verify the read-and-parse path returns a wrapped
+// "read snapshot" error when the file does not exist — this is the
+// failure mode that used to silently no-op the upgrade migration
+// when the upgrade-loop called Restore with the new (not-yet-existing)
+// version key.
+func TestRestore_LooksUpByArgVersion(t *testing.T) {
+	redirectDataDir(t)
+
+	// There is no snapshot for fnm 99.99.99 — Restore must surface
+	// that as a wrapped read error, not a silent success.
+	v, err := semver.NewVersion("99.99.99")
+	if err != nil {
+		t.Fatalf("parse version: %v", err)
+	}
+	restoreErr := Restore(context.Background(), "fnm", *v)
+	if restoreErr == nil {
+		t.Fatal("expected error for missing snapshot, got nil")
+	}
+	if !strings.Contains(restoreErr.Error(), "read snapshot") {
+		t.Errorf("error %q does not mention read snapshot failure", restoreErr)
+	}
+	// The user-facing error should be wrapped — `read snapshot:` is
+	// the human-readable prefix and the underlying fs.PathError must
+	// remain accessible via errors.Is so callers can branch on
+	// fs.ErrNotExist. Stdlib's *PathError satisfies this naturally,
+	// so we don't add an Is() method on our error type.
+	if !errors.Is(restoreErr, fs.ErrNotExist) {
+		t.Errorf("error %q does not unwrap to fs.ErrNotExist; callers cannot branch on missing-file", restoreErr)
+	}
+}
+
+// TestRestoreFromSnapshot_ReadsSourcePathRegardlessOfNodeVersion is the
+// regression test for the upgrade-loop fix: RestoreFromSnapshot reads
+// the snapshot at the path it's given regardless of the Node version
+// key in the filename. This is what the upgrade flow relies on —
+// the snapshot path comes from the sentinel, which records the
+// latest-INSTALLED-version's snapshot (e.g., fnm-20.10.0.json), not a
+// fresh-version snapshot keyed on the just-installed Node.
+func TestRestoreFromSnapshot_ReadsSourcePathRegardlessOfNodeVersion(t *testing.T) {
+	redirectDataDir(t)
+
+	// Write a snapshot at an arbitrary path (does not follow the
+	// `<mgr>-<ver>.json` convention — RestoreFromSnapshot is
+	// explicitly path-based).
+	tmp := t.TempDir()
+	arbitraryPath := filepath.Join(tmp, "old-installed-set.json")
+	snap := SnapshotData{
+		Manager:     "fnm",
+		NodeVersion: "20.10.0",   // the OLD installed version's key
+		Packages:    []Package{}, // no packages → install loop is a no-op
+	}
+	data, err := json.MarshalIndent(snap, "", "  ")
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+	if err := os.WriteFile(arbitraryPath, data, 0o644); err != nil {
+		t.Fatalf("write: %v", err)
+	}
+
+	// RestoreFromSnapshot should succeed: the install loop iterates
+	// over zero packages and exits cleanly. This proves that the
+	// path-based restore does NOT depend on the version-key in the
+	// filename matching some "new version" expectation — it just
+	// reads what's at the path.
+	if err := RestoreFromSnapshot(context.Background(), arbitraryPath); err != nil {
+		t.Errorf("RestoreFromSnapshot: %v", err)
+	}
+}