V2.0.0 alpha.3

[leogdion]

No description provided.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: dbaac698-3345-4bd0-a673-bb281aa20ea7

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch v2.0.0-alpha.3

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

PR Review: V2.0.0 alpha.3

This PR combines CI/CD infrastructure updates, a Mint→mise tooling migration, and several WatchConnectivity bug fixes. The infrastructure changes look well-considered; the WatchConnectivity fixes address real issues but have a correctness concern worth addressing before merging.

---

Thread Safety: replied bool in reply-handler guard

File: Sources/SundialKitConnectivity/WatchConnectivitySession+WCSessionDelegate.swift

Both the dictionary and data delegate paths introduce a var replied = false guard shared between two closures:

var replied = false
let safeReply: ([String: Any]) -> Void = { response in
    guard !replied else { return }
    replied = true
    replyHandler(response)
}
delegate?.session(self, didReceiveMessage: sendableMessage, replyHandler: handler)
if !replied {
    replyHandler([:])
}

The read/write of replied is not synchronized. If the delegate stores the handler and calls it asynchronously (on a different queue) while the auto-ack fires concurrently, this is a data race — undefined behavior under Swift's memory model. The synchronous path works in practice today, but the contract isn't enforced and Swift's strict concurrency checking will flag it once the closures acquire @Sendable.

Suggested fix — synchronize with a lock:

let lock = NSLock()
var replied = false
let safeReply: ([String: Any]) -> Void = { response in
    lock.lock(); defer { lock.unlock() }
    guard !replied else { return }
    replied = true
    replyHandler(response)
}
// ...
lock.lock(); let needsAck = !replied; lock.unlock()
if needsAck { replyHandler([:]) }

---

unsafeBitCast on closure types

File: Sources/SundialKitConnectivity/WatchConnectivitySession+WCSessionDelegate.swift

The new safeReply closures are cast with unsafeBitCast:

let handler = unsafeBitCast(safeReply, to: ConnectivityHandler.self)
// ConnectivityHandler = @Sendable ([String: any Sendable]) -> Void
// safeReply           =           ([String: Any])          -> Void

This pre-existed for the original replyHandler, but the new code extends it to the wrapped closures. Casting between ([String: Any]) -> Void and @Sendable ([String: any Sendable]) -> Void relies on Any and any Sendable having identical runtime representations — true today (Sendable is a marker protocol) but not ABI-guaranteed. A wrapper closure removes the UB:

let handler: ConnectivityHandler = { message in safeReply(message as [String: Any]) }

---

Shell injection risk in setup-sundialkit action

File: .github/actions/setup-sundialkit/action.yml

run: |
  BRANCH='${{ inputs.branch }}'

${{ inputs.branch }} is expanded before the shell sees the script. A branch name containing a single quote breaks the bash string. Prefer the env: block to bypass shell interpretation:

env:
  BRANCH: ${{ inputs.branch }}
run: |
  REF=$(git ls-remote https://github.com/brightdigit/SundialKit.git "$BRANCH" | head -n1 | cut -f1)

The Windows step has the same pattern.

---

Positives

Delegate registration moved to activate() — correct and well-motivated. Moving session.delegate = self out of init prevents throwaway instances (e.g. from SwiftUI @State re-evaluation) from hijacking delivery. The inline comment and updated docstring explain the invariant clearly.

Auto-acknowledgment guard — addressing WCErrorDomain 7012 timeouts when no consumer replies is the right fix; just needs the synchronization above to be complete.

Android API 36 removal — the comment clearly documents the constraint and the re-add path.

Tooling updates — CodeQL v3→v4, actions/checkout@v4→@v6, Periphery 3.2→3.7.4, SwiftLint 0.61→0.62.2 are all appropriate maintenance.

Mint→mise migration — generate-protos.sh is simpler and consistent with the rest of the project toolchain.

---

Minor notes

• Secret rename (APP_STORE_CONNECT_API_KEY_KEY → APP_STORE_CONNECT_API_KEY_KEY_BASE64): breaking change — the GitHub repo secret needs renaming before these workflows pass. Worth calling out in the PR description.
• CLAUDE.md still references realm/SwiftLint@0.61.0; this PR bumps it to 0.62.2. A one-line update would keep them in sync.
• atleast-beta.6 branch in the CI trigger list is fine for the alpha cycle; just remember to remove it before cutting a stable release.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.47%. Comparing base (c74d7f2) to head (5483614).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #85      +/-   ##
==========================================
- Coverage   65.66%   65.47%   -0.19%     
==========================================
  Files          32       32              
  Lines         533      533              
==========================================
- Hits          350      349       -1     
- Misses        183      184       +1     

Flag	Coverage Δ
spm	66.85% <ø> (-0.20%)	⬇️
swift-6.1	65.91% <ø> (ø)
swift-6.2	65.91% <ø> (ø)
swift-6.3	65.91% <ø> (ø)
ubuntu	65.91% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Code Review: V2.0.0 alpha.3

Summary: This PR covers CI/tooling infrastructure upgrades (Xcode 26.5, CodeQL v4, mise tool updates) alongside three behavioral fixes in SundialKitConnectivity around delegate registration and WCSession reply handler safety.

---

✅ Good Changes

Delegate registration moved to activate() (WatchConnectivitySession.swift, WatchConnectivitySession+ConnectivitySession.swift)
Deferring session.delegate = self from init to activate() is the right design. It eliminates the global side effect of construction and prevents throwaway instances (e.g. from SwiftUI @State re-evaluation) from hijacking the active session's delegate. The comment explains this clearly and the approach is sound.

One-shot reply handler guard (intent is correct)
Preventing replyHandler from firing twice is the right fix — WCSession's behavior when replyHandler is invoked more than once is undefined and causes WCErrorDomain 7012 timeouts.

CI matrix expansion — testing against Xcode 26.1, 26.5, and 16.4 simultaneously is good coverage. Documenting why Android API 36 was dropped is appreciated.

Tooling migrations — swiftlint → aqua provider and periphery 3.7.4 are reasonable upgrades.

---

🐛 Critical Bug: Auto-acknowledge bypasses the guard

In both didReceiveMessage and didReceiveMessageData, the auto-acknowledge at the end calls the original replyHandler directly rather than going through safeReply:

// WatchConnectivitySession+WCSessionDelegate.swift (message path ~line 1237)
var replied = false
let safeReply: ([String: Any]) -> Void = { response in
    guard !replied else { return }
    replied = true
    replyHandler(response)   // ← sets replied = true
}
let handler = unsafeBitCast(safeReply, to: ConnectivityHandler.self)
delegate?.session(self, didReceiveMessage: sendableMessage, replyHandler: handler)
if !replied {
    replyHandler([:])        // ← bypasses safeReply, does NOT set replied = true
}

When the delegate stores handler and fires it asynchronously (which SundialKitStream.ConnectivityObserver does — it's an actor), the race is:

1. delegate?.session(...) returns immediately (replied = false)
2. if !replied → replyHandler([:]) fires — first invocation (does not set replied)
3. Actor later calls handler → safeReply → replied still false → replyHandler(response) fires — second invocation

The fix: call safeReply in the auto-acknowledge, not replyHandler directly:

// Correct fix — same one-shot guarantee applies to auto-ack:
if !replied {
    safeReply([:])   // or safeReply(Data()) for the data path
}

This applies identically to the binary didReceiveMessageData path.

---

⚠️ Thread-Safety of replied flag

The replied variable is a plain var Bool shared between the delegate closure and the auto-ack check. If replyHandler is ever called concurrently (e.g. from a race between two WCSession callbacks or from within an actor switching threads), replied has no synchronization.

In practice WCSession serializes delivery per-session, so this is low risk, but Swift 6 strict concurrency enforcement means this could also surface as a compiler warning. Consider using nonisolated(unsafe) var replied or replacing the flag with a CheckedContinuation/os_unfair_lock if stricter safety is needed.

---

⚠️ Missing tests for behavioral changes

The following changes alter observable behavior but have no accompanying test additions:

• didReceiveMessage now auto-acknowledges (new behavior — previously no auto-ack on the message dictionary path)
• didReceiveMessageData now conditionally auto-acknowledges (changed from unconditional)
• Delegate is set in activate() not init() (timing change)

MockSession and the existing test infrastructure could cover the reply-handler invocation count and delegate registration timing.

---

⚠️ GitHub Actions injection risk in composite action

.github/actions/setup-sundialkit/action.yml passes inputs.branch via single-quoted '${{ inputs.branch }}'. GitHub expression substitution happens before the shell parses the string, so a branch name containing ' breaks the quoting. Prefer environment variable indirection:

env:
  BRANCH: ${{ inputs.branch }}
run: |
  REF=$(git ls-remote https://github.com/brightdigit/SundialKit.git "$BRANCH" | head -n1 | cut -f1)

The Windows step has the same issue with the unquoted $branch in git ls-remote ... $branch.

---

📋 Operational Note: Secret rename requires coordinated update

APP_STORE_CONNECT_API_KEY_KEY → APP_STORE_CONNECT_API_KEY_KEY_BASE64 is changed in the workflow YAML and Fastfile, but the actual GitHub Actions repository secret must be renamed/recreated before the sundial-demo.yml workflow runs successfully post-merge. Ensure this is done before merging or the release pipeline will fail.

---

🔍 Minor

• The atleast-beta.6 branch in the CI trigger looks like a temporary dev branch. Fine for alpha work, but worth removing before the v2.0.0 final release PR.
• unsafeBitCast on function types is pre-existing but worth revisiting in a follow-up — the cast from ([String: Any]) -> Void to @Sendable (ConnectivityMessage) -> Void makes assumptions about ABI layout that aren't guaranteed. Replacing ConnectivityHandler with a proper type or using a wrapper closure would be safer.

---

Bottom line: The infrastructure updates are straightforward and well-motivated. The delegate-registration change is the right design. The reply-handler guard logic has a correctness bug (auto-ack bypasses safeReply, potentially causing double invocation with async delegates) that should be fixed before merging. Adding tests for the new behavioral guarantees would also strengthen confidence in the fix.