fix(upstream): prepend docker bundle dir to child PATH so docker finds its credential helper (#715)

docs/docker-isolation.md

@@ -277,6 +277,26 @@ docker stats
   succeeds. A `false` value with `docker_path: ""` means the CLI could not be
   resolved on the spawn path.
 
+**`error getting credentials … docker-credential-desktop … not found in $PATH` on macOS (image not yet cached):**
+- Docker Desktop's default `~/.docker/config.json` sets `"credsStore": "desktop"`,
+  so `docker` shells out to `docker-credential-desktop` for **every** registry
+  operation — even an anonymous pull of a public image. That helper lives in the
+  **same bundle dir** as the docker CLI
+  (`/Applications/Docker.app/Contents/Resources/bin/`), which mcpproxy's
+  sanitized spawn `PATH` omits. When the isolation image isn't cached locally,
+  the pull invokes the helper and fails; a pre-pulled image sidesteps it because
+  `docker run` then performs no registry op (which is why direct-exec alone
+  looked complete on cached images — issue #715 / MCP-2877).
+- mcpproxy now **prepends the resolved docker binary's bundle dir to the child
+  `PATH`** whenever `docker` resolves to an absolute path, so the spawned docker
+  can exec its sibling tooling (`docker-credential-*`, `docker-compose`,
+  `docker-buildx`) exactly as it would from a normal Docker Desktop shell. This
+  is applied on every docker spawn path (isolated uvx/npx servers and
+  user-supplied `docker run …` upstreams) and is a no-op when `docker` did not
+  resolve to an absolute path. If you still see this error, confirm the helper
+  exists at the bundle path above, or pre-pull the image with
+  `docker pull <image>`.
+
 ## Security Considerations
 
 Docker isolation provides strong security boundaries but consider:

internal/upstream/core/connection_docker.go

@@ -18,12 +18,22 @@ import (
 var resolveDockerBinary = shellwrap.ResolveDockerPath
 
 // setupDockerIsolation configures Docker isolation for the MCP server process.
-// Returns the docker command, arguments, and whether the returned command was
-// wrapped in the user's login shell. shellWrapped governs how the caller
-// inserts --cidfile: a direct-exec spawn (shellWrapped == false) uses the
-// args-based insertCidfileIntoDockerArgs, while the login-shell fallback
-// (shellWrapped == true) uses the string-based insertCidfileIntoShellDockerCommand.
-func (c *Client) setupDockerIsolation(command string, args []string) (dockerCommand string, dockerArgs []string, shellWrapped bool) {
+// Returns the docker command, arguments, whether the returned command was
+// wrapped in the user's login shell, and the docker binary's bundle directory
+// (empty unless docker resolved to a verified absolute executable).
+//
+// shellWrapped governs how the caller inserts --cidfile: a direct-exec spawn
+// (shellWrapped == false) uses the args-based insertCidfileIntoDockerArgs, while
+// the login-shell fallback (shellWrapped == true) uses the string-based
+// insertCidfileIntoShellDockerCommand.
+//
+// dockerDir is the directory of the resolved absolute docker binary (e.g.
+// /Applications/Docker.app/Contents/Resources/bin). The caller must prepend it
+// to the child PATH (prependDockerDirToPath) so the spawned docker can exec its
+// sibling tooling — most importantly the docker-credential-* helper named by
+// ~/.docker/config.json's credsStore, which Docker Desktop installs into the
+// same bundle dir and which docker shells out to for every registry op (#715).
+func (c *Client) setupDockerIsolation(command string, args []string) (dockerCommand string, dockerArgs []string, shellWrapped bool, dockerDir string) {
 	// Detect the runtime type from the command
 	runtimeType := c.isolationManager.DetectRuntimeType(command)
 	c.logger.Debug("Detected runtime type for Docker isolation",
@@ -38,7 +48,7 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm
 			zap.String("server", c.config.Name),
 			zap.Error(err))
 		shellCmd, shellArgs := c.wrapWithUserShell(command, args)
-		return shellCmd, shellArgs, true
+		return shellCmd, shellArgs, true, ""
 	}
 
 	// Extract container name from Docker args for tracking
@@ -95,6 +105,14 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm
 // the login-shell fallback (shellWrapped == true) uses the string-based
 // insertCidfileIntoShellDockerCommand.
 //
+// dockerDir is the directory of the resolved absolute docker binary (empty
+// unless docker resolved to a verified absolute executable). The caller must
+// prepend it to the child PATH (prependDockerDirToPath) so the spawned docker
+// can exec its sibling tooling — most importantly the docker-credential-*
+// helper named by ~/.docker/config.json's credsStore, which Docker Desktop
+// installs into the same bundle dir and which docker shells out to for every
+// registry op (#715 / MCP-2877).
+//
 // CRITICAL FIX (#696 / MCP-2744 / MCP-2753 / MCP-2868): resolve `docker` to an
 // ABSOLUTE path and exec it DIRECTLY — no login-shell wrap. Docker Desktop
 // installed the default way on macOS (without the optional, admin-gated "install
@@ -133,7 +151,7 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm
 // When we fall back to the shell wrap we still prefer the resolved absolute path
 // as the wrapped command (it sidesteps the login shell's PATH re-derivation),
 // dropping to bare "docker" only when resolution failed.
-func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, dockerArgs []string, shellWrapped bool) {
+func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, dockerArgs []string, shellWrapped bool, dockerDir string) {
 	resolved, resErr := resolveDockerBinary(c.logger)
 	switch {
 	case resErr == nil && isDirectExecutable(resolved) && dockerDaemonEnvGuaranteed():
@@ -145,7 +163,7 @@ func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, d
 			zap.String("server", c.config.Name),
 			zap.String("docker_path", resolved),
 			zap.Bool("shell_wrapped", false))
-		return resolved, finalArgs, false
+		return resolved, finalArgs, false, filepath.Dir(resolved)
 	case resErr != nil:
 		c.logger.Warn("Could not resolve docker to an absolute path; falling back to bare 'docker' via login shell (isolated server may fail if docker is not on the spawn PATH)",
 			zap.String("server", c.config.Name),
@@ -164,8 +182,14 @@ func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, d
 	}
 
 	dockerBin := cmdDocker
+	// Seed the bundle dir for PATH augmentation whenever docker resolved to a
+	// verified absolute executable — even though we keep the login-shell wrap
+	// here. It is harmless on the shell-wrap path and still helps if the login
+	// shell preserves the inherited PATH (#715).
+	var resolvedDir string
 	if isDirectExecutable(resolved) {
 		dockerBin = resolved
+		resolvedDir = filepath.Dir(resolved)
 	}
 	// INFO: the login-shell fallback is the ONLY path that can produce #696's
 	// `command not found: docker`, so surface that we took it (and whether we at
@@ -176,7 +200,47 @@ func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, d
 		zap.Bool("docker_resolved", isDirectExecutable(resolved)),
 		zap.Bool("shell_wrapped", true))
 	shellCmd, shellArgs := c.wrapWithUserShell(dockerBin, finalArgs)
-	return shellCmd, shellArgs, true
+	return shellCmd, shellArgs, true, resolvedDir
+}
+
+// prependDockerDirToPath returns env with dockerDir prepended to its PATH entry
+// so a spawned docker can resolve the sibling binaries it depends on — the
+// docker-credential-* helper named by ~/.docker/config.json's credsStore,
+// docker-compose, docker-buildx — which Docker Desktop installs into the same
+// bundle dir as the docker CLI but which mcpproxy's sanitized PATH omits (#715).
+//
+// dockerDir is prepended (existing entries preserved) and deduped: if it is
+// already present anywhere on PATH the env is returned unchanged. An empty
+// dockerDir (docker not resolved to an absolute path) is a no-op. The input
+// slice is never mutated. os.PathListSeparator keeps it correct on Windows.
+func prependDockerDirToPath(env []string, dockerDir string) []string {
+	if dockerDir == "" {
+		return env
+	}
+	for i, kv := range env {
+		name, val, ok := strings.Cut(kv, "=")
+		if !ok || name != "PATH" {
+			continue
+		}
+		// Dedup: leave PATH untouched if the bundle dir is already on it.
+		for _, p := range filepath.SplitList(val) {
+			if p == dockerDir {
+				return env
+			}
+		}
+		out := make([]string, len(env))
+		copy(out, env)
+		if val == "" {
+			out[i] = "PATH=" + dockerDir
+		} else {
+			out[i] = "PATH=" + dockerDir + string(os.PathListSeparator) + val
+		}
+		return out
+	}
+	// No PATH entry present — add one so docker still finds its bundle dir.
+	out := make([]string, len(env), len(env)+1)
+	copy(out, env)
+	return append(out, "PATH="+dockerDir)
 }
 
 // isDirectExecutable reports whether path is safe to hand to exec directly

internal/upstream/core/connection_docker_exec_e2e_test.go

@@ -69,7 +69,7 @@ func TestE2E_DockerSpawnExecsAbsoluteBundlePath(t *testing.T) {
 	t.Setenv("SHELL", "/nonexistent/shell-must-not-be-invoked")
 
 	c := newIsolatedTestClient()
-	cmd, args, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args)
+	cmd, args, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args)
 	require.False(t, shellWrapped, "#696: bundle-resolved docker must be direct-exec'd")
 
 	// Exec it exactly as connection_stdio would (exec.CommandContext(cmd, args...)).

internal/upstream/core/connection_docker_integration_test.go

@@ -42,7 +42,7 @@ func TestSpawnDecisionIsObservable_DirectExec(t *testing.T) {
 	t.Setenv("SHELL", "/nonexistent/shell-must-not-be-invoked")
 
 	c, recorded := newObservedIsolatedClient(t)
-	_, _, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args)
+	_, _, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args)
 	require.False(t, shellWrapped)
 
 	entries := recorded.FilterMessage("Docker spawn: direct-exec of resolved docker binary").All()
@@ -63,7 +63,7 @@ func TestSpawnDecisionIsObservable_ShellWrapFallback(t *testing.T) {
 	}
 
 	c, recorded := newObservedIsolatedClient(t)
-	_, _, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args)
+	_, _, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args)
 	require.True(t, shellWrapped)
 
 	entries := recorded.FilterMessage("Docker spawn: login-shell wrap fallback").All()
@@ -122,7 +122,7 @@ func TestIntegration_DockerOnlyAtBundlePath_DirectExecs(t *testing.T) {
 	t.Setenv("SHELL", "/nonexistent/shell-must-not-be-invoked")
 
 	c := newIsolatedTestClient()
-	cmd, args, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args)
+	cmd, args, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args)
 
 	require.False(t, shellWrapped,
 		"#696: docker resolved at the bundle path MUST be direct-exec'd, not shell-wrapped with bare docker")
@@ -154,7 +154,7 @@ func TestIntegration_DockerOnPath_DirectExecs(t *testing.T) {
 	t.Setenv("SHELL", "/nonexistent/shell-must-not-be-invoked")
 
 	c := newIsolatedTestClient()
-	cmd, args, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args)
+	cmd, args, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args)
 
 	require.False(t, shellWrapped, "docker on PATH must resolve to an absolute path and direct-exec")
 	assert.Equal(t, pathDocker, cmd, "spawn must exec the LookPath-resolved absolute path, got %q", cmd)
@@ -183,7 +183,7 @@ func TestIntegration_DockerUnresolvable_FallsBackToBareDocker(t *testing.T) {
 	t.Setenv("SHELL", "/nonexistent/shell-must-not-be-invoked")
 
 	c := newIsolatedTestClient()
-	_, shellArgs, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args)
+	_, shellArgs, shellWrapped, _ := c.setupDockerIsolation(c.config.Command, c.config.Args)
 
 	require.True(t, shellWrapped, "unresolvable docker must fall back to the login-shell wrap")
 	require.NotEmpty(t, shellArgs)

internal/upstream/core/connection_docker_path_test.go

@@ -0,0 +1,129 @@
+package core
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+)
+
+// TestSetupDockerIsolationReturnsBundleDirOnDirectExec is the root-cause
+// assertion for #715 / MCP-2877. When docker resolves to a verified absolute
+// bundle-dir binary and is direct-exec'd, setupDockerIsolation must report that
+// binary's directory so the caller can prepend it to the child PATH. Without
+// the bundle dir on PATH, docker cannot exec its sibling credential helper
+// (docker-credential-desktop, named by ~/.docker/config.json's credsStore) and
+// any registry pull of an uncached isolation image fails.
+func TestSetupDockerIsolationReturnsBundleDirOnDirectExec(t *testing.T) {
+	fakeDocker := writeFakeDockerExecutable(t)
+	forceDockerDaemonEnvGOOS(t, osDarwin)
+
+	orig := resolveDockerBinary
+	t.Cleanup(func() { resolveDockerBinary = orig })
+	resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil }
+
+	c := newIsolatedTestClient()
+	cmd, _, shellWrapped, dockerDir := c.setupDockerIsolation(c.config.Command, c.config.Args)
+
+	require.False(t, shellWrapped)
+	require.Equal(t, fakeDocker, cmd)
+	assert.Equal(t, filepath.Dir(fakeDocker), dockerDir,
+		"setupDockerIsolation must report the resolved docker binary's bundle dir for PATH augmentation")
+}
+
+// TestSetupDockerIsolationReturnsBundleDirOnShellWrapFallback verifies the dir
+// is reported even when we keep the login-shell wrap (non-Darwin, no DOCKER_HOST
+// in env): the issue specifies seeding the env PATH is harmless on the shell-wrap
+// path and helps if the login shell preserves the inherited PATH. The gate is
+// "resolved to a verified absolute executable", not "direct-exec'd".
+func TestSetupDockerIsolationReturnsBundleDirOnShellWrapFallback(t *testing.T) {
+	fakeDocker := writeFakeDockerExecutable(t)
+	forceDockerDaemonEnvGOOS(t, "linux")
+	t.Setenv("DOCKER_HOST", "")
+	t.Setenv("DOCKER_CONTEXT", "")
+
+	orig := resolveDockerBinary
+	t.Cleanup(func() { resolveDockerBinary = orig })
+	resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil }
+
+	c := newIsolatedTestClient()
+	_, _, shellWrapped, dockerDir := c.setupDockerIsolation(c.config.Command, c.config.Args)
+
+	require.True(t, shellWrapped, "non-Darwin without DOCKER_HOST keeps the shell wrap")
+	assert.Equal(t, filepath.Dir(fakeDocker), dockerDir,
+		"a verified absolute resolved path must report its bundle dir even on the shell-wrap fallback")
+}
+
+// TestSetupDockerIsolationNoBundleDirWhenUnresolved verifies no dir is reported
+// when docker did not resolve to a verified absolute path (bare-'docker'
+// fallback) — there is no filepath.Dir to take, and augmenting PATH would be
+// meaningless / unsafe.
+func TestSetupDockerIsolationNoBundleDirWhenUnresolved(t *testing.T) {
+	orig := resolveDockerBinary
+	t.Cleanup(func() { resolveDockerBinary = orig })
+	resolveDockerBinary = func(_ *zap.Logger) (string, error) { return "docker", nil }
+
+	c := newIsolatedTestClient()
+	_, _, shellWrapped, dockerDir := c.setupDockerIsolation(c.config.Command, c.config.Args)
+
+	require.True(t, shellWrapped)
+	assert.Empty(t, dockerDir, "a non-absolute resolved value must not report a bundle dir")
+}
+
+// TestPrependDockerDirToPath unit-tests the pure PATH-augmentation helper.
+func TestPrependDockerDirToPath(t *testing.T) {
+	sep := string(os.PathListSeparator)
+	dir := filepath.FromSlash("/Applications/Docker.app/Contents/Resources/bin")
+
+	t.Run("prepends to existing PATH preserving entries", func(t *testing.T) {
+		env := []string{"FOO=bar", "PATH=/usr/bin" + sep + "/bin"}
+		got := prependDockerDirToPath(env, dir)
+		var path string
+		for _, kv := range got {
+			if strings.HasPrefix(kv, "PATH=") {
+				path = strings.TrimPrefix(kv, "PATH=")
+			}
+		}
+		parts := filepath.SplitList(path)
+		require.NotEmpty(t, parts)
+		assert.Equal(t, dir, parts[0], "bundle dir must be prepended (front), got: %s", path)
+		assert.Contains(t, parts, "/usr/bin", "existing entries preserved")
+		assert.Contains(t, parts, "/bin", "existing entries preserved")
+		assert.Contains(t, got, "FOO=bar", "unrelated env vars untouched")
+	})
+
+	t.Run("dedup: already present leaves PATH unchanged", func(t *testing.T) {
+		env := []string{"PATH=/usr/bin" + sep + dir + sep + "/bin"}
+		got := prependDockerDirToPath(env, dir)
+		assert.Equal(t, env, got, "must not re-add a dir already on PATH")
+	})
+
+	t.Run("empty dir is a no-op", func(t *testing.T) {
+		env := []string{"PATH=/usr/bin"}
+		got := prependDockerDirToPath(env, "")
+		assert.Equal(t, env, got)
+	})
+
+	t.Run("adds PATH when none present", func(t *testing.T) {
+		env := []string{"FOO=bar"}
+		got := prependDockerDirToPath(env, dir)
+		assert.Contains(t, got, "PATH="+dir)
+		assert.Contains(t, got, "FOO=bar")
+	})
+
+	t.Run("empty PATH value yields just the dir", func(t *testing.T) {
+		env := []string{"PATH="}
+		got := prependDockerDirToPath(env, dir)
+		assert.Contains(t, got, "PATH="+dir)
+	})
+
+	t.Run("does not mutate the input slice", func(t *testing.T) {
+		env := []string{"PATH=/usr/bin"}
+		_ = prependDockerDirToPath(env, dir)
+		assert.Equal(t, "PATH=/usr/bin", env[0], "input slice must not be mutated")
+	})
+}