[shellenv] fix PATH nesting for devbox-project and devbox-global (#1508)

Author: savil

internal/boxcli/shellenv.go

@@ -15,10 +15,11 @@ import (
 
 type shellEnvCmdFlags struct {
 	envFlag
-	config      configFlags
-	runInitHook bool
-	install     bool
-	pure        bool
+	config            configFlags
+	runInitHook       bool
+	install           bool
+	pure              bool
+	preservePathStack bool
 }
 
 func shellEnvCmd() *cobra.Command {
@@ -48,6 +49,11 @@ func shellEnvCmd() *cobra.Command {
 
 	command.Flags().BoolVar(
 		&flags.pure, "pure", false, "If this flag is specified, devbox creates an isolated environment inheriting almost no variables from the current environment. A few variables, in particular HOME, USER and DISPLAY, are retained.")
+	command.Flags().BoolVar(
+		&flags.preservePathStack, "preserve-path-stack", false,
+		"Preserves existing PATH order if this project's environment is already in PATH. "+
+			"Useful if you want to avoid overshadowing another devbox project that is already active")
+	_ = command.Flags().MarkHidden("preserve-path-stack")
 
 	flags.config.register(command)
 	flags.envFlag.register(command)
@@ -63,10 +69,11 @@ func shellEnvFunc(cmd *cobra.Command, flags shellEnvCmdFlags) (string, error) {
 		return "", err
 	}
 	box, err := devbox.Open(&devopt.Opts{
-		Dir:    flags.config.path,
-		Stderr: cmd.ErrOrStderr(),
-		Pure:   flags.pure,
-		Env:    env,
+		Dir:               flags.config.path,
+		Stderr:            cmd.ErrOrStderr(),
+		PreservePathStack: flags.preservePathStack,
+		Pure:              flags.pure,
+		Env:               env,
 	})
 	if err != nil {
 		return "", err

internal/envir/util.go

@@ -5,7 +5,9 @@ package envir
 
 import (
 	"os"
+	"slices"
 	"strconv"
+	"strings"
 )
 
 func IsDevboxCloud() bool {
@@ -43,3 +45,31 @@ func GetValueOrDefault(key, def string) string {
 
 	return val
 }
+
+// MapToPairs creates a slice of environment variable "key=value" pairs from a
+// map.
+func MapToPairs(m map[string]string) []string {
+	pairs := make([]string, len(m))
+	i := 0
+	for k, v := range m {
+		pairs[i] = k + "=" + v
+		i++
+	}
+	slices.Sort(pairs) // for reproducibility
+	return pairs
+}
+
+// PairsToMap creates a map from a slice of "key=value" environment variable
+// pairs. Note that maps are not ordered, which can affect the final variable
+// values when pairs contains duplicate keys.
+func PairsToMap(pairs []string) map[string]string {
+	vars := make(map[string]string, len(pairs))
+	for _, p := range pairs {
+		k, v, ok := strings.Cut(p, "=")
+		if !ok {
+			continue
+		}
+		vars[k] = v
+	}
+	return vars
+}

internal/impl/devbox.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"io"
 	"io/fs"
+	"maps"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -21,6 +22,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/samber/lo"
 	"go.jetpack.io/devbox/internal/devpkg"
+	"go.jetpack.io/devbox/internal/impl/envpath"
 	"go.jetpack.io/devbox/internal/impl/generate"
 	"go.jetpack.io/devbox/internal/searcher"
 	"go.jetpack.io/devbox/internal/shellgen"
@@ -59,6 +61,7 @@ type Devbox struct {
 	nix                      nix.Nixer
 	projectDir               string
 	pluginManager            *plugin.Manager
+	preservePathStack        bool
 	pure                     bool
 	allowInsecureAdds        bool
 	customProcessComposeFile string
@@ -89,6 +92,7 @@ func Open(opts *devopt.Opts) (*Devbox, error) {
 		projectDir:               projectDir,
 		pluginManager:            plugin.NewManager(),
 		stderr:                   opts.Stderr,
+		preservePathStack:        opts.PreservePathStack,
 		pure:                     opts.Pure,
 		customProcessComposeFile: opts.CustomProcessComposeFile,
 		allowInsecureAdds:        opts.AllowInsecureAdds,
@@ -317,7 +321,7 @@ func (d *Devbox) EnvVars(ctx context.Context) ([]string, error) {
 	if err != nil {
 		return nil, err
 	}
-	return mapToPairs(envs), nil
+	return envir.MapToPairs(envs), nil
 }
 
 func (d *Devbox) ShellEnvHash(ctx context.Context) (string, error) {
@@ -782,18 +786,10 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m
 		}
 	}
 
-	currentEnvPath := env["PATH"]
-	debug.Log("current environment PATH is: %s", currentEnvPath)
-	// Use the original path, if available. If not available, set it for future calls.
-	// See https://github.com/jetpack-io/devbox/issues/687
-	// We add the project dir hash to ensure that we don't have conflicts
-	// between different projects (including global)
-	// (moving a project would change the hash and that's fine)
-	originalPath, ok := env[d.ogPathKey()]
-	if !ok {
-		env[d.ogPathKey()] = currentEnvPath
-		originalPath = currentEnvPath
-	}
+	debug.Log("current environment PATH is: %s", env["PATH"])
+
+	originalEnv := make(map[string]string, len(env))
+	maps.Copy(originalEnv, env)
 
 	vaf, err := d.nix.PrintDevEnv(ctx, &nix.PrintDevEnvArgs{
 		FlakesFilePath:       d.nixFlakesFilePath(),
@@ -853,13 +849,13 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m
 
 	addEnvIfNotPreviouslySetByDevbox(env, pluginEnv)
 
-	env["PATH"] = JoinPathLists(
+	env["PATH"] = envpath.JoinPathLists(
 		nix.ProfileBinPath(d.projectDir),
 		env["PATH"],
 	)
 
 	if !d.OmitBinWrappersFromPath {
-		env["PATH"] = JoinPathLists(
+		env["PATH"] = envpath.JoinPathLists(
 			filepath.Join(d.projectDir, plugin.WrapperBinPath),
 			env["PATH"],
 		)
@@ -899,14 +895,18 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m
 	})
 	debug.Log("PATH after filtering with buildInputs (%v) is: %s", buildInputs, nixEnvPath)
 
-	env["PATH"] = JoinPathLists(nixEnvPath, originalPath)
+	pathStack := envpath.Stack(env, originalEnv)
+	pathStack.Push(env, d.projectDirHash(), nixEnvPath, d.preservePathStack)
+	env["PATH"] = pathStack.Path(env)
+	debug.Log("New path stack is: %s", pathStack)
+
 	debug.Log("computed environment PATH is: %s", env["PATH"])
 
 	d.setCommonHelperEnvVars(env)
 
 	if !d.pure {
 		// preserve the original XDG_DATA_DIRS by prepending to it
-		env["XDG_DATA_DIRS"] = JoinPathLists(env["XDG_DATA_DIRS"], os.Getenv("XDG_DATA_DIRS"))
+		env["XDG_DATA_DIRS"] = envpath.JoinPathLists(env["XDG_DATA_DIRS"], os.Getenv("XDG_DATA_DIRS"))
 	}
 
 	for k, v := range d.env {
@@ -941,10 +941,6 @@ func (d *Devbox) nixEnv(ctx context.Context) (map[string]string, error) {
 	return d.computeNixEnv(ctx, usePrintDevEnvCache)
 }
 
-func (d *Devbox) ogPathKey() string {
-	return "DEVBOX_OG_PATH_" + d.projectDirHash()
-}
-
 func (d *Devbox) nixPrintDevEnvCachePath() string {
 	return filepath.Join(d.projectDir, ".devbox/.nix-print-dev-env-cache")
 }
@@ -1120,8 +1116,8 @@ var ignoreDevEnvVar = map[string]bool{
 // common setups (e.g. gradio, rust)
 func (d *Devbox) setCommonHelperEnvVars(env map[string]string) {
 	profileLibDir := filepath.Join(d.projectDir, nix.ProfilePath, "lib")
-	env["LD_LIBRARY_PATH"] = JoinPathLists(profileLibDir, env["LD_LIBRARY_PATH"])
-	env["LIBRARY_PATH"] = JoinPathLists(profileLibDir, env["LIBRARY_PATH"])
+	env["LD_LIBRARY_PATH"] = envpath.JoinPathLists(profileLibDir, env["LD_LIBRARY_PATH"])
+	env["LIBRARY_PATH"] = envpath.JoinPathLists(profileLibDir, env["LIBRARY_PATH"])
 }
 
 // NixBins returns the paths to all the nix binaries that are installed by
@@ -1197,7 +1193,7 @@ func (d *Devbox) parseEnvAndExcludeSpecialCases(currentEnv []string) (map[string
 			return nil, err
 		}
 		includedInPath = append(includedInPath, dotdevboxBinPath(d))
-		env["PATH"] = JoinPathLists(includedInPath...)
+		env["PATH"] = envpath.JoinPathLists(includedInPath...)
 	}
 	return env, nil
 }

internal/impl/devbox_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/bmatcuk/doublestar/v4"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.jetpack.io/devbox/internal/impl/envpath"
 
 	"go.jetpack.io/devbox/internal/devconfig"
 	"go.jetpack.io/devbox/internal/envir"
@@ -85,10 +86,9 @@ func TestComputeNixPathIsIdempotent(t *testing.T) {
 	assert.NotEmpty(t, path, "path should not be nil")
 
 	t.Setenv("PATH", path)
-	t.Setenv(
-		"DEVBOX_OG_PATH_"+devbox.projectDirHash(),
-		env["DEVBOX_OG_PATH_"+devbox.projectDirHash()],
-	)
+	t.Setenv(envpath.InitPathEnv, env[envpath.InitPathEnv])
+	t.Setenv(envpath.PathStackEnv, env[envpath.PathStackEnv])
+	t.Setenv(envpath.Key(devbox.projectDirHash()), env[envpath.Key(devbox.projectDirHash())])
 
 	env, err = devbox.computeNixEnv(ctx, false /*use cache*/)
 	require.NoError(t, err, "computeNixEnv should not fail")
@@ -108,10 +108,9 @@ func TestComputeNixPathWhenRemoving(t *testing.T) {
 	assert.Contains(t, path, "/tmp/my/path", "path should contain /tmp/my/path")
 
 	t.Setenv("PATH", path)
-	t.Setenv(
-		"DEVBOX_OG_PATH_"+devbox.projectDirHash(),
-		env["DEVBOX_OG_PATH_"+devbox.projectDirHash()],
-	)
+	t.Setenv(envpath.InitPathEnv, env[envpath.InitPathEnv])
+	t.Setenv(envpath.PathStackEnv, env[envpath.PathStackEnv])
+	t.Setenv(envpath.Key(devbox.projectDirHash()), env[envpath.Key(devbox.projectDirHash())])
 
 	devbox.nix.(*testNix).path = ""
 	env, err = devbox.computeNixEnv(ctx, false /*use cache*/)

internal/impl/devopt/devboxopts.go

@@ -8,6 +8,7 @@ type Opts struct {
 	AllowInsecureAdds        bool
 	Dir                      string
 	Env                      map[string]string
+	PreservePathStack        bool
 	Pure                     bool
 	IgnoreWarnings           bool
 	CustomProcessComposeFile string

internal/impl/envpath/pathlists.go

@@ -0,0 +1,37 @@
+package envpath
+
+import (
+	"path/filepath"
+	"strings"
+)
+
+// JoinPathLists joins and cleans PATH-style strings of
+// [os.ListSeparator] delimited paths. To clean a path list, it splits it and
+// does the following for each element:
+//
+//  1. Applies [filepath.Clean].
+//  2. Removes the path if it's relative (must begin with '/' and not be '.').
+//  3. Removes the path if it's a duplicate.
+func JoinPathLists(pathLists ...string) string {
+	if len(pathLists) == 0 {
+		return ""
+	}
+
+	seen := make(map[string]bool)
+	var cleaned []string
+	for _, path := range pathLists {
+		for _, path := range filepath.SplitList(path) {
+			path = filepath.Clean(path)
+			if path == "." || path[0] != '/' {
+				// Remove empty paths and don't allow relative
+				// paths for security reasons.
+				continue
+			}
+			if !seen[path] {
+				cleaned = append(cleaned, path)
+			}
+			seen[path] = true
+		}
+	}
+	return strings.Join(cleaned, string(filepath.ListSeparator))
+}

internal/impl/envpath/pathlists_test.go

@@ -0,0 +1,32 @@
+package envpath
+
+import (
+	"testing"
+)
+
+func TestCleanEnvPath(t *testing.T) {
+	tests := []struct {
+		name    string
+		inPath  string
+		outPath string
+	}{
+		{
+			name:    "NoEmptyPaths",
+			inPath:  "/usr/local/bin::",
+			outPath: "/usr/local/bin",
+		},
+		{
+			name:    "NoRelativePaths",
+			inPath:  "/usr/local/bin:/usr/bin:../test:/bin:/usr/sbin:/sbin:.:..",
+			outPath: "/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin",
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got := JoinPathLists(test.inPath)
+			if got != test.outPath {
+				t.Errorf("Got incorrect cleaned PATH.\ngot:  %s\nwant: %s", got, test.outPath)
+			}
+		})
+	}
+}