Skip to content

fix(docker): recognise docker.exe and backslash paths as a direct docker run - #1777

Merged
Dumbris merged 6 commits into
smart-mcp-proxy:mainfrom
rudycelekli:fix/direct-docker-exe-detect-20261024-01
Oct 30, 2026
Merged

Dumbris merged 6 commits into
smart-mcp-proxy:mainfrom
rudycelekli:fix/direct-docker-exe-detect-20261024-01

Conversation

@rudycelekli

Copy link
Copy Markdown
Contributor

Problem

connectStdio and the launcher path decide whether an upstream is a user-supplied docker run with:

(c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun

On Windows the command is often docker.exe or a backslash path such as C:\\Program Files\\Docker\\Docker\\resources\\bin\\docker.exe. Neither matches, so the server goes through the plain shell-wrap branch:

  • config.Env isn't injected as -e flags, so the container never sees those variables (API keys etc.). They only reach the docker CLI process.
  • No --cidfile is added, and isDockerCommand is false, so the container isn't tracked for cleanup on stop or restart.
  • The per-server container lock and ensureNoExistingContainers are skipped.

Fix

Move the check into isDockerRunCommand(command, args). It takes the base name after the last / or \\, compares it case-insensitively, and accepts docker or docker.exe. All four call sites (stdio and launcher, lock and branch) now use it. Behaviour for docker and /path/to/docker is unchanged.

Test

TestIsDockerRunCommand covers bare, POSIX path, docker.exe, upper-case, and both Windows paths, plus negatives (docker ps, no args, docker-compose, dockerize). With the previous predicate the four Windows cases fail; with this change all pass. go test -count=1 ./internal/upstream/core/ passes.

Made with Cursor

…ker run

The user-supplied docker run check matched only "docker" or a path
ending in "/docker". A Windows config naming docker.exe, or the CLI by
its backslash path, fell through to the plain shell wrap: config env
was never injected as -e flags (the container did not see it), no
cidfile was written, and the server was not treated as docker for
container cleanup. Match the base name in either separator style,
case-insensitively, with or without .exe.
@rudycelekli
rudycelekli force-pushed the fix/direct-docker-exe-detect-20261024-01 branch from 9c909a3 to 28cc369 Compare October 24, 2026 22:18
@codecov-commenter

codecov-commenter commented Oct 24, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 78.78788% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/upstream/core/connection_docker.go 75.86% 5 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

Dumbris and others added 5 commits October 29, 2026 11:40
… docker run

Review follow-up: recognising docker.exe and backslash paths routes such
commands through resolveDockerSpawn, which resolved docker on its own and
dropped an explicit C:\custom\docker.exe (or /opt/x/bin/docker),
failing when that docker was not on PATH or running a different one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…_dir

Sol review round 2 of smart-mcp-proxy#1777: ./bin/docker or .\bin\docker.exe still fell
through to the resolver. A path with a separator is now evaluated like exec
does (against working_dir, else the process directory); a bare name stays a
PATH lookup.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gured docker

Sol review round 3 of smart-mcp-proxy#1777: ./server + ./bin/docker joined to a relative
path that isDirectExecutable rejects, so the resolver's docker won again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oss Windows drives

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Dumbris
Dumbris merged commit 09cca5f into smart-mcp-proxy:main Oct 30, 2026
47 checks passed
@Dumbris

Dumbris commented Oct 30, 2026

Copy link
Copy Markdown
Member

Merged. Thank you, @rudycelekli! A direct docker run server now spawns the docker binary it was configured with instead of whichever one the resolver finds first, so containers start reliably on hosts with more than one docker install. I pushed two small follow-ups to your branch: a configured relative docker path (and a relative working_dir) now resolves the same way, and the test works when the temp dir and checkout sit on different Windows drives.

Contributions like this are what keep MCPProxy moving, and they matter a lot to the project.

One small ask, only if you feel like it: a short public note about your change (a tweet, a LinkedIn post, a line in a community you are part of) helps more people discover MCPProxy. No pressure at all, and I would be glad to repost it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants