fix: red-team bug-fix sweep (~50 bugs, 49 new tests)#5
Merged
Conversation
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-<name>, 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.
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.
…ed 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.
…ef 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.
…ness 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.
- 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 <container> <image>' 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.
…ly used 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.
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.
…ssions)
- 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).
- 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).
… flags
- 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 '<project>_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).
…`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.
…s, 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.
…s 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.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR updates CLI parsing and command wiring, revises Compose model and translation behavior, changes Docker-format rendering, and adjusts pool stale-member reaping and claim handling. Several tests were added or updated for the affected paths. ChangesCLI, Compose, and Runtime Changes
Compose Command Flow
Compose Model and Translation
Docker Format Rendering
Pool Stale Handling
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)CLAUDE.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
cmd/compose_helpers_test.go (1)
97-124: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a failing-config case for
config -q.This test proves quiet mode suppresses stdout for valid input, but it would still pass if
-qreturned before validation. Add an invalid compose file assertion so the “still validate” contract is covered.Proposed test coverage addition
if out := run(nil); !strings.Contains(out, "nginx") { t.Errorf("config (no -q) should render the config; got %q", out) } + 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 should still validate and fail on invalid compose.yaml") + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/compose_helpers_test.go` around lines 97 - 124, `TestComposeConfigQuiet` only checks that `composeConfig` suppresses output on valid input, but not that `-q` still validates the compose file. Extend this test with a failing-compose case using the same `composeConfig()`/`cmd.Execute()` path and assert an invalid `compose.yaml` returns an error even when `-q` is set, so the quiet flag cannot bypass validation.cmd/translate_test.go (1)
167-169: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the mount value, not just the flag.
The test says the valueless key must pass through untouched, but it would still pass if the implementation dropped or rewrote the mount payload.
Proposed test tightening
- if !contains(got, "--mount") { - t.Errorf("%q: expected a --mount arg; got %v", spec, got) + if !containsPair(got, "--mount", spec) { + t.Errorf("%q: expected unchanged --mount payload; got %v", spec, got) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/translate_test.go` around lines 167 - 169, The test in translate_test should verify the full mount argument, not only the presence of the "--mount" flag. Update the assertion around the spec handling in the translate test to check the exact mount payload/value produced by the translate logic, so it fails if the implementation drops, rewrites, or alters the argument while still keeping the same flag name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/compose.go`:
- Around line 439-442: Short-circuit the foreground compose up flow when no
containers were started, since effectiveReplicas() can now return 0 and
followAndWait() will otherwise hang on the signal channel. In the command path
that currently unconditionally calls followAndWait(p, started, noPrefix,
timeoutChanged, timeout), add an early return when started is empty so --scale
svc=0 exits cleanly. Apply the same guard in the other foreground up path that
also delegates to followAndWait, keeping the behavior consistent across both
call sites.
In `@cmd/machine.go`:
- Around line 120-127: `dcon run` is allowing users to create containers that
match the machine namespace, which can be picked up by `matchMachine` and
interfere with `machine stop`. Update the name/label validation in `cmd/run.go`
to import and use `internal/machine`’s `ValidateName` so machine-style names are
rejected, and also block any `dcon.machine` label from being set on run-created
containers. Ensure the checks happen where the container name and labels are
assembled before the container is created.
In `@cmd/misc.go`:
- Around line 186-190: The cpIsContainerRef helper is still misclassifying
native Windows drive-letter paths as container refs because it only excludes
absolute and dot-prefixed paths before checking for a colon. Update
cpIsContainerRef to explicitly recognize Windows local paths such as
drive-letter formats before the strings.IndexByte(p, ':') check, and add a
regression case in TestCpIsContainerRef for C:\tmp\file and C:/tmp/file to
ensure they are treated as local paths.
In `@cmd/ps.go`:
- Around line 197-200: Normalize the ancestor filter value in ancestorMatches
before comparing, since only the reference is currently passed through
dockerfmt.ShortImage and fully qualified ancestor values like
docker.io/library/alpine can miss matches. Update ancestorMatches to derive a
shortened form for val as well, then compare reference, short, repo, and
repo:tag against both normalized forms so ancestor filters work for stored
shortened image refs.
In `@cmd/volume.go`:
- Around line 27-37: The resolveVolumeName helper currently falls through to
randomName() when --name was explicitly provided but empty, so update the
flag/args handling in resolveVolumeName to treat nameChanged as an explicit
input even when flagName is empty and return an error instead of generating a
volume name. Keep the existing conflict check for positional args vs --name, and
add a regression test covering resolveVolumeName("", true, nil) to assert that
an empty explicit name is rejected rather than producing an anonymous volume.
In `@internal/compose/model_test.go`:
- Around line 265-293: The test currently only exercises Service.Environment via
EnvMap, so it no longer protects the MapList.UnmarshalYAML empty-key behavior.
Update TestMapListSkipsEmptyKeys to include a labels: case (or rename it to
reflect EnvMap coverage) and assert that blank or keyless entries are dropped
through MapList so the regression remains covered.
- Around line 300-311: Harden the unset-variable tests in the compose model
tests by making the host environment deterministic and verifying the fixture
file write. In TestEnvBareKeyHostPassthrough and the related unset-var cases
(including the other referenced tests), explicitly clear or isolate
DCON_UNSET_XYZ, UNSETV, and DCON_REQ_UNSET before calling Load so assertions
don’t depend on the runner’s environment. Also check the result of os.WriteFile
when creating the compose.yaml fixture so the test fails immediately if the file
was not written, using the existing test helpers and Load call sites as anchors.
In `@internal/compose/model.go`:
- Around line 381-392: The flattenLongVolume helper is dropping the mount type,
so a long-form Compose mount with type set to tmpfs gets converted into a normal
volume instead of preserving tmpfs behavior. Update flattenLongVolume and the
surrounding mount conversion logic in internal/compose/model.go to either route
type: tmpfs through the existing tmpfs handling path or explicitly reject it,
and make sure the logic still uses source, target, and read_only correctly for
non-tmpfs mounts.
- Around line 510-524: The interpolation in model loading is happening on the
raw YAML string, so required-variable errors from os.Expand can be triggered by
comments or single-quoted literals that should stay literal. Update the
Load/expansion flow around expandVar and os.Expand in model.go to interpolate
only parsed scalar nodes (or otherwise skip comments and single-quoted values)
before propagating interpErr, so required vars are enforced only where Compose
actually treats them as expandable.
In `@internal/compose/translate.go`:
- Around line 353-355: The volume translation in translate.go is incorrectly
prefixing external named volumes via Project.VolumeName, causing mount name
collisions. Update the logic around the p.Volumes lookup and the VolumeName
helper so that when a VolumeSpec is marked external, it returns the explicit
spec.Name if present, otherwise the raw volume key unchanged, instead of
applying the project-name prefix. Keep the existing behavior only for
non-external declared volumes and use the VolumeName/Volumes lookup path to
distinguish these cases.
In `@internal/pool/pool_test.go`:
- Around line 15-40: The regression test in pool_test.go currently depends on
directory permissions and skips under root, so make it deterministic for all
users by forcing save() to fail via the temp-file path instead. Update the
Claim/Add setup around dir(), withLock/save(), and the os.Chmod-based failure
simulation so that pool.json.tmp is created as a directory before calling
Claim("alpine"), then restore cleanup and keep the assertion that the member is
still claimable afterward.
In `@internal/pool/pool.go`:
- Around line 241-247: The reaping path in partitionStale/withLock is destroying
VMs before the state change is guaranteed to persist. Update the stale-member
removal flow so the result from withLock is checked and DestroyAsync is only
called after save(s) succeeds and pool.json has been updated; if persistence
fails, do not destroy any VMs. Use the existing symbols withLock,
partitionStale, DestroyAsync, and state/Members to keep the fix localized.
---
Nitpick comments:
In `@cmd/compose_helpers_test.go`:
- Around line 97-124: `TestComposeConfigQuiet` only checks that `composeConfig`
suppresses output on valid input, but not that `-q` still validates the compose
file. Extend this test with a failing-compose case using the same
`composeConfig()`/`cmd.Execute()` path and assert an invalid `compose.yaml`
returns an error even when `-q` is set, so the quiet flag cannot bypass
validation.
In `@cmd/translate_test.go`:
- Around line 167-169: The test in translate_test should verify the full mount
argument, not only the presence of the "--mount" flag. Update the assertion
around the spec handling in the translate test to check the exact mount
payload/value produced by the translate logic, so it fails if the implementation
drops, rewrites, or alters the argument while still keeping the same flag name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a5bdf44-d9af-497c-bb88-9a392bde75a3
📒 Files selected for processing (28)
cmd/build.gocmd/cli_fixes_test.gocmd/compose.gocmd/compose_helpers_test.gocmd/doctor.gocmd/filters_test.gocmd/history.gocmd/images.gocmd/lifecycle.gocmd/machine.gocmd/machine_test.gocmd/misc.gocmd/network.gocmd/parity_test.gocmd/ps.gocmd/run.gocmd/stats.gocmd/system.gocmd/translate_test.gocmd/volume.gointernal/compose/model.gointernal/compose/model_test.gointernal/compose/translate.gointernal/compose/translate_test.gointernal/dockerfmt/render.gointernal/dockerfmt/render_test.gointernal/pool/pool.gointernal/pool/pool_test.go
Comment on lines
+510
to
+524
| // 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 { | ||
| // 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 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Avoid expanding required vars across raw YAML text.
Because os.Expand runs before YAML parsing, ${MISSING:?msg} inside comments or single-quoted literals will now abort Load, even though those locations should remain literal. Interpolate parsed scalar nodes instead, or at least skip comments/single-quoted values before surfacing required-variable errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/compose/model.go` around lines 510 - 524, The interpolation in model
loading is happening on the raw YAML string, so required-variable errors from
os.Expand can be triggered by comments or single-quoted literals that should
stay literal. Update the Load/expansion flow around expandVar and os.Expand in
model.go to interpolate only parsed scalar nodes (or otherwise skip comments and
single-quoted values) before propagating interpErr, so required vars are
enforced only where Compose actually treats them as expandable.
Owner
Author
There was a problem hiding this comment.
Partially declining. The single-quote half of this is incorrect for Compose: YAML single-quotes are not shell quotes, and Compose interpolates string values regardless of quoting style ($$ is the only escape) — so '${VAR}' is expected to expand. The genuinely valid part is ${VAR:?} inside a YAML comment wrongly aborting Load, which is a narrow edge case.
The correct general fix (interpolate parsed scalar nodes instead of raw text) is a load-path rewrite that would touch the $$-escape, default-operator, and required-var tests added in this sweep. I'd rather not land that refactor under a bugfix-cleanup pass where the risk/reward is poor; tracking it separately as a follow-up rather than rushing it here.
There was a problem hiding this comment.
@o1x3, that makes sense — thanks for the correction.
You're right that the single-quoted part of my comment was off: for Compose, YAML quoting does not suppress interpolation, and $$ is the escape that matters. The remaining real issue is the narrower comment-path false positive from doing os.Expand(...) on raw YAML before parsing.
Given the scope here, I agree not forcing a load-path rewrite into this bugfix sweep is the better tradeoff. If you want, I can open a small follow-up issue to track the comment-only interpolation bug separately.
(ᵔᴥᵔ)₊˚🐇
✏️ Learnings added
Learnt from: o1x3
Repo: o1x3/dcon PR: 5
File: internal/compose/model.go:520-534
Timestamp: 2026-06-25T18:04:40.473Z
Learning: In `internal/compose/model.go`, Compose variable interpolation should still apply to YAML single-quoted string values; YAML single quotes are not treated as shell-quoting, and `$$` is the relevant escape mechanism.
Learnt from: o1x3
Repo: o1x3/dcon PR: 5
File: internal/compose/model.go:520-534
Timestamp: 2026-06-25T18:04:40.474Z
Learning: For this PR in repository `o1x3/dcon`, the remaining interpolation concern in `internal/compose/model.go` is specifically that raw-text `os.Expand(...)` can incorrectly enforce `${VAR:?}` inside YAML comments. The maintainer chose to defer a broader parsed-node interpolation refactor to a follow-up change because it would touch the `$$` escape, default-operator, and required-variable test coverage added in this bugfix sweep.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
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.
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).
Owner
Author
Addressed in 1365...
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Red-team bug-fix sweep
A systematic, multi-pass adversarial audit of dcon. Each finding was verified against the code (adversarial second-opinion verification, not taken on faith), fixed, and locked in with a reproducing unit test. Nine audit passes were run over the whole tree; the confirmed-bug count converged pass over pass — 26 → 10 → 5 → 5 → 3 → 1 → 4 → 8 → 0 — with a final full fresh-eyes sweep finding nothing.
~50 bugs fixed, 49 new reproducing tests (187 total).
go test -race ./...,go vet ./..., andgofmtare all clean. Non-TTY byte-identical render contract preserved.Security / isolation
machine rm/stop/shell NAMEresolved by the attacker-controllabledcon.machine.namelabel, so a non-prefixeddcon run --label dcon.machine=1 --label dcon.machine.name=webcontainer could be acted on. Now resolves strictly by the prefixed backend id + verified label.Claimdouble-hand-out: on a state-write failureClaimreturned ok with the member still listed on disk, so two runs could exec into one microVM. Now treated as a miss (cold-run fallback).ReapStaleTOCTOU: reaped a VM a concurrent run had just claimed. Removal is now atomic under the state lock.Crash / data-corruption
run --mount …,tmpfs-size(no value) panicked (slice OOB) — now bounds-safe.--filtercomma-corruption onps/images/volume/network(StringSlice split label values) → StringArray.ports:/volumes:(sequence-of-maps) failed to unmarshal, breaking the whole file →PortList/VolumeList.rm --forcedefaulted true (always force-deleted a running machine) → default false.compose engine correctness
${VAR:-default}worked;${VAR:?}/${VAR:+}/-/+/:=silently produced empty even for set vars, and$$(literal$) was deleted. Full bash/compose parameter-expansion now implemented; required-but-unset${VAR:?}aborts load.<project>_namethatupactually creates (container got the wrong volume;down -vremoved the wrong one).--buildtagged the derived name but ranimage:— the freshly built image was never used.run --rm=falsestripped a literal--rmfrom the user's command; entrypoint override / command override dropped or kept the wrong post-image tokens; one-offruncontainers were labeledoneoff=Falseand collided with service replicas inexec/ps/down.exec -T … < filedropped--interactive(redirected stdin lost);compose runnever forwarded--tty/--interactive;kill --signal/stop|restart --timeoutwere dropped.up --scale SERVICE=0ran 1;images [SERVICE]ignored the filter;ls/config -q,ls --allwere dead flags; multiple-ffiles were silently dropped (now warned);-f/-pshorthand broke when a root flag precededcompose.CLI parity
top web -ef(dashed ps options) no longer errors;ps -n 0shows none;cp ./a:b.txt …no longer misread as a container ref;historytruncates by runes (valid UTF-8);restart --signalhonored;inspect <container> <image>(mixed) works;build --output …,name=preserved as a tag; images registry-port/digest/bad-pattern filters fixed;system prunepropagates errors;volume create --namehonored;stats | catno longer injects ANSI.🤖 Generated with Claude Code
Summary by CodeRabbit
compose upsupports--pull always;buildoutput specs are translated more reliably (including name→tag behavior).compose,ps,images,volume/network ls(comma-safe labels,status=behavior,ancestor=matching, correct-n/--last).machine rmforce default, stricter reserved namespace checks,volume createname conflicts).