Repository navigation
Conversation
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
On Windows without DOCKER_HOST the docker spawn keeps the shell wrap, and the command string starts with the resolved docker.exe path. The literal "docker run" search missed it, so --cidfile was appended as extra shell argv where docker never saw it and the container could not be tracked for cleanup. Match the first token as docker or docker.exe (bare, path, or quoted) and insert --cidfile right after the run verb.
e3518a8 to
7cb664f
Compare
Review finding on smart-mcp-proxy#1773: the cidfile path was spliced unquoted into the string cmd.exe /c or bash -c re-parses. A %TEMP% with a space split the path under cmd.exe and broke container start; under Git Bash the Windows path lost its backslashes so the container ID was never tracked. Quote it per shell dialect, keyed on the /c vs -c flag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dumbris
left a comment
Dumbris
left a comment
There was a problem hiding this comment.
Verified: the bug reproduces on main and the PR fixes it. The review found one gap, closed by the maintainer commit on top (regression test included). Race tests and CI lint for the touched packages are clean on this branch merged with main, and the result was cross-reviewed clean.
# Conflicts: # internal/upstream/core/connection_docker.go
|
Merged. Thank you, @rudycelekli! A shell-wrapped 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. |
Problem
When the docker spawn keeps the login-shell wrap (non-Darwin without
DOCKER_HOST/DOCKER_CONTEXTin the process env, which is the usual Windows case),insertCidfileIntoShellDockerCommandsearches the command string for the literaldocker run. On Windows the string begins with the resolveddocker.exepath, often quoted ("C:\Program Files\Docker\resources\bin\docker.exe" run ...), so the search misses. The fallback then appends--cidfile <file>as extra shell argv. cmd.exe never passes that to docker, so no cidfile is written and the container can't be tracked for cleanup.Fix
Read the first shell word (bare, path, or quoted), accept it when its base name is
dockerordocker.exe(case-insensitive), and insert--cidfileright after a followingrunverb. Commands that only mentiondocker runlater in the string are no longer rewritten.Test
TestInsertCidfile_DockerExecovers baredocker.exe, a quoteddocker.exepath, plaindocker, and a non-docker command. It fails on main and passes with this change.go test -count=1 ./internal/upstream/core/passes.Made with Cursor