fix(upstream): direct-exec resolved docker for user-supplied docker run (#696/MCP-2868)#714
Merged
Merged
Conversation
…ocker run 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
Deploying mcpproxy-docs with
|
| Latest commit: |
b047e0a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3e36a8aa.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-mcp2868-docker-direct-ex.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 27744840131 --repo smart-mcp-proxy/mcpproxy-go
|
![mcpproxy-gatekeeper[bot]](https://avatars.githubusercontent.com/u/216428285?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the remaining GH #696 failure, confirmed still broken on v0.41.0. Upstream servers whose config
commandISdocker(a user-supplieddocker run … <image>) resolved the bundled Docker Desktop CLI for the status field but the spawn still shell-wrapped baredockervia$SHELL -l -c "docker run …". On a default Docker Desktop macOS install the docker CLI lives only at/Applications/Docker.app/Contents/Resources/bin/dockerand is not on the login-shell PATH →zsh:1: command not found: docker.PR #703's direct-exec fix only covered the isolation-injection path (
setupDockerIsolation, used for uvx/npx wrapped intodocker run). The user-supplieddocker runbranch inconnectStdiowas never covered.Change
dockerto an absolute path, gate direct-exec on a verified absolute executable + guaranteed daemon env, else login-shell-wrap the resolved path / baredocker) out ofsetupDockerIsolationinto a shared method(*Client).resolveDockerSpawn(finalArgs).setupDockerIsolationnow delegates to it — behavior unchanged.connection_stdio.go, the user-supplieddocker runbranch now callsresolveDockerSpawn(argsToWrap)instead ofwrapWithUserShell("docker", …), then inserts--cidfilevia the helper matching the spawn shape (insertCidfileIntoDockerArgson direct-exec,insertCidfileIntoShellDockerCommandon the shell-wrap fallback). Existing-eenv injection is preserved; new debug log lines reuse Debug logs leak upstream secrets in cleartext (inconsistent argv redaction) #712'sshellwrap.RedactDockerArgs.docs/docker-isolation.md) updated: direct-exec now covers both Docker spawn paths.Tests (TDD — failing test written first)
TestResolveDockerSpawnUserSuppliedDockerRunDirectExec— userdocker run, resolved absolute executable +dockerDaemonEnvGOOS=darwin→finalCommand== resolved absolute path,shellWrapped == false,-e KEY=VALUEenv flags present, args-based cidfile inserted right afterrun.TestResolveDockerSpawnUserSuppliedFallsBackToShellWrap— resolution fails → shell-wrapped baredocker, env value preserved in the command string.Verification
go test -race ./internal/upstream/core/...✅golangci-lint run --config .github/.golangci.yml ./internal/upstream/core/...→ 0 issues ✅go build ./...✅Rebased onto current main (includes #712 / eba4a85).
Related #696
Related #712