From b66a1c7efebf2f605fa5c063db9fcf5a015b62aa Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 12 Nov 2026 19:40:48 -0300 Subject: [PATCH 1/3] fix(cli): report a failed container lookup in stop and down stop and down dropped the error of the container lookup and of the inspect that follows it, and of os.Getwd. An engine that refused the list reported {"outcome":"success","result":"no-container-found"} with exit 0, and a container that could not be inspected was stopped as a plain container even when it belonged to a Compose project. Each now ends in an error result. stop and down are Go-only commands, so no reference behavior applies. Co-Authored-By: Claude Opus 5.5 --- internal/cli/stop.go | 24 +++++++++++++------ internal/cli/stop_held_test.go | 42 ++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 7 deletions(-) diff --git a/internal/cli/stop.go b/internal/cli/stop.go index 5c441e6..d514d49 100644 --- a/internal/cli/stop.go +++ b/internal/cli/stop.go @@ -77,7 +77,11 @@ func addStopFlags(cmd *cobra.Command, opts *stopOpts) { func runStopOrDown(ctx context.Context, out Output, opts stopOpts, remove bool) error { if opts.workspaceFolder == "" { - opts.workspaceFolder, _ = os.Getwd() + wd, err := os.Getwd() + if err != nil { + return writeErrorResult(out, fmt.Sprintf("current directory: %v", err)) + } + opts.workspaceFolder = wd } logger := log.New(log.Options{Writer: out.Stderr(), Format: "text"}) @@ -87,13 +91,20 @@ func runStopOrDown(ctx context.Context, out Output, opts stopOpts, remove bool) } defer engine.Close() - id := resolveWorkspaceContainer(ctx, engine, opts) + id, err := resolveWorkspaceContainer(ctx, engine, opts) + if err != nil { + return writeErrorResult(out, err.Error()) + } if id == "" { return writeSuccessJSON(out, map[string]any{"outcome": "success", "result": "no-container-found"}) } + insp, err := engine.InspectContainer(ctx, id) + if err != nil { + return writeErrorResult(out, fmt.Sprintf("inspect container %s: %v", id, err)) + } project := "" - if insp, ierr := engine.InspectContainer(ctx, id); ierr == nil && insp.Config != nil { + if insp.Config != nil { project = insp.Config.Labels["com.docker.compose.project"] } @@ -129,14 +140,13 @@ func runStopOrDown(ctx context.Context, out Output, opts stopOpts, remove bool) }) } -func resolveWorkspaceContainer(ctx context.Context, engine *docker.EngineClient, opts stopOpts) string { +func resolveWorkspaceContainer(ctx context.Context, engine *docker.EngineClient, opts stopOpts) (string, error) { if opts.containerID != "" { - return opts.containerID + return opts.containerID, nil } configFile := "" if opts.configPath != "" { configFile = resolvePath(opts.configPath) } - id, _ := resolveContainerID(ctx, engine, opts.workspaceFolder, configFile, opts.idLabels) - return id + return resolveContainerID(ctx, engine, opts.workspaceFolder, configFile, opts.idLabels) } diff --git a/internal/cli/stop_held_test.go b/internal/cli/stop_held_test.go index 630aa23..4a09e8c 100644 --- a/internal/cli/stop_held_test.go +++ b/internal/cli/stop_held_test.go @@ -20,6 +20,7 @@ type fakeEngine struct { mu sync.Mutex containers map[string]map[string]string stopFails bool + listFails bool calls []string } @@ -69,6 +70,11 @@ func (e *fakeEngine) serve(w http.ResponseWriter, r *http.Request) { } slices.Sort(want) e.record("list " + strings.Join(want, ",")) + if e.listFails { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, `{"message":"cannot list"}`) + return + } items := []map[string]string{} for id, labels := range e.containers { if hasLabels(labels, want) { @@ -219,6 +225,42 @@ func TestStopWithNoContainerSucceedsWithoutDoingAnything(t *testing.T) { } } +func TestStopOrDownReportsALookupTheEngineRefused(t *testing.T) { + engine := startFakeEngine(t, map[string]map[string]string{}) + engine.listFails = true + cli, _ := fakeDockerCLI(t, false, 0) + + got := runStopOrDownForTest(t, stopOpts{workspaceFolder: t.TempDir(), dockerPath: cli}, true) + + assertResult(t, got, map[string]any{"outcome": "error"}) + if msg, _ := got["message"].(string); !strings.HasPrefix(msg, "Command failed: docker ps ") { + t.Errorf("message = %q, want the failed container lookup", msg) + } + for _, c := range engine.recorded() { + if !strings.HasPrefix(c, "list ") { + t.Errorf("engine call %q after the lookup failed", c) + } + } +} + +func TestStopOrDownReportsAContainerItCannotInspect(t *testing.T) { + engine := startFakeEngine(t, map[string]map[string]string{}) + cli, composeCalls := fakeDockerCLI(t, false, 0) + + got := runStopOrDownForTest(t, stopOpts{containerID: "gone", dockerPath: cli}, false) + + assertResult(t, got, map[string]any{"outcome": "error"}) + if msg, _ := got["message"].(string); !strings.HasPrefix(msg, "inspect container gone: ") { + t.Errorf("message = %q, want the inspect failure", msg) + } + if calls := engine.recorded(); len(calls) != 0 { + t.Errorf("engine calls = %v after the inspect failed", calls) + } + if calls := composeCalls(); calls != nil { + t.Errorf("docker CLI ran %v", calls) + } +} + func TestDownStopsAndRemovesTheContainer(t *testing.T) { engine := startFakeEngine(t, map[string]map[string]string{"c1": {}}) cli, _ := fakeDockerCLI(t, false, 0) From 2d6c9f4f78d3e2c2143d06d95c5bd9ef2f802aa6 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 12 Nov 2026 19:41:39 -0300 Subject: [PATCH 2/3] fix(docker): stop panicking when the docker context can't be read FindDockerDriverBuilder read the stdout of `docker context inspect` without checking its error, and Run returns a nil result when the command does not start or the context is cancelled, so a Feature build on a docker-container builder could end in a nil pointer panic. A context that can't be read now counts as no current context, and the first docker-driver builder is still chosen, as when `buildx ls` names no builder for the current context. Co-Authored-By: Claude Opus 5.5 --- internal/docker/argbuilders_test.go | 3 ++- internal/docker/builder_detect_held_test.go | 12 ++++++++++++ internal/docker/client.go | 6 ++++-- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/internal/docker/argbuilders_test.go b/internal/docker/argbuilders_test.go index b268239..b6786aa 100644 --- a/internal/docker/argbuilders_test.go +++ b/internal/docker/argbuilders_test.go @@ -23,6 +23,7 @@ type cannedResponse struct { stdout []byte stderr []byte code int + err error } func (r *recordingRunner) Run(_ context.Context, stream io.Writer, name string, args ...string) ([]byte, []byte, int, error) { @@ -34,7 +35,7 @@ func (r *recordingRunner) Run(_ context.Context, stream io.Writer, name string, _, _ = stream.Write(resp.stdout) _, _ = stream.Write(resp.stderr) } - return resp.stdout, resp.stderr, resp.code, nil + return resp.stdout, resp.stderr, resp.code, resp.err } return nil, nil, 0, nil } diff --git a/internal/docker/builder_detect_held_test.go b/internal/docker/builder_detect_held_test.go index 1af47a7..f3b0da3 100644 --- a/internal/docker/builder_detect_held_test.go +++ b/internal/docker/builder_detect_held_test.go @@ -1,6 +1,7 @@ package docker import ( + "errors" "testing" "github.com/devcontainers/cli/internal/log" @@ -22,3 +23,14 @@ func TestBuilderDetectionIgnoresAFailedBuildx(t *testing.T) { t.Errorf("FindDockerDriverBuilder after a failed ls = %q, want empty", got) } } + +func TestFindDockerDriverBuilderWhenTheContextCannotBeRead(t *testing.T) { + rr := &recordingRunner{responses: []cannedResponse{ + {err: errors.New("exec docker: no such file")}, + {stdout: []byte("NAME DRIVER\nremote* docker-container running\ndefault docker running\n")}, + }} + c := &Client{DockerPath: "docker", Log: log.Null, Runner: rr} + if got := c.FindDockerDriverBuilder(t.Context()); got != "default" { + t.Errorf("FindDockerDriverBuilder with no current context = %q, want the first docker-driver builder", got) + } +} diff --git a/internal/docker/client.go b/internal/docker/client.go index 10ddf56..7ace159 100644 --- a/internal/docker/client.go +++ b/internal/docker/client.go @@ -218,8 +218,10 @@ func (c *Client) DetectActiveBuilder(ctx context.Context) *BuilderInfo { } func (c *Client) FindDockerDriverBuilder(ctx context.Context) string { - ctxRes, _ := c.Run(ctx, "context", "inspect", "--format", "{{.Name}}") - currentContext := strings.TrimSpace(string(ctxRes.Stdout)) + currentContext := "" + if ctxRes, err := c.Run(ctx, "context", "inspect", "--format", "{{.Name}}"); err == nil && ctxRes.ExitCode == 0 { + currentContext = strings.TrimSpace(string(ctxRes.Stdout)) + } res, err := c.Run(ctx, "buildx", "ls") if err != nil || res.ExitCode != 0 { From 2db1aeaf3db9e2b772fd3c6b888708627998ba84 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 12 Nov 2026 19:42:46 -0300 Subject: [PATCH 3/3] fix(lifecycle): log why the user environment probe failed When the userEnvProbe could not run in the container, the lifecycle commands got an empty environment and nothing said why. The reference CLI (injectHeadless.ts runUserEnvProbe) also falls back to an empty environment, but writes the error first with toErrorText; Go now does the same, through a log.ToErrorText that renders TS's red text. Co-Authored-By: Claude Opus 5.5 --- internal/lifecycle/probe.go | 5 ++++- internal/lifecycle/shell_test.go | 9 ++++++++- internal/log/log.go | 8 ++++++++ internal/log/log_test.go | 7 +++++++ 4 files changed, 27 insertions(+), 2 deletions(-) diff --git a/internal/lifecycle/probe.go b/internal/lifecycle/probe.go index 02d074a..1643bfb 100644 --- a/internal/lifecycle/probe.go +++ b/internal/lifecycle/probe.go @@ -171,7 +171,10 @@ func ProbedRemoteEnv(ctx context.Context, exec ContainerExecutor, logger log.Log } server := NewShellServer(ctx, exec, containerID, remoteUser) defer server.Close() - env, _ := ProbeRemoteEnv(logger, server, strategy, remoteUser, sessionDataFolder) + env, err := ProbeRemoteEnv(logger, server, strategy, remoteUser, sessionDataFolder) + if err != nil { + logger.Write(log.ToErrorText(err.Error()), log.LevelInfo) + } if env == nil { env = map[string]string{} } diff --git a/internal/lifecycle/shell_test.go b/internal/lifecycle/shell_test.go index 1cb09a4..19c36bd 100644 --- a/internal/lifecycle/shell_test.go +++ b/internal/lifecycle/shell_test.go @@ -1,8 +1,10 @@ package lifecycle import ( + "bytes" "context" "errors" + "strings" "testing" "github.com/devcontainers/cli/internal/log" @@ -119,7 +121,12 @@ func TestProbedRemoteEnv(t *testing.T) { } fe = &fakeExecutor{fn: func(string) (string, string, int, error) { return "", "", 1, errors.New("no shell") }} - if env := ProbedRemoteEnv(t.Context(), fe, log.Null, "cid", "vscode", ProbeLoginShell, ""); env == nil || len(env) != 0 { + var logged bytes.Buffer + logger := log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: &logged}) + if env := ProbedRemoteEnv(t.Context(), fe, logger, "cid", "vscode", ProbeLoginShell, ""); env == nil || len(env) != 0 { t.Fatalf("failed probe: env = %v, want empty", env) } + if !strings.Contains(logged.String(), "no shell") { + t.Errorf("failed probe logged %q, want the probe's error", logged.String()) + } } diff --git a/internal/log/log.go b/internal/log/log.go index 65de9e1..a3e0b4e 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -284,6 +284,14 @@ const ( numberColor = "38;2;86;156;214" ) +func ToErrorText(s string) string { + lines := newlines.Split(s, -1) + for i, l := range lines { + lines[i] = color("31", l) + } + return strings.Join(lines, "\r\n") + "\r\n" +} + func color(c, s string) string { return "\x1b[1m\x1b[" + c + "m" + s + "\x1b[39m\x1b[22m" } diff --git a/internal/log/log_test.go b/internal/log/log_test.go index e3ac32d..8369367 100644 --- a/internal/log/log_test.go +++ b/internal/log/log_test.go @@ -189,3 +189,10 @@ func TestLogLevelFilter_TerminalVsJSON(t *testing.T) { t.Errorf("json must emit all levels, got %q", js) } } + +func TestToErrorTextMatchesTheReference(t *testing.T) { + want := "\x1b[1m\x1b[31mfirst\x1b[39m\x1b[22m\r\n\x1b[1m\x1b[31msecond\x1b[39m\x1b[22m\r\n" + if got := ToErrorText("first\r\nsecond"); got != want { + t.Errorf("ToErrorText = %q, want %q", got, want) + } +}