fix(lock,devpkg,nix): fix package parsing, flake input names, and lockfile resolve bugs

internal/devbox/packages.go

@@ -63,7 +63,9 @@ func (d *Devbox) Outdated(ctx context.Context) (map[string]UpdateVersion, error)
 			continue
 		}
 		existingLockPackage := lockfile.Packages[pkg.Raw]
-		if lockPackage.Version == existingLockPackage.Version {
+		// Packages that aren't locked yet will be locked to the latest version
+		// on the next install, so they aren't outdated.
+		if existingLockPackage == nil || lockPackage.Version == existingLockPackage.Version {
 			continue
 		}
 

internal/devpkg/package.go

@@ -25,6 +25,7 @@ import (
 	"go.jetify.com/devbox/internal/devpkg/pkgtype"
 	"go.jetify.com/devbox/internal/lock"
 	"go.jetify.com/devbox/internal/nix"
+	"go.jetify.com/devbox/internal/searcher"
 	"go.jetify.com/devbox/internal/ux"
 	"go.jetify.com/devbox/nix/flake"
 	"go.jetify.com/devbox/plugins"
@@ -159,7 +160,7 @@ func newPackage(raw string, isInstallable func() bool, locker lock.Locker) *Pack
 		return resolve(pkg)
 	})
 	pkg.setInstallable(parsed, locker.ProjectDir())
-	pkg.outputs = outputs{selectedNames: strings.Split(parsed.Outputs, ",")}
+	pkg.outputs = outputs{selectedNames: lo.Compact(strings.Split(parsed.Outputs, ","))}
 	pkg.Patch = pkgNeedsPatch(pkg.CanonicalName(), configfile.PatchAuto)
 	return pkg
 }
@@ -232,6 +233,11 @@ func (p *Package) FlakeInputName() string {
 			} else if p.installable.Ref.Ref != "" {
 				result += "-" + p.installable.Ref.Ref
 			}
+			// Disambiguate flakes in different subdirectories or hosts of
+			// the same repo.
+			if p.installable.Ref.Dir != "" || p.installable.Ref.Host != "" {
+				result += "-" + cachehash.Bytes6([]byte(p.installable.Ref.String()))
+			}
 		}
 	default:
 		result = p.installable.Ref.String() + "-" + p.Hash()
@@ -501,8 +507,10 @@ func (p *Package) CanonicalName() string {
 	if !p.IsDevboxPackage {
 		return ""
 	}
-	name, _, _ := strings.Cut(p.Raw, "@")
-	return name
+	if name, _, ok := searcher.ParseVersionedPackage(p.Raw); ok {
+		return name
+	}
+	return p.Raw
 }
 
 func (p *Package) Versioned() string {
@@ -560,12 +568,13 @@ func (p *Package) version() string {
 	if !p.IsDevboxPackage {
 		return ""
 	}
-	_, version, _ := strings.Cut(p.Raw, "@")
+	_, version, _ := searcher.ParseVersionedPackage(p.Raw)
 	return version
 }
 
 func (p *Package) isVersioned() bool {
-	return p.IsDevboxPackage && strings.Contains(p.Raw, "@")
+	_, _, versioned := searcher.ParseVersionedPackage(p.Raw)
+	return p.IsDevboxPackage && versioned
 }
 
 func (p *Package) HashFromNixPkgsURL() string {

internal/devpkg/package_test.go

@@ -6,10 +6,12 @@ package devpkg
 import (
 	"fmt"
 	"path/filepath"
+	"slices"
 	"strings"
 	"testing"
 
 	"github.com/samber/lo"
+	"go.jetify.com/devbox/internal/devbox/devopt"
 	"go.jetify.com/devbox/internal/lock"
 	"go.jetify.com/devbox/internal/nix"
 	"go.jetify.com/devbox/nix/flake"
@@ -97,6 +99,23 @@ func TestInput(t *testing.T) {
 	}
 }
 
+func TestFlakeInputNameUnique(t *testing.T) {
+	pkgs := []string{
+		"github:owner/repo/abc123#x",
+		"github:owner/repo/abc123?dir=a#x",
+		"github:owner/repo/abc123?dir=b#y",
+		"github:owner/repo/abc123?host=git.example.com#x",
+	}
+	seen := map[string]string{}
+	for _, pkg := range pkgs {
+		name := testInputFromString(pkg, "/tmp/my-project").FlakeInputName()
+		if other, ok := seen[name]; ok {
+			t.Errorf("FlakeInputName() = %q for both %q and %q", name, other, pkg)
+		}
+		seen[name] = pkg
+	}
+}
+
 type testInput struct {
 	*Package
 }
@@ -194,6 +213,8 @@ func TestCanonicalName(t *testing.T) {
 		{"go", "go"},
 		{"go@latest", "go"},
 		{"go@1.21", "go"},
+		{"nodePackages.@vue/cli@5.0", "nodePackages.@vue/cli"},
+		{"emacsPackages.@", "emacsPackages.@"},
 		{"runx:golangci/golangci-lint@latest", "runx:golangci/golangci-lint"},
 		{"runx:golangci/golangci-lint@v0.0.2", "runx:golangci/golangci-lint"},
 		{"runx:golangci/golangci-lint", "runx:golangci/golangci-lint"},
@@ -211,3 +232,48 @@ func TestCanonicalName(t *testing.T) {
 		})
 	}
 }
+
+func TestVersioned(t *testing.T) {
+	tests := []struct {
+		pkgName   string
+		version   string
+		versioned string
+	}{
+		{"go", "", "go@latest"},
+		{"go@1.21", "1.21", "go@1.21"},
+		{"nodePackages.@vue/cli@5.0", "5.0", "nodePackages.@vue/cli@5.0"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.pkgName, func(t *testing.T) {
+			pkg := PackageFromStringWithDefaults(tt.pkgName, &lockfile{})
+			if got := pkg.version(); got != tt.version {
+				t.Errorf("version() = %q, want %q", got, tt.version)
+			}
+			if got := pkg.Versioned(); got != tt.versioned {
+				t.Errorf("Versioned() = %q, want %q", got, tt.versioned)
+			}
+		})
+	}
+}
+
+func TestFlakeOutputNames(t *testing.T) {
+	tests := []struct {
+		pkg     string
+		opts    []string
+		outputs []string
+	}{
+		{"github:NixOS/nixpkgs/12345#curl", nil, []string{}},
+		{"github:NixOS/nixpkgs/12345#curl", []string{"dev", "out"}, []string{"dev", "out"}},
+		{"github:NixOS/nixpkgs/12345#curl^bin,dev", nil, []string{"bin", "dev"}},
+		{"github:NixOS/nixpkgs/12345#curl^bin", []string{"dev"}, []string{"bin", "dev"}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.pkg, func(t *testing.T) {
+			pkg := PackageFromStringWithOptions(tt.pkg, &lockfile{}, devopt.AddOpts{Outputs: tt.opts})
+			got := pkg.outputs.selectedNames
+			if !slices.Equal(got, tt.outputs) {
+				t.Errorf("got outputs %q, want %q", got, tt.outputs)
+			}
+		})
+	}
+}

internal/lock/lockfile.go

@@ -145,10 +145,18 @@ func (f *File) UpdateStdenv() error {
 	if err := nix.ClearFlakeCache(f.devboxProject.Stdenv()); err != nil {
 		return err
 	}
-	if err := f.Remove(f.devboxProject.Stdenv().String()); err != nil {
+	// Remove the old entry in memory only so that a failed resolve doesn't
+	// leave the lockfile without a stdenv.
+	stdenv := f.devboxProject.Stdenv().String()
+	old, hasOld := f.Packages[stdenv]
+	delete(f.Packages, stdenv)
+	if err := f.Add(stdenv); err != nil {
+		if hasOld {
+			f.Packages[stdenv] = old
+		}
 		return err
 	}
-	return f.Add(f.devboxProject.Stdenv().String())
+	return nil
 }
 
 // TODO: We should improve a few issues with this function:

internal/lock/resolve.go

@@ -42,10 +42,11 @@ func (f *File) FetchResolvedPackage(pkg string, refresh bool) (*Package, error)
 		if err != nil {
 			return nil, err
 		}
-		return &Package{
-			Resolved:     installable.String(),
-			LastModified: time.Unix(installable.Ref.LastModified, 0).UTC().Format(time.RFC3339),
-		}, nil
+		locked := &Package{Resolved: installable.String()}
+		if installable.Ref.LastModified != 0 {
+			locked.LastModified = time.Unix(installable.Ref.LastModified, 0).UTC().Format(time.RFC3339)
+		}
+		return locked, nil
 	}
 
 	name, version, _ := searcher.ParseVersionedPackage(pkg)
@@ -68,9 +69,12 @@ func (f *File) FetchResolvedPackage(pkg string, refresh bool) (*Package, error)
 	}
 
 	packageVersion, err := searcher.Client().Resolve(name, version)
-	if err != nil {
+	if errors.Is(err, searcher.ErrNotFound) {
 		return nil, errors.Wrapf(nix.ErrPackageNotFound, "%s@%s", name, version)
 	}
+	if err != nil {
+		return nil, err
+	}
 
 	sysInfos, err := buildLockSystemInfos(packageVersion)
 	if err != nil {
@@ -220,9 +224,9 @@ func lockFlake(ctx context.Context, ref flake.Ref, refresh bool) (flake.Ref, err
 	// file is a bit more lenient and only requires a revision so that we
 	// don't need to download the nixpkgs source for cached packages. If the
 	// search index is ever able to return the NAR hash then we can remove
-	// this check. Skip this shortcut when refresh is requested — the caller
-	// is asking us to re-query upstream for a newer rev.
-	if !refresh && ref.Type == flake.TypeGitHub && (ref.Rev != "") {
+	// this check. A pinned rev can't change, so this also applies when
+	// refresh is requested.
+	if ref.Type == flake.TypeGitHub && ref.Rev != "" {
 		return ref, nil
 	}
 

internal/lock/resolve_test.go

@@ -0,0 +1,58 @@
+// Copyright 2024 Jetify Inc. and contributors. All rights reserved.
+// Use of this source code is governed by the license in the LICENSE file.
+
+package lock
+
+import (
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"go.jetify.com/devbox/internal/envir"
+	"go.jetify.com/devbox/internal/nix"
+)
+
+func TestFetchResolvedPackageSearchErrors(t *testing.T) {
+	tests := []struct {
+		status       int
+		wantNotFound bool
+	}{
+		{http.StatusNotFound, true},
+		{http.StatusInternalServerError, false},
+	}
+	for _, test := range tests {
+		t.Run(http.StatusText(test.status), func(t *testing.T) {
+			srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.WriteHeader(test.status)
+			}))
+			defer srv.Close()
+			t.Setenv(envir.DevboxSearchHost, srv.URL)
+			t.Setenv("DEVBOX_FEATURE_RESOLVE_V2", "0")
+
+			_, err := (&File{}).FetchResolvedPackage("hello@1.0", false)
+			if err == nil {
+				t.Fatal("got nil error")
+			}
+			if got := errors.Is(err, nix.ErrPackageNotFound); got != test.wantNotFound {
+				t.Errorf("errors.Is(err, ErrPackageNotFound) = %v, want %v (err: %v)", got, test.wantNotFound, err)
+			}
+		})
+	}
+}
+
+func TestFetchResolvedPackagePinnedGitHubFlake(t *testing.T) {
+	const pkg = "github:owner/repo/1111111111111111111111111111111111111111#hello"
+	for _, refresh := range []bool{false, true} {
+		got, err := (&File{}).FetchResolvedPackage(pkg, refresh)
+		if err != nil {
+			t.Fatalf("refresh=%v: got error: %v", refresh, err)
+		}
+		if got.Resolved != pkg {
+			t.Errorf("refresh=%v: got Resolved %q, want %q", refresh, got.Resolved, pkg)
+		}
+		if got.LastModified != "" {
+			t.Errorf("refresh=%v: got LastModified %q, want empty", refresh, got.LastModified)
+		}
+	}
+}

internal/nix/config.go

@@ -56,6 +56,10 @@ func (c Config) IsUserTrusted(ctx context.Context, username string) (bool, error
 	if len(trusted) == 0 {
 		return false, nil
 	}
+	// A "*" entry trusts all users.
+	if slices.Contains(trusted, "*") {
+		return true, nil
+	}
 
 	current, err := user.Lookup(username)
 	if err != nil {

internal/nix/config_test.go

@@ -29,6 +29,16 @@ func TestConfigIsUserTrusted(t *testing.T) {
 			t.Error("got trusted = false, want true")
 		}
 	})
+	t.Run("Wildcard", func(t *testing.T) {
+		cfg := Config{TrustedUsers: ConfigField[[]string]{Value: []string{"*"}}}
+		trusted, err := cfg.IsUserTrusted(t.Context(), curUser.Username)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if !trusted {
+			t.Error("got trusted = false, want true")
+		}
+	})
 	t.Run("UserGroupInList", func(t *testing.T) {
 		g, err := user.LookupGroupId(curUser.Gid)
 		if err != nil {

internal/nix/nix.go

@@ -59,7 +59,9 @@ func (*NixInstance) PrintDevEnv(ctx context.Context, args *PrintDevEnvArgs) (*Pr
 		data, err = os.ReadFile(args.PrintDevEnvCachePath)
 		if err == nil {
 			if err := json.Unmarshal(data, &out); err != nil {
-				return nil, errors.WithStack(err)
+				// The cache may be truncated or corrupt, so recompute it.
+				slog.Debug("ignoring invalid print-dev-env cache", "err", err)
+				data, out = nil, PrintDevEnvOut{}
 			}
 		} else if !errors.Is(err, fs.ErrNotExist) {
 			return nil, errors.WithStack(err)

internal/nix/store.go

@@ -161,7 +161,7 @@ func DaemonVersion(ctx context.Context) (string, error) {
 	}
 
 	if len(out) == 0 {
-		return "", redact.Errorf("command %s: empty output", redact.Safe(cmd), err)
+		return "", redact.Errorf("command %s: empty output", redact.Safe(cmd))
 	}
 	if canJSON {
 		info := struct{ Version string }{}

internal/nix/storepath.go

@@ -26,13 +26,20 @@ func NewStorePathParts(path string) StorePathParts {
 
 	hash, name := path[:32], path[33:]
 	dashIndex := 0
-	for i, r := range name {
-		if dashIndex != 0 && !unicode.IsLetter(r) {
-			version, output, _ := strings.Cut(name[i:], "-")
+	for i, char := range name {
+		if dashIndex != 0 && !unicode.IsLetter(char) {
+			// Versions may contain dashes (2.39-52, 0-unstable-2024-01-01),
+			// so only treat the last component as the output if it starts
+			// with a letter.
+			version, output := name[i:], ""
+			if j := strings.LastIndex(version, "-"); j != -1 && j+1 < len(version) &&
+				unicode.IsLetter(rune(version[j+1])) {
+				version, output = version[:j], version[j+1:]
+			}
 			return StorePathParts{Hash: hash, Name: name[:dashIndex], Version: version, Output: output}
 		}
 		dashIndex = 0
-		if r == '-' {
+		if char == '-' {
 			dashIndex = i
 		}
 	}

internal/nix/storepath_test.go

@@ -45,6 +45,35 @@ func TestStorePathParts(t *testing.T) {
 				Output:  "bar",
 			},
 		},
+		// version with dashes and an output
+		{
+			storePath: "/nix/store/0z1zq1zq1zq1zq1zq1zq1zq1zq1zq1zq-glibc-2.39-52-bin",
+			expected: StorePathParts{
+				Hash:    "0z1zq1zq1zq1zq1zq1zq1zq1zq1zq1zq",
+				Name:    "glibc",
+				Version: "2.39-52",
+				Output:  "bin",
+			},
+		},
+		// version with dashes and no output
+		{
+			storePath: "/nix/store/0z1zq1zq1zq1zq1zq1zq1zq1zq1zq1zq-foo-0-unstable-2024-01-01",
+			expected: StorePathParts{
+				Hash:    "0z1zq1zq1zq1zq1zq1zq1zq1zq1zq1zq",
+				Name:    "foo",
+				Version: "0-unstable-2024-01-01",
+			},
+		},
+		// unstable version with an output
+		{
+			storePath: "/nix/store/0z1zq1zq1zq1zq1zq1zq1zq1zq1zq1zq-foo-0-unstable-2024-01-01-dev",
+			expected: StorePathParts{
+				Hash:    "0z1zq1zq1zq1zq1zq1zq1zq1zq1zq1zq",
+				Name:    "foo",
+				Version: "0-unstable-2024-01-01",
+				Output:  "dev",
+			},
+		},
 	}
 
 	for _, testCase := range testCases {