feat(ci): re-enable Swift sidecar CI on macos-26 - #508
Conversation
|
🏭 Fabrik — stage: Implement Re-enabled Used 35/100 turns, 2.7M input (64 raw + 2.6M cache-read + 75k cache-write) / 12k output tokens. |
![handarbeit-pruefer[bot]](https://avatars.githubusercontent.com/in/4408765?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the re-enablement of Swift sidecar CI: .github/workflows/swift.yml (removes if: false, pins macos-26, adds an explicit swift build step), the corresponding watch-list addition in .github/workflows/ci-failure-notify.yml, the README note, and new ADR-0502 plus its docs/adr/index.md entry. The rationale for pinning an explicit runner label instead of floating on macos-latest, and for not yet making the check required, is well-documented and consistent across the workflow header, the ADR, and ci-failure-notify.yml's own comments. (Note: this environment's git history only contains the PR's tip commit with no reachable parent, so this review is based on inspecting the final file contents at HEAD rather than a literal git diff against main.)
One structural gap: swift.yml's on: block declares both push: (with no branches: filter) and pull_request: over the same paths, with no concurrency: group to dedupe them — the exact double-trigger pattern ci.yml's own header comment documents having previously caused two concurrent runs per PR push and had to fix by scoping push to main/maint/** and adding a concurrency group. swift.yml doesn't inherit that fix, so every push to an open PR branch touching native/local-inference/** will kick off two concurrent macos-26 runs (an expensive runner class) for the same commit.
283b0e7 to
44ca607
Compare
|
🏭 Fabrik — stage: Implement (review feedback addressed) The review thread is resolved. No issue body changes are needed — this was a workflow-file fix, not a spec change. Fixed the double-trigger gap flagged in review: Threads addressed:
Resolved 1 review thread(s) across 2 comment(s). |
There was a problem hiding this comment.
Reviewed the CI re-enablement changes for the Swift sidecar workflow: .github/workflows/swift.yml, .github/workflows/ci-failure-notify.yml, native/local-inference/README.md, and the new ADR-0502 (plus index entry). The working tree reflects the fix for the previously open thread (missing branches:/concurrency: scoping causing duplicate concurrent runs on PR pushes) — the push trigger is now scoped to main/maint/** plus a release-tag glob matching the proven pattern already used by this repo's release.yml, and a concurrency group mirroring ci.yml's established fix has been added. The ci-failure-notify.yml watch-list entry matches the workflow's name: exactly, and the ADR's rationale for pinning macos-26 and deferring required-check status is sound and consistent with the rest of the diff. No new defects found.
|
🏭 Fabrik — stage: Review Review FindingsFixed
Verified
Not blocking
PR is ready for human review. Reviewed the Swift sidecar CI re-enablement PR. Found and fixed one issue: Used 28/100 turns, 1.2M input (34 raw + 1.2M cache-read + 65k cache-write) / 4k output tokens. |
|
🏭 Fabrik — stage: Review (review feedback addressed) The new comment is No action taken: the bot review comment reports "No new defects found" and requests nothing. Working tree is clean with all prior fixes already committed and pushed. Threads addressed: Resolved 0 review thread(s) across 1 comment(s). |
There was a problem hiding this comment.
Reviewed the re-enablement of Swift sidecar CI: .github/workflows/swift.yml (removes if: false, pins runs-on: macos-26, adds a swift build step, scopes push to main/maint/**+tags, and adds a concurrency group), .github/workflows/ci-failure-notify.yml (adds "Swift sidecar CI" to the watch list), native/local-inference/README.md, and the new ADR-0502 plus its index entry. The previously-flagged double-trigger issue (unscoped push + no concurrency group) is now fixed — push is scoped to main/maint/**/tags and a concurrency group with cancel-in-progress: true is present. The workflow name (Swift sidecar CI) matches what's added to the notify workflow's watch list, and the ADR's reasoning for pinning an explicit runner label and deferring required-check status is coherent and consistent with the code. No defects found.
|
🏭 Fabrik — stage: Validate No TODO/FIXME markers found. All requirements are verified and the branch is clean. Compiling the final validation report. Validation ReportRequirements: 7/7 passed
Test Suite / Live CI VerificationI independently reproduced the Swift job locally (this machine happens to run macOS 26.5.1 / Swift 6.3.3) and cross-checked against the actual GitHub-hosted
Correction to Review stage's assessmentReview stage stated this is a "docs/workflow-only diff... Rust jobs correctly show as skipped per ADR-0322." That's inaccurate: the classifier's Regressions: None detectedNo Swift source, fixtures, or Rust code were touched by this PR. The only newly-observed failure (#512) is pre-existing and unrelated to this diff. Branch State
Issues Found
Verdict: READY TO MERGEValidation passed — all 7 FRs verified via a live reproduction of both the local and GitHub-hosted Used 51/100 turns, 4.7M input (86 raw + 4.6M cache-read + 122k cache-write) / 32k output tokens. |
Removes the if: false gate that has left native/local-inference/** with no CI signal since 2026-05-30 (every run, including release tags v0.13.0-v0.13.4, reported skipped). GitHub's macos-latest migration to macOS 26 completed ~2026-07-15, so the original tools-version blocker no longer applies. Pins the explicit macos-26 runner label instead of macos-latest so a future SDK requirement bump fails loudly rather than silently skipping again. Adds an explicit swift build step ahead of swift test so FR-001/FR-003 each get independent log visibility. Also wires "Swift sidecar CI" into ci-failure-notify.yml's workflows: list, per that workflow's own header comment instructing this "when it's re-enabled" (ADR-0298).
Adds ADR-0502 documenting why the re-enabled swift.yml pins the explicit macos-26 runner label instead of floating on macos-latest, and why the check is not yet a required branch-protection status check. Also updates the sidecar README to mention the new CI job alongside the existing local swift test instruction.
Review feedback: swift.yml's push trigger had no branches filter and no concurrency group, so a push to an open PR branch touching native/local-inference/** fired two concurrent macos-26 runs on the same commit (one via push, one via pull_request) — the same double-trigger pattern ci.yml's own header documents having hit and fixed. Scope push to main/maint/** plus release tags (a plain branches filter would otherwise exclude tag refs, breaking FR-002/SC-002's release-tag signal) and add a concurrency group that cancels a superseded in-flight run, mirroring ci.yml's fix.
Restores the ADR-0502 index row dropped by an --ours conflict resolution during the rebase onto main (which had landed ADR-0503 adjacent to it in docs/adr/index.md).
64a21b5 to
50981d0
Compare
There was a problem hiding this comment.
Reviewed .github/workflows/swift.yml, .github/workflows/ci-failure-notify.yml, native/local-inference/README.md, and the new ADR-0502 (plus its index entry). Note: this checkout has no main ref or history available (single detached commit, no remote configured), so I reviewed the current file contents directly rather than a diff against main.
The previously-resolved concurrency/push-scoping thread is correctly addressed: push is scoped to main/maint/** plus tags, and a concurrency group with cancel-in-progress: true is present. The ci-failure-notify.yml addition and README/ADR changes match the PR description and are internally consistent.
One real defect: the tags: glob pattern intended to match release tags (**[0-9]+.[0-9]+.[0-9]+*) uses + as if it were a regex quantifier, but GitHub Actions tag filters use glob/minimatch semantics where a bare + outside an extglob group is a literal character. Verified against minimatch that this pattern does not match any of the actual release tags mentioned in the PR's own comments (v0.13.0–v0.13.4) or any plausible semver tag — the tags trigger is effectively dead code, silently defeating the stated goal (documented in the workflow's own header) of giving release tags their own CI trigger.
|
🏭 Fabrik — stage: Validate (review feedback addressed) SummaryInvestigated No code change made — changing it would create an unexplained divergence from PR state: Threads addressed:
Resolved 1 review thread(s) across 2 comment(s). |
1 participant
Closes #502
Summary
native/local-inference/**has had zero CI signal since 2026-05-30 —.github/workflows/swift.yml's job wasif: falsebecause GitHub'smacos-latestrunner image was still macOS 15 + Swift 6.1, while the package requiresswift-tools-version: 6.2/platforms: [.macOS(.v26)]. Every run since, including release tags v0.13.0–v0.13.4, reportedskipped.GitHub's
macos-latestmigration to macOS 26 completed by ~2026-07-15 (peractions/runner-images#14167), so the original blocker should no longer apply. This PR re-enables the job.Changes
.github/workflows/swift.yml: removesif: false; pinsruns-on: macos-26(an explicit runner label) instead of resuming on the floatingmacos-latestalias, so a future platform-requirement bump fails loudly instead of silently skipping again; adds an explicitswift buildstep ahead of the existingswift teststep so build vs. test failures are independently visible in the log; rewrites the header comment to describe the current state..github/workflows/ci-failure-notify.yml: adds"Swift sidecar CI"to theworkflow_run.workflows:watch list and updates its header comment, per that workflow's own instruction to do this "when it's re-enabled" (ADR-0298) — closes the gap where a post-merge failure of this job onmainwould otherwise go unnoticed.native/local-inference/README.md: notes the new CI job in the Tests section, alongside (not replacing) the existing localswift testpre-push instruction.docs/adr/0502-pin-explicit-macos-runner-for-swift-sidecar-ci.md(new) +docs/adr/index.md: records whymacos-26is pinned rather than floating, and why the check is intentionally not yet a required branch-protection status check (this PR's own run is the first live signal from this job in three months, so making it required before that run is observed risks blocking unrelated PRs on a runner/toolchain quirk rather than a real defect).No test-filtering logic was needed: research established the existing
Tests/LocalInferenceTests/suite already runs entirely on stub.mlpackagefixtures and mock adapters, with no dependency on the ~400 MB CoreML embedding asset or live Foundation Models availability.How to test
This PR's own
Swift sidecar CIcheck is the test — watch it on this PR to confirm it reports a real pass/fail outcome instead ofskipped. If it fails for a runner/toolchain reason unrelated to sidecar code (e.g.macos-26's default Xcode not resolving to Swift 6.2+), that's the User Story 2 / FR-006 fallback path: capture the evidence and update the ADR rather than silently reintroducingif: false.