Independent pre-push review: T-1939 exact head

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex 9f65d53..c26428e 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).  - T-1803: `UpdateStatusIntent` now includes a missing requested `displayId` in its `TASK_NOT_FOUND` hint (`No task with displayId N`), while missing UUIDs retain the existing generic lookup hint and malformed identifiers, duplicate IDs, status validation, and atomic mutation behavior remain unchanged. ### Fixed+- Display-ID collision guards now build candidate-blocking sets from committed task/milestone IDs fetched through a fresh transient `ModelContext`, unioned with live/pending registered main-context values and allocator-issued IDs (T-1939). The shared guard applies to task and milestone creation, provisional promotion, and duplicate repair, so a stale registered bystander can no longer hide a peer-synced committed ID. Regression coverage commits the peer value through an independent context, proves the receiving bystander remains clean and unrefreshed, and separately verifies unsaved IDs remain blocked. Either store view failing still fails closed; allocation serialization, cancellation, maintenance stale-loser probes, and selective save recovery are unchanged. - MCP `create_task` and JSON `CreateTaskIntent` now reject present non-object `metadata` values (string, array, number, boolean, or null) with field-specific errors before task insertion, while omitted metadata and T-723's tolerant handling of values inside valid objects remain unchanged (T-1991). - T-2036: `QueryTasksIntent` now treats empty `completionDate` and `lastStatusChangeDate` objects as valid no-op filters, including when both are present. Raw-object emptiness is preserved before Codable decoding, while nested nulls, malformed non-empty objects, strict dates, reversed ranges, open bounds, and relative precedence remain covered by regression tests. - MCP request validation now rejects explicit JSON `id: null` as JSON-RPC `-32600 Invalid Request` under protocol `2025-03-26`, while preserving omitted-ID notifications and valid string/integer IDs across single and batch requests (T-1863).

diff --git a/Transit/Transit/Services/DisplayIDAllocator.swift b/Transit/Transit/Services/DisplayIDAllocator.swiftindex 165e8c0..f762840 100644--- a/Transit/Transit/Services/DisplayIDAllocator.swift+++ b/Transit/Transit/Services/DisplayIDAllocator.swift@@ -211,9 +211,9 @@ final class DisplayIDAllocator: @unchecked Sendable {     /// Finds tasks with provisional display IDs (permanentDisplayId == nil),     /// sorts them by creation date, and allocates permanent IDs one at a time.     /// `save` is injectable for tests that need to simulate a save failure-    /// after the permanent ID has been assigned in memory. `usedTaskIDs` is-    /// injectable for tests that need to simulate an unreadable local store-    /// (T-1621); it defaults to reading the committed IDs from `context`.+    /// after the permanent ID has been assigned in memory. `usedTaskIDs` supplements+    /// the default committed-store and live/pending IDs; tests can inject a+    /// throwing source to verify fail-closed behavior (T-1621, T-1939).     func promoteProvisionalTasks(         in context: ModelContext,         usedTaskIDs: (@MainActor @Sendable () throws -> Set<Int>)? = nil,@@ -235,11 +235,11 @@ final class DisplayIDAllocator: @unchecked Sendable {             return         } -        // Exclude IDs already committed locally (recomputed inside the gate on-        // every attempt so just-promoted IDs are included) so promotion never-        // 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() }+        // Recompute all blockers inside the gate; any source failure fails closed (T-1395, T-1621, T-1939).+        let storedAndLiveIDs = UsedDisplayIDs(modelContext: context)+        let usedIDs: @MainActor @Sendable () throws -> Set<Int> = {+            try storedAndLiveIDs.tasks().union(usedTaskIDs?() ?? [])+        }         let recordLookup = DisplayIDRecordLookup(modelContext: context)          for task in tasks {

diff --git a/Transit/Transit/Services/DisplayIDMaintenanceService.swift b/Transit/Transit/Services/DisplayIDMaintenanceService.swiftindex edc4245..fce2e63 100644--- a/Transit/Transit/Services/DisplayIDMaintenanceService.swift+++ b/Transit/Transit/Services/DisplayIDMaintenanceService.swift@@ -24,7 +24,8 @@ final class DisplayIDMaintenanceService {         taskAllocator: DisplayIDAllocator,         milestoneAllocator: DisplayIDAllocator,         commentService: CommentService,-        clock: @escaping () -> Date = { Date.now }+        clock: @escaping () -> Date = { Date.now },+        usedIDFetcher: (any ModelFetching)? = nil     ) {         self.modelContext = modelContext         self.taskAllocator = taskAllocator@@ -32,7 +33,10 @@ final class DisplayIDMaintenanceService {         self.commentService = commentService         self.clock = clock         self.lookup = DisplayIDRecordLookup(modelContext: modelContext)-        self.usedDisplayIDs = UsedDisplayIDs(modelContext)+        self.usedDisplayIDs = UsedDisplayIDs(+            modelContext: modelContext,+            liveFetcher: usedIDFetcher ?? modelContext+        )     }      // MARK: - Scan

diff --git a/Transit/Transit/Services/MilestoneService.swift b/Transit/Transit/Services/MilestoneService.swiftindex 877e9f4..d7b9cc6 100644--- a/Transit/Transit/Services/MilestoneService.swift+++ b/Transit/Transit/Services/MilestoneService.swift@@ -10,10 +10,10 @@ final class MilestoneService {     private let modelContext: ModelContext     private let displayIDAllocator: DisplayIDAllocator -    /// Store reads for the two invariants that are derived from a fetch result —-    /// name uniqueness and the used-display-ID snapshot. Separate from-    /// `modelContext` only so tests can inject a failing fetch (T-1614, T-1621);-    /// production always passes the same context.+    /// Store reads for invariants derived from fetch results. Name uniqueness uses+    /// the live fetcher directly. Display-ID guards additionally union a transient+    /// committed-store read so peer-synced IDs cannot be hidden by cached models+    /// (T-1614, T-1621, T-1939).     private let fetcher: any ModelFetching     private let usedDisplayIDs: UsedDisplayIDs @@ -29,7 +29,7 @@ final class MilestoneService {         self.modelContext = modelContext         self.displayIDAllocator = displayIDAllocator         self.fetcher = fetcher ?? modelContext-        self.usedDisplayIDs = UsedDisplayIDs(fetcher ?? modelContext)+        self.usedDisplayIDs = UsedDisplayIDs(modelContext: modelContext, liveFetcher: self.fetcher)     }      // MARK: - CRUD

diff --git a/Transit/Transit/Services/TaskService.swift b/Transit/Transit/Services/TaskService.swiftindex 43714cc..c374269 100644--- a/Transit/Transit/Services/TaskService.swift+++ b/Transit/Transit/Services/TaskService.swift@@ -42,9 +42,10 @@ final class TaskService {     private let createSave: (ModelContext) throws -> Void     private let statusSave: (ModelContext) throws -> Void -    /// Committed display IDs feeding the allocator's collision guard. Reads through-    /// an injectable seam only so tests can simulate an unreadable store (T-1621);-    /// production reads the same context.+    /// Display IDs feeding the allocator's collision guard. Production unions a+    /// transient committed-store read with the live main-context values; the+    /// injectable live fetcher lets tests simulate stale or unreadable registered+    /// state without replacing the committed-store read (T-1621, T-1939).     private let usedDisplayIDs: UsedDisplayIDs      init(@@ -56,7 +57,10 @@ final class TaskService {     ) {         self.modelContext = modelContext         self.displayIDAllocator = displayIDAllocator-        self.usedDisplayIDs = UsedDisplayIDs(fetcher ?? modelContext)+        self.usedDisplayIDs = UsedDisplayIDs(+            modelContext: modelContext,+            liveFetcher: fetcher ?? modelContext+        )         self.createSave = createSave         self.statusSave = statusSave     }

diff --git a/Transit/Transit/Services/UsedDisplayIDs.swift b/Transit/Transit/Services/UsedDisplayIDs.swiftindex d9b9471..9765295 100644--- a/Transit/Transit/Services/UsedDisplayIDs.swift+++ b/Transit/Transit/Services/UsedDisplayIDs.swift@@ -1,36 +1,49 @@ import Foundation import SwiftData -/// The permanent display IDs already committed to the local store.+/// The permanent display IDs that must block an allocation candidate. ///-/// Supplied to `DisplayIDAllocator.allocateNextID(excluding:)`, where it is the-/// collision guard against a stale or stuck CloudKit counter (T-1395): the-/// allocator skips past any candidate this set already contains.+/// SwiftData's main `ModelContext` can keep registered `@Model` values that lag+/// peer changes already committed to the local store. It is still authoritative+/// for live/pending values that have not been saved. Build the guard from both:+/// a fresh transient context reads committed rows without registered-object+/// caching, then the live fetcher contributes current in-process values (T-1939). ///-/// A fetch failure **throws** rather than degrading to an empty set. An empty set-/// is indistinguishable from "no IDs are in use", which silently disables the-/// guard and lets the allocator hand back an ID a local record already holds —-/// and SwiftData + CloudKit cannot express `@Attribute(.unique)`, so nothing-/// downstream would catch the duplicate (T-1621).+/// `DisplayIDAllocator` unions this set with IDs it has issued but not yet seen+/// committed. Together those three sources close the stale counter and+/// allocate-before-save collision windows.+///+/// Either fetch failing **throws** rather than degrading to an empty/partial set.+/// A partial set silently disables part of the uniqueness guard, and SwiftData ++/// CloudKit cannot express `@Attribute(.unique)` to catch a duplicate later+/// (T-1621). struct UsedDisplayIDs { -    private let fetcher: any ModelFetching+    private let modelContext: ModelContext+    private let liveFetcher: any ModelFetching -    init(_ fetcher: any ModelFetching) {-        self.fetcher = fetcher+    init(modelContext: ModelContext, liveFetcher: (any ModelFetching)? = nil) {+        self.modelContext = modelContext+        self.liveFetcher = liveFetcher ?? modelContext     }      func tasks() throws -> Set<Int> {         let descriptor = FetchDescriptor<TransitTask>(             predicate: #Predicate { $0.permanentDisplayId != nil }         )-        return Set(try fetcher.fetch(descriptor).compactMap(\.permanentDisplayId))+        let committedContext = ModelContext(modelContext.container)+        let committed = Set(try committedContext.fetch(descriptor).compactMap(\.permanentDisplayId))+        let live = Set(try liveFetcher.fetch(descriptor).compactMap(\.permanentDisplayId))+        return committed.union(live)     }      func milestones() throws -> Set<Int> {         let descriptor = FetchDescriptor<Milestone>(             predicate: #Predicate { $0.permanentDisplayId != nil }         )-        return Set(try fetcher.fetch(descriptor).compactMap(\.permanentDisplayId))+        let committedContext = ModelContext(modelContext.container)+        let committed = Set(try committedContext.fetch(descriptor).compactMap(\.permanentDisplayId))+        let live = Set(try liveFetcher.fetch(descriptor).compactMap(\.permanentDisplayId))+        return committed.union(live)     } }

diff --git a/Transit/TransitTests/StaleRegisteredBystanderDisplayIDTests.swift b/Transit/TransitTests/StaleRegisteredBystanderDisplayIDTests.swiftnew file mode 100644index 0000000..c873450--- /dev/null+++ b/Transit/TransitTests/StaleRegisteredBystanderDisplayIDTests.swift@@ -0,0 +1,387 @@+import Foundation+import SwiftData+import Testing+@testable import Transit++// swiftlint:disable type_body_length++/// Regression coverage for T-1939. An independent peer context commits display+/// ID 10 while the receiving context keeps a clean registered value of 9. This+/// models the stale peer-merge snapshot directly and separately proves that+/// unsaved live IDs remain blocked by the combined collision set.+@MainActor+@Suite(.serialized)+struct StaleRegisteredBystanderDisplayIDTests {++    /// Deterministic live-context snapshot used after an independent peer commit.+    /// The models are the receiving context's actual registered objects, not+    /// detached substitutes, so the fixture proves they are clean and stale+    /// before this seam prevents a test fetch from refreshing them.+    private struct RegisteredSnapshotFetcher: ModelFetching {+        let tasks: [TransitTask]+        let milestones: [Milestone]++        func fetch<T: PersistentModel>(_ descriptor: FetchDescriptor<T>) throws -> [T] {+            if T.self == TransitTask.self {+                return tasks.compactMap { $0 as? T }+            }+            if T.self == Milestone.self {+                return milestones.compactMap { $0 as? T }+            }+            return []+        }+    }++    private struct Environment {+        let container: ModelContainer+        let context: ModelContext+        let project: Project+        let taskAllocator: DisplayIDAllocator+        let milestoneAllocator: DisplayIDAllocator+    }++    private func makeEnvironment() throws -> Environment {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let project = Project(name: "Test", description: "", gitRepo: nil, colorHex: "#FF0000")+        context.insert(project)++        let taskAllocator = DisplayIDAllocator(+            store: InMemoryCounterStore(initialNextDisplayID: 10)+        )+        let milestoneAllocator = DisplayIDAllocator(+            store: InMemoryCounterStore(initialNextDisplayID: 10)+        )+        return Environment(+            container: testContainer.container,+            context: context,+            project: project,+            taskAllocator: taskAllocator,+            milestoneAllocator: milestoneAllocator+        )+    }++    @discardableResult+    private func insertTask(+        in environment: Environment,+        name: String,+        displayID: DisplayID,+        creationDate: Date = .now+    ) -> TransitTask {+        let task = TransitTask(+            name: name,+            type: .feature,+            project: environment.project,+            displayID: displayID+        )+        task.creationDate = creationDate+        environment.context.insert(task)+        return task+    }++    @discardableResult+    private func insertMilestone(+        in environment: Environment,+        name: String,+        displayID: DisplayID,+        creationDate: Date = .now+    ) -> Milestone {+        let milestone = Milestone(+            name: name,+            project: environment.project,+            displayID: displayID+        )+        milestone.creationDate = creationDate+        environment.context.insert(milestone)+        return milestone+    }++    private func commitPeerTaskUpdateKeepingRegisteredBystanderStale(+        _ task: TransitTask,+        in environment: Environment+    ) throws {+        let taskID = task.id+        let peerContext = ModelContext(environment.container)+        let peerTask = try #require(try peerContext.fetch(FetchDescriptor<TransitTask>(+            predicate: #Predicate { $0.id == taskID }+        )).first)+        peerTask.permanentDisplayId = 10+        try peerContext.save()++        #expect(task.permanentDisplayId == 9,+                "The registered bystander must remain stale after the peer commit")+        #expect(!environment.context.hasChanges,+                "The stale bystander must be clean, not an unsaved local edit")+        #expect(try storedTaskIDs(in: environment.container).contains(10),+                "A transient context must observe the peer-committed ID")+    }++    private func commitPeerMilestoneUpdateKeepingRegisteredBystanderStale(+        _ milestone: Milestone,+        in environment: Environment+    ) throws {+        let milestoneID = milestone.id+        let peerContext = ModelContext(environment.container)+        let peerMilestone = try #require(try peerContext.fetch(FetchDescriptor<Milestone>(+            predicate: #Predicate { $0.id == milestoneID }+        )).first)+        peerMilestone.permanentDisplayId = 10+        try peerContext.save()++        #expect(milestone.permanentDisplayId == 9,+                "The registered bystander must remain stale after the peer commit")+        #expect(!environment.context.hasChanges,+                "The stale bystander must be clean, not an unsaved local edit")+        #expect(try storedMilestoneIDs(in: environment.container).contains(10),+                "A transient context must observe the peer-committed ID")+    }++    private func storedTaskIDs(in container: ModelContainer) throws -> [Int] {+        let probe = ModelContext(container)+        return try probe.fetch(FetchDescriptor<TransitTask>()).compactMap(\.permanentDisplayId)+    }++    private func storedMilestoneIDs(in container: ModelContainer) throws -> [Int] {+        let probe = ModelContext(container)+        return try probe.fetch(FetchDescriptor<Milestone>()).compactMap(\.permanentDisplayId)+    }++    @Test func taskGuardUnionsPeerCommittedAndLivePendingIDs() throws {+        let environment = try makeEnvironment()+        let bystander = insertTask(+            in: environment,+            name: "Bystander",+            displayID: .permanent(9)+        )+        try environment.context.save()+        try commitPeerTaskUpdateKeepingRegisteredBystanderStale(bystander, in: environment)++        let pending = insertTask(in: environment, name: "Pending", displayID: .permanent(11))+        let live = RegisteredSnapshotFetcher(tasks: [bystander, pending], milestones: [])+        let ids = try UsedDisplayIDs(+            modelContext: environment.context,+            liveFetcher: live+        ).tasks()++        #expect(ids == [9, 10, 11],+                "The guard must union stale live, peer-committed, and unsaved IDs")+        #expect(bystander.permanentDisplayId == 9,+                "Building the guard must not refresh the registered bystander")+        #expect(environment.context.hasChanges,+                "The pending ID must remain unsaved after the read")+    }++    @Test func milestoneGuardUnionsPeerCommittedAndLivePendingIDs() throws {+        let environment = try makeEnvironment()+        let bystander = insertMilestone(+            in: environment,+            name: "Bystander",+            displayID: .permanent(9)+        )+        try environment.context.save()+        try commitPeerMilestoneUpdateKeepingRegisteredBystanderStale(bystander, in: environment)++        let pending = insertMilestone(in: environment, name: "Pending", displayID: .permanent(11))+        let live = RegisteredSnapshotFetcher(tasks: [], milestones: [bystander, pending])+        let ids = try UsedDisplayIDs(+            modelContext: environment.context,+            liveFetcher: live+        ).milestones()++        #expect(ids == [9, 10, 11],+                "The guard must union stale live, peer-committed, and unsaved IDs")+        #expect(bystander.permanentDisplayId == 9,+                "Building the guard must not refresh the registered bystander")+        #expect(environment.context.hasChanges,+                "The pending ID must remain unsaved after the read")+    }++    @Test func taskCreationBlocksPeerCommittedIDHiddenByStaleRegisteredBystander() async throws {+        let environment = try makeEnvironment()+        let bystander = insertTask(+            in: environment,+            name: "Bystander",+            displayID: .permanent(9)+        )+        try environment.context.save()+        try commitPeerTaskUpdateKeepingRegisteredBystanderStale(bystander, in: environment)++        let taskService = TaskService(+            modelContext: environment.context,+            displayIDAllocator: environment.taskAllocator,+            fetcher: RegisteredSnapshotFetcher(tasks: [bystander], milestones: [])+        )+        let created = try await taskService.createTask(+            name: "Created",+            description: nil,+            type: .feature,+            project: environment.project+        )++        #expect(created.permanentDisplayId == 11)+        let ids = try storedTaskIDs(in: environment.container)+        #expect(Set(ids).count == ids.count, "Creation must not duplicate the peer-committed ID")+    }++    @Test func milestoneCreationBlocksPeerCommittedIDHiddenByStaleRegisteredBystander() async throws {+        let environment = try makeEnvironment()+        let bystander = insertMilestone(+            in: environment,+            name: "Bystander",+            displayID: .permanent(9)+        )+        try environment.context.save()+        try commitPeerMilestoneUpdateKeepingRegisteredBystanderStale(bystander, in: environment)++        let milestoneService = MilestoneService(+            modelContext: environment.context,+            displayIDAllocator: environment.milestoneAllocator,+            fetcher: RegisteredSnapshotFetcher(tasks: [], milestones: [bystander])+        )+        let created = try await milestoneService.createMilestone(+            name: "Created",+            description: nil,+            project: environment.project+        )++        #expect(created.permanentDisplayId == 11)+        let ids = try storedMilestoneIDs(in: environment.container)+        #expect(Set(ids).count == ids.count, "Creation must not duplicate the peer-committed ID")+    }++    @Test func taskPromotionBlocksPeerCommittedIDHiddenByStaleRegisteredBystander() async throws {+        let environment = try makeEnvironment()+        let bystander = insertTask(+            in: environment,+            name: "Bystander",+            displayID: .permanent(9)+        )+        let provisional = insertTask(+            in: environment,+            name: "Provisional",+            displayID: .provisional+        )+        try environment.context.save()+        try commitPeerTaskUpdateKeepingRegisteredBystanderStale(bystander, in: environment)++        await environment.taskAllocator.promoteProvisionalTasks(+            in: environment.context,+            usedTaskIDs: { [9] }+        )++        #expect(provisional.permanentDisplayId == 11)+        let ids = try storedTaskIDs(in: environment.container)+        #expect(Set(ids).count == ids.count, "Promotion must not duplicate the peer-committed ID")+    }++    @Test func milestonePromotionBlocksPeerCommittedIDHiddenByStaleRegisteredBystander() async throws {+        let environment = try makeEnvironment()+        let bystander = insertMilestone(+            in: environment,+            name: "Bystander",+            displayID: .permanent(9)+        )+        let provisional = insertMilestone(+            in: environment,+            name: "Provisional",+            displayID: .provisional+        )+        try environment.context.save()+        try commitPeerMilestoneUpdateKeepingRegisteredBystanderStale(bystander, in: environment)++        let milestoneService = MilestoneService(+            modelContext: environment.context,+            displayIDAllocator: environment.milestoneAllocator,+            fetcher: RegisteredSnapshotFetcher(tasks: [], milestones: [bystander])+        )+        await milestoneService.promoteProvisionalMilestones()++        #expect(provisional.permanentDisplayId == 11)+        let ids = try storedMilestoneIDs(in: environment.container)+        #expect(Set(ids).count == ids.count, "Promotion must not duplicate the peer-committed ID")+    }++    @Test func taskRepairBlocksPeerCommittedIDHiddenByStaleRegisteredBystander() async throws {+        let environment = try makeEnvironment()+        insertTask(+            in: environment,+            name: "Winner",+            displayID: .permanent(5),+            creationDate: Date(timeIntervalSince1970: 1_000)+        )+        insertTask(+            in: environment,+            name: "Loser",+            displayID: .permanent(5),+            creationDate: Date(timeIntervalSince1970: 2_000)+        )+        let bystander = insertTask(+            in: environment,+            name: "Bystander",+            displayID: .permanent(9),+            creationDate: Date(timeIntervalSince1970: 3_000)+        )+        try environment.context.save()+        try commitPeerTaskUpdateKeepingRegisteredBystanderStale(bystander, in: environment)++        let maintenanceService = DisplayIDMaintenanceService(+            modelContext: environment.context,+            taskAllocator: DisplayIDAllocator(+                store: StaleReadCounterStore(staleValue: 10, staleReads: 3)+            ),+            milestoneAllocator: environment.milestoneAllocator,+            commentService: CommentService(modelContext: environment.context),+            usedIDFetcher: RegisteredSnapshotFetcher(tasks: [bystander], milestones: [])+        )+        let result = await maintenanceService.reassignDuplicates()++        let group = try #require(result.groups.first(where: { $0.type == .task }))+        #expect(group.failure == nil)+        #expect(group.reassignments.first?.newDisplayId == 11)+        let ids = try storedTaskIDs(in: environment.container)+        #expect(Set(ids).count == ids.count, "Repair must not duplicate the peer-committed ID")+    }++    @Test func milestoneRepairBlocksPeerCommittedIDHiddenByStaleRegisteredBystander() async throws {+        let environment = try makeEnvironment()+        insertMilestone(+            in: environment,+            name: "Winner",+            displayID: .permanent(5),+            creationDate: Date(timeIntervalSince1970: 1_000)+        )+        insertMilestone(+            in: environment,+            name: "Loser",+            displayID: .permanent(5),+            creationDate: Date(timeIntervalSince1970: 2_000)+        )+        let bystander = insertMilestone(+            in: environment,+            name: "Bystander",+            displayID: .permanent(9),+            creationDate: Date(timeIntervalSince1970: 3_000)+        )+        try environment.context.save()+        try commitPeerMilestoneUpdateKeepingRegisteredBystanderStale(bystander, in: environment)++        let maintenanceService = DisplayIDMaintenanceService(+            modelContext: environment.context,+            taskAllocator: environment.taskAllocator,+            milestoneAllocator: DisplayIDAllocator(+                store: StaleReadCounterStore(staleValue: 10, staleReads: 3)+            ),+            commentService: CommentService(modelContext: environment.context),+            usedIDFetcher: RegisteredSnapshotFetcher(tasks: [], milestones: [bystander])+        )+        let result = await maintenanceService.reassignDuplicates()++        let group = try #require(result.groups.first(where: { $0.type == .milestone }))+        #expect(group.failure == nil)+        #expect(group.reassignments.first?.newDisplayId == 11)+        let ids = try storedMilestoneIDs(in: environment.container)+        #expect(Set(ids).count == ids.count, "Repair must not duplicate the peer-committed ID")+    }+}++// swiftlint:enable type_body_length

diff --git a/docs/agent-notes/display-id-maintenance.md b/docs/agent-notes/display-id-maintenance.mdindex 1e8896a..c4ad05c 100644--- a/docs/agent-notes/display-id-maintenance.md+++ b/docs/agent-notes/display-id-maintenance.md@@ -9,6 +9,7 @@  ## Current gotchas +- Every allocator `excluding:` closure must use `UsedDisplayIDs`, which unions committed IDs from a fresh transient `ModelContext(modelContext.container)` with live/pending values from the registered main context (T-1939). Neither view subsumes the other: the main context can lag a peer-merged store row, while the transient context cannot see unsaved local models/edits. `DisplayIDAllocator` adds its in-process issued/reserved IDs inside the serialized allocation gate. The injectable `ModelFetching` seam represents the live view only; it must never replace the transient committed read. Any source failing continues to fail closed per T-1621. - The stale-ID guard protects against peer devices changing a loser between scan and write. Per Decision 12, it reads the loser's committed `permanentDisplayId` through a *transient* `ModelContext(modelContext.container)`. The transient context has no registered objects, so its fetch bypasses the main context's scan-time snapshot and reads directly from the local SQLite row — including any CloudKit-merged peer change. SwiftData has **no** public `ModelContext.refresh(_:mergeChanges:)`; that API is Core Data only. Each loser is probed both before allocation and again after `allocateNextID` returns, immediately before mutation/save: allocation suspends, so the first probe alone can expire if a peer update lands during the await (T-2019). A second-probe mismatch returns `stale-id`, deliberately skips the allocated counter value, and creates no task audit comment. - The single-flight test (`secondConcurrentCallReturnsBusy`) interleaves two `Task`s using `Task.yield()`, which is not a hard ordering guarantee. The test passes consistently in practice; `AllocationGatedCounterStore` in the same file (added for T-2019) is the deterministic gate to reuse when hardening it — do not build a second one. That gate parks on the *third* `loadCounter`, which encodes that `advanceCounterIfNeeded` performs exactly two reads; its tests assert the counter ended at 101 so a changed call sequence fails loudly instead of silently gating in the wrong place. - The stale-ID guard's positive path is now covered by `peerUpdatedLoserIsSkippedWithStaleId`, which fakes the cached-stale state by saving a value to the store and then mutating the registered instance in memory before calling `reassignDuplicates`. `staleIdSkipsGroupWithoutWriting` remains as a coverage of the "scan no longer sees the duplicate" outcome.

diff --git a/specs/bugfixes/display-id-collision-guards-miss-peer-synced-ids/report.md b/specs/bugfixes/display-id-collision-guards-miss-peer-synced-ids/report.mdnew file mode 100644index 0000000..68559da--- /dev/null+++ b/specs/bugfixes/display-id-collision-guards-miss-peer-synced-ids/report.md@@ -0,0 +1,147 @@+# Bugfix Report: Display ID Collision Guards Miss Peer-Synced IDs++**Date:** 2026-08-03+**Status:** Fixed+**Ticket:** T-1939++## Description of the Issue++Display-ID allocation asks each caller for IDs already in use before accepting a counter candidate. Every production closure currently fetches through the shared main `ModelContext`. When that context has a registered `@Model` whose cached display ID predates a peer merge, the fetch can return the cached value instead of the newer committed row. A stale counter can then offer the peer-committed ID and the guard accepts it.++**Reproduction steps:**+1. Register a task or milestone carrying display ID 9 in the main context.+2. Through a peer context on the same store, commit display ID 10 for that UUID while the main registered object still reads 9 and has no pending changes.+3. Make the counter offer 10 during creation, provisional promotion, or duplicate repair.+4. Observe the existing main-context-only guard accept 10 and assign it to another record.++**Impact:** Task and milestone creation, provisional promotion, and duplicate repair can report success while creating a duplicate human-facing T-/M- identifier. SwiftData + CloudKit cannot enforce `@Attribute(.unique)`, so there is no downstream uniqueness constraint to reject the write.++## Investigation Summary++### Phase 1: Initial overview++- **Expected:** A candidate is blocked when either the committed local store, live/pending main-context state, or allocator-issued state already uses it.+- **Actual:** Caller closures supply only main-context fetch results; allocator-issued IDs are unioned separately inside `DisplayIDAllocator`.+- **Context:** The defect appears after a peer/CloudKit merge reaches the persistent store while a registered main-context model remains cached at its older value.++### Phase 2: Systematic inspection++- **Data-flow defect:** `UsedDisplayIDs` accepts one `ModelFetching` and treats that single view as authoritative.+- **Affected call sites:** `TaskService.createTask`, `MilestoneService.createMilestone`, `DisplayIDAllocator.promoteProvisionalTasks`, `MilestoneService.promoteProvisionalMilestones`, and both task/milestone branches of `DisplayIDMaintenanceService.reassignDuplicates`.+- **Existing compatible pattern:** `DisplayIDRecordLookup` uses a fresh transient `ModelContext(modelContext.container)` to bypass registered-object snapshots for T-1061/T-2019/T-2020 probes.+- **Preserved safeguards:** Allocation serialization and issued-ID reservation live inside `DisplayIDAllocator`; loser post-allocation re-probes, promotion precondition probes, cancellation checks, selective save recovery, single-flight maintenance guards, and throwing fetch semantics are independent and must remain unchanged.++### Phase 3: Root cause analysis++**Defect type:** Stale-context data-flow error.++**Five Whys:**+1. Why can a used ID be reissued? The collision set does not contain the committed candidate.+2. Why is it absent? The used-ID closure sees a registered object's cached pre-merge value.+3. Why does the closure trust that value? It fetches only through the main context.+4. Why is the main context insufficient? SwiftData can retain registered `@Model` values after another context commits a newer row, and it exposes no public per-object refresh API.+5. Why do existing transient probes not prevent this? They validate only the loser/provisional target, not unrelated bystanders that occupy the allocator candidate.++**Root cause:** Candidate blocking incorrectly treats one cached main-context view as the committed-store truth. The correct set must combine independent committed-store and live/pending views.++### Phase 4: Solution and verification plan++1. Make `UsedDisplayIDs` build each set from a fresh transient-context committed fetch unioned with the live/pending main-context fetch.+2. Preserve the injectable failing-fetch seam so either required view failing still fails closed as T-1621 requires.+3. Keep allocator `issuedIDs` unioning unchanged so uncommitted allocations remain reserved.+4. Route every affected creation, promotion, and maintenance call through the same helper.+5. Add deterministic stale-registered-bystander regressions for task and milestone creation, promotion, and repair.+6. Re-run T-1061/T-1621/T-1766/T-2019/T-2020, cancellation, rollback, and full unit/lint validation.++## Discovered Root Cause++`UsedDisplayIDs` reads only the registered main `ModelContext`. That context is the correct source for live/pending in-process values but is not a reliable source for peer-committed store values. Unrelated transient probes do not add bystander IDs to the allocation candidate set.++**Defect type:** Stale cache / missing authoritative store read.++**Contributing factors:** SwiftData has no uniqueness constraint under CloudKit and no public targeted refresh API; display-ID assignment spans a direct CloudKit counter and SwiftData model save.++## Resolution for the Issue++**Changes made:**+- `Transit/Transit/Services/UsedDisplayIDs.swift` — each task/milestone snapshot now unions committed IDs from a newly created transient context with live/pending IDs from the registered main context. Either fetch throwing aborts the snapshot.+- `Transit/Transit/Services/TaskService.swift` — task creation supplies its main/injected live view to the shared two-source helper.+- `Transit/Transit/Services/MilestoneService.swift` — milestone creation and promotion use the same helper while name uniqueness continues to use its existing fetcher.+- `Transit/Transit/Services/DisplayIDAllocator.swift` — task promotion always includes the shared committed + live set; its injectable test set supplements rather than replaces those sources. Allocator-issued IDs remain unioned inside the serialized allocation gate.+- `Transit/Transit/Services/DisplayIDMaintenanceService.swift` — both duplicate-repair paths use the shared helper; an optional live-view seam lets tests supply the receiving context's actual clean stale registered objects while production uses the main context.+- `Transit/TransitTests/StaleRegisteredBystanderDisplayIDTests.swift` — six task/milestone path regressions cover creation, promotion, and repair; two focused helper tests prove the committed/live/pending union directly.++**Approach rationale:** A transient context is the smallest authoritative committed-store read SwiftData exposes, while the main context remains necessary for unsaved/pending values. Unioning the two in the existing shared `UsedDisplayIDs` helper fixes all callers without changing allocator serialization or mutation control flow. `DisplayIDAllocator` already owns the third source—issued but not yet committed IDs—so that reservation remains inside its gate.++**Alternatives considered:**+- **Refresh the main context before each allocation:** Rejected because SwiftData exposes no public targeted refresh API, and a broad refetch can overwrite or hide legitimate pending values.+- **Use only a transient context:** Rejected because it cannot see newly inserted or edited main-context IDs that have not been committed yet.+- **Add a second collision check after allocation:** Rejected as duplication at six call paths; the allocator already evaluates its exclusion closure inside the serialized gate on every retry.+- **Change CloudKit schema or introduce owner reservations:** Unnecessary for this local stale-store-view defect and materially larger than the shared snapshot fix.++**Preserved behavior:** T-1061/T-2019 loser probes and post-allocation re-probes, T-1621 fail-closed lookup semantics, T-1766 maintenance guards, allocator serialization and issued-ID reservation, cancellation propagation/checks, single-flight guards, and failure rollback/selective reset paths are unchanged.++## Regression Test++**Test file:** `Transit/TransitTests/StaleRegisteredBystanderDisplayIDTests.swift`++**Test names:**+- `taskGuardUnionsPeerCommittedAndLivePendingIDs`+- `milestoneGuardUnionsPeerCommittedAndLivePendingIDs`+- `taskCreationBlocksPeerCommittedIDHiddenByStaleRegisteredBystander`+- `milestoneCreationBlocksPeerCommittedIDHiddenByStaleRegisteredBystander`+- `taskPromotionBlocksPeerCommittedIDHiddenByStaleRegisteredBystander`+- `milestonePromotionBlocksPeerCommittedIDHiddenByStaleRegisteredBystander`+- `taskRepairBlocksPeerCommittedIDHiddenByStaleRegisteredBystander`+- `milestoneRepairBlocksPeerCommittedIDHiddenByStaleRegisteredBystander`++**What they verify:** An independent peer `ModelContext` commits ID 10 while the receiving context retains a registered ID 9. Before every path test, assertions prove the registered value is still 9, the receiving context has no pending changes, and a fresh transient probe sees committed ID 10—so the fixture is a clean stale bystander, not a synthetic unsaved edit or refreshed object. A counter offering 10 must skip to 11 on all six affected paths. Two focused tests then add an unsaved ID 11 and prove `UsedDisplayIDs` returns the complete `{9, 10, 11}` union without refreshing or saving the receiving context.++**Run command:** `make test-quick`++## Affected Files++| File | Change |+|------|--------|+| `Transit/Transit/Services/UsedDisplayIDs.swift` | Shared committed transient + live/pending ID union |+| `Transit/Transit/Services/TaskService.swift` | Task-creation wiring |+| `Transit/Transit/Services/MilestoneService.swift` | Milestone creation/promotion wiring |+| `Transit/Transit/Services/DisplayIDAllocator.swift` | Task-promotion shared set plus injected supplement |+| `Transit/Transit/Services/DisplayIDMaintenanceService.swift` | Duplicate-repair wiring and injectable live-snapshot seam |+| `Transit/TransitTests/StaleRegisteredBystanderDisplayIDTests.swift` | Six end-to-end stale-bystander regressions plus two focused union tests |+| `docs/agent-notes/display-id-maintenance.md` | Project-specific candidate-set invariant |+| `CHANGELOG.md` | Unreleased T-1939 behavior |++## Verification++**Automated:**+- [x] Red baseline: the original six T-1939 path cases accepted ID 10 before the fix (verified from the macOS xcresult bundle).+- [x] Review hardening: all six path regressions now use independent peer-context commits and assert the receiving registered bystander remains clean and unrefreshed; two focused tests separately prove pending IDs are unioned.+- [x] Green regression/full macOS unit suite after review hardening: `make test-quick` — 1,700 passed, 0 failed, 0 skipped.+- [x] Full iOS run: `make test` — 1,210 passed and 3 unrelated UI failures; all unit tests and all six T-1939 cases passed.+- [x] Linters/validators: `make lint` — SwiftLint strict mode and the SwiftData ownership guard passed.+- [ ] Dedicated UI suite is not fully green: `make test-ui` — 18 passed, 3 failed.++**Unrelated UI failures:**+- `TransitUITests.testClearAll`+- `TransitUITests.testEditViewPreservesTaskMilestone`+- `DataMaintenanceUITests.testDataMaintenanceGoldenPath` — XCTest reports duplicate matching accessibility elements for the confirmation button.++The same three failures are documented on the base branch in the T-2020 bug report, and none of the T-1939 changes touch views, UI tests, or accessibility identifiers. Both long iOS commands exceeded the CLI wrapper's timeout after Xcode had finalized their result bundles; counts and failures above were read directly with `xcresulttool`.++**Manual review:** The diff changes only candidate-set construction and dependency wiring. T-1061/T-2019/T-2020 probes, allocation gate/issued-ID state, cancellation checks, maintenance single-flight handling, and save rollback/selective reset blocks are byte-for-byte unchanged.++## Prevention++- Treat a registered main-context fetch as live/pending state, not as an authoritative committed-store snapshot.+- Centralize display-ID candidate blocking so every allocator caller combines the same state sources.+- Establish stale-cache regressions with an independent peer context, assert the receiving context stays clean, and probe committed state through another transient context without refreshing the registered bystander.++## Related++- T-1061 — transient committed loser probes.+- T-1395 — allocation serialization and allocator-issued ID reservation.+- T-1621 — used-ID fetch failures fail closed.+- T-1766 — maintenance collision guards.+- T-2019 — duplicate-repair post-allocation loser re-probe.+- T-2020 — promotion post-allocation provisional-state re-probe.