fix(upstream): launcher user-supplied docker run misses bundle-dir PATH + direct-exec (MCP-2881)#717
Conversation
…eDockerSpawn The launcher path's user-supplied `docker run` branch (buildLauncherCmd else clause) still shell-wrapped bare `docker` and never prepended the docker bundle dir to the child PATH. Unlike the stdio path it never adopted resolveDockerSpawn/direct-exec, so a user-supplied `docker run …` upstream launched via the launcher (HTTP/SSE transport with Command set) did not get the #715 credential-helper fix and could fail with `docker-credential-desktop … not found in $PATH` on an uncached image pull. Route this branch through the same resolveDockerSpawn + prependDockerDirToPath wiring the stdio path already uses, so direct-exec and bundle-dir PATH augmentation apply uniformly across every docker spawn entrypoint. Pre-existing tech debt, not a regression from #716. Related #715
Deploying mcpproxy-docs with
|
| Latest commit: |
2328ae8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a0294813.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-mcp-2881-launcher-docker.mcpproxy-docs.pages.dev |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 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 27756242864 --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.
APPROVE — reviewed via Claude Code on the ACTUAL #717 branch (HEAD 2328ae8; a prior reviewer falsely called it a no-op by diffing the wrong branch). Routes buildLauncherCmd's user-supplied 'docker run' through shared resolveDockerSpawn + prependDockerDirToPath — faithful mirror of merged stdio fix; direct-exec/shell-wrap gating + cidfile helper selection + env injection all consistent across the 3 entrypoints; isolation path untouched. go build clean, internal/upstream tests pass (3 new launcher tests), golangci-lint v2 0 issues. MCP-2881.
2 participants
Summary
Follow-up to #715/#716 (the docker credential-helper PATH fix). An independent review of #716 found a pre-existing gap not addressed there.
The launcher path's user-supplied
docker runbranch inbuildLauncherCmd(internal/upstream/core/connection_launcher.go) still calledwrapWithUserShell(...)directly. Unlike the stdio path, it never adoptedresolveDockerSpawn/direct-exec, so it got no bundle-dir PATH prepend (prependDockerDirToPath) and no direct-exec of the resolved absolute docker binary.Consequence: a user-supplied
docker run …upstream launched via the launcher (HTTP/SSE transport withCommandalso set) did not receive the #715 credential-helper fix and could still fail withdocker-credential-desktop … not found in $PATHon an uncached image pull.This is pre-existing tech debt, not a regression from #716 (confirmed against
origin/main= the #716 merge). #716 correctly covered the launcher's isolation branch and all stdio paths.Fix
Route the launcher's user-supplied
docker runbranch through the sameresolveDockerSpawn+prependDockerDirToPath(envVars, dockerDir)wiring the stdio path already uses — a one-to-one mirror ofconnectStdio'selse if isDirectDockerRunbranch — so direct-exec + bundle-dir PATH augmentation apply uniformly across every docker spawn entrypoint (isolation-injection, stdio user-supplied, launcher user-supplied).Tests (TDD)
New
connection_launcher_test.go:TestBuildLauncherCmdUserSuppliedDockerRunDirectExec— failing-first: asserts the launcher path direct-execs the resolved absolute docker, carries the injected-e KEY=VALUEflags as raw argv, inserts--cidfilevia the args-based helper, and prepends the bundle dir to the spawn env PATH.TestBuildLauncherCmdUserSuppliedDockerRunShellWrapFallback— resolution-failure keeps the login-shell wrap (no regression of Docker Desktop not detected on macOS: servers fail withcommand not found: dockerdespitedocker_available: true#696's last-resort PATH lookup).TestBuildLauncherCmdPlainCommandStillShellWraps— non-docker launcher commands still shell-wrap and get no docker PATH augmentation / cidfile.Verification
go test -race ./internal/upstream/...— greengolangci-lint run --config .github/.golangci.yml ./internal/upstream/...— 0 issuesgofmtclean,go build ./cmd/mcpproxyOKRelated #715