fix: red-team bug-fix sweep (~50 bugs, 49 new tests)

CLAUDE.md

@@ -11,7 +11,7 @@ Pipeline: `you/CI → dcon (Go) → container CLI → container-apiserver + plug
 ## Commands
 
 ```sh
-make build          # build ./dcon with release flags (-s -w -trimpath, ~5.3 MB); plain `go build` is larger/unstripped
+make build          # build ./dcon with release flags (-s -w -trimpath, ~6.2 MB); plain `go build` is larger/unstripped
 make test           # go test ./...
 make test-race      # go test -race ./...   (what CI runs, with coverage)
 make cover          # scripts/coverage.sh — total coverage + refreshes the README badge
@@ -37,6 +37,10 @@ To **run** dcon for real you need Apple `container` installed and started (`dcon
 
 **`internal/compose`** is a built-in compose engine: `model.go` parses `compose.yaml` into a `Project`; `translate.go` turns each service into `container run` args. `Project.Order()` is topological; `Project.Levels()` groups services into dependency levels. `cmd/compose.go`'s `up` brings each level up **concurrently** (capped by `DCON_COMPOSE_PARALLEL`, default 8), preserving `depends_on` across levels.
 
+**`internal/ui`** is the optional Charm/lipgloss styling layer. `ui.Enabled()` is the single gate — true only when stdout is a TTY and neither `DCON_PLAIN` nor `NO_COLOR` is set. `dockerfmt.Render` upgrades the default (non `--format`/`-q`) table to a styled `ui.Table` **only** when `ui.Enabled() && len(views) > 0`; every machine-readable path (json/template/`-q`, pipes, CI, empty lists) falls through to the byte-identical tabwriter output. `doctor`/`version`/`info`/`compose` colourise via `ui.*` helpers, which are no-ops when disabled. **Non-TTY output must never change** — `internal/dockerfmt/render_test.go` locks the byte-for-byte contract; `ui.SetEnabled(bool)` forces the gate in tests (CI has no TTY).
+
+**`internal/machine` + `cmd/machine.go`** are OrbStack-style persistent Linux machines: a machine is a long-lived detached `container run -d --entrypoint sleep <distro-image> 2147483647` with labels `dcon.machine=1`/`.name`/`.distro`, named `dcon-machine-<name>` so it can never collide with a user container. `registry.go` maps 16 distro ids → images; `run.go`'s `BuildRunArgs` is the pure (unit-tested) arg builder; `state.go` persists only the *default* machine pointer (flock JSON, same pattern as pool) — the machine list is derived from the backend by label. Every mutating command resolves bare name → prefixed id via `resolveMachine`, which **re-verifies the label** before acting. Persistence across stop/start is the standard container lifecycle (verified manually). `rename` is unsupported (the backend container name is immutable).
+
 **`internal/pool`** is the warm-VM pool — the one subsystem with real concurrency and persisted state. It pre-boots single-use microVMs so an eligible `--rm` run can `exec` into a ready VM (~90 ms) instead of cold-booting (~700 ms). Key invariants:
 - **Daemonless state**: a flock-guarded JSON file (`~/Library/Application Support/dcon/pool.json`) lists only *available* members; `Claim` pops one atomically so concurrent runs never share a VM.
 - **Single-use = isolation preserved**: each member is handed out exactly once then destroyed, so every run still gets a fresh microVM. Background boot/destroy/replenish are detached (`setsid`) processes that outlive the CLI.

cmd/build.go

@@ -28,33 +28,53 @@ func newBuildCmd() *cobra.Command {
 }
 
 // translateOutput maps a Docker --output exporter spec onto what the container
-// backend accepts (oci|tar|local). docker/image become oci; registry errors.
-func translateOutput(spec string) (string, error) {
+// backend accepts. It returns the --output value to pass (empty = omit
+// --output) and a tag to apply via --tag (empty = none).
+//
+//   - oci/tar/local: passed through unchanged.
+//   - docker/image WITHOUT a dest: this is buildx's "load into the local image
+//     store" (the long form of --load), which is the backend's default. Omit
+//     --output entirely and carry name= through as a --tag, so the requested
+//     name isn't silently lost. (The old code remapped this to type=oci, which
+//     is an OCI-layout export, NOT a load — divergent from --load and it dropped
+//     name= outright.)
+//   - docker/image WITH a dest: a file export; remap type to oci, keep dest,
+//     drop name (a file export can't tag the local store).
+//   - registry / anything else: error.
+func translateOutput(spec string) (output, tag string, err error) {
 	fields := strings.Split(spec, ",")
-	var typ string
+	var typ, dest, name string
 	for _, fld := range fields {
-		if strings.HasPrefix(fld, "type=") {
+		switch {
+		case strings.HasPrefix(fld, "type="):
 			typ = strings.TrimPrefix(fld, "type=")
+		case strings.HasPrefix(fld, "dest="):
+			dest = strings.TrimPrefix(fld, "dest=")
+		case strings.HasPrefix(fld, "name="):
+			name = strings.TrimPrefix(fld, "name=")
 		}
 	}
 	switch typ {
 	case "", "oci", "tar", "local":
-		return spec, nil
+		return spec, "", nil
 	case "docker", "image":
-		// load into the local image store: remap to oci, keep other fields.
+		if dest == "" {
+			return "", name, nil // local-store load (backend default); name -> --tag
+		}
 		var out []string
 		for _, fld := range fields {
-			if strings.HasPrefix(fld, "type=") {
+			switch {
+			case strings.HasPrefix(fld, "type="):
 				out = append(out, "type=oci")
-			} else if strings.HasPrefix(fld, "name=") {
-				continue // covered by --tag
-			} else {
+			case strings.HasPrefix(fld, "name="):
+				// drop: a file export can't tag the local store
+			default:
 				out = append(out, fld)
 			}
 		}
-		return strings.Join(out, ","), nil
+		return strings.Join(out, ","), "", nil
 	default:
-		return "", fmt.Errorf("--output type=%s is not supported by the backend (use docker, image, oci, tar, or local); push separately with 'dcon push'", typ)
+		return "", "", fmt.Errorf("--output type=%s is not supported by the backend (use docker, image, oci, tar, or local); push separately with 'dcon push'", typ)
 	}
 }
 
@@ -96,16 +116,25 @@ func buildBuildArgs(cmd *cobra.Command, args []string) ([]string, error) {
 	if p, _ := f.GetString("platform"); p != "" {
 		cargs = append(cargs, "--platform", p)
 	}
+	userHasTag := len(mustStringArray(f, "tag")) > 0
 	for _, o := range mustStringArray(f, "output") {
-		mapped, err := translateOutput(o)
+		mapped, tag, err := translateOutput(o)
 		if err != nil {
 			return nil, err
 		}
-		cargs = append(cargs, "--output", mapped)
+		if mapped != "" {
+			cargs = append(cargs, "--output", mapped)
+		}
+		// Preserve a name= from --output as the image tag when the user gave no
+		// explicit -t, so 'build --output type=docker,name=x' still tags x.
+		if tag != "" && !userHasTag {
+			cargs = append(cargs, "--tag", tag)
+		}
 	}
 	if pr, _ := f.GetString("progress"); pr != "" && pr != "auto" {
-		// docker has rawjson/quiet; container supports auto|plain|tty.
-		if pr == "rawjson" {
+		// docker has rawjson/quiet; container supports auto|plain|tty. Remap the
+		// docker-only values to plain so the backend doesn't reject them.
+		if pr == "rawjson" || pr == "quiet" {
 			pr = "plain"
 		}
 		cargs = append(cargs, "--progress", pr)

cmd/cli_fixes_test.go

@@ -0,0 +1,229 @@
+package cmd
+
+import (
+	"encoding/json"
+	"io"
+	"reflect"
+	"strings"
+	"testing"
+	"unicode/utf8"
+
+	"dcon/internal/dockerfmt"
+
+	"github.com/spf13/cobra"
+)
+
+// TestTopPassesDashedPsOptions reproduces the bug where `top web -ef` aborted
+// with "unknown shorthand flag: 'e'" because cobra parsed the ps options as
+// flags of the top command. With SetInterspersed(false) they pass through.
+func TestTopPassesDashedPsOptions(t *testing.T) {
+	cmd := newTopCmd()
+	var got []string
+	cmd.RunE = func(c *cobra.Command, args []string) error { got = args; return nil }
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+	cmd.SetArgs([]string{"web", "-ef"})
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("`top web -ef` must not error: %v", err)
+	}
+	if !reflect.DeepEqual(got, []string{"web", "-ef"}) {
+		t.Errorf("args reaching RunE = %v, want [web -ef]", got)
+	}
+}
+
+// TestFormatCreatedByRuneSafe reproduces the byte-truncation bug: slicing
+// created_by at byte 42 could split a multibyte UTF-8 rune, emitting invalid
+// UTF-8. Truncation must be rune-safe.
+func TestFormatCreatedByRuneSafe(t *testing.T) {
+	// Leading ASCII + 3-byte runes so byte 42 lands mid-rune.
+	in := "x" + strings.Repeat("あ", 50)
+	out := formatCreatedBy(in, false)
+	if !utf8.ValidString(out) {
+		t.Errorf("rune-unsafe truncation produced invalid UTF-8: %q", out)
+	}
+	if r := []rune(out); len(r) != 45 || string(r[len(r)-3:]) != "..." {
+		t.Errorf("want 42 runes + '...'; got %d runes (%q)", len(r), out)
+	}
+	if got := formatCreatedBy("/bin/sh -c #(nop) CMD [\"x\"]", false); got != `CMD ["x"]` {
+		t.Errorf("prefix strip wrong: %q", got)
+	}
+	if got := formatCreatedBy("/bin/sh -c apk add curl", false); got != "apk add curl" {
+		t.Errorf("shell prefix strip wrong: %q", got)
+	}
+}
+
+// TestCpIsContainerRef reproduces the bug where a local path containing a colon
+// (./my:file.txt) was misclassified as a CONTAINER:PATH reference.
+func TestCpIsContainerRef(t *testing.T) {
+	for _, p := range []string{"./my:file.txt", "../x:y", "/abs:path", "/abs/path", "plainfile", ":leading"} {
+		if cpIsContainerRef(p) {
+			t.Errorf("%q should be treated as a local path", p)
+		}
+	}
+	for _, p := range []string{"web:/tmp", "mycontainer:/var/log", "abc123:/x"} {
+		if !cpIsContainerRef(p) {
+			t.Errorf("%q should be treated as a container ref", p)
+		}
+	}
+}
+
+// TestPortMappingLinesExpandsRange reproduces the bug where `port` ignored a
+// published range (PublishPort.Count>1), printing only the base port. A range
+// must expand to one line per port, and per-port filtering must resolve into it.
+func TestPortMappingLinesExpandsRange(t *testing.T) {
+	ports := []dockerfmt.PublishPort{
+		{HostAddress: "0.0.0.0", HostPort: 8000, ContainerPort: 80, Proto: "tcp", Count: 3},
+	}
+	all := portMappingLines(ports, "", "")
+	want := []string{
+		"80/tcp -> 0.0.0.0:8000",
+		"81/tcp -> 0.0.0.0:8001",
+		"82/tcp -> 0.0.0.0:8002",
+	}
+	if !reflect.DeepEqual(all, want) {
+		t.Errorf("range expansion = %v, want %v", all, want)
+	}
+	// Per-port filter resolves a non-base port within the range.
+	if got := portMappingLines(ports, "81", ""); !reflect.DeepEqual(got, []string{"0.0.0.0:8001"}) {
+		t.Errorf("filter 81 = %v, want [0.0.0.0:8001]", got)
+	}
+	// Count 0 is treated as a single port.
+	single := portMappingLines([]dockerfmt.PublishPort{{HostPort: 9000, ContainerPort: 90, Proto: "tcp"}}, "", "")
+	if len(single) != 1 {
+		t.Errorf("count 0 should yield one line, got %v", single)
+	}
+}
+
+// TestRestartStopArgsForwardsSignal reproduces the bug where restart's
+// --signal flag was defined but never used. It must be forwarded to the stop
+// phase (the backend stop accepts --signal), and --time only when set.
+func TestRestartStopArgsForwardsSignal(t *testing.T) {
+	if got := restartStopArgs(false, 5, ""); !reflect.DeepEqual(got, []string{"stop"}) {
+		t.Errorf("no flags set: got %v, want [stop]", got)
+	}
+	if got := restartStopArgs(false, 5, "SIGTERM"); !reflect.DeepEqual(got, []string{"stop", "--signal", "SIGTERM"}) {
+		t.Errorf("--signal must be forwarded: got %v", got)
+	}
+	if got := restartStopArgs(true, 10, "SIGKILL"); !reflect.DeepEqual(got, []string{"stop", "--time", "10", "--signal", "SIGKILL"}) {
+		t.Errorf("--time + --signal: got %v", got)
+	}
+}
+
+// TestMergeInspectArrays guards the mixed container+image inspect merge.
+func TestMergeInspectArrays(t *testing.T) {
+	got, err := mergeInspectArrays([]string{`[{"id":"a"}]`, "", `[{"id":"b"}]`})
+	if err != nil {
+		t.Fatal(err)
+	}
+	var items []map[string]any
+	if jerr := json.Unmarshal([]byte(got), &items); jerr != nil {
+		t.Fatalf("merged output is not valid JSON: %v (%q)", jerr, got)
+	}
+	if len(items) != 2 || items[0]["id"] != "a" || items[1]["id"] != "b" {
+		t.Errorf("merged = %v, want two elements a,b", items)
+	}
+	if out, _ := mergeInspectArrays(nil); out != "" {
+		t.Errorf("empty input should yield empty string, got %q", out)
+	}
+	if _, err := mergeInspectArrays([]string{"not json"}); err == nil {
+		t.Error("invalid JSON input should error")
+	}
+}
+
+// TestResolveVolumeName reproduces the bug where `volume create --name X` was
+// ignored (a random-named volume was created). --name (and the positional) must
+// be honored, with both-supplied a conflict.
+func TestResolveVolumeName(t *testing.T) {
+	if n, err := resolveVolumeName("myvol", true, nil); err != nil || n != "myvol" {
+		t.Errorf("--name myvol -> (%q,%v), want myvol", n, err)
+	}
+	if n, err := resolveVolumeName("", false, []string{"posvol"}); err != nil || n != "posvol" {
+		t.Errorf("positional -> (%q,%v), want posvol", n, err)
+	}
+	if _, err := resolveVolumeName("myvol", true, []string{"posvol"}); err == nil {
+		t.Error("supplying both --name and a positional must error")
+	}
+	if n, err := resolveVolumeName("", false, nil); err != nil || len(n) != 64 {
+		t.Errorf("no name -> random 64-hex id; got len %d err %v", len(n), err)
+	}
+	// An explicit but empty --name is invalid usage, not a request for an
+	// anonymous volume — it must error rather than silently generate a name.
+	if _, err := resolveVolumeName("", true, nil); err == nil {
+		t.Error("explicit empty --name must error, not generate an anonymous volume")
+	}
+}
+
+// TestComposeStopAndKillArgs reproduces the bug where compose kill --signal and
+// compose stop/restart --timeout were dropped: the backend stop accepts --time
+// and kill accepts --signal, so they must be forwarded.
+func TestComposeStopAndKillArgs(t *testing.T) {
+	if got := composeStopArgs(false, 10, "c1"); !reflect.DeepEqual(got, []string{"stop", "c1"}) {
+		t.Errorf("timeout unset: %v, want [stop c1]", got)
+	}
+	if got := composeStopArgs(true, 30, "c1"); !reflect.DeepEqual(got, []string{"stop", "--time", "30", "c1"}) {
+		t.Errorf("timeout 30: %v, want [stop --time 30 c1]", got)
+	}
+	if got := composeKillArgs("SIGTERM", "c1"); !reflect.DeepEqual(got, []string{"kill", "--signal", "SIGTERM", "c1"}) {
+		t.Errorf("kill -s SIGTERM: %v", got)
+	}
+	if got := composeKillArgs("", "c1"); !reflect.DeepEqual(got, []string{"kill", "c1"}) {
+		t.Errorf("kill no signal: %v", got)
+	}
+}
+
+// TestBuildProgressQuiet reproduces the bug where `build --progress quiet` was
+// forwarded verbatim (the backend only accepts auto|plain|tty); quiet (like
+// rawjson) must be remapped to plain.
+func TestBuildProgressQuiet(t *testing.T) {
+	for _, v := range []string{"quiet", "rawjson"} {
+		c := parse(t, newBuildCmd(), []string{"--progress", v, "."})
+		got, err := buildBuildArgs(c, c.Flags().Args())
+		if err != nil {
+			t.Fatal(err)
+		}
+		if !containsPair(got, "--progress", "plain") {
+			t.Errorf("--progress %s should map to plain; got %v", v, got)
+		}
+		if containsPair(got, "--progress", v) {
+			t.Errorf("--progress %s should not be forwarded verbatim; got %v", v, got)
+		}
+	}
+}
+
+// TestBuilderRunning reproduces the doctor bug where the substring "running"
+// also matched "not running".
+func TestBuilderRunning(t *testing.T) {
+	for _, out := range []string{"running", "Builder is running", "status: running"} {
+		if !builderRunning(out) {
+			t.Errorf("builderRunning(%q) = false, want true", out)
+		}
+	}
+	for _, out := range []string{"not running", "Builder is not running", "stopped", ""} {
+		if builderRunning(out) {
+			t.Errorf("builderRunning(%q) = true, want false", out)
+		}
+	}
+}
+
+// TestSystemPrunePlan covers the prune step plan that the error-propagating
+// loop runs (the bug fixed alongside it was that every step's error was
+// discarded and the command always exited 0).
+func TestSystemPrunePlan(t *testing.T) {
+	base := systemPrunePlan(false, false)
+	if len(base) != 3 {
+		t.Fatalf("base plan has %d steps, want 3", len(base))
+	}
+	if reflect.DeepEqual(base[1].args, []string{"image", "prune", "--all"}) {
+		t.Error("default prune must not pass --all to image prune")
+	}
+	full := systemPrunePlan(true, true)
+	if len(full) != 4 {
+		t.Fatalf("all+volumes plan has %d steps, want 4", len(full))
+	}
+	if !reflect.DeepEqual(full[1].args, []string{"image", "prune", "--all"}) {
+		t.Errorf("--all should add --all to image prune; got %v", full[1].args)
+	}
+	if !reflect.DeepEqual(full[3].args, []string{"volume", "prune"}) {
+		t.Errorf("--volumes should append a volume prune; got %v", full[3].args)
+	}
+}