feat!: remove the Apple Containers backend - #127
Conversation
The backend was darwin/arm64-only and reached Apple's `container` stack through a cgo shim over a Swift package, so it could never be built or exercised on Linux CI. Its CI jobs were removed in #121 and README has documented Docker as the only backend since #122, leaving ~3.7k lines of Go, C and Swift that nothing in CI compiles and nothing in the project claims to support. Removed: runtime/applecontainer (incl. shim.c / shim.h) applecontainer-bridge (SwiftPM package + ac_bridge.h) cmd/devcontainer/runtime_applecontainer_{darwin_arm64,other}.go test/integration/applecontainer_*_test.go (8 suites) design/runtime-applecontainer.md Makefile: the bridge / bridge-clean targets and the test / test-integration dependency on them The CLI keeps --runtime as the seam for wiring a future backend, but it now accepts only `docker`; `--runtime applecontainer` is refused with `unknown runtime "applecontainer" (want docker)`. Comment-only elsewhere: doc comments describing backend-agnostic behavior no longer cite Apple as the divergent implementation, and compose/plan_test.go's all-false capability fixture is renamed from appleCaps to limitedCaps (it was always the zero value, not an Apple-specific profile). Deliberately left in place: the Capabilities gating machinery. Every field's only false case was Apple, so the refusal branches in Plan.Validate and the /etc/hosts fallback behind ServiceNameDNS are now unreachable — but their Apple provenance comments are the record of why each flag exists, and whether Capabilities() stays on the Runtime interface is a separate decision. design/compose-native.md keeps its Apple sections for the same reason design/README.md gives: those records document the state of the world when written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
On-demand reviews are free for the next 20 days. After that, they cost $0.25 per reviewed file. Or wait 7 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Your 53 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
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 ignored due to path filters (1)
📒 Files selected for processing (61)
💤 Files with no reviewable changes (47)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. 📝 WalkthroughWalkthroughThe pull request removes the Apple Container backend, Swift bridge, runtime implementation, integration tests, Makefile targets, and design record. CLI runtime selection, Compose capability tests, and runtime documentation now describe Docker-only or backend-neutral behavior. ChangesRuntime backend removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change removes the unsupported Apple Containers backend and makes Docker the only accepted runtime; based on the reported checks, no actionable merge-blocking risk remains beyond normal review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 9 files. (5 skipped: 5 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![dap-code-review-by-crunchloop[bot]](https://avatars.githubusercontent.com/u/97309830?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — head a61738e
Reviewed head a61738ed804495d4730a1eb9fa9117ec5440436f after checking out the PR; the range is one commit against merge base 6702fb2f2dbce8dbaab8c47603c83b2f58e3e60f and contains 65 changed paths (62 files reported by the PR metadata, with the deleted-tree accounting differing). Audited the Docker-only CLI selection, runtime interface and Docker implementation, compose orchestration callers, Makefile/CI references, deleted backend/bridge paths, and documentation links. Applied the base-commit .dap/review/engineering.md directives, plus CONTRIBUTING.md and the touched runtime/compose design records. No test or system code was executed. The base directive references PRD.md, but that path does not exist at the supplied directives SHA, so PRD-specific review was not available. Submitted the documentation consistency finding; no other concrete reachable regression was established. Coverage focused on D1, D10, D11, D13, and D14 as applicable; backend parity and Apple implementation behavior are removed rather than newly implemented, and runtime/integration execution was not performed.
Commented inline
- [LOW] [D11] Deleting the Apple runtime record leaves broken references in the retained compose design —
design/README.md:23
Verdict
ADVISORY — findings worth reading, none of them blocking.
| | [`runtime.md`](runtime.md) | The `runtime.Runtime` interface, the `Workspace` value object, the `Engine` layer, container-context substitution, and lifecycle idempotency. The boundary that makes a second backend possible. | | ||
| | [`runtime-applecontainer.md`](runtime-applecontainer.md) | The second `Runtime` backend: Apple's `container` stack on macOS via a cgo + Swift bridge. Daemon model, bridge ABI, version pinning, build constraints. | | ||
| | [`compose-native.md`](compose-native.md) | The runtime-agnostic compose orchestrator that drives any backend through `runtime.Runtime` primitives. Replaces (when opted in) the `docker compose` shell-out, and is what enables compose source on apple-container. | | ||
| | [`compose-native.md`](compose-native.md) | The runtime-agnostic compose orchestrator that drives any backend through `runtime.Runtime` primitives. Replaces (when opted in) the `docker compose` shell-out. | |
There was a problem hiding this comment.
[LOW] [D11] Deleting the Apple runtime record leaves broken references in the retained compose design
- Witness: Command
test -e design/runtime-applecontainer.md && echo present || echo absent; grep -nF 'runtime-applecontainer' design/compose-native.md | head -30producedabsentfollowed by references atdesign/compose-native.md:13,:54,:379,:403,:413,:522,:529,:931, and:1044. The PR deletesdesign/runtime-applecontainer.mdwhile retainingdesign/compose-native.md, so these links and cross-references now point at a file that no longer exists. - Consumer: design/compose-native.md:13 and the other listed references are read by contributors as the retained architecture record; its companion link and cited sections can no longer be opened after this deletion.
- Fix: Either retain the removed record as an explicitly historical document, or update
design/compose-native.mdto remove/replace every reference todesign/runtime-applecontainer.mdand its sections.
There was a problem hiding this comment.
The reference not resolving is real. But I don't think either remedy fits this repo's recorded convention, and I'd rather settle that before editing the retained record — evidence below.
design/README.md:10-15 sets the policy for this directory: the records "reflect the state of the world at the time they were written", "when a record disagrees with main, the code is authoritative", and sections that call out alternatives, probe results or "future work" are kept because the reasoning is still useful. Under that policy a companion citation inside a record is a historical reference, not a navigable link.
That is already how design/compose-native.md behaves, independent of this PR. The sentence you flagged spans :11-14, and its first citation is design/compose.md — which has never existed in this repo: git log --oneline --diff-filter=D -- design/compose.md returns nothing and test -e design/compose.md reports absent. design/status.md:971 is the same ("the breakdown lives in design/status.md once this design is approved"). So this document carried unresolvable design citations before this change; deleting the Apple record adds to that set rather than creating the condition.
Precedent for the handling: #124 deleted design/checkpoint-restore.md, design/podman-backend.md and design/compose-native-health.md along with the code they described, left design/compose-native.md untouched, and recorded in the CHANGELOG that all three "remain readable in git history at tag v0.4.3". This PR's CHANGELOG entry does the same for design/runtime-applecontainer.md.
On the two remedies specifically. Retaining the removed record as an explicitly historical document reverses the deletion this PR exists to make, and contradicts #124's handling of the identical situation. Rewriting the nine cited sections would edit the historical body of a retained record, which is what design/README.md tells contributors not to do — and the §8 / §10.1 / §11 citations carry information (which probe, which primitive mapping, which discipline was followed) that cannot be replaced without losing it.
If you consider the dangling-citation condition worth addressing on its own terms, the cheaper fix that fits the convention is the mechanism the repo already uses — the tag pointer: a single line in the compose-native.md header recording that the companion records it cites are not present in the tree, and the tag where the Apple one is readable. One site, covers all nine references plus the two that predate this PR, and leaves the historical prose intact. Say the word and I'll push that instead; I did not want to rewrite the record first and argue afterwards.
There was a problem hiding this comment.
Pushed the tag-pointer fix in 9092268 rather than leave this standing on the argument alone.
design/compose-native.md now carries one header note recording that the three companion records it cites are not in the tree — design/runtime-applecontainer.md, deleted here and readable at tag v0.4.3, plus design/compose.md and design/status.md, which never landed — and that references to them and their section numbers are historical citations. That covers all nine references you listed at a single site, plus the two that predated this PR, without editing the record's historical body. No other retained record cites a missing design file; I checked every cross-reference under design/.
There was a problem hiding this comment.
Resolved — the reviewer verified this was fixed.
Verified: Verified at head 9092268: design/README.md no longer lists the deleted runtime-applecontainer.md row, and design/compose-native.md:8-16 explicitly labels references to that deleted record as historical citations rather than links. The retained references therefore no longer present broken navigable links.
The review comment above records what was claimed at the time, and is left unedited.
…itations design/compose-native.md cites three companion records that are not in the tree: design/runtime-applecontainer.md, deleted with the Apple Containers backend in this PR, and design/compose.md / design/status.md, which were named as companions but never landed (neither has a delete commit in git history). Adds one note in the header pointing at the tag where the Apple record is readable and stating that references to all three — including their section numbers — are historical citations rather than links. This is the tag-pointer mechanism the CHANGELOG already uses for the records #124 deleted, and it fixes all nine references at one site. Rewriting the nine cited sections was the alternative; it would have edited the historical body of a retained record, which design/README.md tells contributors not to do, and the §8 / §10.1 / §11 citations carry which-probe and which-mapping detail that a rewrite would lose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review — head 9092268
Reviewed PR head 9092268, two commits and 63 files changed from base 6702fb2. Checked the Docker-only CLI wiring, removal of all Apple runtime/bridge/build/test paths, Makefile and documentation updates, retained Runtime/compose interfaces and callers, and searched the full HEAD for dangling Apple symbols or imports. Worked D1/R2, D10, D11/R5, D12, D13, and the relevant directive checks; the carried broken-design-reference finding was verified fixed and resolved. No new concrete findings survived reachability and caller checks. The base-directed CONTRIBUTING.md and design records were read; PRD.md is referenced by the directive but absent at the base commit. I did not execute tests, type-checks, builds, or services per review policy.
Verdict
CLEAN — this round found nothing.
1 participant
Step 1 of the Docker-only cleanup, following #121 (CI jobs), #122 (README), #123/#124 (Podman + checkpoint/restore) and #125.
BREAKING. The backend was darwin/arm64-only, reached Apple's
containerstack through a cgo shim over a Swift package, and could never be built or exercised on Linux CI. Its CI jobs went in #121 and the README has documented Docker as the only backend since #122 — so this was ~3.7k lines of Go, C and Swift that nothing in CI compiles and nothing in the project claims to support.62 files changed, 72 insertions(+), 7674 deletions(-)Removed
runtime/applecontainer/shim.c/shim.hcgo layerapplecontainer-bridge/include/ac_bridge.hcmd/devcontainer/runtime_applecontainer_{darwin_arm64,other}.gotest/integration/applecontainer_*_test.godesign/runtime-applecontainer.mddesign/index rowMakefilebridge/bridge-cleantargets and thetest/test-integrationdependency on them — neither target shells out toswiftany moreAlso: the
.devcontainer/README.mdnote about needing a macOS checkout, theexamples/applecontainer-spike/ignore, and the.dap/review/engineering.mddirectives that described two backends and told the reviewer to skip the Swift bridge. (.gitignore's*.dylibstays — it's in the generic# Binariesblock, not Apple-specific.)The CLI keeps
--runtimeas the seam for wiring a future backend, but it now accepts onlydocker:Everything else is comment-only: doc comments describing backend-agnostic behavior no longer cite Apple as the divergent implementation, and
compose/plan_test.go's all-false capability fixture is renamedappleCaps→limitedCaps(it was alwaysruntime.Capabilities{}, never an Apple-specific profile — no assertion depended on the name).Deliberately left in place
The
Capabilitiesgating machinery.runtime/docker/compose_primitives.go:196-205returnstruefor all six fields, and every field's doc comment records an Apple limitation — so with Apple gone,refuseBackendGatedincompose/plan.go(64 lines) and the/etc/hostsfallback behindServiceNameDNSincompose/orchestrator.go:225(~100 lines incl.patchHostsFiles,containerIP,renderHostsBlock,appendHostsBlock) are unreachable. Two public error types are also left without a producer:runtime.BuilderUnavailableErrorandruntime.UnsupportedOptionErrorwere only ever constructed by the Apple backend.That is a separate concern and a separate decision — whether
Capabilities()stays on theRuntimeinterface as the extension seam, or goes with the divergence it was built to describe — so it is deliberately not in this PR. The Apple citations in those comments are the record of why each flag exists, and deleting them here would erase the justification while the flags remain. Note this means the/etc/hostspath is now untested: its only coverage wasapplecontainer_compose_native_test.go, deleted above.design/compose-native.md's Apple sections, for the reasondesign/README.md:10-15gives: the records document the state of the world when written, the code is authoritative when they disagree, and probe results and rejected alternatives are kept because the reasoning is still useful. #124 applied the same rule — it deleteddesign/podman-backend.mdwith the code and leftcompose-native.mdalone. The deletedruntime-applecontainer.mdremains readable at tagv0.4.3.Verification
make lint testgreen —golangci-lint: 0 issues;go vetclean under both the default andintegrationtags;gofmtclean.go test -tags=integration -liststill enumerates 37 integration tests (the Apple suites were darwin/arm64-gated, so Linux coverage is unchanged).--runtime applecontaineris refused as above,--runtime dockerstill reaches the runtime.Summary by CodeRabbit