chore(ci): overhaul CI matrix, add Windows/Android builds, clean up tooling scaffolding#78
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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 |
leogdion
left a comment
leogdion
left a comment
There was a problem hiding this comment.
update swift-format and SwiftLint rules
| @@ -29,4 +29,4 @@ | |||
| } | |||
There was a problem hiding this comment.
these need to be updated to use the new swift extension
.github/workflows/SundialKit.yml
Outdated
| exclude: | ||
| - swift: {version: "6.1"} | ||
| type: wasm | ||
| - swift: {version: "6.1"} | ||
| type: wasm-embedded | ||
| - swift: {version: "6.2"} | ||
| type: wasm | ||
| - swift: {version: "6.2"} | ||
| type: wasm-embedded |
.github/workflows/SundialKit.yml
Outdated
| scheme: ${{ env.PACKAGE_NAME }} | ||
| - uses: sersoft-gmbh/swift-coverage-action@v4 | ||
| type: ${{ matrix.type }} | ||
| wasmtime-version: 41.0.3 |
| - name: Install curl | ||
| if: steps.build.outputs.contains-code-coverage == 'true' | ||
| run: | | ||
| apt-get update -q | ||
| apt-get install -y curl |
There was a problem hiding this comment.
comment this out for now
.github/workflows/SundialKit.yml
Outdated
| - name: Enable git symlinks | ||
| shell: pwsh | ||
| run: git config --global core.symlinks true |
There was a problem hiding this comment.
make all matrices fail fast false temporarily
- Comment out wasm builds, windows job, and install curl step in CI - Add fail-fast: false to build-ubuntu matrix - Update devcontainers to use swift-server.swift extension - Migrate .swiftlint.yml and .swift-format from Swift-App-Template Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: CI/CD & Tooling Configuration UpdateThis is a solid infrastructure modernization. Here's my feedback organized by severity: Potential Issues
Same concern applies to Release tags run a reduced test matrix # In configure job, consider:
if [[ "${{ github.ref }}" == "refs/heads/main" || "${{ github.event_name }}" == "pull_request" || "${{ github.ref }}" == refs/tags/* ]]; then
Observations / Minor ItemsCommented-out code is noisy
If these changes are intentional (e.g. the leading underscore change is useful for actor isolation patterns), they're reasonable. But the doc comment relaxations may allow API documentation to degrade over time unless enforced elsewhere.
Positive Changes
Summary: The architecture of this workflow update is good. The main blocker to address is verifying |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v2.0.0-alpha.2 #78 +/- ##
==================================================
+ Coverage 65.20% 65.66% +0.45%
==================================================
Files 31 32 +1
Lines 526 533 +7
==================================================
+ Hits 343 350 +7
Misses 183 183
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:
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: CI/CD & Tooling OverhaulThis is a solid infrastructure upgrade overall. Here are observations organized by area: ✅ Positives
Issues & ConcernsCI Workflow (
|
- Set LINT_MODE=STRICT in CI lint job so failures actually fail the job
- Fix all SwiftLint strict-mode violations:
- Wrap long doc comment lines (line_length > 90 chars) across sources and tests
- Break long function signatures onto multiple lines
- Split large test structs exceeding 125-line body length into extensions
- Extract TestBinaryMessage and TestObserverImpl to dedicated files
to satisfy one_declaration_per_file rule
- Rename NetworkMonitorConcurrencyTests to NetworkMonitorTests+ConcurrencyTests
to satisfy file_name rule
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: chore: Update SundialKit.ymlThis is a substantial infrastructure PR with a workflow overhaul, tooling cleanup, source refactors, and test reorganization. Overall direction is good — here are the observations: CI/CD Workflow (
|
PR Review
Missing Labels / ConventionsPer CLAUDE.md, component labels are required. Based on the changes, at minimum these apply:
CI Workflow (
|
| Area | Assessment |
|---|---|
| CI workflow structure | ✅ Good overall, see checkout@v6 concern |
| New platform targets (Windows, Android) | ✅ Solid addition |
.swift-format changes |
|
.swiftlint.yml changes |
line_length: [90, 90] may be too aggressive |
SundialLogger refactor |
✅ Cleaner and correct |
| PR scope/title | |
| Labels | ❌ Missing required component labels per CLAUDE.md |
The most critical item to resolve before merge is actions/checkout@v6 — if that version doesn't exist, CI will fail on every job in this workflow.
PR Review:
|
| Severity | Issue |
|---|---|
| Critical | actions/checkout@v6 is not a valid action version — will break all CI jobs |
| Warning | WASM ubuntu-type matrix output is computed but never consumed (commented out) |
| Warning | AllPublicDeclarationsHaveDocumentation + ValidateDocumentationComments disabled together |
| Minor | File header says ObserverRegistryTestHelpers.swift in TestObserverImpl.swift |
| Minor | Copyright year inconsistency (2025 in new test files, 2026 elsewhere) |
The actions/checkout@v6 issue should be resolved before merging.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: chore: Update SundialKit.ymlThis PR does a lot of good work — CI modernization, cross-platform expansion, and lint enforcement. Here's targeted feedback by area. CI/CD:
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ort(Dispatch) DispatchQueue is not available on WASM, so wrap NetworkMonitoring, PathMonitor, NetworkMonitor, and PingCoordinator with #if canImport(Dispatch). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: chore: Update SundialKit.yml
✅ Positives
Issues & ObservationsCI/CD1. echo 'ubuntu-type=["","wasm","wasm-embedded"]' >> "$GITHUB_OUTPUT"SundialKit uses 2. The main workflow was updated to 3. - name: Upload coverage to Codecov
if: steps.build.outputs.contains-code-coverage == 'true'
uses: codecov/codecov-action@v6
with:
fail_ci_if_error: true
flags: swift-${{ matrix.swift.version }},windows
...
# No 'files:' parameterUnlike the Ubuntu job which passes 4. The non-full Linting & Formatting5. These two changes together substantially reduce documentation enforcement for a library package. Public API consumers rely on inline docs for discovery. If these were relaxed to address specific noisy false positives, it's worth commenting why in the config. If the intent is to let docs slide, that warrants a separate discussion — the CLAUDE.md architecture section implies this codebase prioritizes documentation. 6. Line length set to 90/90 (warning = error) line_length:
- 90
- 90Setting both thresholds to the same value eliminates the warning-before-error gradient. Any line exceeding 90 characters immediately errors. This is quite strict — consider 90/120 or similar so developers have a warning window before it blocks CI. 7. Tightened thresholds may break existing code in CI The following are significantly tightened from the previous config:
Have these been validated against the existing source? If not, CI may fail immediately on merge. 8. Inconsistent commenting style in #- closure_body_length # ← no space after #
# - discouraged_optional_boolean # ← space after #These commented-out entries in the Minor
SummaryThe CI restructure (dynamic matrix, Windows support, concurrency control) is solid and represents a genuine improvement. The main things to address before merging:
|
- Add isWasm boolean (via canImport(Dispatch)) to SundialKitCoreTests and SundialKitNetworkTests - Wrap MockPathMonitor (both targets) in #if canImport(Dispatch) - Guard each test function body with #if canImport(Dispatch)/#else, recording an issue in the #else branch as a safety net - Add .disabled(if: isWasm) trait to NetworkMonitorTests suite Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: chore: Update SundialKit.ymlThis PR touches a large surface area (159 files, +2375/-6739 lines) despite the narrow title. Here's a breakdown by area: PR Title / ScopeSuggestion: The title "chore: Update SundialKit.yml" significantly understates the scope. This PR also includes test reorganization, swift-format rule changes, devcontainer updates, and removal of tooling files ( CI Workflow (
|
| Rule | Before | After | Impact |
|---|---|---|---|
AllPublicDeclarationsHaveDocumentation |
true |
false |
Public API may ship without docs |
AlwaysUseLiteralForEmptyCollectionInit |
false |
true |
Enforces [] over Array<T>() — good |
BeginDocumentationCommentWithOneLineSummary |
false |
true |
Enforces one-line summary in doc comments — good |
NoLeadingUnderscores |
true |
false |
Allows _-prefixed public symbols |
UseLetInEveryBoundCaseVariable |
true |
false |
Allows var in pattern bindings |
Concerns:
- Disabling
AllPublicDeclarationsHaveDocumentationreduces documentation enforcement for a public library. This seems at odds with enablingBeginDocumentationCommentWithOneLineSummaryin the same commit. Consider keeping the documentation requirement on — or clarify the rationale. - Disabling
NoLeadingUnderscoresfor a public library is unusual._-prefixed names in public APIs are unconventional in Swift. If this is needed to allow internal naming patterns, consider scoping it more carefully.
Test Reorganization
The split of large test files into focused extension files (e.g., ConnectivityErrorTests+HashableTests.swift, ObserverRegistryTests+NotifyTests.swift) follows Swift conventions and improves navigability. Good practice.
The addition of WASMSupport.swift shims in test targets suggests WASM build compatibility is being systematically addressed — aligns well with the CI matrix additions.
Tooling Cleanup (.claude/, .cursor/, .mcp.json, .taskmaster/)
Large deletion of Task Master commands, agent definitions, Cursor rules, and MCP config. If these were committed inadvertently (e.g., local dev tooling that shouldn't be in the repo), this cleanup is correct. Just confirm none of these are shared team tooling that others depend on.
Minor Nits
- Several Swift source files show only copyright year changes (2025→2026) and doc comment reformatting. If these are purely mechanical, consider whether they add review noise without benefit — they make the diff harder to read.
- The old
build-ubuntumatrix had a duplicateversion: "6.2"entry (one regular, one nightly). The new config correctly deduplicates — good catch.
Summary
The CI restructuring is well-thought-out. The main open questions are around the removal of nightly builds, the AllPublicDeclarationsHaveDocumentation rule change, and whether the PR title should better reflect the scope. Everything else looks solid.
…agable tests WASM32 requires aligned memory access; Data buffers are not guaranteed 4-byte aligned, causing a fatal crash with load(as:). loadUnaligned is safe and correct on all platforms. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: chore: Update SundialKit.ymlSubstantial infrastructure PR covering CI/CD overhaul, linting config tightening, WASM/cross-platform support, and housekeeping. Overall direction is solid. CI/CD Workflow (.github/workflows/SundialKit.yml)Positive changes:
Potential issue - tagged releases get the reduced matrix: The Minor: The matrix only defines Swift Format Config (.swift-format)Two doc-related changes pull in opposite directions:
Result: docs are no longer required, but when present must open with a one-liner. Reasonable stance, but a deliberate policy change worth calling out. Other notable changes:
SwiftLint Config (.swiftlint.yml)
Source Code Changes
Deleted Files (.claude/, .mcp.json)Removing Task Master agent definitions, command guides, and Nits
SummaryThe main functional concern before merge: tagged releases run the reduced matrix, which could miss compatibility regressions at release time. The |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code ReviewThis PR primarily updates the CI/CD workflow ( CI/CD Workflow (
|
Code ReviewOverviewThe PR title says "chore: Update SundialKit.yml" but this touches ~100 files including source code, tests, devcontainers, and extensive tooling cleanup. Worth considering a more descriptive title (e.g., CI/CD Workflow (
|
| Area | Status |
|---|---|
| CI/CD: Windows, Android, dynamic matrix, concurrency | Good direction |
| Action version availability (@v6, @v4) | Verify before merging |
| Nightly Swift build coverage | Confirm intentional removal |
| SundialLogger unification | Cleaner |
| FallbackLogger.init access widening (fileprivate → internal) | Revisit if it should stay private |
| NetworkMonitoring WASM guard | Correct approach |
| ObserverRegistry test deletions | Coverage regression — needs explanation or restoration |
| PR title vs. actual scope | Misleading — consider updating |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
leogdion
changed the title
leogdion
added
component:infrastructure
PR Review: CI Overhaul, Windows/Android Builds, Tooling CleanupOverall this is a solid infrastructure improvement. The concurrency group, matrix factorization into a CI Workflow —
|
| Area | Status | Notes |
|---|---|---|
| Concurrency control / matrix config | ✅ | Clean improvement |
build-macos matrix entries |
Only 1 entry remains — intentional? | |
Duplicate if: key (Ubuntu steps) |
Verify YAML renders correctly | |
| Windows/Android scope | Fine if intent is core-only compilation; add a comment | |
build-windows/android missing !cancelled() |
ℹ️ | Minor inconsistency |
swift-build@v1 unpinned |
ℹ️ | Minor; intentional? |
| Source code changes | ✅ | Correct, minimal |
| Test reorganization | ✅ | Coverage maintained |
| Dev tooling cleanup | ✅ | Appropriate |
| SwiftLint rule addition | ✅ | Good practice |
The two items worth addressing before merge are the build-macos matrix completeness question and the duplicate if: key in the Ubuntu build steps. The Windows/Android scope comment is a nice-to-have for future maintainers.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: CI Matrix Overhaul + Windows/Android BuildsOverall this is a solid, well-scoped infrastructure improvement. The dynamic matrix approach, cross-platform expansion, and source-level WASM guards are all good additions. A few issues are worth addressing before merge. Issues1.
|
| Area | Status |
|---|---|
actions/checkout@v6 existence |
|
Undefined matrix fields in build-macos |
|
swift_project non-standard Codecov field |
|
Inconsistent fail_ci_if_error |
|
| WASM platform guards | ✅ Correct and consistent |
Dynamic CI matrix (configure job) |
✅ Clean design |
| Concurrency cancellation group | ✅ Good addition |
build-windows / build-android additions |
✅ Valuable cross-platform coverage |
SundialLogger unification |
✅ Cleaner than the old split enum approach |
one_declaration_per_file SwiftLint rule |
✅ Good enforcement |
Add is_key_content_base64: true to app_store_connect_api_key calls to resolve OpenSSL::PKey::ECError invalid curve name when using a base64- encoded APP_STORE_CONNECT_API_KEY_KEY secret. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: CI Matrix Overhaul & Tooling CleanupOverall this is a well-structured CI improvement. The dynamic matrix approach and new platform coverage are solid additions. A few items worth discussing: Action Version Pinning — Security ConcernSeveral actions are now using floating major/minor version tags: uses: brightdigit/swift-build@v1 # was @v1.4.0
uses: sersoft-gmbh/swift-coverage-action@v5 # was @v4
uses: codecov/codecov-action@v6 # was @v4
uses: actions/checkout@v6 # was @v4
uses: mise-action@v4 # was @v2Floating tags ( uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v6Android Disk Space CleanupThe aggressive free-disk-space step removes Android tooling, haskell, dotnet, and docker images before the Android build. Make sure the Windows Git Symlink Configurationgit config --global core.symlinks trueThis is set at the job level before checkout. This is the right approach for Windows CI, but note it applies globally to the runner's git config. Ensure the checkout step correctly handles symlinks (e.g.
|
| Area | Verdict |
|---|---|
| Dynamic CI matrix design | ✅ Good approach — reduces feature branch CI cost |
| Windows/Android coverage | ✅ Valuable additions |
Devcontainer VS Code extension (swift-server.swift) |
✅ Correct, better-maintained extension |
| Copyright year 2025→2026 | ✅ Fine |
| Action version pinning | |
.env.example removal |
|
Tooling scaffolding cleanup (.claude/, .cursor/) |
✅ Clean removal of generated scaffolding |
The core CI restructuring is solid. The main ask before merging is addressing the action version pinning for supply-chain security.
…project Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CLAUDE.mdThis file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. Project OverviewSundialKit v2.0.0 is a Swift 6.1+ communications library for Apple platforms with a modern three-layer architecture: Layer 1: Core Protocols (SundialKitCore, SundialKitNetwork, SundialKitConnectivity)
Layer 2: Observation Plugins (Choose your concurrency model)
Key Features:
Build & Test CommandsUsing Make (Recommended)make build # Build the package
make test # Run tests with coverage
make lint # Run linting and formatting (strict mode)
make format # Format code only
make clean # Clean build artifacts
make help # Show all available commandsUsing Swift Directlyswift build
swift test
swift test --enable-code-coverage
swift test --filter <TestName>Formatting & Linting (Mise)SundialKit uses mise to manage development tools:
Install mise (macOS)curl https://mise.run | sh
# or
brew install miseInstall Development Toolsmise install # Installs tools from .mise.tomlRun linting script./Scripts/lint.sh # Normal mode
LINT_MODE=STRICT ./Scripts/lint.sh # Strict mode (CI)
FORMAT_ONLY=1 ./Scripts/lint.sh # Format onlyArchitectureThree-Layer Architecture (v2.0.0)SundialKit v2.0.0 uses a layered architecture separating protocols, wrappers, and observation patterns: Layer 1: Core Protocols & WrappersSundialKitCore (
SundialKitNetwork (
SundialKitConnectivity (
Layer 2: Observation Plugins (Choose One)SundialKitStream (
SundialKitCombine (
Message Encoding/DecodingMessagable Protocol
MessageDecoder
Testing Architecture
Key Design Patterns
Platform Support
Development NotesDevelopment ToolsDevelopment tools (formatter, linter, unused code detector) are managed via mise and defined in Important Type Aliases
Usage ExamplesNetwork Monitoring with SundialKitStream (Async/Await)import SundialKitStream
import SundialKitNetwork
// Create observer (actor-based)
let observer = NetworkObserver(
monitor: NWPathMonitorAdapter(),
ping: nil // or provide a NetworkPing implementation
)
// Start monitoring
observer.start(queue: .global())
// Consume path updates using AsyncStream
Task {
for await status in observer.pathStatusStream {
print("Network status: \(status)")
}
}
// Or get raw path updates
Task {
for await path in observer.pathUpdates() {
print("Path: \(path)")
}
}Network Monitoring with SundialKitCombine (Combine + SwiftUI)import SundialKitCombine
import SundialKitNetwork
import Combine
// Create observer (@MainActor-based)
let observer = NetworkObserver(
monitor: NWPathMonitorAdapter(),
ping: nil
)
// Start monitoring on main queue
observer.start()
// Use @Published properties in SwiftUI
var cancellables = Set<AnyCancellable>()
observer.$pathStatus
.sink { status in
print("Network status: \(status)")
}
.store(in: &cancellables)
observer.$isExpensive
.sink { isExpensive in
print("Is expensive: \(isExpensive)")
}
.store(in: &cancellables)WatchConnectivity with SundialKitStream (Async/Await)import SundialKitStream
import SundialKitConnectivity
// Create observer (actor-based)
let observer = ConnectivityObserver()
// Activate session
try await observer.activate()
// Listen for messages using AsyncStream
Task {
for await result in observer.messageStream() {
switch result.context {
case .replyWith(let handler):
print("Message: \(result.message)")
handler(["response": "acknowledged"])
case .applicationContext:
print("Context update: \(result.message)")
}
}
}
// Send messages
let result = try await observer.sendMessage(["key": "value"])
print("Sent via: \(result.context)")WatchConnectivity with SundialKitCombine (Combine + SwiftUI)import SundialKitCombine
import SundialKitConnectivity
import Combine
// Create observer (@MainActor-based)
let observer = ConnectivityObserver()
// Activate session
try observer.activate()
// Use publishers
var cancellables = Set<AnyCancellable>()
observer.messageReceived
.sink { result in
switch result.context {
case .replyWith(let handler):
print("Message: \(result.message)")
handler(["response": "acknowledged"])
case .applicationContext:
print("Context update: \(result.message)")
}
}
.store(in: &cancellables)
observer.$activationState
.sink { state in
print("State: \(state)")
}
.store(in: &cancellables)
// Send messages asynchronously
Task {
let result = try await observer.sendMessage(["key": "value"])
print("Sent via: \(result.context)")
}Common Pitfalls
Repository Structure & GitHub Workflow IntegrationPackage Structure (v2.0.0)SundialKit v2.0.0 uses a modular Swift Package Manager architecture: Repository Structure:
Package Dependencies: SundialKit uses standard Swift Package Manager dependencies. The Package.swift file references plugin packages as remote dependencies: dependencies: [
.package(url: "https://github.com/brightdigit/SundialKitStream.git", from: "1.0.0-alpha.1"),
.package(url: "https://github.com/brightdigit/SundialKitCombine.git", from: "1.0.0-alpha.1")
]Working with Dependencies: # Update all package dependencies
swift package update
# Resolve dependencies
swift package resolve
# Clean and rebuild
swift package clean
swift buildGitHub Issues & Pull RequestsThis project uses GitHub Issues and Pull Requests with component-based organization:
Component Labels:
|
|
PR Review: CI Matrix Overhaul, Windows/Android/WASM Builds, Tooling Cleanup This is a solid infrastructure PR that significantly expands platform coverage and cleans up tooling scaffolding. Positives
Issues / Concerns 1. Action versions not pinned to SHAs (security) Actions are pinned to mutable tags (actions/checkout@v6, brightdigit/swift-build@v1, jlumbroso/free-disk-space@v1.3.1, etc.). Tags can be overwritten, creating a supply chain risk. GitHub security guidance recommends pinning to full commit SHAs. This is especially important for third-party actions like jlumbroso/free-disk-space and sersoft-gmbh/swift-coverage-action. 2. actions/checkout@v6 — verify this version exists Please confirm actions/checkout@v6 actually exists and resolves to the intended version. At last check, actions/checkout was at v4. If this was a typo for v4, the workflow may fail or silently use an unexpected version. 3. fail-fast: false only on build-ubuntu fail-fast: false was added to the Ubuntu matrix but not to build-macos, build-windows, or build-android. For a cross-platform project a failure on one variant (e.g., Swift 6.1 on Windows) should not cancel all other runs. Consider adding fail-fast: false to the remaining matrix jobs. 4. Missing PR description The PR body is empty. With 18 commits spanning WASM support, Windows/Android CI, lint fixes, Fastlane base64 API key fix, and Swift 6.3 adoption, a short summary would help reviewers and future readers. 5. .env.example deleted without replacement The removed .env.example documented environment variables (App Store Connect API key fields, etc.) needed for CI and Fastlane. New contributors/CI maintainers now have no record of required secrets. Consider migrating this documentation to the README, a docs/ci-secrets.md file, or comments in Fastfile/SundialKit.yml. 6. is_key_content_base64: true in Fastfile — potential local dev breaking change This is correct for CI where secrets are stored as base64, but developers running Fastlane locally with a raw .p8 file path will have their invocations broken. Document the expected secret format. Nits
Summary The core infrastructure work is solid and the WASM/Windows/Android coverage additions are meaningful. Main asks before merging:
The Swift source changes (copyright year, doc comment reformatting, WASM guards) all look correct and appropriate. |
1 participant
No description provided.