fix(docker): resolve docker by absolute path on macOS spawn; honest docker_available (#696) (#697)

* fix(docker): resolve docker by absolute path on macOS spawn; honest docker_available (#696)

On macOS, Docker Desktop installed the default way (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. mcpproxy spawned isolated servers with bare
`docker` on a sanitized PATH that omits the bundle dir, so docker was
unresolvable at spawn (~20x `command not found: docker`), while
`upstream_servers list` misreported `docker_available: true`.

- Spawn (root cause): setupDockerIsolation resolves docker to an absolute path
  via shellwrap.ResolveDockerPath (mirroring newDockerCmd) before shell-wrapping;
  falls back to bare "docker" only when resolution fails.
- Enhanced spawn PATH (defense in depth): add the Docker Desktop bundle bin dir
  on darwin; split the candidate list into a pure, unit-testable function.
- Honest availability: MCPProxyServer.resolveDockerStatus and
  Runtime.IsDockerAvailable report available only when the CLI is resolvable AND
  `docker info` succeeds — no bare-docker fallback that would misreport. Surface
  the resolved binary as docker_status.docker_path; diagnostics.checkDockerDaemon
  reports unavailable with an actionable error when unresolvable.
- Diagnostics: formatRecentStderr leads a single actionable "Docker CLI not
  found" hint on `command not found` and collapses identical consecutive lines
  into "… (repeated N×)", replacing the 20-line wall.

macOS-specific paths guarded behind runtime.GOOS=="darwin"; Linux/Windows
unaffected. Docs updated for docker_path, telemetry counter semantics, and a
new troubleshooting entry.

Related #696

* fix(docker): accept /c (Windows cmd) in cidfile insertion + regression test

insertCidfileIntoShellDockerCommand checked shellArgs[n-2]=="-c" to
distinguish a shell-wrapped command string from an unrecognised format.
On Windows with cmd.exe (no bash), WrapWithUserShell returns ["/c", cmdStr]
so the flag is "/c", not "-c"; the guard rejected it and silently skipped
cidfile insertion, breaking TestSetupDockerIsolationCidfileInsertionWithAbsolutePath
on windows-latest CI.

Accept both flags ("-c" and "/c") so cidfile insertion works on every
platform. Add TestInsertCidfileWindowsCmdFormat (pure unit, no real Docker)
to pin the Windows cmd format.

Author: Dumbris

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:

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()

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

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

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")
+}

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")
+}

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,16 +3064,17 @@ 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,
 		},
 	}
 
 	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