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. |
1 participant
Summary
Validation
actionlint .github/workflows/ci.ymlgit diff --check