diff --git a/docs/docker-isolation.md b/docs/docker-isolation.md index dca099a55..550a23c16 100644 --- a/docs/docker-isolation.md +++ b/docs/docker-isolation.md @@ -123,7 +123,7 @@ Starting in this release, MCPProxy emits a one-time warning in the main log when When anonymous telemetry is enabled, MCPProxy reports two Docker-related counters at daily cadence: -- `server_docker_available_bool` — whether the host has a reachable Docker daemon. Probed with `docker info --format {{.ServerVersion}}` and cached for up to 15 minutes (5 minutes when the previous probe failed, so a late Docker-Desktop launch is picked up promptly). +- `server_docker_available_bool` — whether Docker is actually invocable. Reported `true` only when the `docker` CLI is **resolvable to an absolute path** *and* `docker info --format {{.ServerVersion}}` succeeds (it does **not** fall back to a bare `docker` PATH probe, which could misreport availability when the binary is only inside the macOS app bundle — see issue #696). Cached for up to 15 minutes (5 minutes when the previous probe failed, so a late Docker-Desktop launch is picked up promptly). - `server_docker_isolated_count` — how many of your configured stdio servers are **configured** for isolation, i.e. servers for which `ShouldIsolate()` returns true. This is a configuration metric, not a count of running containers; it goes to zero whenever the global flag is off regardless of per-server opt-ins. ## Runtime Detection @@ -245,6 +245,23 @@ docker stats - Check container logs for specific error messages - Verify network access for package repositories +**`command not found: docker` on macOS (Docker Desktop installed):** +- Docker Desktop installed the default way leaves the `docker` CLI only inside + the app bundle at `/Applications/Docker.app/Contents/Resources/bin/docker` — + it is **not** on a standard `PATH` dir unless you ran the optional, + 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** before + spawning an isolated server (and also adds the bundle bin dir to the enhanced + spawn `PATH`), so isolation works even without the CLI-tools step. If you + still see this error, confirm the binary exists at the bundle path above, or + run Docker Desktop's "install CLI tools". +- `upstream_servers list` reports `docker_status.docker_path` (the resolved + binary) and reports `docker_status.available` / per-server `docker_available` + as `true` **only** when the CLI is actually resolvable *and* `docker info` + succeeds. A `false` value with `docker_path: ""` means the CLI could not be + resolved on the spawn path. + ## Security Considerations Docker isolation provides strong security boundaries but consider: diff --git a/internal/management/diagnostics.go b/internal/management/diagnostics.go index f4e6fc0b4..b462d4fe1 100644 --- a/internal/management/diagnostics.go +++ b/internal/management/diagnostics.go @@ -191,7 +191,14 @@ func (s *service) checkDockerDaemon() *contracts.DockerStatus { } dockerBin, resolveErr := shellwrap.ResolveDockerPath(logger) if resolveErr != nil || dockerBin == "" { - dockerBin = "docker" + // Honest availability (#696): if the CLI can't be resolved to an + // absolute path, Docker-isolated servers can't spawn it. Report + // unavailable with an actionable error rather than probing a bare + // "docker" that is not the binary used for spawning. + status.Available = false + status.Error = "Docker CLI not found on PATH or well-known install locations (on macOS the CLI ships at /Applications/Docker.app/Contents/Resources/bin/docker even without the optional CLI-tools step)" + s.logger.Debugw("Docker CLI not resolvable; reporting docker unavailable", "error", resolveErr) + return status } cmd := exec.CommandContext(ctx, dockerBin, "info", "--format", "{{.ServerVersion}}") output, err := cmd.Output() diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 978ccb697..19ea928d8 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -2564,12 +2564,20 @@ func (r *Runtime) IsDockerAvailable() bool { // tray with a minimal inherited PATH (see issue: tray "Docker daemon not // available" warning despite healthy daemon + socket). dockerBin, resolveErr := shellwrap.ResolveDockerPath(r.logger) + var err error + var newResult bool if resolveErr != nil || dockerBin == "" { - dockerBin = "docker" + // Honest availability (#696): if the CLI can't be resolved to an + // absolute path, Docker-isolated servers can't spawn it — report + // unavailable rather than probing a bare "docker" that is not the + // binary used for spawning. + err = resolveErr + newResult = false + } else { + cmd := exec.CommandContext(ctx, dockerBin, "info", "--format", "{{.ServerVersion}}") + err = cmd.Run() + newResult = err == nil } - cmd := exec.CommandContext(ctx, dockerBin, "info", "--format", "{{.ServerVersion}}") - err := cmd.Run() - newResult := err == nil // Log only on state changes (or first probe) so repeated heartbeats don't // spam the log. Rationale: users care about transitions ("Docker just diff --git a/internal/secureenv/manager.go b/internal/secureenv/manager.go index f92065c30..341aa5fa5 100644 --- a/internal/secureenv/manager.go +++ b/internal/secureenv/manager.go @@ -136,10 +136,26 @@ func (m *Manager) discoverPaths() *PathDiscovery { return discovery } -// discoverUnixPaths discovers common Unix/macOS tool paths +// discoverUnixPaths discovers common Unix/macOS tool paths that actually exist. func (m *Manager) discoverUnixPaths() []string { + // Filter the platform candidate list to only paths that actually exist. + var existingPaths []string + for _, path := range unixCandidatePaths() { + if _, err := os.Stat(path); err == nil { + existingPaths = append(existingPaths, path) + } + } + + return existingPaths +} + +// unixCandidatePaths returns the ordered list of well-known Unix/macOS tool +// directories to probe for existence. Split out (pure, no I/O) so platform- +// specific inclusion can be unit-tested without depending on what is installed +// on the test host. +func unixCandidatePaths() []string { commonPaths := []string{ - "/usr/local/bin", // Homebrew, Docker Desktop, etc. + "/usr/local/bin", // Homebrew, Docker Desktop symlink, etc. "/usr/bin", // System binaries "/bin", // Core system binaries "/opt/homebrew/bin", // Apple Silicon Homebrew @@ -148,6 +164,16 @@ func (m *Manager) discoverUnixPaths() []string { "/sbin", // System admin binaries } + // macOS (#696): Docker Desktop installed the default way (without the + // optional, admin-gated "install CLI tools" step) leaves the docker CLI + // only inside the app bundle, which is not on any standard PATH dir. Adding + // it (defense in depth for the absolute-path spawn fix) lets nested/child + // docker resolution work for isolated servers. Existence is filtered by the + // caller, so this is a no-op on hosts without Docker Desktop. + if runtime.GOOS == "darwin" { + commonPaths = append(commonPaths, "/Applications/Docker.app/Contents/Resources/bin") + } + // Add user-specific paths if homeDir, err := os.UserHomeDir(); err == nil { commonPaths = append(commonPaths, @@ -159,15 +185,7 @@ func (m *Manager) discoverUnixPaths() []string { ) } - // Filter to only include paths that actually exist - var existingPaths []string - for _, path := range commonPaths { - if _, err := os.Stat(path); err == nil { - existingPaths = append(existingPaths, path) - } - } - - return existingPaths + return commonPaths } // discoverWindowsPaths discovers common Windows tool paths diff --git a/internal/secureenv/unix_candidate_paths_test.go b/internal/secureenv/unix_candidate_paths_test.go new file mode 100644 index 000000000..033ea65c4 --- /dev/null +++ b/internal/secureenv/unix_candidate_paths_test.go @@ -0,0 +1,33 @@ +package secureenv + +import ( + "runtime" + "testing" + + "github.com/stretchr/testify/assert" +) + +// dockerBundleBinDir is the macOS Docker Desktop app-bundle bin dir that holds +// the docker CLI even when the optional, admin-gated "install CLI tools" step +// was skipped (issue #696). +const dockerBundleBinDir = "/Applications/Docker.app/Contents/Resources/bin" + +// TestUnixCandidatePathsIncludesDockerBundleOnMacOS asserts that the macOS +// Docker Desktop app-bundle bin dir is among the candidate spawn-PATH dirs on +// darwin (defense in depth for the absolute-path docker spawn fix), and is not +// injected on other platforms. +func TestUnixCandidatePathsIncludesDockerBundleOnMacOS(t *testing.T) { + paths := unixCandidatePaths() + + if runtime.GOOS == "darwin" { + assert.Contains(t, paths, dockerBundleBinDir, + "macOS candidate paths must include the Docker Desktop bundle bin dir (#696)") + } else { + assert.NotContains(t, paths, dockerBundleBinDir, + "non-macOS candidate paths must not include the macOS-only bundle bin dir") + } + + // Existing well-known dirs must be preserved regardless of platform. + assert.Contains(t, paths, "/usr/local/bin") + assert.Contains(t, paths, "/usr/bin") +} diff --git a/internal/server/docker_status_test.go b/internal/server/docker_status_test.go new file mode 100644 index 000000000..417b0d2ad --- /dev/null +++ b/internal/server/docker_status_test.go @@ -0,0 +1,76 @@ +package server + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +// TestResolveDockerStatusUnresolvableReportsUnavailable verifies the honesty +// fix (#696): when the docker CLI cannot be resolved to an absolute path, +// docker is reported unavailable with an empty path — NOT available via a bare +// "docker" probe that is not the binary used for spawning. +func TestResolveDockerStatusUnresolvableReportsUnavailable(t *testing.T) { + orig := dockerPathResolver + t.Cleanup(func() { dockerPathResolver = orig }) + dockerPathResolver = func(_ *zap.Logger) (string, error) { + return "", assert.AnError + } + + p := &MCPProxyServer{logger: zap.NewNop()} + available, path := p.resolveDockerStatus() + + assert.False(t, available, "docker must be reported unavailable when unresolvable") + assert.Equal(t, "", path, "docker_path must be empty when unresolvable") +} + +// TestResolveDockerStatusResolvableAndWorking verifies that a resolvable docker +// whose `docker info` exits 0 is reported available with its resolved path. +func TestResolveDockerStatusResolvableAndWorking(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses a shell-script fake docker; not portable to Windows") + } + + // A fake "docker" that exits 0 for any args (including `info`). + dir := t.TempDir() + fakeDocker := filepath.Join(dir, "docker") + require.NoError(t, os.WriteFile(fakeDocker, []byte("#!/bin/sh\nexit 0\n"), 0o755)) + + orig := dockerPathResolver + t.Cleanup(func() { dockerPathResolver = orig }) + dockerPathResolver = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } + + p := &MCPProxyServer{logger: zap.NewNop()} + available, path := p.resolveDockerStatus() + + assert.True(t, available, "docker must be reported available when resolvable and `info` succeeds") + assert.Equal(t, fakeDocker, path, "docker_path must surface the resolved binary") +} + +// TestResolveDockerStatusResolvableDaemonDown verifies that a resolvable docker +// whose `docker info` fails (daemon down) is reported unavailable but still +// surfaces the resolved path for diagnostics. +func TestResolveDockerStatusResolvableDaemonDown(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses a shell-script fake docker; not portable to Windows") + } + + dir := t.TempDir() + fakeDocker := filepath.Join(dir, "docker") + require.NoError(t, os.WriteFile(fakeDocker, []byte("#!/bin/sh\nexit 1\n"), 0o755)) + + orig := dockerPathResolver + t.Cleanup(func() { dockerPathResolver = orig }) + dockerPathResolver = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } + + p := &MCPProxyServer{logger: zap.NewNop()} + available, path := p.resolveDockerStatus() + + assert.False(t, available, "docker must be reported unavailable when `info` fails") + assert.Equal(t, fakeDocker, path, "docker_path must surface the resolved binary even when the daemon is down") +} diff --git a/internal/server/mcp.go b/internal/server/mcp.go index a2c8e8e27..14a3986c4 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -123,6 +123,7 @@ type MCPProxyServer struct { // Docker availability cache dockerAvailableCache *bool + dockerPathCache string dockerCacheTime time.Time // JavaScript runtime pool for code execution @@ -2889,8 +2890,9 @@ func (p *MCPProxyServer) handleListUpstreams(ctx context.Context) (*mcp.CallTool // Check Docker availability only if Docker isolation is globally enabled dockerIsolationGlobalEnabled := p.config.DockerIsolation != nil && p.config.DockerIsolation.Enabled var dockerAvailable bool + var dockerPath string if dockerIsolationGlobalEnabled { - dockerAvailable = p.checkDockerAvailable() + dockerAvailable, dockerPath = p.resolveDockerStatus() } // Enhance server list with connection status and Docker isolation info @@ -3062,6 +3064,7 @@ func (p *MCPProxyServer) handleListUpstreams(ctx context.Context) (*mcp.CallTool "total": len(servers), "docker_status": map[string]interface{}{ "available": dockerAvailable, + "docker_path": dockerPath, "global_enabled": dockerIsolationGlobalEnabled, "isolation_config": p.config.DockerIsolation, }, @@ -3069,9 +3072,9 @@ func (p *MCPProxyServer) handleListUpstreams(ctx context.Context) (*mcp.CallTool if !dockerAvailable && dockerIsolationGlobalEnabled { result["warnings"] = []string{ - "Docker isolation is enabled but Docker daemon is not available", + "Docker isolation is enabled but the Docker CLI is not resolvable / the daemon is not reachable", "Servers configured for isolation will fail to start", - "Install Docker or disable isolation in config", + "Install Docker (on macOS the CLI ships at /Applications/Docker.app/Contents/Resources/bin/docker even without the optional CLI-tools step), or disable isolation in config", } } @@ -3189,35 +3192,52 @@ func (p *MCPProxyServer) handleDoctor(ctx context.Context, request mcp.CallToolR return mcp.NewToolResultError("Management service not available"), nil } -// checkDockerAvailable checks if Docker daemon is available with caching -func (p *MCPProxyServer) checkDockerAvailable() bool { +// dockerPathResolver resolves the absolute docker path. Indirected through a +// package var so tests can stub resolution without a real Docker install. +var dockerPathResolver = shellwrap.ResolveDockerPath + +// resolveDockerStatus reports whether docker is actually invocable and the +// absolute path it resolved to (empty when unresolvable). Result cached 30s. +// +// Honesty (#696): docker is reported available ONLY when the CLI is RESOLVABLE +// (ResolveDockerPath succeeds) AND `docker info` succeeds. We deliberately do +// NOT fall back to a bare "docker" probe when resolution fails — that would +// report available:true even though Docker-isolated servers (which invoke +// docker by its resolved absolute path) cannot launch it, exactly the +// misreport in issue #696. +func (p *MCPProxyServer) resolveDockerStatus() (available bool, dockerPath string) { // Cache result for 30 seconds to avoid repeated expensive checks now := time.Now() if p.dockerAvailableCache != nil && now.Sub(p.dockerCacheTime) < 30*time.Second { - return *p.dockerAvailableCache + return *p.dockerAvailableCache, p.dockerPathCache } - ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) - defer cancel() // Resolve docker via shellwrap so tray-launched processes with a minimal // inherited PATH still find Docker Desktop / Homebrew / Colima installs. - dockerBin, resolveErr := shellwrap.ResolveDockerPath(p.logger) + dockerBin, resolveErr := dockerPathResolver(p.logger) if resolveErr != nil || dockerBin == "" { - dockerBin = "docker" + p.logger.Debug("Docker CLI not resolvable; reporting docker unavailable", zap.Error(resolveErr)) + unavailable := false + p.dockerAvailableCache = &unavailable + p.dockerPathCache = "" + p.dockerCacheTime = now + return false, "" } - cmd := exec.CommandContext(ctx, dockerBin, "info") - err := cmd.Run() - available := err == nil + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) + defer cancel() + err := exec.CommandContext(ctx, dockerBin, "info").Run() + available = err == nil // Cache the result p.dockerAvailableCache = &available + p.dockerPathCache = dockerBin p.dockerCacheTime = now if !available { - p.logger.Debug("Docker daemon not available", zap.Error(err)) + p.logger.Debug("Docker daemon not available", zap.String("docker_path", dockerBin), zap.Error(err)) } - return available + return available, dockerBin } // getIsolationManager returns the isolation manager for checking settings diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index fd3c098e2..e2d882b45 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -4,9 +4,16 @@ import ( "fmt" "strings" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/shellwrap" "go.uber.org/zap" ) +// resolveDockerBinary resolves the absolute path to the `docker` binary. It is +// indirected through a package var (rather than calling shellwrap.ResolveDockerPath +// directly) so tests can stub resolution without a real Docker install. Mirrors +// newDockerCmd's resolution so spawn and runtime-cleanup use the same binary. +var resolveDockerBinary = shellwrap.ResolveDockerPath + // setupDockerIsolation configures Docker isolation for the MCP server process. // Returns the docker command and arguments to execute. func (c *Client) setupDockerIsolation(command string, args []string) (dockerCommand string, dockerArgs []string) { @@ -54,9 +61,27 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm zap.String("container_command", containerCommand)) } - // CRITICAL FIX: Wrap Docker command with user shell to inherit proper PATH - // This fixes issues when mcpproxy is launched via Launchpad/GUI where PATH doesn't include Docker - return c.wrapWithUserShell(cmdDocker, finalArgs) + // CRITICAL FIX (#696): resolve `docker` to an ABSOLUTE path before shell-wrapping. + // 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. Invoking docker by absolute path — mirroring + // newDockerCmd — bypasses PATH entirely so isolated servers spawn successfully. + // Fall back to the bare "docker" command only when resolution fails. + // + // We still wrap in the user login shell so env-var inheritance and the + // existing cidfile insertion (which scans the command string for "docker run") + // keep working; the trailing "docker run" substring matches the absolute path. + dockerBin := cmdDocker + if resolved, resErr := resolveDockerBinary(c.logger); resErr == nil && resolved != "" { + dockerBin = resolved + } else if resErr != nil { + c.logger.Warn("Could not resolve docker to an absolute path; falling back to bare 'docker' (isolated server may fail if docker is not on the spawn PATH)", + zap.String("server", c.config.Name), + zap.Error(resErr)) + } + return c.wrapWithUserShell(dockerBin, finalArgs) } // injectEnvVarsIntoDockerArgs injects environment variables as -e flags into Docker run args @@ -84,15 +109,23 @@ func (c *Client) injectEnvVarsIntoDockerArgs(args []string, envVars map[string]s // insertCidfileIntoShellDockerCommand inserts --cidfile into a shell-wrapped Docker command func (c *Client) insertCidfileIntoShellDockerCommand(shellArgs []string, cidFile string) []string { - // Shell args typically look like: ["-l", "-c", "docker run -i --rm mcp/duckduckgo"] - // Fix: Check for correct shell format - args can be 2 or 3 elements - if len(shellArgs) < 2 || shellArgs[len(shellArgs)-2] != "-c" { - // If it's not the expected format, log error and fall back + // Shell args look like: + // Unix/bash: ["-l", "-c", "docker run …"] → second-to-last is "-c" + // Windows cmd: ["/c", "docker run …"] → second-to-last is "/c" + // Accept both flags so cidfile insertion works on all platforms. + if len(shellArgs) < 2 { + c.logger.Error("Unexpected shell command format for Docker cidfile insertion - cannot track container ID", + zap.String("server", c.config.Name), + zap.Strings("shell_args", shellArgs), + zap.String("expected_format", "[shell, -c, docker_command] or [-l, -c, docker_command]")) + return shellArgs + } + secondToLast := shellArgs[len(shellArgs)-2] + if secondToLast != "-c" && secondToLast != "/c" { c.logger.Error("Unexpected shell command format for Docker cidfile insertion - cannot track container ID", zap.String("server", c.config.Name), zap.Strings("shell_args", shellArgs), zap.String("expected_format", "[shell, -c, docker_command] or [-l, -c, docker_command]")) - // Don't append cidfile to shell args as it won't work return shellArgs } diff --git a/internal/upstream/core/connection_docker_test.go b/internal/upstream/core/connection_docker_test.go new file mode 100644 index 000000000..42ff2d0a1 --- /dev/null +++ b/internal/upstream/core/connection_docker_test.go @@ -0,0 +1,102 @@ +package core + +import ( + "fmt" + "strings" + "testing" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +func newIsolatedTestClient() *Client { + return &Client{ + config: &config.ServerConfig{ + Name: "iso-server", + Command: "python", + Args: []string{"-m", "mcp_server"}, + }, + logger: zap.NewNop(), + isolationManager: NewIsolationManager(config.DefaultDockerIsolationConfig()), + } +} + +// TestSetupDockerIsolationUsesResolvedAbsolutePath verifies the root-cause fix +// for #696: an isolated server is spawned with docker invoked by its resolved +// ABSOLUTE path (bypassing PATH), not the bare "docker" command that the +// login-shell PATH may be unable to resolve. +func TestSetupDockerIsolationUsesResolvedAbsolutePath(t *testing.T) { + const fakeDocker = "/opt/fake/Docker.app/Contents/Resources/bin/docker" + + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } + + c := newIsolatedTestClient() + shellCmd, shellArgs := c.setupDockerIsolation(c.config.Command, c.config.Args) + + require.NotEmpty(t, shellCmd, "shell command must not be empty") + require.NotEmpty(t, shellArgs, "shell args must not be empty") + + // The shell-wrapped command string is the last shell arg. + cmdStr := shellArgs[len(shellArgs)-1] + assert.Contains(t, cmdStr, fakeDocker, + "docker must be invoked by its resolved absolute path, got: %s", cmdStr) + assert.False(t, strings.HasPrefix(cmdStr, "docker run"), + "docker must not be invoked as the bare 'docker' command, got: %s", cmdStr) +} + +// TestSetupDockerIsolationCidfileInsertionWithAbsolutePath verifies the +// existing cidfile-insertion logic still matches when docker is referenced by +// its absolute path (the trailing "docker run" substring is preserved). +func TestSetupDockerIsolationCidfileInsertionWithAbsolutePath(t *testing.T) { + const fakeDocker = "/opt/fake/Docker.app/Contents/Resources/bin/docker" + + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } + + c := newIsolatedTestClient() + _, shellArgs := c.setupDockerIsolation(c.config.Command, c.config.Args) + withCid := c.insertCidfileIntoShellDockerCommand(shellArgs, "/tmp/cid.txt") + + cmdStr := withCid[len(withCid)-1] + assert.Contains(t, cmdStr, fakeDocker+" run --cidfile /tmp/cid.txt", + "cidfile must be inserted right after the absolute-path docker run, got: %s", cmdStr) +} + +// TestInsertCidfileWindowsCmdFormat verifies cidfile insertion works with the +// Windows cmd.exe shell-wrap format: ["/c", "docker run …"] (second-to-last +// arg is "/c", not "-c"). This is a cross-platform unit test for the guard +// that previously rejected the /c flag as an unrecognised format. +func TestInsertCidfileWindowsCmdFormat(t *testing.T) { + const fakeDocker = "/opt/fake/bin/docker" + c := newIsolatedTestClient() + // Simulate Windows cmd.exe output: shell returns ["/c", cmdStr] + windowsShellArgs := []string{"/c", fakeDocker + " run --rm -i ghcr.io/some/image python -m srv"} + withCid := c.insertCidfileIntoShellDockerCommand(windowsShellArgs, "/tmp/cid.txt") + cmdStr := withCid[len(withCid)-1] + assert.Contains(t, cmdStr, fakeDocker+" run --cidfile /tmp/cid.txt", + "cidfile must be inserted in Windows cmd /c format too, got: %s", cmdStr) +} + +// TestSetupDockerIsolationFallsBackToBareDocker verifies that when docker +// cannot be resolved to an absolute path, the spawn falls back to the bare +// "docker" command (preserving prior behavior / login-shell resolution). +func TestSetupDockerIsolationFallsBackToBareDocker(t *testing.T) { + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { + return "", fmt.Errorf("docker not found") + } + + c := newIsolatedTestClient() + _, shellArgs := c.setupDockerIsolation(c.config.Command, c.config.Args) + + 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) +} diff --git a/internal/upstream/core/monitoring.go b/internal/upstream/core/monitoring.go index 0e743cae5..ffeb2eb8a 100644 --- a/internal/upstream/core/monitoring.go +++ b/internal/upstream/core/monitoring.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "os" + "regexp" "strings" "time" @@ -326,13 +327,23 @@ func (c *Client) RecentStderrSnapshot() []string { // formatRecentStderr returns a human-readable, indented block of recent // stderr lines suitable for embedding in an error message. Empty when no // stderr has been captured. +// +// Two readability transforms are applied (#696): a "command not found" +// actionable hint is led when the child failed to resolve a binary (notably +// docker), and runs of identical consecutive lines are collapsed into a single +// "… (repeated N×)" entry so a process that prints the same error on each of +// its ~20 connection retries produces one readable line instead of a wall. func (c *Client) formatRecentStderr() string { lines := c.RecentStderrSnapshot() if len(lines) == 0 { return "" } var b strings.Builder - for _, l := range lines { + if hint := commandNotFoundHint(lines); hint != "" { + b.WriteString(hint) + b.WriteByte('\n') + } + for _, l := range collapseRepeatedLines(lines) { b.WriteString(" | ") b.WriteString(l) b.WriteByte('\n') @@ -340,6 +351,65 @@ func (c *Client) formatRecentStderr() string { return strings.TrimRight(b.String(), "\n") } +// collapseRepeatedLines collapses runs of identical consecutive lines into a +// single " (repeated N×)" entry. Non-repeated lines pass through verbatim. +func collapseRepeatedLines(lines []string) []string { + if len(lines) == 0 { + return nil + } + out := make([]string, 0, len(lines)) + for i := 0; i < len(lines); { + j := i + 1 + for j < len(lines) && lines[j] == lines[i] { + j++ + } + if n := j - i; n > 1 { + out = append(out, fmt.Sprintf("%s (repeated %d×)", lines[i], n)) + } else { + out = append(out, lines[i]) + } + i = j + } + return out +} + +var ( + // cmdNotFoundZshRe matches zsh's form: "zsh:1: command not found: docker". + cmdNotFoundZshRe = regexp.MustCompile(`command not found: (\S+)`) + // cmdNotFoundBashRe matches bash/sh's form: "bash: docker: command not found". + cmdNotFoundBashRe = regexp.MustCompile(`([^\s:]+): command not found`) +) + +// extractMissingCommand returns the name of a binary the shell could not find +// in a stderr line, or "" if the line is not a "command not found" error. +func extractMissingCommand(line string) string { + if m := cmdNotFoundZshRe.FindStringSubmatch(line); m != nil { + return strings.Trim(m[1], `"'`) + } + if m := cmdNotFoundBashRe.FindStringSubmatch(line); m != nil { + return strings.Trim(m[1], `"'`) + } + return "" +} + +// commandNotFoundHint scans captured stderr for a shell "command not found" +// error and returns a single actionable message, or "" if none is present. The +// docker-specific case (#696) points the user at the app-bundle binary that +// Docker Desktop ships even when the optional CLI-tools step was skipped. +func commandNotFoundHint(lines []string) string { + for _, l := range lines { + cmd := extractMissingCommand(l) + if cmd == "" { + continue + } + if cmd == "docker" { + return "Docker CLI not found on PATH. Install Docker Desktop CLI tools, or it is bundled at /Applications/Docker.app/Contents/Resources/bin/docker — restart the affected servers." + } + return fmt.Sprintf("Command %q not found on the spawn PATH. Ensure it is installed and on PATH, then restart the affected servers.", cmd) + } + return "" +} + func shortContainerID(id string) string { if len(id) <= 12 { return id diff --git a/internal/upstream/core/monitoring_stderr_test.go b/internal/upstream/core/monitoring_stderr_test.go new file mode 100644 index 000000000..39d445a91 --- /dev/null +++ b/internal/upstream/core/monitoring_stderr_test.go @@ -0,0 +1,77 @@ +package core + +import ( + "strings" + "testing" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + + "github.com/stretchr/testify/assert" + "go.uber.org/zap" +) + +func TestExtractMissingCommand(t *testing.T) { + cases := []struct { + line string + want string + }{ + {"zsh:1: command not found: docker", "docker"}, + {"bash: docker: command not found", "docker"}, + {"bash: line 1: npx: command not found", "npx"}, + {"sh: 1: uvx: not found", ""}, // dash form not matched; not a regression target + {"some unrelated stderr line", ""}, + {"", ""}, + } + for _, tc := range cases { + assert.Equal(t, tc.want, extractMissingCommand(tc.line), "line=%q", tc.line) + } +} + +func TestCommandNotFoundHintDocker(t *testing.T) { + hint := commandNotFoundHint([]string{ + "zsh:1: command not found: docker", + "zsh:1: command not found: docker", + }) + assert.Contains(t, hint, "Docker CLI not found on PATH") + assert.Contains(t, hint, "/Applications/Docker.app/Contents/Resources/bin/docker") +} + +func TestCommandNotFoundHintGeneric(t *testing.T) { + hint := commandNotFoundHint([]string{"bash: line 1: npx: command not found"}) + assert.Contains(t, hint, "npx") + assert.NotContains(t, hint, "Docker CLI not found") +} + +func TestCommandNotFoundHintNone(t *testing.T) { + assert.Equal(t, "", commandNotFoundHint([]string{"server listening on stdio", "ready"})) +} + +func TestCollapseRepeatedLines(t *testing.T) { + in := []string{"a", "a", "a", "b", "c", "c"} + got := collapseRepeatedLines(in) + assert.Equal(t, []string{"a (repeated 3×)", "b", "c (repeated 2×)"}, got) + + // Non-consecutive duplicates are not collapsed. + assert.Equal(t, []string{"a", "b", "a"}, collapseRepeatedLines([]string{"a", "b", "a"})) + assert.Nil(t, collapseRepeatedLines(nil)) +} + +// TestFormatRecentStderrDockerWall verifies the end-to-end transform: a wall of +// ~20 identical "command not found: docker" lines becomes a single actionable +// hint plus one collapsed line instead of a 20-line wall (#696). +func TestFormatRecentStderrDockerWall(t *testing.T) { + c := &Client{ + config: &config.ServerConfig{Name: "iso"}, + logger: zap.NewNop(), + } + for i := 0; i < 21; i++ { + c.recordRecentStderr("zsh:1: command not found: docker") + } + + out := c.formatRecentStderr() + assert.Contains(t, out, "Docker CLI not found on PATH") + assert.Contains(t, out, "(repeated") + // The raw line must not appear 20 separate times. + assert.LessOrEqual(t, strings.Count(out, "command not found: docker"), 2, + "repeated identical lines must be collapsed, got:\n%s", out) +}