ci: remove Apple Containers jobs - #121
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 27 days. After that, they cost $0.25 per reviewed file. Or wait 57 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 65 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![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 0cc5e7c
Reviewed PR #121 at head 0cc5e7c against base e46401a: one changed workflow file in one commit (2 additions, 160 deletions). Read the repository engineering directives at the required base SHA, CONTRIBUTING.md, the complete changed workflow, the Apple backend build constraints/tests, README, design record, and CI history. Focused on backend-specific CI coverage and documentation/contract conformance; no application runtime paths were changed. PRD.md is referenced by the repository documents but is absent at the base SHA, and no CLAUDE.md or AGENTS.md exists, so those could not be reviewed. I did not execute project code or tests. Inline-thread replies will not reach this review; respond in the pull-request conversation if clarification is needed.
Commented inline
- [MEDIUM] [R8] Removing the only macOS job leaves the supported Apple backend completely outside CI —
.github/workflows/ci.yml:84 - [LOW] [D11] README still claims CI runs the removed macOS Apple-container job —
.github/workflows/ci.yml:84
Verdict
ADVISORY — findings worth reading, none of them blocking.
| # Apple-container backend lives in runtime/applecontainer and is | ||
| # darwin/arm64-only (see build tags). This job builds the Swift | ||
| # bridge and runs the Go test suite on macOS so we get coverage of | ||
| # cgo compilation, go:embed of libACBridge.dylib, and the | ||
| # daemon-free unit tests. Daemon-dependent tests skip cleanly via | ||
| # runtimeOrSkip when Apple's `container` apiserver isn't running. | ||
| test-darwin: | ||
| runs-on: macos-26 | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| go: ["1.25", "1.26"] | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: ${{ matrix.go }} | ||
| cache: true | ||
| - name: Select Xcode with Swift 6.2 | ||
| # apple/container 0.12.3 declares swift-tools-version 6.2; | ||
| # the macos-15 image ships an older Xcode by default. Pick | ||
| # the newest installed so SwiftPM can resolve the package. | ||
| run: sudo xcode-select -s /Applications/Xcode_latest.app || sudo xcode-select -s "$(ls -d /Applications/Xcode_*.app | sort -V | tail -1)" | ||
| - name: Cache SwiftPM artifacts | ||
| uses: actions/cache@v5 | ||
| with: | ||
| path: | | ||
| applecontainer-bridge/.build | ||
| ~/Library/Caches/org.swift.swiftpm | ||
| # Key on Package.resolved so the cache busts when dependency | ||
| # versions move. Bump the `v1` prefix to force a full miss | ||
| # after a toolchain change that breaks artifact compat. | ||
| key: swiftpm-v1-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('applecontainer-bridge/Package.resolved') }} | ||
| restore-keys: | | ||
| swiftpm-v1-${{ runner.os }}-${{ runner.arch }}- | ||
| - name: Build Swift bridge | ||
| run: make bridge | ||
| - run: go vet ./... | ||
| - run: go test -race -count=1 ./... |
There was a problem hiding this comment.
[MEDIUM] [R8] Removing the only macOS job leaves the supported Apple backend completely outside CI
- Witness: The deleted
test-darwinjob was the only workflow job that ranmake bridgeandgo vet ./.../go test ./...on macOS. Verbatim checks on the new workflow:grep -nE 'go test|go vet|runs-on|test-darwin|test-integration-darwin' .github/workflows/ci.ymlreturns onlyruns-on: ubuntu-latestand the Linux/Podman commands; separately,grep -RIl '^//go:build darwin && arm64' runtime/applecontainer test/integrationreturns the Apple runtime and test files. Thus a broken cgo/Swift embed or any Darwin-only compile/test regression can pass every remaining CI job. - Consumer:
runtime/applecontainer/runtime_darwin_arm64.go:1is guarded by//go:build darwin && arm64, so the remaining Ubuntugo test ./...at.github/workflows/ci.yml:44excludes the supported Apple implementation and its Darwin tests; macOS consumers can consequently receive a release that CI never compiled. - Fix: Retain a Darwin/arm64 compile-and-unit-test job (it can omit the unavailable live-daemon integration job), or add an explicit supported-backend validation elsewhere that builds the bridge and runs the Darwin-tagged package/tests.
| # Apple-container backend lives in runtime/applecontainer and is | ||
| # darwin/arm64-only (see build tags). This job builds the Swift | ||
| # bridge and runs the Go test suite on macOS so we get coverage of | ||
| # cgo compilation, go:embed of libACBridge.dylib, and the | ||
| # daemon-free unit tests. Daemon-dependent tests skip cleanly via | ||
| # runtimeOrSkip when Apple's `container` apiserver isn't running. | ||
| test-darwin: | ||
| runs-on: macos-26 | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| go: ["1.25", "1.26"] | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: ${{ matrix.go }} | ||
| cache: true | ||
| - name: Select Xcode with Swift 6.2 | ||
| # apple/container 0.12.3 declares swift-tools-version 6.2; | ||
| # the macos-15 image ships an older Xcode by default. Pick | ||
| # the newest installed so SwiftPM can resolve the package. | ||
| run: sudo xcode-select -s /Applications/Xcode_latest.app || sudo xcode-select -s "$(ls -d /Applications/Xcode_*.app | sort -V | tail -1)" | ||
| - name: Cache SwiftPM artifacts | ||
| uses: actions/cache@v5 | ||
| with: | ||
| path: | | ||
| applecontainer-bridge/.build | ||
| ~/Library/Caches/org.swift.swiftpm | ||
| # Key on Package.resolved so the cache busts when dependency | ||
| # versions move. Bump the `v1` prefix to force a full miss | ||
| # after a toolchain change that breaks artifact compat. | ||
| key: swiftpm-v1-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('applecontainer-bridge/Package.resolved') }} | ||
| restore-keys: | | ||
| swiftpm-v1-${{ runner.os }}-${{ runner.arch }}- | ||
| - name: Build Swift bridge | ||
| run: make bridge | ||
| - run: go vet ./... | ||
| - run: go test -race -count=1 ./... |
There was a problem hiding this comment.
[LOW] [D11] README still claims CI runs the removed macOS Apple-container job
- Witness: The new workflow contains no
test-darwinortest-integration-darwinjob (the deleted jobs are the only matching entries in the base workflow), butREADME.md:297-301still says:CI runs both the Linux + Docker suite and a macos-26 job that builds the Swift bridge and runs the applecontainer unit tests.A contributor or user checking the documented CI guarantee will therefore be told that Apple builds are validated when they are no longer run. - Consumer:
README.md:297-301is the documentation consumed by contributors/users to determine which backend CI validates; reading it yields a macOS validation guarantee that.github/workflows/ci.ymlno longer provides. - Fix: Update the Tests section to state that Apple-container tests are not currently run in hosted CI and explain the supported/manual validation path, or restore a matching macOS job.
There was a problem hiding this comment.
Code Review — head df112b0
Re-reviewed the synchronized head df112b0 (two commits, two changed files; the new commit only updates README.md). Verified the documentation finding is fixed: README now states Apple-container integration tests are not run in hosted CI. Rechecked the remaining CI coverage issue against the new workflow and Darwin-only build constraints; no macOS compile/unit-test job was restored, so it remains open. No additional defects were found in the README update. Directives were again applied from base e46401a; PRD.md remains absent at that SHA. No project code or tests were executed.
Commented inline
- [MEDIUM] [R8] Removing the only macOS job leaves the supported Apple backend completely outside CI —
.github/workflows/ci.yml:84
Verdict
ADVISORY — findings worth reading, none of them blocking.
| # Apple-container backend lives in runtime/applecontainer and is | ||
| # darwin/arm64-only (see build tags). This job builds the Swift | ||
| # bridge and runs the Go test suite on macOS so we get coverage of | ||
| # cgo compilation, go:embed of libACBridge.dylib, and the | ||
| # daemon-free unit tests. Daemon-dependent tests skip cleanly via | ||
| # runtimeOrSkip when Apple's `container` apiserver isn't running. | ||
| test-darwin: | ||
| runs-on: macos-26 | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| go: ["1.25", "1.26"] | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: ${{ matrix.go }} | ||
| cache: true | ||
| - name: Select Xcode with Swift 6.2 | ||
| # apple/container 0.12.3 declares swift-tools-version 6.2; | ||
| # the macos-15 image ships an older Xcode by default. Pick | ||
| # the newest installed so SwiftPM can resolve the package. | ||
| run: sudo xcode-select -s /Applications/Xcode_latest.app || sudo xcode-select -s "$(ls -d /Applications/Xcode_*.app | sort -V | tail -1)" | ||
| - name: Cache SwiftPM artifacts | ||
| uses: actions/cache@v5 | ||
| with: | ||
| path: | | ||
| applecontainer-bridge/.build | ||
| ~/Library/Caches/org.swift.swiftpm | ||
| # Key on Package.resolved so the cache busts when dependency | ||
| # versions move. Bump the `v1` prefix to force a full miss | ||
| # after a toolchain change that breaks artifact compat. | ||
| key: swiftpm-v1-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('applecontainer-bridge/Package.resolved') }} | ||
| restore-keys: | | ||
| swiftpm-v1-${{ runner.os }}-${{ runner.arch }}- | ||
| - name: Build Swift bridge | ||
| run: make bridge | ||
| - run: go vet ./... | ||
| - run: go test -race -count=1 ./... |
There was a problem hiding this comment.
[MEDIUM] [R8] Removing the only macOS job leaves the supported Apple backend completely outside CI
- Witness: The deleted
test-darwinjob was the only workflow job that ranmake bridgeandgo vet ./.../go test ./...on macOS. Verbatim checks on the new workflow:grep -nE 'go test|go vet|runs-on|test-darwin|test-integration-darwin' .github/workflows/ci.ymlreturns onlyruns-on: ubuntu-latestand the Linux/Podman commands; separately,grep -RIl '^//go:build darwin && arm64' runtime/applecontainer test/integrationreturns the Apple runtime and test files. Thus a broken cgo/Swift embed or any Darwin-only compile/test regression can pass every remaining CI job. - Consumer:
runtime/applecontainer/runtime_darwin_arm64.go:1is guarded by//go:build darwin && arm64, so the remaining Ubuntugo test ./...at.github/workflows/ci.yml:44excludes the supported Apple implementation and its Darwin tests; macOS consumers can consequently receive a release that CI never compiled. - Fix: Retain a Darwin/arm64 compile-and-unit-test job (it can omit the unavailable live-daemon integration job), or add an explicit supported-backend validation elsewhere that builds the bridge and runs the Darwin-tagged package/tests.
|
Counterargument for DAP finding 3 (“Removing the only macOS job leaves the supported Apple backend completely outside CI”): The repository directive R8 requires backend-specific behavior to have backend coverage or an explicit statement of which backend is untested. This PR explicitly removes both Apple matrices, and commit Given that the unsupported hosted-CI path and the remaining manual test path are now explicit, absence of a hosted macOS job is an intentional, documented validation boundary rather than an undisclosed coverage defect. Please re-check that argument against R8 and dismiss finding 3 if you agree that its claim no longer holds as a review defect. |
There was a problem hiding this comment.
Code Review — head df112b0
Reviewed the two-commit range at df112b045e595da0925a08bbb9a79cf199b068ad (2 files changed: .github/workflows/ci.yml and README.md). Read the applicable engineering.md directives at the supplied base, plus CONTRIBUTING.md and the referenced Apple runtime design record; PRD.md is absent at the directives base. Checked the full workflow and README and traced the remaining Linux commands against the darwin/arm64 build constraints and Apple CLI consumer. Covered CI/test coverage and documentation/design conformance (R5/R8), workflow dependency shape, and platform-specific build coverage. One R8 finding was recorded: the supported Apple backend is now completely outside CI. No runtime, tenancy, migration, event-flow, or untrusted-input changes were present. Replies in inline review threads will not reach this run; respond in the pull request conversation.
Commented inline
- [MEDIUM] [R8] Removing the macOS job leaves the supported darwin/arm64 backend completely uncompiled in CI —
.github/workflows/ci.yml:84
Verdict
ADVISORY — findings worth reading, none of them blocking.
| test-darwin: | ||
| runs-on: macos-26 | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| go: ["1.25", "1.26"] | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: ${{ matrix.go }} | ||
| cache: true | ||
| - name: Select Xcode with Swift 6.2 | ||
| # apple/container 0.12.3 declares swift-tools-version 6.2; | ||
| # the macos-15 image ships an older Xcode by default. Pick | ||
| # the newest installed so SwiftPM can resolve the package. | ||
| run: sudo xcode-select -s /Applications/Xcode_latest.app || sudo xcode-select -s "$(ls -d /Applications/Xcode_*.app | sort -V | tail -1)" | ||
| - name: Cache SwiftPM artifacts | ||
| uses: actions/cache@v5 | ||
| with: | ||
| path: | | ||
| applecontainer-bridge/.build | ||
| ~/Library/Caches/org.swift.swiftpm | ||
| # Key on Package.resolved so the cache busts when dependency | ||
| # versions move. Bump the `v1` prefix to force a full miss | ||
| # after a toolchain change that breaks artifact compat. | ||
| key: swiftpm-v1-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('applecontainer-bridge/Package.resolved') }} | ||
| restore-keys: | | ||
| swiftpm-v1-${{ runner.os }}-${{ runner.arch }}- | ||
| - name: Build Swift bridge | ||
| run: make bridge | ||
| - run: go vet ./... | ||
| - run: go test -race -count=1 ./... |
There was a problem hiding this comment.
[MEDIUM] [R8] Removing the macOS job leaves the supported darwin/arm64 backend completely uncompiled in CI
- Witness: The deleted
test-darwinjob was the only workflow that ranmake bridge,go vet ./..., andgo test -race -count=1 ./...on macOS. The remaining commands at.github/workflows/ci.yml:43-44run on Ubuntu, while every implementation and test file inruntime/applecontaineris guarded by//go:build darwin && arm64(for exampleruntime/applecontainer/runtime_darwin_arm64.go:1andruntime/applecontainer/embed_darwin_arm64.go:1), so Linux CI cannot compile the cgo/Swift embedding path or execute these tests. The repository design also specifies a macOS arm64 runner for this backend atdesign/runtime-applecontainer.md:200-202. - Consumer:
cmd/devcontainer/runtime_applecontainer_darwin_arm64.go:1-16imports and constructsruntime/applecontaineron the supported macOS target, but no remaining CI job builds that target; a broken cgo ABI, missing embedded dylib, or darwin-only compile error can therefore merge unnoticed. - Fix: Keep a macOS arm64 compile/unit-test job (the deleted
test-darwinjob is sufficient and does not require a live daemon because its daemon-dependent tests skip), and remove only the hosted-daemon integration job if virtualization is unavailable. Alternatively provide an equivalent self-hosted macOS arm64 workflow.
1 participant
Summary
Validation
actionlint .github/workflows/ci.ymlgit diff --check