fix(upstream): direct-exec resolved docker for user-supplied docker run (#696/MCP-2868)

docs/docker-isolation.md

@@ -252,9 +252,11 @@ docker stats
   admin-gated "install CLI tools" step. When mcpproxy is launched from a
   LaunchAgent / tray, the captured login-shell `PATH` may omit this directory.
 - mcpproxy resolves the `docker` binary to its **absolute path** and then
-  **exec's it directly** (no login-shell wrap) when spawning an isolated server,
-  so the spawn bypasses `PATH` entirely and works even without the CLI-tools
-  step. (The enhanced spawn `PATH` still includes the bundle bin dir as a
+  **exec's it directly** (no login-shell wrap) when spawning a Docker upstream —
+  both servers that mcpproxy *isolates* into `docker run` (uvx/npx) and upstreams
+  whose config `command` **is** `docker` (a user-supplied `docker run …`) — so
+  the spawn bypasses `PATH` entirely and works even without the CLI-tools step.
+  (The enhanced spawn `PATH` still includes the bundle bin dir as a
   belt-and-suspenders measure.) Earlier builds resolved the absolute path but
   still routed the spawn through `$SHELL -l -c "<docker> run …"`, where the
   login shell re-derived `PATH` from rc files and could drop the bundle dir —

internal/upstream/core/connection_docker.go

@@ -69,44 +69,71 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm
 			zap.String("container_command", containerCommand))
 	}
 
-	// CRITICAL FIX (#696 / MCP-2744 / MCP-2753): 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 CLI
-	// tools" step) leaves the docker CLI only inside the app bundle at
-	// /Applications/Docker.app/Contents/Resources/bin/docker, which is NOT on
-	// any standard PATH dir nor on the (often unreliable) login-shell PATH a
-	// LaunchAgent captures.
-	//
-	// #697 resolved the absolute path but still routed the spawn through
-	// `$SHELL -l -c "<docker> run …"`. There the absolute path is only a token
-	// inside the -c string: the login shell re-derives $PATH from rc files and
-	// can drop the bundle dir, so the bug persisted. Exec'ing the absolute path
-	// directly (mirroring newDockerCmd's exec.CommandContext(dockerBin, …))
-	// bypasses PATH entirely.
-	//
-	// Two preconditions gate the direct-exec:
-	//
-	//  1. The resolved value must be a VERIFIED absolute executable
-	//     (isDirectExecutable). ResolveDockerPath's last resort runs
-	//     `command -v docker` in the login shell, which can emit a shell
-	//     function name, an alias, or a bare command name rather than a real
-	//     binary path; direct-exec'ing that would fail with "no such file".
-	//
-	//  2. The docker daemon-config env (DOCKER_HOST/DOCKER_CONTEXT) must be
-	//     guaranteed in the spawn env WITHOUT the login shell
-	//     (dockerDaemonEnvGuaranteed). Dropping the `$SHELL -l` wrap also drops
-	//     rc-file env inheritance. On macOS the startup hydration (MCP-2751)
-	//     already captured DOCKER_* from the login shell into os.Environ(), and
-	//     secureenv forwards them — but that hydration is Darwin-only. On Linux
-	//     a rootless/remote daemon whose DOCKER_HOST lives only in .profile would
-	//     be lost by direct-exec (the regression #699 kept the shell wrap to
-	//     avoid). So on non-Darwin we only direct-exec when DOCKER_HOST/
-	//     DOCKER_CONTEXT are already in os.Environ(); otherwise we keep the
-	//     login-shell wrap so `docker run` still finds the daemon.
-	//
-	// 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.
+	// Resolve `docker` to an absolute path and decide between a direct-exec and a
+	// login-shell wrap. Shared with the user-supplied `docker run` path
+	// (connectStdio) via resolveDockerSpawn so both spawn paths behave identically
+	// (#696 / MCP-2868).
+	return c.resolveDockerSpawn(finalArgs)
+}
+
+// resolveDockerSpawn decides how to spawn `docker` given the fully built docker
+// run argv (finalArgs, beginning with the "run" token). It resolves `docker` to
+// an absolute path and, when safe, returns that absolute path for a DIRECT exec
+// (shellWrapped == false); otherwise it returns a login-shell wrap
+// (shellWrapped == true). It is shared by BOTH docker spawn paths:
+//
+//   - setupDockerIsolation — uvx/npx servers wrapped INTO `docker run` by the
+//     isolation manager (isolation-injection path).
+//   - connectStdio's user-supplied `docker run` branch — upstreams whose config
+//     Command IS `docker` (GH #696 / MCP-2868). Before this extraction that path
+//     shell-wrapped bare `docker`, so on a default Docker Desktop macOS install
+//     (docker CLI only inside the app bundle, not on the login-shell PATH) it
+//     failed with `zsh:1: command not found: docker`.
+//
+// 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.
+//
+// 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
+// CLI tools" step) leaves the docker CLI only inside the app bundle at
+// /Applications/Docker.app/Contents/Resources/bin/docker, which is NOT on any
+// standard PATH dir nor on the (often unreliable) login-shell PATH a LaunchAgent
+// captures.
+//
+// #697 resolved the absolute path but still routed the spawn through
+// `$SHELL -l -c "<docker> run …"`. There the absolute path is only a token
+// inside the -c string: the login shell re-derives $PATH from rc files and can
+// drop the bundle dir, so the bug persisted. Exec'ing the absolute path directly
+// (mirroring newDockerCmd's exec.CommandContext(dockerBin, …)) bypasses PATH
+// entirely.
+//
+// Two preconditions gate the direct-exec:
+//
+//  1. The resolved value must be a VERIFIED absolute executable
+//     (isDirectExecutable). ResolveDockerPath's last resort runs
+//     `command -v docker` in the login shell, which can emit a shell function
+//     name, an alias, or a bare command name rather than a real binary path;
+//     direct-exec'ing that would fail with "no such file".
+//
+//  2. The docker daemon-config env (DOCKER_HOST/DOCKER_CONTEXT) must be
+//     guaranteed in the spawn env WITHOUT the login shell
+//     (dockerDaemonEnvGuaranteed). Dropping the `$SHELL -l` wrap also drops
+//     rc-file env inheritance. On macOS the startup hydration (MCP-2751) already
+//     captured DOCKER_* from the login shell into os.Environ(), and secureenv
+//     forwards them — but that hydration is Darwin-only. On Linux a
+//     rootless/remote daemon whose DOCKER_HOST lives only in .profile would be
+//     lost by direct-exec (the regression #699 kept the shell wrap to avoid). So
+//     on non-Darwin we only direct-exec when DOCKER_HOST/DOCKER_CONTEXT are
+//     already in os.Environ(); otherwise we keep the login-shell wrap so
+//     `docker run` still finds the daemon.
+//
+// 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) {
 	resolved, resErr := resolveDockerBinary(c.logger)
 	switch {
 	case resErr == nil && isDirectExecutable(resolved) && dockerDaemonEnvGuaranteed():

internal/upstream/core/connection_docker_test.go

@@ -27,6 +27,23 @@ func newIsolatedTestClient() *Client {
 	}
 }
 
+// newUserSuppliedDockerTestClient builds a Client whose config Command IS
+// `docker` (a user-supplied `docker run …` upstream, as opposed to a uvx/npx
+// server wrapped INTO docker by the isolation manager). This is the GH #696 /
+// MCP-2868 path.
+func newUserSuppliedDockerTestClient() *Client {
+	return &Client{
+		config: &config.ServerConfig{
+			Name:    "user-docker-server",
+			Command: "docker",
+			Args:    []string{"run", "-i", "--rm", "mcp/example"},
+			Env:     map[string]string{"SLACK_TOKEN": "xoxb-secret"},
+		},
+		logger:           zap.NewNop(),
+		isolationManager: NewIsolationManager(config.DefaultDockerIsolationConfig()),
+	}
+}
+
 // forceDockerDaemonEnvGOOS overrides the GOOS the daemon-env guard sees, so the
 // Darwin (env-guaranteed-by-hydration) and non-Darwin branches can both be
 // exercised on a single CI host. Restored on cleanup.
@@ -290,6 +307,81 @@ func TestSetupDockerIsolationDirectExecsWhenDockerHostInEnv(t *testing.T) {
 	assert.Equal(t, cmdRun, args[0])
 }
 
+// TestResolveDockerSpawnUserSuppliedDockerRunDirectExec is the MCP-2868
+// root-cause assertion. A USER-SUPPLIED `docker run` upstream (config.Command IS
+// `docker`) must reuse the SAME resolve→direct-exec decision as the
+// isolation-injection path. With docker resolved to a verified absolute
+// executable and the daemon env guaranteed (macOS), the spawn must direct-exec
+// the ABSOLUTE docker path (no `$SHELL -l -c` wrap), carry the injected
+// `-e KEY=VALUE` env flags, and accept an args-based cidfile insertion. Before
+// this fix the user path shell-wrapped bare `docker`, producing GH #696's
+// `zsh:1: command not found: docker` on a default Docker Desktop macOS install
+// (docker CLI only inside the app bundle, not on the login-shell PATH).
+func TestResolveDockerSpawnUserSuppliedDockerRunDirectExec(t *testing.T) {
+	fakeDocker := writeFakeDockerExecutable(t)
+	forceDockerDaemonEnvGOOS(t, osDarwin) // daemon env guaranteed via startup hydration
+
+	orig := resolveDockerBinary
+	t.Cleanup(func() { resolveDockerBinary = orig })
+	resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil }
+
+	c := newUserSuppliedDockerTestClient()
+	// Mimic connectStdio's user-supplied docker branch: inject env vars as -e
+	// flags into the docker run argv before deciding how to spawn.
+	argsToWrap := c.injectEnvVarsIntoDockerArgs(c.config.Args, c.config.Env)
+
+	cmd, args, shellWrapped := c.resolveDockerSpawn(argsToWrap)
+
+	assert.False(t, shellWrapped, "verified absolute docker must direct-exec, not shell-wrap")
+	assert.Equal(t, fakeDocker, cmd,
+		"user docker must be exec'd by its resolved absolute path, not bare 'docker', got: %s", cmd)
+	require.NotEmpty(t, args)
+	assert.Equal(t, cmdRun, args[0],
+		"first arg must be the raw 'run' token (not a shell -c string), got: %v", args)
+	// The injected -e env flag must survive into the docker run argv.
+	assert.Subset(t, args, []string{"-e", "SLACK_TOKEN=xoxb-secret"},
+		"injected -e env flags must be present in the direct-exec argv, got: %v", args)
+	for _, a := range args {
+		assert.NotContains(t, a, " run ",
+			"args must be raw argv tokens, not a single shell command string, got: %v", args)
+	}
+
+	// On the direct-exec path the caller inserts --cidfile via the args-based
+	// helper (right after the "run" token).
+	withCid := c.insertCidfileIntoDockerArgs(args, "/tmp/cid.txt")
+	require.GreaterOrEqual(t, len(withCid), 3)
+	assert.Equal(t, []string{cmdRun, "--cidfile", "/tmp/cid.txt"}, withCid[:3],
+		"cidfile must be inserted right after 'run' on the direct-exec path, got: %v", withCid)
+}
+
+// TestResolveDockerSpawnUserSuppliedFallsBackToShellWrap verifies the
+// resolution-failure fallback for a user-supplied `docker run`: the spawn must
+// shell-wrap bare `docker` (preserving login-shell PATH resolution as a last
+// resort) and the cidfile is inserted via the string-based helper.
+func TestResolveDockerSpawnUserSuppliedFallsBackToShellWrap(t *testing.T) {
+	orig := resolveDockerBinary
+	t.Cleanup(func() { resolveDockerBinary = orig })
+	resolveDockerBinary = func(_ *zap.Logger) (string, error) {
+		return "", fmt.Errorf("docker not found")
+	}
+
+	c := newUserSuppliedDockerTestClient()
+	argsToWrap := c.injectEnvVarsIntoDockerArgs(c.config.Args, c.config.Env)
+
+	cmd, shellArgs, shellWrapped := c.resolveDockerSpawn(argsToWrap)
+
+	require.True(t, shellWrapped, "on resolution failure the user docker spawn must be shell-wrapped")
+	assert.NotEqual(t, cmdDocker, cmd, "shell-wrap fallback exec's the login shell, not bare 'docker'")
+	require.NotEmpty(t, shellArgs)
+	cmdStr := shellArgs[len(shellArgs)-1]
+	assert.True(t, strings.HasPrefix(cmdStr, "docker run"),
+		"on resolution failure the spawn must fall back to bare 'docker', got: %s", cmdStr)
+	// The real command string still carries the actual env value (redaction is a
+	// log-only concern, not a spawn concern).
+	assert.Contains(t, cmdStr, "-e SLACK_TOKEN=xoxb-secret",
+		"injected -e env flag must survive into the shell command string, got: %s", cmdStr)
+}
+
 // TestDockerDaemonEnvGuaranteed unit-tests the gate directly.
 func TestDockerDaemonEnvGuaranteed(t *testing.T) {
 	t.Run("darwin is always guaranteed (startup hydration)", func(t *testing.T) {

internal/upstream/core/connection_stdio.go

@@ -157,32 +157,44 @@ func (c *Client) connectStdio(ctx context.Context) error {
 				finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile)
 			}
 		}
-	} else {
-		// For direct docker commands, inject env vars as -e flags before shell wrapping
+	} else if isDirectDockerRun := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun; isDirectDockerRun {
+		// USER-SUPPLIED `docker run …` upstream (config.Command IS `docker`).
+		// Inject env vars as -e flags, then reuse the SAME resolve→spawn decision
+		// as the isolation path (resolveDockerSpawn) so this path also direct-execs
+		// the resolved ABSOLUTE docker binary instead of shell-wrapping bare
+		// `docker` — the GH #696 / MCP-2868 fix. Previously this branch always
+		// called wrapWithUserShell("docker", …), which failed with
+		// `command not found: docker` on a default Docker Desktop macOS install.
+		c.isDockerCommand = true
+
 		argsToWrap := args
-		isDirectDockerRun := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun
-		if isDirectDockerRun && len(c.config.Env) > 0 {
+		if len(c.config.Env) > 0 {
 			argsToWrap = c.injectEnvVarsIntoDockerArgs(args, c.config.Env)
 			c.logger.Debug("Injected env vars into direct docker command",
 				zap.String("server", c.config.Name),
 				zap.Int("env_count", len(c.config.Env)),
 				zap.Strings("modified_args", shellwrap.RedactDockerArgs(argsToWrap)))
 		}
 
-		// Use shell wrapping for environment inheritance
-		// This fixes issues when mcpproxy is launched via Launchd and doesn't inherit
-		// user's shell environment (like PATH customizations from .bashrc, .zshrc, etc.)
-		finalCommand, finalArgs = c.wrapWithUserShell(c.config.Command, argsToWrap)
-		c.isDockerCommand = false
+		var dockerShellWrapped bool
+		finalCommand, finalArgs, dockerShellWrapped = c.resolveDockerSpawn(argsToWrap)
 
-		// Handle explicit docker commands
-		if isDirectDockerRun {
-			c.isDockerCommand = true
-			if cidFile != "" {
-				// For shell-wrapped Docker commands, we need to modify the shell command string
+		// Insert --cidfile via the helper that matches how we spawn: args-based on
+		// the direct-exec path, string-based on the login-shell fallback.
+		if cidFile != "" {
+			if dockerShellWrapped {
 				finalArgs = c.insertCidfileIntoShellDockerCommand(finalArgs, cidFile)
+			} else {
+				finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile)
 			}
 		}
+	} else {
+		// Plain (non-docker) stdio command. Use shell wrapping for environment
+		// inheritance. This fixes issues when mcpproxy is launched via Launchd and
+		// doesn't inherit the user's shell environment (PATH customizations from
+		// .bashrc, .zshrc, etc.).
+		finalCommand, finalArgs = c.wrapWithUserShell(c.config.Command, args)
+		c.isDockerCommand = false
 	}
 
 	// Upstream transport with working directory support and process group management