fix(docker): direct-exec resolved docker binary for isolated spawn (#696 follow-up)

[Dumbris]

Summary

Docker-isolated upstream servers still failed to start with command not found: docker when the docker CLI is not on the spawn PATH (Docker Desktop installed without the optional CLI-tools step), even after PR #697. #697 resolved the absolute path but still routed the spawn through $SHELL -l -c "<docker> run …", where the login shell re-derives $PATH from rc files and can drop the bundle dir. The absolute path was only a token inside the -c string, so the bug persisted while upstream_servers list reported a healthy docker_path (same resolver, different time/path → "status available, spawn not found").

Related #696. Branched from origin/main (contains #697 / 79849e0c).

Changes

A — direct-exec the resolved docker binary (primary). On successful resolution, setupDockerIsolation now returns the resolved absolute docker binary as the exec command with the raw docker argv (no login-shell wrap), mirroring the management path's newDockerCmd (exec.CommandContext(dockerBin, …)). It returns a shellWrapped flag so callers select the matching cidfile helper. Login-shell wrap of bare docker is kept only on the resolution-failure fallback. The spawn env already carries the enhanced PATH (BuildSecureEnvironment, EnhancePath=true), and exec'ing an absolute path needs no PATH at all.

• New insertCidfileIntoDockerArgs inserts --cidfile <file> right after the run token in the argv slice — platform-agnostic, sidestepping the -c vs /c shell quirk. Both call sites (connection_stdio.go, connection_launcher.go) branch on shellWrapped.

B — negative-cache cannot shadow a live os.Stat probe. ResolveDockerPath re-runs the cheap, sandbox-immune well-known os.Stat probe before honoring a still-live cached negative, so a failure cached when only the restricted login-shell leg failed can never permanently shadow a docker binary sitting at a well-known path.

Tests (TDD — written first, watched fail, then implemented)

• TestSetupDockerIsolationExecsResolvedBinaryDirectly (new, the regression that failed today): asserts finalCommand == <resolved>, args[0] == "run", no shell -c indirection.
• Updated TestSetupDockerIsolationUsesResolvedAbsolutePath, …CidfileInsertionWithAbsolutePath, …FallsBackToBareDocker, TestInsertCidfileWindowsCmdFormat for the args-based cidfile + direct-exec shape; added TestInsertCidfileIntoDockerArgs.
• TestResolveDockerPath_StatProbeOverridesLiveNegative (new) — a still-live negative must not shadow a now-present well-known docker.

Verification

go test ./internal/upstream/... ./internal/shellwrap/... -race   # ok
go build ./cmd/mcpproxy                                          # ok
golangci-lint run --config .github/.golangci.yml ./internal/upstream/core/... ./internal/shellwrap/...  # 0 issues (v2)

Docs: docs/docker-isolation.md troubleshooting section updated to describe direct-exec (ENG-9).

Generated with mcpproxy cockpit — engineer agent. Human merges at the pre-merge gate.

[cloudflare-workers-and-pages]

Deploying mcpproxy-docs with    Cloudflare Pages

Latest commit:	e7ac964
Status:	✅  Deploy successful!
Preview URL:	https://2321a134.mcpproxy-docs.pages.dev
Branch Preview URL:	https://fix-mcp-2744-docker-direct-e.mcpproxy-docs.pages.dev

View logs

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/shellwrap/shellwrap.go	90.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[github-actions]

📦 Build Artifacts

Workflow Run: View Run
Branch: fix/mcp-2744-docker-direct-exec

Available Artifacts

• archive-darwin-amd64 (28 MB)
• archive-darwin-arm64 (25 MB)
• archive-linux-amd64 (16 MB)
• archive-linux-arm64 (14 MB)
• archive-windows-amd64 (28 MB)
• archive-windows-arm64 (25 MB)
• frontend-dist-pr (0 MB)
• installer-dmg-darwin-amd64 (21 MB)
• installer-dmg-darwin-arm64 (19 MB)

How to Download

Option 1: GitHub Web UI (easiest)

1. Go to the workflow run page linked above
2. Scroll to the bottom "Artifacts" section
3. Click on the artifact you want to download

Option 2: GitHub CLI

gh run download 27673404798 --repo smart-mcp-proxy/mcpproxy-go

Note: Artifacts expire in 14 days.

[Dumbris]

🤖 Codex review (model gpt-5.5, grounded in fix/mcp-2744-docker-direct-exec vs origin/main)

Verdict: REQUEST_CHANGES — the binary-resolution fix is correct, but the direct-exec path introduces a daemon-connectivity regression for shell-derived Docker environments (P2).

[P2] Preserve shell-derived Docker environment for direct exec — internal/upstream/core/connection_docker.go:89

When Docker daemon access depends on variables initialized by the user's login shell — e.g. DOCKER_HOST / DOCKER_CONTEXT for Colima / rootless Docker — the new direct-exec path skips wrapWithUserShell and runs docker with only BuildSecureEnvironment(), whose default allowlist does not include Docker-specific variables. Previously the isolated launch sourced the login shell before docker run, so these setups could start containers; now binary resolution succeeds but docker run cannot connect to the daemon — a silent regression for Colima/rootless users.

Fix options: add the Docker env vars (DOCKER_HOST, DOCKER_CONTEXT, DOCKER_CONFIG, DOCKER_TLS_VERIFY, DOCKER_CERT_PATH) to the secure-env allowlist for the docker-spawn path, capture them from the login shell once at resolve time, or fall back to shell-wrapping when any of these are set. Add a regression test for the Colima case (DOCKER_HOST set, must survive to the spawn env).

Reviewed via codex exec review --base origin/main in a checkout of the PR branch.

[Dumbris]

Addressed Codex P2 — Docker daemon env preserved on direct-exec path (b54d0b0)

Good catch — confirmed and fixed.

Root cause: the direct-exec spawn (connection_docker.go:88) replaced the $SHELL -l -c login-shell wrap, so the child now inherits only BuildSecureEnvironment(). The allowlist in secureenv.DefaultEnvConfig excluded the Docker daemon-access vars, so for Colima/rootless/remote daemons the binary resolved but docker run could not reach the daemon.

Fix:

• Added secureenv.DockerDaemonEnvVars() = {DOCKER_HOST, DOCKER_CONTEXT, DOCKER_CONFIG, DOCKER_TLS_VERIFY, DOCKER_CERT_PATH} and included it in DefaultEnvConfig's allowlist (cross-platform). These are connection pointers, not secrets.
• client.go also threads them through for command/stdio servers even when an operator supplied a custom Environment allowlist that omits them (the slice is rebuilt, not appended in place, so the shared source config is never mutated).

Tests (TDD, all offline):

• secureenv.TestBuildSecureEnvironmentPreservesDockerHost — the requested Colima regression: DOCKER_HOST/DOCKER_CONTEXT set in env must survive into the spawn env.
• secureenv.TestDefaultEnvConfigAllowsDockerDaemonVars — allowlist guard for all five vars.
• core.TestDockerIsolationSpawnEnvPreservesDockerHost — end-to-end via NewClient with a restrictive custom allowlist; DOCKER_HOST still threads through.

Verified: go build ./cmd/mcpproxy; go test ./internal/secureenv/... ./internal/upstream/core/... ./internal/shellwrap/... -race green; golangci-lint v2.5.0 (CI config) 0 issues.

[Dumbris]

🤖 Codex re-review (round 2) — still REQUEST_CHANGES (1×P2)

The env-var preservation fix doesn't actually cover the documented Colima/rootless case.

[P2] Allowlist only preserves inherited vars — capture from the login shell — internal/upstream/core/client.go:194-197

Adding DOCKER_HOST/DOCKER_CONTEXT to AllowedSystemVars only keeps values already present in os.Environ(). When mcpproxy is launched from the tray/LaunchAgent and a Colima/rootless setup exports DOCKER_HOST/DOCKER_CONTEXT only from login-shell startup files, those keys are absent from os.Environ(), so the allowlist is a no-op and the direct-exec path still hits the default socket → fails for exactly those users.

Fix: capture these specific Docker vars from the login shell (reuse the same login-shell resolution mechanism that already discovers PATH/docker), or fall back to shell-wrapping the docker spawn when DOCKER_HOST/DOCKER_CONTEXT are unset in os.Environ(). Add a regression test simulating "var present only via login shell, absent from os.Environ()".

Reviewed via codex exec review --base origin/main on b54d0b07.

[Dumbris]

Round-2 (P2) addressed — capture DOCKER_* from the login shell (c9eac4fc)

You're right: round-1 only allowlisted the vars, which preserves what's in os.Environ(). Under tray/LaunchAgent those vars live only in rc files, so the allowlist was a no-op and direct-exec still hit the default socket.

Fix: capture the daemon-access vars from the login shell, mirroring the existing LoginShellPATH machinery rather than re-introducing the command shell-wrap.

• shellwrap.LoginShellEnvVars(logger, keys) — one <shell> -l -c run prints the requested vars bracketed by unique markers, separated by 0x1F (banner-safe, same approach as LoginShellPATH), parsed and cached per-key process-wide. Non-empty only; no-op on Windows/failure.
• secureenv.BuildSecureEnvironment (gated on EnhancePath) injects any docker daemon var still missing from the env via the login shell. Only forks when something is actually missing; cached. Vars already present (allowlist + os.Environ) are untouched — so the round-1 allowlist append still covers the os.Environ-present + custom-allowlist case. Complementary, not redundant.

Regression test you asked for (var only in login shell, absent from os.Environ()):

• TestLoginShellEnvVars_CapturesFromShellOnly — fake login shell exports DOCKER_HOST/DOCKER_CONTEXT internally (never in the parent env); asserts they're captured and an unset var is omitted.
• TestBuildSecureEnvironment_InjectsDockerVarFromLoginShell — restrictive allowlist {PATH,HOME} guarantees os.Environ() cannot supply DOCKER_HOST, so the pass proves the login-shell path did it.
• TestBuildSecureEnvironment_NoDockerLoginShellWhenEnhanceDisabled — gating guard.

All offline. go test ./internal/shellwrap/... ./internal/secureenv/... ./internal/upstream/core/... -race green; golangci-lint v2 (CI config) 0 issues; go build ./cmd/mcpproxy ok.

[Dumbris]

🤖 Codex re-review (round 3) — still REQUEST_CHANGES (2×P2)

The login-shell DOCKER_* capture works, but it introduced two new issues — one robustness, one isolation/security.

[P2] Verify the resolved path before direct-exec — internal/upstream/core/connection_docker.go:89

ResolveDockerPath's last-resort lookup is command -v docker in the login shell, which can resolve to a shell function/alias or a bare name rather than an executable file. Direct-exec.Command on that fails before the isolated server starts. Only direct-exec when the resolved value is a verified absolute path to an executable (filepath.IsAbs + stat + executable bit); otherwise keep the shell-wrapped fallback.

[P2] Scope DOCKER_* passthrough to Docker launches only — internal/upstream/core/client.go:194-197

This block runs for every configured command, so a non-Docker upstream whose Environment.allowed_system_vars intentionally excludes DOCKER_* still receives them from the parent env — weakening configured env filtering and exposing Docker context/config paths to arbitrary MCP subprocesses. Only capture/append these vars when the server actually uses docker run / Docker isolation.

Reviewed via codex exec review --base origin/main on c9eac4fc.

Note: the recurring "fix one var, break another coupling" pattern across rounds 1–3 is being addressed at the root with a separate systematic launch-context env-hydration design (in progress). Recommend this PR be reduced to the safe core (verified-absolute direct-exec + shell fallback, docker-scoped env) and leave broad env hydration to that follow-up.

[Dumbris]

Revised to the minimal #696 fix per board decision (e7ac964d, force-pushed)

Per Algis's decision, dropped the direct-exec migration and all DOCKER_* env experiments (which caused the 3 rounds of Codex P2s). PR #699 is now just the negative-cache fix + test.

Net diff (vs origin/main): 2 files, +65/-8

• internal/shellwrap/shellwrap.go — ResolveDockerPath re-runs the pure-os.Stat well-known-path probe before honoring a still-live cached negative; on success it upgrades the cache to a permanent success.
• internal/shellwrap/shellwrap_test.go — TestResolveDockerPath_StatProbeOverridesLiveNegative: a successful os.Stat probe wins over a still-live cached negative (no TTL expiry).

Reverted (deferred to a new systematic task):

• setupDockerIsolation direct-exec — kept the post-fix(docker): resolve docker by absolute path on macOS spawn; honest docker_available (#696) #697 $SHELL -l -c wrap, which already runs the resolved absolute docker path as the command token (login-shell PATH irrelevant) and preserves login env (incl DOCKER_*) for free.
• the client.go global DOCKER_* capture and secureenv/LoginShellEnvVars work.

Why this fixes #696 with no regression: the bug was the negative-cache poisoning making spawn (early, sandboxed) and status (later) diverge. With the cache fixed, the existing shell wrap runs /Applications/Docker.app/.../docker run … as an absolute token → no command not found, no Colima env regression, no env hack.

Verification:

• go build ./cmd/mcpproxy ✓
• go test ./internal/shellwrap/... ./internal/upstream/core/... ./internal/secureenv/... -race ✓
• golangci-lint v2.5.0 (CI config) → 0 issues ✓

Follow-up systematic task (direct-exec migration + login-shell env hydration) being filed separately. Re-requesting Codex review on this minimal diff.

[Dumbris]

🤖 Codex review — ACCEPT ✅

Re-reviewed after re-scoping to the minimal #696 fix (negative-cache stat-probe override; direct-exec + DOCKER_* experiments reverted).

The change is narrowly scoped and preserves existing cache behavior while allowing well-known Docker paths to override a live negative cache. The added regression test passes.

No P0–P2 findings. Fixes the "status shows docker_path but spawn used bare docker" symptom at the source (the 60s negative-cache no longer shadows a stat-resolvable absolute path), with the $SHELL -l -c wrap retained so the login env (incl. DOCKER_*) is preserved — no Colima regression. Broader login-shell env hydration tracked separately.

Reviewed via codex exec review --base origin/main on e7ac964d.

[mcpproxy-gatekeeper]

Approved via Claude Code review (Codex quota exhausted).