compose: runtime-agnostic orchestrator (opt-in)#64
Conversation
Compose orchestrator (forthcoming) will drive backends through five new primitives (CreateNetwork / RemoveNetwork / CreateVolume / RemoveVolume / ListContainers) plus a Capabilities() advertiser. This commit lands only the runtime-neutral input/output types so the diff to the Runtime interface itself in the next commit is isolated and reviewable. Naming follows the existing Spec/Details pattern. Each Capabilities field documents the upstream apple/container issue (or architectural-impossibility note) governing whether the apple backend can ever flip it true, so the per-backend conditional surface stays small and discoverable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add seven methods + Capabilities() to runtime.Runtime so the forthcoming compose orchestrator (compose/, runtime-agnostic) can drive any backend through one interface: - CreateNetwork / RemoveNetwork - CreateVolume / RemoveVolume - ListContainers / ListImages / RemoveImage - Capabilities() Each backend is stubbed in the same commit so bisect stays green; real Docker implementations land next. The applecontainer stubs are permanent for this PR — PR15 swaps them for real impls after the §11 probes confirm what's achievable on apple/container 0.12.x. Docker's Capabilities() returns the all-true baseline. Applecontainer's returns all-false (Healthchecks, ExitCodes, NamespaceSharing, RestartPolicies, SharedVolumes) per the empirically-verified findings in design/compose-native.md §11. engine_test.go's fakeRuntime gets matching stubs so existing tests keep compiling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the stubs from the previous commit with real Docker Engine
API translations via the moby/client SDK. All seven new methods
plus Capabilities() now drive actual daemon state — networks,
volumes, container listing, image listing/removal.
Key decisions:
- CreateNetwork / CreateVolume are idempotent on (name, label
superset). The orchestrator may re-run Up against an existing
project; we return the existing ID rather than erroring.
- RemoveNetwork / RemoveVolume / RemoveImage swallow "not found"
so teardown can be called defensively without pre-checks.
- ListContainers / ListImages reject empty filters — we never
want to enumerate everything.
- Container state mapping mirrors the conversion in inspect.go;
kept local to avoid widening that file's exports.
Adds isNotFoundErr() as the network/volume analogue of the
existing isContainerNotFound / isImageNotFound pattern-matchers.
Unit tests cover labelsMatch, mapContainerState, and pin the
Capabilities() all-true baseline so a future contributor flipping
a flag false silently is caught at CI time. End-to-end coverage
of the primitives themselves lands with the orchestrator
mock-runtime tests in a later commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pure-Go scaffolding the upcoming Orchestrator depends on. Each file
is independent and unit-tested.
compose/graph.go — topo-sort with cycle detection. Returns levels
(parallel-startable groups) rather than a flat list so the
orchestrator can issue concurrent RunContainer calls within a
level. depends_on is the primary edge source; network_mode:
service:<x> is treated as an implicit edge too, so ordering still
respects services that share a network namespace.
compose/hash.go — ConfigHash(imageID, svc) -> sha256. Implementation
is one line of encoding/json + sha256; probe 1 in
design/compose-native.md §11.1 verified determinism across 1000+
iterations and slice/map permutations. Slice order (Ports, Command)
is semantic and intentionally affects the hash; map iteration order
does not.
compose/plan.go — Plan / DownPlan types plus Plan.Validate, which
walks the project and refuses:
- Hard always-refused fields (§2.2): secrets, configs, deploy,
develop, links, scale>1, non-default networks. Single
UnsupportedFieldError listing every offender so the user can
fix them in one edit.
- Backend-gated features keyed off Capabilities():
depends_on.condition: service_healthy /
service_completed_successfully, namespace-sharing modes,
shared named volumes. Each surfaces a typed error.
compose/errors.go — UnsupportedFieldError,
UnsupportedFeatureOnBackendError, VolumeSharedAcrossServicesError,
PartialUpError, HealthTimeoutError, CycleError. All typed; all
sortable for stable test output. Plus the
ErrComposeUnsupportedOnBackend sentinel for "backend doesn't
satisfy any of the new primitives".
Tests cover map-order independence + slice-order sensitivity for
the hash, chain / fan-out / cycle / dangling-dep / network_mode
edges for the graph, and the full §2.2 / backend-gated refusal
matrix for the plan validator. The Orchestrator itself comes next.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Orchestrator drives any runtime.Runtime implementation through
a Plan (Up) or DownPlan (Down). Compose semantics live entirely
here — topological ordering, idempotent reuse via ConfigHash,
health gating, partial-failure handling, label-scoped teardown.
Backends own nothing compose-specific.
Up algorithm (matches design/compose-native.md §5.1):
1. Plan.Validate against runtime.Capabilities().
2. TopoSort -> levels of parallel-startable services.
3. Create project network + named volumes (idempotent on label
superset; safe to re-run).
4. For each level in order, for each service in parallel:
- ListContainers on (project, service) labels.
- If existing matches our hash AND running: reuse.
- Otherwise stop + remove + RunContainer + StartContainer.
- Stamp dev.containers.config-hash + the compose-CLI interop
labels so external `docker compose ps` keeps working.
5. After each level, gate the next: for services in this level
that any later kept service requires via service_healthy or
service_completed_successfully, poll InspectContainer until
the condition is met or HealthTimeout elapses.
Down algorithm (matches design §5.2):
1. ListContainers filtered by project label.
2. Stop + RemoveContainer each (reverse-topo if the project
file is available, name-order otherwise).
3. Optionally RemoveVolume each declared volume, RemoveImage
each project-labelled image.
Idempotent — missing resources are no-ops.
Failure handling:
- Partial start: returns *PartialUpError naming started + failed
services. Does NOT roll back; running services stay running so
the user can exec into them (design §5.3).
- Health timeout: *HealthTimeoutError naming the service +
condition. Dependents are not started.
Mock-runtime tests exercise the state machine without any backend
installed: single service, dependency ordering, hash-match reuse,
hash-change recreate, partial failure, health-gate timeout,
unsupported-field refusal before any side effect, label-scoped
Down isolation, and Down idempotency.
Scope intentionally cut here, addressed in later PRs:
- Port bindings (runtime.RunSpec doesn't carry Ports yet).
- --rmi local image cleanup wiring (primitive exists; orch call
is one line, deferred to engine integration).
- Per-service health-timeout overrides (one global today).
- Health-status readback from ContainerDetails (uses State as a
proxy until the runtime exposes State.Health.Status; documented
inline at the waitFor switch).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Counterparts of WriteBuildOverride / WriteRunOverride that mutate
the loaded *types.Project directly instead of writing tmpfile YAML
for the shell-out path to ingest. The native Orchestrator
(compose.Orchestrator) reads the Plan whose Project has already
been overridden, so the YAML round-trip + compose's
sequence-replace merge hazard go away.
ApplyBuildOverride pins the primary service's image and clears
its build directive (compose v2 invariant: image: and build: are
mutually exclusive).
ApplyRunOverride appends bind mounts, merges environment, merges
labels. Mutation is naturally additive — unlike the YAML write
path, which re-emits the full merged volume list to dodge the
sequence-replace merge — so the implementation is straight
append/assign.
WriteBuildOverride and WriteRunOverride stay in override.go
unchanged: the shell-out path in runtime/docker/compose.go still
uses them. PR17 deletes that path; this commit does not.
Tests cover:
- image pin + build clear
- missing-service error
- volume append (not replace), env merge, label merge
- nil-map seeding
- round-trip through ConfigHash so the orchestrator's recreate-
on-change detector observes the override (prevents stale
container reuse).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds EngineOptions.ComposeBackend (ComposeBackendShellout default,
ComposeBackendNative opt-in) and routes Engine.Up's
*ComposeSource path to either:
- upComposeShellout: legacy path. Writes dc-build.yaml /
dc-run.yaml to a tmpdir, type-asserts ComposeRuntime, calls
ComposeUp / ComposeContainerID. Unchanged from M4.
- upComposeNative: new path. Mutates the loaded *types.Project
in memory via ApplyBuildOverride / ApplyRunOverride, then
drives compose.Orchestrator.Up against runtime.Runtime
primitives. No tmpfile, no ComposeRuntime assertion, no
docker compose plugin dependency.
The ComposeRuntime type-assertion that used to live at the top of
createFreshCompose now runs only in the Shellout branch, so the
native path doesn't gate on a sub-interface it doesn't need.
The bind-mount assembly (workspace folder + cfg.Mounts +
opts.ExtraMounts) is extracted into composeBindMounts() so both
backends use byte-identical override mount lists.
Test coverage:
- TestUp_ComposeSourceErrors still passes — shellout default on a
runtime missing ComposeRuntime fails fast with ErrNotImplemented.
- TestUp_ComposeBackendNative_DispatchesToOrchestrator confirms
the native flag actually routes through the new path (failure
surfaces from inside the orchestrator's runtime calls, NOT
from the ComposeRuntime assertion).
Default stays Shellout per design/compose-native.md §10 — the
phased-rollout schedule. PR16 (when the native parity suite is
green on Docker) flips the default. PR17 deletes the shellout
path entirely along with runtime.ComposeRuntime and
runtime/docker/compose.go.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
First empirical confirmation that compose.Orchestrator + the new runtime/docker primitives drive a real Docker daemon end-to-end, not just the mock-runtime fakes covered by the unit tests in compose/orchestrator_test.go. Three tests, all green against docker 29.x: - TestNativeOrchestrator_Up_TwoServices — 2-service Up (app + db, both alpine sleeping). Asserts both containers run, the project network was created, dev.containers.config-hash is stamped, compose interop labels round-trip cleanly, and Down(Remove) sweeps everything via the project label scan. - TestNativeOrchestrator_Idempotency — same project Up twice, container ID for "app" must be byte-identical across the two calls (config-hash match -> reuse, not recreate). - TestNativeOrchestrator_DependencyOrder — `app depends_on: db`, both services come up cleanly. Demonstrates the topo-sort edge against a live daemon. Wall clock: ~52s for all three (mostly image-pull + container boot, the orchestrator path itself is sub-second). Tests bypass Engine.Up to keep the surface focused — feature-pipeline parity through Engine.Up is the PR14 work. image_source_test.go gets a small refactor: newEngine() now delegates to newEngineWith(opts) so compose-backend tests can construct an engine with a non-default option set without duplicating the daemon-probe + close boilerplate. Existing callers unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
engine.Down's compose path now honors EngineOptions.ComposeBackend, mirroring the Up dispatch landed earlier. Native routes through compose.Orchestrator.Down, which walks the project's containers by label and removes them; shellout retains the existing ComposeRuntime.ComposeDown path. Two new integration tests against real Docker exercise the full Engine.Up / Engine.Down path under ComposeBackendNative: - TestComposeSource_Native_FullFlow — same fixture as the shellout-path TestComposeSource_FullFlow (image + local feature + sidecar). Asserts feature install ran, feature containerEnv + user-declared env both reach Exec, workspace bind mount applied, primary carries dev.containers.id + compose interop labels. Confirms the feature pipeline integrates cleanly with the native orchestrator. - TestComposeSource_Native_DownRemovesProject — Up via native, Down(Remove,RemoveVolumes), then Attach(ID) must fail because the project was actually removed. This was the test that surfaced the missing Down-side dispatch; without it Engine.Down was silently no-op-ing on native-created compose projects. Wall clock ~22s per test (image pull + container boot dominates). Both shellout tests + both native tests run green back-to-back in 51s total. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Production-relevant gaps the prior commits left open. After this,
the native orchestrator handles the same surface a typical
docker-compose project exercises: published ports reachable from
the host, restart policies enforced by the daemon, healthcheck
directives carried through to docker, depends_on:condition:
service_healthy actually keyed off the daemon's health verdict,
and missing images pulled on demand like compose's `up -d`.
Surface additions on runtime.Runtime:
- RunSpec.Ports (slice of PortBinding{HostIP, HostPort,
ContainerPort, Protocol}). Translates to docker's
Config.ExposedPorts + HostConfig.PortBindings via netip.Addr
and network.ParsePort.
- RunSpec.RestartPolicy ("" / always / on-failure /
unless-stopped). Mapped to container.RestartPolicy.
- RunSpec.HealthCheck (*HealthCheckSpec) with test, interval,
timeout, retries, start_period, start_interval, disable.
Mapped to docker's container.HealthConfig; Disable=true emits
the docker NONE sentinel.
- ContainerDetails.Health (HealthStatus enum: none / starting /
healthy / unhealthy). Populated from container.State.Health
in docker's InspectContainer.
Orchestrator changes:
- serviceToRunSpec now translates compose's ServicePortConfig,
Restart, and HealthCheckConfig fields onto the new RunSpec
fields.
- waitFor(service_healthy) keys on details.Health instead of the
earlier State=Running proxy. HealthNone falls back to
State=Running so projects without healthchecks still gate
correctly (matches compose v2 behavior). HealthUnhealthy
surfaces a clear error rather than timing out.
- ensureService retries RunContainer once after PullImage when
the first attempt returns ImageNotFoundError. Mirrors compose's
implicit-pull behavior on `up -d`.
New integration test:
TestNativeOrchestrator_PortsAndHealth — a 2-service project with
nginx publishing port 80 (host-side ephemeral) and a custom
healthcheck, plus `app` gated on web.condition:service_healthy.
After Up, nginx must have flipped to HealthHealthy AND app must
be running. Catches regressions across all three new fields:
port plumbing, healthcheck plumbing, and health-status readback.
Full suite: 9 unit packages green, all 5 compose integration tests
green (2 shellout, 3 native including ports/health), 120s wall
clock for the full integration run.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the ErrNotImplemented stubs from PR13 with real Swift
bridge + cgo shim + Go wrapper implementations for the seven
runtime primitives the compose orchestrator drives:
- CreateNetwork / RemoveNetwork via NetworkClient
(NetworkConfiguration with .nat + vmnet plugin, the only mode
compose semantics expect).
- CreateVolume / RemoveVolume via ClientVolume. Idempotent on
label-superset, mirroring docker's CreateVolume contract.
- ListContainers via ContainerClient.list with .all filters,
plus client-side label filtering (apple's list endpoint has no
--filter flag in 0.12.x per probe R1b).
- ListImages / RemoveImage via ClientImage.list /
ClientImage.delete.
Swift bridge additions (applecontainer-bridge/Sources/ACBridge/):
- networks.swift — ac_network_create / ac_network_remove
- volumes.swift — ac_volume_create / ac_volume_remove
- list.swift — ac_list_containers / ac_list_images /
ac_remove_image
The cgo shim (runtime/applecontainer/shim.{h,c}) gains the seven
new function-pointer slots + dlsym resolutions + partial-failure
reset paths, following the existing pattern.
Capabilities() still returns all-false: the upstream apple/container
gaps governing each capability (#1502 healthchecks, #1501 exit
codes, #286 restart policies, #889 shared volumes,
namespace-sharing architectural) are still open as of 0.12.x.
Plan.Validate refuses compose projects that require those features;
projects that don't (simple service_started ordering, no shared
volumes, no namespace games, no restart-on-crash) now work.
The shared volume gap matters in particular: probe 4 confirmed
apple's ext4-on-disk-image volumes refuse multi-attach at the VM
layer. Plan.Validate keys on SharedVolumes=false to surface a
typed VolumeSharedAcrossServicesError before any side effect.
Existing applecontainer unit tests (37s) still pass — no
behavioral regressions on the non-compose surface.
The orchestrator-side service-name DNS workaround (post-start
/etc/hosts patching per probe 3) lands next, followed by the
end-to-end compose integration test.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the post-level hosts-patching workaround design probe 3
identified as the path to compose-on-apple. Gated by a new
Capabilities.ServiceNameDNS flag — docker keeps the trivial no-op
path because the project network resolves peers natively.
Mechanism:
1. After each level finishes its health gate, harvest every
known container's primary IPv4 via InspectContainer.
2. Render a service→IP block prefixed with a sentinel comment
("# devcontainer-go compose hosts patch").
3. ExecContainer --user 0 sh -c <script> on every container so
far, which idempotently strips any prior block (sed by
marker) and appends the new one. Re-runs of Up replace the
block cleanly.
Apple inspect now stashes the container's primary IPv4 (with
the CIDR /N prefix stripped) under a synthetic label
`dev.containers.network-ip` in ContainerDetails.Labels. Lets
the orchestrator read it through the existing typed surface
without widening runtime.ContainerDetails with a typed network
slice for a single field.
Capabilities:
- docker: ServiceNameDNS=true (built-in compose DNS aliases).
Orchestrator skips the patch entirely.
- applecontainer: ServiceNameDNS=false. Orchestrator patches
every container after each level. Race window per probe 3:
services within the same level without a depends_on edge
can lose a first DNS lookup against each other; documented
as an apple-backend limitation. Most app code resolves
lazily on first request and works through retries.
Existing test fixtures (engine + compose mocks, docker test cap
constant) bumped to include ServiceNameDNS=true.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without an explicit network attachment, apple/container's apiserver
left orchestrator-created containers with empty networks[]: no
IP, no /etc/hosts self-entry, no way for the hosts patch to know
where peers actually live. Probe revealed apple needs the
ContainerConfiguration.networks field populated at create time
(matches what `container run` does internally); docker similarly
needs NetworkingConfig.EndpointsConfig at ContainerCreate.
Surface addition:
- runtime.RunSpec gains Networks []string. Empty means "backend
default" (current behavior). The compose orchestrator now
populates it with []string{plan.ProjectName + "_default"} so
every service in a compose project lands on the network
CreateNetwork just created.
Docker plumbing (runtime/docker/run.go):
- toNetworkingConfig builds EndpointsConfig[<net>] = {Aliases:
[containerName]} so peers can resolve each other by service
name on the project network — compose's standard behavior.
Apple plumbing:
- lifecycle.swift's RunSpecJSON gains `networks: [String]?`. When
non-empty, runContainer maps each entry to
AttachmentConfiguration(network: $0, hostname: spec.id) and
sets cfg.networks before ContainerClient().create. Empty/nil
falls back to apiserver default.
- Go wire shape in lifecycle_darwin_arm64.go mirrors the new
field; serviceToWire carries it through.
End-to-end apple test (test/integration/applecontainer_compose_
native_test.go, build tag integration && darwin && arm64):
- 2-service compose project (app depends_on db) via Engine.Up
with ComposeBackendNative against runtime/applecontainer.
- Asserts the workspace primary container carries our
dev.containers.id + compose interop labels.
- Diagnostics dump of /etc/hosts confirms the orchestrator's
post-level patch landed:
127.0.0.1 localhost
192.168.69.3 dc-<id>-app-1
# devcontainer-go compose hosts patch
192.168.69.3 app
192.168.69.2 db
- getent ahosts db succeeds from inside app — service-name
resolution works on apple via the hosts workaround.
- Workspace bind mount round-trips.
Wall clock: 25s for the apple parity test. Docker integration
suite (5 compose tests including the new
TestComposeSource_Native_FullFlow) stays green in 120s. No unit
test regressions across 9 packages.
PR15 complete — compose source works on apple-container.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two fixes surfaced while bringing up the real dap devcontainer
(8 services, healthchecks, depends_on, ghcr private images) end
to end through the native compose orchestrator.
1. ApplyRunOverride dedupes by target. The user's compose file
often already declares the workspace bind mount
(`..:/workspaces/dap` for compose-source devcontainers); a blind
append on top of that triggered ContainerCreate's
`Duplicate mount point: /workspaces/dap`. The shellout path
never hit this because compose's YAML merge dedupes implicitly.
Now: walk the existing volumes, build a target-set, skip
appending any extra bind whose target already exists. User's
source wins (their explicit declaration is authoritative).
2. Docker network endpoint aliases now include the service name.
Containers on a compose project network expose two aliases on
the embedded DNS server: the container name (existing behavior
for non-compose callers) AND the service name read off the
com.docker.compose.service label. Before this fix,
`getent hosts postgres` from inside `app` returned NXDOMAIN
even though docker assigned both containers project IPs.
Verified on the real dap devcontainer:
- Up OK in 5s (8 services started)
- getent hosts {postgres,rabbitmq,idp,minio,bifrost} all
resolve to their assigned IPs on the project network
- psql -h postgres ... -c 'SELECT 1' → 1
- curl idp/.well-known/openid-configuration → 200
- curl minio/health/live → 200
- Down cleans up the whole project
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Real-world bringup of the dap devcontainer (8-service compose
project, mix of multi-arch and amd64-only images) surfaced that
lifecycle.swift's `cfg.platform = .current` hardcoded the host
arch even when the image only had a different one. apple/container
rejects such cases with "unsupported: platform <os>/<arch>" or
"does not support required platforms" before bootstrap.
Bridge now mirrors the CLI's containerConfigFromFlags path:
- resolvePlatform(for: img) walks img.index().manifests: prefer
the host's `.current`, fall back to whichever variant the
image actually carries. Handles legacy single-manifest images
(no usable index) by returning .current and letting the
subsequent config(for:) call surface a clearer error.
- getCreateSnapshot(platform:) is called explicitly before
container create so the apiserver finds the per-platform rootfs
snapshot staged on disk. Without this, create fails even when
the manifest exists.
- cfg.rosetta = true when host=arm64 and image=amd64. Apple's
Rosetta-for-Linux translates amd64 userland in an arm64 VM.
Documented inline that an amd64 image still fails to boot if
the host's Virtualization.framework doesn't expose Rosetta to
the VM (surfaces as VZErrorDomain Code=1; same wall the CLI
hits on such machines).
The existing apple-compose integration test (alpine + alpine sidecar,
multi-arch on both sides) stays green. dap on apple is still blocked
on this machine because the dap devcontainer image is amd64-only and
the host's VZ stack rejects amd64 guests — that's an environmental
limitation, not a bridge bug.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a runtime-agnostic compose orchestrator, extends runtime compose primitives, implements Docker and Apple backends (Swift bridge + shim), wires engine compose backend selection (native vs shellout), and adds unit and integration tests. ChangesNative Compose Orchestrator
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
test/integration/compose_native_orchestrator_test.go (1)
213-282: ⚡ Quick winAdd an explicit host-port binding assertion in
PortsAndHealth.The test currently validates health gating, but it doesn’t actually assert published host-port behavior despite the test intent/comments. Add a check that
webhas a host binding for container port80.🤖 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 `@test/integration/compose_native_orchestrator_test.go` around lines 213 - 282, Add an assertion in TestNativeOrchestrator_PortsAndHealth that the inspected web container has a host binding for container port 80: after obtaining d via rt.InspectContainer(ctx, webID) (the existing variable d), check the network/port bindings from the inspection result (e.g. the Ports/NetworkSettings mapping for "80/tcp") and fail the test if no host port is published for that container port, so the test verifies published host-port behavior in addition to health gating.
🤖 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 `@compose/graph_test.go`:
- Around line 31-33: Run gofmt on the file to fix formatting lint failures;
specifically reformat the test that constructs the map with keys "app" and
"sidecar" in compose/graph_test.go (the map literal shown in the diff) so the
file is properly gofmt'ed and the lint error at that map is resolved.
In `@compose/graph.go`:
- Around line 29-31: The TopoSort function currently dereferences project
(accessing project.Services) without a nil check which panics for TopoSort(nil);
add an early guard at the start of TopoSort that returns a clear error (e.g.,
fmt.Errorf("nil project")) when project == nil, so callers receive an error
instead of a panic; update any tests if present to expect the error and ensure
the rest of TopoSort (which uses project.Services) assumes a non-nil project.
In `@compose/hash.go`:
- Around line 36-39: Run gofmt on the file to fix the formatting around the
hashInput struct declaration (type hashInput struct { ImageID string
`json:"image_id"` Svc composetypes.ServiceConfig `json:"svc"` }) so the struct
fields and tags are properly aligned; simply run `gofmt -w` (or `gofmt -w
compose/hash.go`) to reformat the file and resolve the lint failure.
In `@compose/orchestrator_test.go`:
- Around line 31-35: The struct field alignment for the resources block (fields
networks, volumes, containers) is inconsistent; run gofmt (or gofmt -w) on the
file or reformat the struct so the field names and types use standard Go
formatting (e.g., networks map[string]map[string]string, volumes
map[string]map[string]string, containers map[string]*mockContainer) to make
spacing/tabs consistent and fix the formatting.
In `@compose/orchestrator.go`:
- Around line 654-656: The CapAdd field is being assigned with an unnecessary
conversion ([]string(svc.CapAdd)) even though svc.CapAdd is already
composetypes.StringList (a []string alias); simply assign CapAdd: svc.CapAdd
instead of performing the explicit conversion to remove redundancy and keep
types consistent in the struct literal where Init and CapAdd are set.
- Around line 358-372: Remove the dead runtime call and unused vars: delete the
o.rt.ListContainers(ctx, runtime.LabelFilter{Match:
map[string]string{LabelComposeProject: plan.ProjectName}}) invocation and the
subsequent "_ = nets" discard (remove variables nets and err), and replace that
block with the existing explanatory comment placeholder about network cleanup
being out-of-scope; look for the o.rt.ListContainers call and the nets/err
bindings in the orchestrator.go Down-related code to edit.
In `@compose/plan.go`:
- Around line 278-291: The loop over the map variable users yields a
non-deterministic volName when multiple shared volumes exist; to make the
reported VolumeSharedAcrossServicesError deterministic, collect the map keys
(volume names) into a slice, sort that slice, then iterate over the sorted
volume names instead of ranging directly over users and keep the same logic that
builds services (using the existing set -> services conversion and returning
VolumeSharedAcrossServicesError with Volume: volName and Services: services).
In `@down.go`:
- Around line 108-117: The ContainerStoppedEvent is currently emitted even when
e.runtime.StopContainer returns a not-found error; change the logic in the
stop-without-remove branch so bus.Emit(events.ContainerStoppedEvent{ContainerID:
ws.Container.ID, ExitCode: -1}) is only called when StopContainer succeeded (err
== nil). Keep the existing error handling: if err != nil && !isNotFound(err)
return the wrapped error, if isNotFound(err) skip emitting the event and return
nil, and if err == nil emit the event and then return nil; update the block
around e.runtime.StopContainer, isNotFound, and bus.Emit accordingly.
In `@runtime/docker/compose_primitives.go`:
- Around line 209-214: The labelsMatch function can return true when want has a
key with an empty string but have lacks that key; update labelsMatch to
explicitly check key existence before comparing values—inside labelsMatch
(function name reference) for each k in want, use a presence check (e.g., _, ok
:= have[k]) and return false if !ok or if the value doesn't equal want's v, so
missing keys are treated as mismatches and prevent incorrect reuse of resources.
In `@test/integration/applecontainer_compose_native_test.go`:
- Around line 68-70: The no-op defer after calling newAppleContainerEngine(t)
leaves the runtime (rt) unclosed; replace "defer func() {}()" with a real
cleanup call such as "defer rt.Close()" (or the appropriate shutdown method on
the returned runtime) so the runtime returned by newAppleContainerEngine is
properly cleaned up before creating the devcontainer via devcontainer.New with
devcontainer.EngineOptions.
In `@up.go`:
- Around line 469-480: The code only prevents shellout for fresh creates in
createFreshCompose but leaves Recreate flows calling composeDownExisting which
may still type-assert to runtime.ComposeRuntime and perform shellout; update the
Recreate path so native backend never uses runtime.ComposeRuntime shellout:
either modify composeDownExisting to check e.opts.ComposeBackend (or the Up
method when opts.Recreate is true) and, when
ComposeBackend==ComposeBackendNative, use the native removal logic (the same
code path/operations the native backend expects that remove all project
containers/resources) instead of asserting to runtime.ComposeRuntime, or return
ErrNotImplemented if the native runtime cannot perform the required native
removal; reference createFreshCompose, composeDownExisting, Up,
Engine.opts.ComposeBackend, opts.Recreate, and runtime.ComposeRuntime when
making the change.
- Around line 536-543: The switch on e.opts.ComposeBackend currently treats any
unexpected value as the default shellout path; change it to explicitly handle
known cases and return an error for unknown values: keep the
ComposeBackendNative case calling upComposeNative and the ComposeBackendShellout
(or equivalent explicit enum) case calling upComposeShellout, and add a final
branch that returns a clear error (including the invalid e.opts.ComposeBackend
value) so callers fail fast instead of silently falling back; update
callers/signature if needed to propagate the error.
---
Nitpick comments:
In `@test/integration/compose_native_orchestrator_test.go`:
- Around line 213-282: Add an assertion in TestNativeOrchestrator_PortsAndHealth
that the inspected web container has a host binding for container port 80: after
obtaining d via rt.InspectContainer(ctx, webID) (the existing variable d), check
the network/port bindings from the inspection result (e.g. the
Ports/NetworkSettings mapping for "80/tcp") and fail the test if no host port is
published for that container port, so the test verifies published host-port
behavior in addition to health gating.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b28a6f7-09e8-485a-84f2-21f6dbea87c0
📒 Files selected for processing (34)
applecontainer-bridge/Sources/ACBridge/lifecycle.swiftapplecontainer-bridge/Sources/ACBridge/list.swiftapplecontainer-bridge/Sources/ACBridge/networks.swiftapplecontainer-bridge/Sources/ACBridge/volumes.swiftcompose/apply_override.gocompose/apply_override_test.gocompose/errors.gocompose/graph.gocompose/graph_test.gocompose/hash.gocompose/hash_test.gocompose/orchestrator.gocompose/orchestrator_test.gocompose/plan.gocompose/plan_test.godown.goengine.goengine_test.goruntime/applecontainer/compose_primitives_darwin_arm64.goruntime/applecontainer/inspect_darwin_arm64.goruntime/applecontainer/lifecycle_darwin_arm64.goruntime/applecontainer/shim.cruntime/applecontainer/shim.hruntime/compose_primitives.goruntime/docker/compose_primitives.goruntime/docker/compose_primitives_test.goruntime/docker/inspect.goruntime/docker/run.goruntime/runtime.gotest/integration/applecontainer_compose_native_test.gotest/integration/compose_native_engine_test.gotest/integration/compose_native_orchestrator_test.gotest/integration/image_source_test.goup.go
CI's lint job has been failing on this branch with a mix of
gofmt-formatting drift and three minor staticcheck-style warnings.
Running gofmt -w on the offending files (compose/* tests,
runtime/applecontainer/*, test/integration/*) plus three
one-line fixes brings the lint job back to green:
- errcheck: explicit `_ = pw.Close()` in
logs_darwin_arm64_test.go
- ineffassign: drop the dead ListContainers stash in Down (the
network-removal hole it was a comment-marker for is fixed
in a follow-up commit, not here).
- unconvert: svc.CapAdd is already []string; the explicit
conversion is redundant.
No behavioral changes. Unit suite stays green; lint goes from
13 issues to 0.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Side-by-side review against docker/compose v2's
pkg/compose/{hash,convergence,down,dependencies}.go surfaced five
correctness gaps in our orchestrator. Fixes + tests:
1. Down leaks the project network. orchestrator.Down skipped
network removal entirely (long-standing TODO comment).
Removed; Down now calls RemoveNetwork(<project>_default).
Backends already swallow not-found, so it's safely idempotent.
Regression: TestDown_RemovesProjectNetwork.
2. Down's reverse-topo ordering was broken. serviceLabelOf
returned the container *name* (e.g. "dc-x-app-1"), not the
compose service label, so the level-index lookup keyed wrong
and order degraded to alphabetic. Widen runtime.Container with
a Labels map (populated by ListContainers on both docker and
apple backends), then key off com.docker.compose.service.
Regression: TestDown_ReverseTopoOrder asserts app->api->db
stop sequence.
3. ConfigHash keyed on the image tag, not the digest. After a
moving tag updates (docker pull lands a new digest under the
same tag), our hash didn't change and we'd reuse the stale
container. Resolve via InspectImage, stamp the resolved digest
as a separate dev.containers.image-digest label, and require
both labels to match for reuse. Mirrors compose-v2's
ImageDigestLabel cross-check.
Regression: TestUp_RecreateOnImageDigestChange.
4. Anonymous volumes investigated. Turned out to be a false
positive in the side-by-side analysis: anonymous volumes flow
through mount.Mount{Type:volume, Source:""} correctly; the
apparent "drop" in ensureNamedVolumes is the correct behavior
(we don't CreateVolume for unnamed entries). Added
TestUp_AnonymousVolumesFlowThrough and a comment on
mountSourceOf to document the contract.
5. ConfigHash hashed non-runtime fields. Build, PullPolicy,
Scale, Deploy, DependsOn, Profiles all participated in the
hash, so editing a depends_on edge or adding a profile
triggered service recreates even though the container's
runtime config was unchanged. Match docker/compose's
pkg/compose/hash.go: strip those fields before hashing.
Regression: TestConfigHash_StripsNonRuntimeFields covers all
six per-field subtests.
Bonus: optional depends_on (Required=false). gateLevel treated
every depends_on edge as required; per compose spec, a service
with required=false should NOT block the dependent's start on
gate timeout. Now we record optional vs strict per dep and
swallow the timeout on optional ones.
Regression: TestUp_OptionalDependencySkipsOnTimeout.
Full suite: 9 unit packages green, docker compose integration
suite green (115s, 5 tests).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Seven comments from the first review round that I didn't fold into
the bug-fix commit. Now resolved:
- TopoSort: guard against nil project input. TopoSort(nil) used to
panic at project.Services; returns *compose.TopoSort: nil project*
error now.
- Plan.Validate (shared-volume rejection): iterate volume names in
sorted order before picking the first violation. Map iteration
was non-deterministic, so a project with multiple shared volumes
would surface a different one each run, hurting test stability
and user trust.
- Engine.Down (compose stop path): emit ContainerStoppedEvent only
after a successful StopContainer. Previously, when the container
was already gone, we'd swallow the not-found error AND emit a
synthetic stop transition. Now: not-found returns nil without an
event; only real successful stops fire the event.
- runtime/docker labelsMatch: existing labels with an empty value
used to falsely match expected labels that were also empty,
because have[k] returns "" for missing keys. Explicit ok-check.
- test/integration/applecontainer_compose_native_test.go: drop
the no-op `defer func() {}()` — apple Runtime has no Close
method, so there's nothing to clean up.
- up.go (Recreate flow): composeDownExisting used to type-assert
ComposeRuntime regardless of EngineOptions.ComposeBackend, so
Recreate on the native backend would still shell out to
docker compose down. Dispatch on ComposeBackend now: native
uses compose.Orchestrator.Down, shellout stays on
ComposeRuntime.
- up.go (createFreshCompose dispatch): switch's default branch
routed every unrecognized ComposeBackend enum value to shellout.
Now: only ComposeBackendNative + ComposeBackendShellout are
accepted; anything else returns a typed "unknown ComposeBackend
value" error.
Lint still 0 issues; full unit suite stays green across 9 packages.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/integration/applecontainer_compose_native_test.go (1)
90-95: ⚡ Quick winBound deferred teardown with a timeout context.
Cleanup currently uses
context.Background(), so a stuck teardown can hang the test unnecessarily long.Proposed change
defer func() { - _ = eng.Down(context.Background(), wsObj, devcontainer.DownOptions{ + downCtx, downCancel := context.WithTimeout(context.Background(), 60*time.Second) + defer downCancel() + _ = eng.Down(downCtx, wsObj, devcontainer.DownOptions{ Remove: true, RemoveVolumes: true, }) }()🤖 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 `@test/integration/applecontainer_compose_native_test.go` around lines 90 - 95, The deferred teardown currently calls eng.Down with context.Background(), which can hang; change it to use a timeout context (e.g., create ctx, cancel := context.WithTimeout(context.Background(), <reasonableDuration>) and defer cancel) and pass that ctx to eng.Down so the devcontainer.DownOptions cleanup (invoked on wsObj) is bounded; ensure you still call eng.Down(wsObj, devcontainer.DownOptions{Remove:true, RemoveVolumes:true}) with the new ctx and choose an appropriate timeout value.
🤖 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 `@compose/orchestrator_test.go`:
- Around line 219-234: The mock implementations of ListContainers and ListImages
must enforce the non-empty-filter contract: in mockRuntime.ListContainers and
mockRuntime.ListImages, validate the provided runtime.LabelFilter (e.g., check
filter.Match is nil or has zero length) and return a non-nil error immediately
when the filter is empty instead of proceeding/returning nil results; mirror the
real runtime behavior by returning an appropriate error value (and documentable
message) so tests exercising empty-filter rejection will fail against the mock
as they would against the real implementation.
---
Nitpick comments:
In `@test/integration/applecontainer_compose_native_test.go`:
- Around line 90-95: The deferred teardown currently calls eng.Down with
context.Background(), which can hang; change it to use a timeout context (e.g.,
create ctx, cancel := context.WithTimeout(context.Background(),
<reasonableDuration>) and defer cancel) and pass that ctx to eng.Down so the
devcontainer.DownOptions cleanup (invoked on wsObj) is bounded; ensure you still
call eng.Down(wsObj, devcontainer.DownOptions{Remove:true, RemoveVolumes:true})
with the new ctx and choose an appropriate timeout value.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3a54348-0bb4-4c95-8a81-07604ca61e1b
📒 Files selected for processing (21)
compose/graph.gocompose/graph_test.gocompose/hash.gocompose/hash_test.gocompose/orchestrator.gocompose/orchestrator_test.gocompose/plan.godown.goruntime/applecontainer/compose_primitives_darwin_arm64.goruntime/applecontainer/embed_darwin_arm64.goruntime/applecontainer/exec_darwin_arm64.goruntime/applecontainer/inspect_darwin_arm64.goruntime/applecontainer/lifecycle_darwin_arm64_test.goruntime/applecontainer/logs_darwin_arm64.goruntime/applecontainer/logs_darwin_arm64_test.goruntime/applecontainer/runtime_darwin_arm64.goruntime/docker/compose_primitives.goruntime/runtime.gotest/integration/applecontainer_compose_native_test.gotest/integration/applecontainer_image_source_test.goup.go
💤 Files with no reviewable changes (3)
- test/integration/applecontainer_image_source_test.go
- runtime/applecontainer/runtime_darwin_arm64.go
- runtime/applecontainer/logs_darwin_arm64.go
✅ Files skipped from review due to trivial changes (3)
- runtime/applecontainer/lifecycle_darwin_arm64_test.go
- runtime/applecontainer/exec_darwin_arm64.go
- runtime/applecontainer/embed_darwin_arm64.go
🚧 Files skipped from review as they are similar to previous changes (9)
- compose/hash.go
- runtime/applecontainer/inspect_darwin_arm64.go
- up.go
- compose/graph_test.go
- runtime/applecontainer/compose_primitives_darwin_arm64.go
- compose/plan.go
- runtime/docker/compose_primitives.go
- compose/graph.go
- compose/orchestrator.go
The README documented Docker as the sole backend and described the compose path as shell-out-only. Both stopped being true once the applecontainer backend landed and again with this branch's native compose orchestrator. Updates: - New "Backends" subsection under Status listing runtime/docker (default) vs runtime/applecontainer (macOS 15+ Apple silicon, via Swift bridge). Notes both implement the same Runtime interface so the engine, feature pipeline, lifecycle, and compose paths don't care which one is wired in. - Spec-compliance row for dockerComposeFile now reads "compose-go parse + either shell-out (default) or in-process orchestrator via ComposeBackendNative — works against both backends." - Install requirements split into a "Docker backend" and "Apple container backend" bullet so the compose-plugin requirement is scoped correctly (only when ComposeBackend != Native). - Quick-start gains a small "swap the backend" snippet pointing at runtime/applecontainer so newcomers see this is genuinely runtime-agnostic, not a Docker-flavored API. - Sub-packages list adds runtime/applecontainer and expands the compose entry to mention the orchestrator + native path. - Tests section calls out the apple-container integration build tag (integration && darwin && arm64) and notes CI runs the macos-26 job for the Swift bridge + applecontainer unit tests. Also folds in one straggler PR review comment: the mock runtime's ListContainers / ListImages now enforce the non-empty-filter contract the real Runtime interface documents, so orchestrator regressions that drop the filter surface in tests instead of silently passing. Validated: full unit suite green across 9 packages, lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@compose/orchestrator_test.go`:
- Around line 325-329: The test currently indexes into order while iterating
over want which can panic if order is shorter; update the test (the section
using variables want and order in the for i, name := range want loop) to first
assert lengths match (e.g., check len(order) == len(want) and fail the test
early with t.Fatalf or t.Errorf+return if not) and only then run the indexed
comparisons inside the for loop to avoid panics and produce a clear failure
message.
In `@README.md`:
- Line 38: Update the product name casing from "Apple silicon" to "Apple
Silicon" in the README text (e.g., the line containing "Lets you run
devcontainers on Apple silicon without Docker" and the other occurrence
mentioned) so both user-facing mentions use the official "Apple Silicon" casing.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98946da1-d009-486d-becf-715293c0a154
📒 Files selected for processing (2)
README.mdcompose/orchestrator_test.go
Adds an "Apple-container gotchas" subsection under Backends covering
the things that bit us bringing dap up on apple, plus the things
likely to bite next users. Scope: items specific to the apple
backend that don't apply to docker. None are bugs in this library;
all reflect the current state of apple/container 0.12.x.
Documented:
- Manual daemon + builder lifecycle (`container system start`,
`container builder start`).
- Image-store credentials separate from Docker's
(~/.docker/config.json not consulted).
- Multi-arch builds: builder VM is amd64 (Rosetta), but output
images target host arch by default — feature builds produce
native arm64 unless the FROM base is amd64-only.
- amd64 default kernel is not installed; how to install it
without nuking the arm64 registration.
- Port forwarding silently dropped today.
- Plan-validate refusals for compose features gated on upstream
apple/container fixes (#1502, #1501, #889, #286, #856).
- /etc/hosts patching workaround for service-name DNS.
Also folds in two review comments while I was here:
- compose/orchestrator_test.go: assert `len(order) == len(want)`
before the indexed loop so a short order surfaces the count
instead of panicking on out-of-bounds.
- README: "Apple silicon" → "Apple Silicon" everywhere (official
product casing).
Validated: full unit suite green across 9 packages, lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Adds a Go-native, runtime-agnostic compose orchestrator alongside the existing
docker composeshell-out. Backends drive compose throughruntime.Runtimeprimitives — nodocker composev2 plugin dependency, and the path works on backends that have no Docker-API equivalent (specifically: the Applecontainerruntime, which the orchestrator now drives end-to-end).Opt-in via a feature flag
Existing behavior is unchanged. Callers continue to use the shell-out path by default; the native orchestrator is selected explicitly:
The flag's zero value (
ComposeBackendShellout) keeps every existing caller on the legacy path. Nothing flips silently.What's in the box
compose/package — runtime-agnostic orchestrator. Up + Down state machine, topological planning overdepends_on,config-hashbased reuse/recreate, partial-failure semantics, healthcheck and exit-code gating, named volume creation, project network creation, in-memory override application (no tmpfile YAML), pull-on-miss.runtime.Runtimesurface additions. Seven new methods (CreateNetwork/RemoveNetwork/CreateVolume/RemoveVolume/ListContainers/ListImages/RemoveImage) plus aCapabilities()advertiser so the orchestrator can refuse compose features a backend can't honor (instead of silently dropping them or surfacing a confusing backend error).RunSpecgainsNetworks,Ports,RestartPolicy,HealthCheck;ContainerDetailsgains a typedHealthfield.Docker backend. Real implementations of every new primitive via the moby SDK. Network endpoint aliases include the compose service name so
getent hosts <service>works between peers.Capabilities()advertises the full feature set.Apple-container backend. Same primitive surface implemented over Apple's
ContainerClient/NetworkClient/ClientVolume/ClientImage. Adds Swift bridge exports + cgo shim plumbing. Container creation picks the right image manifest per service (multi-arch images run natively; amd64-only images opt into Rosetta when the host supports it). For projects without service-name DNS resolution, the orchestrator patches/etc/hostspost-level sodepends_on-declared peers resolve.Capability matrix so backends self-describe what they support —
Healthchecks,ExitCodes,NamespaceSharing,RestartPolicies,SharedVolumes,ServiceNameDNS. The plan validator refuses projects that need unsupported capabilities with typed errors before any side effects.Test plan
go test ./...).service_healthygating.Engine.Upparity (feature pipeline + workspace mount + sidecar Up) underComposeBackendNativeagainst real Docker./etc/hosts-patched DNS → Down) verified on darwin/arm64.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Runtime
Apple container bridge
Tests
Documentation