Pre-push review: PR #200 final head

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex 9b26644..321243e 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed  - MCP `update_task_status` now returns the exact comment created by its atomic status-plus-comment mutation (T-1823). `TaskService.updateStatus` propagates the created `Comment` to the handler for direct serialization instead of refetching every task comment and taking the last `creationDate`; a future-dated existing or concurrently imported comment can no longer replace the mutation result. If the atomic save fails, the newly inserted comment is deleted before the task state is rolled back so a later save cannot resurrect it. Regressions verify exact UUID propagation, future-clock-skew behavior, and service/MCP failure recovery.+- Cross-device task and milestone promotion now re-probes committed SwiftData state after display-ID allocation and skips assignment when a peer has already supplied a permanent ID (T-2020). Missing or unreadable records fail closed, save-failure recovery preserves peer-merged values, and deterministic two-context tests pin both record types. Because SwiftData has no field-level compare-and-set, this closes the peer-merge-during-await window but cannot prevent two devices whose final local probes both precede either CloudKit merge from racing; the limitation and owner-reservation alternative are documented.  - MCP `initialize` now requires object params with string `protocolVersion`, object `capabilities`, and typed `clientInfo` identity fields (T-1778). Missing or malformed method parameters return JSON-RPC `-32602 Invalid Params`. JSON-RPC request decoding now rejects scalar/null top-level `params` server-wide with `-32600 Invalid Request`. Supported `2025-03-26` requests are echoed and unsupported versions receive the server's latest advertised supported version. Focused handshake tests cover absent params/fields, wrong types, malformed client information, request-shape errors, and both negotiation paths. - MCP server start, stop, and same-port restart now run through one async desired-state lifecycle coordinator (T-1826). Each active listener owns its Hummingbird `ServiceGroup` and run task; teardown explicitly triggers graceful shutdown and awaits the group before rebinding, rather than assuming cancellation completion releases the socket. Graceful shutdown is time-bounded so a stalled request cannot wedge the coordinator, and the group installs no SIGTERM/SIGINT traps (`runService()`'s default) because those change the process's signal disposition permanently. Requests arriving during teardown replace the pending target so a burst converges on the latest state, Settings uses an explicit restart operation, and live loopback regressions cover stop, 20 serialized same-port repetitions, different-port restart, rapid off/on, an occupied port surfacing the bind failure, an invalid port releasing the running listener, bounded shutdown escalation, empty signal configuration, and reusable bind-probe behavior.

diff --git a/Transit/Transit/Services/DisplayIDAllocator.swift b/Transit/Transit/Services/DisplayIDAllocator.swiftindex bb7988f..165e8c0 100644--- a/Transit/Transit/Services/DisplayIDAllocator.swift+++ b/Transit/Transit/Services/DisplayIDAllocator.swift@@ -240,17 +240,35 @@ final class DisplayIDAllocator: @unchecked Sendable {         // assigns a duplicate (T-1395). A failed read throws rather than yielding         // an empty set, which would disable the guard entirely (T-1621).         let usedIDs = usedTaskIDs ?? { try UsedDisplayIDs(context).tasks() }+        let recordLookup = DisplayIDRecordLookup(modelContext: context)          for task in tasks {+            let newID: Int+            do {+                newID = try await allocateNextID(excluding: usedIDs)+                // Allocation suspends, so re-read committed state through a transient+                // context before mutating the stale registered object (T-2020).+                // Missing or unreadable records fail closed.+                guard try recordLookup.taskIsStillProvisional(id: task.id) else {+                    // The allocated counter value cannot be reused safely; another+                    // record may already have observed it, so deliberately leave a gap.+                    continue+                }+            } catch {+                // Stop on allocation or committed-state read failure. Remaining+                // tasks will be retried by the next lifecycle promotion pass.+                break+            }++            task.permanentDisplayId = newID             do {-                let newID = try await allocateNextID(excluding: usedIDs)-                task.permanentDisplayId = newID                 try save(context)             } catch {-                // Revert only this promotion attempt so unrelated unsaved edits-                // on the shared context survive connectivity-triggered retries.-                task.permanentDisplayId = nil-                // Stop on first failure -- remaining tasks will be retried next pass.+                // Revert only the value this promotion assigned. If SwiftData merged+                // a peer value, preserve it rather than resetting the record to nil.+                if task.permanentDisplayId == newID {+                    task.permanentDisplayId = nil+                }                 break             }         }

diff --git a/Transit/Transit/Services/DisplayIDRecordLookup.swift b/Transit/Transit/Services/DisplayIDRecordLookup.swiftindex 1e1b56c..cd71b99 100644--- a/Transit/Transit/Services/DisplayIDRecordLookup.swift+++ b/Transit/Transit/Services/DisplayIDRecordLookup.swift@@ -28,6 +28,26 @@ struct DisplayIDRecordLookup {         return try? modelContext.fetch(descriptor).first     } +    // MARK: - Promotion precondition probes++    /// Returns true only when the task exists and its committed display ID is still+    /// provisional. Fetch failures are thrown and a missing record returns false, so+    /// promotion fails closed instead of treating unreadable state as `nil` (T-2020).+    func taskIsStillProvisional(id: UUID) throws -> Bool {+        let descriptor = FetchDescriptor<TransitTask>(predicate: #Predicate { $0.id == id })+        let probe = ModelContext(modelContext.container)+        guard let task = try probe.fetch(descriptor).first else { return false }+        return task.permanentDisplayId == nil+    }++    /// Milestone companion to `taskIsStillProvisional` (T-2020).+    func milestoneIsStillProvisional(id: UUID) throws -> Bool {+        let descriptor = FetchDescriptor<Milestone>(predicate: #Predicate { $0.id == id })+        let probe = ModelContext(modelContext.container)+        guard let milestone = try probe.fetch(descriptor).first else { return false }+        return milestone.permanentDisplayId == nil+    }+     // MARK: - Committed display ID probes      /// Reads the task's committed `permanentDisplayId`. SwiftData has no

diff --git a/Transit/Transit/Services/MilestoneService.swift b/Transit/Transit/Services/MilestoneService.swiftindex 1eb25e3..877e9f4 100644--- a/Transit/Transit/Services/MilestoneService.swift+++ b/Transit/Transit/Services/MilestoneService.swift@@ -212,21 +212,34 @@ final class MilestoneService {             return         } +        let recordLookup = DisplayIDRecordLookup(modelContext: modelContext)+         for milestone in milestones {+            let newID: Int             do {-                // Recompute used IDs inside the gate so a promoted ID never-                // collides with one already committed (including ones just-                // assigned in this loop) (T-1395).-                let newID = try await displayIDAllocator.allocateNextID(+                // Recompute used IDs inside the gate so just-committed IDs are excluded (T-1395).+                newID = try await displayIDAllocator.allocateNextID(                     excluding: { try self.usedDisplayIDs.milestones() }                 )-                milestone.permanentDisplayId = newID+                // After suspension, transiently re-read committed state before+                // mutating the stale object; missing/unreadable records fail closed (T-2020).+                guard try recordLookup.milestoneIsStillProvisional(id: milestone.id) else {+                    // Leave a gap: the allocated value cannot be safely reused.+                    continue+                }+            } catch {+                // Stop; the next lifecycle pass retries remaining milestones.+                break+            }++            milestone.permanentDisplayId = newID+            do {                 try save(modelContext)             } catch {-                // Revert only this promotion attempt so unrelated unsaved edits-                // on the shared context survive connectivity-triggered retries.-                milestone.permanentDisplayId = nil-                // Stop on first failure -- remaining milestones will be retried next pass.+                // Reset only this run's value; preserve a peer merge.+                if milestone.permanentDisplayId == newID {+                    milestone.permanentDisplayId = nil+                }                 break             }         }

diff --git a/Transit/TransitTests/CrossDevicePromotionTests.swift b/Transit/TransitTests/CrossDevicePromotionTests.swiftnew file mode 100644index 0000000..e8a609c--- /dev/null+++ b/Transit/TransitTests/CrossDevicePromotionTests.swift@@ -0,0 +1,168 @@+import Foundation+import SwiftData+import Testing+@testable import Transit++/// Controlled T-2020 store-merge regressions. Independent contexts model a peer+/// change that has already reached the receiving device's local SwiftData store.+@MainActor @Suite(.serialized)+struct CrossDevicePromotionTests {++    private func expectStoredTaskAndProject(+        in container: ModelContainer,+        taskID: UUID,+        projectID: UUID,+        expectedTaskDisplayID: Int+    ) throws {+        let probe = ModelContext(container)+        let storedTask = try #require(try probe.fetch(FetchDescriptor<TransitTask>(+            predicate: #Predicate { $0.id == taskID }+        )).first)+        #expect(storedTask.permanentDisplayId == expectedTaskDisplayID,+                "A peer's permanent task ID must survive promotion")+        try expectProjectDraftRemainsUnsaved(in: probe, projectID: projectID)+    }++    private func expectStoredMilestoneAndProject(+        in container: ModelContainer,+        milestoneID: UUID,+        projectID: UUID,+        expectedMilestoneDisplayID: Int+    ) throws {+        let probe = ModelContext(container)+        let storedMilestone = try #require(try probe.fetch(FetchDescriptor<Milestone>(+            predicate: #Predicate { $0.id == milestoneID }+        )).first)+        #expect(storedMilestone.permanentDisplayId == expectedMilestoneDisplayID,+                "A peer's permanent milestone ID must survive promotion")+        try expectProjectDraftRemainsUnsaved(in: probe, projectID: projectID)+    }++    private func expectProjectDraftRemainsUnsaved(+        in probe: ModelContext,+        projectID: UUID+    ) throws {+        let storedProject = try #require(try probe.fetch(FetchDescriptor<Project>(+            predicate: #Predicate { $0.id == projectID }+        )).first)+        #expect(storedProject.projectDescription == "",+                "Promotion must not persist an unrelated draft when the peer wins")+    }++    @Test func taskPeerPromotionDuringAllocationIsPreserved() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let gateStore = AllocationGatedCounterStore(initialNextDisplayID: 100)+        let allocator = DisplayIDAllocator(store: gateStore)++        let project = Project(name: "P", description: "", gitRepo: nil, colorHex: "#000000")+        context.insert(project)+        let task = TransitTask(+            name: "Synced provisional", type: .feature, project: project, displayID: .provisional+        )+        context.insert(task)+        try context.save()++        let taskID = task.id+        let projectID = project.id+        let peerContext = ModelContext(testContainer.container)+        let peerTask = try #require(try peerContext.fetch(FetchDescriptor<TransitTask>(+            predicate: #Predicate { $0.id == taskID }+        )).first)+        project.projectDescription = "Unsaved task-promotion draft"++        var promotionSaveCount = 0+        let promotion = Task { @MainActor in+            await allocator.promoteProvisionalTasks(in: context, save: { context in+                promotionSaveCount += 1+                try context.save()+            })+        }+        #expect(await gateStore.waitUntilAllocationStarts(), "Task allocation never parked")++        peerTask.permanentDisplayId = 900+        try peerContext.save()+        await gateStore.releaseAllocation()+        await promotion.value++        #expect(promotionSaveCount == 0,+                "Task promotion must not save after a peer assigns a permanent ID")+        #expect(project.projectDescription == "Unsaved task-promotion draft")+        #expect(context.hasChanges, "Peer-win promotion must preserve unrelated dirty state")+        try expectStoredTaskAndProject(+            in: testContainer.container,+            taskID: taskID,+            projectID: projectID,+            expectedTaskDisplayID: 900+        )+        #expect(await gateStore.currentNextDisplayID() == 101,+                "The allocated counter value is deliberately consumed when the peer wins")++        await allocator.promoteProvisionalTasks(in: context, save: { context in+            promotionSaveCount += 1+            try context.save()+        })+        #expect(promotionSaveCount == 0)+        #expect(await gateStore.currentNextDisplayID() == 101,+                "A committed peer ID must not leak another counter value on a later pass")+    }++    @Test func milestonePeerPromotionDuringAllocationIsPreserved() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let gateStore = AllocationGatedCounterStore(initialNextDisplayID: 200)+        let allocator = DisplayIDAllocator(store: gateStore)+        let service = MilestoneService(modelContext: context, displayIDAllocator: allocator)++        let project = Project(name: "P", description: "", gitRepo: nil, colorHex: "#000000")+        context.insert(project)+        let milestone = Milestone(+            name: "Synced provisional", description: nil, project: project, displayID: .provisional+        )+        context.insert(milestone)+        try context.save()++        let milestoneID = milestone.id+        let projectID = project.id+        let peerContext = ModelContext(testContainer.container)+        let peerMilestone = try #require(try peerContext.fetch(FetchDescriptor<Milestone>(+            predicate: #Predicate { $0.id == milestoneID }+        )).first)+        project.projectDescription = "Unsaved milestone-promotion draft"++        var promotionSaveCount = 0+        let promotion = Task { @MainActor in+            await service.promoteProvisionalMilestones(save: { context in+                promotionSaveCount += 1+                try context.save()+            })+        }+        #expect(await gateStore.waitUntilAllocationStarts(), "Milestone allocation never parked")++        peerMilestone.permanentDisplayId = 901+        try peerContext.save()+        await gateStore.releaseAllocation()+        await promotion.value++        #expect(promotionSaveCount == 0,+                "Milestone promotion must not save after a peer assigns a permanent ID")+        #expect(project.projectDescription == "Unsaved milestone-promotion draft")+        #expect(context.hasChanges, "Peer-win promotion must preserve unrelated dirty state")+        try expectStoredMilestoneAndProject(+            in: testContainer.container,+            milestoneID: milestoneID,+            projectID: projectID,+            expectedMilestoneDisplayID: 901+        )+        #expect(await gateStore.currentNextDisplayID() == 201,+                "The allocated counter value is deliberately consumed when the peer wins")++        await service.promoteProvisionalMilestones(save: { context in+            promotionSaveCount += 1+            try context.save()+        })+        #expect(promotionSaveCount == 0)+        #expect(await gateStore.currentNextDisplayID() == 201,+                "A committed peer milestone ID must not leak another value on a later pass")+    }+}

diff --git a/Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swift b/Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swiftindex e54bf16..3e732a8 100644--- a/Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swift+++ b/Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swift@@ -19,70 +19,6 @@ struct DisplayIDMaintenanceServiceReassignTests {         let project: Project     } -    /// Counter store that parks the loser's allocation after maintenance has-    /// completed its two counter-fence reads. This pins the exact check-to-write-    /// race from T-2019 without relying on scheduler timing.-    private actor AllocationGatedCounterStore: DisplayIDAllocator.CounterStore {-        private var nextDisplayID: Int-        private var changeTag = 0-        private var loadCount = 0--        private var allocationStarted = false-        private var allocationReleased = false-        private var allocationReleaseContinuation: CheckedContinuation<Void, Never>?--        init(initialNextDisplayID: Int = 100) {-            self.nextDisplayID = initialNextDisplayID-        }--        /// Committed counter value. Callers assert on it to prove the gate parked-        /// inside allocation rather than during the counter fence.-        func currentNextDisplayID() -> Int { nextDisplayID }--        /// Waits for the gated load to park, giving up at the deadline. Returning-        /// `false` rather than parking forever means a changed counter-call-        /// sequence fails the test instead of wedging the suite on a continuation-        /// nobody resumes.-        func waitUntilAllocationStarts(timeout: Duration = .seconds(10)) async -> Bool {-            let deadline = ContinuousClock.now + timeout-            while !allocationStarted && ContinuousClock.now < deadline {-                try? await Task.sleep(for: .milliseconds(5))-            }-            return allocationStarted-        }--        func releaseAllocation() {-            allocationReleased = true-            allocationReleaseContinuation?.resume()-            allocationReleaseContinuation = nil-        }--        func loadCounter() async throws -> DisplayIDAllocator.CounterSnapshot {-            loadCount += 1-            // Calls 1 and 2 come from `advanceCounterIfNeeded`: its threshold-            // check and reported snapshot. Call 3 is `allocateLocked` reading the-            // loser's candidate, which is the suspension point this test needs.-            if loadCount == 3 {-                allocationStarted = true-                if !allocationReleased {-                    await withCheckedContinuation { allocationReleaseContinuation = $0 }-                }-            }-            return DisplayIDAllocator.CounterSnapshot(-                nextDisplayID: nextDisplayID,-                changeTag: "\(changeTag)"-            )-        }--        func saveCounter(nextDisplayID: Int, expectedChangeTag: String?) async throws {-            guard expectedChangeTag == "\(changeTag)" else {-                throw DisplayIDAllocator.Error.conflict-            }-            self.nextDisplayID = nextDisplayID-            changeTag += 1-        }-    }-     private struct GatedTestEnv {         let context: ModelContext         let service: DisplayIDMaintenanceService@@ -93,7 +29,9 @@ struct DisplayIDMaintenanceServiceReassignTests {     private func makeGatedEnv(gateTasks: Bool) throws -> GatedTestEnv {         let testContainer = try TestModelContainer()         let context = testContainer.context-        let gateStore = AllocationGatedCounterStore()+        // Reads 1–2 are the maintenance counter fence; read 3 is the+        // loser's allocation, which is the T-2019 suspension point.+        let gateStore = AllocationGatedCounterStore(gatedLoadNumber: 3)         let taskAllocator: DisplayIDAllocator         let milestoneAllocator: DisplayIDAllocator         if gateTasks {

diff --git a/Transit/TransitTests/PromotionPreconditionTests.swift b/Transit/TransitTests/PromotionPreconditionTests.swiftnew file mode 100644index 0000000..a43c183--- /dev/null+++ b/Transit/TransitTests/PromotionPreconditionTests.swift@@ -0,0 +1,102 @@+import Foundation+import SwiftData+import Testing+@testable import Transit++/// T-2020 fail-closed coverage for records removed while display-ID allocation+/// is suspended. The allocated value is consumed once, but no stale model is+/// mutated or saved and later lifecycle passes do not leak more values.+@MainActor @Suite(.serialized)+struct PromotionPreconditionTests {++    @Test func deletedTaskIsSkippedAfterAllocation() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let store = AllocationGatedCounterStore(initialNextDisplayID: 100)+        let allocator = DisplayIDAllocator(store: store)+        let project = Project(name: "P", description: "", gitRepo: nil, colorHex: "#000000")+        context.insert(project)+        let task = TransitTask(+            name: "Deleted by peer", type: .feature, project: project, displayID: .provisional+        )+        context.insert(task)+        try context.save()++        let taskID = task.id+        let peerContext = ModelContext(testContainer.container)+        let peerTask = try #require(try peerContext.fetch(FetchDescriptor<TransitTask>(+            predicate: #Predicate { $0.id == taskID }+        )).first)+        var saveCount = 0+        let promotion = Task { @MainActor in+            await allocator.promoteProvisionalTasks(in: context, save: { context in+                saveCount += 1+                try context.save()+            })+        }+        #expect(await store.waitUntilAllocationStarts(), "Task allocation never parked")++        peerContext.delete(peerTask)+        try peerContext.save()+        await store.releaseAllocation()+        await promotion.value++        #expect(saveCount == 0, "A deleted task must not be mutated or saved")+        let probe = ModelContext(testContainer.container)+        #expect(try probe.fetch(FetchDescriptor<TransitTask>(+            predicate: #Predicate { $0.id == taskID }+        )).isEmpty)+        #expect(await store.currentNextDisplayID() == 101,+                "The in-flight allocation is consumed when the record disappears")++        await allocator.promoteProvisionalTasks(in: context)+        #expect(await store.currentNextDisplayID() == 101,+                "A deleted record must not consume another value on a later pass")+    }++    @Test func deletedMilestoneIsSkippedAfterAllocation() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let store = AllocationGatedCounterStore(initialNextDisplayID: 200)+        let allocator = DisplayIDAllocator(store: store)+        let service = MilestoneService(modelContext: context, displayIDAllocator: allocator)+        let project = Project(name: "P", description: "", gitRepo: nil, colorHex: "#000000")+        context.insert(project)+        let milestone = Milestone(+            name: "Deleted by peer", project: project, displayID: .provisional+        )+        context.insert(milestone)+        try context.save()++        let milestoneID = milestone.id+        let peerContext = ModelContext(testContainer.container)+        let peerMilestone = try #require(try peerContext.fetch(FetchDescriptor<Milestone>(+            predicate: #Predicate { $0.id == milestoneID }+        )).first)+        var saveCount = 0+        let promotion = Task { @MainActor in+            await service.promoteProvisionalMilestones(save: { context in+                saveCount += 1+                try context.save()+            })+        }+        #expect(await store.waitUntilAllocationStarts(), "Milestone allocation never parked")++        peerContext.delete(peerMilestone)+        try peerContext.save()+        await store.releaseAllocation()+        await promotion.value++        #expect(saveCount == 0, "A deleted milestone must not be mutated or saved")+        let probe = ModelContext(testContainer.container)+        #expect(try probe.fetch(FetchDescriptor<Milestone>(+            predicate: #Predicate { $0.id == milestoneID }+        )).isEmpty)+        #expect(await store.currentNextDisplayID() == 201,+                "The in-flight milestone allocation is consumed when the record disappears")++        await service.promoteProvisionalMilestones()+        #expect(await store.currentNextDisplayID() == 201,+                "A deleted milestone must not consume another value on a later pass")+    }+}

diff --git a/Transit/TransitTests/PromotionRollbackTests.swift b/Transit/TransitTests/PromotionRollbackTests.swiftindex 4d591cb..8e39662 100644--- a/Transit/TransitTests/PromotionRollbackTests.swift+++ b/Transit/TransitTests/PromotionRollbackTests.swift@@ -127,4 +127,45 @@ struct PromotionRollbackTests {         #expect(milestone2.permanentDisplayId == nil)         #expect(milestone2.displayID == .provisional)     }++    @Test func failedTaskPromotionPreservesDifferentInMemoryID() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let allocator = DisplayIDAllocator(store: InMemoryCounterStore(initialNextDisplayID: 100))+        let project = Project(name: "P", description: "", gitRepo: nil, colorHex: "#000000")+        context.insert(project)+        let task = TransitTask(+            name: "Peer wins", type: .feature, project: project, displayID: .provisional+        )+        context.insert(task)+        try context.save()++        await allocator.promoteProvisionalTasks(in: context, save: { _ in+            task.permanentDisplayId = 900+            throw SaveFailure.simulated+        })++        #expect(task.permanentDisplayId == 900,+                "Recovery must not clear an ID different from the one this pass assigned")+    }++    @Test func failedMilestonePromotionPreservesDifferentInMemoryID() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let allocator = DisplayIDAllocator(store: InMemoryCounterStore(initialNextDisplayID: 100))+        let service = MilestoneService(modelContext: context, displayIDAllocator: allocator)+        let project = Project(name: "P", description: "", gitRepo: nil, colorHex: "#000000")+        context.insert(project)+        let milestone = Milestone(name: "Peer wins", project: project, displayID: .provisional)+        context.insert(milestone)+        try context.save()++        await service.promoteProvisionalMilestones(save: { _ in+            milestone.permanentDisplayId = 901+            throw SaveFailure.simulated+        })++        #expect(milestone.permanentDisplayId == 901,+                "Milestone recovery must preserve a different merged ID")+    } }

diff --git a/Transit/TransitTests/TaskServiceStatusCommentFailureTests.swift b/Transit/TransitTests/TaskServiceStatusCommentFailureTests.swiftindex 945602b..7023bde 100644--- a/Transit/TransitTests/TaskServiceStatusCommentFailureTests.swift+++ b/Transit/TransitTests/TaskServiceStatusCommentFailureTests.swift@@ -17,7 +17,7 @@ struct TaskServiceStatusCommentFailureTests {         let commentService = CommentService(modelContext: context)         let project = Project(             name: "Test Project",-            description: nil,+            description: "",             gitRepo: nil,             colorHex: "#FF0000"         )

diff --git a/Transit/TransitTests/TestModelContainer.swift b/Transit/TransitTests/TestModelContainer.swiftindex 274645b..725276d 100644--- a/Transit/TransitTests/TestModelContainer.swift+++ b/Transit/TransitTests/TestModelContainer.swift@@ -122,3 +122,64 @@ actor InMemoryCounterStore: DisplayIDAllocator.CounterStore {         changeTag += 1     } }++// MARK: - AllocationGatedCounterStore++/// Deterministic counter store that parks a selected `loadCounter` call.+///+/// Concurrency regressions use this to commit peer-context changes while an+/// allocation is suspended, without relying on scheduler timing. A timeout+/// makes changed counter-call sequences fail instead of wedging the test suite.+actor AllocationGatedCounterStore: DisplayIDAllocator.CounterStore {+    private var nextDisplayID: Int+    private var changeTag = 0+    private var loadCount = 0+    private let gatedLoadNumber: Int++    private var allocationStarted = false+    private var allocationReleased = false+    private var allocationReleaseContinuation: CheckedContinuation<Void, Never>?++    init(initialNextDisplayID: Int = 100, gatedLoadNumber: Int = 1) {+        self.nextDisplayID = initialNextDisplayID+        self.gatedLoadNumber = gatedLoadNumber+    }++    func currentNextDisplayID() -> Int { nextDisplayID }++    func waitUntilAllocationStarts(timeout: Duration = .seconds(10)) async -> Bool {+        let deadline = ContinuousClock.now + timeout+        while !allocationStarted && ContinuousClock.now < deadline {+            try? await Task.sleep(for: .milliseconds(5))+        }+        return allocationStarted+    }++    func releaseAllocation() {+        allocationReleased = true+        allocationReleaseContinuation?.resume()+        allocationReleaseContinuation = nil+    }++    func loadCounter() async throws -> DisplayIDAllocator.CounterSnapshot {+        loadCount += 1+        if loadCount == gatedLoadNumber {+            allocationStarted = true+            if !allocationReleased {+                await withCheckedContinuation { allocationReleaseContinuation = $0 }+            }+        }+        return DisplayIDAllocator.CounterSnapshot(+            nextDisplayID: nextDisplayID,+            changeTag: "\(changeTag)"+        )+    }++    func saveCounter(nextDisplayID: Int, expectedChangeTag: String?) async throws {+        guard expectedChangeTag == "\(changeTag)" else {+            throw DisplayIDAllocator.Error.conflict+        }+        self.nextDisplayID = nextDisplayID+        changeTag += 1+    }+}

diff --git a/docs/agent-notes/technical-constraints.md b/docs/agent-notes/technical-constraints.mdindex 1fd514d..d4f6ebd 100644--- a/docs/agent-notes/technical-constraints.md+++ b/docs/agent-notes/technical-constraints.md@@ -156,6 +156,27 @@ without any save attempt (T-650).  `ConnectivityMonitor.onRestore` is typed as `@MainActor @Sendable` (not just `@Sendable`) because the closure captures MainActor-isolated state (ModelContext) and Swift 6.3 enforces sendability checks on captured values. +## Cross-Device Display ID Promotion Guarantee++After `allocateNextID` returns, task and milestone promotion re-read the record's+committed `permanentDisplayId` through a transient `ModelContext` immediately+before mutating the registered model (T-2020). If the record is missing, the+probe fails, or a peer ID has merged into the local store, promotion fails+closed: it does not mutate or save that record. The already-allocated counter+value is deliberately skipped because reusing it could create a duplicate.++This closes the peer-merge-during-allocation window, but it is **not an atomic+cross-device compare-and-set**. SwiftData exposes no conditional update for one+model field, no public per-object refresh API, and no transaction spanning its+CloudKit-managed record plus the directly managed counter record. Two devices+can still both complete their final local nil probes before either assignment+has merged, then save different IDs; CloudKit conflict resolution may select one+later. A full ownership design would require a deployed direct-CloudKit+reservation keyed by model type + UUID, atomically created with the counter+advance so every promoter receives the same ID. That reservation still cannot+be atomically committed with SwiftData's model write, so it must be durable and+idempotent across retries.+ ## Test File Imports  With `SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor`, test files must explicitly `import Foundation` to use `Date`, `UUID`, `JSONSerialization`, etc. These aren't automatically available in the test target even though the app module imports them.

diff --git a/specs/bugfixes/cross-device-promotion-rewrites-permanent-display-id/report.md b/specs/bugfixes/cross-device-promotion-rewrites-permanent-display-id/report.mdnew file mode 100644index 0000000..ee8b97d--- /dev/null+++ b/specs/bugfixes/cross-device-promotion-rewrites-permanent-display-id/report.md@@ -0,0 +1,150 @@+# Bugfix Report: Cross-Device Promotion Can Rewrite Permanent Display IDs++**Date:** 2026-08-02+**Status:** Fixed+**Ticket:** T-2020++## Description of the Issue++Two devices can hold the same synced task or milestone with a provisional display ID. Each process has its own single-flight guard, so both can allocate a different permanent ID. More immediately, a peer promotion can merge into the local store while this process is suspended in `allocateNextID`; the current code then assigns its allocated ID unconditionally through a stale registered model and overwrites the peer value.++**Reproduction steps:**+1. Save a task or milestone with `permanentDisplayId == nil` and load it in two model contexts.+2. Start promotion in one context and suspend its counter allocation.+3. Commit a permanent display ID for the same UUID from the peer context.+4. Resume allocation and observe the first context replace the peer's ID.++**Impact:** A human-facing T-/M- identifier can change after it was observed on another device. The losing counter value is consumed without an owner or audit trail.++## Investigation Summary++The investigation followed the four systematic-debugging phases.++### Phase 1: Initial overview++- **Expected:** Promotion assigns an ID only while the committed record is still provisional. A permanent ID imported from a peer must win.+- **Actual:** Promotion checks provisional state only in its initial fetch, before asynchronous allocation.+- **Context:** Process-local single-flight guards prevent overlapping lifecycle triggers on one device but do not coordinate devices or processes.++### Phase 2: Systematic inspection++- **Task timing defect:** `DisplayIDAllocator.promoteProvisionalTasks` fetches nil IDs, awaits allocation, then assigns unconditionally.+- **Milestone timing defect:** `MilestoneService.promoteProvisionalMilestones` has the same fetch-await-assign sequence.+- **Stale registered state:** SwiftData has no public per-object refresh API; a context's registered model can retain its pre-merge value while a transient context sees the committed row.+- **Existing compatible pattern:** `DisplayIDRecordLookup` already uses transient contexts to probe committed display IDs for duplicate-cleanup stale-write protection.+- **Existing counter CAS scope:** `CloudKitCounterStore` atomically advances only the counter record. It does not bind an allocation to a SwiftData model UUID.++### Phase 3: Root cause analysis++**Defect type:** Cross-process race / expired precondition.++**Five Whys:**+1. Why can a peer ID be rewritten? Promotion writes after a peer has committed a permanent ID.+2. Why does promotion still write? Its model reference reflects the original provisional fetch.+3. Why can that fetch become stale? `allocateNextID` suspends while CloudKit counter work runs.+4. Why does the single-flight guard not prevent it? The guard is process-local; each device owns a different allocator/service instance.+5. Why is there no atomic ownership check on assignment? SwiftData exposes neither a conditional field update nor a transaction spanning its CloudKit-managed record and the direct CloudKit counter record.++**Root cause:** The committed `permanentDisplayId == nil` precondition is never revalidated after the final suspension point and immediately before mutation/save.++### Phase 4: Solution and verification plan++1. Re-read each task's committed display ID from a transient context after allocation. Assign/save only when it remains nil.+2. Apply the same post-allocation probe to milestones.+3. If a peer ID exists, preserve it and deliberately consume the allocated counter value; reusing that value would risk a duplicate.+4. Add deterministic two-context tests that park allocation, commit a peer ID, resume, and assert the peer ID remains for both model types.+5. Run the complete macOS unit suite and SwiftLint through the Makefile.++A stronger theoretical design is an owner-reservation CloudKit record keyed by model UUID, atomically saved with the counter update. That would make racing devices receive the same ID. It is not a schema-neutral patch: it introduces a directly managed CloudKit record type/fields that must be deployed, and the reservation still cannot be committed atomically with SwiftData's separately managed model record. The selected fix uses the strongest available SwiftData-side stale-write guard without changing the deployed CloudKit schema. The remaining simultaneous pre-merge race will be documented precisely in the finalized report and project note.++## Discovered Root Cause++The initial provisional-state fetch expires across asynchronous allocation. Both promotion paths then mutate a potentially stale registered object without probing the committed store.++**Defect type:** Race condition / stale write.++**Why it occurred:** T-597 added process-local exclusion but did not add cross-context validation after suspension.++**Contributing factors:** SwiftData CloudKit sync provides merge/conflict behavior but no public compare-and-set for a single model property.++## Resolution for the Issue++**Changes made:**+- `Transit/Transit/Services/DisplayIDRecordLookup.swift` — added throwing, fail-closed task and milestone probes that return true only for an existing committed provisional record.+- `Transit/Transit/Services/DisplayIDAllocator.swift` — task promotion now probes after allocation and skips mutation/save when a peer ID exists; save recovery resets only the exact ID this run assigned.+- `Transit/Transit/Services/MilestoneService.swift` — applies the same post-allocation ownership check and selective recovery to milestones.+- `Transit/TransitTests/CrossDevicePromotionTests.swift` — deterministic two-context/allocation regressions for both record types, including unrelated dirty-state preservation and a second lifecycle pass that consumes no further counter value.+- `Transit/TransitTests/PromotionPreconditionTests.swift` — task/milestone regressions for records deleted while allocation is suspended; each path consumes the in-flight value once, performs no stale save, and leaks no value on a later pass.+- `Transit/TransitTests/PromotionRollbackTests.swift` — symmetric selective-recovery tests prove a failed save does not clear a different in-memory ID.+- `Transit/TransitTests/TestModelContainer.swift` — generalized the existing allocation gate as shared test support; duplicate-maintenance tests continue to park their third counter read.++**Approach rationale:** The transient probe is the strongest schema-neutral check SwiftData exposes. It runs after the final suspension point and immediately before the MainActor mutation/save, closes the demonstrated peer-merge window, fails closed on missing/unreadable state, and does not require a CloudKit production-schema deployment.++**Alternatives considered:**+- **Direct CloudKit owner reservation keyed by model type + UUID:** This is the strongest full ownership direction because the reservation and counter advance can be one atomic `CKModifyRecordsOperation`, making every promoter recover the same ID. It was not selected for this patch because it adds a new deployed CloudKit record schema and migration/operational surface. It also cannot share a transaction with SwiftData's separately managed model write, so the reservation must become durable allocation state rather than a short-lived lock.+- **Rely on CloudKit conflict resolution:** Rejected because it allows a visible permanent ID to change after assignment.+- **Process-local locking:** Already present from T-597 and cannot coordinate devices.++**Remaining limitation:** The fix is not an atomic cross-device compare-and-set. If two devices both finish their final transient-context nil probe before either device's assignment has merged into the other's local store, both can still save different IDs and CloudKit can later resolve the conflict. SwiftData provides no conditional field update or transaction spanning its model record and the direct CloudKit counter. The implemented guarantee is specifically: a permanent ID already committed/merged into the local store by the time allocation returns will not be overwritten by that promotion pass.++## Regression Test++**Test files:**+- `Transit/TransitTests/CrossDevicePromotionTests.swift`+- `Transit/TransitTests/PromotionPreconditionTests.swift`+- `Transit/TransitTests/PromotionRollbackTests.swift`++**Focused test names:**+- `taskPeerPromotionDuringAllocationIsPreserved`+- `milestonePeerPromotionDuringAllocationIsPreserved`+- `deletedTaskIsSkippedAfterAllocation`+- `deletedMilestoneIsSkippedAfterAllocation`+- `failedTaskPromotionPreservesDifferentInMemoryID`+- `failedMilestonePromotionPreservesDifferentInMemoryID`++**What they verify:** The gated two-context tests suspend allocation while a second context commits a permanent ID or deletes the same UUID, then prove promotion performs no stale save. The peer-ID cases additionally prove unrelated dirty UI state remains unsaved and a later lifecycle pass consumes no second counter value. The selective-recovery tests exercise the exact-value guard and prove a failed save does not clear a different in-memory ID. These are deterministic local-store simulations of a CloudKit merge; they do not create a cross-device CloudKit transaction or close the documented simultaneous-probe race.++**Run command:** `make test-quick`++## Affected Files++| File | Change |+|------|--------|+| `Transit/TransitTests/CrossDevicePromotionTests.swift` | Controlled two-context regressions for tasks and milestones, dirty-state isolation, and later-pass counter stability |+| `Transit/TransitTests/PromotionPreconditionTests.swift` | Controlled deletion-during-allocation regressions for both record types |+| `Transit/TransitTests/PromotionRollbackTests.swift` | Selective save-failure recovery coverage for both record types |+| `Transit/TransitTests/TestModelContainer.swift` | Shared configurable allocation gate |+| `Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swift` | Reuse shared gate at the existing third-load suspension point |+| `Transit/Transit/Services/DisplayIDRecordLookup.swift` | Fail-closed committed provisional-state probes |+| `Transit/Transit/Services/DisplayIDAllocator.swift` | Post-allocation task recheck and selective save recovery |+| `Transit/Transit/Services/MilestoneService.swift` | Post-allocation milestone recheck and selective save recovery |+| `docs/agent-notes/technical-constraints.md` | Exact guarantee, residual race, and owner-reservation direction |+| `CHANGELOG.md` | Unreleased T-2020 fix entry |++## Verification++**Automated:**+- [x] Regression tests fail before the fix (`make test-quick`; both named T-2020 cases reported failed in the test output)+- [x] Regression tests pass after the fix (`make test-quick`)+- [x] Full macOS unit suite passes (`make test-quick`: 1,614 passed, 0 failed)+- [x] All unit tests in the iOS combined run pass (`make test`: 1,149 passed overall; the only 3 failures were unrelated UI tests)+- [x] Linters/validators pass (`make lint`, including the SwiftData ownership guard)+- [ ] Full iOS/UI suite passes — blocked by unrelated existing UI failures below++**Validation blockers:**+- `make test`: 1,149 passed, 3 failed. Failures were `TransitUITests.testClearAll`, `TransitUITests.testEditViewPreservesTaskMilestone`, and `DataMaintenanceUITests.testDataMaintenanceGoldenPath`; there were zero non-UI failure markers.+- `make test-ui`: 15 passed, 6 failed. It repeated the three failures above and also failed three Settings navigation cases. The xcresult shows `dashboard.settingsButton` in the toolbar's **More** overflow for `testSettingsHasBackChevron`; the other two Settings cases failed their existing assertions. No changed file implements these UI paths.++**Physical-device verification:** Not run. The controlled tests use independent `ModelContext` instances on one retained local store to deterministically model a peer change that has already reached the receiving device's store. They prove the post-allocation local-store guard, dirty-state behavior, deletion handling, and later-pass counter stability; they do not exercise CloudKit transport timing or the residual simultaneous-probe race.++## Prevention++- Treat mutable state checked before an `await` as expired after suspension.+- Put committed-state probes after the final suspension and adjacent to mutation.+- Do not describe SwiftData stale probes as an atomic cross-device compare-and-set.++## Related++- T-2020 — this bug.+- T-597 — process-local promotion single-flight guards.+- T-2019 — equivalent post-allocation stale-ID protection in duplicate cleanup.