fix(shellwrap): capture login-shell PATH for docker credential helpers (#381)

internal/shellwrap/shellwrap.go

@@ -191,6 +191,104 @@ func resetDockerPathCacheForTest() {
 	dockerPathErr = nil
 }
 
+// --- Login-shell PATH capture --------------------------------------------
+
+var (
+	loginShellPathOnce sync.Once
+	loginShellPathVal  string
+)
+
+// LoginShellPATH returns the PATH value emitted by the user's login shell.
+// It is captured exactly once per process via
+// `<shell> -l -c 'printf %s "$PATH"'` and cached for the rest of the
+// process lifetime.
+//
+// Why this exists: when mcpproxy runs as a macOS App Bundle or LaunchAgent,
+// os.Getenv("PATH") is often `/usr/bin:/bin`. That is enough for Go's
+// exec.LookPath to find a docker binary once shellwrap.ResolveDockerPath
+// has cached its absolute path, but it is NOT enough for the docker CLI
+// itself, which re-execs credential helpers like `docker-credential-desktop`
+// via its own $PATH lookup. Those helpers typically live in
+// /usr/local/bin or /opt/homebrew/bin — directories that only exist in
+// the interactive login PATH.
+//
+// On Windows, this function returns "" (credential-helper PATH drift is
+// not the same problem there, and interactive-shell PATH capture would
+// require cmd.exe or PowerShell gymnastics we explicitly avoid).
+//
+// Callers should treat an empty return value as "no override available"
+// and fall back to os.Getenv("PATH").
+func LoginShellPATH(logger *zap.Logger) string {
+	loginShellPathOnce.Do(func() {
+		if runtime.GOOS == osWindows {
+			return
+		}
+		shell := resolveLoginShell()
+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+		defer cancel()
+		// `-l -c 'printf %s "$PATH"'` works on bash, zsh, dash, sh.
+		// We deliberately build the argv ourselves rather than going
+		// through WrapWithUserShell because shellescape would quote
+		// the `$PATH` and suppress expansion.
+		cmd := exec.CommandContext(ctx, shell, "-l", "-c", `printf %s "$PATH"`)
+		out, err := cmd.Output()
+		if err != nil {
+			if logger != nil {
+				logger.Debug("shellwrap: login-shell PATH capture failed",
+					zap.String("shell", shell),
+					zap.Error(err))
+			}
+			return
+		}
+		captured := strings.TrimSpace(string(out))
+		if captured == "" {
+			return
+		}
+		loginShellPathVal = captured
+		if logger != nil {
+			logger.Debug("shellwrap: captured login-shell PATH",
+				zap.String("shell", shell),
+				zap.Int("path_length", len(captured)))
+		}
+	})
+	return loginShellPathVal
+}
+
+// mergePathUnique joins two PATH-style strings into one, preserving the
+// order of `primary` (highest priority) followed by any entries of
+// `secondary` that were not already present. Empty segments are dropped.
+func mergePathUnique(primary, secondary, sep string) string {
+	if primary == "" {
+		return secondary
+	}
+	if secondary == "" {
+		return primary
+	}
+	seen := make(map[string]struct{}, 16)
+	parts := make([]string, 0, 16)
+	add := func(list string) {
+		for _, p := range strings.Split(list, sep) {
+			if p == "" {
+				continue
+			}
+			if _, ok := seen[p]; ok {
+				continue
+			}
+			seen[p] = struct{}{}
+			parts = append(parts, p)
+		}
+	}
+	add(primary)
+	add(secondary)
+	return strings.Join(parts, sep)
+}
+
+// resetLoginShellPathCacheForTest is only referenced from shellwrap_test.go.
+func resetLoginShellPathCacheForTest() {
+	loginShellPathOnce = sync.Once{}
+	loginShellPathVal = ""
+}
+
 // --- Minimal environment for scanner subprocesses ------------------------
 
 // MinimalEnv returns a minimal, allow-listed environment suitable for
@@ -200,10 +298,28 @@ func resetDockerPathCacheForTest() {
 //
 // Callers that need TLS or Docker-specific variables (DOCKER_HOST,
 // DOCKER_CONFIG, …) should append them explicitly.
+//
+// On Unix, PATH is built by merging the user's login-shell PATH
+// (captured once via LoginShellPATH) with the process's ambient PATH.
+// Login-shell entries come first so that docker's own credential-helper
+// lookups can find binaries installed in /opt/homebrew/bin or
+// /usr/local/bin even when mcpproxy was started from a LaunchAgent with
+// a minimal inherited PATH. See issue #381.
 func MinimalEnv() []string {
+	return minimalEnvWithLogger(nil)
+}
+
+// MinimalEnvWithLogger is MinimalEnv with an optional logger used while
+// capturing the login-shell PATH on the first call. Subsequent calls
+// return the cached value without logging.
+func MinimalEnvWithLogger(logger *zap.Logger) []string {
+	return minimalEnvWithLogger(logger)
+}
+
+func minimalEnvWithLogger(logger *zap.Logger) []string {
 	env := make([]string, 0, 8)
-	if v := os.Getenv("PATH"); v != "" {
-		env = append(env, "PATH="+v)
+	if path := buildMinimalPath(logger); path != "" {
+		env = append(env, "PATH="+path)
 	}
 	if runtime.GOOS == osWindows {
 		if v := os.Getenv("USERPROFILE"); v != "" {
@@ -222,3 +338,20 @@ func MinimalEnv() []string {
 	}
 	return env
 }
+
+// buildMinimalPath returns the PATH value that MinimalEnv should set on
+// child processes. On Unix it merges the login-shell PATH with ambient
+// PATH so that docker credential helpers (e.g. docker-credential-desktop)
+// installed in /opt/homebrew/bin or /usr/local/bin are resolvable — see
+// issue #381. On Windows it returns the ambient PATH unchanged.
+func buildMinimalPath(logger *zap.Logger) string {
+	ambient := os.Getenv("PATH")
+	if runtime.GOOS == osWindows {
+		return ambient
+	}
+	login := LoginShellPATH(logger)
+	if login == "" {
+		return ambient
+	}
+	return mergePathUnique(login, ambient, string(os.PathListSeparator))
+}

internal/shellwrap/shellwrap_test.go

@@ -1,6 +1,8 @@
 package shellwrap
 
 import (
+	"os"
+	"path/filepath"
 	"runtime"
 	"strings"
 	"testing"
@@ -110,3 +112,141 @@ func TestMinimalEnv_DropsSecrets(t *testing.T) {
 	}
 	assert.True(t, hasPath, "minimal env must contain PATH")
 }
+
+func TestMergePathUnique(t *testing.T) {
+	cases := []struct {
+		name                    string
+		primary, secondary, sep string
+		want                    string
+	}{
+		{"empty primary", "", "/usr/bin:/bin", ":", "/usr/bin:/bin"},
+		{"empty secondary", "/opt/homebrew/bin", "", ":", "/opt/homebrew/bin"},
+		{
+			"dedup keeps primary order",
+			"/opt/homebrew/bin:/usr/local/bin",
+			"/usr/bin:/opt/homebrew/bin:/bin",
+			":",
+			"/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin",
+		},
+		{
+			"drops empty segments",
+			"/opt/homebrew/bin::/usr/local/bin",
+			":/bin:",
+			":",
+			"/opt/homebrew/bin:/usr/local/bin:/bin",
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.want, mergePathUnique(tc.primary, tc.secondary, tc.sep))
+		})
+	}
+}
+
+// writeFakeShell writes a POSIX shell script at <dir>/fake-shell that
+// ignores its arguments and echoes `wantPath` on stdout. It returns the
+// absolute path to the script so tests can point $SHELL at it.
+func writeFakeShell(t *testing.T, dir, wantPath string) string {
+	t.Helper()
+	script := "#!/bin/sh\nprintf %s '" + wantPath + "'\n"
+	p := filepath.Join(dir, "fake-shell")
+	require.NoError(t, os.WriteFile(p, []byte(script), 0o755))
+	return p
+}
+
+func TestLoginShellPATH_CapturesFromShell(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("login-shell PATH capture is Unix-only")
+	}
+	resetLoginShellPathCacheForTest()
+	t.Cleanup(resetLoginShellPathCacheForTest)
+
+	dir := t.TempDir()
+	want := "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin"
+	fake := writeFakeShell(t, dir, want)
+	t.Setenv("SHELL", fake)
+
+	got := LoginShellPATH(nil)
+	assert.Equal(t, want, got, "should capture the PATH printed by the login shell")
+
+	// Second call must use the cache even if we break $SHELL.
+	t.Setenv("SHELL", "/nonexistent/shell")
+	got2 := LoginShellPATH(nil)
+	assert.Equal(t, want, got2, "second call must return the cached value")
+}
+
+func TestLoginShellPATH_EmptyWhenShellFails(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("unix-only")
+	}
+	resetLoginShellPathCacheForTest()
+	t.Cleanup(resetLoginShellPathCacheForTest)
+
+	t.Setenv("SHELL", "/nonexistent/shell-binary-does-not-exist")
+	got := LoginShellPATH(nil)
+	assert.Equal(t, "", got, "should return empty string when the login shell cannot be executed")
+}
+
+func TestMinimalEnv_PrefersLoginShellPATH(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("unix-only")
+	}
+	resetLoginShellPathCacheForTest()
+	t.Cleanup(resetLoginShellPathCacheForTest)
+
+	// Simulate a LaunchAgent-like ambient PATH (missing homebrew/local).
+	t.Setenv("PATH", "/usr/bin:/bin")
+
+	// Point $SHELL at a fake shell that prints the real (enriched) PATH.
+	dir := t.TempDir()
+	fake := writeFakeShell(t, dir, "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin")
+	t.Setenv("SHELL", fake)
+
+	env := MinimalEnv()
+
+	var pathVal string
+	for _, kv := range env {
+		if strings.HasPrefix(kv, "PATH=") {
+			pathVal = strings.TrimPrefix(kv, "PATH=")
+			break
+		}
+	}
+	require.NotEmpty(t, pathVal, "MinimalEnv must include PATH")
+
+	// The login-shell entries must come first so that docker's
+	// credential-helper lookup can find /opt/homebrew/bin and /usr/local/bin.
+	assert.True(t, strings.HasPrefix(pathVal, "/opt/homebrew/bin:/usr/local/bin"),
+		"login-shell PATH entries must come before ambient, got %q", pathVal)
+	assert.Contains(t, pathVal, "/usr/bin", "ambient PATH entries must still be present")
+	assert.Contains(t, pathVal, "/bin", "ambient PATH entries must still be present")
+
+	// No duplicate segments.
+	segs := strings.Split(pathVal, ":")
+	seen := make(map[string]bool, len(segs))
+	for _, s := range segs {
+		assert.False(t, seen[s], "duplicate PATH segment %q in %q", s, pathVal)
+		seen[s] = true
+	}
+}
+
+func TestMinimalEnv_FallsBackToAmbientPATHWhenShellBroken(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("unix-only")
+	}
+	resetLoginShellPathCacheForTest()
+	t.Cleanup(resetLoginShellPathCacheForTest)
+
+	t.Setenv("PATH", "/usr/bin:/bin")
+	t.Setenv("SHELL", "/nonexistent/shell-binary-does-not-exist")
+
+	env := MinimalEnv()
+
+	var pathVal string
+	for _, kv := range env {
+		if strings.HasPrefix(kv, "PATH=") {
+			pathVal = strings.TrimPrefix(kv, "PATH=")
+			break
+		}
+	}
+	assert.Equal(t, "/usr/bin:/bin", pathVal, "must fall back to ambient PATH when login shell fails")
+}