fix(docker): direct-exec resolved docker binary for isolated spawn (MCP-2753)#703
Conversation
…CP-2753) PR #697 resolved the docker CLI to an absolute path but still routed the isolated spawn through `$SHELL -l -c "<docker> run …"`, where the login shell re-derives $PATH from rc files and can drop the Docker Desktop bundle dir. The absolute path was only a token in the -c string, so docker-isolated servers still failed with `command not found: docker` when the CLI was not on the LaunchAgent/tray spawn PATH — even though `upstream_servers list` reported a good docker_path. On successful resolution to a VERIFIED absolute executable, setupDockerIsolation now returns the resolved binary as the exec command with raw docker argv (no shell wrap), mirroring the management path's newDockerCmd. It returns a shellWrapped flag so callers pick the matching cidfile helper: the new args-based insertCidfileIntoDockerArgs inserts `--cidfile` after the `run` token (platform-agnostic, sidestepping the -c vs /c quirk), while the login-shell fallback keeps the string-based insertCidfileIntoShellDockerCommand. The direct-exec branch is gated by isDirectExecutable (filepath.IsAbs + os.Stat + exec-bit). ResolveDockerPath's last resort runs `command -v docker` in the login shell, which can emit a shell function/alias/bare name; such a value is rejected and shell-wrapped instead of failing a direct exec. Login-shell wrap of bare `docker` is also kept on the resolution-failure fallback. The DOCKER_* daemon-env half of MCP-2753 is already satisfied by the startup login-shell hydration shipped in MCP-2751 (#701): DOCKER_* are hydrated into os.Environ() and carried through BuildSecureEnvironment's default allow-list, so the direct-exec'd docker process still reaches Colima/rootless/remote daemons without a per-spawn shell capture. Related #699 Related #696
Deploying mcpproxy-docs with
|
| Latest commit: |
04dbe4b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://45a3f27c.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-mcp-2753-docker-direct-e.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 27686130044 --repo smart-mcp-proxy/mcpproxy-go
|
🤖 Codex review — REQUEST_CHANGES (1×P2)[P2] Keep login-shell Docker vars before direct-exec on non-Darwin —
|
…otless regression) Codex round-4 (PR #703 REQUEST_CHANGES, P2): the direct-exec branch regressed non-Darwin rootless/remote Docker. Dropping the `$SHELL -l` wrap also drops rc-file env inheritance, and HydrateFromLoginShell (MCP-2751) is Darwin-only while BuildSecureEnvironment only forwards DOCKER_* already in os.Environ(). So on Linux a daemon whose DOCKER_HOST/DOCKER_CONTEXT lives only in .profile would be lost by direct-exec — the same DOCKER_* loss that made #699 keep the shell wrap. Add a second precondition (dockerDaemonEnvGuaranteed) alongside the verified- absolute-executable check: direct-exec only when the daemon env is guaranteed without the login shell — macOS (startup hydration captured DOCKER_* into os.Environ()) OR DOCKER_HOST/DOCKER_CONTEXT already present in the process env on any platform. Otherwise keep the `$SHELL -l` wrap so `docker run` inherits the rc-file daemon config; the wrap now prefers the resolved absolute path (still sidestepping the login shell's PATH re-derivation) and drops to bare "docker" only when resolution failed. dockerDaemonEnvGOOS is a package var so the Darwin branch is testable on a non-Darwin CI host. New tests: non-Darwin + DOCKER_HOST-only-in-rc stays shell-wrapped with the absolute path; non-Darwin + DOCKER_HOST in env direct- execs; and a table test for the gate. Related #703 Related #699 Co-Authored-By: Paperclip <noreply@paperclip.ing>
Addressed Codex P2 — non-Darwin rootless daemon-env regression (
|
✅ Review (Claude Code, Codex rate-limited) — ACCEPTRigorous substitute review while Codex is rate-limited (per maintainer direction). Previously-flagged P2 (non-Darwin rootless regression) — CONFIRMED RESOLVED. Direct-exec is gated in Other points ✓ — Tests ✓ — new guards VERDICT: ACCEPT |
![mcpproxy-gatekeeper[bot]](https://avatars.githubusercontent.com/u/216428285?s=60&v=4){kind=small}
…ocker run (#714) User-supplied `docker run …` upstreams (config command IS `docker`) resolved the bundled Docker Desktop CLI for the status field but still shell-wrapped bare `docker` via `$SHELL -l -c "docker run …"` at spawn. On a default Docker Desktop macOS install the CLI lives only inside the app bundle and is not on the login-shell PATH, so the spawn failed with `zsh:1: command not found: docker` (GH #696, still broken on v0.41.0). PR #703's direct-exec fix covered only the isolation-injection path (setupDockerIsolation). Extract its resolve→spawn-decision logic into a shared `resolveDockerSpawn` method and reuse it on the user-supplied `docker run` branch in connectStdio, inserting --cidfile via the helper that matches the spawn shape (args-based on direct-exec, string-based on the login-shell fallback) and keeping the existing -e env injection. setupDockerIsolation behavior is unchanged (now delegates to the shared method). Docs updated to note direct-exec now covers both Docker spawn paths. Related #696 Related #712
2 participants
Summary
Systematic follow-up deferred from #699 / MCP-2744 (board kept #699 minimal). Migrates the docker-isolation spawn from a
$SHELL -l -c "<docker> run …"login-shell wrap to direct exec of the resolved absolute docker binary, so the spawn bypassesPATHentirely.PR #697 resolved the docker CLI to an absolute path but still routed the spawn through the login shell, where
$PATHis re-derived from rc files and can drop the Docker Desktop bundle dir — so docker-isolated servers still hitcommand not found: dockerdespite a gooddocker_path. Exec'ing the absolute path directly (mirroring the management path'snewDockerCmd) fixes that.What changed
setupDockerIsolationnow returns(cmd, args, shellWrapped bool). On resolution to a verified absolute executable it returns the resolved binary + raw docker argv withshellWrapped=false.isDirectExecutableguard (Codex round-3 P2 Add goreleaser configuration and update dependencies #1):filepath.IsAbs+os.Stat+ exec-bit.ResolveDockerPath's last resort runscommand -v dockerin the login shell, which can emit a shell function/alias/bare name — such a value is rejected and shell-wrapped instead of failing a direct exec.insertCidfileIntoDockerArgs(inserts--cidfileafter theruntoken; platform-agnostic, sidesteps the-cvs/cshell quirk). Both call sites (connection_stdio.go,connection_launcher.go) branch onshellWrappedto pick the matching cidfile helper.dockerretained as the resolution-failure / non-executable fallback.docs/docker-isolation.mdupdated to describe direct-exec behavior.Scope #2 (DOCKER_* env hydration) — already shipped
The issue's second scope item (login-shell DOCKER_* hydration) is satisfied by the merged MCP-2751 / #701, which hydrates
DOCKER_*intoos.Environ()at startup and carries them throughBuildSecureEnvironment's default allow-list. So the direct-exec'd docker process still reaches Colima/rootless/remote daemons with no per-spawn shell capture. The reverted prior-art commitsb54d0b07(per-spawn allowlist) andc9eac4fc(per-spawn capture) are obsolete and were not recovered — #701 explicitly supersedes them.Verification
go test ./internal/upstream/core/... ./internal/shellwrap/... ./internal/secureenv/... -race→ all greengolangci-lint(CI v2 config) on the three packages → 0 issuesgo build ./cmd/mcpproxy,go vet,gofmtcleaninsertCidfileIntoDockerArgsunit table.Related #699
Related #696