fix(shellwrap): Git Bash on Windows cannot exec backslash-style paths

[LocoLoboZ]

Summary

• Fixes Windows: Git Bash cannot exec backslash-style Windows paths, breaks every wrapper-script stdio server #1318: on Windows, when the resolved login shell is Git Bash / MSYS, WrapWithUserShell correctly single-quoted an absolute Windows path but Git Bash still couldn't execute it — MSYS's exec layer only resolves POSIX-style paths, so a backslash path falls through to bash's PATH lookup and fails with command not found (mangling the path in its own error output).
• This broke every stdio server configured with an absolute Windows-path command (wrapper .cmd/.bat scripts are the common case) whenever $SHELL resolves to a bash-like shell on Windows — a common setup for anyone using Git Bash / MSYS2 / VS Code's integrated terminal as their default shell.
• Fix: convert backslashes to forward slashes in the command and args before shell-escaping, but only on the Windows + bash-like-shell branch. Both CreateProcess and MSYS's exec layer accept forward-slash paths.
• Updated TestSetupDockerIsolationShellWrapsWhenDaemonEnvMissingNonDarwin, which exercises the same shell-wrap code path for the Docker isolation fallback and was hitting the identical latent bug — its assertion now accepts either separator style.
• Added TestWrapWithUserShell_GitBashCanExecWindowsPath, a Windows-only regression test that actually execs a real binary (whoami.exe) through the wrapped Git Bash command, so a regression here fails loudly instead of only failing a string-quoting assertion.

Test plan

• go test ./internal/shellwrap/... — pass, including the new regression test
• go test ./internal/upstream/... — pass (all subpackages)
• go build ./... — clean
• gofmt -l on changed files — clean
• go vet ./internal/shellwrap/... ./internal/upstream/core/... — clean
• Manual end-to-end: rebuilt mcpproxy with this change and reconnected a real config with 14 wrapper-.cmd-based stdio servers that were all failing with this exact error before the fix. All connected afterward — connected server count went from 13 to 27, tool count from 249 to 403.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 61.53846% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/shellwrap/shellwrap.go	61.53%	1 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

[Dumbris]

Thanks for the detailed writeup and the manual verification with 14 wrapper-.cmd servers — that's a great regression to have caught and a clear repro. The fix direction (stop keying the quoting dialect on GOOS and instead ask "what shell will actually parse this") is the right one.

I did find a few things worth another pass before merging, mostly around how broadly the new backslash→forward-slash conversion applies:

1. toBashPath rewrites backslashes in every arg, not just paths (internal/shellwrap/shellwrap.go around the new closure). It's applied unconditionally to command and every element of args whenever GOOS=windows and the shell is bash-like, with no check that the string is actually a Windows path. That'll also mangle non-path values that happen to contain a literal \ — e.g. a SQL Server instance name like .\SQLEXPRESS, a DOMAIN\user credential, a regex (\d+\.log), or a Docker -e KEY=VALUE value injected via injectEnvVarsIntoDockerArgs — silently, with no error. Might be worth scoping the rewrite to things that look like Windows paths (e.g. ^[A-Za-z]:\\ or UNC \\) rather than any string containing \.

2. The new regression test may not exercise the fix it's meant to guard. TestWrapWithUserShell_GitBashCanExecWindowsPath never sets $SHELL before calling WrapWithUserShell, and resolveLoginShell() reads $SHELL first — so on a Windows box/runner where $SHELL isn't set, it silently falls through to the cmd.exe branch instead of the Git Bash one, and the test would pass without ever touching the code path it's testing. On top of that, .github/workflows/unit-tests.yml only runs windows-latest on push, not on pull_request — so as written this test can't gate a PR either way. Could you add t.Setenv("SHELL", bashPath) and see if the Windows job can run on PRs (or is skipped intentionally for cost reasons)?

3. Smaller / lower-confidence, worth a look:

   • isBashLikeShell's strings.Contains(lower, "sh") also matches pwsh.exe, fish, csh/tcsh. It previously only picked argv flags (-l -c vs /c); now it also gates the quoting dialect and the backslash rewrite, so a false-positive there (e.g. $SHELL=pwsh.exe) would now silently mis-quote/mis-convert rather than just fail loudly.
   • insertCidfileIntoShellDockerCommand's literal "docker run" substring match (internal/upstream/core/connection_docker.go) breaks once the docker path is quoted — which happens for the very common Docker Desktop default install path C:\Program Files\Docker\.... Pre-existing, but directly hit by this PR's target scenario, so flagging it here.
   • The public Shellescape() still keys its dialect on bare runtime.GOOS, unaware of isBash — its doc comment ("mirrors... so both code paths can converge") is now a bit stale, and a future caller of it could reintroduce this exact bug.

None of this is a knock on the diagnosis or the manual verification — the root cause and the general direction look right to me. Requesting changes mainly for #1 (the unscoped rewrite) and #2 (test coverage), since those affect correctness/confidence in the fix itself. Happy to take another look once addressed!