From 6d2262624f3eeb29eaed7d296b4e2aebb7cff456 Mon Sep 17 00:00:00 2001 From: Adron Hall Date: Wed, 16 Sep 2026 13:29:35 -0700 Subject: [PATCH 1/2] feat(sync-agent): move the agent's configuration into per-machine app settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Document Sync Agent kept its whole configuration in UserDefaults, which meant it did not survive a clean reinstall, a second Mac had to be set up from scratch with nothing to say that was expected, and the Applications pane could list a machine while telling you nothing about what that machine was doing. /help/app-settings describes per-machine storage as being for exactly this — a value meaningful on one machine only — and a sync-folder path is the textbook case. The state that moves was enumerated from PreferencesManager rather than guessed: enabled, poll interval, launch-at-login, the four notification toggles, and the sync folder. Two things the issue names are deliberately left where they are. The ledger's lastSyncAt is the delta cursor and is only meaningful beside the per-document entries in the same file, so what moves is a *reported* last-sync timestamp that nothing reads back. And there is no conflict policy to move: ConflictResolver is a fixed remote-wins table with no user-facing setting, so storing one would create a setting nothing consumes. Three things needed care. Security-scoped bookmarks are machine-local, and a document can still cross machines through copy-to-shared or a new machine seeding from the main workstation. The stored shape is therefore the bookmark plus a display path plus the device id that created it, and a reference stamped with any other machine is dropped on read — the Mac reports itself unconfigured and prompts for a folder, rather than resolving a bookmark that at best points nowhere and at worst points at an unrelated folder it would then write documents into. An unstamped bookmark counts as foreign too: "cannot prove it is ours" has to read as "not ours". Migration is one-way and gated on a confirmed write. The launch decision is a pure four-case function because it guards the one thing here that can lose settings for good, and the case that matters is the distinction between "nothing stored" and "could not ask" — collapsing those two would let an offline launch overwrite a newer server document with stale local values. Writes are compare-and-set, and the agent writes from the background where losing the race is likeliest. A 409 re-bases and retries rather than surfacing, bounded at three attempts so a lost race cannot become a request storm. The agent's own API client keeps the typed 409 body, unlike the main app's, so the retry re-bases from the winning document at no extra request. The overlay preserves every key outside the documentSync. namespace, because the PUT replaces the document wholesale and that document is shared with the main app. Nothing is stored in shared settings: shared seeds a new machine on first sign-in, which is precisely the failure this exists to prevent. The two processes have to agree on one device id or one Mac gets two registry rows and its settings split in half, and they cannot share UserDefaults across bundle identifiers. The id now travels through the shared Keychain group that already carries the bearer token, rather than adding an app-group entitlement that would mean re-provisioning both signed bundles. The app mints and publishes at launch; the agent only ever reads, and falls back to local settings when nothing has been published yet. Refs #104 Co-Authored-By: Claude Opus 5 --- App/Composition/AppEnvironment.swift | 6 + App/Composition/DeviceIdentity.swift | 54 ++- App/Features/Settings/DevicesView.swift | 28 ++ AppTests/DeviceIdentityTests.swift | 105 ++++++ .../Models/DocumentSyncStatus.swift | 75 +++++ .../DocumentSyncStatusTests.swift | 128 +++++++ .../InterlinedKit/Auth/DeviceIDStore.swift | 146 ++++++++ .../API/AppSettingsModels.swift | 169 ++++++++++ .../API/SyncAPIClient.swift | 95 +++++- .../App/AppDelegate.swift | 46 ++- .../Auth/SharedDeviceIdentity.swift | 97 ++++++ .../Configuration.swift | 33 ++ .../Storage/PreferencesManager.swift | 210 +++++++++++- .../Storage/RemoteConfigurationStore.swift | 223 +++++++++++++ .../Storage/SyncAgentConfiguration.swift | 223 +++++++++++++ .../FakeDeviceSettingsAPI.swift | 81 +++++ .../PreferencesManagerTests.swift | 237 +++++++++++++ .../RemoteConfigurationStoreTests.swift | 312 ++++++++++++++++++ .../SyncAPIClientTests.swift | 144 ++++++++ .../SyncAgentConfigurationTests.swift | 140 ++++++++ 20 files changed, 2532 insertions(+), 20 deletions(-) create mode 100644 AppTests/DeviceIdentityTests.swift create mode 100644 Packages/InterlinedDomain/Sources/InterlinedDomain/Models/DocumentSyncStatus.swift create mode 100644 Packages/InterlinedDomain/Tests/InterlinedDomainTests/DocumentSyncStatusTests.swift create mode 100644 Packages/InterlinedKit/Sources/InterlinedKit/Auth/DeviceIDStore.swift create mode 100644 SyncAgent/Sources/InterlinedListSyncCore/API/AppSettingsModels.swift create mode 100644 SyncAgent/Sources/InterlinedListSyncCore/Auth/SharedDeviceIdentity.swift create mode 100644 SyncAgent/Sources/InterlinedListSyncCore/Storage/RemoteConfigurationStore.swift create mode 100644 SyncAgent/Sources/InterlinedListSyncCore/Storage/SyncAgentConfiguration.swift create mode 100644 SyncAgent/Tests/InterlinedListSyncCoreTests/FakeDeviceSettingsAPI.swift create mode 100644 SyncAgent/Tests/InterlinedListSyncCoreTests/RemoteConfigurationStoreTests.swift create mode 100644 SyncAgent/Tests/InterlinedListSyncCoreTests/SyncAgentConfigurationTests.swift diff --git a/App/Composition/AppEnvironment.swift b/App/Composition/AppEnvironment.swift index 66e87af..adc8e2b 100644 --- a/App/Composition/AppEnvironment.swift +++ b/App/Composition/AppEnvironment.swift @@ -627,6 +627,12 @@ final class AppEnvironment: ObservableObject { // who opts in *after* a crash still has a report to send. let crashReportService = CrashReportService() Self.installCrashHandler(service: crashReportService) + // Publish this machine's device id to the shared Keychain group at + // launch (GitHub issue #104), not when Settings ▸ Applications is first + // opened. The document-sync agent needs the id to address its + // per-machine settings document, and most users never visit that pane — + // leaving the agent stranded on local `UserDefaults` forever. + DeviceIdentity.current() return AppEnvironment( messages: messages, lists: lists, diff --git a/App/Composition/DeviceIdentity.swift b/App/Composition/DeviceIdentity.swift index e58dc24..4aeaf8c 100644 --- a/App/Composition/DeviceIdentity.swift +++ b/App/Composition/DeviceIdentity.swift @@ -9,21 +9,63 @@ // It is an opaque UUID minted once per machine — no hardware identifier, so it // carries nothing personally identifying and resets cleanly if the user wipes // preferences. +// +// GitHub issue #104 adds a second home for the same value: the shared Keychain +// group, so the bundled document-sync agent — a separate process with its own +// defaults domain — can address the *same* per-machine settings document this +// app does. `UserDefaults` remains the primary source of truth so machines +// already registered under an id keep it. import Foundation +import InterlinedKit enum DeviceIdentity { private static let defaultsKey = "com.interlinedlist.deviceId" - /// The stable id for this machine, minting and persisting one on first use. - static func current(defaults: UserDefaults = .standard) -> String { + /// The shared-Keychain channel the sync agent reads. Service and group must + /// match the agent's `SyncConfiguration.deviceIDService` / + /// `sharedAccessGroup` — two independent codebases agreeing on one contract. + static let sharedStore: any DeviceIDStoring = KeychainDeviceIDStore( + service: "com.interlinedlist.macos.device-id", + accessGroup: "BJA9558E4B.com.interlinedlist.shared" + ) + + /// The stable id for this machine, minting and persisting one on first use, + /// and publishing it where the sync agent can find it. + /// + /// Resolution order matters: + /// + /// 1. **`UserDefaults`** — a machine already registered with the server + /// keeps the id its registry row is keyed on. Preferring anything else + /// would orphan that row and its per-machine settings. + /// 2. **the shared Keychain** — the app's preferences were wiped or + /// reinstalled while the agent stayed put. Adopting the published id + /// reunites the app with the machine's existing registry row instead of + /// minting a duplicate. + /// 3. **mint** — genuinely new machine. + @discardableResult + static func current( + defaults: UserDefaults = .standard, + sharedStore: any DeviceIDStoring = DeviceIdentity.sharedStore + ) -> String { + let resolved: String if let existing = defaults.string(forKey: defaultsKey), !existing.isEmpty { - return existing + resolved = existing + } else if let published = sharedStore.read(), !published.isEmpty { + resolved = published + defaults.set(published, forKey: defaultsKey) + } else { + resolved = UUID().uuidString + defaults.set(resolved, forKey: defaultsKey) + } + + // Publish only on a mismatch. The Keychain write is cheap but not free, + // and `current()` is called on every Applications-pane load. + if sharedStore.read() != resolved { + sharedStore.write(resolved) } - let minted = UUID().uuidString - defaults.set(minted, forKey: defaultsKey) - return minted + return resolved } /// A human-friendly default name for this machine, used when registering. diff --git a/App/Features/Settings/DevicesView.swift b/App/Features/Settings/DevicesView.swift index 2bc2cb5..cfb6581 100644 --- a/App/Features/Settings/DevicesView.swift +++ b/App/Features/Settings/DevicesView.swift @@ -310,6 +310,34 @@ struct DeviceSettingsInspector: View { Section("Settings saved for this machine") { SettingsDocumentSummary(document: document) } + // What the machine is actually doing, not just that it has a + // document (GitHub issue #104). Only the document-sync agent + // currently writes anything a human can be told about, so + // this is the one interpreted section on an otherwise + // deliberately opaque sheet. + if let sync = DocumentSyncStatus( + bag: document.bag, + deviceID: inspection.device.id + ) { + Section("Document Sync") { + LabeledContent("Sync folder") { + // The path itself is not shown — see the Keys + // section below on why values stay hidden. + Text(sync.isConfigured ? "Configured on this machine" : "Not configured here") + .foregroundStyle(sync.isConfigured ? .primary : .secondary) + } + LabeledContent("Syncing") { + Text(sync.isEnabled ? "On" : "Paused") + } + LabeledContent("Last synced") { + if let lastSyncAt = sync.lastSyncAt { + Text(lastSyncAt.formatted(.relative(presentation: .named))) + } else { + Text("Never").foregroundStyle(.secondary) + } + } + } + } Section("Keys") { if document.bag.isEmpty { Text("None").foregroundStyle(.secondary) diff --git a/AppTests/DeviceIdentityTests.swift b/AppTests/DeviceIdentityTests.swift new file mode 100644 index 0000000..ce14246 --- /dev/null +++ b/AppTests/DeviceIdentityTests.swift @@ -0,0 +1,105 @@ +// DeviceIdentityTests +// +// This machine's app-settings device id, and the shared-Keychain channel that +// lets the bundled document-sync agent address the *same* per-machine settings +// document the app does (GitHub issue #104). +// +// Two ids for one Mac would mean two rows in the device registry and a +// configuration split across two documents, so the resolution order is the +// behaviour under test — not an implementation detail. + +import XCTest +import InterlinedKit +@testable import InterlinedList + +final class DeviceIdentityTests: XCTestCase { + + private func freshDefaults() -> UserDefaults { + UserDefaults(suiteName: "iltest-device-\(UUID().uuidString)")! + } + + // MARK: - Happy path + + func test_givenAMachineAlreadyRegistered_whenResolving_thenItKeepsTheIdItsRegistryRowUses() { + let defaults = freshDefaults() + defaults.set("existing-id", forKey: "com.interlinedlist.deviceId") + let shared = InMemoryDeviceIDStore() + + let resolved = DeviceIdentity.current(defaults: defaults, sharedStore: shared) + + // Preferring anything else would orphan the registry row and the + // per-machine settings filed under it. + XCTAssertEqual(resolved, "existing-id") + XCTAssertEqual(shared.read(), "existing-id", "…and the agent is told about it") + } + + func test_givenANewMachine_whenResolving_thenItMintsOnceAndPublishesIt() { + let defaults = freshDefaults() + let shared = InMemoryDeviceIDStore() + + let first = DeviceIdentity.current(defaults: defaults, sharedStore: shared) + let second = DeviceIdentity.current(defaults: defaults, sharedStore: shared) + + XCTAssertFalse(first.isEmpty) + XCTAssertEqual(first, second, "Minting twice would register the Mac twice") + XCTAssertEqual(shared.read(), first) + } + + // MARK: - Boundary: the app's preferences were wiped + + func test_givenTheAppsPreferencesWereWipedButTheAgentRemains_whenResolving_thenThePublishedIdIsAdopted() { + let defaults = freshDefaults() + let shared = InMemoryDeviceIDStore(initial: "published-id") + + let resolved = DeviceIdentity.current(defaults: defaults, sharedStore: shared) + + // Reinstalling the app must reunite it with this machine's existing + // registry row rather than minting a duplicate alongside it. + XCTAssertEqual(resolved, "published-id") + XCTAssertEqual(defaults.string(forKey: "com.interlinedlist.deviceId"), "published-id") + } + + // MARK: - Invalid input + + func test_givenAnEmptyStoredId_whenResolving_thenItIsTreatedAsAbsent() { + let defaults = freshDefaults() + defaults.set("", forKey: "com.interlinedlist.deviceId") + let shared = InMemoryDeviceIDStore() + + let resolved = DeviceIdentity.current(defaults: defaults, sharedStore: shared) + + XCTAssertFalse(resolved.isEmpty) + } + + func test_givenAnEmptyPublishedId_whenResolving_thenItIsTreatedAsAbsent() { + let defaults = freshDefaults() + let shared = InMemoryDeviceIDStore(initial: "") + + let resolved = DeviceIdentity.current(defaults: defaults, sharedStore: shared) + + XCTAssertFalse(resolved.isEmpty) + XCTAssertEqual(shared.read(), resolved) + } + + // MARK: - Upstream failure: the Keychain is unavailable + + func test_givenTheSharedStoreCannotBeWritten_whenResolving_thenTheAppStillGetsAnId() { + // An unsigned build, or one without the shared access-group entitlement, + // cannot use the Keychain at all. The app degrades to local-only — the + // agent falls back to its own settings — rather than failing at launch. + let defaults = freshDefaults() + let shared = UnwritableDeviceIDStore() + + let resolved = DeviceIdentity.current(defaults: defaults, sharedStore: shared) + + XCTAssertFalse(resolved.isEmpty) + XCTAssertEqual(defaults.string(forKey: "com.interlinedlist.deviceId"), resolved) + } +} + +/// A shared store that silently drops every write, standing in for a process +/// that is not entitled to the shared Keychain group. +private final class UnwritableDeviceIDStore: DeviceIDStoring, @unchecked Sendable { + func read() -> String? { nil } + func write(_ id: String) {} +} diff --git a/Packages/InterlinedDomain/Sources/InterlinedDomain/Models/DocumentSyncStatus.swift b/Packages/InterlinedDomain/Sources/InterlinedDomain/Models/DocumentSyncStatus.swift new file mode 100644 index 0000000..dc4ef50 --- /dev/null +++ b/Packages/InterlinedDomain/Sources/InterlinedDomain/Models/DocumentSyncStatus.swift @@ -0,0 +1,75 @@ +import Foundation +import InterlinedKit + +/// What the Document Sync Agent is doing on one machine, read out of that +/// machine's per-machine app-settings document (GitHub issue #104). +/// +/// The agent stores its configuration there rather than in `UserDefaults`, which +/// is what makes this readable at all: before the move, Settings ▸ Applications +/// could list a machine and say nothing whatsoever about what it was set up to +/// do. +/// +/// **This type reads keys written by a different codebase.** The agent +/// (`SyncAgent/`) is a clean-room package with no dependency on `InterlinedKit` +/// or `InterlinedDomain`, so the key names below are a wire contract with +/// `DocumentSyncSettingsKeys` in +/// `SyncAgent/Sources/InterlinedListSyncCore/Storage/SyncAgentConfiguration.swift`. +/// Renaming one side without the other does not fail to build — it silently +/// stops the pane reporting anything. +public struct DocumentSyncStatus: Sendable, Equatable { + + /// The wire keys, mirrored from the agent. + private enum Key { + static let prefix = "documentSync." + static let enabled = prefix + "enabled" + static let folderBookmark = prefix + "folderBookmark" + static let folderMachineID = prefix + "folderMachineId" + static let lastSyncAt = prefix + "lastSyncAt" + } + + /// Whether a sync folder is set up **on the machine that owns this + /// document**. + /// + /// False when the stored bookmark was created on another Mac. That can + /// happen legitimately — copying a machine's settings to shared, or a new + /// machine seeding from the main workstation, both move the document across + /// machines — and a bookmark is meaningless anywhere but where it was made. + /// Reporting it as configured would promise the user something that is not + /// true on that computer. + public let isConfigured: Bool + + /// Whether syncing is switched on. A machine can be configured but paused. + public let isEnabled: Bool + + /// When that machine last completed a sync cycle, if it has reported one. + /// Published on a throttle by the agent, so it lags by up to an hour. + public let lastSyncAt: Date? + + public init(isConfigured: Bool, isEnabled: Bool, lastSyncAt: Date? = nil) { + self.isConfigured = isConfigured + self.isEnabled = isEnabled + self.lastSyncAt = lastSyncAt + } + + /// Projects the agent's status out of a settings document. + /// + /// Returns nil when the document holds none of the agent's keys — that + /// machine has never run the agent, which is a different statement from + /// "has it installed but switched off", and the pane says so differently. + /// + /// - Parameter deviceID: the machine the document belongs to, used to tell + /// its own sync folder from one that travelled in from elsewhere. + public init?(bag: AppSettingsBag, deviceID: String) { + guard bag.keys.contains(where: { $0.hasPrefix(Key.prefix) }) else { return nil } + let bookmark = bag[string: Key.folderBookmark] + let owner = bag[string: Key.folderMachineID] + self.init( + // An unstamped bookmark counts as foreign: nothing has ever written + // one, so the only way to see one is a document this machine did not + // produce, and "cannot prove it is ours" must read as "not ours". + isConfigured: !(bookmark ?? "").isEmpty && owner == deviceID, + isEnabled: bag[bool: Key.enabled] ?? false, + lastSyncAt: bag[string: Key.lastSyncAt].flatMap(JSONCoders.parseDate) + ) + } +} diff --git a/Packages/InterlinedDomain/Tests/InterlinedDomainTests/DocumentSyncStatusTests.swift b/Packages/InterlinedDomain/Tests/InterlinedDomainTests/DocumentSyncStatusTests.swift new file mode 100644 index 0000000..ce0b147 --- /dev/null +++ b/Packages/InterlinedDomain/Tests/InterlinedDomainTests/DocumentSyncStatusTests.swift @@ -0,0 +1,128 @@ +import XCTest +@testable import InterlinedDomain + +/// Reading the Document Sync Agent's state out of a machine's per-machine +/// settings document (GitHub issue #104), so Settings ▸ Applications can say +/// what a machine is doing rather than only that it exists. +/// +/// The keys are a wire contract with `DocumentSyncSettingsKeys` in the agent +/// package, which shares no code with this one — so these tests spell the key +/// strings out literally. A test that reused a constant from the same file it is +/// testing would pass just as happily after a rename that broke the contract. +final class DocumentSyncStatusTests: XCTestCase { + + private let deviceID = "mac-1" + + private func bag( + enabled: Bool = true, + bookmark: String? = "YmFzZTY0", + machineID: String? = "mac-1", + lastSyncAt: String? = "2026-09-16T19:39:07.135Z" + ) -> AppSettingsBag { + var bag = AppSettingsBag() + bag[bool: "documentSync.enabled"] = enabled + bag[string: "documentSync.folderBookmark"] = bookmark + bag[string: "documentSync.folderMachineId"] = machineID + bag[string: "documentSync.lastSyncAt"] = lastSyncAt + return bag + } + + // MARK: - Happy path + + func test_givenAMachineRunningTheAgent_whenReadingItsDocument_thenItReportsConfiguredAndOn() throws { + let status = try XCTUnwrap(DocumentSyncStatus(bag: bag(), deviceID: deviceID)) + + XCTAssertTrue(status.isConfigured) + XCTAssertTrue(status.isEnabled) + let lastSync = try XCTUnwrap(status.lastSyncAt) + XCTAssertEqual(lastSync.timeIntervalSince1970, 1_789_587_547.135, accuracy: 0.01) + } + + func test_givenAConfiguredButPausedMachine_whenReadingItsDocument_thenItIsConfiguredAndOff() throws { + let status = try XCTUnwrap( + DocumentSyncStatus(bag: bag(enabled: false), deviceID: deviceID) + ) + + // Configured and paused is a real, distinct state — the folder is set up, + // the user just switched syncing off. + XCTAssertTrue(status.isConfigured) + XCTAssertFalse(status.isEnabled) + } + + // MARK: - Invalid input + + func test_givenAFolderChosenOnAnotherMac_whenReadingTheDocument_thenItIsNotConfiguredHere() throws { + let status = try XCTUnwrap( + DocumentSyncStatus(bag: bag(machineID: "mac-2"), deviceID: deviceID) + ) + + // A security-scoped bookmark is meaningless anywhere but where it was + // made, so reporting it as configured would promise something untrue. + XCTAssertFalse(status.isConfigured) + } + + func test_givenABookmarkWithNoMachineStamp_whenReadingTheDocument_thenItIsNotConfiguredHere() throws { + let status = try XCTUnwrap( + DocumentSyncStatus(bag: bag(machineID: nil), deviceID: deviceID) + ) + + XCTAssertFalse(status.isConfigured) + } + + // MARK: - Upstream failure (a payload this build does not understand) + + func test_givenAnUnparseableTimestamp_whenReadingTheDocument_thenTheRestStillReports() throws { + let status = try XCTUnwrap( + DocumentSyncStatus(bag: bag(lastSyncAt: "yesterday"), deviceID: deviceID) + ) + + // The payload is client-owned and opaque to the server, so a future build + // may store something this one cannot read. One unreadable field must not + // take the whole pane down with it. + XCTAssertNil(status.lastSyncAt) + XCTAssertTrue(status.isConfigured) + } + + func test_givenAFieldOfTheWrongType_whenReadingTheDocument_thenItDegradesToUnset() throws { + var mixed = bag() + // A build that stored the flag as a string rather than a bool. + mixed[string: "documentSync.enabled"] = "yes" + + let status = try XCTUnwrap(DocumentSyncStatus(bag: mixed, deviceID: deviceID)) + + XCTAssertFalse(status.isEnabled) + XCTAssertTrue(status.isConfigured) + } + + // MARK: - Boundary + + func test_givenAMachineThatHasNeverRunTheAgent_whenReadingItsDocument_thenThereIsNothingToReport() { + var other = AppSettingsBag() + other[int: "sidebarWidth"] = 280 + + // Distinct from "installed but switched off" — the pane shows no Document + // Sync section at all rather than asserting the agent is off. + XCTAssertNil(DocumentSyncStatus(bag: other, deviceID: deviceID)) + } + + func test_givenAnEmptyDocument_whenReadingIt_thenThereIsNothingToReport() { + XCTAssertNil(DocumentSyncStatus(bag: AppSettingsBag(), deviceID: deviceID)) + } + + func test_givenTheAgentRanButNoFolderWasChosen_whenReadingTheDocument_thenItIsNotConfigured() throws { + let status = try XCTUnwrap( + DocumentSyncStatus(bag: bag(bookmark: nil), deviceID: deviceID) + ) + + XCTAssertFalse(status.isConfigured) + XCTAssertTrue(status.isEnabled) + } + + func test_givenAnEmptyBookmarkString_whenReadingTheDocument_thenItIsNotConfigured() throws { + let status = try XCTUnwrap( + DocumentSyncStatus(bag: bag(bookmark: ""), deviceID: deviceID) + ) + + XCTAssertFalse(status.isConfigured) + } +} diff --git a/Packages/InterlinedKit/Sources/InterlinedKit/Auth/DeviceIDStore.swift b/Packages/InterlinedKit/Sources/InterlinedKit/Auth/DeviceIDStore.swift new file mode 100644 index 0000000..dfdb409 --- /dev/null +++ b/Packages/InterlinedKit/Sources/InterlinedKit/Auth/DeviceIDStore.swift @@ -0,0 +1,146 @@ +import Foundation +import Security +import os + +/// Publishes this Mac's app-settings device id to a place the bundled +/// document-sync agent can read it (GitHub issue #104). +/// +/// The agent keeps its configuration in per-machine app settings, which live at +/// `…/devices/{deviceId}/settings`. Addressing that document means the agent and +/// the app must agree on **one** device id per machine — two ids would produce +/// two rows in the registry for the same computer and split its settings in +/// half. +/// +/// They cannot simply share `UserDefaults`: the agent is a separate process with +/// its own bundle identifier (`com.interlinedlist.macos.sync`) and therefore its +/// own defaults domain. The shared Keychain access group is the channel that +/// already exists and already works — the agent reads the bearer token through +/// it — so the device id travels the same way rather than adding an app-group +/// entitlement, which would mean re-provisioning both signed bundles. +/// +/// The device id is **not a secret**; the Keychain is used here purely as the +/// cross-process store both sandboxes are entitled to. That is why this type is +/// separate from ``KeychainTokenStore`` despite the similar mechanics: the +/// handling rules for a bearer token and for an opaque machine id are not the +/// same, and sharing one type would invite treating them as if they were. +public protocol DeviceIDStoring: Sendable { + /// The published id, or nil when none has been written yet. + func read() -> String? + /// Publishes `id`, replacing any previous value. + func write(_ id: String) +} + +// MARK: - Keychain + +/// Shared-Keychain implementation. Deliberately non-throwing. +/// +/// A device id that cannot be shared is a degraded-but-working state, not a +/// failure: the app keeps its own copy in `UserDefaults` and the agent falls +/// back to local settings. Propagating a Keychain `OSStatus` out of here would +/// give every caller an error it can do nothing about, on a path that must +/// never block launch. +public struct KeychainDeviceIDStore: DeviceIDStoring { + + private let service: String + private let account: String + private let accessGroup: String? + private let logger = Logger( + subsystem: Bundle.main.bundleIdentifier ?? "com.interlinedlist.kit", + category: "DeviceIDStore" + ) + + /// - Parameters: + /// - service: `kSecAttrService`. **Must match the agent's + /// `SyncConfiguration.deviceIDService`** — the two processes are + /// independent codebases agreeing on one wire contract. + /// - accessGroup: the shared group both bundles list in + /// `keychain-access-groups`. + public init( + service: String = "com.interlinedlist.macos.device-id", + account: String = "default", + accessGroup: String? = nil + ) { + self.service = service + self.account = account + self.accessGroup = accessGroup + } + + /// `errSecMissingEntitlement` — returned when querying an access group the + /// process is not entitled to, which is the normal state for an ad-hoc + /// signed test host. Treated as a miss so tests and unsigned local builds + /// degrade to "nothing published" instead of failing. + private static let missingEntitlement: OSStatus = -34018 + + private func baseQuery() -> [CFString: Any] { + var query: [CFString: Any] = [ + kSecClass: kSecClassGenericPassword, + kSecAttrService: service, + kSecAttrAccount: account + ] + if let accessGroup { query[kSecAttrAccessGroup] = accessGroup } + return query + } + + public func read() -> String? { + var query = baseQuery() + query[kSecMatchLimit] = kSecMatchLimitOne + query[kSecReturnData] = true + + var item: CFTypeRef? + let status = SecItemCopyMatching(query as CFDictionary, &item) + switch status { + case errSecSuccess: + guard let data = item as? Data, + let id = String(data: data, encoding: .utf8), + !id.isEmpty else { return nil } + return id + case errSecItemNotFound, Self.missingEntitlement: + return nil + default: + logger.error("Device id read failed with OSStatus \(status, privacy: .public)") + return nil + } + } + + public func write(_ id: String) { + guard let data = id.data(using: .utf8) else { return } + let query = baseQuery() + + // Update first, then add. `SecItemAdd` on an existing item answers + // `errSecDuplicateItem` rather than replacing, so add-then-update would + // leave a stale id in place on every machine that already has one. + let update = SecItemUpdate(query as CFDictionary, [kSecValueData: data] as CFDictionary) + if update == errSecSuccess { return } + + var insert = query + insert[kSecValueData] = data + // The agent runs as a login item, so the item has to be readable before + // the user unlocks the screen for the first time after a reboot. + insert[kSecAttrAccessible] = kSecAttrAccessibleAfterFirstUnlock + let status = SecItemAdd(insert as CFDictionary, nil) + if status != errSecSuccess && status != Self.missingEntitlement { + logger.error("Device id write failed with OSStatus \(status, privacy: .public)") + } + } +} + +// MARK: - In-memory (tests + previews) + +/// In-memory implementation for unit tests and previews, where touching the +/// real Keychain is both undesirable and (unsigned) impossible. +public final class InMemoryDeviceIDStore: DeviceIDStoring, @unchecked Sendable { + private let lock = NSLock() + private var id: String? + + public init(initial: String? = nil) { self.id = initial } + + public func read() -> String? { + lock.lock(); defer { lock.unlock() } + return id + } + + public func write(_ id: String) { + lock.lock(); defer { lock.unlock() } + self.id = id + } +} diff --git a/SyncAgent/Sources/InterlinedListSyncCore/API/AppSettingsModels.swift b/SyncAgent/Sources/InterlinedListSyncCore/API/AppSettingsModels.swift new file mode 100644 index 0000000..32942e9 --- /dev/null +++ b/SyncAgent/Sources/InterlinedListSyncCore/API/AppSettingsModels.swift @@ -0,0 +1,169 @@ +import Foundation + +// Wire models for the **per-machine app-settings** routes the agent stores its +// configuration in (GitHub issue #104): +// +// GET /api/user/app-settings/{appKey}/devices/{deviceId}/settings +// PUT /api/user/app-settings/{appKey}/devices/{deviceId}/settings +// +// Independent of the main app's `InterlinedKit` DTOs, like every other model in +// this package — the agent is a clean-room implementation. Both sides were +// verified live against the same routes on 2026-09-16 (PR #102), so they agree +// because the wire says so, not because they share code. + +// MARK: - Opaque settings values + +/// A losslessly round-trippable JSON value. +/// +/// The server stores the settings payload **verbatim and uninterpreted**, which +/// means this agent shares one document with the main app. Decoding into a +/// concrete struct would drop every key this build does not know about — and +/// since the PUT *replaces* the document rather than merging it, the next write +/// would then delete the main app's per-machine settings outright. +/// +/// Keeping values opaque is what makes the overlay in +/// ``SyncAgentConfiguration/apply(to:)`` safe. +public enum SettingsValue: Codable, Sendable, Equatable { + case null + case bool(Bool) + case number(Double) + case string(String) + case array([SettingsValue]) + case object([String: SettingsValue]) + + public init(from decoder: any Decoder) throws { + let c = try decoder.singleValueContainer() + if c.decodeNil() { self = .null; return } + if let v = try? c.decode(Bool.self) { self = .bool(v); return } + if let v = try? c.decode(Double.self) { self = .number(v); return } + if let v = try? c.decode(String.self) { self = .string(v); return } + if let v = try? c.decode([SettingsValue].self) { self = .array(v); return } + if let v = try? c.decode([String: SettingsValue].self) { self = .object(v); return } + throw DecodingError.dataCorruptedError(in: c, debugDescription: "Unrecognised JSON value") + } + + public func encode(to encoder: any Encoder) throws { + var c = encoder.singleValueContainer() + switch self { + case .null: try c.encodeNil() + case .bool(let v): try c.encode(v) + case .number(let v): try c.encode(v) + case .string(let v): try c.encode(v) + case .array(let v): try c.encode(v) + case .object(let v): try c.encode(v) + } + } + + // Readers that answer nil rather than trapping when the stored value turns + // out to be another type — a settings blob is client-owned and a future + // build may legitimately change a field's shape. + public var boolValue: Bool? { if case .bool(let v) = self { return v }; return nil } + public var stringValue: String? { if case .string(let v) = self { return v }; return nil } + public var doubleValue: Double? { if case .number(let v) = self { return v }; return nil } +} + +// MARK: - Documents + +/// A stored settings document. +/// +/// Shape verified live 2026-09-16 (PR #102): the document comes back **bare**, +/// with no envelope. +/// +/// ``` +/// {"appKey":"interlinedlist-macos","scope":"device","deviceId":"…", +/// "version":3,"updatedAt":"2026-09-16T19:39:07.135Z","schemaVersion":1, +/// "settings":{…}} +/// ``` +/// +/// `version` cannot be optional: it is the `baseVersion` of the next write, and +/// a client that cannot read it cannot legally write at all. +public struct DeviceSettingsDocument: Decodable, Sendable, Equatable { + public let version: Int + public let updatedAt: Date? + /// The machine this document belongs to. Present on device-scoped + /// documents; nil on the account-wide one. + public let deviceId: String? + public let schemaVersion: Int? + /// Opaque, client-owned payload — see ``SettingsValue``. + public let settings: [String: SettingsValue] + + public init( + version: Int, + updatedAt: Date? = nil, + deviceId: String? = nil, + schemaVersion: Int? = nil, + settings: [String: SettingsValue] = [:] + ) { + self.version = version + self.updatedAt = updatedAt + self.deviceId = deviceId + self.schemaVersion = schemaVersion + self.settings = settings + } +} + +/// `PUT …/devices/{deviceId}/settings` body. +/// +/// **`baseVersion` is mandatory**, verified live: omitting it fails every write +/// with `400 {"error":"baseVersion must be an integer >= 0"}`. Send `0` to +/// create, or the `version` last read to update. A stale value answers `409` and +/// writes nothing — see ``APIError/versionConflict(current:)``. +public struct WriteDeviceSettingsBody: Encodable, Sendable, Equatable { + public let settings: [String: SettingsValue] + public let baseVersion: Int + public let schemaVersion: Int? + + public init(settings: [String: SettingsValue], baseVersion: Int, schemaVersion: Int? = nil) { + self.settings = settings + self.baseVersion = baseVersion + self.schemaVersion = schemaVersion + } +} + +/// `POST …/devices` body — register this machine. +/// +/// The agent sends this on exactly one path: a settings write that answered 404 +/// because the device is absent from the registry. See +/// ``DeviceSettingsAPI/registerDevice(deviceId:deviceName:)``. +public struct RegisterDeviceBody: Encodable, Sendable, Equatable { + public let deviceId: String + public let deviceName: String + public let platform: String + + public init(deviceId: String, deviceName: String, platform: String = "macos") { + self.deviceId = deviceId + self.deviceName = deviceName + self.platform = platform + } +} + +// MARK: - API surface + +/// The per-machine settings operations the agent needs. +/// +/// Separate from ``DocumentSyncAPI`` rather than bolted onto it: the sync engine +/// has no business with settings, and widening its protocol would force every +/// engine test double to implement routes it never calls. +public protocol DeviceSettingsAPI: Sendable { + /// This machine's stored document, or nil when it has never written one + /// (`404`, the ordinary first-run state). + func fetchDeviceSettings(deviceId: String) async throws -> DeviceSettingsDocument? + + /// Compare-and-set write. Throws ``APIError/versionConflict(current:)`` when + /// `baseVersion` is stale, and ``APIError/deviceNotRegistered`` when the + /// machine is absent from the registry. + func writeDeviceSettings( + deviceId: String, + settings: [String: SettingsValue], + baseVersion: Int + ) async throws -> DeviceSettingsDocument + + /// Adds this machine to the registry. + /// + /// Verified live 2026-09-16: `POST …/devices` is an **upsert keyed on + /// `deviceId`** — re-posting an existing id overwrites its `deviceName` + /// rather than duplicating the row. Calling it unconditionally would reset a + /// machine the user had renamed back to its hostname. The agent therefore + /// calls it only after a write proved the device absent. + func registerDevice(deviceId: String, deviceName: String) async throws +} diff --git a/SyncAgent/Sources/InterlinedListSyncCore/API/SyncAPIClient.swift b/SyncAgent/Sources/InterlinedListSyncCore/API/SyncAPIClient.swift index 6265ed8..71f830d 100644 --- a/SyncAgent/Sources/InterlinedListSyncCore/API/SyncAPIClient.swift +++ b/SyncAgent/Sources/InterlinedListSyncCore/API/SyncAPIClient.swift @@ -11,6 +11,23 @@ public enum APIError: Error, Sendable, Equatable { case transport(String) case decoding(String) case invalidResponse + /// A compare-and-set settings write lost the race: the stored document moved + /// on since it was read and **nothing was written**. + /// + /// `current` is the winning document, taken straight from the 409 body, so a + /// retry can re-base without spending another request. The main app cannot + /// do this — its `APIClient` reduces every non-2xx body to a message string — + /// but this client owns its error path end to end, and the agent writes from + /// the background where losing the race is likeliest. + case versionConflict(current: DeviceSettingsDocument?) + /// A per-device settings write was addressed to a machine that is not in the + /// registry. Retrying cannot fix it; registering the device can. + case deviceNotRegistered + /// The main app has not published a device id into the shared Keychain + /// group yet, so there is no per-machine document to address. Distinct from + /// ``deviceNotRegistered``: nothing is wrong, the app just has not launched + /// since this agent was installed. + case noDeviceIdentity } /// The document-sync operations the engine needs. A protocol so tests can @@ -29,7 +46,7 @@ public protocol DocumentSyncAPI: Sendable { /// URLSession-backed client for the InterlinedList Documents API. Reads the /// bearer token fresh on every request via the injected ``TokenProviding`` so a /// mid-run sign-in / sign-out in the main app is picked up immediately. -public actor SyncAPIClient: DocumentSyncAPI { +public actor SyncAPIClient: DocumentSyncAPI, DeviceSettingsAPI { private let baseURL: URL private let session: URLSession @@ -140,6 +157,82 @@ public actor SyncAPIClient: DocumentSyncAPI { } } + // MARK: - DeviceSettingsAPI (GitHub issue #104) + + private var deviceSettingsPathPrefix: String { + "/api/user/app-settings/\(pathEncode(SyncConfiguration.appSettingsKey))/devices" + } + + public func fetchDeviceSettings(deviceId: String) async throws -> DeviceSettingsDocument? { + let request = try makeRequest( + method: "GET", + path: "\(deviceSettingsPathPrefix)/\(pathEncode(deviceId))/settings" + ) + do { + return try await perform(request, as: DeviceSettingsDocument.self) + } catch APIError.notFound { + // A machine that has never written settings answers 404. That is the + // ordinary first-run state — the distinction the caller needs is + // "nothing stored" (nil) versus "could not ask" (a thrown error), + // because only the first one may trigger a migration. + return nil + } + } + + public func writeDeviceSettings( + deviceId: String, + settings: [String: SettingsValue], + baseVersion: Int + ) async throws -> DeviceSettingsDocument { + let body = WriteDeviceSettingsBody( + settings: settings, + baseVersion: baseVersion, + schemaVersion: SyncConfiguration.settingsSchemaVersion + ) + let request = try makeRequest( + method: "PUT", + path: "\(deviceSettingsPathPrefix)/\(pathEncode(deviceId))/settings", + jsonBody: body + ) + do { + return try await perform(request, as: DeviceSettingsDocument.self) + } catch APIError.notFound { + // 404 on the *write* route is not "no document yet" — `baseVersion: + // 0` creates one happily. It means the device is missing from the + // registry, which a retry will never fix. + throw APIError.deviceNotRegistered + } catch APIError.http(let status, let responseBody) where status == 409 { + throw APIError.versionConflict(current: Self.conflictDocument(from: responseBody)) + } + } + + public func registerDevice(deviceId: String, deviceName: String) async throws { + let request = try makeRequest( + method: "POST", + path: deviceSettingsPathPrefix, + jsonBody: RegisterDeviceBody(deviceId: deviceId, deviceName: deviceName) + ) + // The response wraps the device (`{"device":{…}}`) but the agent has no + // use for it: it registers only so the settings write that just failed + // can be retried. + _ = try await sendExpectingSuccess(request) + } + + /// Pulls the winning document out of a 409 body. + /// + /// ``` + /// 409 {"error":"version_conflict","code":"version_conflict","current":{…SettingsDoc…}} + /// ``` + /// + /// Best-effort by design: a conflict whose body cannot be parsed still has + /// to surface as a conflict, so the retry falls back to a fresh read rather + /// than the whole write failing on a decoding error. + static func conflictDocument(from body: String?) -> DeviceSettingsDocument? { + guard let data = body?.data(using: .utf8) else { return nil } + struct ConflictEnvelope: Decodable { let current: DeviceSettingsDocument? } + return try? JSONCoding.makeDecoder().decode(ConflictEnvelope.self, from: data).current + } + // MARK: - Request building private func makeRequest( diff --git a/SyncAgent/Sources/InterlinedListSyncCore/App/AppDelegate.swift b/SyncAgent/Sources/InterlinedListSyncCore/App/AppDelegate.swift index e73924a..2d901f6 100644 --- a/SyncAgent/Sources/InterlinedListSyncCore/App/AppDelegate.swift +++ b/SyncAgent/Sources/InterlinedListSyncCore/App/AppDelegate.swift @@ -9,12 +9,29 @@ public final class AppDelegate: NSObject, NSApplicationDelegate { private let logger = Logger(subsystem: SyncConfiguration.logSubsystem, category: "AppDelegate") - private let prefs = PreferencesManager() private let state = SyncStateModel() private let notifications = NotificationManager() private let loginItems: any LoginItemManaging = LoginItemManager() private let tokenStore = SharedTokenStore() + /// The agent's configuration lives in per-machine app settings + /// (GitHub issue #104); `UserDefaults` is the mirror it falls back to until + /// a write is confirmed. + /// + /// Its own `SharedTokenStore` and `SyncAPIClient`, not the engine's: a + /// property initialiser cannot reach `self`, and the two have different + /// lifetimes anyway — the engine's client is torn down and rebuilt whenever + /// the sync folder changes, while settings must stay writable throughout. + /// Both token stores are stateless readers of the same shared Keychain item, + /// so a mid-run sign-in or sign-out is still picked up by both. + private let prefs = PreferencesManager( + remote: RemoteConfigurationStore( + api: SyncAPIClient(tokenProvider: SharedTokenStore()), + identity: SharedDeviceIdentity(), + deviceName: DeviceNaming.suggestedName + ) + ) + private var engine: SyncEngine? private var statusItem: StatusItemController? private var eventTask: Task? @@ -27,8 +44,29 @@ public final class AppDelegate: NSObject, NSApplicationDelegate { statusItem = StatusItemController(state: state, actions: makeActions()) + // Reconcile with per-machine app settings *before* deciding whether this + // machine is configured (GitHub issue #104). The stored configuration + // can name a sync folder the local mirror has never heard of — a + // reinstall is exactly that case — so asking `hasSyncFolder` first would + // prompt the user to choose a folder they already chose. + // + // Signed out there is nothing to read, so the old local-only path runs + // unchanged rather than waiting on a request that cannot succeed. + if tokenStore.hasToken { + Task { [weak self] in + await self?.prefs.synchronize() + self?.startOrPrompt() + } + } else { + startOrPrompt() + } + } + + /// Starts the engine, or asks for a sync folder when this machine has none. + private func startOrPrompt() { if !prefs.hasSyncFolder { - // First run without a folder: let the user pick one. + // No folder here — either a genuine first run, or a configuration + // whose bookmark belongs to another Mac and was therefore refused. openPreferences() } else if tokenStore.hasToken { startEngine() @@ -90,6 +128,10 @@ public final class AppDelegate: NSObject, NSApplicationDelegate { state.apply(event) switch event { case .cycleCompleted(let summary): + // Report the cycle to per-machine app settings so Settings ▸ + // Applications can say what this machine is doing, not just that it + // exists. Throttled inside `recordSync`. + prefs.recordSync(at: summary.finishedAt) if prefs.notificationsEnabled && prefs.notifyOnCompletion { notifications.notifySyncCompleted(summary) } diff --git a/SyncAgent/Sources/InterlinedListSyncCore/Auth/SharedDeviceIdentity.swift b/SyncAgent/Sources/InterlinedListSyncCore/Auth/SharedDeviceIdentity.swift new file mode 100644 index 0000000..b041785 --- /dev/null +++ b/SyncAgent/Sources/InterlinedListSyncCore/Auth/SharedDeviceIdentity.swift @@ -0,0 +1,97 @@ +import Foundation +import Security +import os + +/// Supplies this machine's app-settings device id — the address of the +/// per-machine settings document the agent stores its configuration in +/// (GitHub issue #104). +public protocol DeviceIdentifying: Sendable { + /// The id for this machine, or nil when the main app has not published one. + func currentDeviceID() -> String? +} + +/// Reads the app-settings device id the **main app** publishes into the shared +/// Keychain access group. +/// +/// **Read-only, and that is the whole point.** The id keys a row in the server's +/// device registry; if this process minted its own when it found none, one Mac +/// would appear twice and its settings would split across two documents. The +/// app owns minting (`DeviceIdentity.current()`); the agent only ever consumes. +/// +/// No published id therefore means "per-machine settings are not addressable +/// yet", which the configuration store treats as +/// ``RemoteConfigurationState/unavailable`` — the agent keeps running off local +/// `UserDefaults` exactly as it did before this feature existed. +/// +/// Sharing contract (must match the main app's `KeychainDeviceIDStore`): +/// `kSecClassGenericPassword`, service ``SyncConfiguration/deviceIDService``, +/// account ``SyncConfiguration/deviceIDAccount``, access group +/// ``SyncConfiguration/sharedAccessGroup``. +public struct SharedDeviceIdentity: DeviceIdentifying { + + private let service: String + private let account: String + private let accessGroup: String? + private let logger = Logger(subsystem: SyncConfiguration.logSubsystem, category: "DeviceIdentity") + + public init( + service: String = SyncConfiguration.deviceIDService, + account: String = SyncConfiguration.deviceIDAccount, + accessGroup: String? = SyncConfiguration.sharedAccessGroup + ) { + self.service = service + self.account = account + self.accessGroup = accessGroup + } + + public func currentDeviceID() -> String? { + var query: [CFString: Any] = [ + kSecClass: kSecClassGenericPassword, + kSecAttrService: service, + kSecAttrAccount: account, + kSecMatchLimit: kSecMatchLimitOne, + kSecReturnData: true + ] + if let accessGroup { + query[kSecAttrAccessGroup] = accessGroup + } + + var item: CFTypeRef? + let status = SecItemCopyMatching(query as CFDictionary, &item) + switch status { + case errSecSuccess: + guard let data = item as? Data, + let id = String(data: data, encoding: .utf8), + !id.isEmpty else { + logger.error("Device id item present but not decodable") + return nil + } + return id + case errSecItemNotFound: + return nil + default: + logger.error("Device id read failed: OSStatus \(status, privacy: .public)") + return nil + } + } +} + +/// A fixed in-memory device id, for tests and previews. +public struct StaticDeviceIdentity: DeviceIdentifying { + private let id: String? + public init(_ id: String?) { self.id = id } + public func currentDeviceID() -> String? { id } +} + +/// How this machine names itself when it has to add its own registry row. +/// +/// Mirrors the main app's `DeviceIdentity.suggestedName` so a machine registered +/// by either process gets the same label, and the user sees one name rather than +/// two spellings of the same Mac. +public enum DeviceNaming { + public static var suggestedName: String { + let host = ProcessInfo.processInfo.hostName + // `hostName` usually comes back as "studio-mac.local"; trim the suffix. + return host.hasSuffix(".local") ? String(host.dropLast(6)) : host + } +} diff --git a/SyncAgent/Sources/InterlinedListSyncCore/Configuration.swift b/SyncAgent/Sources/InterlinedListSyncCore/Configuration.swift index 0cc959c..a46e68c 100644 --- a/SyncAgent/Sources/InterlinedListSyncCore/Configuration.swift +++ b/SyncAgent/Sources/InterlinedListSyncCore/Configuration.swift @@ -26,6 +26,39 @@ public enum SyncConfiguration { /// Fully-qualified access group used in Keychain queries. public static let sharedAccessGroup = "\(teamIdentifier).\(sharedAccessGroupSuffix)" + /// `kSecAttrService` of the app-settings **device id** the main app + /// publishes into the same shared group (GitHub issue #104). + /// + /// Must match the main app's `DeviceIdentity.sharedStore`. The id is not a + /// secret; the Keychain is simply the cross-process channel both sandboxes + /// are already entitled to — the agent has its own bundle identifier and so + /// its own `UserDefaults` domain, which is why the value cannot just be a + /// shared preference. + public static let deviceIDService = "com.interlinedlist.macos.device-id" + /// `kSecAttrAccount` of that item. + public static let deviceIDAccount = "default" + + // MARK: - Per-machine app settings (GitHub issue #104) + + /// The app-settings namespace this client's settings live under. **Must not + /// change** — it is the key every stored setting is filed under, and a new + /// one orphans all of them. Matches the main app's + /// `AppEnvironment.appSettingsKey`. + public static let appSettingsKey = "interlinedlist-macos" + + /// Version stamped on the settings payload this build writes, so a future + /// build can tell what it is reading before it interprets it. + public static let settingsSchemaVersion = 1 + + /// How stale a published "last synced" timestamp is allowed to get. + /// + /// The timestamp is status, not configuration: it exists so Settings ▸ + /// Applications can say what a machine is actually doing rather than only + /// that it exists. Writing it on every poll cycle would mean a settings PUT + /// every minute per machine, forever, to move a value nobody is watching in + /// real time — so it rides an hourly throttle instead. + public static let lastSyncPublishInterval: TimeInterval = 3600 + // MARK: - Filesystem correlation /// Extended-attribute key holding the server document id on each `.md` file. diff --git a/SyncAgent/Sources/InterlinedListSyncCore/Storage/PreferencesManager.swift b/SyncAgent/Sources/InterlinedListSyncCore/Storage/PreferencesManager.swift index 8be5362..978818a 100644 --- a/SyncAgent/Sources/InterlinedListSyncCore/Storage/PreferencesManager.swift +++ b/SyncAgent/Sources/InterlinedListSyncCore/Storage/PreferencesManager.swift @@ -1,9 +1,26 @@ import Foundation import AppKit +import os -/// User-facing settings, persisted to `UserDefaults`, plus the security-scoped -/// bookmark for the sync folder. `@MainActor` because it is observed by SwiftUI -/// and drives `NSOpenPanel`. +/// User-facing settings plus the security-scoped bookmark for the sync folder. +/// `@MainActor` because it is observed by SwiftUI and drives `NSOpenPanel`. +/// +/// Two homes, one of them authoritative (GitHub issue #104): +/// +/// - **Per-machine app settings** (`…/devices/{deviceId}/settings`) are where +/// the configuration actually lives. That is what `/help/app-settings` +/// describes per-machine storage as being for — a value meaningful on one +/// machine only — and it means a reinstall recovers the user's setup instead +/// of starting from nothing. +/// - **`UserDefaults`** stays as the local mirror and the fallback. It is what +/// the agent runs on before the first successful remote write is confirmed, +/// while offline, and on a machine the main app has not yet published a device +/// id for. Every setter still writes it, so nothing in the agent has to care +/// whether the network is up. +/// +/// The bookmark is written to both, but it is only ever *resolved* from +/// `UserDefaults` — a bookmark stamped with another machine is dropped on the +/// way in, so what reaches local storage is always this Mac's own. @MainActor public final class PreferencesManager: ObservableObject { @@ -17,37 +34,67 @@ public final class PreferencesManager: ObservableObject { static let launchAtLogin = "launchAtLogin" static let folderBookmark = "syncFolderBookmark" static let folderPath = "syncFolderPath" + /// Set only once a migration write has been **confirmed** by the server. + /// Until then the agent keeps reading `UserDefaults`, which is the whole + /// point: a half-finished migration must not look like a finished one. + static let didMigrateToAppSettings = "didMigrateSettingsToAppSettings" + /// When the reported "last synced" value was last published, so the + /// hourly throttle survives a relaunch instead of firing a write on + /// every launch. + static let lastSyncPublishedAt = "lastSyncPublishedAt" } private let defaults: UserDefaults + private let remote: RemoteConfigurationStore? + private let logger = Logger(subsystem: SyncConfiguration.logSubsystem, category: "Preferences") private var accessingURL: URL? + /// Coalescing window for remote writes. Each toggle in the preferences + /// window is one `didSet`; without this, dragging the poll-interval slider + /// would issue a settings PUT per step. + private let remoteSaveDebounce: Duration + private var pendingSave: Task? + /// Suppresses the write-back that applying a freshly-read remote + /// configuration would otherwise trigger. + private var isApplyingRemote = false + @Published public var pollIntervalSeconds: TimeInterval { - didSet { defaults.set(pollIntervalSeconds, forKey: Key.pollInterval) } + didSet { defaults.set(pollIntervalSeconds, forKey: Key.pollInterval); scheduleRemoteSave() } } @Published public var syncEnabled: Bool { - didSet { defaults.set(syncEnabled, forKey: Key.syncEnabled) } + didSet { defaults.set(syncEnabled, forKey: Key.syncEnabled); scheduleRemoteSave() } } @Published public var notificationsEnabled: Bool { - didSet { defaults.set(notificationsEnabled, forKey: Key.notificationsEnabled) } + didSet { defaults.set(notificationsEnabled, forKey: Key.notificationsEnabled); scheduleRemoteSave() } } @Published public var notifyOnCompletion: Bool { - didSet { defaults.set(notifyOnCompletion, forKey: Key.notifyOnCompletion) } + didSet { defaults.set(notifyOnCompletion, forKey: Key.notifyOnCompletion); scheduleRemoteSave() } } @Published public var notifyOnErrors: Bool { - didSet { defaults.set(notifyOnErrors, forKey: Key.notifyOnErrors) } + didSet { defaults.set(notifyOnErrors, forKey: Key.notifyOnErrors); scheduleRemoteSave() } } @Published public var notifyOnConflicts: Bool { - didSet { defaults.set(notifyOnConflicts, forKey: Key.notifyOnConflicts) } + didSet { defaults.set(notifyOnConflicts, forKey: Key.notifyOnConflicts); scheduleRemoteSave() } } @Published public var launchAtLogin: Bool { - didSet { defaults.set(launchAtLogin, forKey: Key.launchAtLogin) } + didSet { defaults.set(launchAtLogin, forKey: Key.launchAtLogin); scheduleRemoteSave() } } /// Display-only path of the chosen folder (the source of truth is the bookmark). @Published public private(set) var syncFolderPath: String? - public init(defaults: UserDefaults = .standard) { + /// When the last completed sync cycle was reported to app settings. + /// Reporting only — the engine's own cursor lives in the on-disk ledger. + private var lastSyncAt: Date? + private var lastSyncPublishedAt: Date? + + public init( + defaults: UserDefaults = .standard, + remote: RemoteConfigurationStore? = nil, + remoteSaveDebounce: Duration = .seconds(2) + ) { self.defaults = defaults + self.remote = remote + self.remoteSaveDebounce = remoteSaveDebounce let stored = defaults.object(forKey: Key.pollInterval) as? TimeInterval self.pollIntervalSeconds = stored.map { min(max($0, SyncConfiguration.minPollInterval), SyncConfiguration.maxPollInterval) @@ -59,6 +106,143 @@ public final class PreferencesManager: ObservableObject { self.notifyOnConflicts = (defaults.object(forKey: Key.notifyOnConflicts) as? Bool) ?? true self.launchAtLogin = (defaults.object(forKey: Key.launchAtLogin) as? Bool) ?? false self.syncFolderPath = defaults.string(forKey: Key.folderPath) + self.lastSyncPublishedAt = defaults.object(forKey: Key.lastSyncPublishedAt) as? Date + } + + // MARK: - Per-machine app settings + + /// True once a configuration write to per-machine app settings has been + /// confirmed. Until then `UserDefaults` is the fallback the agent runs on. + public var hasMigratedToAppSettings: Bool { + defaults.bool(forKey: Key.didMigrateToAppSettings) + } + + /// Brings this machine's configuration in line with per-machine app + /// settings, migrating the local one up exactly once if the server has none. + /// + /// Call once at launch, before starting the engine: a remote configuration + /// may name a different sync folder than the local mirror does. + public func synchronize() async { + guard let remote, let machineID = await remote.currentDeviceID else { return } + + let state = await remote.load() + let resolution = ConfigurationResolver.resolve( + remote: state, + local: configuration(machineID: machineID), + hasMigrated: hasMigratedToAppSettings + ) + apply(resolution.configuration) + + if resolution.shouldMigrate { + await write(configuration(machineID: machineID), to: remote) + return + } + // Applying a stored configuration can legitimately leave this machine + // holding more than the server does — a folder chosen while offline is + // kept rather than cleared (see `apply`). Push the difference back so the + // two agree, instead of silently diverging until the next toggle. + let effective = configuration(machineID: machineID) + if case .stored(let stored) = state, stored != effective { + await write(effective, to: remote) + } + } + + /// Reports a completed sync cycle, throttled. + /// + /// Status, not configuration: Settings ▸ Applications shows it so a machine + /// can say what it is doing, and nothing reads it back. Writing it every + /// cycle would be a settings PUT a minute, per machine, forever. + /// + /// Returns the publishing task, or nil when the throttle swallowed the + /// report. Callers in the agent ignore it; tests await it, so the publish is + /// observable without sleeping on a timer. + @discardableResult + public func recordSync(at date: Date) -> Task? { + lastSyncAt = date + guard let remote else { return nil } + if let published = lastSyncPublishedAt, + date.timeIntervalSince(published) < SyncConfiguration.lastSyncPublishInterval { + return nil + } + lastSyncPublishedAt = date + defaults.set(date, forKey: Key.lastSyncPublishedAt) + return Task { [weak self] in + guard let self, let machineID = await remote.currentDeviceID else { return } + await self.write(self.configuration(machineID: machineID), to: remote) + } + } + + /// The current configuration as stored remotely. + func configuration(machineID: String) -> SyncAgentConfiguration { + SyncAgentConfiguration( + syncEnabled: syncEnabled, + pollIntervalSeconds: pollIntervalSeconds, + launchAtLogin: launchAtLogin, + notificationsEnabled: notificationsEnabled, + notifyOnCompletion: notifyOnCompletion, + notifyOnErrors: notifyOnErrors, + notifyOnConflicts: notifyOnConflicts, + folder: defaults.data(forKey: Key.folderBookmark).map { + SyncFolderReference( + bookmark: $0, + displayPath: defaults.string(forKey: Key.folderPath) ?? "", + machineID: machineID + ) + }, + lastSyncAt: lastSyncAt + ) + } + + /// Adopts a configuration read from per-machine app settings. + /// + /// The folder is only ever *added*, never cleared. A stored configuration + /// with no folder means one had not been chosen when it was written, or that + /// it belonged to another machine and was dropped on the way in — neither is + /// evidence the user unset anything, and there is no UI to unset a folder, + /// only to change it. Clearing would throw away a folder chosen while this + /// machine was offline. + func apply(_ configuration: SyncAgentConfiguration) { + isApplyingRemote = true + defer { isApplyingRemote = false } + + pollIntervalSeconds = configuration.pollIntervalSeconds + syncEnabled = configuration.syncEnabled + launchAtLogin = configuration.launchAtLogin + notificationsEnabled = configuration.notificationsEnabled + notifyOnCompletion = configuration.notifyOnCompletion + notifyOnErrors = configuration.notifyOnErrors + notifyOnConflicts = configuration.notifyOnConflicts + lastSyncAt = configuration.lastSyncAt ?? lastSyncAt + + if let folder = configuration.folder { + defaults.set(folder.bookmark, forKey: Key.folderBookmark) + defaults.set(folder.displayPath, forKey: Key.folderPath) + syncFolderPath = folder.displayPath + } + } + + private func write(_ configuration: SyncAgentConfiguration, to remote: RemoteConfigurationStore) async { + do { + try await remote.save(configuration) + defaults.set(true, forKey: Key.didMigrateToAppSettings) + } catch { + // Deliberately silent. The agent has no window open most of the + // time, and a failed settings write costs the user nothing: the + // local mirror is still authoritative until a write is confirmed, + // and the next change tries again. + logger.error("Per-machine settings write failed: \(error.localizedDescription, privacy: .public)") + } + } + + private func scheduleRemoteSave() { + guard !isApplyingRemote, let remote else { return } + pendingSave?.cancel() + pendingSave = Task { [weak self] in + guard let self else { return } + try? await Task.sleep(for: self.remoteSaveDebounce) + guard !Task.isCancelled, let machineID = await remote.currentDeviceID else { return } + await self.write(self.configuration(machineID: machineID), to: remote) + } } // MARK: - Sync folder @@ -84,6 +268,10 @@ public final class PreferencesManager: ObservableObject { } defaults.set(url.path, forKey: Key.folderPath) syncFolderPath = url.path + // Not a `@Published` setter, so it has no `didSet` to ride on — and a + // folder change is the single most valuable thing to get stored + // remotely, since it is what a reinstall needs back. + scheduleRemoteSave() } /// Resolves the stored bookmark and begins accessing it. Returns the URL, or diff --git a/SyncAgent/Sources/InterlinedListSyncCore/Storage/RemoteConfigurationStore.swift b/SyncAgent/Sources/InterlinedListSyncCore/Storage/RemoteConfigurationStore.swift new file mode 100644 index 0000000..ef71189 --- /dev/null +++ b/SyncAgent/Sources/InterlinedListSyncCore/Storage/RemoteConfigurationStore.swift @@ -0,0 +1,223 @@ +import Foundation +import os + +/// What a read of this machine's per-machine settings found. +/// +/// The three cases are not interchangeable, and conflating two of them is the +/// bug GitHub issue #104 warns about. "Nothing stored" invites a one-time +/// migration; "could not ask" must never invite anything, because the document +/// on the server may be newer than the local values and re-uploading them would +/// destroy it. +public enum RemoteConfigurationState: Sendable, Equatable { + /// The server holds a configuration written by this agent. + case stored(SyncAgentConfiguration) + /// The server holds no configuration for this machine — a fresh install. + case absent + /// The question could not be asked: no device id published yet, no token, + /// or the request failed. + case unavailable +} + +/// Which configuration wins at launch, and whether the one-way migration should +/// run. +public struct ConfigurationResolution: Sendable, Equatable { + public let configuration: SyncAgentConfiguration + public let shouldMigrate: Bool + + public init(configuration: SyncAgentConfiguration, shouldMigrate: Bool) { + self.configuration = configuration + self.shouldMigrate = shouldMigrate + } +} + +/// The launch-time decision, extracted as a pure function. +/// +/// It is four lines of logic guarding the one thing in this feature that can +/// lose a user's settings for good, so it is worth being able to test without a +/// network, a Keychain, or a clock. +public enum ConfigurationResolver { + + /// - Parameters: + /// - remote: what the server said. + /// - local: the `UserDefaults` configuration this machine has been using. + /// - hasMigrated: whether a migration write has already been *confirmed*. + public static func resolve( + remote: RemoteConfigurationState, + local: SyncAgentConfiguration, + hasMigrated: Bool + ) -> ConfigurationResolution { + switch remote { + case .stored(let configuration): + // Once a document exists it is authoritative, full stop. It may have + // been written minutes ago by this same machine, or by a user who + // reinstalled and expects their folder back. Merging it with local + // values would mean choosing a winner field by field with no + // timestamps to choose by. + return ConfigurationResolution(configuration: configuration, shouldMigrate: false) + + case .absent where !hasMigrated: + // The one moment migration is safe: the server has nothing, so + // nothing can be overwritten. + return ConfigurationResolution(configuration: local, shouldMigrate: true) + + case .absent: + // Already migrated once and the document is gone — the machine was + // deregistered, or the settings were deleted deliberately. Writing + // the local values back would resurrect a configuration the user + // removed. The agent keeps running on them locally; the next change + // the user makes will store them again as a deliberate act. + return ConfigurationResolution(configuration: local, shouldMigrate: false) + + case .unavailable: + // Offline, signed out, or not yet addressable. Carry on locally and + // decide nothing — the server's answer is simply not in evidence. + return ConfigurationResolution(configuration: local, shouldMigrate: false) + } + } +} + +/// Reads and writes the agent's configuration in per-machine app settings +/// (`…/devices/{deviceId}/settings`, GitHub issue #104). +/// +/// **Per-machine, never shared.** The account-wide document seeds a brand-new +/// machine on first sign-in, so a sync-folder path stored there would arrive +/// pre-filled on a Mac where it means nothing — the exact failure this feature +/// exists to prevent. +/// +/// An actor because the agent writes from wherever a preference changed and from +/// the sync loop's completion handler, and the cached `baseVersion` must not be +/// read by one of those while the other is replacing it. +public actor RemoteConfigurationStore { + + private let api: any DeviceSettingsAPI + private let identity: any DeviceIdentifying + /// Name to register this machine under if it turns out not to be in the + /// registry. Hostname-derived, matching what the main app would use. + private let deviceName: String + private let logger = Logger(subsystem: SyncConfiguration.logSubsystem, category: "RemoteConfig") + + /// The document as last seen, so a write can overlay rather than replace and + /// can send a `baseVersion` the server will accept. + private var cachedPayload: [String: SettingsValue] = [:] + private var cachedVersion = 0 + private var hasReadDocument = false + + /// Attempts a single `save` will make before giving up. + /// + /// Bounded on purpose. A 409 is re-based and retried rather than surfaced — + /// the agent runs in the background, where there is no one to show a failure + /// toast to — but an unbounded retry against a document another process is + /// rewriting in a loop would turn a lost race into a request storm. + private static let maxSaveAttempts = 3 + + public init( + api: any DeviceSettingsAPI, + identity: any DeviceIdentifying, + deviceName: String + ) { + self.api = api + self.identity = identity + self.deviceName = deviceName + } + + /// This machine's device id, or nil when the main app has not published one + /// into the shared Keychain group yet — in which case there is no + /// per-machine document to address and the agent stays on local settings. + public var currentDeviceID: String? { identity.currentDeviceID() } + + // MARK: - Reading + + public func load() async -> RemoteConfigurationState { + guard let deviceID = identity.currentDeviceID() else { + // The main app mints and publishes the id; the agent only consumes + // it (see `SharedDeviceIdentity`). Until it appears there is no + // document to address, which is "cannot ask", not "nothing stored". + logger.info("No device id published yet; per-machine settings unavailable") + return .unavailable + } + do { + guard let document = try await api.fetchDeviceSettings(deviceId: deviceID) else { + cachedPayload = [:] + cachedVersion = 0 + hasReadDocument = true + return .absent + } + cache(document) + guard let configuration = SyncAgentConfiguration( + settings: document.settings, + thisMachineID: deviceID + ) else { + // A document exists but holds none of this agent's keys — the + // main app stored per-machine settings of its own. Absent for + // our purposes, and the cached payload means the migration write + // will preserve those keys. + return .absent + } + return .stored(configuration) + } catch { + logger.error("Per-machine settings read failed: \(error.localizedDescription, privacy: .public)") + return .unavailable + } + } + + // MARK: - Writing + + /// Stores `configuration`, re-basing and retrying if the compare-and-set + /// write loses. + /// + /// Throws only when the write genuinely cannot be completed; the caller + /// treats that as "stay on local settings" rather than as an error to show. + public func save(_ configuration: SyncAgentConfiguration) async throws { + guard let deviceID = identity.currentDeviceID() else { throw APIError.noDeviceIdentity } + + // Read before the first write so the overlay has the real document to + // preserve. Skipping this and writing with `baseVersion: 0` would 409 + // anyway — but only after the payload had already been built from + // nothing, which is how the main app's keys would get dropped. + if !hasReadDocument { + _ = await load() + } + + var didRegister = false + var attempt = 0 + while true { + attempt += 1 + do { + let document = try await api.writeDeviceSettings( + deviceId: deviceID, + settings: configuration.apply(to: cachedPayload), + baseVersion: cachedVersion + ) + cache(document) + return + } catch APIError.versionConflict(let current) { + guard attempt < Self.maxSaveAttempts else { throw APIError.versionConflict(current: current) } + if let current { + // The 409 body carries the winning document, so re-basing + // costs nothing. This is why the agent's client keeps typed + // error payloads where the main app's does not. + cache(current) + } else if let refreshed = try? await api.fetchDeviceSettings(deviceId: deviceID) { + cache(refreshed) + } else { + throw APIError.versionConflict(current: nil) + } + } catch APIError.deviceNotRegistered { + // Registering is safe *here specifically*: the 404 proves the id + // is absent from the registry, so the upsert has no existing + // `deviceName` to clobber. Once only — a second 404 after a + // successful register is a server-side problem retrying cannot + // solve. + guard !didRegister else { throw APIError.deviceNotRegistered } + didRegister = true + try await api.registerDevice(deviceId: deviceID, deviceName: deviceName) + } + } + } + + private func cache(_ document: DeviceSettingsDocument) { + cachedPayload = document.settings + cachedVersion = document.version + hasReadDocument = true + } +} diff --git a/SyncAgent/Sources/InterlinedListSyncCore/Storage/SyncAgentConfiguration.swift b/SyncAgent/Sources/InterlinedListSyncCore/Storage/SyncAgentConfiguration.swift new file mode 100644 index 0000000..5c0d669 --- /dev/null +++ b/SyncAgent/Sources/InterlinedListSyncCore/Storage/SyncAgentConfiguration.swift @@ -0,0 +1,223 @@ +import Foundation + +/// The keys the agent's configuration occupies inside a per-machine app-settings +/// document (GitHub issue #104). +/// +/// **This list is a wire contract with the main app.** `InterlinedDomain`'s +/// `DocumentSyncStatus` reads the same keys out of the same document so +/// Settings ▸ Applications can report what a machine is doing. The two are +/// independent codebases — this package deliberately does not depend on +/// `InterlinedKit`/`InterlinedDomain` — so a rename here without a matching +/// rename there silently stops the pane reporting anything. Change both. +/// +/// Every key is prefixed. The document is **shared** with the main app's own +/// per-machine settings, and the PUT replaces it wholesale, so an unprefixed +/// `enabled` would be one careless commit away from meaning two things. +public enum DocumentSyncSettingsKeys { + /// Prefix identifying keys this agent owns. Everything else in the document + /// belongs to someone else and must survive our writes untouched. + public static let prefix = "documentSync." + + public static let schemaVersion = prefix + "schemaVersion" + public static let enabled = prefix + "enabled" + public static let pollIntervalSeconds = prefix + "pollIntervalSeconds" + public static let launchAtLogin = prefix + "launchAtLogin" + public static let notificationsEnabled = prefix + "notificationsEnabled" + public static let notifyOnCompletion = prefix + "notifyOnCompletion" + public static let notifyOnErrors = prefix + "notifyOnErrors" + public static let notifyOnConflicts = prefix + "notifyOnConflicts" + /// Human-readable path of the sync folder. **Display only** — a sandboxed + /// process cannot reopen a user-chosen folder from a path string. + public static let folderPath = prefix + "folderPath" + /// Base64 of the security-scoped bookmark. The actual key to the folder. + public static let folderBookmark = prefix + "folderBookmark" + /// The device id of the Mac that created the bookmark. + public static let folderMachineID = prefix + "folderMachineId" + /// ISO-8601 timestamp of the last completed sync cycle. Status, not + /// configuration — see ``SyncConfiguration/lastSyncPublishInterval``. + public static let lastSyncAt = prefix + "lastSyncAt" +} + +// MARK: - The sync folder + +/// A chosen sync folder: the bookmark that actually opens it, the path to show +/// a human, and the machine both belong to. +/// +/// The machine stamp is not redundant. Per-machine settings are *meant* to stay +/// on one Mac, but two documented paths move a document across machines anyway: +/// Settings ▸ Applications can copy a machine's settings to the account-wide +/// document, and a brand-new machine's first sign-in seeds from the main +/// workstation. Either one can hand this Mac a bookmark another Mac created. +/// +/// Resolving a foreign bookmark does not fail cleanly — at best it resolves to a +/// path that does not exist here, at worst to an unrelated folder with the same +/// name, which the agent would then start writing documents into. So a foreign +/// reference is dropped on read and the machine reports itself unconfigured, +/// which is both true and recoverable: the user picks a folder. +public struct SyncFolderReference: Sendable, Equatable { + /// Security-scoped bookmark data. Only meaningful on ``machineID``. + public let bookmark: Data + /// Where the folder lives, for display. Never used to open anything. + public let displayPath: String + /// The device id of the Mac this bookmark was created on. + public let machineID: String + + public init(bookmark: Data, displayPath: String, machineID: String) { + self.bookmark = bookmark + self.displayPath = displayPath + self.machineID = machineID + } +} + +// MARK: - The configuration + +/// Everything the Document Sync Agent needs to know about itself, in the shape +/// it is stored in per-machine app settings. +/// +/// This is the complete set of state the agent used to keep in `UserDefaults`, +/// enumerated from `PreferencesManager` rather than guessed. Two things named in +/// GitHub issue #104 are deliberately **not** here: +/// +/// - **The sync ledger's cursor.** `LedgerData.lastSyncAt` is the delta cursor, +/// and it is only meaningful next to the per-document entries stored in the +/// same file. Uploading the cursor alone would let a machine resume from a +/// cursor its ledger cannot account for, and conclude nothing had changed. +/// ``lastSyncAt`` here is a *report* of when the last cycle finished, read by +/// Settings ▸ Applications and never fed back into the engine. +/// - **A conflict policy.** There is not one to move: ``ConflictResolver`` is a +/// fixed remote-wins decision table with no user-facing setting. Inventing a +/// stored policy to satisfy the migration would create a setting nothing +/// reads. +public struct SyncAgentConfiguration: Sendable, Equatable { + + public var syncEnabled: Bool + public var pollIntervalSeconds: TimeInterval + public var launchAtLogin: Bool + public var notificationsEnabled: Bool + public var notifyOnCompletion: Bool + public var notifyOnErrors: Bool + public var notifyOnConflicts: Bool + /// The chosen folder, or nil when this machine has none. + public var folder: SyncFolderReference? + /// When the last sync cycle finished, for reporting only. + public var lastSyncAt: Date? + + public init( + syncEnabled: Bool = true, + pollIntervalSeconds: TimeInterval = SyncConfiguration.defaultPollInterval, + launchAtLogin: Bool = false, + notificationsEnabled: Bool = true, + notifyOnCompletion: Bool = false, + notifyOnErrors: Bool = true, + notifyOnConflicts: Bool = true, + folder: SyncFolderReference? = nil, + lastSyncAt: Date? = nil + ) { + self.syncEnabled = syncEnabled + self.pollIntervalSeconds = pollIntervalSeconds + self.launchAtLogin = launchAtLogin + self.notificationsEnabled = notificationsEnabled + self.notifyOnCompletion = notifyOnCompletion + self.notifyOnErrors = notifyOnErrors + self.notifyOnConflicts = notifyOnConflicts + self.folder = folder + self.lastSyncAt = lastSyncAt + } + + // MARK: - Reading a stored document + + /// Projects the agent's configuration out of a settings payload. + /// + /// Returns nil when the payload holds none of this agent's keys — the main + /// app may well have written per-machine settings of its own, but a document + /// without a `documentSync.` key has never been written by the agent, and + /// treating it as a stored-but-default configuration would suppress the + /// migration that is supposed to run exactly once. + /// + /// - Parameter thisMachineID: the device id of the Mac doing the reading. + /// A folder stamped with any other machine is dropped; see + /// ``SyncFolderReference``. + public init?(settings: [String: SettingsValue], thisMachineID: String) { + let keys = DocumentSyncSettingsKeys.self + guard settings.keys.contains(where: { $0.hasPrefix(keys.prefix) }) else { return nil } + + let defaults = SyncAgentConfiguration() + self.init( + syncEnabled: settings[keys.enabled]?.boolValue ?? defaults.syncEnabled, + // Clamped on read, not just on write: a value stored by a build with + // different bounds must not make this one poll every second. + pollIntervalSeconds: (settings[keys.pollIntervalSeconds]?.doubleValue) + .map { min(max($0, SyncConfiguration.minPollInterval), SyncConfiguration.maxPollInterval) } + ?? defaults.pollIntervalSeconds, + launchAtLogin: settings[keys.launchAtLogin]?.boolValue ?? defaults.launchAtLogin, + notificationsEnabled: settings[keys.notificationsEnabled]?.boolValue ?? defaults.notificationsEnabled, + notifyOnCompletion: settings[keys.notifyOnCompletion]?.boolValue ?? defaults.notifyOnCompletion, + notifyOnErrors: settings[keys.notifyOnErrors]?.boolValue ?? defaults.notifyOnErrors, + notifyOnConflicts: settings[keys.notifyOnConflicts]?.boolValue ?? defaults.notifyOnConflicts, + folder: Self.folder(from: settings, thisMachineID: thisMachineID), + lastSyncAt: settings[keys.lastSyncAt]?.stringValue.flatMap(JSONCoding.parseISO8601) + ) + } + + private static func folder( + from settings: [String: SettingsValue], + thisMachineID: String + ) -> SyncFolderReference? { + let keys = DocumentSyncSettingsKeys.self + guard let encoded = settings[keys.folderBookmark]?.stringValue, + let bookmark = Data(base64Encoded: encoded), + !bookmark.isEmpty + else { return nil } + + // An unstamped bookmark is treated as foreign too. Nothing has ever + // written one, so the only way to see one is a document this agent did + // not produce — and "cannot prove it is ours" has to read as "not ours", + // because the failure it prevents is syncing into the wrong folder. + guard let machineID = settings[keys.folderMachineID]?.stringValue, + machineID == thisMachineID + else { return nil } + + return SyncFolderReference( + bookmark: bookmark, + displayPath: settings[keys.folderPath]?.stringValue ?? "", + machineID: machineID + ) + } + + // MARK: - Writing a stored document + + /// Overlays this configuration onto an existing settings payload. + /// + /// The PUT **replaces** the document rather than merging it (verified live: + /// a write carrying only `theme` deleted a stored `sidebarWidth`), so the + /// payload handed to it has to be the whole document. Keys outside the + /// `documentSync.` namespace are copied through untouched — they belong to + /// the main app, or to a newer build of this one, and dropping them would + /// make every agent write silently delete settings it never owned. + /// + /// Keys inside the namespace are rebuilt from scratch, so unsetting a folder + /// removes its keys instead of leaving a stale bookmark behind. + public func apply(to existing: [String: SettingsValue]) -> [String: SettingsValue] { + let keys = DocumentSyncSettingsKeys.self + var payload = existing.filter { !$0.key.hasPrefix(keys.prefix) } + + payload[keys.schemaVersion] = .number(Double(SyncConfiguration.settingsSchemaVersion)) + payload[keys.enabled] = .bool(syncEnabled) + payload[keys.pollIntervalSeconds] = .number(pollIntervalSeconds) + payload[keys.launchAtLogin] = .bool(launchAtLogin) + payload[keys.notificationsEnabled] = .bool(notificationsEnabled) + payload[keys.notifyOnCompletion] = .bool(notifyOnCompletion) + payload[keys.notifyOnErrors] = .bool(notifyOnErrors) + payload[keys.notifyOnConflicts] = .bool(notifyOnConflicts) + + if let folder { + payload[keys.folderBookmark] = .string(folder.bookmark.base64EncodedString()) + payload[keys.folderPath] = .string(folder.displayPath) + payload[keys.folderMachineID] = .string(folder.machineID) + } + if let lastSyncAt { + payload[keys.lastSyncAt] = .string(JSONCoding.iso8601String(lastSyncAt)) + } + return payload + } +} diff --git a/SyncAgent/Tests/InterlinedListSyncCoreTests/FakeDeviceSettingsAPI.swift b/SyncAgent/Tests/InterlinedListSyncCoreTests/FakeDeviceSettingsAPI.swift new file mode 100644 index 0000000..588da1d --- /dev/null +++ b/SyncAgent/Tests/InterlinedListSyncCoreTests/FakeDeviceSettingsAPI.swift @@ -0,0 +1,81 @@ +import Foundation +@testable import InterlinedListSyncCore + +/// In-memory stand-in for the per-machine app-settings routes (GitHub issue +/// #104), with the server's real compare-and-set semantics: a write carrying a +/// `baseVersion` other than the stored one is rejected with the winning document +/// attached, exactly as the live `409 version_conflict` body does. +/// +/// Modelling the conflict rather than stubbing it matters here — the whole point +/// of the store's retry is that it re-bases onto a document it did not write. +actor FakeDeviceSettingsAPI: DeviceSettingsAPI { + + struct WriteRecord: Sendable, Equatable { + let deviceId: String + let settings: [String: SettingsValue] + let baseVersion: Int + } + + private(set) var reads: [String] = [] + private(set) var writes: [WriteRecord] = [] + private(set) var registrations: [String] = [] + + private var document: DeviceSettingsDocument? + private var readError: APIError? + /// Errors to throw from the next writes, consumed one per attempt. + private var writeFailures: [APIError] = [] + + // MARK: - Test control + + func seed(settings: [String: SettingsValue], version: Int, deviceId: String = "mac-1") { + document = DeviceSettingsDocument( + version: version, + updatedAt: Date(timeIntervalSince1970: 1_700_000_000), + deviceId: deviceId, + schemaVersion: 1, + settings: settings + ) + } + + func failReads(with error: APIError) { readError = error } + func failNextWrites(with errors: [APIError]) { writeFailures = errors } + + func storedSettings() -> [String: SettingsValue] { document?.settings ?? [:] } + func storedVersion() -> Int { document?.version ?? 0 } + + // MARK: - DeviceSettingsAPI + + func fetchDeviceSettings(deviceId: String) async throws -> DeviceSettingsDocument? { + reads.append(deviceId) + if let readError { throw readError } + return document + } + + func writeDeviceSettings( + deviceId: String, + settings: [String: SettingsValue], + baseVersion: Int + ) async throws -> DeviceSettingsDocument { + writes.append(WriteRecord(deviceId: deviceId, settings: settings, baseVersion: baseVersion)) + if !writeFailures.isEmpty { + throw writeFailures.removeFirst() + } + let currentVersion = document?.version ?? 0 + guard baseVersion == currentVersion else { + throw APIError.versionConflict(current: document) + } + let updated = DeviceSettingsDocument( + version: currentVersion + 1, + updatedAt: Date(timeIntervalSince1970: 1_700_000_100), + deviceId: deviceId, + schemaVersion: 1, + settings: settings + ) + document = updated + return updated + } + + func registerDevice(deviceId: String, deviceName: String) async throws { + registrations.append(deviceId) + } +} diff --git a/SyncAgent/Tests/InterlinedListSyncCoreTests/PreferencesManagerTests.swift b/SyncAgent/Tests/InterlinedListSyncCoreTests/PreferencesManagerTests.swift index 4630314..3bf8ae6 100644 --- a/SyncAgent/Tests/InterlinedListSyncCoreTests/PreferencesManagerTests.swift +++ b/SyncAgent/Tests/InterlinedListSyncCoreTests/PreferencesManagerTests.swift @@ -4,11 +4,30 @@ import XCTest @MainActor final class PreferencesManagerTests: XCTestCase { + private let deviceID = "mac-1" + private func freshDefaults() -> UserDefaults { let suite = "iltest-prefs-\(UUID().uuidString)" return UserDefaults(suiteName: suite)! } + /// Debounced saves are parked far enough out that no test observes one. The + /// writes these tests assert on are the explicit ones `synchronize()` makes; + /// a stray coalesced write would make the counts meaningless. + private func makeManager( + defaults: UserDefaults, + api: FakeDeviceSettingsAPI, + identity: any DeviceIdentifying + ) -> PreferencesManager { + PreferencesManager( + defaults: defaults, + remote: RemoteConfigurationStore(api: api, identity: identity, deviceName: "Studio Mac"), + remoteSaveDebounce: .seconds(3600) + ) + } + + // MARK: - Local behaviour (unchanged by the move to app settings) + func test_defaults_areSensible() { let prefs = PreferencesManager(defaults: freshDefaults()) XCTAssertEqual(prefs.pollIntervalSeconds, SyncConfiguration.defaultPollInterval) @@ -36,4 +55,222 @@ final class PreferencesManagerTests: XCTestCase { XCTAssertTrue(reloaded.notifyOnCompletion) XCTAssertTrue(reloaded.launchAtLogin) } + + // MARK: - Happy path: adopting stored per-machine settings + + func test_givenStoredSettingsForThisMac_whenSynchronizing_thenTheyReplaceTheLocalOnes() async { + let defaults = freshDefaults() + let api = FakeDeviceSettingsAPI() + let stored = SyncAgentConfiguration( + syncEnabled: false, + pollIntervalSeconds: 300, + notifyOnCompletion: true, + folder: SyncFolderReference( + bookmark: Data("bm".utf8), + displayPath: "/Users/someone/Vault", + machineID: deviceID + ) + ) + await api.seed(settings: stored.apply(to: [:]), version: 2) + let prefs = makeManager(defaults: defaults, api: api, identity: StaticDeviceIdentity(deviceID)) + + await prefs.synchronize() + + XCTAssertEqual(prefs.pollIntervalSeconds, 300) + XCTAssertFalse(prefs.syncEnabled) + XCTAssertTrue(prefs.notifyOnCompletion) + // The bookmark reaches local storage, which is the only place the agent + // ever resolves it from — so a reinstall recovers the folder. + XCTAssertTrue(prefs.hasSyncFolder) + XCTAssertEqual(prefs.syncFolderPath, "/Users/someone/Vault") + } + + // MARK: - Migration + + func test_givenLocalSettingsAndAnEmptyServer_whenSynchronizing_thenTheyMigrateUpAndAreMarkedDone() async { + let defaults = freshDefaults() + let api = FakeDeviceSettingsAPI() + let prefs = makeManager(defaults: defaults, api: api, identity: StaticDeviceIdentity(deviceID)) + prefs.pollIntervalSeconds = 180 + prefs.notifyOnErrors = false + + await prefs.synchronize() + + let stored = await api.storedSettings() + XCTAssertEqual(stored[DocumentSyncSettingsKeys.pollIntervalSeconds]?.doubleValue, 180) + XCTAssertEqual(stored[DocumentSyncSettingsKeys.notifyOnErrors]?.boolValue, false) + XCTAssertTrue(prefs.hasMigratedToAppSettings) + } + + func test_givenAFreshInstallWithNoLocalSettings_whenSynchronizing_thenTheDefaultsAreStoredOnce() async { + // The boundary GitHub issue #104 calls out: `UserDefaults` is empty, so + // there is nothing to migrate but the defaults themselves — and the + // migration must still be recorded, or it would run again every launch. + let defaults = freshDefaults() + let api = FakeDeviceSettingsAPI() + let prefs = makeManager(defaults: defaults, api: api, identity: StaticDeviceIdentity(deviceID)) + + await prefs.synchronize() + + let writes = await api.writes + XCTAssertEqual(writes.count, 1) + let stored = await api.storedSettings() + XCTAssertEqual( + stored[DocumentSyncSettingsKeys.pollIntervalSeconds]?.doubleValue, + SyncConfiguration.defaultPollInterval + ) + XCTAssertTrue(prefs.hasMigratedToAppSettings) + } + + func test_givenTheMigrationAlreadyRan_whenSynchronizingAgain_thenNothingIsWrittenBack() async { + let defaults = freshDefaults() + let api = FakeDeviceSettingsAPI() + let prefs = makeManager(defaults: defaults, api: api, identity: StaticDeviceIdentity(deviceID)) + await prefs.synchronize() + let writesAfterMigration = await api.writes.count + + // A second launch with the document now stored. It must read, adopt, and + // stop — a migration that re-ran every launch would be worse than none. + await prefs.synchronize() + + let writesAfterSecondLaunch = await api.writes.count + XCTAssertEqual(writesAfterSecondLaunch, writesAfterMigration) + } + + // MARK: - Upstream failure + + func test_givenTheServerCannotBeReached_whenSynchronizing_thenLocalSettingsStandAndNothingIsMigrated() async { + let defaults = freshDefaults() + let api = FakeDeviceSettingsAPI() + await api.failReads(with: .transport("offline")) + let prefs = makeManager(defaults: defaults, api: api, identity: StaticDeviceIdentity(deviceID)) + prefs.pollIntervalSeconds = 240 + + await prefs.synchronize() + + XCTAssertEqual(prefs.pollIntervalSeconds, 240) + let writes = await api.writes + XCTAssertTrue(writes.isEmpty, "A failed read must never be read as an empty server") + XCTAssertFalse(prefs.hasMigratedToAppSettings) + } + + func test_givenTheMigrationWriteFails_whenSynchronizing_thenItIsNotMarkedDone() async { + let defaults = freshDefaults() + let api = FakeDeviceSettingsAPI() + await api.failNextWrites(with: [.transport("offline"), .transport("offline"), .transport("offline")]) + let prefs = makeManager(defaults: defaults, api: api, identity: StaticDeviceIdentity(deviceID)) + + await prefs.synchronize() + + // Unconfirmed means unmigrated: `UserDefaults` stays the fallback and + // the next launch tries again. + XCTAssertFalse(prefs.hasMigratedToAppSettings) + } + + // MARK: - Boundary: a folder that belongs to another Mac + + func test_givenTheStoredFolderBelongsToAnotherMac_whenSynchronizing_thenThisMacIsNotConfigured() async { + let defaults = freshDefaults() + let api = FakeDeviceSettingsAPI() + let fromAnotherMac = SyncAgentConfiguration( + pollIntervalSeconds: 300, + folder: SyncFolderReference( + bookmark: Data("bm".utf8), + displayPath: "/Volumes/OtherMac/Vault", + machineID: "mac-2" + ) + ) + await api.seed(settings: fromAnotherMac.apply(to: [:]), version: 1) + let prefs = makeManager(defaults: defaults, api: api, identity: StaticDeviceIdentity(deviceID)) + + await prefs.synchronize() + + // The rest of the configuration still applies; only the folder is + // refused, and the agent will prompt for one. + XCTAssertEqual(prefs.pollIntervalSeconds, 300) + XCTAssertFalse(prefs.hasSyncFolder) + XCTAssertNil(prefs.syncFolderPath) + } + + func test_givenTheServerHasNoFolderButThisMacDoes_whenSynchronizing_thenTheLocalFolderSurvives() async { + // A folder chosen while offline. The stored configuration predates it, + // and there is no UI for unsetting a folder — so "no folder stored" is + // never evidence the user cleared one. + let defaults = freshDefaults() + defaults.set(Data("local-bm".utf8), forKey: "syncFolderBookmark") + defaults.set("/Users/someone/Local", forKey: "syncFolderPath") + let api = FakeDeviceSettingsAPI() + await api.seed( + settings: SyncAgentConfiguration(pollIntervalSeconds: 120).apply(to: [:]), + version: 1 + ) + let prefs = makeManager(defaults: defaults, api: api, identity: StaticDeviceIdentity(deviceID)) + + await prefs.synchronize() + + XCTAssertTrue(prefs.hasSyncFolder) + XCTAssertEqual(prefs.syncFolderPath, "/Users/someone/Local") + // …and the server is brought up to date rather than left disagreeing. + let stored = await api.storedSettings() + XCTAssertEqual(stored[DocumentSyncSettingsKeys.folderMachineID]?.stringValue, deviceID) + } + + // MARK: - Boundary: no device id published yet + + func test_givenTheMainAppHasNotPublishedADeviceID_whenSynchronizing_thenNothingIsReadOrWritten() async { + let defaults = freshDefaults() + let api = FakeDeviceSettingsAPI() + let prefs = makeManager(defaults: defaults, api: api, identity: StaticDeviceIdentity(nil)) + + await prefs.synchronize() + + let reads = await api.reads + let writes = await api.writes + XCTAssertTrue(reads.isEmpty) + XCTAssertTrue(writes.isEmpty) + XCTAssertFalse(prefs.hasMigratedToAppSettings) + } + + // MARK: - Reported status + + func test_givenARecentlyPublishedSync_whenAnotherCycleCompletes_thenItIsNotPublishedAgain() async { + let defaults = freshDefaults() + let api = FakeDeviceSettingsAPI() + let prefs = makeManager(defaults: defaults, api: api, identity: StaticDeviceIdentity(deviceID)) + await prefs.synchronize() + let writesAfterMigration = await api.writes.count + + let now = Date() + // Awaited rather than slept on: the publish is a task the manager hands + // back precisely so this assertion needs no timer. + await prefs.recordSync(at: now)?.value + let inWindow = prefs.recordSync(at: now.addingTimeInterval(60)) + let alsoInWindow = prefs.recordSync(at: now.addingTimeInterval(120)) + + // The first crosses the throttle (nothing published yet); the rest fall + // inside the window. A settings PUT per poll cycle is what this prevents. + XCTAssertNil(inWindow) + XCTAssertNil(alsoInWindow) + let writesAfterReports = await api.writes.count + XCTAssertEqual(writesAfterReports, writesAfterMigration + 1) + } + + func test_givenTheThrottleWindowHasPassed_whenACycleCompletes_thenItIsPublishedAgain() async { + let defaults = freshDefaults() + let api = FakeDeviceSettingsAPI() + let prefs = makeManager(defaults: defaults, api: api, identity: StaticDeviceIdentity(deviceID)) + await prefs.synchronize() + let writesAfterMigration = await api.writes.count + + let now = Date() + await prefs.recordSync(at: now)?.value + await prefs.recordSync( + at: now.addingTimeInterval(SyncConfiguration.lastSyncPublishInterval + 1) + )?.value + + let writesAfterReports = await api.writes.count + XCTAssertEqual(writesAfterReports, writesAfterMigration + 2) + let stored = await api.storedSettings() + XCTAssertNotNil(stored[DocumentSyncSettingsKeys.lastSyncAt]?.stringValue) + } } diff --git a/SyncAgent/Tests/InterlinedListSyncCoreTests/RemoteConfigurationStoreTests.swift b/SyncAgent/Tests/InterlinedListSyncCoreTests/RemoteConfigurationStoreTests.swift new file mode 100644 index 0000000..8a5f4d5 --- /dev/null +++ b/SyncAgent/Tests/InterlinedListSyncCoreTests/RemoteConfigurationStoreTests.swift @@ -0,0 +1,312 @@ +import XCTest +@testable import InterlinedListSyncCore + +/// The read/write half of the move to per-machine app settings (GitHub issue +/// #104): what a read means, when a migration is allowed, and what the agent +/// does when its background write loses the compare-and-set race. +final class RemoteConfigurationStoreTests: XCTestCase { + + private let deviceID = "mac-1" + + private func makeStore( + api: FakeDeviceSettingsAPI, + identity: any DeviceIdentifying + ) -> RemoteConfigurationStore { + RemoteConfigurationStore(api: api, identity: identity, deviceName: "Studio Mac") + } + + private func sampleConfiguration(machineID: String? = nil) -> SyncAgentConfiguration { + SyncAgentConfiguration( + syncEnabled: true, + pollIntervalSeconds: 90, + folder: machineID.map { + SyncFolderReference( + bookmark: Data("bm".utf8), + displayPath: "/Users/someone/Vault", + machineID: $0 + ) + } + ) + } + + // MARK: - Happy path + + func test_givenNothingStored_whenSaving_thenItCreatesTheDocumentAtBaseVersionZero() async throws { + let api = FakeDeviceSettingsAPI() + let store = makeStore(api: api, identity: StaticDeviceIdentity(deviceID)) + + try await store.save(sampleConfiguration(machineID: deviceID)) + + let writes = await api.writes + XCTAssertEqual(writes.count, 1) + XCTAssertEqual(writes.first?.baseVersion, 0) + XCTAssertEqual(writes.first?.deviceId, deviceID) + let stored = await api.storedSettings() + XCTAssertEqual(stored[DocumentSyncSettingsKeys.pollIntervalSeconds]?.doubleValue, 90) + } + + func test_givenAStoredConfiguration_whenLoading_thenItIsReturned() async throws { + let api = FakeDeviceSettingsAPI() + await api.seed( + settings: sampleConfiguration(machineID: deviceID).apply(to: [:]), + version: 4 + ) + let store = makeStore(api: api, identity: StaticDeviceIdentity(deviceID)) + + let state = await store.load() + + guard case .stored(let configuration) = state else { + return XCTFail("Expected a stored configuration, got \(state)") + } + XCTAssertEqual(configuration.pollIntervalSeconds, 90) + XCTAssertEqual(configuration.folder?.displayPath, "/Users/someone/Vault") + } + + func test_givenAStoredConfiguration_whenSaving_thenItWritesAgainstTheVersionItRead() async throws { + let api = FakeDeviceSettingsAPI() + await api.seed(settings: sampleConfiguration(machineID: deviceID).apply(to: [:]), version: 7) + let store = makeStore(api: api, identity: StaticDeviceIdentity(deviceID)) + + var updated = sampleConfiguration(machineID: deviceID) + updated.pollIntervalSeconds = 300 + try await store.save(updated) + + let writes = await api.writes + // No explicit load() first: the store reads before its first write so the + // overlay has the real document to preserve and a version to send. + XCTAssertEqual(writes.map(\.baseVersion), [7]) + let version = await api.storedVersion() + XCTAssertEqual(version, 8) + } + + // MARK: - Invalid input + + func test_givenNoPublishedDeviceID_whenLoading_thenItIsUnavailableAndNothingIsRequested() async { + let api = FakeDeviceSettingsAPI() + let store = makeStore(api: api, identity: StaticDeviceIdentity(nil)) + + let state = await store.load() + + XCTAssertEqual(state, .unavailable) + let reads = await api.reads + XCTAssertTrue(reads.isEmpty, "Nothing is addressable without a device id") + } + + func test_givenNoPublishedDeviceID_whenSaving_thenItThrowsWithoutCallingTheAPI() async { + let api = FakeDeviceSettingsAPI() + let store = makeStore(api: api, identity: StaticDeviceIdentity(nil)) + + do { + try await store.save(sampleConfiguration()) + XCTFail("Expected a failure") + } catch { + XCTAssertEqual(error as? APIError, .noDeviceIdentity) + } + let writes = await api.writes + XCTAssertTrue(writes.isEmpty) + } + + // MARK: - Upstream failure + + func test_givenTheReadFails_whenLoading_thenItIsUnavailableRatherThanAbsent() async { + let api = FakeDeviceSettingsAPI() + await api.failReads(with: .transport("offline")) + let store = makeStore(api: api, identity: StaticDeviceIdentity(deviceID)) + + let state = await store.load() + + // The distinction is the whole safety property: `.absent` would invite a + // migration that overwrites a server document this client never saw. + XCTAssertEqual(state, .unavailable) + } + + func test_givenTheWriteKeepsConflicting_whenSaving_thenItGivesUpRatherThanRetryingForever() async { + let api = FakeDeviceSettingsAPI() + await api.seed(settings: [:], version: 1) + await api.failNextWrites(with: [ + .versionConflict(current: nil), + .versionConflict(current: nil), + .versionConflict(current: nil) + ]) + let store = makeStore(api: api, identity: StaticDeviceIdentity(deviceID)) + + do { + try await store.save(sampleConfiguration(machineID: deviceID)) + XCTFail("Expected a failure") + } catch { + guard case APIError.versionConflict = error else { + return XCTFail("Expected a version conflict, got \(error)") + } + } + let writes = await api.writes + XCTAssertEqual(writes.count, 3, "Retries are bounded, not unlimited") + } + + // MARK: - Compare-and-set + + func test_givenTheDocumentMovedOn_whenSaving_thenItRebasesFromThe409AndSucceeds() async throws { + let api = FakeDeviceSettingsAPI() + let winner = DeviceSettingsDocument( + version: 9, + deviceId: deviceID, + settings: ["theme": .string("dark")] + ) + await api.seed(settings: ["theme": .string("dark")], version: 9) + // The store's first write is built on a stale version, exactly as a + // background write racing the Applications pane would be. + await api.failNextWrites(with: [.versionConflict(current: winner)]) + let store = makeStore(api: api, identity: StaticDeviceIdentity(deviceID)) + + try await store.save(sampleConfiguration(machineID: deviceID)) + + let writes = await api.writes + XCTAssertEqual(writes.count, 2) + XCTAssertEqual(writes.last?.baseVersion, 9, "Re-based onto the winning document") + // Re-basing must also adopt the winner's payload, or the retry would + // delete keys the other writer had just added. + XCTAssertEqual(writes.last?.settings["theme"]?.stringValue, "dark") + let reads = await api.reads + XCTAssertEqual(reads.count, 1, "The 409 body carried the winner; no extra read") + } + + func test_givenA409WithNoBody_whenSaving_thenItRereadsAndRetries() async throws { + let api = FakeDeviceSettingsAPI() + await api.seed(settings: ["theme": .string("dark")], version: 3) + await api.failNextWrites(with: [.versionConflict(current: nil)]) + let store = makeStore(api: api, identity: StaticDeviceIdentity(deviceID)) + + try await store.save(sampleConfiguration(machineID: deviceID)) + + let reads = await api.reads + XCTAssertEqual(reads.count, 2, "One read before the first write, one to re-base") + let writes = await api.writes + XCTAssertEqual(writes.last?.baseVersion, 3) + } + + func test_givenThisMacIsNotInTheRegistry_whenSaving_thenItRegistersOnceAndRetries() async throws { + let api = FakeDeviceSettingsAPI() + await api.failNextWrites(with: [.deviceNotRegistered]) + let store = makeStore(api: api, identity: StaticDeviceIdentity(deviceID)) + + try await store.save(sampleConfiguration(machineID: deviceID)) + + let registrations = await api.registrations + XCTAssertEqual(registrations, [deviceID]) + let writes = await api.writes + XCTAssertEqual(writes.count, 2) + } + + func test_givenRegistrationDoesNotHelp_whenSaving_thenItStopsInsteadOfLooping() async { + let api = FakeDeviceSettingsAPI() + await api.failNextWrites(with: [.deviceNotRegistered, .deviceNotRegistered]) + let store = makeStore(api: api, identity: StaticDeviceIdentity(deviceID)) + + do { + try await store.save(sampleConfiguration(machineID: deviceID)) + XCTFail("Expected a failure") + } catch { + XCTAssertEqual(error as? APIError, .deviceNotRegistered) + } + let registrations = await api.registrations + XCTAssertEqual(registrations.count, 1, "Registering twice would not change the answer") + } + + // MARK: - Boundary + + func test_givenNothingStored_whenLoading_thenItIsAbsent() async { + let api = FakeDeviceSettingsAPI() + let store = makeStore(api: api, identity: StaticDeviceIdentity(deviceID)) + + let state = await store.load() + + XCTAssertEqual(state, .absent) + } + + func test_givenOnlyTheMainAppsKeys_whenLoadingThenSaving_thenThoseKeysSurvive() async throws { + let api = FakeDeviceSettingsAPI() + await api.seed(settings: ["sidebarWidth": .number(280)], version: 2) + let store = makeStore(api: api, identity: StaticDeviceIdentity(deviceID)) + + // A document with no agent keys reads as absent — the agent has never + // run here — but its contents must still survive the migration write. + let state = await store.load() + XCTAssertEqual(state, .absent) + try await store.save(sampleConfiguration(machineID: deviceID)) + + let stored = await api.storedSettings() + XCTAssertEqual(stored["sidebarWidth"]?.doubleValue, 280) + XCTAssertNotNil(stored[DocumentSyncSettingsKeys.enabled]) + } +} + +// MARK: - The launch decision + +/// ``ConfigurationResolver`` is four cases of pure logic guarding the one thing +/// in this feature that can destroy a user's settings, so it gets its own tests +/// with no network, Keychain, or clock in the way. +final class ConfigurationResolverTests: XCTestCase { + + private let local = SyncAgentConfiguration(syncEnabled: false, pollIntervalSeconds: 45) + private let remote = SyncAgentConfiguration(syncEnabled: true, pollIntervalSeconds: 300) + + func test_givenBothSourcesHoldAValue_whenResolving_thenTheStoredOneWinsAndNothingMigrates() { + let resolution = ConfigurationResolver.resolve( + remote: .stored(remote), + local: local, + hasMigrated: false + ) + + XCTAssertEqual(resolution.configuration, remote) + XCTAssertFalse(resolution.shouldMigrate) + } + + func test_givenAFreshInstallWithNothingStoredEitherSide_whenResolving_thenDefaultsMigrateUp() { + let defaults = SyncAgentConfiguration() + + let resolution = ConfigurationResolver.resolve( + remote: .absent, + local: defaults, + hasMigrated: false + ) + + XCTAssertEqual(resolution.configuration, defaults) + XCTAssertTrue(resolution.shouldMigrate) + } + + func test_givenLocalSettingsAndAnEmptyServer_whenResolving_thenTheyMigrateUpOnce() { + let resolution = ConfigurationResolver.resolve( + remote: .absent, + local: local, + hasMigrated: false + ) + + XCTAssertEqual(resolution.configuration, local) + XCTAssertTrue(resolution.shouldMigrate) + } + + func test_givenAlreadyMigratedAndTheDocumentIsGone_whenResolving_thenItIsNotRecreated() { + // The machine was deregistered, or the settings were deleted on purpose. + // Re-uploading them would resurrect what the user removed. + let resolution = ConfigurationResolver.resolve( + remote: .absent, + local: local, + hasMigrated: true + ) + + XCTAssertEqual(resolution.configuration, local) + XCTAssertFalse(resolution.shouldMigrate) + } + + func test_givenTheServerCannotBeReached_whenResolving_thenNothingMigrates() { + // The dangerous case: treating "could not ask" as "nothing stored" would + // overwrite a newer server document with a stale local one. + let resolution = ConfigurationResolver.resolve( + remote: .unavailable, + local: local, + hasMigrated: false + ) + + XCTAssertEqual(resolution.configuration, local) + XCTAssertFalse(resolution.shouldMigrate) + } +} diff --git a/SyncAgent/Tests/InterlinedListSyncCoreTests/SyncAPIClientTests.swift b/SyncAgent/Tests/InterlinedListSyncCoreTests/SyncAPIClientTests.swift index eaa2b7a..a8ecd47 100644 --- a/SyncAgent/Tests/InterlinedListSyncCoreTests/SyncAPIClientTests.swift +++ b/SyncAgent/Tests/InterlinedListSyncCoreTests/SyncAPIClientTests.swift @@ -100,4 +100,148 @@ final class SyncAPIClientTests: XCTestCase { let client = makeClient() try await client.deleteDocument(id: "gone") // must not throw } + + // MARK: - Per-machine app settings (GitHub issue #104) + // + // Path, method and body are asserted together on purpose. A wrong body is + // as breaking as a wrong path and far quieter — PR #102 found this exact + // family shipping `{"name":…}` for weeks while every path assertion passed. + + func test_givenAConfiguredMachine_whenWritingSettings_thenItPutsToTheDeviceRouteWithABaseVersion() async throws { + MockURLProtocol.handler = { request in + XCTAssertEqual(request.httpMethod, "PUT") + XCTAssertEqual( + request.url?.path, + "/api/user/app-settings/interlinedlist-macos/devices/mac-1/settings" + ) + let body = try XCTUnwrap(request.httpBodyStream.map { stream -> Data in + stream.open() + defer { stream.close() } + var data = Data() + var buffer = [UInt8](repeating: 0, count: 4096) + while stream.hasBytesAvailable { + let read = stream.read(&buffer, maxLength: buffer.count) + if read <= 0 { break } + data.append(buffer, count: read) + } + return data + } ?? request.httpBody) + let json = try XCTUnwrap(JSONSerialization.jsonObject(with: body) as? [String: Any]) + XCTAssertEqual(json["baseVersion"] as? Int, 3) + XCTAssertEqual(json["schemaVersion"] as? Int, SyncConfiguration.settingsSchemaVersion) + let settings = try XCTUnwrap(json["settings"] as? [String: Any]) + XCTAssertEqual(settings[DocumentSyncSettingsKeys.enabled] as? Bool, true) + + let response = #"{"appKey":"interlinedlist-macos","scope":"device","deviceId":"mac-1","version":4,"updatedAt":"2026-09-16T19:39:07.135Z","schemaVersion":1,"settings":{"documentSync.enabled":true}}"# + return ( + HTTPURLResponse(url: request.url!, statusCode: 200, httpVersion: nil, headerFields: nil)!, + Data(response.utf8) + ) + } + let client = makeClient() + + let document = try await client.writeDeviceSettings( + deviceId: "mac-1", + settings: [DocumentSyncSettingsKeys.enabled: .bool(true)], + baseVersion: 3 + ) + + XCTAssertEqual(document.version, 4) + XCTAssertEqual(document.deviceId, "mac-1") + } + + func test_givenAMachineThatHasNeverStoredSettings_whenReading_thenItAnswersNilRatherThanThrowing() async throws { + MockURLProtocol.handler = { request in + XCTAssertEqual(request.httpMethod, "GET") + return ( + HTTPURLResponse(url: request.url!, statusCode: 404, httpVersion: nil, headerFields: nil)!, + Data(#"{"error":"Not found","code":"not_found"}"#.utf8) + ) + } + let client = makeClient() + + let document = try await client.fetchDeviceSettings(deviceId: "mac-1") + + // First run, not a failure. The caller has to be able to tell this from + // "the request failed", because only this one may trigger a migration. + XCTAssertNil(document) + } + + func test_givenAStaleBaseVersion_whenWritingSettings_thenTheConflictCarriesTheWinningDocument() async { + MockURLProtocol.handler = { request in + let response = #"{"error":"version_conflict","code":"version_conflict","current":{"appKey":"interlinedlist-macos","scope":"device","deviceId":"mac-1","version":9,"schemaVersion":1,"settings":{"theme":"dark"}}}"# + return ( + HTTPURLResponse(url: request.url!, statusCode: 409, httpVersion: nil, headerFields: nil)!, + Data(response.utf8) + ) + } + let client = makeClient() + + do { + _ = try await client.writeDeviceSettings(deviceId: "mac-1", settings: [:], baseVersion: 1) + XCTFail("expected a version conflict") + } catch APIError.versionConflict(let current) { + // Lifting `current` out of the 409 is what lets a background retry + // re-base without spending another request. + XCTAssertEqual(current?.version, 9) + XCTAssertEqual(current?.settings["theme"]?.stringValue, "dark") + } catch { + XCTFail("expected a version conflict, got \(error)") + } + } + + func test_givenAnUnparseableConflictBody_whenWritingSettings_thenItStillSurfacesAsAConflict() async { + MockURLProtocol.handler = { request in + ( + HTTPURLResponse(url: request.url!, statusCode: 409, httpVersion: nil, headerFields: nil)!, + Data("not json".utf8) + ) + } + let client = makeClient() + + do { + _ = try await client.writeDeviceSettings(deviceId: "mac-1", settings: [:], baseVersion: 1) + XCTFail("expected a version conflict") + } catch APIError.versionConflict(let current) { + // The retry falls back to a fresh read rather than the whole write + // failing on a decoding error. + XCTAssertNil(current) + } catch { + XCTFail("expected a version conflict, got \(error)") + } + } + + func test_givenAnUnregisteredMachine_whenWritingSettings_thenItIsNotMistakenForAnEmptyDocument() async { + MockURLProtocol.handler = { request in + ( + HTTPURLResponse(url: request.url!, statusCode: 404, httpVersion: nil, headerFields: nil)!, + Data(#"{"error":"device not registered"}"#.utf8) + ) + } + let client = makeClient() + + do { + _ = try await client.writeDeviceSettings(deviceId: "mac-1", settings: [:], baseVersion: 0) + XCTFail("expected deviceNotRegistered") + } catch { + // `baseVersion: 0` creates a document happily, so a 404 here can only + // mean the device is missing — mapping it to "empty" would silently + // discard the user's settings. + XCTAssertEqual(error as? APIError, .deviceNotRegistered) + } + } + + func test_givenAnUnregisteredMachine_whenRegistering_thenItPostsTheLiveFieldNames() async throws { + MockURLProtocol.handler = { request in + XCTAssertEqual(request.httpMethod, "POST") + XCTAssertEqual(request.url?.path, "/api/user/app-settings/interlinedlist-macos/devices") + return ( + HTTPURLResponse(url: request.url!, statusCode: 200, httpVersion: nil, headerFields: nil)!, + Data(#"{"device":{"deviceId":"mac-1","deviceName":"Studio Mac","isDefault":true}}"#.utf8) + ) + } + let client = makeClient() + + try await client.registerDevice(deviceId: "mac-1", deviceName: "Studio Mac") + } } diff --git a/SyncAgent/Tests/InterlinedListSyncCoreTests/SyncAgentConfigurationTests.swift b/SyncAgent/Tests/InterlinedListSyncCoreTests/SyncAgentConfigurationTests.swift new file mode 100644 index 0000000..febbf95 --- /dev/null +++ b/SyncAgent/Tests/InterlinedListSyncCoreTests/SyncAgentConfigurationTests.swift @@ -0,0 +1,140 @@ +import XCTest +@testable import InterlinedListSyncCore + +/// The payload shape the agent stores in per-machine app settings (GitHub issue +/// #104) — in particular the rule that a security-scoped bookmark belonging to +/// another Mac is not a configuration, it is noise. +final class SyncAgentConfigurationTests: XCTestCase { + + private let thisMachine = "mac-1" + private let bookmark = Data("bookmark-bytes".utf8) + + private func configured(machineID: String) -> SyncAgentConfiguration { + SyncAgentConfiguration( + syncEnabled: false, + pollIntervalSeconds: 120, + launchAtLogin: true, + notificationsEnabled: false, + notifyOnCompletion: true, + notifyOnErrors: false, + notifyOnConflicts: false, + folder: SyncFolderReference( + bookmark: bookmark, + displayPath: "/Users/someone/Vault", + machineID: machineID + ), + lastSyncAt: Date(timeIntervalSince1970: 1_700_000_000) + ) + } + + // MARK: - Happy path + + func test_givenAConfigurationWithAFolder_whenWrittenAndReadBack_thenEveryFieldSurvives() throws { + let original = configured(machineID: thisMachine) + + let payload = original.apply(to: [:]) + let restored = try XCTUnwrap( + SyncAgentConfiguration(settings: payload, thisMachineID: thisMachine) + ) + + XCTAssertEqual(restored, original) + XCTAssertEqual( + payload[DocumentSyncSettingsKeys.schemaVersion]?.doubleValue, + Double(SyncConfiguration.settingsSchemaVersion) + ) + } + + func test_givenADocumentWrittenHere_whenRead_thenTheFolderIsAccepted() throws { + let payload = configured(machineID: thisMachine).apply(to: [:]) + + let restored = try XCTUnwrap( + SyncAgentConfiguration(settings: payload, thisMachineID: thisMachine) + ) + + XCTAssertEqual(restored.folder?.bookmark, bookmark) + XCTAssertEqual(restored.folder?.displayPath, "/Users/someone/Vault") + } + + // MARK: - Invalid input + + func test_givenABookmarkFromAnotherMac_whenRead_thenTheMachineIsNotConfiguredHere() throws { + let payload = configured(machineID: "mac-2").apply(to: [:]) + + let restored = try XCTUnwrap( + SyncAgentConfiguration(settings: payload, thisMachineID: thisMachine) + ) + + // Everything else still applies — only the folder is refused, because + // only the folder is machine-local. + XCTAssertNil(restored.folder) + XCTAssertEqual(restored.pollIntervalSeconds, 120) + XCTAssertFalse(restored.syncEnabled) + } + + func test_givenABookmarkWithNoMachineStamp_whenRead_thenTheFolderIsRefused() throws { + var payload = configured(machineID: thisMachine).apply(to: [:]) + payload[DocumentSyncSettingsKeys.folderMachineID] = nil + + let restored = try XCTUnwrap( + SyncAgentConfiguration(settings: payload, thisMachineID: thisMachine) + ) + + XCTAssertNil(restored.folder) + } + + func test_givenAPollIntervalBelowTheFloor_whenRead_thenItIsClamped() throws { + var payload = configured(machineID: thisMachine).apply(to: [:]) + payload[DocumentSyncSettingsKeys.pollIntervalSeconds] = .number(1) + + let restored = try XCTUnwrap( + SyncAgentConfiguration(settings: payload, thisMachineID: thisMachine) + ) + + XCTAssertEqual(restored.pollIntervalSeconds, SyncConfiguration.minPollInterval) + } + + // MARK: - Boundary + + func test_givenADocumentWithNoAgentKeys_whenRead_thenThereIsNoStoredConfiguration() { + // The main app may well have stored per-machine settings of its own. + // That is not a configuration this agent wrote, so the migration must + // still be allowed to run. + let payload: [String: SettingsValue] = ["sidebarWidth": .number(280)] + + XCTAssertNil(SyncAgentConfiguration(settings: payload, thisMachineID: thisMachine)) + } + + func test_givenAnEmptyDocument_whenRead_thenThereIsNoStoredConfiguration() { + XCTAssertNil(SyncAgentConfiguration(settings: [:], thisMachineID: thisMachine)) + } + + // MARK: - Shared document + + func test_givenKeysOwnedByAnotherClient_whenWriting_thenTheySurvive() { + let existing: [String: SettingsValue] = [ + "sidebarWidth": .number(280), + "theme": .string("dark") + ] + + let payload = configured(machineID: thisMachine).apply(to: existing) + + // The PUT replaces the document wholesale, so anything the overlay drops + // is deleted from the server. These keys belong to the main app. + XCTAssertEqual(payload["sidebarWidth"]?.doubleValue, 280) + XCTAssertEqual(payload["theme"]?.stringValue, "dark") + } + + func test_givenAStoredFolder_whenWritingAConfigurationWithoutOne_thenTheStaleFolderKeysGo() { + let existing = configured(machineID: thisMachine).apply(to: [:]) + var unconfigured = configured(machineID: thisMachine) + unconfigured.folder = nil + + let payload = unconfigured.apply(to: existing) + + // Rebuilt from scratch inside the namespace: leaving a stale bookmark + // behind would let a cleared folder come back on the next read. + XCTAssertNil(payload[DocumentSyncSettingsKeys.folderBookmark]) + XCTAssertNil(payload[DocumentSyncSettingsKeys.folderPath]) + XCTAssertNil(payload[DocumentSyncSettingsKeys.folderMachineID]) + } +} From 3cde1bfba89266fe0e3fabc88bbdf36d5d4c5461 Mon Sep 17 00:00:00 2001 From: Adron Hall Date: Thu, 17 Sep 2026 01:44:12 -0700 Subject: [PATCH 2/2] fix(sync-agent): publish the device id off the launch path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merging dev exposed a real defect, not a merge artefact: the App test host hung before establishing connection, on every run, and `xcodebuild test` failed with no test having executed. Removing the launch-time `DeviceIdentity.current()` call made 1080 tests pass; restoring it reproduced the hang. That is the whole diagnosis. `DeviceIdentity.current()` reads and writes the Keychain, which is synchronous IPC to `securityd` against a shared access group. An unsigned or wrongly entitled build has no claim on that group, and the failure mode is a stall rather than a clean `errSecMissingEntitlement` — so the process never finished launching. This is not only a test problem. Even signed and entitled, a locked or first-unlock Keychain can make the call slow, and blocking the launch path on it is wrong regardless of who is watching. The publish is now detached at utility priority. The agent reads the published id on its own schedule, so being a few hundred milliseconds late costs nothing, and the Applications pane still calls `current()` directly where it genuinely needs the value synchronously. The resolution order the type documents — UserDefaults, then the shared Keychain, then mint — is untouched. Refs #104 Co-Authored-By: Claude Opus 5 --- App/Composition/AppEnvironment.swift | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/App/Composition/AppEnvironment.swift b/App/Composition/AppEnvironment.swift index adc8e2b..ac307be 100644 --- a/App/Composition/AppEnvironment.swift +++ b/App/Composition/AppEnvironment.swift @@ -632,7 +632,24 @@ final class AppEnvironment: ObservableObject { // opened. The document-sync agent needs the id to address its // per-machine settings document, and most users never visit that pane — // leaving the agent stranded on local `UserDefaults` forever. - DeviceIdentity.current() + // + // **Detached, and that is not a nicety.** `DeviceIdentity.current()` + // reads and writes the Keychain, which is synchronous IPC to `securityd` + // against a shared access group. Calling it inline here hung the process + // at launch outright — the App test host never finished starting, and + // `xcodebuild test` failed with "The test runner hung before + // establishing connection" on every run. An unsigned or + // wrongly-entitled build has no claim on that access group, and the + // failure mode is a stall rather than a clean `errSecMissingEntitlement`. + // + // Even signed and entitled, a locked or first-unlock Keychain can make + // this slow. Nothing on the launch path should wait on it: the agent + // reads the published id on its own schedule, so being a few hundred + // milliseconds late costs nothing, and the Applications pane calls + // `current()` directly when it genuinely needs the value synchronously. + Task.detached(priority: .utility) { + DeviceIdentity.current() + } return AppEnvironment( messages: messages, lists: lists,