From 10d82bc8af563ce8e6d4ce8c43fee8a1fd0318db Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 10:17:04 +0530 Subject: [PATCH 01/17] fix(machine): close label-spoof resolution + force-by-default footguns 1. resolveMachine matched a container when the user-controllable dcon.machine.name label equalled the requested name. Because `dcon run` forwards arbitrary --label to the backend, an ordinary container created with --label dcon.machine=1 --label dcon.machine.name=web (and any non-prefixed name) would be resolved as machine `web`, turning `machine rm/stop/shell web` into a confused deputy against an arbitrary container and defeating the dcon-machine- prefix namespace. Resolve strictly by the prefixed backend id plus the verified dcon.machine label via a new pure matchMachine helper; a genuine machine always has id == dcon-machine-, so no real capability is lost. 2. `machine rm` had --force default to true, so a bare `rm` always force-deleted a running machine (irreversible FS loss) and -f was a no-op. Default to false to mirror `docker rm`: a bare rm of a running machine fails and tells the user to pass -f. Both bugs are covered by reproducing unit tests. --- cmd/machine.go | 50 +++++++++++++++++++++++++---------- cmd/machine_test.go | 63 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 13 deletions(-) diff --git a/cmd/machine.go b/cmd/machine.go index 8623826..6257453 100644 --- a/cmd/machine.go +++ b/cmd/machine.go @@ -109,6 +109,27 @@ func machineStateName(state string) string { } } +// matchMachine finds the machine container for a user-facing name among all +// backend containers. Resolution is by the prefixed backend ID *and* the +// verified dcon.machine label only — never by the free-form dcon.machine.name +// label, which is attacker-controllable (any `dcon run --label` can forge it). +// Matching the label would let a non-prefixed user container masquerade as a +// machine, defeating the prefix namespace and turning `machine rm/stop/shell` +// into a confused deputy against arbitrary containers. A genuine machine always +// has c.ID == dcon-machine-, so the prefix match loses no real capability. +func matchMachine(all []dockerfmt.Container, name string) (dockerfmt.Container, bool) { + cn := machine.ContainerName(name) + for _, c := range all { + if c.Configuration.Labels[machine.LabelMachine] != "1" { + continue + } + if c.ID == cn { + return c, true + } + } + return dockerfmt.Container{}, false +} + // resolveMachine looks up a machine by its user-facing name, re-verifying the // dcon.machine label so a mutating command can never act on a same-named // ordinary container. Returns the backing container (whose ID is the backend @@ -118,14 +139,8 @@ func resolveMachine(name string) (dockerfmt.Container, error) { if err != nil { return dockerfmt.Container{}, err } - cn := machine.ContainerName(name) - for _, c := range all { - if c.Configuration.Labels[machine.LabelMachine] != "1" { - continue - } - if c.ID == cn || c.Configuration.Labels[machine.LabelName] == name { - return c, nil - } + if c, ok := matchMachine(all, name); ok { + return c, nil } return dockerfmt.Container{}, fmt.Errorf("no such machine: %s", name) } @@ -424,10 +439,7 @@ func machineRmCmd() *cobra.Command { Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { force, _ := cmd.Flags().GetBool("force") - cargs := []string{"delete"} - if force { - cargs = append(cargs, "--force") - } + cargs := machineDeleteArgs(force) return forEachMachine(args, func(id, name string) error { if _, err := runtime.CaptureSilent(append(append([]string{}, cargs...), id)...); err != nil { return err @@ -438,10 +450,22 @@ func machineRmCmd() *cobra.Command { }) }, } - cmd.Flags().BoolP("force", "f", true, "Force removal of a running machine (stops it first)") + // Default false (not true): a bare `rm` of a running machine should fail and + // tell the user to pass -f, mirroring `docker rm`. Defaulting to force would + // make -f a no-op and silently destroy a running machine's filesystem. + cmd.Flags().BoolP("force", "f", false, "Force removal of a running machine (stops it first)") return cmd } +// machineDeleteArgs builds the backend delete argv, adding --force only when the +// user asked for it. +func machineDeleteArgs(force bool) []string { + if force { + return []string{"delete", "--force"} + } + return []string{"delete"} +} + // forEachMachine resolves and applies fn to each named machine, surfacing the // first error but attempting all of them. func forEachMachine(names []string, fn func(id, name string) error) error { diff --git a/cmd/machine_test.go b/cmd/machine_test.go index 11dfb0a..4cc6185 100644 --- a/cmd/machine_test.go +++ b/cmd/machine_test.go @@ -5,9 +5,56 @@ import ( "reflect" "testing" + "dcon/internal/dockerfmt" + "dcon/internal/machine" + "github.com/spf13/cobra" ) +// mkContainer is a tiny helper for building backend container fixtures. +func mkContainer(id string, labels map[string]string) dockerfmt.Container { + var c dockerfmt.Container + c.ID = id + c.Configuration.Labels = labels + return c +} + +// TestMatchMachineRejectsLabelSpoof guards the prefix-namespace invariant: a +// machine must resolve only by its prefixed backend ID + the dcon.machine +// label, never by the attacker-controllable dcon.machine.name label. A container +// that carries forged machine labels but is NOT named dcon-machine-* (which any +// `dcon run --label dcon.machine=1 --label dcon.machine.name=web` can create) +// must never be resolved as machine "web" — otherwise machine rm/stop/shell +// becomes a confused deputy against an arbitrary user container. +func TestMatchMachineRejectsLabelSpoof(t *testing.T) { + realMachine := mkContainer(machine.ContainerName("web"), map[string]string{ + machine.LabelMachine: "1", machine.LabelName: "web", + }) + spoof := mkContainer("evil", map[string]string{ + machine.LabelMachine: "1", machine.LabelName: "web", + }) + prefixedNonMachine := mkContainer(machine.ContainerName("web"), map[string]string{ + // user ran `dcon run --name dcon-machine-web` with no machine label + }) + + // 1. A lone spoof must NOT resolve as "web". + if c, ok := matchMachine([]dockerfmt.Container{spoof}, "web"); ok { + t.Errorf("label spoof resolved as machine web: got id %q (confused deputy)", c.ID) + } + // 2. With both present, only the genuinely prefixed machine resolves. + if c, ok := matchMachine([]dockerfmt.Container{spoof, realMachine}, "web"); !ok || c.ID != machine.ContainerName("web") { + t.Errorf("expected real machine, got id=%q ok=%v", c.ID, ok) + } + // 3. A prefixed container without the machine label is not a machine. + if _, ok := matchMachine([]dockerfmt.Container{prefixedNonMachine}, "web"); ok { + t.Error("prefixed but unlabeled container resolved as a machine") + } + // 4. A genuine machine resolves. + if _, ok := matchMachine([]dockerfmt.Container{realMachine}, "web"); !ok { + t.Error("genuine machine failed to resolve") + } +} + // runSplit drives splitNameAndCommand through a real cobra parse so the // ArgsLenAtDash (`--`) handling is exercised exactly as in production. func runSplit(t *testing.T, argv []string) (name string, command []string) { @@ -56,6 +103,22 @@ func TestSplitNameAndCommand(t *testing.T) { } } +// TestMachineRmForceDefault locks in that `machine rm` does NOT force by +// default — a bare rm of a running machine must fail (no --force) so its +// filesystem isn't silently destroyed, matching `docker rm`. -f opts in. +func TestMachineRmForceDefault(t *testing.T) { + cmd := machineRmCmd() + if def, _ := cmd.Flags().GetBool("force"); def { + t.Error("machine rm --force defaults to true; should be false (data-loss footgun)") + } + if got := machineDeleteArgs(false); !reflect.DeepEqual(got, []string{"delete"}) { + t.Errorf("machineDeleteArgs(false) = %v, want [delete]", got) + } + if got := machineDeleteArgs(true); !reflect.DeepEqual(got, []string{"delete", "--force"}) { + t.Errorf("machineDeleteArgs(true) = %v, want [delete --force]", got) + } +} + func TestMachineStateName(t *testing.T) { for in, want := range map[string]string{ "running": "running", From 744124a97a11f1c3eb0835a2362be390fed17d02 Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 10:19:46 +0530 Subject: [PATCH 02/17] fix(compose): parse long-form ports/volumes and drop empty env keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ports: and volumes: were plain []string, so the Compose long (mapping) form — e.g. 'ports: [{target: 80, published: "8080"}]' or 'volumes: [{type: bind, source: ./d, target: /d}]' — failed to unmarshal and hard-errored the entire compose file (every up/down/ps/config). This is valid, common syntax (what 'docker compose config' emits). Add PortList/VolumeList named types that accept both the short string and long mapping forms, flattening the long form to the same [host_ip:][published:]target[/proto] and [source:]target[:ro] strings the translator already emits. yaml scalar .Value is read directly so numeric ports (- 9000, target: 80) don't trip an int->string decode. Also drop blank/keyless environment & labels list entries ('' or '=value') in MapList, which previously injected a malformed empty-key '--env =' argument. Both covered by reproducing tests. --- internal/compose/model.go | 87 +++++++++++++++++++++++++++++++++- internal/compose/model_test.go | 83 ++++++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+), 2 deletions(-) diff --git a/internal/compose/model.go b/internal/compose/model.go index b81318f..15067bb 100644 --- a/internal/compose/model.go +++ b/internal/compose/model.go @@ -38,9 +38,9 @@ type Service struct { Entrypoint StringList `yaml:"entrypoint"` Environment MapList `yaml:"environment"` EnvFile EnvFile `yaml:"env_file"` - Ports []string `yaml:"ports"` + Ports PortList `yaml:"ports"` Expose []string `yaml:"expose"` - Volumes []string `yaml:"volumes"` + Volumes VolumeList `yaml:"volumes"` Networks StringKeys `yaml:"networks"` DependsOn DependsList `yaml:"depends_on"` Restart string `yaml:"restart"` @@ -266,6 +266,11 @@ func (m *MapList) UnmarshalYAML(value *yaml.Node) error { } for _, item := range list { kv := strings.SplitN(item, "=", 2) + // Skip blank / keyless entries (e.g. "" or "=value"): they would + // otherwise inject a malformed empty-key `--env =…` arg. + if kv[0] == "" { + continue + } if len(kv) == 2 { out[kv[0]] = kv[1] } else { @@ -277,6 +282,84 @@ func (m *MapList) UnmarshalYAML(value *yaml.Node) error { return nil } +// PortList handles compose `ports:` in both the short string form +// ("8080:80", "127.0.0.1:8080:80/tcp", "3000") and the long mapping form +// ({target, published, protocol, host_ip}), flattening the long form into the +// [host_ip:][published:]target[/protocol] string the translator emits via +// --publish. Without this, a sequence-of-maps fails to unmarshal into []string +// and breaks the entire compose file. +type PortList []string + +func (p *PortList) UnmarshalYAML(value *yaml.Node) error { + *p = decodeShortOrLong(value, flattenLongPort) + return nil +} + +func flattenLongPort(m map[string]string) string { + s := m["target"] + if pub := m["published"]; pub != "" { + s = pub + ":" + s + } + if hip := m["host_ip"]; hip != "" { + s = hip + ":" + s + } + if proto := m["protocol"]; proto != "" { + s += "/" + proto + } + return s +} + +// VolumeList handles compose `volumes:` in both the short string form +// ("./data:/data", "named:/data:ro") and the long mapping form +// ({type, source, target, read_only}), flattening the long form into the +// [source:]target[:ro] string the translator emits via --volume. +type VolumeList []string + +func (v *VolumeList) UnmarshalYAML(value *yaml.Node) error { + *v = decodeShortOrLong(value, flattenLongVolume) + return nil +} + +func flattenLongVolume(m map[string]string) string { + s := m["target"] + if src := m["source"]; src != "" { + s = src + ":" + s + } + if m["read_only"] == "true" { + s += ":ro" + } + return s +} + +// decodeShortOrLong flattens a compose sequence whose items are either scalar +// strings (short form, kept verbatim) or mappings (long form, flattened by fn). +// A bare scalar (single, unwrapped value) is also accepted. yaml scalar nodes +// carry their raw text in .Value, so numeric ports like `- 8080` and +// `target: 80` are read as "8080"/"80" without an int→string decode error. +func decodeShortOrLong(value *yaml.Node, fn func(map[string]string) string) []string { + var out []string + switch value.Kind { + case yaml.ScalarNode: + out = append(out, value.Value) + case yaml.SequenceNode: + for _, it := range value.Content { + switch it.Kind { + case yaml.ScalarNode: + out = append(out, it.Value) + case yaml.MappingNode: + m := map[string]string{} + for i := 0; i+1 < len(it.Content); i += 2 { + m[it.Content[i].Value] = it.Content[i+1].Value + } + if s := fn(m); s != "" { + out = append(out, s) + } + } + } + } + return out +} + // StringKeys handles networks which can be a list or a map (keyed by name). type StringKeys []string diff --git a/internal/compose/model_test.go b/internal/compose/model_test.go index 160f5d5..04a4440 100644 --- a/internal/compose/model_test.go +++ b/internal/compose/model_test.go @@ -189,6 +189,89 @@ services: } } +// TestLongFormPortsAndVolumes reproduces the bug where `ports:`/`volumes:` in +// the Compose long (mapping) form failed to unmarshal into []string, hard- +// erroring the entire compose file. They must parse and flatten to the same +// short-form strings the translator emits, alongside short-form entries. +func TestLongFormPortsAndVolumes(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "compose.yaml") + yaml := ` +services: + web: + image: nginx + ports: + - "8080:80" + - target: 443 + published: "8443" + protocol: tcp + - 9000 + volumes: + - ./short:/short + - type: bind + source: ./data + target: /data + read_only: true +` + if err := os.WriteFile(path, []byte(yaml), 0o644); err != nil { + t.Fatal(err) + } + p, err := Load(path, "") + if err != nil { + t.Fatalf("Load must not fail on long-form ports/volumes: %v", err) + } + web := p.Services["web"] + wantPorts := []string{"8080:80", "8443:443/tcp", "9000"} + if len(web.Ports) != len(wantPorts) { + t.Fatalf("ports = %v, want %v", web.Ports, wantPorts) + } + for i, w := range wantPorts { + if web.Ports[i] != w { + t.Errorf("ports[%d] = %q, want %q", i, web.Ports[i], w) + } + } + wantVols := []string{"./short:/short", "./data:/data:ro"} + if len(web.Volumes) != len(wantVols) { + t.Fatalf("volumes = %v, want %v", web.Volumes, wantVols) + } + for i, w := range wantVols { + if web.Volumes[i] != w { + t.Errorf("volumes[%d] = %q, want %q", i, web.Volumes[i], w) + } + } +} + +// TestMapListSkipsEmptyKeys reproduces the bug where a blank ("") or keyless +// ("=value") environment/labels list entry produced an empty-key map entry, +// later emitted as a malformed `--env =…` arg. Such entries must be dropped. +func TestMapListSkipsEmptyKeys(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "compose.yaml") + yaml := ` +services: + web: + image: nginx + environment: + - "GOOD=1" + - "" + - "=orphan" +` + if err := os.WriteFile(path, []byte(yaml), 0o644); err != nil { + t.Fatal(err) + } + p, err := Load(path, "") + if err != nil { + t.Fatal(err) + } + env := p.Services["web"].Environment + if _, bad := env[""]; bad { + t.Errorf("empty-key env entry leaked: %v", env) + } + if env["GOOD"] != "1" || len(env) != 1 { + t.Errorf("environment = %v, want only GOOD=1", env) + } +} + func TestServiceEnabledProfiles(t *testing.T) { noProf := &Service{} if !noProf.Enabled(map[string]bool{}) { From e8296d9426fec22784499742658e204b29713ca3 Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 10:20:56 +0530 Subject: [PATCH 03/17] fix(pool): reap stale warm VMs atomically to avoid destroying a claimed one ReapStale enumerated members with List(), then for each stale one called forget() and DestroyAsync() as separate steps. A concurrent Claim could pop a member in that window; the reaper would then DestroyAsync the very VM the live run was about to exec into. (It self-healed via the cold-path fallback, but wasted the warm slot.) Pop stale members inside the state lock via a pure partitionStale helper, then destroy them after releasing it. A concurrent Claim and a reap can now never both own the same member. --- internal/pool/pool.go | 26 +++++++++++++++++++++++--- internal/pool/pool_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/internal/pool/pool.go b/internal/pool/pool.go index 0060b22..53a367b 100644 --- a/internal/pool/pool.go +++ b/internal/pool/pool.go @@ -232,12 +232,32 @@ func ReapStale() { return } cutoff := time.Now().Unix() - int64(ttl.Seconds()) - for _, m := range List() { + // Pop the stale members atomically under the lock before destroying them. + // Removing-then-destroying as two steps (List → forget → Destroy) raced with + // Claim: a run could claim a member after we listed it, then we'd destroy the + // VM out from under the live exec. Doing the removal inside the lock means a + // concurrent Claim and a reap can never both own the same member. + var stale []Member + _ = withLock(func(s *state) error { + stale, s.Members = partitionStale(s.Members, cutoff) + return nil + }) + for _, m := range stale { + DestroyAsync(m.ID) + } +} + +// partitionStale splits members into those booted before cutoff (stale) and the +// rest (kept), preserving order. Pure so the reap policy is unit-testable. +func partitionStale(members []Member, cutoff int64) (stale, kept []Member) { + for _, m := range members { if m.BootedAt < cutoff { - forget(m.ID) - DestroyAsync(m.ID) + stale = append(stale, m) + } else { + kept = append(kept, m) } } + return stale, kept } // AutoEnabled reports whether dcon should self-prime the pool after eligible diff --git a/internal/pool/pool_test.go b/internal/pool/pool_test.go index c34649d..b4e6abb 100644 --- a/internal/pool/pool_test.go +++ b/internal/pool/pool_test.go @@ -25,6 +25,31 @@ func TestNormalizeRef(t *testing.T) { } } +// TestPartitionStale verifies the reap policy splits members by boot time. The +// real fix it guards is atomicity: ReapStale now removes stale members inside +// the state lock (via this helper) instead of List→forget→Destroy, so a +// concurrent Claim and a reap can never both own — and the reaper can never +// destroy — a VM a live run just claimed. +func TestPartitionStale(t *testing.T) { + members := []Member{ + {ID: "old1", BootedAt: 100}, + {ID: "fresh", BootedAt: 1000}, + {ID: "old2", BootedAt: 200}, + } + stale, kept := partitionStale(members, 500) + if len(stale) != 2 || stale[0].ID != "old1" || stale[1].ID != "old2" { + t.Errorf("stale = %v, want [old1 old2]", stale) + } + if len(kept) != 1 || kept[0].ID != "fresh" { + t.Errorf("kept = %v, want [fresh]", kept) + } + // Nothing stale: all kept, none reaped. + stale, kept = partitionStale(members, 0) + if len(stale) != 0 || len(kept) != 3 { + t.Errorf("cutoff 0: stale=%v kept=%v, want none stale", stale, kept) + } +} + func TestEnvKnobs(t *testing.T) { // AutoEnabled for _, v := range []string{"auto", "1", "on", "true", "yes", "AUTO", " on "} { From 50e0df42a7f0d2fe4e17fdd86ca593fc2d6d86d6 Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 10:27:10 +0530 Subject: [PATCH 04/17] fix: stop --mount panic, --filter comma-corruption, ps -n 0, images ref filters - run: a valueless '--mount type=tmpfs,...,tmpfs-size' (no =value) sliced past the string end and panicked the process; pass such fields through. - ps/images/volume/network: --filter was StringSlice, which comma-splits a single value, so 'label=team=a,b' became two bogus filters and matched the wrong set. Use StringArray (the project's comma-bearing-flag rule). - ps: 'ps -n 0' listed every running container instead of none; trimLast now distinguishes the unset sentinel (-1) from an explicit 0. - images: a registry-port ref (registry:5000/img) was misread as tag ':latest' and hid all other tags; a digest ref (img@sha256:..) compared the digest against the tag column and always returned empty. Parse via imageRefFilter and match digests against the digest column. - images: a malformed reference= glob silently hid every image; validate the pattern up front and error like docker. All covered by reproducing tests. --- cmd/filters_test.go | 120 ++++++++++++++++++++++++++++++++++++++++++ cmd/images.go | 53 ++++++++++++++++--- cmd/network.go | 5 +- cmd/ps.go | 27 ++++++++-- cmd/run.go | 18 +++++-- cmd/translate_test.go | 20 +++++++ cmd/volume.go | 5 +- 7 files changed, 227 insertions(+), 21 deletions(-) create mode 100644 cmd/filters_test.go diff --git a/cmd/filters_test.go b/cmd/filters_test.go new file mode 100644 index 0000000..e42d26d --- /dev/null +++ b/cmd/filters_test.go @@ -0,0 +1,120 @@ +package cmd + +import ( + "testing" + + "dcon/internal/dockerfmt" + + "github.com/spf13/cobra" +) + +// subcmd finds a direct subcommand by name (e.g. the "ls" under a group). +func subcmd(t *testing.T, parent *cobra.Command, name string) *cobra.Command { + t.Helper() + for _, c := range parent.Commands() { + if c.Name() == name { + return c + } + } + t.Fatalf("subcommand %q not found under %q", name, parent.Name()) + return nil +} + +// TestFilterFlagsNotCommaSplit reproduces the StringSlice corruption: a single +// --filter value with a comma in a label value (e.g. label=team=a,b) was split +// into two bogus filters. The flag must be StringArray so each --filter is taken +// verbatim. Covers ps, images, and the volume/network ls commands. +func TestFilterFlagsNotCommaSplit(t *testing.T) { + const raw = "label=team=a,b" + check := func(t *testing.T, c *cobra.Command) { + if err := c.ParseFlags([]string{"--filter", raw}); err != nil { + t.Fatalf("ParseFlags: %v", err) + } + got, err := c.Flags().GetStringArray("filter") + if err != nil { + t.Fatalf("filter is not a StringArray flag: %v", err) + } + if len(got) != 1 || got[0] != raw { + t.Errorf("filter = %v, want exactly [%q] (comma must not split)", got, raw) + } + } + t.Run("ps", func(t *testing.T) { check(t, newPsCmd()) }) + t.Run("images", func(t *testing.T) { check(t, newImagesCmd()) }) + t.Run("volume", func(t *testing.T) { check(t, subcmd(t, newVolumeGroupCmd(), "ls")) }) + t.Run("network", func(t *testing.T) { check(t, subcmd(t, newNetworkGroupCmd(), "ls")) }) +} + +// TestApplyFiltersCommaLabelValue confirms the downstream predicate matches a +// label whose value contains a comma, once the flag is no longer split. +func TestApplyFiltersCommaLabelValue(t *testing.T) { + mk := func(id, team string) dockerfmt.Container { + var c dockerfmt.Container + c.ID = id + c.Configuration.Labels = map[string]string{"team": team} + return c + } + list := []dockerfmt.Container{mk("hit", "a,b"), mk("miss", "a")} + out := applyFilters(list, []string{"label=team=a,b"}) + if len(out) != 1 || out[0].ID != "hit" { + t.Errorf("applyFilters by comma label value = %v, want [hit]", out) + } +} + +// TestTrimLast covers docker's -n/--last semantics, including the regression +// where `ps -n 0` listed everything instead of showing none. +func TestTrimLast(t *testing.T) { + mk := func(n int) []dockerfmt.Container { + out := make([]dockerfmt.Container, n) + for i := range out { + out[i].ID = string(rune('a' + i)) + } + return out + } + list := mk(3) + if got := trimLast(list, -1); len(got) != 3 { + t.Errorf("last=-1 (unset) should keep all; got %d", len(got)) + } + if got := trimLast(list, 0); len(got) != 0 { + t.Errorf("last=0 should show none; got %d", len(got)) + } + if got := trimLast(list, 2); len(got) != 2 { + t.Errorf("last=2 should keep 2; got %d", len(got)) + } + if got := trimLast(list, 9); len(got) != 3 { + t.Errorf("last>len should keep all; got %d", len(got)) + } +} + +// TestImageRefFilter guards the positional `images REPO[:TAG|@DIGEST]` parsing: +// a registry-port colon must NOT be read as a tag (which hid all non-:latest +// tags), and a digest must filter the digest column, not the tag. +func TestImageRefFilter(t *testing.T) { + cases := []struct { + ref string + wantRepo, wantTag, wantDg string + }{ + {"alpine", "alpine", "", ""}, + {"alpine:3.18", "alpine", "3.18", ""}, + {"registry:5000/myimage", "registry:5000/myimage", "", ""}, // port, not a tag + {"registry:5000/myimage:1.0", "registry:5000/myimage", "1.0", ""}, // real tag kept + {"alpine@sha256:abc", "alpine", "", "sha256:abc"}, // digest, not tag + } + for _, c := range cases { + repo, tag, dg := imageRefFilter(c.ref) + if repo != c.wantRepo || tag != c.wantTag || dg != c.wantDg { + t.Errorf("imageRefFilter(%q) = (%q,%q,%q), want (%q,%q,%q)", + c.ref, repo, tag, dg, c.wantRepo, c.wantTag, c.wantDg) + } + } +} + +// TestValidateImageFilters confirms a malformed reference= glob errors loudly +// instead of silently hiding every image. +func TestValidateImageFilters(t *testing.T) { + if err := validateImageFilters([]string{"reference=ngin[x"}); err == nil { + t.Error("malformed reference pattern should error") + } + if err := validateImageFilters([]string{"reference=nginx*", "label=a=b"}); err != nil { + t.Errorf("valid filters should not error: %v", err) + } +} diff --git a/cmd/images.go b/cmd/images.go index c82d0c5..3040c04 100644 --- a/cmd/images.go +++ b/cmd/images.go @@ -98,7 +98,8 @@ func addImagesFlags(cmd *cobra.Command) { f.BoolP("quiet", "q", false, "Only show image IDs") f.Bool("no-trunc", false, "Don't truncate output") f.String("format", "", "Format output using a Go template or 'json'") - f.StringSliceP("filter", "f", nil, "Filter output based on conditions provided") + // StringArray, not StringSlice: a label-value filter may contain commas. + f.StringArrayP("filter", "f", nil, "Filter output based on conditions provided") f.Bool("digests", false, "Show digests") f.Bool("tree", false, "List multi-platform images as a tree (unsupported; falls back to flat list)") } @@ -108,23 +109,23 @@ func runImages(cmd *cobra.Command, args []string) error { noTrunc, _ := cmd.Flags().GetBool("no-trunc") format, _ := cmd.Flags().GetString("format") digests, _ := cmd.Flags().GetBool("digests") - filters, _ := cmd.Flags().GetStringSlice("filter") + filters, _ := cmd.Flags().GetStringArray("filter") if cmd.Flags().Changed("tree") { fmt.Fprintln(os.Stderr, "dcon: warning: --tree is not supported by the backend; showing a flat list") } + if err := validateImageFilters(filters); err != nil { + return err + } list, err := getImages() if err != nil { return err } - // positional REPO[:TAG] filter - var repoFilter, tagFilter string + // positional REPO[:TAG|@DIGEST] filter + var repoFilter, tagFilter, digestFilter string if len(args) == 1 { - repoFilter, tagFilter = dockerfmt.SplitRepoTag(args[0]) - if !strings.Contains(args[0], ":") { - tagFilter = "" - } + repoFilter, tagFilter, digestFilter = imageRefFilter(args[0]) } views := make([]any, 0, len(list)) @@ -136,6 +137,9 @@ func runImages(cmd *cobra.Command, args []string) error { if tagFilter != "" && v.Tag != tagFilter { continue } + if digestFilter != "" && !strings.HasPrefix(v.Digest, digestFilter) { + continue + } if !matchImageFilters(v, filters) { continue } @@ -165,6 +169,39 @@ func runImages(cmd *cobra.Command, args []string) error { return dockerfmt.Render(format, quiet, views, def) } +// imageRefFilter splits a positional `images REPO[:TAG|@DIGEST]` argument into +// repo, tag, and digest filters. A tag filter is set ONLY when the ref has an +// explicit tag — a colon after the last slash. A registry-port colon like +// registry:5000/img is not a tag, so it must not pin tag=latest and hide every +// other tag of that repo. A digest ref filters by the digest column, never by +// the human tag (which is never a digest), which otherwise yields an empty list. +func imageRefFilter(ref string) (repo, tag, digest string) { + if i := strings.LastIndex(ref, "@"); i >= 0 { + return ref[:i], "", ref[i+1:] + } + repo, t := dockerfmt.SplitRepoTag(ref) + if j := strings.LastIndex(ref, ":"); j >= 0 && !strings.Contains(ref[j:], "/") { + tag = t // a real tag separator (after the last slash) was present + } + return repo, tag, "" +} + +// validateImageFilters rejects malformed reference= glob patterns up front +// (mirroring docker, which errors on a bad filter) so a pattern like +// "ngin[x" surfaces an error instead of silently matching nothing and +// returning an empty image list. +func validateImageFilters(filters []string) error { + for _, fl := range filters { + kv := strings.SplitN(fl, "=", 2) + if len(kv) == 2 && kv[0] == "reference" { + if _, err := filepath.Match(kv[1], ""); err != nil { + return fmt.Errorf("invalid filter 'reference=%s': %v", kv[1], err) + } + } + } + return nil +} + func matchImageFilters(v imageView, filters []string) bool { for _, fl := range filters { kv := strings.SplitN(fl, "=", 2) diff --git a/cmd/network.go b/cmd/network.go index 59191b0..adcee9b 100644 --- a/cmd/network.go +++ b/cmd/network.go @@ -146,7 +146,7 @@ func newNetworkGroupCmd() *cobra.Command { quiet, _ := cmd.Flags().GetBool("quiet") noTrunc, _ := cmd.Flags().GetBool("no-trunc") format, _ := cmd.Flags().GetString("format") - filters, _ := cmd.Flags().GetStringSlice("filter") + filters, _ := cmd.Flags().GetStringArray("filter") var list []dockerfmt.Network if err := runtime.CaptureJSON(&list, "network", "list", "--format", "json"); err != nil { return err @@ -186,7 +186,8 @@ func newNetworkGroupCmd() *cobra.Command { ls.Flags().BoolP("quiet", "q", false, "Only display network IDs") ls.Flags().Bool("no-trunc", false, "Do not truncate the output") ls.Flags().String("format", "", "Format output using a Go template or 'json'") - ls.Flags().StringSliceP("filter", "f", nil, "Provide filter values") + // StringArray, not StringSlice: a label-value filter may contain commas. + ls.Flags().StringArrayP("filter", "f", nil, "Provide filter values") rm := &cobra.Command{ Use: "rm NETWORK [NETWORK...]", diff --git a/cmd/ps.go b/cmd/ps.go index ec7bf81..f746514 100644 --- a/cmd/ps.go +++ b/cmd/ps.go @@ -226,6 +226,23 @@ func applyFilters(list []dockerfmt.Container, filters []string) []dockerfmt.Cont return out } +// trimLast applies docker's -n/--last semantics: a negative value (the unset +// sentinel) leaves the list untouched; 0 shows nothing (header only); a +// positive n keeps the first n. Docker's `ps -n 0` shows zero containers — the +// old `last > 0` guard wrongly treated 0 like "unset" and listed everything. +func trimLast(list []dockerfmt.Container, last int) []dockerfmt.Container { + switch { + case last < 0: + return list + case last == 0: + return nil + case last < len(list): + return list[:last] + default: + return list + } +} + func newPsCmd() *cobra.Command { cmd := &cobra.Command{ Use: "ps [OPTIONS]", @@ -237,7 +254,7 @@ func newPsCmd() *cobra.Command { quiet, _ := cmd.Flags().GetBool("quiet") noTrunc, _ := cmd.Flags().GetBool("no-trunc") format, _ := cmd.Flags().GetString("format") - filters, _ := cmd.Flags().GetStringSlice("filter") + filters, _ := cmd.Flags().GetStringArray("filter") last, _ := cmd.Flags().GetInt("last") latest, _ := cmd.Flags().GetBool("latest") @@ -256,9 +273,7 @@ func newPsCmd() *cobra.Command { if latest { last = 1 } - if last > 0 && last < len(list) { - list = list[:last] - } + list = trimLast(list, last) views := make([]any, 0, len(list)) for _, c := range list { @@ -280,7 +295,9 @@ func newPsCmd() *cobra.Command { f.BoolP("quiet", "q", false, "Only display container IDs") f.Bool("no-trunc", false, "Don't truncate output") f.String("format", "", "Format output using a Go template or 'json'") - f.StringSliceP("filter", "f", nil, "Filter output based on conditions provided") + // StringArray (not StringSlice): a single --filter value legitimately + // contains commas (e.g. label=team=a,b), which StringSlice would split. + f.StringArrayP("filter", "f", nil, "Filter output based on conditions provided") f.IntP("last", "n", -1, "Show n last created containers (includes all states)") f.BoolP("latest", "l", false, "Show the latest created container") f.BoolP("size", "s", false, "Display total file sizes") diff --git a/cmd/run.go b/cmd/run.go index d576af6..4a121df 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -387,15 +387,25 @@ func normalizeMount(spec string, warnings *[]string) string { fields := strings.Split(spec, ",") var out []string for _, fld := range fields { - key := fld + key, val, hasEq := fld, "", false if i := strings.Index(fld, "="); i >= 0 { - key = fld[:i] + key, val, hasEq = fld[:i], fld[i+1:], true } switch key { case "tmpfs-size": - out = append(out, "size="+fld[len("tmpfs-size="):]) + // A valueless "tmpfs-size" (no '=') must not slice past the string; + // pass it through untouched rather than panicking. + if hasEq { + out = append(out, "size="+val) + } else { + out = append(out, fld) + } case "tmpfs-mode": - out = append(out, "mode="+fld[len("tmpfs-mode="):]) + if hasEq { + out = append(out, "mode="+val) + } else { + out = append(out, fld) + } case "volume-driver", "volume-opt", "bind-propagation", "consistency": *warnings = append(*warnings, fmt.Sprintf("--mount option %q is not supported by the container backend and was ignored", key)) default: diff --git a/cmd/translate_test.go b/cmd/translate_test.go index 9a8f777..e0c4234 100644 --- a/cmd/translate_test.go +++ b/cmd/translate_test.go @@ -150,6 +150,26 @@ func TestRunMountTmpfsKeysRewritten(t *testing.T) { } } +// TestRunMountValuelessTmpfsKeyNoPanic reproduces a slice-out-of-range panic: +// a --mount field of a bare "tmpfs-size"/"tmpfs-mode" (no =value) matched the +// rewrite case and sliced past the string end, crashing the process. It must +// now pass the field through untouched instead. +func TestRunMountValuelessTmpfsKeyNoPanic(t *testing.T) { + for _, spec := range []string{ + "type=tmpfs,destination=/x,tmpfs-size", + "type=tmpfs,destination=/x,tmpfs-mode", + } { + c := parse(t, newRunCmd(), []string{"--mount", spec, "alpine"}) + got, err := buildContainerArgs(c, c.Flags().Args(), "run") // must not panic + if err != nil { + t.Fatalf("%q: unexpected error %v", spec, err) + } + if !contains(got, "--mount") { + t.Errorf("%q: expected a --mount arg; got %v", spec, got) + } + } +} + func TestBuildTranslation(t *testing.T) { c := newBuildCmd() parse(t, c, []string{ diff --git a/cmd/volume.go b/cmd/volume.go index c242a55..393b9f2 100644 --- a/cmd/volume.go +++ b/cmd/volume.go @@ -107,7 +107,7 @@ func newVolumeGroupCmd() *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { quiet, _ := cmd.Flags().GetBool("quiet") format, _ := cmd.Flags().GetString("format") - filters, _ := cmd.Flags().GetStringSlice("filter") + filters, _ := cmd.Flags().GetStringArray("filter") var list []dockerfmt.Volume if err := runtime.CaptureJSON(&list, "volume", "list", "--format", "json"); err != nil { return err @@ -142,7 +142,8 @@ func newVolumeGroupCmd() *cobra.Command { } ls.Flags().BoolP("quiet", "q", false, "Only display volume names") ls.Flags().String("format", "", "Format output using a Go template or 'json'") - ls.Flags().StringSliceP("filter", "f", nil, "Provide filter values") + // StringArray, not StringSlice: a label-value filter may contain commas. + ls.Flags().StringArrayP("filter", "f", nil, "Provide filter values") rm := &cobra.Command{ Use: "rm [OPTIONS] VOLUME [VOLUME...]", From 4a4f86dae218cbb6aa8d538aeb84262d187e6e67 Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 10:38:56 +0530 Subject: [PATCH 05/17] fix: compose exec/run/create + top, cp, history, system prune correctness compose: - exec gated --interactive on stdin being a TTY, so 'compose exec -T db psql < dump.sql' dropped the redirected stdin. Forward --interactive on the flag value alone (TTY handled separately) via a pure composeExecArgs. - run --rm=false used a global token strip that removed EVERY '--rm', including one passed to the in-container command. Pass rm into OneOffArgs so the run-level flag is controlled at the source; delete dropFlag. - up/create's '--detach' strip had the same hazard for a service command containing a literal --detach; add a positional CreateArgs builder. - warn (don't silently drop) when multiple -f/--file files are given, since dcon does not merge them like docker. cli: - top: pass dashed ps options through (SetInterspersed false) so 'top web -ef' no longer dies on an unknown-flag parse error. - cp: a local path with a colon (./my:file.txt) was misread as a CONTAINER:PATH ref; classify like docker's splitCpArg. - history: truncate CREATED BY by runes, not bytes, so multibyte UTF-8 isn't split into an invalid sequence. - system df --verbose / prune --filter now warn instead of silently ignoring; system prune propagates backend errors instead of always exiting 0. All covered by reproducing tests. --- cmd/cli_fixes_test.go | 88 +++++++++++++++++++++++++++++ cmd/compose.go | 90 ++++++++++++++++-------------- cmd/compose_helpers_test.go | 46 +++++++++------ cmd/history.go | 20 +++++-- cmd/lifecycle.go | 8 ++- cmd/misc.go | 16 +++++- cmd/system.go | 52 ++++++++++++----- internal/compose/translate.go | 25 +++++++-- internal/compose/translate_test.go | 63 ++++++++++++++++++++- 9 files changed, 319 insertions(+), 89 deletions(-) create mode 100644 cmd/cli_fixes_test.go diff --git a/cmd/cli_fixes_test.go b/cmd/cli_fixes_test.go new file mode 100644 index 0000000..ff67d9a --- /dev/null +++ b/cmd/cli_fixes_test.go @@ -0,0 +1,88 @@ +package cmd + +import ( + "io" + "reflect" + "strings" + "testing" + "unicode/utf8" + + "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) + } + } +} + +// 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) + } +} diff --git a/cmd/compose.go b/cmd/compose.go index 937a2f5..0689396 100644 --- a/cmd/compose.go +++ b/cmd/compose.go @@ -98,6 +98,11 @@ func loadProject(cmd *cobra.Command) (*compose.Project, error) { if len(files) > 0 { explicit = files[0] } + if len(files) > 1 { + // Docker merges multiple -f files (later override earlier); dcon does not + // yet. Warn loudly rather than silently dropping the override files. + fmt.Fprintf(os.Stderr, "dcon: warning: multiple -f/--file compose files are not merged; using only %s\n", explicit) + } path, err := compose.Find(explicit) if err != nil { return nil, err @@ -315,8 +320,7 @@ func composeUp() *cobra.Command { continue } if noStart { - rargs := dropFlag(p.RunArgs(name, svc, i, net, nil), "--detach") - rargs[0] = "create" + rargs := p.CreateArgs(name, svc, i, net) fmt.Println(composeStep("Creating", cname)) if err := runtime.Run(rargs...); err != nil { return local, err @@ -1041,8 +1045,7 @@ func composeCreate() *cobra.Command { return err } } - rargs := dropFlag(p.RunArgs(name, svc, 1, net, nil), "--detach") - rargs[0] = "create" + rargs := p.CreateArgs(name, svc, 1, net) cname := p.ContainerName(name, 1, svc) if forceRecreate { _, _ = runtime.CaptureSilent("delete", "--force", cname) @@ -1084,34 +1087,17 @@ func composeExec() *cobra.Command { if err != nil { return err } - cargs := []string{"exec"} - if v, _ := cmd.Flags().GetBool("detach"); v { - cargs = append(cargs, "--detach") - } - if v, _ := cmd.Flags().GetBool("interactive"); v && isTerminal(os.Stdin) { - cargs = append(cargs, "--interactive") - } - // Only allocate a PTY when one actually exists (docker behaviour), - // so `compose exec svc cmd | grep ...` works. -T/--no-TTY forces it off - // (the common CI form: `compose exec -T db psql < dump.sql`). - noTTY, _ := cmd.Flags().GetBool("no-TTY") - if v, _ := cmd.Flags().GetBool("tty"); v && !noTTY && haveTTY() { - cargs = append(cargs, "--tty") - } if cmd.Flags().Changed("privileged") { fmt.Fprintln(os.Stderr, "dcon: warning: --privileged is not supported by exec and was ignored") } - if u, _ := cmd.Flags().GetString("user"); u != "" { - cargs = append(cargs, "--user", u) - } - if w, _ := cmd.Flags().GetString("workdir"); w != "" { - cargs = append(cargs, "--workdir", w) - } - for _, e := range mustStringArray(cmd.Flags(), "env") { - cargs = append(cargs, "--env", e) - } - cargs = append(cargs, c) - cargs = append(cargs, rest...) + detach, _ := cmd.Flags().GetBool("detach") + interactive, _ := cmd.Flags().GetBool("interactive") + tty, _ := cmd.Flags().GetBool("tty") + noTTY, _ := cmd.Flags().GetBool("no-TTY") + user, _ := cmd.Flags().GetString("user") + workdir, _ := cmd.Flags().GetString("workdir") + env := mustStringArray(cmd.Flags(), "env") + cargs := composeExecArgs(detach, interactive, tty, noTTY, haveTTY(), user, workdir, env, c, rest) return runtime.Run(cargs...) }, } @@ -1128,6 +1114,36 @@ func composeExec() *cobra.Command { return cmd } +// composeExecArgs builds the `container exec ...` argv for `compose exec`. +// --interactive follows the flag value alone (it defaults to true), NOT whether +// stdin is a terminal: piping stdin (`compose exec -T db psql < dump.sql`) is +// exactly the case that needs STDIN kept open, and gating it on a TTY dropped +// the redirected input. The PTY (--tty) is separate: only when requested, not +// suppressed by -T/--no-TTY, and a real terminal is present. +func composeExecArgs(detach, interactive, tty, noTTY, hasTTY bool, user, workdir string, env []string, containerID string, rest []string) []string { + cargs := []string{"exec"} + if detach { + cargs = append(cargs, "--detach") + } + if interactive { + cargs = append(cargs, "--interactive") + } + if tty && !noTTY && hasTTY { + cargs = append(cargs, "--tty") + } + if user != "" { + cargs = append(cargs, "--user", user) + } + if workdir != "" { + cargs = append(cargs, "--workdir", workdir) + } + for _, e := range env { + cargs = append(cargs, "--env", e) + } + cargs = append(cargs, containerID) + return append(cargs, rest...) +} + // serviceContainerByIndex resolves a project service's container by replica // index (1-based), preferring a running one. func serviceContainerByIndex(project, service string, idx int) (string, error) { @@ -1204,10 +1220,8 @@ func composeRun() *cobra.Command { } entrypoint, _ := cmd.Flags().GetString("entrypoint") - run := p.OneOffArgs(svcName, svc, net, args[1:], overrides, entrypoint) - if rm, _ := cmd.Flags().GetBool("rm"); !rm { - run = dropFlag(run, "--rm") - } + rm, _ := cmd.Flags().GetBool("rm") + run := p.OneOffArgs(svcName, svc, net, args[1:], overrides, entrypoint, rm) if d, _ := cmd.Flags().GetBool("detach"); d { run = append([]string{run[0], "--detach"}, run[1:]...) } @@ -1656,13 +1670,3 @@ func firstServiceContainer(project, service string) (string, error) { return "", fmt.Errorf("no running container for service %q", service) } -func dropFlag(args []string, flag string) []string { - out := args[:0:0] - for _, a := range args { - if a == flag { - continue - } - out = append(out, a) - } - return out -} diff --git a/cmd/compose_helpers_test.go b/cmd/compose_helpers_test.go index e91b8df..15323b5 100644 --- a/cmd/compose_helpers_test.go +++ b/cmd/compose_helpers_test.go @@ -9,6 +9,35 @@ import ( "github.com/spf13/cobra" ) +// TestComposeExecArgsInteractiveWithoutTTY reproduces the bug where `compose +// exec -T svc cmd < file` (no TTY) dropped --interactive, so redirected stdin +// never reached the process. --interactive must follow the flag, not the TTY. +func TestComposeExecArgsInteractiveWithoutTTY(t *testing.T) { + // -T form with no terminal: interactive=true, tty=true, noTTY=true, hasTTY=false. + got := composeExecArgs(false, true, true, true, false, "", "", nil, "cid", []string{"psql"}) + if !contains(got, "--interactive") { + t.Errorf("piped `exec -T` must keep --interactive (for `< file`): %v", got) + } + if contains(got, "--tty") { + t.Errorf("--tty must not be set when -T/no TTY: %v", got) + } + want := []string{"exec", "--interactive", "cid", "psql"} + if !reflect.DeepEqual(got, want) { + t.Errorf("exec args = %v, want %v", got, want) + } +} + +// TestComposeExecArgsTTYGating confirms --tty is added only when requested, not +// suppressed by -T, and a real terminal is present. +func TestComposeExecArgsTTYGating(t *testing.T) { + if got := composeExecArgs(false, true, true, false, true, "", "", nil, "cid", []string{"sh"}); !contains(got, "--tty") { + t.Errorf("tty && !noTTY && hasTTY should set --tty: %v", got) + } + if got := composeExecArgs(false, true, true, false, false, "", "", nil, "cid", []string{"sh"}); contains(got, "--tty") { + t.Errorf("no real TTY: --tty must be absent: %v", got) + } +} + func TestServiceSet(t *testing.T) { if got := serviceSet(nil); got != nil { t.Errorf("serviceSet(nil) = %v, want nil", got) @@ -20,23 +49,6 @@ func TestServiceSet(t *testing.T) { } } -func TestDropFlag(t *testing.T) { - in := []string{"run", "--detach", "--name", "x", "alpine"} - got := dropFlag(in, "--detach") - want := []string{"run", "--name", "x", "alpine"} - if !reflect.DeepEqual(got, want) { - t.Errorf("dropFlag = %v, want %v", got, want) - } - // Input must not be mutated (dropFlag allocates a fresh backing array). - if in[1] != "--detach" { - t.Errorf("dropFlag mutated its input: %v", in) - } - // Absent flag -> unchanged contents. - if got := dropFlag([]string{"a", "b"}, "--x"); !reflect.DeepEqual(got, []string{"a", "b"}) { - t.Errorf("dropFlag(absent) = %v, want [a b]", got) - } -} - func TestParallelLimit(t *testing.T) { cases := map[string]int{"": 8, "4": 4, "1": 1, "0": 0, "-3": 0, "bad": 8} for in, want := range cases { diff --git a/cmd/history.go b/cmd/history.go index 27f30d5..3dcbe64 100644 --- a/cmd/history.go +++ b/cmd/history.go @@ -10,6 +10,19 @@ import ( "github.com/spf13/cobra" ) +// formatCreatedBy renders a layer's created_by for the CREATED BY column: +// strip docker's shell wrapper prefixes, then truncate to 45 columns. The +// truncation is by runes (not bytes) so a multibyte UTF-8 character landing on +// the cut boundary is never split into an invalid byte sequence. +func formatCreatedBy(raw string, noTrunc bool) string { + cb := strings.TrimPrefix(raw, "/bin/sh -c #(nop) ") + cb = strings.TrimSpace(strings.TrimPrefix(cb, "/bin/sh -c")) + if r := []rune(cb); !noTrunc && len(r) > 45 { + cb = string(r[:42]) + "..." + } + return cb +} + // ociHistory mirrors the OCI image-config history entries that container // embeds under variants[].config.history. type ociHistory struct { @@ -62,15 +75,10 @@ func newHistoryCmd() *cobra.Command { // Docker lists newest layer first. for i := len(hist) - 1; i >= 0; i-- { h := hist[i] - cb := strings.TrimPrefix(h.CreatedBy, "/bin/sh -c #(nop) ") - cb = strings.TrimSpace(strings.TrimPrefix(cb, "/bin/sh -c")) - if !noTrunc && len(cb) > 45 { - cb = cb[:42] + "..." - } views = append(views, historyView{ ID: "", CreatedSince: dockerfmt.RelativeAgo(h.Created), - CreatedBy: cb, + CreatedBy: formatCreatedBy(h.CreatedBy, noTrunc), // Per-layer size is not present in the OCI config and is // unrecoverable from the backend; use a non-numeric sentinel. Size: "unknown", diff --git a/cmd/lifecycle.go b/cmd/lifecycle.go index af443ad..0b41238 100644 --- a/cmd/lifecycle.go +++ b/cmd/lifecycle.go @@ -219,7 +219,7 @@ func newRenameCmd() *cobra.Command { } func newTopCmd() *cobra.Command { - return &cobra.Command{ + cmd := &cobra.Command{ Use: "top CONTAINER [ps OPTIONS]", Short: "Display the running processes of a container", Args: cobra.MinimumNArgs(1), @@ -233,6 +233,12 @@ func newTopCmd() *cobra.Command { return runtime.Run(cargs...) }, } + // `top` is a pure pass-through of [ps OPTIONS] to ps inside the container. + // Stop flag parsing after the container name so dashed ps options + // (`top web -ef`, `top web -eo pid,comm`) reach ps instead of erroring as + // unknown flags on the top command itself. Mirrors exec/machine exec. + cmd.Flags().SetInterspersed(false) + return cmd } func newPortCmd() *cobra.Command { diff --git a/cmd/misc.go b/cmd/misc.go index b25ebf5..ec111f4 100644 --- a/cmd/misc.go +++ b/cmd/misc.go @@ -125,6 +125,19 @@ func inspectRaw(typ string, ids []string) (string, error) { } } +// cpIsContainerRef reports whether a `cp` argument is a CONTAINER:PATH +// reference rather than a local path, mirroring docker's splitCpArg: an +// absolute path (/...) or one whose part before the first colon starts with +// "." (./x:y, ../x:y) is a local path, even though it contains a colon. The old +// `strings.IndexByte(p,':')>0` test misclassified local paths like +// ./my:file.txt as a container ref and copied to the wrong place. +func cpIsContainerRef(p string) bool { + if strings.HasPrefix(p, "/") || strings.HasPrefix(p, ".") { + return false // absolute or explicitly-relative local path + } + return strings.IndexByte(p, ':') > 0 +} + func newCpCmd() *cobra.Command { cmd := &cobra.Command{ Use: "cp [OPTIONS] SRC_PATH|CONTAINER:SRC_PATH DEST_PATH|CONTAINER:DEST_PATH", @@ -135,8 +148,7 @@ func newCpCmd() *cobra.Command { if src == "-" || dst == "-" { return fmt.Errorf("streaming copy (-) is not supported by the backend; copy to/from a file path instead") } - isCtr := func(p string) bool { return strings.IndexByte(p, ':') > 0 } - if !isCtr(src) && !isCtr(dst) { + if !cpIsContainerRef(src) && !cpIsContainerRef(dst) { return fmt.Errorf("copying between two local paths is not supported; one of SRC or DEST must be CONTAINER:PATH") } for _, flag := range []string{"archive", "follow-link"} { diff --git a/cmd/system.go b/cmd/system.go index 61e82a0..1df5834 100644 --- a/cmd/system.go +++ b/cmd/system.go @@ -2,6 +2,7 @@ package cmd import ( "encoding/json" + "errors" "fmt" "os" "regexp" @@ -16,6 +17,31 @@ import ( "github.com/spf13/cobra" ) +// pruneStep is one backend prune invocation with its progress message. +type pruneStep struct { + msg string + args []string +} + +// systemPrunePlan is the ordered set of backend prune calls `system prune` +// makes, varying with --all (images) and --volumes. Pure, so the plan is +// unit-testable without a backend. +func systemPrunePlan(all, volumes bool) []pruneStep { + imageArgs := []string{"image", "prune"} + if all { + imageArgs = append(imageArgs, "--all") + } + steps := []pruneStep{ + {"Deleting stopped containers...", []string{"prune"}}, + {"Deleting unused images...", imageArgs}, + {"Deleting unused networks...", []string{"network", "prune"}}, + } + if volumes { + steps = append(steps, pruneStep{"Deleting unused volumes...", []string{"volume", "prune"}}) + } + return steps +} + var verRe = regexp.MustCompile(`version\s+([0-9][^\s]*)\s*\(build:\s*([^,]+),\s*commit:\s*([^)]+)\)`) type versionComponent struct { @@ -219,6 +245,9 @@ func newSystemGroupCmd() *cobra.Command { Short: "Show docker disk usage", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { + if cmd.Flags().Changed("verbose") { + fmt.Fprintln(os.Stderr, "dcon: warning: --verbose is not supported by the backend and was ignored") + } cargs := []string{"system", "df"} if f, _ := cmd.Flags().GetString("format"); f != "" { cargs = append(cargs, "--format", f) @@ -236,21 +265,18 @@ func newSystemGroupCmd() *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { all, _ := cmd.Flags().GetBool("all") volumes, _ := cmd.Flags().GetBool("volumes") - fmt.Println("Deleting stopped containers...") - _ = rt.Run("prune") - fmt.Println("Deleting unused images...") - if all { - _ = rt.Run("image", "prune", "--all") - } else { - _ = rt.Run("image", "prune") + if cmd.Flags().Changed("filter") { + fmt.Fprintln(os.Stderr, "dcon: warning: --filter is not supported by the backend and was ignored") } - fmt.Println("Deleting unused networks...") - _ = rt.Run("network", "prune") - if volumes { - fmt.Println("Deleting unused volumes...") - _ = rt.Run("volume", "prune") + // Run every step but propagate failures: previously all errors were + // discarded and the command always exited 0, so a wholly failed prune + // (e.g. backend down) reported success and misled scripts/CI. + var errs []error + for _, step := range systemPrunePlan(all, volumes) { + fmt.Println(step.msg) + errs = append(errs, rt.Run(step.args...)) } - return nil + return errors.Join(errs...) }, } prune.Flags().BoolP("all", "a", false, "Remove all unused images, not just dangling ones") diff --git a/internal/compose/translate.go b/internal/compose/translate.go index 0d13bc6..7521f42 100644 --- a/internal/compose/translate.go +++ b/internal/compose/translate.go @@ -236,19 +236,24 @@ func (p *Project) ImageRef(service string, svc *Service) string { return p.imageRef(service, svc) } -// OneOffArgs builds `container run --rm ...` for a `compose run` invocation: +// OneOffArgs builds `container run [--rm] ...` for a `compose run` invocation: // the service config without the fixed name/detach, plus CLI override flag // tokens (overrides, e.g. ["--env","K=V","--volume","a:b"]) injected before the // image, an optional entrypoint override (replacing the service's), and an // optional command override. The image boundary is located positionally, never // by string matching, so flag/command values equal to the image reference -// cannot misplace the override. -func (p *Project) OneOffArgs(service string, svc *Service, netName string, cmdOverride, overrides []string, entrypoint string) []string { +// cannot misplace the override. rm controls --rm directly: it is added (or not) +// here rather than being stripped afterward, so a literal "--rm" in the user's +// command args is never removed. +func (p *Project) OneOffArgs(service string, svc *Service, netName string, cmdOverride, overrides []string, entrypoint string, rm bool) []string { base, imageIdx := p.runArgs(service, svc, 1, netName, nil) flags := base[1:imageIdx] // the run flags span (after "run", before image) image := base[imageIdx] - out := []string{"run", "--rm"} + out := []string{"run"} + if rm { + out = append(out, "--rm") + } for i := 0; i < len(flags); i++ { switch flags[i] { case "--detach": @@ -275,6 +280,18 @@ func (p *Project) OneOffArgs(service string, svc *Service, netName string, cmdOv return append(out, base[imageIdx+1:]...) // keep the service's own command } +// CreateArgs builds the `container create ...` args for a service: the same +// configuration RunArgs produces but as a non-detached create. It drops the +// leading "--detach" positionally (runArgs always emits it at index 1), so a +// service command/entrypoint that itself contains a literal "--detach" token is +// never corrupted — unlike a blind token strip. +func (p *Project) CreateArgs(service string, svc *Service, index int, netName string) []string { + base, _ := p.runArgs(service, svc, index, netName, nil) + // base = ["run", "--detach", ...flags..., image, ...command...] + out := append([]string{"create"}, base[2:]...) + return out +} + // BuildImageName is the local tag used for services built from a Dockerfile. func (p *Project) BuildImageName(service string) string { return fmt.Sprintf("%s-%s:latest", p.Name, service) diff --git a/internal/compose/translate_test.go b/internal/compose/translate_test.go index 1971142..ea4c067 100644 --- a/internal/compose/translate_test.go +++ b/internal/compose/translate_test.go @@ -161,7 +161,7 @@ func indexOf(args []string, want string) int { func TestOneOffArgsEnvAndOverride(t *testing.T) { p := &Project{Name: "proj", Dir: "/tmp"} svc := &Service{Image: "nginx", Command: StringList{"orig"}} - got := p.OneOffArgs("web", svc, "proj_default", []string{"echo", "hi"}, []string{"--env", "FOO=bar"}, "") + got := p.OneOffArgs("web", svc, "proj_default", []string{"echo", "hi"}, []string{"--env", "FOO=bar"}, "", true) mustContain(t, got, "--rm") mustContainPair(t, got, "--env", "FOO=bar") // --name and --detach must be dropped for one-off @@ -183,7 +183,7 @@ func TestOneOffArgsImageEqualFlagValue(t *testing.T) { // regression: a flag value equal to the image ref must not break splitting p := &Project{Name: "proj", Dir: "/tmp"} svc := &Service{Image: "myimg", Platform: "myimg"} // platform value == image - got := p.OneOffArgs("web", svc, "", []string{"run-cmd"}, nil, "") + got := p.OneOffArgs("web", svc, "", []string{"run-cmd"}, nil, "", true) img := indexOf(got, "myimg") // the image is the LAST occurrence (platform value is earlier as a flag arg) last := -1 @@ -203,7 +203,7 @@ func TestOneOffArgsImageEqualFlagValue(t *testing.T) { func TestOneOffArgsOverridesEntrypoint(t *testing.T) { p := &Project{Name: "proj", Dir: "/tmp"} svc := &Service{Image: "nginx", Entrypoint: StringList{"/svc-ep"}} - got := p.OneOffArgs("web", svc, "", nil, []string{"--volume", "/a:/b"}, "/override-ep") + got := p.OneOffArgs("web", svc, "", nil, []string{"--volume", "/a:/b"}, "/override-ep", true) // CLI entrypoint replaces the service entrypoint (no duplicate, no /svc-ep) if indexOf(got, "/svc-ep") != -1 { t.Errorf("service entrypoint should be replaced: %v", got) @@ -216,6 +216,63 @@ func TestOneOffArgsOverridesEntrypoint(t *testing.T) { } } +// TestOneOffArgsRmFalsePreservesCommandRm reproduces the bug where, with +// rm=false, a global token strip removed EVERY "--rm" — including one the user +// passed as an argument to the in-container command (compose run --rm=false web +// mytool --rm). With rm=false the run-level --rm must be absent, but the +// command's own --rm must survive. +func TestOneOffArgsRmFalsePreservesCommandRm(t *testing.T) { + p := &Project{Name: "proj", Dir: "/tmp"} + svc := &Service{Image: "nginx"} + got := p.OneOffArgs("web", svc, "", []string{"mytool", "--rm"}, nil, "", false) + img := indexOf(got, "nginx") + if img < 0 { + t.Fatalf("image not found: %v", got) + } + // No run-level --rm before the image. + for i := 0; i < img; i++ { + if got[i] == "--rm" { + t.Errorf("rm=false must not emit a run-level --rm: %v", got) + } + } + // The command's own --rm (after the image) must be preserved. + foundCmdRm := false + for i := img + 1; i < len(got); i++ { + if got[i] == "--rm" { + foundCmdRm = true + } + } + if !foundCmdRm { + t.Errorf("the command's own --rm arg was stripped: %v", got) + } +} + +// TestCreateArgsPreservesDetachInCommand verifies CreateArgs drops only the +// leading run-level --detach (positionally), not a literal "--detach" the +// service's own command contains. +func TestCreateArgsPreservesDetachInCommand(t *testing.T) { + p := &Project{Name: "proj", Dir: "/tmp"} + svc := &Service{Image: "agent", Command: StringList{"serve", "--detach"}} + got := p.CreateArgs("web", svc, 1, "proj_default") + if len(got) == 0 || got[0] != "create" { + t.Fatalf("CreateArgs must start with create: %v", got) + } + img := indexOf(got, "agent") + if img < 0 { + t.Fatalf("image not found: %v", got) + } + // No run-level --detach before the image. + for i := 0; i < img; i++ { + if got[i] == "--detach" { + t.Errorf("CreateArgs must not emit a run-level --detach: %v", got) + } + } + // The command's own --detach (after the image) must survive. + if indexOf(got[img+1:], "--detach") < 0 { + t.Errorf("command's --detach was stripped: %v", got) + } +} + func TestRunArgsEntrypointAndCommandOrder(t *testing.T) { p := &Project{Name: "p", Dir: "/tmp"} svc := &Service{Image: "img", Entrypoint: StringList{"/ep", "--flag"}, Command: StringList{"arg1"}} From 9ed82afc964a27de3e41e5cdef8d18033876df43 Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 10:42:36 +0530 Subject: [PATCH 06/17] fix: build --output name=, restart --signal, mixed inspect ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - build: '--output type=docker|image,name=X' with no -t silently dropped the name, yielding an untagged image. A dest-less type=docker/image is buildx's local-store load (the long form of --load) — the backend default — so omit --output and carry name= through as --tag. With dest it stays a file export (type=oci). translateOutput now returns (output, tag, err). - restart: the --signal flag was defined but never used; forward it to the stop phase (the backend stop accepts --signal), matching docker. - inspect: a mixed 'inspect ' failed because each namespace was tried as one batch. Fall back to per-id resolution and merge the JSON arrays so both print, like docker. All covered by reproducing tests. --- cmd/build.go | 56 +++++++++++++++++++++++++++--------- cmd/cli_fixes_test.go | 37 ++++++++++++++++++++++++ cmd/lifecycle.go | 25 +++++++++++----- cmd/misc.go | 66 ++++++++++++++++++++++++++++++++++++++----- cmd/translate_test.go | 47 +++++++++++++++++++++++------- 5 files changed, 193 insertions(+), 38 deletions(-) diff --git a/cmd/build.go b/cmd/build.go index b17d73a..ad9f843 100644 --- a/cmd/build.go +++ b/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,12 +116,20 @@ 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. diff --git a/cmd/cli_fixes_test.go b/cmd/cli_fixes_test.go index ff67d9a..a83524c 100644 --- a/cmd/cli_fixes_test.go +++ b/cmd/cli_fixes_test.go @@ -1,6 +1,7 @@ package cmd import ( + "encoding/json" "io" "reflect" "strings" @@ -64,6 +65,42 @@ func TestCpIsContainerRef(t *testing.T) { } } +// 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") + } +} + // 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). diff --git a/cmd/lifecycle.go b/cmd/lifecycle.go index 0b41238..9b5503f 100644 --- a/cmd/lifecycle.go +++ b/cmd/lifecycle.go @@ -79,15 +79,12 @@ func newRestartCmd() *cobra.Command { Short: "Restart one or more containers", Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - tflag := []string{} - if cmd.Flags().Changed("time") { - t, _ := cmd.Flags().GetInt("time") - tflag = []string{"--time", fmt.Sprint(t)} - } + tv, _ := cmd.Flags().GetInt("time") + sig, _ := cmd.Flags().GetString("signal") + stopFlags := restartStopArgs(cmd.Flags().Changed("time"), tv, sig) var firstErr error for _, id := range args { - stopArgs := append([]string{"stop"}, tflag...) - stopArgs = append(stopArgs, id) + stopArgs := append(append([]string{}, stopFlags...), id) // Best-effort stop (suppress its id echo), then start (which // echoes the id once, matching docker restart). _, _ = runtime.CaptureSilent(stopArgs...) @@ -106,6 +103,20 @@ func newRestartCmd() *cobra.Command { return cmd } +// restartStopArgs builds the backend stop argv for `restart` (restart = stop + +// start). The --signal flag was previously defined but ignored; it is forwarded +// to the stop phase here (the backend stop accepts --signal), matching docker. +func restartStopArgs(timeChanged bool, timeVal int, signal string) []string { + args := []string{"stop"} + if timeChanged { + args = append(args, "--time", fmt.Sprint(timeVal)) + } + if signal != "" { + args = append(args, "--signal", signal) + } + return args +} + func newKillCmd() *cobra.Command { cmd := &cobra.Command{ Use: "kill [OPTIONS] CONTAINER [CONTAINER...]", diff --git a/cmd/misc.go b/cmd/misc.go index ec111f4..67d5f96 100644 --- a/cmd/misc.go +++ b/cmd/misc.go @@ -112,17 +112,69 @@ func inspectRaw(typ string, ids []string) (string, error) { case "container": return runtime.CaptureSilent(append([]string{"inspect"}, ids...)...) default: - out, err := runtime.CaptureSilent(append([]string{"inspect"}, ids...)...) - if err == nil { + // Fast path: all ids resolve as containers, or all as images — a single + // batch call preserves the backend's native formatting. + if out, err := runtime.CaptureSilent(append([]string{"inspect"}, ids...)...); err == nil { return out, nil } - // Fall back to image inspect. - out2, err2 := runtime.CaptureSilent(append([]string{"image", "inspect"}, ids...)...) - if err2 == nil { - return out2, nil + if out, err := runtime.CaptureSilent(append([]string{"image", "inspect"}, ids...)...); err == nil { + return out, nil + } + // Mixed (some containers, some images) or partly missing: resolve each id + // independently and merge, so `inspect ` works like + // docker instead of failing because no single namespace holds them all. + var results []string + var missing []string + for _, id := range ids { + out, err := runtime.CaptureSilent("inspect", id) + if err != nil { + out, err = runtime.CaptureSilent("image", "inspect", id) + } + if err != nil { + missing = append(missing, id) + continue + } + results = append(results, out) + } + merged, err := mergeInspectArrays(results) + if err != nil { + return "", err + } + if merged == "" { + return "", fmt.Errorf("no such object: %s", strings.Join(missing, " ")) + } + if len(missing) > 0 { + fmt.Fprintf(os.Stderr, "dcon: warning: no such object: %s\n", strings.Join(missing, " ")) } - return "", fmt.Errorf("no such object %v: %v; %v", ids, err, err2) + return merged, nil + } +} + +// mergeInspectArrays concatenates several `inspect` JSON-array outputs into a +// single pretty-printed JSON array, so a mixed container+image inspect prints +// one combined array (as docker does). Empty/blank outputs are skipped; an +// all-empty input yields "". +func mergeInspectArrays(outs []string) (string, error) { + var merged []json.RawMessage + for _, o := range outs { + o = strings.TrimSpace(o) + if o == "" { + continue + } + var items []json.RawMessage + if err := json.Unmarshal([]byte(o), &items); err != nil { + return "", err + } + merged = append(merged, items...) + } + if len(merged) == 0 { + return "", nil + } + b, err := json.MarshalIndent(merged, "", " ") + if err != nil { + return "", err } + return string(b), nil } // cpIsContainerRef reports whether a `cp` argument is a CONTAINER:PATH diff --git a/cmd/translate_test.go b/cmd/translate_test.go index e0c4234..d987061 100644 --- a/cmd/translate_test.go +++ b/cmd/translate_test.go @@ -201,29 +201,56 @@ func TestBuildTranslation(t *testing.T) { func TestTranslateOutput(t *testing.T) { cases := []struct { - in, want string - err bool + in, wantOut, wantTag string + err bool }{ - {"type=oci", "type=oci", false}, - {"type=local,dest=out", "type=local,dest=out", false}, - {"type=docker,dest=x.tar", "type=oci,dest=x.tar", false}, - {"type=image,name=foo", "type=oci", false}, - {"type=registry,ref=foo", "", true}, + {in: "type=oci", wantOut: "type=oci"}, + {in: "type=local,dest=out", wantOut: "type=local,dest=out"}, + {in: "type=docker,dest=x.tar", wantOut: "type=oci,dest=x.tar"}, + // no dest: omit --output (backend default is local-store load), carry name as tag + {in: "type=image,name=foo", wantOut: "", wantTag: "foo"}, + {in: "type=docker", wantOut: "", wantTag: ""}, + {in: "type=registry,ref=foo", err: true}, } for _, c := range cases { - got, err := translateOutput(c.in) + out, tag, err := translateOutput(c.in) if c.err { if err == nil { t.Errorf("translateOutput(%q) expected error", c.in) } continue } - if err != nil || got != c.want { - t.Errorf("translateOutput(%q) = (%q,%v); want %q", c.in, got, err, c.want) + if err != nil || out != c.wantOut || tag != c.wantTag { + t.Errorf("translateOutput(%q) = (out=%q,tag=%q,err=%v); want (out=%q,tag=%q)", + c.in, out, tag, err, c.wantOut, c.wantTag) } } } +// TestBuildOutputNamePreservedAsTag reproduces the bug where '--output +// type=docker,name=X' (no -t) silently dropped the name, producing an untagged +// image. The name must become a --tag, and no bogus --output should be emitted +// for the dest-less local-store load. +func TestBuildOutputNamePreservedAsTag(t *testing.T) { + c := parse(t, newBuildCmd(), []string{"--output", "type=docker,name=myrepo:1", "."}) + got, err := buildBuildArgs(c, c.Flags().Args()) + if err != nil { + t.Fatal(err) + } + if !containsPair(got, "--tag", "myrepo:1") { + t.Errorf("name= should become --tag myrepo:1; got %v", got) + } + if contains(got, "--output") { + t.Errorf("dest-less type=docker should omit --output (backend default load); got %v", got) + } + // With an explicit -t, the name= must not add a second tag. + c2 := parse(t, newBuildCmd(), []string{"-t", "explicit:9", "--output", "type=image,name=other", "."}) + got2, _ := buildBuildArgs(c2, c2.Flags().Args()) + if !containsPair(got2, "--tag", "explicit:9") || containsPair(got2, "--tag", "other") { + t.Errorf("explicit -t should win; name= must not add a tag; got %v", got2) + } +} + func TestExecTranslation(t *testing.T) { c := newExecCmd() parse(t, c, []string{"-it", "-u", "root", "-w", "/app", "-e", "K=V", "ct", "sh", "-c", "ls"}) From c869c573a94ca43fcd3731372652beef993e5e1c Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 10:46:14 +0530 Subject: [PATCH 07/17] fix(compose): tag built image with image: so --build output is actually used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A service with both build: and image: built an image tagged with the derived project name (project-service:latest) via BuildArgs, but the container was run as svc.Image (imageRef). So 'compose up --build' (and 'compose build') produced an image the run never referenced — the run used a stale/pulled svc.Image instead of the freshly built one. Tag the built image with imageRef (svc.Image when set, else the derived name), matching what RunArgs/OneOffArgs run. This also makes 'down --rmi local' consistent: a service with a custom image: keeps its tag (removed only by --rmi all), while build-only services still tag and remove the derived name. Covered by a reproducing test. --- internal/compose/model_test.go | 21 +++++++++++++++++++++ internal/compose/translate.go | 6 +++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/internal/compose/model_test.go b/internal/compose/model_test.go index 04a4440..bc938c0 100644 --- a/internal/compose/model_test.go +++ b/internal/compose/model_test.go @@ -136,6 +136,27 @@ func TestBuildArgs(t *testing.T) { } } +// TestBuildArgsTagsExplicitImage reproduces the bug where a service with BOTH +// build: and image: built an image tagged with the derived project name while +// the container ran svc.Image — so the freshly built image was never used. +// BuildArgs must tag the explicit image: so build output and run ref agree. +func TestBuildArgsTagsExplicitImage(t *testing.T) { + p := &Project{Name: "proj", Dir: "/tmp"} + svc := &Service{Image: "myorg/web:1.0", Build: Build{Context: "."}} + svc.Build.set = true + args := p.BuildArgs("web", svc) + if !containsSub(args, "myorg/web:1.0") { + t.Errorf("build must tag the explicit image: ref; got %v", args) + } + if containsSub(args, p.BuildImageName("web")) { + t.Errorf("build must NOT tag the derived name when image: is set; got %v", args) + } + // Sanity: build tag == run image ref. + if p.ImageRef("web", svc) != "myorg/web:1.0" { + t.Errorf("imageRef mismatch: %q", p.ImageRef("web", svc)) + } +} + func TestDefaultNetworkAndName(t *testing.T) { p := loadSample(t) if p.DefaultNetwork() != "shop_default" { diff --git a/internal/compose/translate.go b/internal/compose/translate.go index 7521f42..5891e5e 100644 --- a/internal/compose/translate.go +++ b/internal/compose/translate.go @@ -327,13 +327,17 @@ func normalizePort(spec string) string { } // BuildArgs returns the `container build ...` args for a service with build:. +// It tags the built image with imageRef — i.e. the service's explicit `image:` +// when set, otherwise the derived project image name. This must match what the +// container is run as (RunArgs/OneOffArgs also use imageRef): tagging only the +// derived name while running `image:` would build an image the run never uses. func (p *Project) BuildArgs(service string, svc *Service) []string { ctx := svc.Build.Context if ctx == "" { ctx = "." } ctx = p.resolve(ctx) - args := []string{"build", "--tag", p.BuildImageName(service)} + args := []string{"build", "--tag", p.imageRef(service, svc)} if svc.Build.Dockerfile != "" { args = append(args, "--file", filepath.Join(ctx, svc.Build.Dockerfile)) } From f642b99ed0d3614fc59f81a48dd51598090d318f Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 11:04:14 +0530 Subject: [PATCH 08/17] fix(compose/pool/cli): 10 issues from second adversarial red-team pass compose: - run now forwards --tty/--interactive (defaults: keep stdin open, allocate a TTY unless -T or no terminal); previously it read neither so 'compose run web bash' got no PTY. (composeRunPtyFlags) - OneOffArgs: overriding --entrypoint on a multi-token service entrypoint no longer leaks the old entrypoint's trailing args onto the new one. - one-off run containers are labeled oneoff=True and excluded by the service-replica resolvers, so 'compose exec/ps' no longer confuses a surviving 'run --rm=false' container with replica #1. - up --scale SERVICE=0 now runs zero replicas (explicit 0 was treated as unset and clamped to 1). (effectiveReplicas) - long-form port host_ip without published emits host_ip::target, not the malformed host_ip:target (which parsed as host_port:container). - bare environment/build-arg keys (- FOO / FOO:) inherit the host value (compose passthrough) instead of being forced to FOO="". (EnvMap) - correct the inaccurate Levels() cycle doc comment. cli: - port expands a published range (PublishPort.Count>1) to one line per port and resolves per-port filters into it. (portMappingLines) dockerfmt: - table header derivation handles function/pipeline-prefixed actions ({{upper .Name}}) instead of leaking the raw template into the header. pool: - Claim treats a state-write failure as a miss instead of returning ok on an unpersisted pop, preventing the same warm VM being handed out twice. All covered by reproducing tests. --- cmd/cli_fixes_test.go | 29 +++++++++++ cmd/compose.go | 58 ++++++++++++++++++++- cmd/compose_helpers_test.go | 39 ++++++++++++++ cmd/lifecycle.go | 60 ++++++++++++++------- internal/compose/model.go | 83 +++++++++++++++++++++++++----- internal/compose/model_test.go | 48 +++++++++++++++++ internal/compose/translate.go | 31 ++++++++--- internal/compose/translate_test.go | 75 ++++++++++++++++++++++++++- internal/dockerfmt/render.go | 28 +++++++--- internal/dockerfmt/render_test.go | 17 ++++++ internal/pool/pool.go | 11 +++- internal/pool/pool_test.go | 40 ++++++++++++++ 12 files changed, 466 insertions(+), 53 deletions(-) diff --git a/cmd/cli_fixes_test.go b/cmd/cli_fixes_test.go index a83524c..32195b3 100644 --- a/cmd/cli_fixes_test.go +++ b/cmd/cli_fixes_test.go @@ -8,6 +8,8 @@ import ( "testing" "unicode/utf8" + "dcon/internal/dockerfmt" + "github.com/spf13/cobra" ) @@ -65,6 +67,33 @@ func TestCpIsContainerRef(t *testing.T) { } } +// 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. diff --git a/cmd/compose.go b/cmd/compose.go index 0689396..3ff4ab2 100644 --- a/cmd/compose.go +++ b/cmd/compose.go @@ -305,7 +305,7 @@ func composeUp() *cobra.Command { return nil, fmt.Errorf("build %s: %w", name, err) } } - count := p.Replicas(svc, scale[name]) + count := effectiveReplicas(svc, scale, name) if count > 1 && svc.ContainerName != "" { return nil, fmt.Errorf("service %q has a container_name and cannot be scaled to %d", name, count) } @@ -1144,6 +1144,21 @@ func composeExecArgs(detach, interactive, tty, noTTY, hasTTY bool, user, workdir return append(cargs, rest...) } +// composeRunPtyFlags returns the --interactive/--tty tokens for `compose run`. +// Docker compose run keeps STDIN open (-i, default true) and allocates a TTY by +// default unless -T/--no-TTY; the PTY is only allocated when a real terminal is +// present (so `compose run web cmd | cat` and CI pipelines still work). +func composeRunPtyFlags(interactive, noTTY, hasTTY bool) []string { + var out []string + if interactive { + out = append(out, "--interactive") + } + if !noTTY && hasTTY { + out = append(out, "--tty") + } + return out +} + // serviceContainerByIndex resolves a project service's container by replica // index (1-based), preferring a running one. func serviceContainerByIndex(project, service string, idx int) (string, error) { @@ -1154,6 +1169,11 @@ func serviceContainerByIndex(project, service string, idx int) (string, error) { want := strconv.Itoa(idx) var fallback string for _, c := range containers { + // Never resolve a service replica to a one-off `compose run` container: + // it shares service/number labels but is a separate, ephemeral thing. + if c.Configuration.Labels[compose.LabelOneoff] == "True" { + continue + } if serviceOf(c) != service || c.Configuration.Labels[compose.LabelNumber] != want { continue } @@ -1220,9 +1240,20 @@ func composeRun() *cobra.Command { } entrypoint, _ := cmd.Flags().GetString("entrypoint") + detach, _ := cmd.Flags().GetBool("detach") + // Forward TTY/interactive to the backend (the run RunE previously read + // neither, so `compose run web bash` got no PTY). Docker compose run + // allocates a TTY by default unless -T, and keeps stdin open (-i). + // Skip when detached. + if !detach { + interactive, _ := cmd.Flags().GetBool("interactive") + noTTY, _ := cmd.Flags().GetBool("no-TTY") + overrides = append(overrides, composeRunPtyFlags(interactive, noTTY, haveTTY())...) + } + rm, _ := cmd.Flags().GetBool("rm") run := p.OneOffArgs(svcName, svc, net, args[1:], overrides, entrypoint, rm) - if d, _ := cmd.Flags().GetBool("detach"); d { + if detach { run = append([]string{run[0], "--detach"}, run[1:]...) } return runtime.Run(run...) @@ -1255,6 +1286,23 @@ func composeRun() *cobra.Command { return cmd } +// effectiveReplicas resolves how many replicas `up` should run for a service. +// An explicit --scale entry wins — INCLUDING 0 (docker's `up --scale web=0` +// runs zero). p.Replicas can't express this because it clamps a 0 override up +// to 1 (treating 0 as "unset"). A negative scale is floored to 0. +func effectiveReplicas(svc *compose.Service, scale map[string]int, name string) int { + if v, ok := scale[name]; ok { + if v < 0 { + v = 0 + } + return v + } + if svc.Scale < 1 { + return 1 // no --scale and no service-level scale: one replica + } + return svc.Scale +} + // parseScale turns ["web=3","db=2"] into {web:3, db:2}. func parseScale(specs []string) map[string]int { out := map[string]int{} @@ -1657,12 +1705,18 @@ func firstServiceContainer(project, service string) (string, error) { return "", err } for _, c := range containers { + if c.Configuration.Labels[compose.LabelOneoff] == "True" { + continue + } if serviceOf(c) == service && c.Status.State == "running" { return c.ID, nil } } // fall back to any state for _, c := range containers { + if c.Configuration.Labels[compose.LabelOneoff] == "True" { + continue + } if serviceOf(c) == service { return c.ID, nil } diff --git a/cmd/compose_helpers_test.go b/cmd/compose_helpers_test.go index 15323b5..68118a7 100644 --- a/cmd/compose_helpers_test.go +++ b/cmd/compose_helpers_test.go @@ -38,6 +38,45 @@ func TestComposeExecArgsTTYGating(t *testing.T) { } } +// TestComposeRunPtyFlags reproduces the bug where `compose run` never forwarded +// --tty/--interactive: a plain run keeps stdin open and allocates a TTY (unless +// -T or no terminal), matching docker compose run defaults. +func TestComposeRunPtyFlags(t *testing.T) { + // interactive=true, not -T, real terminal -> both. + if got := composeRunPtyFlags(true, false, true); !reflect.DeepEqual(got, []string{"--interactive", "--tty"}) { + t.Errorf("run pty flags = %v, want [--interactive --tty]", got) + } + // -T suppresses the PTY but keeps interactive. + if got := composeRunPtyFlags(true, true, true); !reflect.DeepEqual(got, []string{"--interactive"}) { + t.Errorf("-T should drop --tty: %v", got) + } + // No real terminal -> no PTY (so `compose run web cmd | cat` works). + if got := composeRunPtyFlags(true, false, false); !reflect.DeepEqual(got, []string{"--interactive"}) { + t.Errorf("no TTY: %v", got) + } +} + +// TestEffectiveReplicas reproduces the bug where `up --scale web=0` ran 1 +// replica: an explicit scale (including 0) must win over the unset default. +func TestEffectiveReplicas(t *testing.T) { + svc := &compose.Service{} + if n := effectiveReplicas(svc, map[string]int{"web": 0}, "web"); n != 0 { + t.Errorf("--scale web=0 should yield 0 replicas, got %d", n) + } + if n := effectiveReplicas(svc, map[string]int{"web": 3}, "web"); n != 3 { + t.Errorf("--scale web=3 should yield 3, got %d", n) + } + if n := effectiveReplicas(svc, nil, "web"); n != 1 { + t.Errorf("no scale, no service scale -> 1, got %d", n) + } + if n := effectiveReplicas(&compose.Service{Scale: 2}, nil, "web"); n != 2 { + t.Errorf("service-level scale 2 -> 2, got %d", n) + } + if n := effectiveReplicas(svc, map[string]int{"web": -5}, "web"); n != 0 { + t.Errorf("negative scale floored to 0, got %d", n) + } +} + func TestServiceSet(t *testing.T) { if got := serviceSet(nil); got != nil { t.Errorf("serviceSet(nil) = %v, want nil", got) diff --git a/cmd/lifecycle.go b/cmd/lifecycle.go index 9b5503f..6df8825 100644 --- a/cmd/lifecycle.go +++ b/cmd/lifecycle.go @@ -273,32 +273,52 @@ func newPortCmd() *cobra.Command { filterProto = parts[1] } } - for _, p := range list[0].Configuration.Ports { - proto := p.Proto - if proto == "" { - proto = "tcp" - } - if filterPort != "" && fmt.Sprint(p.ContainerPort) != filterPort { - continue - } - if filterProto != "" && proto != filterProto { - continue - } - host := p.HostAddress - if host == "" { - host = "0.0.0.0" - } - if filterPort != "" { - fmt.Printf("%s:%d\n", host, p.HostPort) - } else { - fmt.Printf("%d/%s -> %s:%d\n", p.ContainerPort, proto, host, p.HostPort) - } + for _, line := range portMappingLines(list[0].Configuration.Ports, filterPort, filterProto) { + fmt.Println(line) } return nil }, } } +// portMappingLines renders a container's published ports the way `docker port` +// does. A published range arrives from the backend as one PublishPort with +// Count>1; it is expanded to one line per port so every port of the range is +// listed and per-port filtering (`dcon port web 81`) resolves into the range. +func portMappingLines(ports []dockerfmt.PublishPort, filterPort, filterProto string) []string { + var out []string + for _, p := range ports { + proto := p.Proto + if proto == "" { + proto = "tcp" + } + host := p.HostAddress + if host == "" { + host = "0.0.0.0" + } + cnt := p.Count + if cnt < 1 { + cnt = 1 + } + for k := 0; k < cnt; k++ { + cport := p.ContainerPort + k + hport := p.HostPort + k + if filterPort != "" && fmt.Sprint(cport) != filterPort { + continue + } + if filterProto != "" && proto != filterProto { + continue + } + if filterPort != "" { + out = append(out, fmt.Sprintf("%s:%d", host, hport)) + } else { + out = append(out, fmt.Sprintf("%d/%s -> %s:%d", cport, proto, host, hport)) + } + } + } + return out +} + func newAttachCmd() *cobra.Command { return &cobra.Command{ Use: "attach CONTAINER", diff --git a/internal/compose/model.go b/internal/compose/model.go index 15067bb..3e7c7da 100644 --- a/internal/compose/model.go +++ b/internal/compose/model.go @@ -36,7 +36,7 @@ type Service struct { ContainerName string `yaml:"container_name"` Command StringList `yaml:"command"` Entrypoint StringList `yaml:"entrypoint"` - Environment MapList `yaml:"environment"` + Environment EnvMap `yaml:"environment"` EnvFile EnvFile `yaml:"env_file"` Ports PortList `yaml:"ports"` Expose []string `yaml:"expose"` @@ -139,10 +139,10 @@ func (u *Ulimits) UnmarshalYAML(value *yaml.Node) error { // Build is `build:` which may be a string (context) or a mapping. type Build struct { - Context string `yaml:"context"` - Dockerfile string `yaml:"dockerfile"` - Args MapList `yaml:"args"` - Target string `yaml:"target"` + Context string `yaml:"context"` + Dockerfile string `yaml:"dockerfile"` + Args EnvMap `yaml:"args"` + Target string `yaml:"target"` set bool } @@ -282,6 +282,53 @@ func (m *MapList) UnmarshalYAML(value *yaml.Node) error { return nil } +// EnvMap is like MapList but with docker-compose environment/build-arg +// passthrough semantics: a bare key — list form `- FOO` or map form `FOO:` +// (null value) — inherits FOO from the host environment, and is OMITTED when +// the host doesn't set it. (MapList, used for labels, instead sets bare keys to +// "" — and emitting `--env FOO=` would clobber a host FOO with empty.) +type EnvMap map[string]string + +func (m *EnvMap) UnmarshalYAML(value *yaml.Node) error { + out := map[string]string{} + switch value.Kind { + case yaml.MappingNode: + for i := 0; i+1 < len(value.Content); i += 2 { + k := value.Content[i].Value + vn := value.Content[i+1] + if k == "" { + continue + } + if vn.Kind == yaml.ScalarNode && vn.Tag == "!!null" { + if v, ok := os.LookupEnv(k); ok { // `FOO:` -> host passthrough + out[k] = v + } + continue + } + if vn.Kind == yaml.ScalarNode { + out[k] = vn.Value + } + } + case yaml.SequenceNode: + for _, it := range value.Content { + if it.Kind != yaml.ScalarNode { + continue + } + kv := strings.SplitN(it.Value, "=", 2) + if kv[0] == "" { + continue + } + if len(kv) == 2 { + out[kv[0]] = kv[1] + } else if v, ok := os.LookupEnv(kv[0]); ok { // `- FOO` -> host passthrough + out[kv[0]] = v + } + } + } + *m = out + return nil +} + // PortList handles compose `ports:` in both the short string form // ("8080:80", "127.0.0.1:8080:80/tcp", "3000") and the long mapping form // ({target, published, protocol, host_ip}), flattening the long form into the @@ -296,12 +343,22 @@ func (p *PortList) UnmarshalYAML(value *yaml.Node) error { } func flattenLongPort(m map[string]string) string { - s := m["target"] - if pub := m["published"]; pub != "" { - s = pub + ":" + s + target := m["target"] + pub := m["published"] + s := target + if pub != "" { + s = pub + ":" + target } if hip := m["host_ip"]; hip != "" { - s = hip + ":" + s + // A host_ip needs a published segment to be parsed as an IP (the 3-part + // host_ip:host:container form). With no published port, emit the explicit + // empty-published form host_ip::container — otherwise "host_ip:container" + // is misread as host_port:container_port (host port = the IP literal). + if pub != "" { + s = hip + ":" + s + } else { + s = hip + "::" + target + } } if proto := m["protocol"]; proto != "" { s += "/" + proto @@ -534,9 +591,11 @@ func (p *Project) Order() []string { // Levels groups services into dependency levels: every service in level i // depends only on services in earlier levels, so all services within a level // have no ordering constraint between them and can be brought up concurrently. -// Built on Order(), so undefined deps and cycles degrade identically (a cycle's -// members collapse toward level 0 rather than deadlocking). Returns nil for an -// empty project. +// Built on Order(), so undefined deps and cycles degrade safely without +// deadlocking: a cycle's members are still all emitted and started, but (because +// Order breaks the cycle's back-edge) they land on successive non-zero levels +// with an empty leading level rather than collapsing to level 0. The empty level +// is a harmless no-op in the up loop. Returns nil for an empty project. func (p *Project) Levels() [][]string { order := p.Order() if len(order) == 0 { diff --git a/internal/compose/model_test.go b/internal/compose/model_test.go index bc938c0..159c89f 100644 --- a/internal/compose/model_test.go +++ b/internal/compose/model_test.go @@ -293,6 +293,54 @@ services: } } +// TestEnvBareKeyHostPassthrough reproduces the bug where a bare environment key +// (`- FOO` list form or `FOO:` null map form) was set to "" instead of +// inheriting the host's $FOO (docker compose passthrough). An unset bare key is +// omitted, not emitted as FOO="". +func TestEnvBareKeyHostPassthrough(t *testing.T) { + t.Setenv("DCON_TEST_PASSTHRU", "from-host") + dir := t.TempDir() + path := filepath.Join(dir, "compose.yaml") + yaml := ` +services: + list: + image: x + environment: + - DCON_TEST_PASSTHRU + - EXPLICIT=val + - DCON_UNSET_XYZ + mapform: + image: x + environment: + DCON_TEST_PASSTHRU: + KEYED: v2 +` + if err := os.WriteFile(path, []byte(yaml), 0o644); err != nil { + t.Fatal(err) + } + p, err := Load(path, "") + if err != nil { + t.Fatal(err) + } + le := p.Services["list"].Environment + if le["DCON_TEST_PASSTHRU"] != "from-host" { + t.Errorf("list bare key should inherit host value; got %q", le["DCON_TEST_PASSTHRU"]) + } + if le["EXPLICIT"] != "val" { + t.Errorf("explicit value wrong: %q", le["EXPLICIT"]) + } + if _, present := le["DCON_UNSET_XYZ"]; present { + t.Errorf("unset bare key must be omitted, not empty: %v", le) + } + me := p.Services["mapform"].Environment + if me["DCON_TEST_PASSTHRU"] != "from-host" { + t.Errorf("map null-value key should inherit host value; got %q", me["DCON_TEST_PASSTHRU"]) + } + if me["KEYED"] != "v2" { + t.Errorf("map keyed value wrong: %q", me["KEYED"]) + } +} + func TestServiceEnabledProfiles(t *testing.T) { noProf := &Service{} if !noProf.Enabled(map[string]bool{}) { diff --git a/internal/compose/translate.go b/internal/compose/translate.go index 5891e5e..1b08a5e 100644 --- a/internal/compose/translate.go +++ b/internal/compose/translate.go @@ -84,23 +84,29 @@ func ShellSplit(s string) []string { // netName, when non-empty, is attached via --network. index is the replica // number (1-based). func (p *Project) RunArgs(service string, svc *Service, index int, netName string, extraEnv map[string]string) []string { - a, _ := p.runArgs(service, svc, index, netName, extraEnv) + a, _ := p.runArgs(service, svc, index, netName, extraEnv, false) return a } // runArgs builds the run args and returns the index of the image token, so // callers can split flags / image / command positionally instead of by fragile -// string matching. -func (p *Project) runArgs(service string, svc *Service, index int, netName string, extraEnv map[string]string) (args []string, imageIdx int) { +// string matching. oneoff stamps the compose oneoff label True/False so a +// `compose run` container is distinguishable from a service replica (and is +// excluded by the replica resolvers), matching Docker. +func (p *Project) runArgs(service string, svc *Service, index int, netName string, extraEnv map[string]string, oneoff bool) (args []string, imageIdx int) { args = []string{"run", "--detach"} args = append(args, "--name", p.ContainerName(service, index, svc)) + oneoffVal := "False" + if oneoff { + oneoffVal = "True" + } // compose identity labels args = append(args, "--label", LabelProject+"="+p.Name, "--label", LabelService+"="+service, "--label", fmt.Sprintf("%s=%d", LabelNumber, index), - "--label", LabelOneoff+"=False", + "--label", LabelOneoff+"="+oneoffVal, "--label", LabelConfigDir+"="+p.Dir, ) for _, k := range sortedKeys(svc.Labels) { @@ -246,7 +252,7 @@ func (p *Project) ImageRef(service string, svc *Service) string { // here rather than being stripped afterward, so a literal "--rm" in the user's // command args is never removed. func (p *Project) OneOffArgs(service string, svc *Service, netName string, cmdOverride, overrides []string, entrypoint string, rm bool) []string { - base, imageIdx := p.runArgs(service, svc, 1, netName, nil) + base, imageIdx := p.runArgs(service, svc, 1, netName, nil, true) // oneoff=true flags := base[1:imageIdx] // the run flags span (after "run", before image) image := base[imageIdx] @@ -277,7 +283,18 @@ func (p *Project) OneOffArgs(service string, svc *Service, netName string, cmdOv if len(cmdOverride) > 0 { return append(out, cmdOverride...) // override replaces the service command } - return append(out, base[imageIdx+1:]...) // keep the service's own command + // Keep the service's own command (the tokens after the image). runArgs places + // the service entrypoint's extra tokens (Entrypoint[1:]) there first; when the + // entrypoint is being overridden, those extras belong to the OLD entrypoint and + // must be dropped — overriding the entrypoint replaces it whole (Docker + // semantics), it must not leave the old entrypoint's args bound to the new one. + tail := base[imageIdx+1:] + if entrypoint != "" && len(svc.Entrypoint) > 1 { + if drop := len(svc.Entrypoint) - 1; drop <= len(tail) { + tail = tail[drop:] + } + } + return append(out, tail...) } // CreateArgs builds the `container create ...` args for a service: the same @@ -286,7 +303,7 @@ func (p *Project) OneOffArgs(service string, svc *Service, netName string, cmdOv // service command/entrypoint that itself contains a literal "--detach" token is // never corrupted — unlike a blind token strip. func (p *Project) CreateArgs(service string, svc *Service, index int, netName string) []string { - base, _ := p.runArgs(service, svc, index, netName, nil) + base, _ := p.runArgs(service, svc, index, netName, nil, false) // base = ["run", "--detach", ...flags..., image, ...command...] out := append([]string{"create"}, base[2:]...) return out diff --git a/internal/compose/translate_test.go b/internal/compose/translate_test.go index ea4c067..7d1976f 100644 --- a/internal/compose/translate_test.go +++ b/internal/compose/translate_test.go @@ -112,7 +112,7 @@ func TestRunArgsBasics(t *testing.T) { p := &Project{Name: "proj", Dir: "/tmp/proj"} svc := &Service{ Image: "nginx", - Environment: MapList{"A": "1"}, + Environment: EnvMap{"A": "1"}, Ports: []string{"8080:80"}, Command: StringList{"nginx -g daemon"}, } @@ -273,6 +273,77 @@ func TestCreateArgsPreservesDetachInCommand(t *testing.T) { } } +// TestOneOffArgsEntrypointOverrideDropsExtras reproduces the bug where a +// service with a multi-token entrypoint (["/svc-ep","--flag"]), run with +// --entrypoint override and no command, leaked the old entrypoint's "--flag" as +// an argument to the new entrypoint. Overriding the entrypoint replaces it whole. +func TestOneOffArgsEntrypointOverrideDropsExtras(t *testing.T) { + p := &Project{Name: "proj", Dir: "/tmp"} + svc := &Service{Image: "nginx", Entrypoint: StringList{"/svc-ep", "--flag"}} + got := p.OneOffArgs("web", svc, "", nil, nil, "/override-ep", true) + if indexOf(got, "--flag") != -1 { + t.Errorf("stale entrypoint extra --flag must be dropped on override: %v", got) + } + mustContainPair(t, got, "--entrypoint", "/override-ep") + // Without an override, the extras stay (they belong to the service entrypoint). + keep := p.OneOffArgs("web", svc, "", nil, nil, "", true) + if indexOf(keep, "--flag") == -1 { + t.Errorf("without override, the entrypoint extra should remain: %v", keep) + } +} + +// TestOneOffArgsLabelsOneoff reproduces the cross-talk bug: a one-off `compose +// run` container was labeled oneoff=False, indistinguishable from service +// replica #1. It must carry oneoff=True so ps/exec/down don't treat it as a +// replica, while RunArgs (real services) stays oneoff=False. +func TestOneOffArgsLabelsOneoff(t *testing.T) { + p := &Project{Name: "proj", Dir: "/tmp"} + svc := &Service{Image: "nginx"} + one := p.OneOffArgs("web", svc, "", []string{"sh"}, nil, "", true) + mustContainPair(t, one, "--label", LabelOneoff+"=True") + if indexOf(one, LabelOneoff+"=False") != -1 { + t.Errorf("one-off must not also carry oneoff=False: %v", one) + } + svc2 := &Service{Image: "nginx"} + rep := p.RunArgs("web", svc2, 1, "", nil) + mustContainPair(t, rep, "--label", LabelOneoff+"=False") +} + +// TestFlattenLongPortHostIP reproduces the bug where host_ip without published +// produced "host_ip:target" (misread as host_port:container). It must use the +// explicit empty-published 3-part form host_ip::target. +func TestFlattenLongPortHostIP(t *testing.T) { + if got := flattenLongPort(map[string]string{"target": "80", "host_ip": "127.0.0.1"}); got != "127.0.0.1::80" { + t.Errorf("host_ip without published = %q, want 127.0.0.1::80", got) + } + if got := flattenLongPort(map[string]string{"target": "80", "published": "8080", "host_ip": "127.0.0.1"}); got != "127.0.0.1:8080:80" { + t.Errorf("full form = %q, want 127.0.0.1:8080:80", got) + } + if got := flattenLongPort(map[string]string{"target": "80", "published": "8080", "protocol": "udp"}); got != "8080:80/udp" { + t.Errorf("proto form = %q, want 8080:80/udp", got) + } +} + +// TestLevelsCycleNoDeadlock guards that a depends_on cycle is handled safely: +// every service is still emitted (no deadlock, no panic), even if the leading +// level is empty. +func TestLevelsCycleNoDeadlock(t *testing.T) { + p := &Project{Name: "t", Services: map[string]*Service{ + "a": {DependsOn: DependsList{"b"}}, + "b": {DependsOn: DependsList{"a"}}, + }} + levels := p.Levels() + seen := map[string]bool{} + for _, lv := range levels { + for _, n := range lv { + seen[n] = true + } + } + if !seen["a"] || !seen["b"] { + t.Errorf("cycle members must all be emitted; got levels %v", levels) + } +} + func TestRunArgsEntrypointAndCommandOrder(t *testing.T) { p := &Project{Name: "p", Dir: "/tmp"} svc := &Service{Image: "img", Entrypoint: StringList{"/ep", "--flag"}, Command: StringList{"arg1"}} @@ -298,7 +369,7 @@ func TestRunArgsEmptyEntrypointNotEmitted(t *testing.T) { func TestRunArgsDeterministic(t *testing.T) { p := &Project{Name: "p", Dir: "/tmp"} - svc := &Service{Image: "img", Environment: MapList{"B": "2", "A": "1", "C": "3"}, Labels: MapList{"z": "1", "a": "2"}} + svc := &Service{Image: "img", Environment: EnvMap{"B": "2", "A": "1", "C": "3"}, Labels: MapList{"z": "1", "a": "2"}} first := p.RunArgs("s", svc, 1, "", nil) for i := 0; i < 20; i++ { if !reflect.DeepEqual(first, p.RunArgs("s", svc, 1, "", nil)) { diff --git a/internal/dockerfmt/render.go b/internal/dockerfmt/render.go index af7dca8..58b85f7 100644 --- a/internal/dockerfmt/render.go +++ b/internal/dockerfmt/render.go @@ -58,10 +58,16 @@ var fieldHeader = map[string]string{ ".PIDs": "PIDS", } -// fieldActionRe matches a whole `{{ .Field … }}` action so the header -// derivation can replace the entire action with the column header, leaving any -// surrounding literal text (dots, version strings) untouched. -var fieldActionRe = regexp.MustCompile(`{{\s*(\.[A-Za-z0-9_]+)[^}]*}}`) +// actionRe matches a whole `{{ … }}` template action; fieldRefRe finds a +// `.Field` reference within one. Together they let the header derivation replace +// each action with its column header, leaving surrounding literal text (dots, +// version strings) untouched — and crucially handling actions where the field is +// NOT the first token, e.g. `{{upper .Names}}` or `{{printf "%s" .ID}}`, which an +// anchored `{{\s*\.Field` regex missed (leaking the raw template into the header). +var ( + actionRe = regexp.MustCompile(`{{[^}]*}}`) + fieldRefRe = regexp.MustCompile(`\.([A-Za-z0-9_]+)`) +) // Render emits a list of view objects honouring Docker's -q / --format / // default-table conventions. @@ -117,10 +123,16 @@ func Render(format string, quiet bool, views []any, def TableDef) error { return err } w := NewTabWriter() - // Header row: replace each whole {{.Field …}} action with its column - // header; literal text between actions is preserved verbatim. - header := fieldActionRe.ReplaceAllStringFunc(body, func(m string) string { - tok := fieldActionRe.FindStringSubmatch(m)[1] + // Header row: replace each whole {{…}} action with its column header, + // derived from the LAST .Field reference inside the action (so function- + // or pipeline-prefixed actions resolve too); literal text between actions + // is preserved verbatim. An action with no field reference is dropped. + header := actionRe.ReplaceAllStringFunc(body, func(m string) string { + refs := fieldRefRe.FindAllStringSubmatch(m, -1) + if len(refs) == 0 { + return "" + } + tok := "." + refs[len(refs)-1][1] if h, ok := fieldHeader[tok]; ok { return h } diff --git a/internal/dockerfmt/render_test.go b/internal/dockerfmt/render_test.go index 9ac7f33..43e5d5d 100644 --- a/internal/dockerfmt/render_test.go +++ b/internal/dockerfmt/render_test.go @@ -89,6 +89,23 @@ func TestRenderTableTemplateHeaders(t *testing.T) { } } +// TestRenderTableHeaderFunctionPrefixed reproduces the regression where a +// table format whose action starts with a function/pipeline (e.g. +// `{{upper .Name}}`) leaked the raw `{{upper .Name}}` text into the header row +// because the header regex only matched actions beginning with `.Field`. The +// header must derive from the field reference regardless of leading function. +func TestRenderTableHeaderFunctionPrefixed(t *testing.T) { + views, def := sampleDef() + out := captureStdout(t, func() { Render("table {{.ID}}\t{{upper .Name}}", false, views, def) }) + header := strings.SplitN(out, "\n", 2)[0] + if strings.Contains(header, "{{") || strings.Contains(header, "}}") { + t.Errorf("header leaked raw template text: %q", header) + } + if !strings.Contains(header, "NAME") || !strings.Contains(header, "ID") { + t.Errorf("header should derive NAME and ID columns: %q", header) + } +} + // TestRenderPlainPathByteIdentical locks the drop-in contract: with styling // forced OFF, the default table is byte-for-byte the tabwriter output and // carries no ANSI/box-drawing characters — exactly what pipes and CI parse. diff --git a/internal/pool/pool.go b/internal/pool/pool.go index 53a367b..abb1f83 100644 --- a/internal/pool/pool.go +++ b/internal/pool/pool.go @@ -289,7 +289,12 @@ func Claim(image string) (Member, bool) { norm := NormalizeRef(image) var claimed Member var ok bool - _ = withLock(func(s *state) error { + // If withLock's save() fails, the in-memory removal of the member is NOT + // persisted — the member is still listed in pool.json on disk, so a concurrent + // or subsequent Claim would hand out the SAME live VM (two runs exec'ing into + // one microVM, breaking single-use isolation). Treat a persist failure as a + // pool miss so the caller cold-runs a fresh VM instead. + if err := withLock(func(s *state) error { for i, m := range s.Members { if m.Image == norm { claimed = m @@ -299,7 +304,9 @@ func Claim(image string) (Member, bool) { } } return nil - }) + }); err != nil { + return Member{}, false + } return claimed, ok } diff --git a/internal/pool/pool_test.go b/internal/pool/pool_test.go index b4e6abb..5be2cd7 100644 --- a/internal/pool/pool_test.go +++ b/internal/pool/pool_test.go @@ -1,9 +1,49 @@ package pool import ( + "os" + "path/filepath" "testing" ) +// TestClaimMissOnSaveFailure reproduces the isolation bug where Claim ignored a +// state-write failure and still returned ok=true: the popped member stayed in +// pool.json on disk, so a concurrent/next Claim could hand out the SAME live VM +// (two runs exec'ing into one microVM). On a persist failure Claim must report a +// miss (so the caller cold-runs) and leave the member available. +func TestClaimMissOnSaveFailure(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("running as root bypasses directory permissions") + } + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + + if err := Add(Member{ID: "vm1", Image: NormalizeRef("alpine")}); err != nil { + t.Fatalf("seed Add: %v", err) + } + d, err := dir() + if err != nil { + t.Fatal(err) + } + // Read-only state dir => save()'s temp-file write fails inside withLock. + if err := os.Chmod(d, 0o500); err != nil { + t.Fatal(err) + } + defer os.Chmod(d, 0o755) //nolint:errcheck // restore for temp-dir cleanup + + if _, ok := Claim("alpine"); ok { + t.Error("Claim must report a miss when the state write fails") + } + // Restore and confirm the member was NOT lost (still claimable later). + if err := os.Chmod(d, 0o755); err != nil { + t.Fatal(err) + } + if got := AvailableDepth("alpine"); got != 1 { + t.Errorf("member should remain available after a failed claim; depth=%d", got) + } +} + func TestNormalizeRef(t *testing.T) { cases := map[string]string{ "alpine": "alpine:latest", From 8492f128dd71d0f296fb079325d82d280e0e6254 Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 11:17:52 +0530 Subject: [PATCH 09/17] fix: 5 issues from third red-team pass (incl. 2 self-introduced regressions) - compose long-form volume read_only: parse as a boolean (ParseBool), so 'read_only: True'/'TRUE'/'1' produce a :ro mount instead of silently mounting read-write. (regression in the pass-1 long-form volume support) - OneOffArgs: a command override no longer drops the service entrypoint's extra tokens when --entrypoint is NOT overridden, so 'compose run web shell' on entrypoint [python,-m,flask] runs 'python -m flask shell', not 'python shell'. Post-image tokens are now split into entrypoint-extras (kept unless the entrypoint is replaced) and command (replaced by the override). (regression in the pass-2 entrypoint fix) - compose ls -q now prints only project names (the flag was ignored). - volume create --name is honored instead of silently creating a random-named volume; supplying both --name and a positional errors. - dockerfmt table header derivation strips string literals before scanning for the field, so a dotted token inside a printf format ({{.Name | printf "%s.txt"}}) no longer becomes the header column. Reproducing tests for all except 'compose ls -q' (backend-coupled glue). --- cmd/cli_fixes_test.go | 18 ++++++++++++++ cmd/compose.go | 11 +++++++-- cmd/volume.go | 24 +++++++++++++++--- internal/compose/model.go | 6 ++++- internal/compose/translate.go | 35 ++++++++++++++++++--------- internal/compose/translate_test.go | 39 ++++++++++++++++++++++++++++++ internal/dockerfmt/render.go | 7 +++++- internal/dockerfmt/render_test.go | 16 ++++++++++++ 8 files changed, 137 insertions(+), 19 deletions(-) diff --git a/cmd/cli_fixes_test.go b/cmd/cli_fixes_test.go index 32195b3..07bffab 100644 --- a/cmd/cli_fixes_test.go +++ b/cmd/cli_fixes_test.go @@ -130,6 +130,24 @@ func TestMergeInspectArrays(t *testing.T) { } } +// 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) + } +} + // 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). diff --git a/cmd/compose.go b/cmd/compose.go index 3ff4ab2..d925b6e 100644 --- a/cmd/compose.go +++ b/cmd/compose.go @@ -994,13 +994,20 @@ func composeLs() *cobra.Command { configDir[proj] = d } } - w := dockerfmt.NewTabWriter() - fmt.Fprintln(w, "NAME\tSTATUS\tCONFIG FILES") names := make([]string, 0, len(projects)) for n := range projects { names = append(names, n) } sort.Strings(names) + // -q/--quiet: only project names, one per line (matches docker). + if quiet, _ := cmd.Flags().GetBool("quiet"); quiet { + for _, n := range names { + fmt.Println(n) + } + return nil + } + w := dockerfmt.NewTabWriter() + fmt.Fprintln(w, "NAME\tSTATUS\tCONFIG FILES") for _, n := range names { running := projects[n]["running"] total := 0 diff --git a/cmd/volume.go b/cmd/volume.go index 393b9f2..46d4608 100644 --- a/cmd/volume.go +++ b/cmd/volume.go @@ -20,6 +20,23 @@ func randomName() string { return hex.EncodeToString(b) } +// resolveVolumeName picks the name for `volume create`: the positional VOLUME +// or the (hidden) --name flag, erroring if both are given, and falling back to a +// random anonymous-volume id when neither is supplied. Previously --name was +// silently ignored, creating a random-named volume. +func resolveVolumeName(flagName string, nameChanged bool, args []string) (string, error) { + if len(args) == 1 { + if nameChanged { + return "", fmt.Errorf("conflicting options: cannot supply both --name and a positional VOLUME name") + } + return args[0], nil + } + if flagName != "" { + return flagName, nil + } + return randomName(), nil +} + // matchVolumeFilters implements docker volume ls --filter (name/driver/label). func matchVolumeFilters(v dockerfmt.Volume, driver string, filters []string) bool { for _, fl := range filters { @@ -67,9 +84,10 @@ func newVolumeGroupCmd() *cobra.Command { Short: "Create a volume", Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - name := randomName() - if len(args) == 1 { - name = args[0] + nf, _ := cmd.Flags().GetString("name") + name, err := resolveVolumeName(nf, cmd.Flags().Changed("name"), args) + if err != nil { + return err } cargs := []string{"volume", "create"} for _, l := range mustStringArray(cmd.Flags(), "label") { diff --git a/internal/compose/model.go b/internal/compose/model.go index 3e7c7da..ba71e13 100644 --- a/internal/compose/model.go +++ b/internal/compose/model.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "sort" + "strconv" "strings" "gopkg.in/yaml.v3" @@ -382,7 +383,10 @@ func flattenLongVolume(m map[string]string) string { if src := m["source"]; src != "" { s = src + ":" + s } - if m["read_only"] == "true" { + // read_only is a YAML boolean; its node text may be true/True/TRUE (and 1). + // Parse it rather than comparing to the lowercase literal, so a read-only + // mount spelled `read_only: True` isn't silently emitted as writable. + if b, err := strconv.ParseBool(m["read_only"]); err == nil && b { s += ":ro" } return s diff --git a/internal/compose/translate.go b/internal/compose/translate.go index 1b08a5e..5846afc 100644 --- a/internal/compose/translate.go +++ b/internal/compose/translate.go @@ -280,21 +280,32 @@ func (p *Project) OneOffArgs(service string, svc *Service, netName string, cmdOv } out = append(out, overrides...) // raw CLI override tokens, before the image out = append(out, image) + + // The tokens after the image are the service entrypoint's extra tokens + // (Entrypoint[1:], which runArgs places here because the backend --entrypoint + // takes a single token) followed by the service command. Split them so each is + // handled per Docker semantics: + // - entrypoint extras: kept when the entrypoint is NOT overridden (they + // belong to the surviving entrypoint); dropped when --entrypoint replaces + // the whole entrypoint. + // - command: replaced by cmdOverride when given, else the service command. + var extras, command []string + if n := len(svc.Entrypoint) - 1; n > 0 { + if avail := len(base) - (imageIdx + 1); n > avail { + n = avail + } + extras = base[imageIdx+1 : imageIdx+1+n] + command = base[imageIdx+1+n:] + } else { + command = base[imageIdx+1:] + } + if entrypoint == "" { + out = append(out, extras...) // keep the service entrypoint's own args + } if len(cmdOverride) > 0 { return append(out, cmdOverride...) // override replaces the service command } - // Keep the service's own command (the tokens after the image). runArgs places - // the service entrypoint's extra tokens (Entrypoint[1:]) there first; when the - // entrypoint is being overridden, those extras belong to the OLD entrypoint and - // must be dropped — overriding the entrypoint replaces it whole (Docker - // semantics), it must not leave the old entrypoint's args bound to the new one. - tail := base[imageIdx+1:] - if entrypoint != "" && len(svc.Entrypoint) > 1 { - if drop := len(svc.Entrypoint) - 1; drop <= len(tail) { - tail = tail[drop:] - } - } - return append(out, tail...) + return append(out, command...) } // CreateArgs builds the `container create ...` args for a service: the same diff --git a/internal/compose/translate_test.go b/internal/compose/translate_test.go index 7d1976f..b751991 100644 --- a/internal/compose/translate_test.go +++ b/internal/compose/translate_test.go @@ -324,6 +324,45 @@ func TestFlattenLongPortHostIP(t *testing.T) { } } +// TestFlattenLongVolumeReadOnlyCasing reproduces the bug where read_only was +// honored only for lowercase "true"; True/TRUE/1 are valid YAML booleans and +// must also produce a :ro (read-only) mount, not a silently writable one. +func TestFlattenLongVolumeReadOnlyCasing(t *testing.T) { + for _, v := range []string{"true", "True", "TRUE", "1"} { + got := flattenLongVolume(map[string]string{"source": "./d", "target": "/d", "read_only": v}) + if got != "./d:/d:ro" { + t.Errorf("read_only=%q -> %q, want ./d:/d:ro", v, got) + } + } + for _, v := range []string{"false", "False", "0", ""} { + got := flattenLongVolume(map[string]string{"source": "./d", "target": "/d", "read_only": v}) + if got != "./d:/d" { + t.Errorf("read_only=%q -> %q, want ./d:/d (writable)", v, got) + } + } +} + +// TestOneOffArgsKeepsEntrypointExtrasWithCmdOverride reproduces the bug where a +// command override dropped the service entrypoint's extra tokens when the +// entrypoint itself was NOT overridden. `compose run web shell` on a service +// with entrypoint [python,-m,flask] + command [run] must run +// `python -m flask shell` (command replaced, entrypoint kept whole). +func TestOneOffArgsKeepsEntrypointExtrasWithCmdOverride(t *testing.T) { + p := &Project{Name: "proj", Dir: "/tmp"} + svc := &Service{Image: "nginx", Entrypoint: StringList{"python", "-m", "flask"}, Command: StringList{"run"}} + got := p.OneOffArgs("web", svc, "", []string{"shell"}, nil, "", true) + mustContainPair(t, got, "--entrypoint", "python") + img := indexOf(got, "nginx") + if img < 0 { + t.Fatalf("image not found: %v", got) + } + post := got[img+1:] + want := []string{"-m", "flask", "shell"} + if !reflect.DeepEqual(post, want) { + t.Errorf("post-image tokens = %v, want %v (entrypoint extras kept, command replaced)", post, want) + } +} + // TestLevelsCycleNoDeadlock guards that a depends_on cycle is handled safely: // every service is still emitted (no deadlock, no panic), even if the leading // level is empty. diff --git a/internal/dockerfmt/render.go b/internal/dockerfmt/render.go index 58b85f7..b54233e 100644 --- a/internal/dockerfmt/render.go +++ b/internal/dockerfmt/render.go @@ -67,6 +67,10 @@ var fieldHeader = map[string]string{ var ( actionRe = regexp.MustCompile(`{{[^}]*}}`) fieldRefRe = regexp.MustCompile(`\.([A-Za-z0-9_]+)`) + // strLitRe matches Go-template string literals ("…", `…`, '…') so a dotted + // token inside one (e.g. the "%s.txt" in `{{.Names | printf "%s.txt"}}`) is + // not mistaken for a field reference during header derivation. + strLitRe = regexp.MustCompile("\"[^\"]*\"|`[^`]*`|'[^']*'") ) // Render emits a list of view objects honouring Docker's -q / --format / @@ -128,7 +132,8 @@ func Render(format string, quiet bool, views []any, def TableDef) error { // or pipeline-prefixed actions resolve too); literal text between actions // is preserved verbatim. An action with no field reference is dropped. header := actionRe.ReplaceAllStringFunc(body, func(m string) string { - refs := fieldRefRe.FindAllStringSubmatch(m, -1) + clean := strLitRe.ReplaceAllString(m, "") // drop string literals first + refs := fieldRefRe.FindAllStringSubmatch(clean, -1) if len(refs) == 0 { return "" } diff --git a/internal/dockerfmt/render_test.go b/internal/dockerfmt/render_test.go index 43e5d5d..6485f99 100644 --- a/internal/dockerfmt/render_test.go +++ b/internal/dockerfmt/render_test.go @@ -106,6 +106,22 @@ func TestRenderTableHeaderFunctionPrefixed(t *testing.T) { } } +// TestRenderTableHeaderDottedLiteral reproduces the regression where a dotted +// token inside a string literal in a pipeline action (e.g. "%s.txt") was +// mistaken for the field reference, deriving the wrong header (TXT) instead of +// the actual field's column (NAME). +func TestRenderTableHeaderDottedLiteral(t *testing.T) { + views, def := sampleDef() + out := captureStdout(t, func() { Render(`table {{.Name | printf "%s.txt"}}`, false, views, def) }) + header := strings.SplitN(out, "\n", 2)[0] + if strings.Contains(header, "TXT") { + t.Errorf("dotted literal inside a string must not become the header: %q", header) + } + if !strings.Contains(header, "NAME") { + t.Errorf("header should derive from the real field .Name: %q", header) + } +} + // TestRenderPlainPathByteIdentical locks the drop-in contract: with styling // forced OFF, the default table is byte-for-byte the tabwriter output and // carries no ANSI/box-drawing characters — exactly what pipes and CI parse. From f3bb6de611dff5c6eb8b063f8c3103b30b75a7c1 Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 11:19:51 +0530 Subject: [PATCH 10/17] style: gofmt alignment/trailing-newline in compose files --- cmd/compose.go | 1 - internal/compose/translate.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/compose.go b/cmd/compose.go index d925b6e..2660db4 100644 --- a/cmd/compose.go +++ b/cmd/compose.go @@ -1730,4 +1730,3 @@ func firstServiceContainer(project, service string) (string, error) { } return "", fmt.Errorf("no running container for service %q", service) } - diff --git a/internal/compose/translate.go b/internal/compose/translate.go index 5846afc..c1a7bc9 100644 --- a/internal/compose/translate.go +++ b/internal/compose/translate.go @@ -253,7 +253,7 @@ func (p *Project) ImageRef(service string, svc *Service) string { // command args is never removed. func (p *Project) OneOffArgs(service string, svc *Service, netName string, cmdOverride, overrides []string, entrypoint string, rm bool) []string { base, imageIdx := p.runArgs(service, svc, 1, netName, nil, true) // oneoff=true - flags := base[1:imageIdx] // the run flags span (after "run", before image) + flags := base[1:imageIdx] // the run flags span (after "run", before image) image := base[imageIdx] out := []string{"run"} From 3779f56173e21f263a37cc8c1102eb3d833efe5c Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 11:28:07 +0530 Subject: [PATCH 11/17] fix(compose/stats): 5 issues from fourth red-team pass - compose: $$ is compose's escape for a literal $; Load() was deleting it (echo $$HOME -> echo HOME). Collapse $$ to a single $ so literal-dollar values (shell vars, cron, hashes) survive interpolation. - compose images now honors the [SERVICE...] positional filter instead of always listing every service's image. - compose ls defaults to projects with a running container; -a/--all now actually includes fully-stopped projects (the flag was dead). - compose config -q/--quiet now validates silently (prints nothing) instead of dumping the full rendered YAML. - stats: the live-table clear-screen escape is gated on a real terminal, so 'dcon stats | cat' no longer injects raw ANSI into piped output. $$-escape and config -q covered by reproducing tests; the others are backend-coupled paths (per the repo's manual-validation convention). --- cmd/compose.go | 15 ++++++++++++++- cmd/compose_helpers_test.go | 32 ++++++++++++++++++++++++++++++++ cmd/stats.go | 7 +++++-- internal/compose/model.go | 6 ++++++ internal/compose/model_test.go | 30 ++++++++++++++++++++++++++++++ 5 files changed, 87 insertions(+), 3 deletions(-) diff --git a/cmd/compose.go b/cmd/compose.go index 2660db4..596c262 100644 --- a/cmd/compose.go +++ b/cmd/compose.go @@ -939,6 +939,10 @@ func composeConfig() *cobra.Command { if err != nil { return err } + // -q/--quiet: only validate (loadProject above did that), print nothing. + if q, _ := cmd.Flags().GetBool("quiet"); q { + return nil + } if v, _ := cmd.Flags().GetBool("services"); v { names := make([]string, 0, len(p.Services)) for n := range p.Services { @@ -994,9 +998,14 @@ func composeLs() *cobra.Command { configDir[proj] = d } } + // Default to projects with a running container; -a/--all includes + // fully-stopped ones too (matches docker compose ls). + showAll, _ := cmd.Flags().GetBool("all") names := make([]string, 0, len(projects)) for n := range projects { - names = append(names, n) + if showAll || projects[n]["running"] > 0 { + names = append(names, n) + } } sort.Strings(names) // -q/--quiet: only project names, one per line (matches docker). @@ -1367,9 +1376,13 @@ func composeImages() *cobra.Command { if err != nil { return err } + selected := serviceSet(args) // nil => all services (docker: images [SERVICE...]) w := dockerfmt.NewTabWriter() fmt.Fprintln(w, "CONTAINER\tREPOSITORY\tTAG\tSIZE") for _, c := range containers { + if selected != nil && !selected[serviceOf(c)] { + continue + } repo, tag := dockerfmt.SplitRepoTag(dockerfmt.ShortImage(c.Configuration.Image.Reference)) fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", c.ID, repo, tag, "N/A") } diff --git a/cmd/compose_helpers_test.go b/cmd/compose_helpers_test.go index 68118a7..1ed333b 100644 --- a/cmd/compose_helpers_test.go +++ b/cmd/compose_helpers_test.go @@ -1,7 +1,10 @@ package cmd import ( + "os" + "path/filepath" "reflect" + "strings" "testing" "dcon/internal/compose" @@ -77,6 +80,35 @@ func TestEffectiveReplicas(t *testing.T) { } } +// TestComposeConfigQuiet reproduces the bug where `compose config -q` printed +// the full rendered config instead of validating silently. With -q it must +// print nothing on a valid file (and still validate, via loadProject). +func TestComposeConfigQuiet(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "compose.yaml"), + []byte("services:\n web:\n image: nginx\n"), 0o644); err != nil { + t.Fatal(err) + } + t.Chdir(dir) // compose.Find searches the working directory + + run := func(args []string) string { + cmd := composeConfig() + cmd.SetArgs(args) + cmd.SilenceUsage, cmd.SilenceErrors = true, true + return captureOut(t, func() { + if err := cmd.Execute(); err != nil { + t.Fatalf("execute %v: %v", args, err) + } + }) + } + if out := run([]string{"-q"}); out != "" { + t.Errorf("config -q must print nothing; got %q", out) + } + if out := run(nil); !strings.Contains(out, "nginx") { + t.Errorf("config (no -q) should render the config; got %q", out) + } +} + func TestServiceSet(t *testing.T) { if got := serviceSet(nil); got != nil { t.Errorf("serviceSet(nil) = %v, want nil", got) diff --git a/cmd/stats.go b/cmd/stats.go index 31ac4e4..6ce7185 100644 --- a/cmd/stats.go +++ b/cmd/stats.go @@ -2,6 +2,7 @@ package cmd import ( "fmt" + "os" "strings" "time" @@ -132,8 +133,10 @@ func newStatsCmd() *cobra.Command { return err } dt := time.Since(prevT).Seconds() - if streaming && tableMode { - fmt.Print("\033[H\033[2J") // clear screen only for the live table view + // Clear the screen only for a live table on a real terminal — never + // when stdout is a pipe/file, so piped output stays free of raw ANSI. + if streaming && tableMode && isTerminal(os.Stdout) { + fmt.Print("\033[H\033[2J") } if err := renderStats(cur, prev, dt, format, noTrunc, extraIDs(cur)); err != nil { return err diff --git a/internal/compose/model.go b/internal/compose/model.go index ba71e13..77d616c 100644 --- a/internal/compose/model.go +++ b/internal/compose/model.go @@ -509,6 +509,12 @@ func Load(path, projectOverride string) (*Project, error) { } // Expand ${VAR} / $VAR from the environment, like compose does. expanded := os.Expand(string(data), func(key string) string { + // `$$` is compose's escape for a literal `$`: os.Expand calls us with + // key=="$" for it, so collapse it back to a single dollar rather than + // treating "$" as a variable name (which would delete the escaped dollar). + if key == "$" { + return "$" + } // support ${VAR:-default} if i := strings.Index(key, ":-"); i >= 0 { name, def := key[:i], key[i+2:] diff --git a/internal/compose/model_test.go b/internal/compose/model_test.go index 159c89f..97ec266 100644 --- a/internal/compose/model_test.go +++ b/internal/compose/model_test.go @@ -341,6 +341,36 @@ services: } } +// TestDollarEscapePreserved reproduces the bug where `$$` (compose's escape for +// a literal `$`) was deleted instead of collapsed to a single `$`. e.g. +// `command: echo $$HOME` must keep a literal `$HOME`, not become `echo HOME`. +func TestDollarEscapePreserved(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "compose.yaml") + yaml := ` +services: + web: + image: alpine + command: echo $$HOME and price $$5 + environment: + - LITERAL=a$$b +` + if err := os.WriteFile(path, []byte(yaml), 0o644); err != nil { + t.Fatal(err) + } + p, err := Load(path, "") + if err != nil { + t.Fatal(err) + } + web := p.Services["web"] + if len(web.Command) != 1 || web.Command[0] != "echo $HOME and price $5" { + t.Errorf("command = %q, want 'echo $HOME and price $5'", web.Command) + } + if web.Environment["LITERAL"] != "a$b" { + t.Errorf("env LITERAL = %q, want a$b", web.Environment["LITERAL"]) + } +} + func TestServiceEnabledProfiles(t *testing.T) { noProf := &Service{} if !noProf.Enabled(map[string]bool{}) { From 18fe3602164af3bd30278d670a34e55fd3d5ff3f Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 11:39:07 +0530 Subject: [PATCH 12/17] fix(compose): full var-interpolation, named-volume scoping, kill/stop flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - interpolation: Load() only honored ${VAR:-default}; every other operator (:?, :+, -, +, :=) silently produced empty — even for SET variables, so 'image: app:${TAG:?required}' with TAG=v2 rendered 'app:'. Implement the bash/compose parameter-expansion family in a new expandVar (-/:- default, +/:+ alternate, ?/:? required-with-error, =/:= default); a required-but- unset ${VAR:?} now aborts Load with a clear error. - named volumes: a service mount of a declared top-level volume used the bare key (e.g. 'data:/var/lib') while ensureVolumes created '_data', so the container got a different (auto-created) volume than declared and 'down -v' removed the wrong one. resolveVolume now rewrites a declared named volume to its VolumeName (project-scoped / explicit name:), mirroring the network resolution. - compose kill --signal and stop/restart --timeout were registered but dropped; lifecycleOnProject now forwards them (the backend kill/stop accept --signal/--time), via composeKillArgs/composeStopArgs. All covered by reproducing tests (expandVar table, required-var Load error, resolveVolume named volume, kill/stop arg builders). --- cmd/cli_fixes_test.go | 18 ++++++ cmd/compose.go | 33 ++++++++++- internal/compose/model.go | 95 +++++++++++++++++++++++++----- internal/compose/model_test.go | 52 ++++++++++++++++ internal/compose/translate.go | 13 +++- internal/compose/translate_test.go | 27 +++++++++ 6 files changed, 218 insertions(+), 20 deletions(-) diff --git a/cmd/cli_fixes_test.go b/cmd/cli_fixes_test.go index 07bffab..cfb9698 100644 --- a/cmd/cli_fixes_test.go +++ b/cmd/cli_fixes_test.go @@ -148,6 +148,24 @@ func TestResolveVolumeName(t *testing.T) { } } +// 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) + } +} + // 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). diff --git a/cmd/compose.go b/cmd/compose.go index 596c262..0a57805 100644 --- a/cmd/compose.go +++ b/cmd/compose.go @@ -880,6 +880,13 @@ func lifecycleOnProject(verb string) func(cmd *cobra.Command, args []string) err return err } selected := serviceSet(args) + // Forward the verb's honored flags (defined only on the relevant + // subcommands; absent flags read as zero/unchanged): kill --signal, and + // stop/restart --timeout. These were previously dropped, so `compose kill + // -s SIGTERM` hard-killed and `compose stop -t 30` ignored the grace period. + signal, _ := cmd.Flags().GetString("signal") + timeoutChanged := cmd.Flags().Changed("timeout") + timeout, _ := cmd.Flags().GetInt("timeout") for _, c := range containers { if selected != nil && !selected[serviceOf(c)] { continue @@ -888,13 +895,13 @@ func lifecycleOnProject(verb string) func(cmd *cobra.Command, args []string) err case "rm": _, _ = runtime.CaptureSilent("delete", "--force", c.ID) case "stop": - _, _ = runtime.CaptureSilent("stop", c.ID) + _, _ = runtime.CaptureSilent(composeStopArgs(timeoutChanged, timeout, c.ID)...) case "start": _, _ = runtime.CaptureSilent("start", c.ID) case "kill": - _, _ = runtime.CaptureSilent("kill", c.ID) + _, _ = runtime.CaptureSilent(composeKillArgs(signal, c.ID)...) case "restart": - _, _ = runtime.CaptureSilent("stop", c.ID) + _, _ = runtime.CaptureSilent(composeStopArgs(timeoutChanged, timeout, c.ID)...) _, _ = runtime.CaptureSilent("start", c.ID) } fmt.Println(c.ID) @@ -903,6 +910,26 @@ func lifecycleOnProject(verb string) func(cmd *cobra.Command, args []string) err } } +// composeStopArgs builds the backend stop argv for compose stop/restart, +// forwarding --time only when the user set --timeout. +func composeStopArgs(timeoutChanged bool, timeout int, id string) []string { + args := []string{"stop"} + if timeoutChanged { + args = append(args, "--time", strconv.Itoa(timeout)) + } + return append(args, id) +} + +// composeKillArgs builds the backend kill argv for compose kill, forwarding the +// chosen --signal (defaults to SIGKILL). +func composeKillArgs(signal, id string) []string { + args := []string{"kill"} + if signal != "" { + args = append(args, "--signal", signal) + } + return append(args, id) +} + func composeStart() *cobra.Command { return &cobra.Command{Use: "start [SERVICE...]", Short: "Start services", RunE: lifecycleOnProject("start")} } diff --git a/internal/compose/model.go b/internal/compose/model.go index 77d616c..4ad077f 100644 --- a/internal/compose/model.go +++ b/internal/compose/model.go @@ -507,24 +507,21 @@ func Load(path, projectOverride string) (*Project, error) { if err != nil { return nil, err } - // Expand ${VAR} / $VAR from the environment, like compose does. + // Expand ${VAR} / $VAR from the environment, like compose does, supporting + // the bash-style operators compose honors: :-/- (default), :+/+ (alternate), + // :?/? (required, error if missing), :=/= (default). A required-but-missing + // variable is surfaced as a load error. + var interpErr error expanded := os.Expand(string(data), func(key string) string { - // `$$` is compose's escape for a literal `$`: os.Expand calls us with - // key=="$" for it, so collapse it back to a single dollar rather than - // treating "$" as a variable name (which would delete the escaped dollar). - if key == "$" { - return "$" - } - // support ${VAR:-default} - if i := strings.Index(key, ":-"); i >= 0 { - name, def := key[:i], key[i+2:] - if v, ok := os.LookupEnv(name); ok { - return v - } - return def + v, err := expandVar(key) + if err != nil && interpErr == nil { + interpErr = err } - return os.Getenv(key) + return v }) + if interpErr != nil { + return nil, interpErr + } var p Project if err := yaml.Unmarshal([]byte(expanded), &p); err != nil { @@ -541,6 +538,74 @@ func Load(path, projectOverride string) (*Project, error) { return &p, nil } +// expandVar resolves one os.Expand key (the text inside ${...} or after $) using +// docker-compose / bash parameter-expansion semantics. os.Expand passes the +// whole brace body as key, so operators must be parsed here. Supported: +// +// $$ (key=="$") -> literal "$" +// ${VAR} -> value (empty if unset) +// ${VAR:-d} / ${VAR-d} -> value, or d when unset (:- also when empty) +// ${VAR:=d} / ${VAR=d} -> same substitution as :- here (no real assignment) +// ${VAR:+a} / ${VAR+a} -> a when set (:+ requires non-empty), else "" +// ${VAR:?m} / ${VAR?m} -> value when set, else an error (m is the message) +func expandVar(key string) (string, error) { + if key == "$" { // $$ escape + return "$", nil + } + i := 0 + for i < len(key) && isNameChar(key[i]) { + i++ + } + name, rest := key[:i], key[i:] + if name == "" { + return os.Getenv(key), nil // unrecognized form: best-effort + } + val, set := os.LookupEnv(name) + if rest == "" { + return val, nil // ${VAR} / $VAR + } + colon := rest[0] == ':' + if colon { + rest = rest[1:] + } + if rest == "" { + return val, nil + } + op, arg := rest[0], rest[1:] + // With a colon, an empty value counts as "not present" (bash semantics). + present := set && (!colon || val != "") + switch op { + case '-', '=': + if present { + return val, nil + } + return arg, nil + case '+': + if present { + return arg, nil + } + return "", nil + case '?': + if present { + return val, nil + } + msg := arg + if msg == "" { + msg = "required variable is not set" + } + return "", fmt.Errorf("compose variable %q: %s", name, msg) + default: + return os.Getenv(key), nil // unknown operator: best-effort + } +} + +func isNameChar(b byte) bool { + return b == '_' || + (b >= 'a' && b <= 'z') || + (b >= 'A' && b <= 'Z') || + (b >= '0' && b <= '9') +} + // SanitizeName makes a string safe for use as a container/network/volume name. func SanitizeName(s string) string { s = strings.ToLower(s) diff --git a/internal/compose/model_test.go b/internal/compose/model_test.go index 97ec266..c8b6a20 100644 --- a/internal/compose/model_test.go +++ b/internal/compose/model_test.go @@ -371,6 +371,58 @@ services: } } +// TestExpandVar reproduces the bug where only ${VAR:-default} was honored; +// every other operator (:?, :+, -, +, :=) silently produced empty, even for SET +// variables. Covers bash/compose parameter-expansion semantics. +func TestExpandVar(t *testing.T) { + t.Setenv("SETV", "x") + t.Setenv("EMPTYV", "") + // (key, want, wantErr) + cases := []struct { + key string + want string + wantErr bool + }{ + {"$", "$", false}, // $$ escape + {"SETV", "x", false}, // ${SETV} + {"UNSETV", "", false}, // ${UNSETV} + {"SETV:-d", "x", false}, // set -> value + {"UNSETV:-d", "d", false}, // unset -> default + {"EMPTYV:-d", "d", false}, // empty (colon) -> default + {"EMPTYV-d", "", false}, // empty (no colon) -> the empty value + {"UNSETV-d", "d", false}, // unset (no colon) -> default + {"SETV:+a", "a", false}, // set non-empty -> alternate + {"EMPTYV:+a", "", false}, // empty (colon) -> "" (not "present") + {"EMPTYV+a", "a", false}, // empty but set (no colon) -> alternate + {"UNSETV:+a", "", false}, // unset -> "" + {"SETV:?msg", "x", false}, // set -> value + {"UNSETV:?msg", "", true}, // unset -> error + {"EMPTYV:?msg", "", true}, // empty (colon) -> error + {"SETV:=d", "x", false}, // set -> value + {"UNSETV:=d", "d", false}, // unset -> default + } + for _, c := range cases { + got, err := expandVar(c.key) + if (err != nil) != c.wantErr { + t.Errorf("expandVar(%q) err=%v, wantErr=%v", c.key, err, c.wantErr) + continue + } + if !c.wantErr && got != c.want { + t.Errorf("expandVar(%q) = %q, want %q", c.key, got, c.want) + } + } +} + +// TestLoadRequiredVarErrors confirms a required-but-unset ${VAR:?} aborts Load. +func TestLoadRequiredVarErrors(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "compose.yaml") + os.WriteFile(path, []byte("services:\n a:\n image: img:${DCON_REQ_UNSET:?tag required}\n"), 0o644) + if _, err := Load(path, ""); err == nil { + t.Error("Load should fail when a required ${VAR:?} variable is unset") + } +} + func TestServiceEnabledProfiles(t *testing.T) { noProf := &Service{} if !noProf.Enabled(map[string]bool{}) { diff --git a/internal/compose/translate.go b/internal/compose/translate.go index c1a7bc9..28b1267 100644 --- a/internal/compose/translate.go +++ b/internal/compose/translate.go @@ -333,8 +333,14 @@ func (p *Project) resolve(path string) string { return filepath.Join(p.Dir, path) } -// resolveVolume rewrites a bind-mount source to an absolute path (named volumes -// and absolute paths pass through unchanged). +// resolveVolume rewrites a service volume spec's source so it matches what the +// up/create flow actually provisions: a relative bind source becomes absolute, +// and a declared named volume (a key in the top-level volumes:) becomes its +// project-scoped backend name (VolumeName). Without the latter, the container +// mounted a bare-keyed volume (e.g. `data`) while ensureVolumes created +// `_data`, so the service got a different volume than declared and +// `down -v` removed the wrong one. Absolute paths and undeclared names pass +// through unchanged. func (p *Project) resolveVolume(spec string) string { parts := strings.SplitN(spec, ":", 2) if len(parts) < 2 { @@ -344,6 +350,9 @@ func (p *Project) resolveVolume(spec string) string { if strings.HasPrefix(src, "./") || strings.HasPrefix(src, "../") || src == "." { return p.resolve(src) + ":" + parts[1] } + if vs, ok := p.Volumes[src]; ok { // declared named volume -> backend name + return p.VolumeName(src, vs) + ":" + parts[1] + } return spec } diff --git a/internal/compose/translate_test.go b/internal/compose/translate_test.go index b751991..dc74bff 100644 --- a/internal/compose/translate_test.go +++ b/internal/compose/translate_test.go @@ -363,6 +363,33 @@ func TestOneOffArgsKeepsEntrypointExtrasWithCmdOverride(t *testing.T) { } } +// TestResolveVolumeNamedVolume reproduces the bug where a declared named volume +// was mounted by its bare compose key instead of the project-scoped backend name +// that ensureVolumes actually creates (e.g. `data` vs `proj_data`), so the +// container got a different volume than declared. +func TestResolveVolumeNamedVolume(t *testing.T) { + p := &Project{ + Name: "proj", + Dir: "/tmp/proj", + Volumes: map[string]*VolumeSpec{"data": {}, "named": {Name: "custom"}}, + } + svc := &Service{Image: "x", Volumes: VolumeList{ + "data:/var/lib", // declared -> proj_data + "named:/n", // declared with explicit name -> custom + "ext:/e", // undeclared -> passthrough + "/abs:/a", // absolute bind -> passthrough + "./rel:/r", // relative bind -> resolved to absolute + }} + args := p.RunArgs("web", svc, 1, "", nil) + mustContainPair(t, args, "--volume", "proj_data:/var/lib") + mustContainPair(t, args, "--volume", "custom:/n") + mustContainPair(t, args, "--volume", "ext:/e") + mustContainPair(t, args, "--volume", "/abs:/a") + if !containsSub(args, "/tmp/proj/rel:/r") { + t.Errorf("relative bind not resolved to absolute: %v", args) + } +} + // TestLevelsCycleNoDeadlock guards that a depends_on cycle is handled safely: // every service is still emitted (no deadlock, no panic), even if the leading // level is empty. From d7adb560a11c867aa867c023d58ab084f11082d5 Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 11:49:08 +0530 Subject: [PATCH 13/17] fix(compose): rewrite -f/-p shorthand even when a root flag precedes `compose` rewriteComposeGlobalShorthands early-returned unless args[0]=="compose", so a root persistent flag before the subcommand (`dcon -D compose -f x.yml up`, `dcon --host x compose -p proj down`) skipped the rewrite and cobra then hard-failed with 'unknown shorthand flag: f', diverging from docker which accepts global flags before `compose`. Add composeIndex to locate the `compose` token past any root flags (skipping the value of separated value-flags like --host/--context/--tls*), and rewrite from there. Covered by new test cases. --- cmd/compose.go | 41 ++++++++++++++++++++++++++++++++++++++--- cmd/parity_test.go | 10 ++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/cmd/compose.go b/cmd/compose.go index 0a57805..7c10a9f 100644 --- a/cmd/compose.go +++ b/cmd/compose.go @@ -47,12 +47,47 @@ var composeValueFlags = map[string]bool{ // pflag panics when an inherited persistent shorthand collides with a local // one. Rewriting the leading tokens before parsing sidesteps that while leaving // post-subcommand shorthands (the `-f` in `compose logs -f`) untouched. +// rootValueFlags are the root persistent flags (separated form) that consume the +// following token as their value, so composeIndex skips that value when locating +// the compose subcommand. +var rootValueFlags = map[string]bool{ + "-H": true, "--host": true, "--context": true, "--log-level": true, + "--config": true, "--tlscacert": true, "--tlscert": true, "--tlskey": true, +} + +// composeIndex returns the index of the `compose` subcommand token, skipping any +// root persistent flags (and their separated values) that precede it — so +// `dcon -D compose ...` / `dcon --host x compose ...` are still recognized. It +// returns -1 when the invocation is not a compose command. +func composeIndex(args []string) int { + i := 0 + for i < len(args) { + a := args[i] + if a == "compose" { + return i + } + if strings.HasPrefix(a, "-") { + if rootValueFlags[a] { + i += 2 // skip the flag and its value + } else { + i++ // bool flag, --flag=value, or -Hvalue (self-contained) + } + continue + } + return -1 // a non-flag token that isn't compose => a different subcommand + } + return -1 +} + func rewriteComposeGlobalShorthands(args []string) []string { - if len(args) == 0 || args[0] != "compose" { + ci := composeIndex(args) + if ci < 0 { return args } - out := []string{"compose"} - i := 1 + // Preserve any root flags before `compose`, then rewrite the leading + // -f/-p shorthands that sit between `compose` and its subcommand. + out := append([]string{}, args[:ci+1]...) + i := ci + 1 for i < len(args) { a := args[i] if !strings.HasPrefix(a, "-") { diff --git a/cmd/parity_test.go b/cmd/parity_test.go index 0de342c..7a3fdd5 100644 --- a/cmd/parity_test.go +++ b/cmd/parity_test.go @@ -143,6 +143,16 @@ func TestRewriteComposeGlobalShorthands(t *testing.T) { // not a compose invocation: leave untouched (e.g. running an image named compose) {[]string{"run", "-f", "compose"}, []string{"run", "-f", "compose"}}, {[]string{"ps", "-a"}, []string{"ps", "-a"}}, + // root flags BEFORE `compose` must not block the -f/-p rewrite + {[]string{"-D", "compose", "-f", "x.yml", "up"}, []string{"-D", "compose", "--file", "x.yml", "up"}}, + {[]string{"--host", "tcp://x", "compose", "-f", "x.yml", "up"}, + []string{"--host", "tcp://x", "compose", "--file", "x.yml", "up"}}, + {[]string{"--host=x", "compose", "-p", "proj", "down"}, + []string{"--host=x", "compose", "--project-name", "proj", "down"}}, + {[]string{"-D", "--context", "c", "compose", "-fx.yml", "up"}, + []string{"-D", "--context", "c", "compose", "--file", "x.yml", "up"}}, + // root flag before a non-compose subcommand: untouched + {[]string{"-D", "ps", "-a"}, []string{"-D", "ps", "-a"}}, } for _, tc := range cases { got := rewriteComposeGlobalShorthands(tc.in) From 21e90671266e44b5b10ced93086618a825d62dba Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 11:59:47 +0530 Subject: [PATCH 14/17] fix: reject non-finite --cpus, unescape --format \t/\n, build progress, doctor builder - parseCPUs accepted inf/NaN (ParseFloat parses them), emitting --cpus 9223372036854775807 (Inf) or --cpus 0 (NaN) to the backend; reject non-finite values like docker does. Shared by run/create/machine create. - --format 'table {{.A}}\t{{.B}}' (and the plain template branch) rendered a literal backslash-t and never aligned columns; unescape \t/\n to real bytes like the docker CLI (this is the documented recipe in SECONDARY.md). - build --progress quiet was forwarded verbatim and rejected by the backend (accepts auto|plain|tty); remap quiet->plain (alongside rawjson). - doctor's image-builder check used substring "running", which also matches "not running"; exclude the negative form via builderRunning. All covered by reproducing tests. --- cmd/build.go | 5 +++-- cmd/cli_fixes_test.go | 34 +++++++++++++++++++++++++++++++ cmd/doctor.go | 9 +++++++- cmd/machine_test.go | 7 +++++++ cmd/run.go | 5 +++++ internal/dockerfmt/render.go | 12 +++++++++-- internal/dockerfmt/render_test.go | 22 ++++++++++++++++++++ 7 files changed, 89 insertions(+), 5 deletions(-) diff --git a/cmd/build.go b/cmd/build.go index ad9f843..40c0094 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -132,8 +132,9 @@ func buildBuildArgs(cmd *cobra.Command, args []string) ([]string, error) { } } 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) diff --git a/cmd/cli_fixes_test.go b/cmd/cli_fixes_test.go index cfb9698..ca39387 100644 --- a/cmd/cli_fixes_test.go +++ b/cmd/cli_fixes_test.go @@ -166,6 +166,40 @@ func TestComposeStopAndKillArgs(t *testing.T) { } } +// 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). diff --git a/cmd/doctor.go b/cmd/doctor.go index 24db96c..f8e9e23 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -136,7 +136,7 @@ func gatherChecks() []check { // 4. Image builder (only needed for `build`; warn if absent). builderOut, _ := rt.CaptureSilent("builder", "status") - if strings.Contains(builderOut, "running") { + if builderRunning(builderOut) { checks = append(checks, check{name: "Image builder", level: levelOK, detail: "running"}) } else { checks = append(checks, check{ @@ -167,6 +167,13 @@ func gatherChecks() []check { // kernelInstalled reports whether a guest kernel is present, by checking the // backend's kernels directory under the application-support root. +// builderRunning reports whether `container builder status` output indicates a +// running builder. "running" is a substring of "not running", so the negative +// form must be excluded explicitly. +func builderRunning(out string) bool { + return strings.Contains(out, "running") && !strings.Contains(out, "not running") +} + func kernelInstalled() bool { entries, err := os.ReadDir(filepath.Join(rt.AppRoot(), "kernels")) if err != nil { diff --git a/cmd/machine_test.go b/cmd/machine_test.go index 4cc6185..2fa1648 100644 --- a/cmd/machine_test.go +++ b/cmd/machine_test.go @@ -157,4 +157,11 @@ func TestParseCPUs(t *testing.T) { if _, _, err := parseCPUs("abc"); err == nil { t.Error("parseCPUs(abc) should error") } + // Non-finite floats parse via ParseFloat but must be rejected, not turned + // into --cpus 0 (NaN) or --cpus 9223372036854775807 (Inf). + for _, v := range []string{"NaN", "nan", "inf", "Inf", "+Inf", "infinity"} { + if n, _, err := parseCPUs(v); err == nil { + t.Errorf("parseCPUs(%q) should error; got n=%d", v, n) + } + } } diff --git a/cmd/run.go b/cmd/run.go index 4a121df..4d5d516 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -174,6 +174,11 @@ func parseCPUs(cv string) (n int, warning string, err error) { if perr != nil { return 0, "", fmt.Errorf("invalid --cpus value %q: must be a number", cv) } + // ParseFloat accepts inf/NaN; reject them before the round-up, which would + // otherwise emit --cpus 9223372036854775807 (Inf) or --cpus 0 (NaN). + if math.IsNaN(fv) || math.IsInf(fv, 0) { + return 0, "", fmt.Errorf("invalid --cpus value %q: must be a finite number", cv) + } if fv <= 0 { return 0, "", fmt.Errorf("invalid --cpus value %q: must be greater than 0", cv) } diff --git a/internal/dockerfmt/render.go b/internal/dockerfmt/render.go index b54233e..f4e6b40 100644 --- a/internal/dockerfmt/render.go +++ b/internal/dockerfmt/render.go @@ -73,6 +73,14 @@ var ( strLitRe = regexp.MustCompile("\"[^\"]*\"|`[^`]*`|'[^']*'") ) +// formatUnescaper turns the literal two-character escapes a shell passes in a +// single-quoted --format (\t, \n) into real tab/newline bytes, matching the +// Docker CLI. Without it, `--format 'table {{.A}}\t{{.B}}'` emits a literal +// "\t" and the tabwriter never sees a tab, so columns don't align. +var formatUnescaper = strings.NewReplacer(`\t`, "\t", `\n`, "\n") + +func unescapeFormat(s string) string { return formatUnescaper.Replace(s) } + // Render emits a list of view objects honouring Docker's -q / --format / // default-table conventions. func Render(format string, quiet bool, views []any, def TableDef) error { @@ -117,7 +125,7 @@ func Render(format string, quiet bool, views []any, def TableDef) error { return nil case strings.HasPrefix(format, "table"): - body := strings.TrimSpace(strings.TrimPrefix(format, "table")) + body := unescapeFormat(strings.TrimSpace(strings.TrimPrefix(format, "table"))) if body == "" { // `--format table` with no template => default table. return Render("", false, views, def) @@ -152,7 +160,7 @@ func Render(format string, quiet bool, views []any, def TableDef) error { return w.Flush() default: - tmpl, err := template.New("row").Funcs(tmplFuncs).Parse(format + "\n") + tmpl, err := template.New("row").Funcs(tmplFuncs).Parse(unescapeFormat(format) + "\n") if err != nil { return err } diff --git a/internal/dockerfmt/render_test.go b/internal/dockerfmt/render_test.go index 6485f99..0f3a5fe 100644 --- a/internal/dockerfmt/render_test.go +++ b/internal/dockerfmt/render_test.go @@ -122,6 +122,28 @@ func TestRenderTableHeaderDottedLiteral(t *testing.T) { } } +// TestRenderTableFormatUnescapesTabs reproduces the bug where a shell-supplied +// `--format 'table {{.Name}}\t{{.ID}}'` (literal backslash-t) emitted a literal +// "\t" and never aligned columns. The escape must become a real tab (matching +// docker), so no literal backslash-t survives in header or rows. +func TestRenderTableFormatUnescapesTabs(t *testing.T) { + views, def := sampleDef() + // The Go literal `\t` here is a real backslash + t — what a single-quoted + // shell argument delivers. + out := captureStdout(t, func() { Render(`table {{.Name}}\t{{.ID}}`, false, views, def) }) + if strings.Contains(out, `\t`) { + t.Errorf("literal backslash-t must be unescaped to a tab; got %q", out) + } + if !strings.Contains(out, "NAME") || !strings.Contains(out, "ID") { + t.Errorf("header columns missing: %q", out) + } + // The plain (non-table) template branch unescapes too. + out2 := captureStdout(t, func() { Render(`{{.Name}}\t{{.ID}}`, false, views, def) }) + if strings.Contains(out2, `\t`) { + t.Errorf("plain template must unescape backslash-t; got %q", out2) + } +} + // TestRenderPlainPathByteIdentical locks the drop-in contract: with styling // forced OFF, the default table is byte-for-byte the tabwriter output and // carries no ANSI/box-drawing characters — exactly what pipes and CI parse. From 01af7ee124280f489132b90164db51d087570f7d Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 12:12:48 +0530 Subject: [PATCH 15/17] fix: honor ignored compose-up flags, fix info/ps/images matching (pass 8) Recurring-class sweep (ignored flags, substring matches): - compose up now honors --no-log-prefix (foreground), --timeout (stop grace), and --pull always (force-refresh each service image before start). These were registered with real-behavior help but never read. (formatLogLine extracted for the prefix; followAndWait threads noPrefix + timeout.) - docker info reported the backend as 'running' when stopped: it used a substring match ("running" is inside "not running"). Use parseSystemStatus exact-match, like doctor. - ps --filter status=exited/created/dead returned nothing without -a (default fetch was running-only); a status= filter now forces the all-states fetch. - ps --filter ancestor=alpine matched superstrings like 'myalpine' (substring); match repo / repo:tag / full ref via ancestorMatches. - images REPO filter was built from the un-normalized ref, so 'images docker.io/library/alpine' returned empty; normalize via ShortImage. - compose port --protocol UDP was compared case-sensitively to the backend's lowercase proto. Pure helpers (hasStatusFilter/ancestorMatches/imageRefFilter/formatLogLine) covered by reproducing tests. --- cmd/compose.go | 52 +++++++++++++++++++++++++++++-------- cmd/compose_helpers_test.go | 14 ++++++++++ cmd/filters_test.go | 38 +++++++++++++++++++++++++++ cmd/images.go | 3 +++ cmd/ps.go | 26 +++++++++++++++++-- cmd/system.go | 9 ++++--- 6 files changed, 125 insertions(+), 17 deletions(-) diff --git a/cmd/compose.go b/cmd/compose.go index 7c10a9f..ff1396c 100644 --- a/cmd/compose.go +++ b/cmd/compose.go @@ -326,6 +326,27 @@ func composeUp() *cobra.Command { net := ensureNetwork(p) ensureVolumes(p) + // --pull always: force-refresh each selected service's explicit image + // before starting (missing/never rely on the backend's on-demand pull). + // Done once per image, off the per-replica bringUp path. + if pull, _ := cmd.Flags().GetString("pull"); pull == "always" { + pulled := map[string]bool{} + for _, name := range p.Order() { + if skipService(p, name, selected, active) { + continue + } + img := p.Services[name].Image + if img == "" || pulled[img] { + continue // built (no explicit image:) or already pulled + } + pulled[img] = true + fmt.Println(composeStep("Pulling", name)) + if _, err := runtime.CaptureSilent("image", "pull", img); err != nil { + fmt.Fprintf(os.Stderr, "dcon: warning: pull %s failed: %v\n", img, err) + } + } + } + // bringUp creates/starts every replica of one service and returns the // container names it brought up. Pure per-service work so independent // services can run concurrently. @@ -415,7 +436,10 @@ func composeUp() *cobra.Command { return nil } // Foreground: stream aggregated logs until interrupted, then stop. - return followAndWait(p, started) + noPrefix, _ := cmd.Flags().GetBool("no-log-prefix") + timeoutChanged := cmd.Flags().Changed("timeout") + timeout, _ := cmd.Flags().GetInt("timeout") + return followAndWait(p, started, noPrefix, timeoutChanged, timeout) }, } f := cmd.Flags() @@ -483,7 +507,7 @@ func removeOrphanContainers(p *compose.Project) { } } -func followAndWait(p *compose.Project, names []string) error { +func followAndWait(p *compose.Project, names []string, noPrefix bool, timeoutChanged bool, timeout int) error { sigc := make(chan os.Signal, 1) signal.Notify(sigc, os.Interrupt, syscall.SIGTERM) @@ -504,7 +528,7 @@ func followAndWait(p *compose.Project, names []string) error { sc := bufio.NewScanner(stdout) sc.Buffer(make([]byte, 1024*1024), 1024*1024) for sc.Scan() { - fmt.Printf("%s | %s\n", ui.Accent(svc), sc.Text()) + fmt.Println(formatLogLine(svc, sc.Text(), noPrefix)) } }() } @@ -513,7 +537,7 @@ func followAndWait(p *compose.Project, names []string) error { <-sigc fmt.Println("\nGracefully stopping...") for _, n := range names { - _, _ = runtime.CaptureSilent("stop", n) + _, _ = runtime.CaptureSilent(composeStopArgs(timeoutChanged, timeout, n)...) } for _, c := range cmds { _ = c.Process.Kill() @@ -757,14 +781,19 @@ func composeLogsTail(cmd *cobra.Command) string { return t } -// printLogLine writes one aggregated log line, with the Docker-style -// "service | …" prefix unless --no-log-prefix was given. -func printLogLine(svc, line string, noPrefix bool) { +// formatLogLine renders one aggregated log line with the Docker-style +// "service | …" prefix unless --no-log-prefix was given. Pure, so the prefix +// behavior is unit-testable. +func formatLogLine(svc, line string, noPrefix bool) string { if noPrefix { - fmt.Println(line) - } else { - fmt.Printf("%s | %s\n", ui.Accent(svc), line) + return line } + return ui.Accent(svc) + " | " + line +} + +// printLogLine writes one aggregated log line (see formatLogLine). +func printLogLine(svc, line string, noPrefix bool) { + fmt.Println(formatLogLine(svc, line, noPrefix)) } // followLogs streams `container logs --follow` for each target concurrently, @@ -1655,6 +1684,7 @@ func composePort() *cobra.Command { } idx, _ := cmd.Flags().GetInt("index") proto, _ := cmd.Flags().GetString("protocol") + proto = strings.ToLower(proto) // backend protos are lowercase; --protocol UDP must match cid, err := serviceContainerByIndex(p.Name, args[0], idx) if err != nil { return err @@ -1671,7 +1701,7 @@ func composePort() *cobra.Command { if pr == "" { pr = "tcp" } - if fmt.Sprint(pt.ContainerPort) != args[1] || pr != proto { + if fmt.Sprint(pt.ContainerPort) != args[1] || strings.ToLower(pr) != proto { continue } host := pt.HostAddress diff --git a/cmd/compose_helpers_test.go b/cmd/compose_helpers_test.go index 1ed333b..d9792d0 100644 --- a/cmd/compose_helpers_test.go +++ b/cmd/compose_helpers_test.go @@ -8,6 +8,7 @@ import ( "testing" "dcon/internal/compose" + "dcon/internal/ui" "github.com/spf13/cobra" ) @@ -80,6 +81,19 @@ func TestEffectiveReplicas(t *testing.T) { } } +// TestFormatLogLine reproduces the bug where `compose up --no-log-prefix` +// (foreground) still printed the "service | " prefix: followAndWait hardcoded +// it. The prefix must be suppressed when noPrefix is set. +func TestFormatLogLine(t *testing.T) { + defer ui.SetEnabled(false)() // deterministic (no color) Accent + if got := formatLogLine("web", "hello", true); got != "hello" { + t.Errorf("noPrefix -> %q, want \"hello\"", got) + } + if got := formatLogLine("web", "hello", false); got != "web | hello" { + t.Errorf("prefixed -> %q, want \"web | hello\"", got) + } +} + // TestComposeConfigQuiet reproduces the bug where `compose config -q` printed // the full rendered config instead of validating silently. With -q it must // print nothing on a valid file (and still validate, via loadProject). diff --git a/cmd/filters_test.go b/cmd/filters_test.go index e42d26d..2ddc65f 100644 --- a/cmd/filters_test.go +++ b/cmd/filters_test.go @@ -98,6 +98,9 @@ func TestImageRefFilter(t *testing.T) { {"registry:5000/myimage", "registry:5000/myimage", "", ""}, // port, not a tag {"registry:5000/myimage:1.0", "registry:5000/myimage", "1.0", ""}, // real tag kept {"alpine@sha256:abc", "alpine", "", "sha256:abc"}, // digest, not tag + {"docker.io/library/alpine", "alpine", "", ""}, // fully-qualified -> short + {"docker.io/library/alpine:3.18", "alpine", "3.18", ""}, + {"docker.io/myorg/app", "myorg/app", "", ""}, } for _, c := range cases { repo, tag, dg := imageRefFilter(c.ref) @@ -108,6 +111,41 @@ func TestImageRefFilter(t *testing.T) { } } +// TestHasStatusFilter reproduces the bug where `ps --filter status=exited` +// without -a fetched only running containers; a status= filter must force the +// all-states fetch. +func TestHasStatusFilter(t *testing.T) { + if !hasStatusFilter([]string{"status=exited"}) { + t.Error("status=exited should require all-states fetch") + } + if !hasStatusFilter([]string{"label=a=b", "status=running"}) { + t.Error("status=running among others should be detected") + } + if hasStatusFilter([]string{"label=a=b", "name=x"}) { + t.Error("no status filter -> false") + } +} + +// TestAncestorMatches reproduces the bug where `ps --filter ancestor=alpine` +// used a loose substring and matched superstrings like "myalpine". +func TestAncestorMatches(t *testing.T) { + // matches: exact repo, repo:tag, full ref, docker.io short form + for _, ref := range []string{"alpine:latest", "alpine:3.18", "docker.io/library/alpine:latest"} { + if !ancestorMatches(ref, "alpine") { + t.Errorf("ancestor=alpine should match image %q", ref) + } + } + if !ancestorMatches("alpine:3.18", "alpine:3.18") { + t.Error("ancestor=alpine:3.18 should match repo:tag") + } + // must NOT match superstring repos + for _, ref := range []string{"myalpine:latest", "alpine-test:1", "notalpine:latest"} { + if ancestorMatches(ref, "alpine") { + t.Errorf("ancestor=alpine must NOT match %q (substring)", ref) + } + } +} + // TestValidateImageFilters confirms a malformed reference= glob errors loudly // instead of silently hiding every image. func TestValidateImageFilters(t *testing.T) { diff --git a/cmd/images.go b/cmd/images.go index 3040c04..1e3922a 100644 --- a/cmd/images.go +++ b/cmd/images.go @@ -176,6 +176,9 @@ func runImages(cmd *cobra.Command, args []string) error { // other tag of that repo. A digest ref filters by the digest column, never by // the human tag (which is never a digest), which otherwise yields an empty list. func imageRefFilter(ref string) (repo, tag, digest string) { + // Normalize like buildImageView (which stores Repository via ShortImage), so a + // fully-qualified docker.io/library/alpine matches the stored "alpine". + ref = dockerfmt.ShortImage(ref) if i := strings.LastIndex(ref, "@"); i >= 0 { return ref[:i], "", ref[i+1:] } diff --git a/cmd/ps.go b/cmd/ps.go index f746514..bb911f6 100644 --- a/cmd/ps.go +++ b/cmd/ps.go @@ -180,6 +180,26 @@ func buildPsView(c dockerfmt.Container, noTrunc bool) psView { } } +// hasStatusFilter reports whether any --filter is a status= predicate, so ps +// knows to fetch all states (not just running) before filtering. +func hasStatusFilter(filters []string) bool { + for _, fl := range filters { + if strings.HasPrefix(fl, "status=") { + return true + } + } + return false +} + +// ancestorMatches implements docker's `--filter ancestor=` against an image +// reference: it matches the repo, the repo:tag, or the full reference — NOT a +// loose substring (which wrongly matched `myalpine` for ancestor=alpine). +func ancestorMatches(reference, val string) bool { + short := dockerfmt.ShortImage(reference) + repo, tag := dockerfmt.SplitRepoTag(short) + return val == reference || val == short || val == repo || val == repo+":"+tag +} + // applyFilters implements the common docker ps --filter predicates. func applyFilters(list []dockerfmt.Container, filters []string) []dockerfmt.Container { if len(filters) == 0 { @@ -208,7 +228,7 @@ func applyFilters(list []dockerfmt.Container, filters []string) []dockerfmt.Cont keep = false } case "ancestor": - if !strings.Contains(c.Configuration.Image.Reference, val) { + if !ancestorMatches(c.Configuration.Image.Reference, val) { keep = false } case "label": @@ -258,7 +278,9 @@ func newPsCmd() *cobra.Command { last, _ := cmd.Flags().GetInt("last") latest, _ := cmd.Flags().GetBool("latest") - list, err := getContainers(all || latest || last > 0) + // A status= filter must see all states, else `ps --filter status=exited` + // (without -a) fetches only running containers and matches nothing. + list, err := getContainers(all || latest || last > 0 || hasStatusFilter(filters)) if err != nil { return err } diff --git a/cmd/system.go b/cmd/system.go index 1df5834..7400ee6 100644 --- a/cmd/system.go +++ b/cmd/system.go @@ -160,11 +160,12 @@ func newInfoCmd() *cobra.Command { imgs, _ := getImages() ver := backendVersion() - // status check + // status check — exact field match (like doctor), not a substring: + // "running" is a substring of "not running". statusOut, _ := rt.CaptureSilent("system", "status") - serverState := "running" - if !strings.Contains(statusOut, "running") { - serverState = "stopped" + serverState := "stopped" + if parseSystemStatus(statusOut)["status"] == "running" { + serverState = "running" } if format, _ := cmd.Flags().GetString("format"); format != "" { From b4c54837791f7bc70c95f7665ea730c5413fc5d8 Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 23:32:12 +0530 Subject: [PATCH 16/17] fix: address CodeRabbit review (pass 9) Resolve the actionable CodeRabbit findings on the red-team sweep: - compose up: short-circuit the foreground path when zero containers start (`up --scale svc=0` no longer hangs on the signal channel) - run/create: reject the reserved `dcon-machine-` name prefix and `dcon.machine*` labels so `dcon run` can't forge a container that `dcon machine stop` would resolve (confused-deputy fix) - ps --filter ancestor: normalize the filter value too, so a fully qualified ancestor matches a shortened stored ref (and vice-versa) - volume create: an explicit empty --name now errors instead of silently creating an anonymous volume - compose: route long-form `type: tmpfs` volumes to --tmpfs instead of a disk-backed anonymous --volume - compose: external named volumes reference their exact name, never the project-prefixed one - pool: destroy reaped VMs only after the state removal is persisted; a failed save() no longer tears down still-claimable members Tests: machine-namespace rejection, ancestor normalization, empty --name, tmpfs routing, external volumes, MapList label empty-key drop, plus deterministic unset-var fixtures and a root-proof pool save-failure case. --- cmd/cli_fixes_test.go | 5 ++++ cmd/compose.go | 5 +++- cmd/compose_helpers_test.go | 13 +++++++++++ cmd/filters_test.go | 8 +++++++ cmd/ps.go | 10 +++++--- cmd/run.go | 19 +++++++++++++++ cmd/translate_test.go | 27 ++++++++++++++++++++-- cmd/volume.go | 5 +++- internal/compose/model.go | 10 ++++++++ internal/compose/model_test.go | 31 ++++++++++++++++++++++++- internal/compose/translate.go | 12 ++++++++++ internal/compose/translate_test.go | 37 +++++++++++++++++++++++++++--- internal/pool/pool.go | 9 ++++++-- internal/pool/pool_test.go | 16 ++++++------- 14 files changed, 186 insertions(+), 21 deletions(-) diff --git a/cmd/cli_fixes_test.go b/cmd/cli_fixes_test.go index ca39387..502636e 100644 --- a/cmd/cli_fixes_test.go +++ b/cmd/cli_fixes_test.go @@ -146,6 +146,11 @@ func TestResolveVolumeName(t *testing.T) { 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 diff --git a/cmd/compose.go b/cmd/compose.go index ff1396c..0a43885 100644 --- a/cmd/compose.go +++ b/cmd/compose.go @@ -432,7 +432,10 @@ func composeUp() *cobra.Command { if removeOrphans { removeOrphanContainers(p) } - if detach || noStart { + // len(started)==0 also short-circuits: effectiveReplicas can legally + // return 0 (e.g. `up --scale web=0`), and followAndWait would otherwise + // print the attach banner and block on the signal channel forever. + if detach || noStart || len(started) == 0 { return nil } // Foreground: stream aggregated logs until interrupted, then stop. diff --git a/cmd/compose_helpers_test.go b/cmd/compose_helpers_test.go index d9792d0..bf5122e 100644 --- a/cmd/compose_helpers_test.go +++ b/cmd/compose_helpers_test.go @@ -121,6 +121,19 @@ func TestComposeConfigQuiet(t *testing.T) { if out := run(nil); !strings.Contains(out, "nginx") { t.Errorf("config (no -q) should render the config; got %q", out) } + // -q suppresses output but must STILL validate: an invalid compose file has + // to error even in quiet mode (a -q that returned before loadProject would + // wrongly report success). + if err := os.WriteFile(filepath.Join(dir, "compose.yaml"), + []byte("services:\n web:\n image: [\n"), 0o644); err != nil { + t.Fatal(err) + } + cmd := composeConfig() + cmd.SetArgs([]string{"-q"}) + cmd.SilenceUsage, cmd.SilenceErrors = true, true + if err := cmd.Execute(); err == nil { + t.Fatal("config -q must still validate and fail on an invalid compose.yaml") + } } func TestServiceSet(t *testing.T) { diff --git a/cmd/filters_test.go b/cmd/filters_test.go index 2ddc65f..fbd10a1 100644 --- a/cmd/filters_test.go +++ b/cmd/filters_test.go @@ -138,6 +138,14 @@ func TestAncestorMatches(t *testing.T) { if !ancestorMatches("alpine:3.18", "alpine:3.18") { t.Error("ancestor=alpine:3.18 should match repo:tag") } + // A fully-qualified ancestor filter must still match a shortened stored ref + // (and vice-versa): both sides are normalized before comparison. + if !ancestorMatches("alpine:latest", "docker.io/library/alpine") { + t.Error("fully-qualified ancestor should match a shortened image ref") + } + if !ancestorMatches("docker.io/library/alpine:latest", "alpine") { + t.Error("short ancestor should match a fully-qualified image ref") + } // must NOT match superstring repos for _, ref := range []string{"myalpine:latest", "alpine-test:1", "notalpine:latest"} { if ancestorMatches(ref, "alpine") { diff --git a/cmd/ps.go b/cmd/ps.go index bb911f6..8c47d69 100644 --- a/cmd/ps.go +++ b/cmd/ps.go @@ -195,9 +195,13 @@ func hasStatusFilter(filters []string) bool { // reference: it matches the repo, the repo:tag, or the full reference — NOT a // loose substring (which wrongly matched `myalpine` for ancestor=alpine). func ancestorMatches(reference, val string) bool { - short := dockerfmt.ShortImage(reference) - repo, tag := dockerfmt.SplitRepoTag(short) - return val == reference || val == short || val == repo || val == repo+":"+tag + // Normalize both sides: a fully-qualified filter (docker.io/library/alpine) + // must still match a container whose stored ref is already shortened (alpine), + // and vice-versa, so shorten val too before comparing. + shortRef := dockerfmt.ShortImage(reference) + shortVal := dockerfmt.ShortImage(val) + repo, tag := dockerfmt.SplitRepoTag(shortRef) + return shortVal == shortRef || shortVal == repo || shortVal == repo+":"+tag } // applyFilters implements the common docker ps --filter predicates. diff --git a/cmd/run.go b/cmd/run.go index 4d5d516..0135b56 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -8,6 +8,7 @@ import ( "strconv" "strings" + "dcon/internal/machine" "dcon/internal/runtime" "github.com/spf13/cobra" @@ -196,6 +197,24 @@ func buildContainerArgs(cmd *cobra.Command, posArgs []string, subcmd string) ([] out := []string{subcmd} var warnings []string + // Guard dcon's machine namespace: a container whose name carries the + // reserved `dcon-machine-` prefix AND the `dcon.machine` label satisfies + // matchMachine (see cmd/machine.go), so `dcon run --name dcon-machine-foo + // --label dcon.machine=1` could make `dcon machine stop foo` act on the + // user's container instead of the real machine. Reject both reserved inputs. + if name, _ := f.GetString("name"); strings.HasPrefix(name, machine.ContainerName("")) { + return nil, fmt.Errorf("container name %q uses the %q prefix reserved by dcon machine", name, machine.ContainerName("")) + } + for _, l := range mustStringArray(f, "label") { + key := l + if i := strings.IndexByte(l, '='); i >= 0 { + key = l[:i] + } + if key == machine.LabelMachine || strings.HasPrefix(key, machine.LabelMachine+".") { + return nil, fmt.Errorf("label %q is reserved by dcon machine and cannot be set on run", key) + } + } + // passthrough bool flag -> container flag boolMap := []struct{ name, flag string }{ {"detach", "--detach"}, {"interactive", "--interactive"}, {"tty", "--tty"}, diff --git a/cmd/translate_test.go b/cmd/translate_test.go index d987061..c17cf1c 100644 --- a/cmd/translate_test.go +++ b/cmd/translate_test.go @@ -164,12 +164,35 @@ func TestRunMountValuelessTmpfsKeyNoPanic(t *testing.T) { if err != nil { t.Fatalf("%q: unexpected error %v", spec, err) } - if !contains(got, "--mount") { - t.Errorf("%q: expected a --mount arg; got %v", spec, got) + if !containsPair(got, "--mount", spec) { + t.Errorf("%q: expected the --mount payload passed through untouched; got %v", spec, got) } } } +// TestRunRejectsMachineNamespace verifies `dcon run` cannot forge a container +// that `dcon machine` would resolve: the reserved `dcon-machine-` name prefix +// and the `dcon.machine*` labels are rejected, closing the confused-deputy hole +// where `machine stop foo` could act on a user container (see cmd/run.go). +func TestRunRejectsMachineNamespace(t *testing.T) { + cases := [][]string{ + {"--name", "dcon-machine-foo", "alpine"}, + {"--label", "dcon.machine=1", "alpine"}, + {"--label", "dcon.machine.name=foo", "alpine"}, + } + for _, cli := range cases { + c := parse(t, newRunCmd(), cli) + if _, err := buildContainerArgs(c, c.Flags().Args(), "run"); err == nil { + t.Errorf("args %v: expected rejection of the reserved machine namespace", cli) + } + } + // A normal name/label must still work. + c := parse(t, newRunCmd(), []string{"--name", "web", "--label", "env=prod", "alpine"}) + if _, err := buildContainerArgs(c, c.Flags().Args(), "run"); err != nil { + t.Errorf("ordinary name/label must be accepted; got %v", err) + } +} + func TestBuildTranslation(t *testing.T) { c := newBuildCmd() parse(t, c, []string{ diff --git a/cmd/volume.go b/cmd/volume.go index 46d4608..98cac6f 100644 --- a/cmd/volume.go +++ b/cmd/volume.go @@ -31,7 +31,10 @@ func resolveVolumeName(flagName string, nameChanged bool, args []string) (string } return args[0], nil } - if flagName != "" { + if nameChanged { + if flagName == "" { + return "", fmt.Errorf("volume name cannot be empty") + } return flagName, nil } return randomName(), nil diff --git a/internal/compose/model.go b/internal/compose/model.go index 4ad077f..4b2de9a 100644 --- a/internal/compose/model.go +++ b/internal/compose/model.go @@ -378,7 +378,17 @@ func (v *VolumeList) UnmarshalYAML(value *yaml.Node) error { return nil } +// tmpfsVolumeMarker prefixes a flattened long-form volume whose type is tmpfs so +// RunArgs emits `--tmpfs ` instead of `--volume`. A NUL byte can't occur +// in a real mount spec, so it can't collide. Without this, `{type: tmpfs, target: +// /cache}` flattened to a bare "/cache" and became a disk-backed anonymous volume +// — silently losing the in-memory, ephemeral tmpfs semantics. +const tmpfsVolumeMarker = "\x00tmpfs\x00" + func flattenLongVolume(m map[string]string) string { + if m["type"] == "tmpfs" { + return tmpfsVolumeMarker + m["target"] + } s := m["target"] if src := m["source"]; src != "" { s = src + ":" + s diff --git a/internal/compose/model_test.go b/internal/compose/model_test.go index c8b6a20..aa207d8 100644 --- a/internal/compose/model_test.go +++ b/internal/compose/model_test.go @@ -276,6 +276,10 @@ services: - "GOOD=1" - "" - "=orphan" + labels: + - "owner=team" + - "" + - "=orphan" ` if err := os.WriteFile(path, []byte(yaml), 0o644); err != nil { t.Fatal(err) @@ -291,6 +295,14 @@ services: if env["GOOD"] != "1" || len(env) != 1 { t.Errorf("environment = %v, want only GOOD=1", env) } + // labels go through MapList (not EnvMap); blank/keyless entries must drop too. + labels := p.Services["web"].Labels + if _, bad := labels[""]; bad { + t.Errorf("empty-key label entry leaked: %v", labels) + } + if labels["owner"] != "team" || len(labels) != 1 { + t.Errorf("labels = %v, want only owner=team", labels) + } } // TestEnvBareKeyHostPassthrough reproduces the bug where a bare environment key @@ -299,6 +311,7 @@ services: // omitted, not emitted as FOO="". func TestEnvBareKeyHostPassthrough(t *testing.T) { t.Setenv("DCON_TEST_PASSTHRU", "from-host") + mustUnsetForTest(t, "DCON_UNSET_XYZ") // assertion below assumes it is unset dir := t.TempDir() path := filepath.Join(dir, "compose.yaml") yaml := ` @@ -377,6 +390,7 @@ services: func TestExpandVar(t *testing.T) { t.Setenv("SETV", "x") t.Setenv("EMPTYV", "") + mustUnsetForTest(t, "UNSETV") // cases below assume UNSETV is absent from the env // (key, want, wantErr) cases := []struct { key string @@ -415,14 +429,29 @@ func TestExpandVar(t *testing.T) { // TestLoadRequiredVarErrors confirms a required-but-unset ${VAR:?} aborts Load. func TestLoadRequiredVarErrors(t *testing.T) { + mustUnsetForTest(t, "DCON_REQ_UNSET") // the required-var error depends on it being unset dir := t.TempDir() path := filepath.Join(dir, "compose.yaml") - os.WriteFile(path, []byte("services:\n a:\n image: img:${DCON_REQ_UNSET:?tag required}\n"), 0o644) + if err := os.WriteFile(path, []byte("services:\n a:\n image: img:${DCON_REQ_UNSET:?tag required}\n"), 0o644); err != nil { + t.Fatal(err) + } if _, err := Load(path, ""); err == nil { t.Error("Load should fail when a required ${VAR:?} variable is unset") } } +// mustUnsetForTest makes key deterministically absent for the duration of the +// test, restoring whatever the host had afterward. t.Setenv records the original +// value (and its restore), then Unsetenv clears it so assertions about an unset +// variable don't depend on the runner's environment. +func mustUnsetForTest(t *testing.T, key string) { + t.Helper() + t.Setenv(key, "") + if err := os.Unsetenv(key); err != nil { + t.Fatalf("unset %s: %v", key, err) + } +} + func TestServiceEnabledProfiles(t *testing.T) { noProf := &Service{} if !noProf.Enabled(map[string]bool{}) { diff --git a/internal/compose/translate.go b/internal/compose/translate.go index 28b1267..f179360 100644 --- a/internal/compose/translate.go +++ b/internal/compose/translate.go @@ -209,6 +209,12 @@ func (p *Project) runArgs(service string, svc *Service, index int, netName strin args = append(args, "--publish", normalizePort(port)) } for _, vol := range svc.Volumes { + // A long-form `type: tmpfs` mount is routed to --tmpfs, not --volume, + // to preserve its in-memory semantics (see tmpfsVolumeMarker). + if target, ok := strings.CutPrefix(vol, tmpfsVolumeMarker); ok { + args = append(args, "--tmpfs", target) + continue + } args = append(args, "--volume", p.resolveVolume(vol)) } @@ -406,6 +412,12 @@ func (p *Project) VolumeName(key string, spec *VolumeSpec) string { if spec != nil && spec.Name != "" { return spec.Name } + // An external volume already exists outside the project; reference it by its + // exact key, never the project-prefixed name (which would mount/create a + // different volume than the one the user declared external). + if spec != nil && spec.External { + return key + } return p.Name + "_" + key } diff --git a/internal/compose/translate_test.go b/internal/compose/translate_test.go index dc74bff..39463cd 100644 --- a/internal/compose/translate_test.go +++ b/internal/compose/translate_test.go @@ -342,6 +342,26 @@ func TestFlattenLongVolumeReadOnlyCasing(t *testing.T) { } } +// TestFlattenLongVolumeTmpfs reproduces the bug where a long-form +// `{type: tmpfs, target: /cache}` mount was flattened to a bare "/cache" and +// emitted as a disk-backed anonymous --volume, silently dropping tmpfs +// semantics. It must instead route to --tmpfs. +func TestFlattenLongVolumeTmpfs(t *testing.T) { + got := flattenLongVolume(map[string]string{"type": "tmpfs", "target": "/cache"}) + if got != tmpfsVolumeMarker+"/cache" { + t.Fatalf("tmpfs flatten -> %q, want marker+/cache", got) + } + p := &Project{Name: "proj", Dir: "/tmp/proj"} + svc := &Service{Image: "alpine", Volumes: VolumeList{got}} + args := p.RunArgs("web", svc, 1, "", nil) + mustContainPair(t, args, "--tmpfs", "/cache") + for _, a := range args { + if a == "--volume" { + t.Errorf("tmpfs mount must not emit a --volume; got %v", args) + } + } +} + // TestOneOffArgsKeepsEntrypointExtrasWithCmdOverride reproduces the bug where a // command override dropped the service entrypoint's extra tokens when the // entrypoint itself was NOT overridden. `compose run web shell` on a service @@ -369,13 +389,22 @@ func TestOneOffArgsKeepsEntrypointExtrasWithCmdOverride(t *testing.T) { // container got a different volume than declared. func TestResolveVolumeNamedVolume(t *testing.T) { p := &Project{ - Name: "proj", - Dir: "/tmp/proj", - Volumes: map[string]*VolumeSpec{"data": {}, "named": {Name: "custom"}}, + Name: "proj", + Dir: "/tmp/proj", + Volumes: map[string]*VolumeSpec{ + "data": {}, + "named": {Name: "custom"}, + // An external volume must reference its exact name, never the + // project-prefixed one: `extkey` by key, `xname` by explicit name. + "extkey": {External: true}, + "xname": {External: true, Name: "shared-cache"}, + }, } svc := &Service{Image: "x", Volumes: VolumeList{ "data:/var/lib", // declared -> proj_data "named:/n", // declared with explicit name -> custom + "extkey:/x", // external, no name -> extkey (no project prefix) + "xname:/c", // external with explicit name -> shared-cache "ext:/e", // undeclared -> passthrough "/abs:/a", // absolute bind -> passthrough "./rel:/r", // relative bind -> resolved to absolute @@ -383,6 +412,8 @@ func TestResolveVolumeNamedVolume(t *testing.T) { args := p.RunArgs("web", svc, 1, "", nil) mustContainPair(t, args, "--volume", "proj_data:/var/lib") mustContainPair(t, args, "--volume", "custom:/n") + mustContainPair(t, args, "--volume", "extkey:/x") + mustContainPair(t, args, "--volume", "shared-cache:/c") mustContainPair(t, args, "--volume", "ext:/e") mustContainPair(t, args, "--volume", "/abs:/a") if !containsSub(args, "/tmp/proj/rel:/r") { diff --git a/internal/pool/pool.go b/internal/pool/pool.go index abb1f83..2f6de86 100644 --- a/internal/pool/pool.go +++ b/internal/pool/pool.go @@ -238,10 +238,15 @@ func ReapStale() { // VM out from under the live exec. Doing the removal inside the lock means a // concurrent Claim and a reap can never both own the same member. var stale []Member - _ = withLock(func(s *state) error { + // Destroy only after save() persists the removal. withLock can run the + // callback then fail to write pool.json; ignoring that and destroying anyway + // would tear down VMs still listed as available (and thus still claimable). + if err := withLock(func(s *state) error { stale, s.Members = partitionStale(s.Members, cutoff) return nil - }) + }); err != nil { + return + } for _, m := range stale { DestroyAsync(m.ID) } diff --git a/internal/pool/pool_test.go b/internal/pool/pool_test.go index 5be2cd7..420ad37 100644 --- a/internal/pool/pool_test.go +++ b/internal/pool/pool_test.go @@ -12,9 +12,6 @@ import ( // (two runs exec'ing into one microVM). On a persist failure Claim must report a // miss (so the caller cold-runs) and leave the member available. func TestClaimMissOnSaveFailure(t *testing.T) { - if os.Geteuid() == 0 { - t.Skip("running as root bypasses directory permissions") - } home := t.TempDir() t.Setenv("HOME", home) t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) @@ -22,21 +19,24 @@ func TestClaimMissOnSaveFailure(t *testing.T) { if err := Add(Member{ID: "vm1", Image: NormalizeRef("alpine")}); err != nil { t.Fatalf("seed Add: %v", err) } - d, err := dir() + p, err := statePath() if err != nil { t.Fatal(err) } - // Read-only state dir => save()'s temp-file write fails inside withLock. - if err := os.Chmod(d, 0o500); err != nil { + // A directory at the temp-file path makes save()'s os.WriteFile fail + // deterministically for every user (EISDIR) — unlike a chmod, which root + // bypasses. save() writes pool.json.tmp then renames it into place. + tmp := p + ".tmp" + if err := os.Mkdir(tmp, 0o755); err != nil { t.Fatal(err) } - defer os.Chmod(d, 0o755) //nolint:errcheck // restore for temp-dir cleanup + defer os.RemoveAll(tmp) //nolint:errcheck // restore for temp-dir cleanup if _, ok := Claim("alpine"); ok { t.Error("Claim must report a miss when the state write fails") } // Restore and confirm the member was NOT lost (still claimable later). - if err := os.Chmod(d, 0o755); err != nil { + if err := os.RemoveAll(tmp); err != nil { t.Fatal(err) } if got := AvailableDepth("alpine"); got != 1 { From 13651a1864a206c8dd4e2712a5ec5cced83e3459 Mon Sep 17 00:00:00 2001 From: Karthik Vinayan Date: Thu, 25 Jun 2026 23:32:21 +0530 Subject: [PATCH 17/17] docs: document ui/machine packages, refresh binary size Add the internal/ui and internal/machine architecture notes to CLAUDE.md and update the make build size estimate (~5.3 MB -> ~6.2 MB). --- CLAUDE.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3c2100a..e7abde8 100644 --- a/CLAUDE.md +++ b/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 2147483647` with labels `dcon.machine=1`/`.name`/`.distro`, named `dcon-machine-` 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.