Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d30a334
CmuxUpdater: package skeleton + state model redesign (WIP)
azooz2003-bit Jun 1, 2026
fab0380
CLAUDE.md: refine sleep-timing policy (Clock.sleep carve-out, demote …
azooz2003-bit Jun 1, 2026
954e406
CmuxUpdater: port driver + controller off Combine/threads/timing-hacks
azooz2003-bit Jun 1, 2026
d89be8e
CmuxUpdater: group DEBUG-only test scaffolding under TestSupport/
azooz2003-bit Jun 1, 2026
0a70386
CmuxUpdaterUI: extract the update pill/popover/badge views (builds + …
azooz2003-bit Jun 1, 2026
c551e7f
Wire CmuxUpdater + CmuxUpdaterUI into the app; invert AppDelegate; re…
azooz2003-bit Jun 1, 2026
b857ab4
Merge remote-tracking branch 'origin/main' into feat-cmux-updater
azooz2003-bit Jun 2, 2026
37d0bf9
Fix cmux-unit compile + stale doc comment (Codex P1)
azooz2003-bit Jun 2, 2026
eb490c1
Fix CmuxUpdater build under CI's Sparkle 2.8.1 (@preconcurrency import)
azooz2003-bit Jun 2, 2026
da2b5d7
Pin package Sparkle resolution to 2.8.1 (match app/nightly), not 2.9.2
azooz2003-bit Jun 2, 2026
78fe53c
Add error-state UI test coverage for the update pill
azooz2003-bit Jun 2, 2026
2063895
Add downloading/extracting/installing pill rendering UI tests
azooz2003-bit Jun 2, 2026
3fce1c2
Fix re-check while an update is showing (dogfood regression)
azooz2003-bit Jun 2, 2026
2b82a24
Merge remote-tracking branch 'origin/main' into feat-cmux-updater
azooz2003-bit Jun 2, 2026
a467b85
Address Greptile: sanitize error-message fallback + single state-chan…
azooz2003-bit Jun 2, 2026
8b16a5e
Address CodeRabbit: document UpdateSettings.init parameters
azooz2003-bit Jun 2, 2026
afd8fad
Address cubic: recheck-task cancel on all paths, localize error key, …
azooz2003-bit Jun 2, 2026
df968da
Address cubic: inject FileManager into UpdateController for testability
azooz2003-bit Jun 2, 2026
296dbbb
Address CodeRabbit (df968da): use injected hostBundle + localize read…
azooz2003-bit Jun 2, 2026
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ When **extracting** existing code that uses a forbidden primitive into a package
- **Combine for change propagation.** No `@Published`, no `ObservableObject`, no `PassthroughSubject`/`CurrentValueSubject`, no `AnyCancellable` for change observation. Use `@Observable` (Observation framework, Swift 5.9+) for SwiftUI state, or `AsyncStream`/`AsyncSequence` for cross-actor change propagation.
- **Completion-handler APIs.** Authoring a new public API with a `(Result<T, Error>) -> Void` or `(T?, Error?) -> Void` callback is forbidden. Use `async throws -> T`. When wrapping a legacy callback at the boundary, use `withCheckedContinuation`/`withCheckedThrowingContinuation` and keep it confined to that one seam.
- **`DispatchQueue.main.async { ... }`.** Annotate the destination with `@MainActor`. Call sites either `await` the main-isolated function or are themselves `@MainActor`.
- **`Task.sleep` in app/runtime code.** Already forbidden by the cmuxterm-hq top-level rule against sleep-based timing. Allowed in tests only.
- **Sleeping as a synchronization substitute.** `Task.sleep` / `Clock.sleep` (or any sleep) used to *poll* for a condition, to let state "settle" before reading it, or to *race* a callback/animation is forbidden — use a real signal (`AsyncStream`, `NSKeyValueObservation`, a completion, a state change). `DispatchQueue.asyncAfter` is banned outright (it is neither cancellable-by-structure nor testable). A *bounded, cancellable, intended* delay or deadline is allowed under the `Clock.sleep` carve-out below.

**Required shape:**

Expand All @@ -383,7 +383,8 @@ These low-level primitives have no async-native replacement. They must be hidden

- `DispatchSource.makeFileSystemObjectSource` for file watching (no Foundation async equivalent).
- `DispatchSource.makeReadSource`/`makeWriteSource` for low-level socket I/O.
- `DispatchSource.makeTimerSource` (one-shot) for a genuine deadline/timeout, e.g. a subprocess time limit. A real deadline needs a timer, and the async-native timers are disallowed here (`Task.sleep` is banned in runtime code; `DispatchQueue.asyncAfter` is a forbidden timing hack). Hide the timer behind the type and cancel it on the non-timeout path. This is for true deadlines only, never to poll or to fake a sleep.
- **A bounded, cancellable `Clock.sleep` (preferred) or `Task.sleep` for a genuine delay/deadline** that is itself the intended behavior — a minimum display duration, an auto-dismiss, a check timeout. Drive it from an *injected* `Clock` (or a duration) so tests advance virtual time with no real waiting, and wire the sleeping `Task`'s cancellation to the relevant lifecycle so a state transition cancels the pending delay (store the `Task`, cancel it on transition; or use `withTaskCancellationHandler`). For true delays/deadlines only, never to poll, settle, or race — those still require a real signal. One-line justification on the call site.
- `DispatchSource.makeTimerSource` (one-shot) **only when a genuine deadline must fire outside any async context** — a non-`async` type with no `Task` to host the sleep. Prefer the `Clock.sleep` carve-out above whenever the code is already on an actor or in async code (it is cancellation-integrated and testable; a raw `DispatchSource` timer is not, and has suspend/resume/cancel footguns). Hide the timer behind the type, cancel it on the non-timeout path, and never use it to poll or fake a sleep.
- A **lock for a synchronous compare-and-set called from non-async callbacks**, where promoting to an `actor` would only add `Task`/`await` hops and reentrancy. The canonical case is a one-shot resume guard: several synchronous `Process`/`DispatchSource` callbacks race to resume one `withCheckedContinuation` exactly once. `OSAllocatedUnfairLock(initialState:)` guarding a `Bool` (claimed once, checked synchronously in each callback) is correct, deterministic, and lets the callback resume the continuation inline. This carve-out is for short, non-blocking critical sections over a tiny flag/counter — not for guarding ongoing domain state (that is still an `actor`). Keep it private to the type, with a one-line justification.
- `NSKeyValueObservation` token (the closure-based API) when wrapping a Foundation/AppKit type that exposes change only via KVO.

Expand All @@ -405,7 +406,7 @@ Without a justification comment, the diff is rejected. `@unchecked Sendable` on

- Applies to: every new file in `Packages/`, every new file in the app target, every meaningful rewrite of an existing Swift file.
- Existing app target code may continue to use the old primitives until rewritten. Do not retrofit blindly.
- Code review checklist (Codex, CodeRabbit, Greptile, and human reviewers): reject diffs that introduce `@Published`/`ObservableObject`/`DispatchQueue.main.async`/`addObserver(_:forKeyPath:...)`/`Task.sleep` outside tests in new code. Reject a lock (`NSLock`/`OSAllocatedUnfairLock`/etc.) or `@unchecked Sendable`/`nonisolated(unsafe)` unless it falls under a documented carve-out *and* carries a one-line justification — and reject a single-method `actor` that exists only to guard a flag (use the lock carve-out instead).
- Code review checklist (Codex, CodeRabbit, Greptile, and human reviewers): reject diffs that introduce `@Published`/`ObservableObject`/`DispatchQueue.main.async`/`addObserver(_:forKeyPath:...)`/`DispatchQueue.asyncAfter` in new code, or `Task.sleep`/`Clock.sleep` used to poll, settle, or race rather than as a bounded, cancellable, injected-clock delay with justification. Reject a lock (`NSLock`/`OSAllocatedUnfairLock`/etc.) or `@unchecked Sendable`/`nonisolated(unsafe)` unless it falls under a documented carve-out *and* carries a one-line justification — and reject a single-method `actor` that exists only to guard a flag (use the lock carve-out instead).

## Test quality policy

Expand Down
15 changes: 15 additions & 0 deletions Packages/CmuxUpdater/Package.resolved

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 41 additions & 0 deletions Packages/CmuxUpdater/Package.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// swift-tools-version: 6.0

import PackageDescription

let package = Package(
name: "CmuxUpdater",
platforms: [
.macOS(.v14),
],
products: [
.library(
name: "CmuxUpdater",
targets: ["CmuxUpdater"]
),
],
dependencies: [
.package(url: "https://github.com/sparkle-project/Sparkle", from: "2.5.1"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Sparkle version constraint from: "2.5.1" contradicts PR description's stated intent to pin Sparkle to 2.8.1. The loose range allows SPM to resolve to versions below 2.8.1 that lack the @preconcurrency compatibility fixes the PR claims were added, which could cause build failures on fresh checkouts or CI environments without a Package.resolved.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxUpdater/Package.swift, line 17:

<comment>Sparkle version constraint `from: "2.5.1"` contradicts PR description's stated intent to pin Sparkle to 2.8.1. The loose range allows SPM to resolve to versions below 2.8.1 that lack the `@preconcurrency` compatibility fixes the PR claims were added, which could cause build failures on fresh checkouts or CI environments without a Package.resolved.</comment>

<file context>
@@ -0,0 +1,41 @@
+        ),
+    ],
+    dependencies: [
+        .package(url: "https://github.com/sparkle-project/Sparkle", from: "2.5.1"),
+    ],
+    targets: [
</file context>

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mitigated. The @preconcurrency Sparkle import makes the code compile against any 2.x (2.8.1 or 2.9.x), so a fresh-checkout resolution difference won't break the build; the committed Package.resolved pins 2.8.1 to match nightly; and keeping the requirement at 'from: 2.5.1' matches the app target's own Sparkle requirement so the combined Xcode resolution unifies cleanly without this leaf package imposing a tighter cap on the whole app.

— Claude Code

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mitigated. The @preconcurrency Sparkle import lets the code compile against any 2.x (2.8.1 or 2.9.x), so a fresh-checkout resolution difference won't break the build; the committed Package.resolved pins 2.8.1 (matching nightly); and 'from: 2.5.1' matches the app target's own Sparkle requirement so the combined Xcode resolution unifies without this leaf package capping the whole app.

— Claude Code

],
targets: [
.target(
name: "CmuxUpdater",
dependencies: [
.product(name: "Sparkle", package: "Sparkle"),
],
swiftSettings: [
.swiftLanguageMode(.v6),
.enableUpcomingFeature("ExistentialAny"),
.enableUpcomingFeature("InternalImportsByDefault"),
]
),
.testTarget(
name: "CmuxUpdaterTests",
dependencies: ["CmuxUpdater"],
swiftSettings: [
.swiftLanguageMode(.v6),
.enableUpcomingFeature("ExistentialAny"),
.enableUpcomingFeature("InternalImportsByDefault"),
]
),
]
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
#if DEBUG
import Foundation
@preconcurrency import Sparkle

/// Drives the ``UpdateStateModel`` into specific states for UI tests, and can run a mock feed
/// check against an in-process appcast (``UpdateTestURLProtocol``).
///
/// DEBUG-only test scaffolding, gated on `CMUX_UI_TEST_*` environment variables. Constructed by
/// the host with the live model and logger.
@MainActor
public struct UpdateTestSupport {
let model: UpdateStateModel
let log: any UpdateLogging

/// Creates the test-support helper bound to a model and logger.
public init(model: UpdateStateModel, log: any UpdateLogging) {
self.model = model
self.log = log
}

/// Applies an initial detected-update banner and/or a forced state from the environment.
public func applyIfNeeded() {
let env = ProcessInfo.processInfo.environment
guard env["CMUX_UI_TEST_MODE"] == "1" else { return }

if let detectedVersion = env["CMUX_UI_TEST_DETECTED_UPDATE_VERSION"],
!detectedVersion.isEmpty {
if let item = Self.makeAppcastItem(displayVersion: detectedVersion) {
model.recordDetectedUpdate(item)
} else {
model.debugSetDetectedVersion(UpdateStateModel.normalizedDetectedUpdateVersion(from: detectedVersion))
}
}

guard let state = env["CMUX_UI_TEST_UPDATE_STATE"] else { return }
switch state {
case "available":
let version = env["CMUX_UI_TEST_UPDATE_VERSION"] ?? "9.9.9"
transition(to: .updateAvailable(.init(
appcastItem: Self.makeAppcastItem(displayVersion: version) ?? SUAppcastItem.empty(),
reply: { _ in }
)))
case "notFound":
transition(to: .notFound(.init(acknowledgement: {})))
case "downloading":
transition(to: .downloading(.init(cancel: {}, expectedLength: 100, progress: 50)))
case "extracting":
transition(to: .extracting(.init(progress: 0.5)))
case "installing":
transition(to: .installing(.init(isAutoUpdate: false, retryTerminatingApplication: {}, dismiss: {})))
case "error":
let message = env["CMUX_UI_TEST_UPDATE_ERROR_MESSAGE"] ?? "Test update error"
let error = NSError(domain: "cmux.update.uitest", code: 1, userInfo: [NSLocalizedDescriptionKey: message])
transition(to: .error(.init(
error: error,
retry: {},
dismiss: {},
technicalDetails: "ui-test error",
feedURLString: env["CMUX_UI_TEST_FEED_URL"]
)))
default:
break
}
}

/// Runs a mock feed check against `CMUX_UI_TEST_FEED_URL` if requested. Returns whether it ran.
public func performMockFeedCheckIfNeeded() -> Bool {
let env = ProcessInfo.processInfo.environment
guard env["CMUX_UI_TEST_TRIGGER_UPDATE_CHECK"] == "1" else { return false }
guard let feedURLString = env["CMUX_UI_TEST_FEED_URL"],
let feedURL = URL(string: feedURLString) else { return false }

log.append("ui test mock feed check: \(feedURLString)")
UpdateTestURLProtocol.registerIfNeeded()
model.applyDriverState(.checking(.init(cancel: {})))

let model = self.model
let task = URLSession.shared.dataTask(with: feedURL) { data, _, _ in
let xml = data.flatMap { String(data: $0, encoding: .utf8) } ?? ""
let version = env["CMUX_UI_TEST_UPDATE_VERSION"] ?? "9.9.9"
let hasItem = xml.contains("<item>")
let delayMilliseconds = Int(env["CMUX_UI_TEST_MOCK_FEED_DELAY_MS"] ?? "") ?? 0
Task { @MainActor in
if delayMilliseconds > 0 {
try? await Task.sleep(for: .milliseconds(delayMilliseconds))
}
if hasItem {
let appcastItem = Self.makeAppcastItem(displayVersion: version) ?? SUAppcastItem.empty()
model.applyDriverState(.updateAvailable(.init(appcastItem: appcastItem, reply: { _ in })))
} else {
model.applyDriverState(.notFound(.init(acknowledgement: {})))
}
}
}
task.resume()
return true
}

private func transition(to state: UpdateState) {
model.applyDriverState(.checking(.init(cancel: {})))
let model = self.model
Task { @MainActor in
try? await Task.sleep(for: .milliseconds(200))
model.applyDriverState(state)
}
}
Comment on lines +99 to +106

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 transition(to:) uses a bare Task.sleep(for: .milliseconds(200)) to create a brief "checking" visual before switching to the target state. This is a settlement/race sleep rather than a bounded, injected-clock delay driven by a real signal, which the updated CLAUDE.md and review rules now explicitly reject even in DEBUG scaffolding. Since UpdateTestSupport already receives the model and log as seams, the clock can be threaded in the same way and used here (clock.sleep rather than Task.sleep) so tests can advance virtual time with no real waiting. The same Task.sleep on line 85 inside performMockFeedCheckIfNeeded is in the same category.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional, and allowed by the same CLAUDE.md rule: 'Deterministic sleeps remain fine in tests and explicit test-only scaffolding.' UpdateTestSupport is #if DEBUG, gated on CMUX_UI_TEST_MODE, and exists only to stage UI-test states - the bounded/injected-clock requirement targets shipping runtime code. The two Task.sleep calls are deterministic test-only staging delays, so I'm leaving them rather than threading a clock seam into DEBUG scaffolding.

— Claude Code


private static func makeAppcastItem(displayVersion: String) -> SUAppcastItem? {
let enclosure: [String: Any] = [
"url": "https://example.com/cmux.zip",
"length": "1024",
"sparkle:version": displayVersion,
"sparkle:shortVersionString": displayVersion,
]
let dict: [String: Any] = [
"title": "cmux \(displayVersion)",
"pubDate": "Wed, 25 Mar 2026 12:00:00 +0000",
"enclosure": enclosure,
]
return SUAppcastItem(dictionary: dict)
}
}
#endif
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
#if DEBUG
import Foundation

/// A `URLProtocol` that serves a synthetic Sparkle appcast (and update archive) for UI tests,
/// so an end-to-end update check can run against an in-process feed.
///
/// DEBUG-only test scaffolding. The feed contents are driven by `CMUX_UI_TEST_*` environment
/// variables.
final class UpdateTestURLProtocol: URLProtocol {
static let host = "cmux.test"
static let appcastPath = "/appcast.xml"
static let updatePath = "/cmux-test.zip"

private static var isRegistered = false
private static let registrationLock = NSLock()
@MainActor private static var isRegistered = false

/// Registers the protocol once. Main-actor isolated so the one-time flag needs no lock.
@MainActor
static func registerIfNeeded() {
registrationLock.lock()
defer { registrationLock.unlock() }
guard !isRegistered else { return }
URLProtocol.registerClass(UpdateTestURLProtocol.self)
isRegistered = true
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// The host-application actions the updater needs but cannot perform itself.
///
/// This is the dependency-inversion seam between the `CmuxUpdater` package and the app: the
/// package calls up through this protocol instead of reaching `AppDelegate`/`TerminalController`
/// directly. The app's delegate conforms and is injected into ``UpdateController`` (which holds
/// it `weak`).
@MainActor
public protocol UpdateActionDelegate: AnyObject {
/// The user asked to retry after an update error. The host should re-initiate a check
/// through its normal entry point.
func updaterRequestsRetryCheckForUpdates()

/// Sparkle is about to relaunch the app to finish installing. The host should persist
/// session state, stop its terminal/runtime, and invalidate restorable state so the
/// relaunched instance starts cleanly.
func updaterWillRelaunchApplication()
}
23 changes: 23 additions & 0 deletions Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateClock.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/// An injectable clock for the updater's bounded, cancellable UI delays (minimum check
/// display, "no updates" auto-dismiss, ready-timeout).
///
/// Production uses ``SystemUpdateClock`` (a real `Task.sleep`); tests inject a fake that
/// returns immediately or coordinates timing, so delay-driven behavior is verified with no
/// real waiting. The delays this clock drives are *intended* delays, never a poll or a
/// "let state settle" hack.
public protocol UpdateClock: Sendable {
/// Suspends for `duration`, throwing `CancellationError` if the surrounding task is
/// cancelled first.
func sleep(for duration: Duration) async throws
}

/// The production ``UpdateClock``, backed by `Task.sleep`.
public struct SystemUpdateClock: UpdateClock {
/// Creates a system clock.
public init() {}

public func sleep(for duration: Duration) async throws {
// Bounded, cancellable, intended delay behind the UpdateClock seam (policy carve-out).
try await Task.sleep(for: duration)
}
}
Loading
Loading