Fix devbox update not refreshing locked revs for flake refs

internal/devbox/packages.go

@@ -60,7 +60,7 @@ func (d *Devbox) Outdated(ctx context.Context) (map[string]UpdateVersion, error)
 			continue
 		}
 
-		lockPackage, err := lockfile.FetchResolvedPackage(pkg.Versioned())
+		lockPackage, err := lockfile.FetchResolvedPackage(pkg.Versioned(), false)
 		if err != nil {
 			warnings = append(warnings, fmt.Sprintf("Note: unable to check updates for %s", pkg.CanonicalName()))
 			continue

internal/devbox/update.go

@@ -7,17 +7,20 @@ import (
 	"context"
 	"fmt"
 	"slices"
+	"strings"
+	"time"
 
 	"github.com/pkg/errors"
 	"go.jetify.com/devbox/internal/devbox/devopt"
 	"go.jetify.com/devbox/internal/devpkg"
+	"go.jetify.com/devbox/internal/devpkg/pkgtype"
 	"go.jetify.com/devbox/internal/lock"
 	"go.jetify.com/devbox/internal/nix"
-	"go.jetify.com/devbox/internal/nix/nixprofile"
 	"go.jetify.com/devbox/internal/plugin"
 	"go.jetify.com/devbox/internal/searcher"
 	"go.jetify.com/devbox/internal/shellgen"
 	"go.jetify.com/devbox/internal/ux"
+	"go.jetify.com/devbox/nix/flake"
 )
 
 func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error {
@@ -68,16 +71,8 @@ func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error {
 		}
 	}
 
-	for _, pkg := range pendingPackagesToUpdate {
-		if _, _, isVersioned := searcher.ParseVersionedPackage(pkg.Raw); !isVersioned {
-			if err = d.attemptToUpgradeFlake(pkg); err != nil {
-				return err
-			}
-		} else {
-			if err = d.updateDevboxPackage(pkg); err != nil {
-				return err
-			}
-		}
+	if err := d.updatePendingPackages(pendingPackagesToUpdate); err != nil {
+		return err
 	}
 
 	d.packagesBeingUpdated = inputs
@@ -124,8 +119,32 @@ func (d *Devbox) inputsToUpdate(
 	return pkgsToUpdate, nil
 }
 
+// updatePendingPackages updates the lockfile entries for each package, using
+// the right strategy per package kind. Flake refs warn-and-continue on
+// failure (see #1180 / #1840); versioned nixpkgs packages abort the update on
+// failure. Unversioned non-flake entries are left alone.
+func (d *Devbox) updatePendingPackages(pkgs []*devpkg.Package) error {
+	for _, pkg := range pkgs {
+		if pkgtype.IsFlake(pkg.Raw) {
+			if err := d.updateDevboxPackage(pkg); err != nil {
+				ux.Fwarningf(d.stderr, "Failed to update %s: %s\n", pkg.Raw, err)
+			}
+			continue
+		}
+		if _, _, isVersioned := searcher.ParseVersionedPackage(pkg.Raw); isVersioned {
+			if err := d.updateDevboxPackage(pkg); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
 func (d *Devbox) updateDevboxPackage(pkg *devpkg.Package) error {
-	resolved, err := d.lockfile.FetchResolvedPackage(pkg.Raw)
+	// refresh=true so flake refs bypass nix's own metadata cache and re-query
+	// upstream. Without this, `devbox update` on a github: ref can return a
+	// stale commit that nix had cached from an earlier call.
+	resolved, err := d.lockfile.FetchResolvedPackage(pkg.Raw, true)
 	if err != nil {
 		return err
 	}
@@ -148,6 +167,14 @@ func (d *Devbox) mergeResolvedPackageToLockfile(
 		return nil
 	}
 
+	// Flake refs have no Version, so the Version-based comparison below would
+	// always report "Already up-to-date" even when the locked rev changed.
+	// Handle them via their Resolved field (which embeds the locked rev) and
+	// LastModified.
+	if pkgtype.IsFlake(pkg.Raw) {
+		return d.mergeResolvedFlakeToLockfile(pkg, resolved, existing, lockfile)
+	}
+
 	if existing.Version != resolved.Version {
 		if existing.LastModified > resolved.LastModified {
 			ux.Fwarningf(
@@ -199,33 +226,76 @@ func (d *Devbox) mergeResolvedPackageToLockfile(
 	return nil
 }
 
-// attemptToUpgradeFlake attempts to upgrade a flake using `nix profile upgrade`
-// and prints an error if it fails, but does not propagate upgrade errors.
-func (d *Devbox) attemptToUpgradeFlake(pkg *devpkg.Package) error {
-	profilePath, err := d.profilePath()
-	if err != nil {
-		return err
+// mergeResolvedFlakeToLockfile updates the lockfile entry for a flake ref. It
+// compares on Resolved (which embeds the locked rev) rather than Version since
+// flake refs don't carry a semver. It honors the same LastModified staleness
+// guard as the nixpkgs path.
+func (d *Devbox) mergeResolvedFlakeToLockfile(
+	pkg *devpkg.Package,
+	resolved *lock.Package,
+	existing *lock.Package,
+	lockfile *lock.File,
+) error {
+	if existing.Resolved == resolved.Resolved {
+		ux.Finfof(d.stderr, "Already up-to-date %s\n", pkg)
+		return nil
 	}
 
-	ux.Finfof(
-		d.stderr,
-		"Attempting to upgrade %s using `nix profile upgrade`\n",
-		pkg.Raw,
-	)
-
-	err = nixprofile.ProfileUpgrade(profilePath, pkg, d.lockfile)
-	if err != nil {
+	// Skip the guard if either side is missing a timestamp — treat unknown as
+	// not-older so we don't block a legit update.
+	if existing.LastModified != "" && resolved.LastModified != "" &&
+		existing.LastModified > resolved.LastModified {
 		ux.Fwarningf(
 			d.stderr,
-			"Failed to upgrade %s using `nix profile upgrade`: %s\n",
-			pkg.Raw,
-			err,
+			"Resolved ref for %s has older last_modified time. Not updating\n",
+			pkg,
 		)
+		return nil
 	}
 
+	ux.Finfof(d.stderr, "Updating %s %s\n", pkg, describeFlakeUpdate(existing, resolved))
+	useResolvedPackageInLockfile(lockfile, pkg, resolved, existing)
 	return nil
 }
 
+// describeFlakeUpdate renders a short human-readable diff between two flake
+// lockfile entries. It prefers short revs when both sides have them, falls
+// back to a date range when not, and omits either piece cleanly if missing.
+func describeFlakeUpdate(existing, resolved *lock.Package) string {
+	var parts []string
+	if oldRev, newRev := shortRev(existing.Resolved), shortRev(resolved.Resolved); oldRev != "" && newRev != "" {
+		parts = append(parts, fmt.Sprintf("%s -> %s", oldRev, newRev))
+	}
+	if oldDate, newDate := shortDate(existing.LastModified), shortDate(resolved.LastModified); oldDate != "" && newDate != "" {
+		parts = append(parts, fmt.Sprintf("(%s → %s)", oldDate, newDate))
+	}
+	return strings.Join(parts, "  ")
+}
+
+// shortRev returns the first 7 chars of the locked git rev, or "" for refs
+// without one (path:, tarball:, unlocked refs).
+func shortRev(resolved string) string {
+	installable, err := flake.ParseInstallable(resolved)
+	if err != nil || installable.Ref.Rev == "" {
+		return ""
+	}
+	if len(installable.Ref.Rev) < 7 {
+		return installable.Ref.Rev
+	}
+	return installable.Ref.Rev[:7]
+}
+
+func shortDate(rfc3339 string) string {
+	if rfc3339 == "" {
+		return ""
+	}
+	t, err := time.Parse(time.RFC3339, rfc3339)
+	if err != nil {
+		return ""
+	}
+	return t.Format("2006-01-02")
+}
+
 func useResolvedPackageInLockfile(
 	lockfile *lock.File,
 	pkg *devpkg.Package,

internal/devbox/update_test.go

@@ -232,3 +232,155 @@ func currentSystem(*testing.T) string {
 	sys := nix.System() // NOTE: we could mock this too, if it helps.
 	return sys
 }
+
+func TestFlakeUpdateRewritesLockEntry(t *testing.T) {
+	devbox := devboxForTesting(t)
+
+	raw := "github:numtide/flake-utils"
+	devPkg := devpkg.PackageFromStringWithDefaults(raw, devbox.lockfile)
+	oldRev := "1111111111111111111111111111111111111111"
+	newRev := "2222222222222222222222222222222222222222"
+	existing := &lock.Package{
+		Resolved:     "github:numtide/flake-utils/" + oldRev,
+		LastModified: "2024-01-01T00:00:00Z",
+	}
+	resolved := &lock.Package{
+		Resolved:     "github:numtide/flake-utils/" + newRev,
+		LastModified: "2025-04-22T00:00:00Z",
+	}
+	lockfile := &lock.File{
+		Packages: map[string]*lock.Package{raw: existing},
+	}
+
+	err := devbox.mergeResolvedPackageToLockfile(devPkg, resolved, lockfile)
+	require.NoError(t, err)
+	require.Equal(t, "github:numtide/flake-utils/"+newRev, lockfile.Packages[raw].Resolved)
+	require.Equal(t, "2025-04-22T00:00:00Z", lockfile.Packages[raw].LastModified)
+}
+
+func TestFlakeUpdateStalenessGuardRejectsOlder(t *testing.T) {
+	devbox := devboxForTesting(t)
+
+	raw := "github:numtide/flake-utils"
+	devPkg := devpkg.PackageFromStringWithDefaults(raw, devbox.lockfile)
+	newerRev := "2222222222222222222222222222222222222222"
+	olderRev := "1111111111111111111111111111111111111111"
+	existing := &lock.Package{
+		Resolved:     "github:numtide/flake-utils/" + newerRev,
+		LastModified: "2025-04-22T00:00:00Z",
+	}
+	resolved := &lock.Package{
+		Resolved:     "github:numtide/flake-utils/" + olderRev,
+		LastModified: "2024-01-01T00:00:00Z",
+	}
+	lockfile := &lock.File{
+		Packages: map[string]*lock.Package{raw: existing},
+	}
+
+	err := devbox.mergeResolvedPackageToLockfile(devPkg, resolved, lockfile)
+	require.NoError(t, err)
+	// Entry must remain on the newer rev.
+	require.Equal(t, "github:numtide/flake-utils/"+newerRev, lockfile.Packages[raw].Resolved)
+}
+
+// Regression: the staleness guard must not trigger when resolved.LastModified
+// is empty (some nix error paths omit it). Missing == unknown, not older.
+func TestFlakeUpdateAllowsMissingResolvedLastModified(t *testing.T) {
+	devbox := devboxForTesting(t)
+
+	raw := "github:numtide/flake-utils"
+	devPkg := devpkg.PackageFromStringWithDefaults(raw, devbox.lockfile)
+	oldRev := "1111111111111111111111111111111111111111"
+	newRev := "2222222222222222222222222222222222222222"
+	existing := &lock.Package{
+		Resolved:     "github:numtide/flake-utils/" + oldRev,
+		LastModified: "2025-04-22T00:00:00Z",
+	}
+	resolved := &lock.Package{
+		Resolved: "github:numtide/flake-utils/" + newRev,
+		// LastModified deliberately empty.
+	}
+	lockfile := &lock.File{
+		Packages: map[string]*lock.Package{raw: existing},
+	}
+
+	err := devbox.mergeResolvedPackageToLockfile(devPkg, resolved, lockfile)
+	require.NoError(t, err)
+	require.Equal(t, "github:numtide/flake-utils/"+newRev, lockfile.Packages[raw].Resolved)
+}
+
+func TestFlakeUpdateNoOpWhenResolvedUnchanged(t *testing.T) {
+	devbox := devboxForTesting(t)
+
+	raw := "github:numtide/flake-utils"
+	devPkg := devpkg.PackageFromStringWithDefaults(raw, devbox.lockfile)
+	rev := "1111111111111111111111111111111111111111"
+	existing := &lock.Package{
+		Resolved:     "github:numtide/flake-utils/" + rev,
+		LastModified: "2024-01-01T00:00:00Z",
+	}
+	resolved := &lock.Package{
+		Resolved:     "github:numtide/flake-utils/" + rev,
+		LastModified: "2024-01-01T00:00:00Z",
+	}
+	lockfile := &lock.File{
+		Packages: map[string]*lock.Package{raw: existing},
+	}
+
+	err := devbox.mergeResolvedPackageToLockfile(devPkg, resolved, lockfile)
+	require.NoError(t, err)
+	require.Same(t, existing, lockfile.Packages[raw], "entry should not be replaced on no-op update")
+}
+
+func TestShortRev(t *testing.T) {
+	// GitHub refs only parse as "locked" (with a Rev) if the third path
+	// component is a 40-char hex SHA. Anything shorter is treated as a ref
+	// name, not a revision.
+	longRev := "abc1234def56789012345678901234567890abcd"
+	cases := []struct {
+		in, want string
+	}{
+		{"github:numtide/flake-utils/" + longRev + "#pkg", "abc1234"},
+		{"path:./local", ""},
+		{"", ""},
+		{"not a flake ref", ""},
+	}
+	for _, tc := range cases {
+		t.Run(tc.in, func(t *testing.T) {
+			require.Equal(t, tc.want, shortRev(tc.in))
+		})
+	}
+}
+
+func TestShortDate(t *testing.T) {
+	require.Equal(t, "2025-04-22", shortDate("2025-04-22T14:30:00Z"))
+	require.Equal(t, "", shortDate(""))
+	require.Equal(t, "", shortDate("not a date"))
+}
+
+func TestDescribeFlakeUpdateFormats(t *testing.T) {
+	oldRev := "abc1234def56789012345678901234567890abcd"
+	newRev := "f4567890123456789abcdef012345678901234ab"
+	oldPkg := &lock.Package{
+		Resolved:     "github:numtide/flake-utils/" + oldRev + "#pkg",
+		LastModified: "2024-11-01T00:00:00Z",
+	}
+	newPkg := &lock.Package{
+		Resolved:     "github:numtide/flake-utils/" + newRev + "#pkg",
+		LastModified: "2025-04-22T00:00:00Z",
+	}
+	require.Equal(t,
+		"abc1234 -> f456789  (2024-11-01 → 2025-04-22)",
+		describeFlakeUpdate(oldPkg, newPkg),
+	)
+
+	// Fallback to date-only when refs have no rev (e.g. path:).
+	oldPath := &lock.Package{Resolved: "path:./x", LastModified: "2024-11-01T00:00:00Z"}
+	newPath := &lock.Package{Resolved: "path:./x", LastModified: "2025-04-22T00:00:00Z"}
+	require.Equal(t, "(2024-11-01 → 2025-04-22)", describeFlakeUpdate(oldPath, newPath))
+
+	// Fallback to rev-only when dates missing.
+	oldNoDate := &lock.Package{Resolved: "github:numtide/flake-utils/" + oldRev + "#pkg"}
+	newNoDate := &lock.Package{Resolved: "github:numtide/flake-utils/" + newRev + "#pkg"}
+	require.Equal(t, "abc1234 -> f456789", describeFlakeUpdate(oldNoDate, newNoDate))
+}

internal/lock/lockfile.go

@@ -82,7 +82,7 @@ func (f *File) Resolve(pkg string) (*Package, error) {
 	locked := &Package{}
 	_, _, versioned := searcher.ParseVersionedPackage(pkg)
 	if pkgtype.IsRunX(pkg) || versioned || pkgtype.IsFlake(pkg) {
-		resolved, err := f.FetchResolvedPackage(pkg)
+		resolved, err := f.FetchResolvedPackage(pkg, false)
 		if err != nil {
 			return nil, err
 		}