Merge pull request #19 from crunchloop/honor-user-env-probe

engine: honor userEnvProbe in Engine.Exec

Author: bilby91

attach.go

@@ -63,10 +63,18 @@ func (e *Engine) AttachWith(ctx context.Context, id WorkspaceID, opts AttachOpti
 		localEnv = environAsMap(os.Environ())
 	}
 
-	return &Workspace{
+	ws := &Workspace{
 		ID:        id,
 		Config:    cfg,
 		Container: details,
 		subst:     newSubstituter(cfg, details, localEnv),
-	}, nil
+	}
+
+	// Re-probe on attach so subsequent Exec calls see PATH additions
+	// from the user's rc files. The original Up populated probedEnv,
+	// but a fresh Attach doesn't share that workspace value.
+	if probed, err := e.probeUserEnv(ctx, ws, cfg.UserEnvProbe); err == nil {
+		ws.probedEnv = probed
+	}
+	return ws, nil
 }

engine_test.go

@@ -376,6 +376,9 @@ func TestExec_SubstitutesContainerEnv(t *testing.T) {
 		t.Fatalf("Up: %v", err)
 	}
 
+	// Up performs a userEnvProbe exec; clear the recorded calls so this
+	// test only asserts on the user-driven Exec.
+	rt.execCalls = nil
 	rt.execResult = runtime.ExecResult{ExitCode: 0, Stdout: "ok"}
 	_, err = eng.Exec(context.Background(), wsObj, ExecOptions{
 		Cmd: []string{"echo", "${containerEnv:HOME}"},

exec.go

@@ -20,6 +20,21 @@ type ExecOptions struct {
 	Stdin      io.Reader
 	Stdout     io.Writer
 	Stderr     io.Writer
+
+	// SkipUserEnvProbe, when true, makes Exec bypass the merge of
+	// probedEnv AND cfg.RemoteEnv into the process environment.
+	// Only opts.Env (after substitution) plus whatever the runtime
+	// inherits from the container reaches the exec'd process.
+	//
+	// Default false: every Exec inherits both probedEnv (so PATH and
+	// other vars set by the user's rc files are visible — nvm/asdf/
+	// feature-installed tools just work) and RemoteEnv (the
+	// devcontainer.json author's declared env).
+	//
+	// Set this for callers that need a clean, deterministic
+	// environment — internal probes, low-level fs operations,
+	// DiscoverPath-style helpers that read raw container env.
+	SkipUserEnvProbe bool
 }
 
 // ExecResult is the outcome of Engine.Exec.
@@ -48,6 +63,11 @@ func (e *Engine) Exec(ctx context.Context, ws *Workspace, opts ExecOptions) (Exe
 	user, _ := ws.subst.String(opts.User)
 	wd, _ := ws.subst.String(opts.WorkingDir)
 
+	if !opts.SkipUserEnvProbe {
+		remoteEnv, _ := ws.subst.Map(ws.Config.RemoteEnv)
+		env = mergeProbedEnv(ws.probedEnv, remoteEnv, env, effectiveUser(ws.Config))
+	}
+
 	res, err := e.runtime.ExecContainer(ctx, ws.Container.ID, runtime.ExecOptions{
 		Cmd:        cmd,
 		Env:        env,

test/integration/userenvprobe_test.go

@@ -0,0 +1,226 @@
+//go:build integration
+
+package integration
+
+import (
+	"context"
+	"strings"
+	"testing"
+	"time"
+
+	devcontainer "github.com/crunchloop/devcontainer"
+)
+
+// bashImage is a tiny image with bash available so the userEnvProbe's
+// `bash -lic` invocation succeeds. plain alpine:3.20 ships only ash;
+// the probe falls back to printenv there, exercised in
+// TestUserEnvProbe_NoBashFallback.
+const bashImage = "bash:5.2-alpine3.20"
+
+// TestUserEnvProbe_PathFromBashrc covers the original bug: a
+// postCreateCommand-installed tool writes its PATH addition to ~/.bashrc;
+// later Exec calls must see that PATH without per-call shell wrapping.
+func TestUserEnvProbe_PathFromBashrc(t *testing.T) {
+	if testing.Short() {
+		t.Skip()
+	}
+	eng, rt := newEngine(t)
+	defer rt.Close()
+
+	ws := writeWorkspace(t, `{
+		"image": "`+bashImage+`",
+		"postCreateCommand": "echo 'export EXTRA_PATH=/from/bashrc' > /etc/profile.d/dc-go-test.sh"
+	}`)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+	defer cancel()
+
+	wsObj, err := eng.Up(ctx, devcontainer.UpOptions{
+		LocalWorkspaceFolder: ws,
+		Recreate:             true,
+	})
+	if err != nil {
+		t.Fatalf("Up: %v", err)
+	}
+	defer func() { _ = eng.Down(context.Background(), wsObj, devcontainer.DownOptions{Remove: true}) }()
+
+	res, err := eng.Exec(ctx, wsObj, devcontainer.ExecOptions{
+		Cmd: []string{"printenv", "EXTRA_PATH"},
+	})
+	if err != nil {
+		t.Fatalf("Exec: %v", err)
+	}
+	if res.ExitCode != 0 {
+		t.Fatalf("printenv EXTRA_PATH exit=%d stderr=%q", res.ExitCode, res.Stderr)
+	}
+	if got := strings.TrimSpace(res.Stdout); got != "/from/bashrc" {
+		t.Errorf("EXTRA_PATH = %q, want %q (probedEnv didn't inject from .bashrc)", got, "/from/bashrc")
+	}
+}
+
+// TestUserEnvProbe_None disables probing via devcontainer.json. The
+// .bashrc-exported var should NOT be visible to subsequent execs.
+func TestUserEnvProbe_None(t *testing.T) {
+	if testing.Short() {
+		t.Skip()
+	}
+	eng, rt := newEngine(t)
+	defer rt.Close()
+
+	ws := writeWorkspace(t, `{
+		"image": "`+bashImage+`",
+		"userEnvProbe": "none",
+		"postCreateCommand": "echo 'export EXTRA_PATH=/from/bashrc' > /etc/profile.d/dc-go-test.sh"
+	}`)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+	defer cancel()
+
+	wsObj, err := eng.Up(ctx, devcontainer.UpOptions{
+		LocalWorkspaceFolder: ws,
+		Recreate:             true,
+	})
+	if err != nil {
+		t.Fatalf("Up: %v", err)
+	}
+	defer func() { _ = eng.Down(context.Background(), wsObj, devcontainer.DownOptions{Remove: true}) }()
+
+	res, err := eng.Exec(ctx, wsObj, devcontainer.ExecOptions{
+		Cmd: []string{"sh", "-c", "printenv EXTRA_PATH; true"},
+	})
+	if err != nil {
+		t.Fatalf("Exec: %v", err)
+	}
+	if got := strings.TrimSpace(res.Stdout); got != "" {
+		t.Errorf("EXTRA_PATH = %q, want empty (userEnvProbe=none should skip injection)", got)
+	}
+}
+
+// TestUserEnvProbe_SkipOption verifies the per-Exec opt-out: even with
+// the default probe, a caller passing SkipUserEnvProbe gets a clean env.
+func TestUserEnvProbe_SkipOption(t *testing.T) {
+	if testing.Short() {
+		t.Skip()
+	}
+	eng, rt := newEngine(t)
+	defer rt.Close()
+
+	ws := writeWorkspace(t, `{
+		"image": "`+bashImage+`",
+		"postCreateCommand": "echo 'export EXTRA_PATH=/from/bashrc' > /etc/profile.d/dc-go-test.sh"
+	}`)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+	defer cancel()
+
+	wsObj, err := eng.Up(ctx, devcontainer.UpOptions{
+		LocalWorkspaceFolder: ws,
+		Recreate:             true,
+	})
+	if err != nil {
+		t.Fatalf("Up: %v", err)
+	}
+	defer func() { _ = eng.Down(context.Background(), wsObj, devcontainer.DownOptions{Remove: true}) }()
+
+	// Default Exec sees it.
+	res, err := eng.Exec(ctx, wsObj, devcontainer.ExecOptions{
+		Cmd: []string{"printenv", "EXTRA_PATH"},
+	})
+	if err != nil {
+		t.Fatalf("default Exec: %v", err)
+	}
+	if got := strings.TrimSpace(res.Stdout); got != "/from/bashrc" {
+		t.Fatalf("default exec EXTRA_PATH = %q, want %q (sanity precondition failed)", got, "/from/bashrc")
+	}
+
+	// SkipUserEnvProbe Exec doesn't.
+	res, err = eng.Exec(ctx, wsObj, devcontainer.ExecOptions{
+		Cmd:              []string{"sh", "-c", "printenv EXTRA_PATH; true"},
+		SkipUserEnvProbe: true,
+	})
+	if err != nil {
+		t.Fatalf("skip Exec: %v", err)
+	}
+	if got := strings.TrimSpace(res.Stdout); got != "" {
+		t.Errorf("SkipUserEnvProbe Exec saw EXTRA_PATH = %q, want empty", got)
+	}
+}
+
+// TestUserEnvProbe_NoBashFallback runs against a bash-less image
+// (alpine ships only ash). The probe's first attempt (bash -lic) fails;
+// we still want Up to succeed and Exec to work normally.
+func TestUserEnvProbe_NoBashFallback(t *testing.T) {
+	if testing.Short() {
+		t.Skip()
+	}
+	eng, rt := newEngine(t)
+	defer rt.Close()
+
+	ws := writeWorkspace(t, `{"image":"`+testImage+`"}`)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+	defer cancel()
+
+	wsObj, err := eng.Up(ctx, devcontainer.UpOptions{
+		LocalWorkspaceFolder: ws,
+		Recreate:             true,
+	})
+	if err != nil {
+		t.Fatalf("Up: %v", err)
+	}
+	defer func() { _ = eng.Down(context.Background(), wsObj, devcontainer.DownOptions{Remove: true}) }()
+
+	// Exec a basic command — failure mode would be an Up-level error
+	// (already caught above) or Exec returning nonzero / wrong output.
+	res, err := eng.Exec(ctx, wsObj, devcontainer.ExecOptions{
+		Cmd: []string{"sh", "-c", "echo ok"},
+	})
+	if err != nil {
+		t.Fatalf("Exec: %v", err)
+	}
+	if !strings.Contains(res.Stdout, "ok") {
+		t.Errorf("Exec stdout = %q, want contains 'ok'", res.Stdout)
+	}
+}
+
+// TestUserEnvProbe_RemoteEnvMerged confirms cfg.RemoteEnv reaches Exec
+// independently of whether the probe ran. RemoteEnv is the
+// devcontainer-author-declared env; it must layer on top of probedEnv
+// and be visible to every Exec call (matching @devcontainers/cli and
+// devpod).
+func TestUserEnvProbe_RemoteEnvMerged(t *testing.T) {
+	if testing.Short() {
+		t.Skip()
+	}
+	eng, rt := newEngine(t)
+	defer rt.Close()
+
+	ws := writeWorkspace(t, `{
+		"image": "`+bashImage+`",
+		"remoteEnv": {
+			"FROM_REMOTE": "remote-value"
+		}
+	}`)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+	defer cancel()
+
+	wsObj, err := eng.Up(ctx, devcontainer.UpOptions{
+		LocalWorkspaceFolder: ws,
+		Recreate:             true,
+	})
+	if err != nil {
+		t.Fatalf("Up: %v", err)
+	}
+	defer func() { _ = eng.Down(context.Background(), wsObj, devcontainer.DownOptions{Remove: true}) }()
+
+	res, err := eng.Exec(ctx, wsObj, devcontainer.ExecOptions{
+		Cmd: []string{"printenv", "FROM_REMOTE"},
+	})
+	if err != nil {
+		t.Fatalf("Exec: %v", err)
+	}
+	if got := strings.TrimSpace(res.Stdout); got != "remote-value" {
+		t.Errorf("FROM_REMOTE = %q, want %q", got, "remote-value")
+	}
+}

up.go

@@ -163,6 +163,16 @@ func (e *Engine) Up(ctx context.Context, opts UpOptions) (*Workspace, error) {
 			return nil, err
 		}
 	}
+
+	// Probe the user's interactive shell environment AFTER lifecycle —
+	// postCreateCommand and friends typically install rc-modifying tools
+	// (nvm, asdf, etc.) and we want their effects reflected in subsequent
+	// Exec calls. Failures are non-fatal: we log via warnings and proceed
+	// without probed env (matches devpod's behavior).
+	probed, err := e.probeUserEnv(ctx, ws, ws.Config.UserEnvProbe)
+	if err == nil {
+		ws.probedEnv = probed
+	}
 	return ws, nil
 }