fix(upstream): launcher user-supplied docker run misses bundle-dir PATH + direct-exec (MCP-2881)

internal/upstream/core/connection_launcher.go

@@ -261,16 +261,41 @@ func (c *Client) buildLauncherCmd(_ context.Context, willUseDocker bool) (*exec.
 				finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile)
 			}
 		}
-	} else {
+	} 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 launched via the launcher path
+		// (config.Command IS `docker`). Reuse the SAME resolve→spawn decision as the
+		// isolation path and the stdio path (resolveDockerSpawn) so this entrypoint
+		// also direct-execs the resolved ABSOLUTE docker binary instead of
+		// shell-wrapping bare `docker`, and gets the docker bundle dir prepended to
+		// the child PATH (#715 / MCP-2881). Before this it always shell-wrapped bare
+		// `docker` with no PATH augmentation, so a registry pull of an uncached image
+		// could fail with `docker-credential-desktop … not found in $PATH`.
 		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)
 		}
-		finalCommand, finalArgs = c.wrapWithUserShell(c.config.Command, argsToWrap)
-		if isDirectDockerRun && cidFile != "" {
-			finalArgs = c.insertCidfileIntoShellDockerCommand(finalArgs, cidFile)
+
+		var dockerShellWrapped bool
+		var dockerDir string
+		finalCommand, finalArgs, dockerShellWrapped, dockerDir = c.resolveDockerSpawn(argsToWrap)
+
+		// Prepend the docker bundle dir to the child PATH so the spawned docker can
+		// exec its sibling credential helper / tooling on a registry pull (#715).
+		// No-op when docker did not resolve to an absolute path.
+		envVars = prependDockerDirToPath(envVars, dockerDir)
+
+		// 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) launcher command. Shell-wrap for login-env inheritance.
+		finalCommand, finalArgs = c.wrapWithUserShell(c.config.Command, args)
 	}
 
 	cmd := exec.Command(finalCommand, finalArgs...)

internal/upstream/core/connection_launcher_test.go

@@ -0,0 +1,123 @@
+package core
+
+import (
+	"context"
+	"fmt"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/secureenv"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+)
+
+// TestBuildLauncherCmdUserSuppliedDockerRunDirectExec is the MCP-2881 root-cause
+// assertion. A user-supplied `docker run` upstream launched via the LAUNCHER
+// path (HTTP/SSE transport that also sets Command, so mcpproxy owns the process
+// lifecycle) must reuse the SAME resolve→direct-exec + bundle-dir PATH wiring as
+// the stdio path. Before this fix the launcher's else branch always shell-wrapped
+// bare `docker` and never prepended the docker bundle dir to the child PATH, so
+// the #715 credential-helper fix never reached this entrypoint: an uncached image
+// pull could still fail with `docker-credential-desktop ... not found in $PATH`.
+func TestBuildLauncherCmdUserSuppliedDockerRunDirectExec(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()
+	c.envManager = secureenv.NewManager(nil)
+
+	cmd, isDocker, cidFile, err := c.buildLauncherCmd(context.Background(), true)
+	require.NoError(t, err)
+	assert.True(t, isDocker)
+
+	// Direct-exec of the resolved ABSOLUTE docker binary — NOT a `$SHELL -l -c` wrap.
+	assert.Equal(t, fakeDocker, cmd.Path,
+		"launcher user-supplied docker run must direct-exec the resolved absolute docker, got: %s", cmd.Path)
+	require.GreaterOrEqual(t, len(cmd.Args), 2)
+	assert.Equal(t, fakeDocker, cmd.Args[0])
+	assert.Equal(t, cmdRun, cmd.Args[1],
+		"second argv token must be the raw 'run' token, not a shell -c string, got: %v", cmd.Args)
+
+	// Injected -e env flag must survive into the docker run argv.
+	assert.Subset(t, cmd.Args, []string{"-e", "SLACK_TOKEN=xoxb-secret"},
+		"injected -e env flags must be present in the direct-exec argv, got: %v", cmd.Args)
+
+	// cidfile inserted right after 'run' via the args-based helper.
+	require.NotEmpty(t, cidFile)
+	assert.Subset(t, cmd.Args, []string{"--cidfile", cidFile},
+		"cidfile must be inserted via the args-based helper on the direct-exec path, got: %v", cmd.Args)
+
+	// The docker bundle dir must be prepended to the spawn env PATH (#715).
+	wantDir := filepath.Dir(fakeDocker)
+	var path string
+	for _, kv := range cmd.Env {
+		if strings.HasPrefix(kv, "PATH=") {
+			path = strings.TrimPrefix(kv, "PATH=")
+		}
+	}
+	parts := filepath.SplitList(path)
+	require.NotEmpty(t, parts, "spawn env must carry a PATH entry")
+	assert.Equal(t, wantDir, parts[0],
+		"docker bundle dir must be prepended to the launcher spawn env PATH, got: %s", path)
+}
+
+// TestBuildLauncherCmdUserSuppliedDockerRunShellWrapFallback verifies that when
+// docker cannot be resolved to a verified absolute executable, the launcher path
+// falls back to a login-shell wrap (preserving #696's last-resort PATH lookup),
+// mirroring the stdio path — rather than producing a broken direct-exec.
+func TestBuildLauncherCmdUserSuppliedDockerRunShellWrapFallback(t *testing.T) {
+	orig := resolveDockerBinary
+	t.Cleanup(func() { resolveDockerBinary = orig })
+	resolveDockerBinary = func(_ *zap.Logger) (string, error) {
+		return "", fmt.Errorf("docker not found")
+	}
+
+	c := newUserSuppliedDockerTestClient()
+	c.envManager = secureenv.NewManager(nil)
+
+	cmd, isDocker, _, err := c.buildLauncherCmd(context.Background(), true)
+	require.NoError(t, err)
+	assert.True(t, isDocker)
+	assert.NotEqual(t, cmdDocker, cmd.Path,
+		"shell-wrap fallback exec's the login shell, not bare 'docker', got: %s", cmd.Path)
+
+	last := cmd.Args[len(cmd.Args)-1]
+	assert.Contains(t, last, "docker run",
+		"fallback must shell-wrap a 'docker run' command string, got: %s", last)
+	assert.Contains(t, last, "-e SLACK_TOKEN=xoxb-secret",
+		"injected -e env flag must survive into the shell command string, got: %s", last)
+}
+
+// TestBuildLauncherCmdPlainCommandStillShellWraps is the no-regression guard: a
+// plain (non-docker) launcher command must still be shell-wrapped for login-env
+// inheritance and must NOT receive docker PATH augmentation or a cidfile.
+func TestBuildLauncherCmdPlainCommandStillShellWraps(t *testing.T) {
+	c := &Client{
+		config: &config.ServerConfig{
+			Name:    "plain-launcher",
+			Command: "my-server",
+			Args:    []string{"--port", "9000"},
+			URL:     "http://127.0.0.1:9000",
+		},
+		logger:           zap.NewNop(),
+		isolationManager: NewIsolationManager(config.DefaultDockerIsolationConfig()),
+		envManager:       secureenv.NewManager(nil),
+	}
+
+	cmd, isDocker, cidFile, err := c.buildLauncherCmd(context.Background(), false)
+	require.NoError(t, err)
+	assert.False(t, isDocker)
+	assert.Empty(t, cidFile, "non-docker launcher command must not create a cidfile")
+
+	last := cmd.Args[len(cmd.Args)-1]
+	assert.Contains(t, last, "my-server --port 9000",
+		"plain command must be shell-wrapped, got: %v", cmd.Args)
+}