chore(ci): migrate workflow to SyntaxKit structure with strict lint e…#9
chore(ci): migrate workflow to SyntaxKit structure with strict lint e…#9leogdion wants to merge 7 commits intov1.0.0-alpha.2from
Conversation
…nforcement - Add configure job for dynamic matrix selection (full vs minimal by branch) - Split build-macos into build-macos (SPM only) + build-macos-full (full platform matrix) - Add concurrency group with cancel-in-progress - Update triggers: branches-ignore → push/pull_request with paths-ignore - Update action versions: checkout@v6, swift-build@v1, swift-coverage-action@v5, codecov@v6 - Move LINT_MODE=STRICT from job env to step env (matches SundialKit fix) - Update mise-action@v2 → @v4 with simplified config - Gate coverage steps with contains-code-coverage output check Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Review: CI Migration to SyntaxKit StructureThis is a well-structured CI refactor with some good improvements. Here's my analysis: ✅ Positive Changes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v1.0.0-alpha.2 #9 +/- ##
==================================================
+ Coverage 74.16% 74.44% +0.28%
==================================================
Files 44 47 +3
Lines 1862 1902 +40
==================================================
+ Hits 1381 1416 +35
- Misses 481 486 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if [[ "${{ github.ref }}" == "refs/heads/main" || "${{ github.event_name }}" == "pull_request" ]]; then | ||
| echo "full-matrix=true" >> "$GITHUB_OUTPUT" | ||
| echo 'ubuntu-os=["noble","jammy"]' >> "$GITHUB_OUTPUT" | ||
| echo 'ubuntu-swift=[{"version":"6.1"},{"version":"6.2"},{"version":"6.3"}]' >> "$GITHUB_OUTPUT" |
| - type: macos | ||
| runs-on: macos-15 | ||
| xcode: "/Applications/Xcode_16.4.app" |
There was a problem hiding this comment.
remove the macOS and add back older iOS and watchOS
…iOS/watchOS - Add ubuntu-type output to configure job (wasm, wasm-embedded) - Wire ubuntu-type into build-ubuntu matrix, excluding Swift 6.1 from wasm types - Add build-windows job (windows-2022, windows-2025; Swift 6.1/6.2/6.3) - Add build-android job (Swift 6.2/6.3; API levels 33-36) - Remove macOS type entries from build-macos-full - Restore older iOS/watchOS builds on macos-15 with Xcode_16.4.app - Update lint job needs to include build-windows and build-android Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: CI Workflow Migration to SyntaxKit StructureThis is a well-structured CI refactor with several meaningful improvements. A few issues worth addressing before merging. Critical: Branch-Pinned Dependency
// Before
.package(url: "https://github.com/brightdigit/SundialKit.git", from: "2.0.0-alpha.1")
// After
.package(url: "https://github.com/brightdigit/SundialKit.git", branch: "atleast-v1.0.0-beta.4")Using a branch reference makes builds non-reproducible. If the branch is rebased, force-pushed, or deleted, all downstream builds break with no warning. The branch name ( Issues in the Workflow1. Both jobs use # build-macos (correct)
if: ${{ !cancelled() && needs.configure.result == 'success' }}
# build-windows / build-android (incomplete)
if: needs.configure.outputs.full-matrix == 'true'If if: ${{ !cancelled() && needs.configure.result == 'success' && needs.configure.outputs.full-matrix == 'true' }}2. PR trigger targets version tags as base branches pull_request:
branches:
- main
- 'v[0-9]*.[0-9]*.[0-9]*'Tags can't be PR base branches — this branch pattern only matches branches whose names look like version strings. If the intent is to support hotfix branches like 3. The original 4. Inconsistent The Ubuntu job explicitly sets this: - uses: sersoft-gmbh/swift-coverage-action@v5
with:
fail-on-empty-output: trueBut the macOS jobs ( 5. Unpinned major version actions
Improvements Worth Noting
Summary: The CI restructuring is solid, but the branch-pinned SundialKit dependency is a blocking concern for reproducibility. The missing cancellation guards on Windows/Android and the |
- Wrap NetworkObserver.start(queue:) in #if canImport(Dispatch) - Wrap MockPathMonitor in #if canImport(Dispatch) - Add WASMSupport.swift with isWasm flag to disable NetworkObserver tests on WASM - Split SundialLogger fallback into SundialLogger+Fallback.swift to fix one_declaration_per_file lint violation - Enable one_declaration_per_file SwiftLint rule Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR ReviewThis is a well-structured CI modernization with WASM compatibility additions. Here's my feedback: Issues
|
pathStatusStream, isExpensiveStream, and isConstrainedStream were spawning an inner Task to call pathUpdates(), causing a race on slow platforms (watchOS/tvOS) where the continuation was not registered before sendPath() fired, dropping the second value. Replace with direct synchronous continuation registration (matching the pathUpdates() pattern), backed by dedicated continuation dictionaries. Extract internal handlers and remove helpers to NetworkObserver+Handlers.swift to keep the main file within the 225-line lint limit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
PR Review: CI migration to SyntaxKit structure with strict lint enforcement This PR is a solid step forward for cross-platform support and CI efficiency. CI WORKFLOW CHANGES Positive changes:
Issues/Concerns:
SOURCE CODE CHANGES NetworkObserver refactor: Concern: .unbounded means the buffer can grow without bound if a subscriber is slow. This is a behavioral change from the previous default (.bufferingNewest(1)) and worth documenting on the stream properties. NetworkObserver+Init.swift: Properties changed from private to internal: Necessary to support the file-split to NetworkObserver+Handlers.swift. Acceptable given Swift file-scope semantics for extensions. TEST CHANGES Potential WASM compilation issue: NetworkObserverTests.swift is decorated with .disabled(if: isWasm) which is a runtime check. However, MockPathMonitor is guarded at compile time with canImport(Dispatch). If any test body inside NetworkObserverTests directly references MockPathMonitor, those tests would fail to compile on WASM even though the suite is runtime-disabled. The .disabled(if:) trait only suppresses test execution; it does not suppress compilation. If test bodies reference MockPathMonitor, they need canImport(Dispatch) compile guards as well, or the entire test file should be wrapped in such a guard. PACKAGE.SWIFT - BRANCH DEPENDENCY The dependency is pinned to branch atleast-v1.0.0-beta.4 rather than a version tag. Branch-pinned dependencies are non-reproducible: if the branch is force-pushed or rebased, Package.resolved can become stale and builds can silently change behavior. This looks intentional for in-progress development but should be resolved to a tagged version before merging to main. SWIFTLINT.YML
SWIFT-FORMAT
SUMMARY CI structure/efficiency: Solid improvement The branch dependency and the potential WASM compile issue are the two items to resolve before this is merge-ready. |
…on-Apple platforms The recent commit wrapped NetworkObserver source in #if canImport(Network), but tests still imported SundialKitNetwork unconditionally, breaking swift test on Linux/WASM. Add canImport(Network) guards to all network test and mock files, and add hasNetwork trait to disable the suite at runtime. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review: CI Migration + SyntaxKit StructureThis is a solid CI modernization PR. Here are my findings across a few categories: Potential Issues1. Branch-Pinned SundialKit Dependency (Medium)// Package.swift
.package(url: "https://github.com/brightdigit/SundialKit.git", branch: "atleast-v1.0.0-beta.4")Pinning to a branch rather than a version tag makes builds non-reproducible in the long run — the next 2. Inconsistent
|
…ule OptionSet Use .enabled(if:) with SupportedModule instead of .disabled(if:) with legacy boolean constants for clearer test suite conditional execution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review: chore(ci): migrate workflow to SyntaxKit structure with strict lint enforcementThis is a solid infrastructure PR. The CI restructuring, cross-platform source guards, and observer refactoring are all well-reasoned. A few issues worth addressing before merge: Bugs / Issues1. Unstable branch dependency in .package(url: "...", branch: "atleast-v1.0.0-beta.4")Branch-based dependencies are not reproducible — a force-push to that branch would silently break builds. This should be either a tagged version ( 2. Both jobs use only: if: needs.configure.outputs.full-matrix == 'true'But if: ${{ !cancelled() && needs.configure.result == 'success' && needs.configure.outputs.full-matrix == 'true' }}Without 3. The matrix defines flags: swift-${{ matrix.swift.version }},windowsThis produces 4.
Design Concerns5. Properties like 6. Redundant Both jobs redeclare Minor Nits7. The file is 8. The build step uses What Looks Good
Overall a well-structured change — the issues above are mostly polish and a couple of correctness gaps in the new CI jobs. |
…nforcement
Perform an AI-assisted review on