fix(packages): continue past failed installs; write MigrationReport on restore

internal/cli/packages.go

@@ -160,12 +160,52 @@ func runRestore(cmd *cobra.Command, args []string) error {
 		}
 	}
 
+	// writeReport persists a MigrationReport so the user has a record
+	// of what migrated. We only write when at least one package was
+	// attempted — skipping the write on an empty results slice avoids
+	// producing a useless "0 packages attempted" file for fresh installs
+	// with no globals. The snapshot's recorded Manager / NodeVersion
+	// are preferred over the CLI's inferred values when present, so the
+	// --from branch (which has no CLI-side manager argument) gets an
+	// accurate report.
+	writeReport := func(snapshot packages.SnapshotData, fallbackMgr, fallbackFrom string, results []packages.PackageResult) {
+		if len(results) == 0 {
+			return
+		}
+		mgr := snapshot.Manager
+		if mgr == "" {
+			mgr = fallbackMgr
+		}
+		fromVer := snapshot.NodeVersion
+		if fromVer == "" {
+			fromVer = fallbackFrom
+		}
+		report := packages.NewMigrationReport(mgr, fromVer, "")
+		for _, r := range results {
+			report.AddResult(r)
+		}
+		if err := report.Save(); err != nil {
+			cmd.Printf("Warning: failed to write migration report: %v\n", err)
+			return
+		}
+		if p, perr := report.Path(); perr == nil {
+			cmd.Printf("Migration report: %s\n", p)
+		}
+	}
+
 	// --from branch: read the path straight off disk, no manager or
 	// version parsing required.
 	if fromPath != "" {
-		if err := packages.RestoreFromSnapshot(ctx, fromPath); err != nil {
+		outcome, err := packages.RestoreFromSnapshot(ctx, fromPath)
+		if err != nil {
+			// Even on partial failure, persist whatever results we
+			// collected so the user has a file to inspect — the
+			// returned slice still contains entries for every package
+			// attempted, including failures (status="failed").
+			writeReport(outcome.Snapshot, "", "", outcome.Results)
 			return fmt.Errorf("restore failed: %w", err)
 		}
+		writeReport(outcome.Snapshot, "", "", outcome.Results)
 		clearSentinel()
 		cmd.Printf("Restored packages from %s\n", fromPath)
 		return nil
@@ -192,9 +232,15 @@ func runRestore(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("invalid version: %w", err)
 	}
 
-	if err := packages.Restore(ctx, managerName, *v); err != nil {
+	outcome, err := packages.Restore(ctx, managerName, *v)
+	if err != nil {
+		// Partial failure: persist a report (with the manager name
+		// and source version so the user can correlate) so they know
+		// which packages to retry.
+		writeReport(outcome.Snapshot, managerName, versionStr, outcome.Results)
 		return fmt.Errorf("restore failed: %w", err)
 	}
+	writeReport(outcome.Snapshot, managerName, versionStr, outcome.Results)
 	clearSentinel()
 
 	cmd.Printf("Restored packages for %s %s\n", managerName, versionStr)

internal/cli/upgrade.go

@@ -96,6 +96,13 @@ func runUpgrade(cmd *cobra.Command, args []string) error {
 	// on disk so a follow-up `nodeup packages restore --from <path>`
 	// (the command printed by PersistentPreRunE's hint) can still find
 	// the resume breadcrumb. See issue #46.
+	//
+	// "Failure" here includes PARTIAL failure: if one or more packages
+	// failed to install but the loop did complete (which it now does —
+	// see #103), restoreSucceeded stays false. The MigrationReport file
+	// is written either way; the sentinel only clears on a fully-successful
+	// migration so a follow-up `nodeup packages restore --from <path>`
+	// remains available to retry just the failed packages.
 	sentinelArmed := false
 	restoreSucceeded := false
 	defer func() {
@@ -306,13 +313,63 @@ func runUpgrade(cmd *cobra.Command, args []string) error {
 	// sentinel already records this snapshot path for
 	// replay-after-interrupt (`nodeup packages restore --from
 	// <sentinel path>`); here we read the same path directly.
+	//
+	// installPackages now LOOP-ALL (see issue #103): a single failing
+	// package no longer aborts the rest of the migration. We collect
+	// the per-package results, persist a MigrationReport to
+	// <DataDir>/reports/ on both success and partial failure, and
+	// surface the report path so the user knows what to re-install
+	// by hand if anything failed. Partial failure keeps the sentinel
+	// armed (restoreSucceeded stays false) so the user can re-run
+	// `nodeup packages restore --from <snapshot>` to retry just the
+	// failures.
 	if !skipMigrate && len(toInstall) > 0 {
 		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)
 		} else {
-			restoreSucceeded = true
+			outcome, rerr := packages.RestoreFromSnapshot(cmd.Context(), restoreSnapshotPath)
+
+			// Write a MigrationReport whenever we actually attempted at
+			// least one package. Skipping the write on empty results
+			// avoids producing a useless "0 packages attempted" file
+			// for a fresh install with no globals. On a context
+			// cancellation, results may be truncated; we still record
+			// whatever we managed to install, with Interrupted=true so
+			// the report is self-describing.
+			if len(outcome.Results) > 0 {
+				// Prefer the snapshot's recorded manager / from-version
+				// if present (the upgrade flow wrote the snapshot with
+				// these fields populated, so this is more accurate than
+				// the CLI's inferred values).
+				reportMgr := outcome.Snapshot.Manager
+				if reportMgr == "" {
+					reportMgr = m.Name()
+				}
+				reportFrom := outcome.Snapshot.NodeVersion
+				if reportFrom == "" {
+					reportFrom = oldVersion
+				}
+				report := packages.NewMigrationReport(reportMgr, reportFrom, toInstall[len(toInstall)-1].String())
+				report.Interrupted = errors.Is(rerr, context.Canceled) || errors.Is(rerr, context.DeadlineExceeded)
+				for _, res := range outcome.Results {
+					report.AddResult(res)
+				}
+				if saveErr := report.Save(); saveErr != nil {
+					cmd.Printf("Warning: failed to write migration report: %v\n", saveErr)
+				} else if reportPath, perr := report.Path(); perr == nil {
+					if rerr != nil {
+						cmd.Printf("Migration report: %s (partial — see file for failures)\n", reportPath)
+					} else {
+						cmd.Printf("Migration report: %s\n", reportPath)
+					}
+				}
+			}
+
+			if rerr != nil {
+				cmd.Printf("Warning: restore failed: %v\n", rerr)
+			} else {
+				restoreSucceeded = true
+			}
 		}
 	}
 

internal/packages/report.go

@@ -1,6 +1,8 @@
 package packages
 
 import (
+	"crypto/rand"
+	"encoding/hex"
 	"encoding/json"
 	"fmt"
 	"os"
@@ -27,9 +29,17 @@ type MigrationReport struct {
 	ToVersion   string          `json:"to_version"`
 	Interrupted bool            `json:"interrupted"`
 	Results     []PackageResult `json:"results"`
+
+	// tag is the on-disk-filename discriminator; assigned lazily on
+	// the first Path()/Save() call. Not serialized — it's purely a
+	// filename concern, not part of the report's content.
+	tag string
 }
 
-// NewMigrationReport creates a fresh report for a migration.
+// NewMigrationReport creates a fresh report for a migration. The
+// returned report has a unique 4-byte hex tag in its Timestamp-derived
+// filename (see Path/Save) so two upgrades running within the same
+// wall-clock second can't accidentally clobber each other's report.
 func NewMigrationReport(manager, fromVersion, toVersion string) *MigrationReport {
 	return &MigrationReport{
 		Timestamp:   time.Now(),
@@ -39,24 +49,80 @@ func NewMigrationReport(manager, fromVersion, toVersion string) *MigrationReport
 	}
 }
 
-// Save writes the report to ~/.nodeup/reports/<timestamp>.json.
+// Save writes the report to <DataDir>/reports/migration-<ts>-<tag>.json.
+// The first call computes a stable 4-byte hex tag and caches it on the
+// report; subsequent calls (Path and Save) reuse the same tag, so the
+// CLI can call Path() before Save() to print the path and have it
+// match the file that Save() actually writes.
+//
+// The tag uses crypto/rand entropy, so two reports produced within the
+// same wall-clock second get distinct filenames. 32 bits of entropy is
+// plenty to make accidental collisions vanishingly unlikely across the
+// lifetime of a single machine's DataDir (~1 report per upgrade,
+// upgrades are minutes-to-hours apart for a real user).
+//
+// Save is NOT safe to call concurrently on the same report instance:
+// the tag is assigned once and shared across Path/Save, and two
+// parallel Save calls would race to write the same file. Callers
+// don't actually need concurrent saves — a single upgrade writes at
+// most one report.
 func (r *MigrationReport) Save() error {
-	dir, err := platform.ReportsDir()
+	path, err := r.Path()
 	if err != nil {
 		return err
 	}
-
-	filename := fmt.Sprintf("migration-%s.json", r.Timestamp.Format("20060102-150405"))
-	path := filepath.Join(dir, filename)
-
 	data, err := json.MarshalIndent(r, "", "  ")
 	if err != nil {
 		return fmt.Errorf("marshal report: %w", err)
 	}
-
 	return os.WriteFile(path, data, 0o644)
 }
 
+// Path returns the on-disk path Save() would write to. The first call
+// assigns a stable random suffix; subsequent calls return the same
+// filename. Exposed so the CLI can print the path to the user without
+// re-deriving the timestamp-formatting rule.
+//
+// Two different MigrationReport instances (i.e., two upgrades) get
+// independent random suffixes — Path() called on each one resolves to
+// a unique filename even within the same second.
+func (r *MigrationReport) Path() (string, error) {
+	if r.tag == "" {
+		tag, err := reportTag()
+		if err != nil {
+			return "", fmt.Errorf("generate report tag: %w", err)
+		}
+		r.tag = tag
+	}
+	dir, err := platform.ReportsDir()
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(dir, r.filename()), nil
+}
+
+// filename is the basename portion of the on-disk path. Centralized
+// so Path() and any future tests stay in lockstep with the format.
+func (r *MigrationReport) filename() string {
+	return fmt.Sprintf("migration-%s-%s.json", r.Timestamp.Format("20060102-150405"), r.tag)
+}
+
+// reportTag returns 4 hex bytes (8 chars) of crypto-random entropy
+// sourced from crypto/rand. 32 bits of entropy is plenty to make
+// accidental collisions vanishingly unlikely across the lifetime of a
+// single machine's DataDir (~1 report per upgrade, upgrades are
+// minutes-to-hours apart for a real user). We deliberately do NOT use
+// math/rand here — that would couple report-naming to Go's seeded PRNG
+// and could produce identical tags in two different processes
+// initialized identically.
+func reportTag() (string, error) {
+	var b [4]byte
+	if _, err := rand.Read(b[:]); err != nil {
+		return "", err
+	}
+	return hex.EncodeToString(b[:]), nil
+}
+
 // AddResult appends a package result to the report.
 func (r *MigrationReport) AddResult(pr PackageResult) {
 	r.Results = append(r.Results, pr)

internal/packages/sentinel_test.go

@@ -380,7 +380,7 @@ func TestRestoreFromSnapshot_ValidFile(t *testing.T) {
 	// With an empty Packages list, RestoreFromSnapshot runs the
 	// install loop zero times and returns nil — no need for a fake
 	// `npm install` binary on PATH.
-	if err := RestoreFromSnapshot(t.Context(), snapPath); err != nil {
+	if _, err := RestoreFromSnapshot(t.Context(), snapPath); err != nil {
 		t.Errorf("RestoreFromSnapshot: %v", err)
 	}
 }
@@ -389,7 +389,7 @@ func TestRestoreFromSnapshot_ValidFile(t *testing.T) {
 // includes both the path they typed AND a "read snapshot" hint so they
 // don't have to read source to figure out what went wrong.
 func TestRestoreFromSnapshot_MissingFile(t *testing.T) {
-	err := RestoreFromSnapshot(t.Context(), "/nonexistent/snapshot.json")
+	_, err := RestoreFromSnapshot(t.Context(), "/nonexistent/snapshot.json")
 	if err == nil {
 		t.Fatal("expected error for missing snapshot file")
 	}
@@ -409,7 +409,7 @@ func TestRestoreFromSnapshot_CorruptFile(t *testing.T) {
 	if err := os.WriteFile(snapPath, []byte("{not valid json"), 0o644); err != nil {
 		t.Fatalf("write: %v", err)
 	}
-	err := RestoreFromSnapshot(t.Context(), snapPath)
+	_, err := RestoreFromSnapshot(t.Context(), snapPath)
 	if err == nil {
 		t.Fatal("expected error for corrupt JSON")
 	}