fix(upstream): prepend docker bundle dir to child PATH so docker finds its credential helper (#715)#716
Conversation
… helper Docker-isolated (and user-supplied `docker run`) upstreams direct-exec the absolute docker binary (#696), but the spawned docker inherits mcpproxy's sanitized PATH, which omits the Docker Desktop bundle dir. So docker cannot exec the sibling binaries it shells out to — most importantly the credential helper from ~/.docker/config.json (`credsStore: desktop` → docker-credential-desktop). Any docker-isolated server whose isolation image isn't already cached fails to pull with: error getting credentials - err: exec: "docker-credential-desktop": executable file not found in $PATH resolveDockerSpawn / setupDockerIsolation now also return the resolved docker binary's bundle directory, and both stdio spawn paths plus the launcher prepend it to the child PATH via prependDockerDirToPath (dedup-aware, os.PathListSeparator, no-op when docker did not resolve to an absolute path). This generalizes the #696 fix from "find docker" to "let docker find its own tooling". Related #715
Deploying mcpproxy-docs with
|
| Latest commit: |
1e6d619
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e7cef8cb.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-715-docker-bundle-dir-pa.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 27754880787 --repo smart-mcp-proxy/mcpproxy-go
|
![mcpproxy-gatekeeper[bot]](https://avatars.githubusercontent.com/u/216428285?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved reflecting an independent Claude Code review on a checkout of the PR branch (HEAD 1e6d619). Verified: prependDockerDirToPath prepends the resolved docker bundle dir to the spawn env PATH on all covered paths (isolation uvx/npx + user-supplied docker run in stdio + launcher isolation), is a correct no-op when docker is not an absolute path, dedups, does not mutate input, and uses os.PathListSeparator. Targeted tests pass under -race; CI fully green. Two non-blocking findings filed as follow-up (launcher user-supplied 'docker run' else-branch is pre-existing tech debt, not a regression; Windows PATH casing matches existing secureenv convention).
2 participants
Summary
Follow-on to #696 (shipped v0.41.1). Fixes GitHub issue #715 / MCP-2877.
We now invoke the absolute docker path for docker-isolated servers, but the spawned
dockerprocess inherits mcpproxy's sanitized PATH, which omits the Docker Desktop bundle dir. So docker cannot exec the sibling binaries it shells out to — most importantly the credential helper from~/.docker/config.json. Any docker-isolated server whose isolation image isn't already cached fails to pull:Docker Desktop's default
credsStore: desktopmakes docker rundocker-credential-desktopfor every registry op (even anonymous public pulls). That helper lives in the same bundle dir as the docker CLI (/Applications/Docker.app/Contents/Resources/bin/), which mcpproxy's spawn PATH omits. A pre-pulled image sidesteps it (no registry op), which is why #696 looked complete.Fix
resolveDockerSpawn/setupDockerIsolationnow also return the resolved docker binary's bundle directory (filepath.Dir(resolved)), set whenever docker resolves to a verified absolute executable — on both the direct-exec and login-shell-wrap fallback paths.prependDockerDirToPath(env, dir): prepends the bundle dir to thePATHentry (existing entries preserved), dedup-aware (no-op if already present), usesos.PathListSeparator(cross-platform), never mutates the input, and is a no-op for an empty dir.connectStdio+ launcher) and user-supplieddocker run …upstreams.This generalizes the #696 fix from "find docker" to "let docker find its own tooling" (
docker-credential-*,docker-compose,docker-buildx).Tests (TDD — failing test written first)
connection_docker_path_test.go: asserts the bundle dir is reported on direct-exec and on the shell-wrap fallback, not reported when docker is unresolved, and full unit coverage ofprependDockerDirToPath(prepend/dedup/empty/no-PATH/empty-value/no-mutation).go test ./internal/upstream/... -race✅golangci-lint --config .github/.golangci.yml ./internal/upstream/...) → 0 issues ✅./scripts/test-api-e2e.sh→ 65/65 passed ✅Docs
docs/docker-isolation.md: new troubleshooting entry for thedocker-credential-desktop … not found in $PATHsymptom.Lineage
#696 → MCP-2715 (find docker) → MCP-2744 (wire resolved path into spawn) → #714/MCP-2868 (shared
resolveDockerSpawn) → this (let docker find its sibling tooling).Related #715