From 28cc369f56b7442fac02922c9c13a6d07ccde3c8 Mon Sep 17 00:00:00 2001 From: Rudy Celekli Date: Sat, 24 Oct 2026 18:11:50 -0400 Subject: [PATCH 1/5] fix(docker): recognise docker.exe and backslash paths as a direct docker 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. --- internal/upstream/core/connection_docker.go | 15 +++++++++ internal/upstream/core/connection_launcher.go | 4 +-- internal/upstream/core/connection_stdio.go | 4 +-- .../upstream/core/docker_run_command_test.go | 33 +++++++++++++++++++ 4 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 internal/upstream/core/docker_run_command_test.go diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index 1630bd855..0679d405e 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -393,6 +393,21 @@ func (c *Client) insertCidfileIntoShellDockerCommand(shellArgs []string, cidFile return append(shellArgs, "--cidfile", cidFile) } +// isDockerRunCommand reports whether a configured command is a user-supplied +// `docker run`. The command may be bare or a path in either separator style, +// and may carry the Windows .exe suffix (case-insensitive, as Windows lookup is). +func isDockerRunCommand(command string, args []string) bool { + if len(args) == 0 || args[0] != cmdRun { + return false + } + base := command + if i := strings.LastIndexAny(base, `/\`); i >= 0 { + base = base[i+1:] + } + base = strings.ToLower(base) + return base == cmdDocker || base == cmdDocker+".exe" +} + // extractContainerNameFromArgs extracts the container name from Docker run arguments func (c *Client) extractContainerNameFromArgs(dockerArgs []string) string { // Look for --name flag in the arguments diff --git a/internal/upstream/core/connection_launcher.go b/internal/upstream/core/connection_launcher.go index 53e6afc78..88e13fb32 100644 --- a/internal/upstream/core/connection_launcher.go +++ b/internal/upstream/core/connection_launcher.go @@ -58,7 +58,7 @@ func (c *Client) connectWithLauncher(ctx context.Context) error { // (matches connectStdio's behaviour at internal/upstream/core/ // connection_stdio.go:97). Otherwise concurrent reconnects could // race to create overlapping containers for the same upstream. - willUseDocker := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(c.config.Args) > 0 && c.config.Args[0] == cmdRun + willUseDocker := isDockerRunCommand(c.config.Command, c.config.Args) if !willUseDocker && c.isolationManager != nil { willUseDocker = c.isolationManager.ShouldIsolate(c.config) } @@ -292,7 +292,7 @@ func (c *Client) buildLauncherCmd(_ context.Context, willUseDocker bool) (*exec. finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile) } } - } else if isDirectDockerRun := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun; isDirectDockerRun { + } else if isDirectDockerRun := isDockerRunCommand(c.config.Command, args); isDirectDockerRun { // USER-SUPPLIED `docker run …` upstream launched via the launcher path // (config.Command IS `docker`). Reuse the SAME resolve→spawn decision as the // isolation path and the stdio path (resolveDockerSpawn) so this entrypoint diff --git a/internal/upstream/core/connection_stdio.go b/internal/upstream/core/connection_stdio.go index e2642a193..91b895915 100644 --- a/internal/upstream/core/connection_stdio.go +++ b/internal/upstream/core/connection_stdio.go @@ -97,7 +97,7 @@ func (c *Client) connectStdio(ctx context.Context) error { var cidFile string // Check if this will be a Docker command (either explicit or through isolation) - willUseDocker := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun + willUseDocker := isDockerRunCommand(c.config.Command, args) if !willUseDocker && c.isolationManager != nil { willUseDocker = c.isolationManager.ShouldIsolate(c.config) } @@ -172,7 +172,7 @@ func (c *Client) connectStdio(ctx context.Context) error { finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile) } } - } else if isDirectDockerRun := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun; isDirectDockerRun { + } else if isDirectDockerRun := isDockerRunCommand(c.config.Command, args); isDirectDockerRun { // USER-SUPPLIED `docker run …` upstream (config.Command IS `docker`). // Inject env vars as -e flags, then reuse the SAME resolve→spawn decision // as the isolation path (resolveDockerSpawn) so this path also direct-execs diff --git a/internal/upstream/core/docker_run_command_test.go b/internal/upstream/core/docker_run_command_test.go new file mode 100644 index 000000000..2ebdae8cc --- /dev/null +++ b/internal/upstream/core/docker_run_command_test.go @@ -0,0 +1,33 @@ +package core + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// A Windows config names the CLI docker.exe, often by its full backslash path. +// Missing it routes the server through the plain shell wrap, so config env is +// never injected as -e flags and the container is not tracked for cleanup. +func TestIsDockerRunCommand(t *testing.T) { + run := []string{"run", "--rm", "-i", "ghcr.io/some/image"} + cases := []struct { + command string + args []string + want bool + }{ + {"docker", run, true}, + {"/usr/local/bin/docker", run, true}, + {"docker.exe", run, true}, + {"DOCKER.EXE", run, true}, + {`C:\Program Files\Docker\Docker\resources\bin\docker.exe`, run, true}, + {`C:\Program Files\Docker\Docker\resources\bin\docker`, run, true}, + {"docker", []string{"ps"}, false}, + {"docker", nil, false}, + {"/usr/bin/docker-compose", run, false}, + {"dockerize", run, false}, + } + for _, tc := range cases { + assert.Equal(t, tc.want, isDockerRunCommand(tc.command, tc.args), "%q %v", tc.command, tc.args) + } +} From 50aef17be44fe0c55222a01164fc60d7333d5f4a Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 29 Oct 2026 11:40:29 +0200 Subject: [PATCH 2/5] fix(docker): spawn the configured absolute docker binary for a direct 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 --- internal/upstream/core/connection_docker.go | 16 ++++++- .../upstream/core/docker_run_command_test.go | 45 +++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index 0679d405e..f12bee9b1 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -157,7 +157,10 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm // as the wrapped command (it sidesteps the login shell's PATH re-derivation), // dropping to bare "docker" only when resolution failed. func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, dockerArgs []string, shellWrapped bool, dockerDir string) { - resolved, resErr := resolveDockerBinary(c.logger) + resolved, resErr := c.configuredDockerBinary() + if resolved == "" { + resolved, resErr = resolveDockerBinary(c.logger) + } switch { case resErr == nil && isDirectExecutable(resolved) && dockerDaemonEnvGuaranteed(): // INFO (not Debug): make the spawn decision observable in main.log so a @@ -393,6 +396,17 @@ func (c *Client) insertCidfileIntoShellDockerCommand(shellArgs []string, cidFile return append(shellArgs, "--cidfile", cidFile) } +// configuredDockerBinary returns the server's own command when it is a +// user-supplied `docker run` naming the docker CLI by an absolute path to an +// executable, so that binary is spawned instead of whichever docker the +// resolver finds. "" means resolve as usual. +func (c *Client) configuredDockerBinary() (string, error) { + if c.config == nil || !isDockerRunCommand(c.config.Command, c.config.Args) || !isDirectExecutable(c.config.Command) { + return "", nil + } + return c.config.Command, nil +} + // isDockerRunCommand reports whether a configured command is a user-supplied // `docker run`. The command may be bare or a path in either separator style, // and may carry the Windows .exe suffix (case-insensitive, as Windows lookup is). diff --git a/internal/upstream/core/docker_run_command_test.go b/internal/upstream/core/docker_run_command_test.go index 2ebdae8cc..05f13eabf 100644 --- a/internal/upstream/core/docker_run_command_test.go +++ b/internal/upstream/core/docker_run_command_test.go @@ -1,9 +1,13 @@ package core import ( + "fmt" + "path/filepath" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" ) // A Windows config names the CLI docker.exe, often by its full backslash path. @@ -31,3 +35,44 @@ func TestIsDockerRunCommand(t *testing.T) { assert.Equal(t, tc.want, isDockerRunCommand(tc.command, tc.args), "%q %v", tc.command, tc.args) } } + +// A user-supplied `docker run` whose command is an absolute path to the docker +// CLI must spawn that binary, not whichever docker the resolver finds. Before +// this, C:\custom\docker.exe (or /opt/x/bin/docker) was replaced by the +// resolved or bare `docker`, failing when that one was absent from PATH. +func TestResolveDockerSpawnHonorsConfiguredAbsoluteDocker(t *testing.T) { + configured := writeFakeDockerExecutable(t) + other := writeFakeDockerExecutable(t) + + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + + for _, tc := range []struct { + name string + goos string + resolve func(*zap.Logger) (string, error) + wantWrap bool + }{ + {"direct exec, resolver finds another docker", osDarwin, func(*zap.Logger) (string, error) { return other, nil }, false}, + {"direct exec, resolver fails", osDarwin, func(*zap.Logger) (string, error) { return "", fmt.Errorf("docker not found") }, false}, + {"shell wrap keeps the configured binary", "linux", func(*zap.Logger) (string, error) { return other, nil }, true}, + } { + t.Run(tc.name, func(t *testing.T) { + forceDockerDaemonEnvGOOS(t, tc.goos) + t.Setenv("DOCKER_HOST", "") + t.Setenv("DOCKER_CONTEXT", "") + resolveDockerBinary = tc.resolve + c := newUserSuppliedDockerTestClient() + c.config.Command = configured + + cmd, args, shellWrapped, dir := c.resolveDockerSpawn(c.config.Args) + require.Equal(t, tc.wantWrap, shellWrapped) + require.Equal(t, filepath.Dir(configured), dir) + if shellWrapped { + require.Contains(t, args[len(args)-1], configured) + return + } + require.Equal(t, configured, cmd) + }) + } +} From f9105b468ae3991ccd0dabba8041e2128c96537f Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 29 Oct 2026 12:01:38 +0200 Subject: [PATCH 3/5] fix(docker): honour a relative configured docker path against working_dir Sol review round 2 of #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 --- internal/upstream/core/connection_docker.go | 26 ++++++++++++++++--- .../upstream/core/docker_run_command_test.go | 25 ++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index f12bee9b1..bf0af1df7 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -397,14 +397,32 @@ func (c *Client) insertCidfileIntoShellDockerCommand(shellArgs []string, cidFile } // configuredDockerBinary returns the server's own command when it is a -// user-supplied `docker run` naming the docker CLI by an absolute path to an -// executable, so that binary is spawned instead of whichever docker the +// user-supplied `docker run` naming the docker CLI by an absolute or relative +// path to an executable, so that binary is spawned instead of whichever docker the // resolver finds. "" means resolve as usual. func (c *Client) configuredDockerBinary() (string, error) { - if c.config == nil || !isDockerRunCommand(c.config.Command, c.config.Args) || !isDirectExecutable(c.config.Command) { + if c.config == nil || !isDockerRunCommand(c.config.Command, c.config.Args) { return "", nil } - return c.config.Command, nil + command := c.config.Command + // A relative path is evaluated against working_dir, as exec does with + // Cmd.Dir (or the process directory without one). A bare name is a PATH + // lookup, which the resolver handles. + if !filepath.IsAbs(command) && strings.ContainsAny(command, `/\`) { + base := c.config.WorkingDir + if base == "" { + wd, err := os.Getwd() + if err != nil { + return "", nil + } + base = wd + } + command = filepath.Join(base, command) + } + if !isDirectExecutable(command) { + return "", nil + } + return command, nil } // isDockerRunCommand reports whether a configured command is a user-supplied diff --git a/internal/upstream/core/docker_run_command_test.go b/internal/upstream/core/docker_run_command_test.go index 05f13eabf..34386dcba 100644 --- a/internal/upstream/core/docker_run_command_test.go +++ b/internal/upstream/core/docker_run_command_test.go @@ -76,3 +76,28 @@ func TestResolveDockerSpawnHonorsConfiguredAbsoluteDocker(t *testing.T) { }) } } + +// A relative docker path (./bin/docker, .\bin\docker.exe) names a binary too, +// relative to the server's working_dir; it must not be swapped for whichever +// docker the resolver finds (Sol review round 2 of #1777). +func TestResolveDockerSpawnHonorsConfiguredRelativeDocker(t *testing.T) { + configured := writeFakeDockerExecutable(t) + other := writeFakeDockerExecutable(t) + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(*zap.Logger) (string, error) { return other, nil } + forceDockerDaemonEnvGOOS(t, osDarwin) + + c := newUserSuppliedDockerTestClient() + c.config.WorkingDir = filepath.Dir(filepath.Dir(configured)) + c.config.Command = "." + string(filepath.Separator) + filepath.Join(filepath.Base(filepath.Dir(configured)), filepath.Base(configured)) + + cmd, _, shellWrapped, _ := c.resolveDockerSpawn(c.config.Args) + require.False(t, shellWrapped) + require.Equal(t, configured, cmd) + + // A bare name is a PATH lookup, so the resolver still decides. + c.config.Command = "docker" + cmd, _, _, _ = c.resolveDockerSpawn(c.config.Args) + require.Equal(t, other, cmd) +} From fb1dbb453f5a02bd9b65095fff1333ea78a89309 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 29 Oct 2026 12:12:25 +0200 Subject: [PATCH 4/5] fix(docker): resolve a relative working_dir before checking the configured docker Sol review round 3 of #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 --- internal/upstream/core/connection_docker.go | 6 +++++- internal/upstream/core/docker_run_command_test.go | 10 ++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index bf0af1df7..7fec7ff94 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -417,7 +417,11 @@ func (c *Client) configuredDockerBinary() (string, error) { } base = wd } - command = filepath.Join(base, command) + abs, err := filepath.Abs(filepath.Join(base, command)) + if err != nil { + return "", nil + } + command = abs } if !isDirectExecutable(command) { return "", nil diff --git a/internal/upstream/core/docker_run_command_test.go b/internal/upstream/core/docker_run_command_test.go index 34386dcba..6561e40c4 100644 --- a/internal/upstream/core/docker_run_command_test.go +++ b/internal/upstream/core/docker_run_command_test.go @@ -2,6 +2,7 @@ package core import ( "fmt" + "os" "path/filepath" "testing" @@ -96,6 +97,15 @@ func TestResolveDockerSpawnHonorsConfiguredRelativeDocker(t *testing.T) { require.False(t, shellWrapped) require.Equal(t, configured, cmd) + // A relative working_dir is itself relative to the process directory. + wd, err := os.Getwd() + require.NoError(t, err) + rel, err := filepath.Rel(wd, filepath.Dir(filepath.Dir(configured))) + require.NoError(t, err) + c.config.WorkingDir = rel + cmd, _, _, _ = c.resolveDockerSpawn(c.config.Args) + require.Equal(t, configured, cmd) + // A bare name is a PATH lookup, so the resolver still decides. c.config.Command = "docker" cmd, _, _, _ = c.resolveDockerSpawn(c.config.Args) From 75018fce16190a64bed16e065e2f0ba1cb485477 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 30 Oct 2026 12:11:28 +0200 Subject: [PATCH 5/5] test(docker): chdir for the relative working_dir case so it works across Windows drives Co-Authored-By: Claude Opus 5.5 --- internal/upstream/core/docker_run_command_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/internal/upstream/core/docker_run_command_test.go b/internal/upstream/core/docker_run_command_test.go index 6561e40c4..67c5faf01 100644 --- a/internal/upstream/core/docker_run_command_test.go +++ b/internal/upstream/core/docker_run_command_test.go @@ -2,7 +2,6 @@ package core import ( "fmt" - "os" "path/filepath" "testing" @@ -98,11 +97,11 @@ func TestResolveDockerSpawnHonorsConfiguredRelativeDocker(t *testing.T) { require.Equal(t, configured, cmd) // A relative working_dir is itself relative to the process directory. - wd, err := os.Getwd() - require.NoError(t, err) - rel, err := filepath.Rel(wd, filepath.Dir(filepath.Dir(configured))) - require.NoError(t, err) - c.config.WorkingDir = rel + // Chdir next to it: on Windows the temp dir and the checkout can sit on + // different drives, where no relative path between them exists. + workDir := filepath.Dir(filepath.Dir(configured)) + t.Chdir(filepath.Dir(workDir)) + c.config.WorkingDir = filepath.Base(workDir) cmd, _, _, _ = c.resolveDockerSpawn(c.config.Args) require.Equal(t, configured, cmd)