Pre-push review: Transit PR #195 / T-2019

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex 7870fe7..1b441b7 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -6,10 +6,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).  ## [Unreleased]  ### Fixed +- Duplicate cleanup now re-probes a task or milestone loser's committed display ID after asynchronous allocation and immediately before mutation (T-2019). Peer changes that land during allocation are preserved and reported as `stale-id`; task cleanup no longer emits a false audit comment for an overwritten peer repair. Deterministic gated regressions cover both record types and the task audit path.+ - SwiftData test fixtures now retain their backing `ModelContainer` for the full test lifetime (T-2003). `TestModelContainer` is a container-owning fixture instead of a bare-context factory, 207 context-acquisition call sites across 92 test files were migrated, and bespoke context helpers in task-entity, task-creation-result, and report tests now delegate to the shared fixture. An escaped-context lifetime regression plus an executable ownership guard with positive/negative fixtures prevent raw container/context factories from recurring.  - Cross-device milestone name conflicts no longer make name-based operations target an arbitrary record (T-1938). `MilestoneService.findByName` now throws on multiple project-scoped matches; all MCP and App Intent callers report the ambiguity (`AMBIGUOUS_MILESTONE` for intents). Launch/foreground/connectivity maintenance deterministically keeps the oldest milestone name and renames other UUID-distinct records with UUID-derived suffixes, preserving records and task assignments. Cross-context tests simulate CloudKit imports and verify ambiguity reporting, reconciliation, idempotence, and data preservation. - Task mutation surfaces no longer hide duplicate task display IDs behind a not-found error (T-1837). `TaskService.findByDisplayID` already threw `duplicateDisplayID` when several tasks share a `permanentDisplayId` (possible under CloudKit, which cannot enforce uniqueness), but MCP `update_task_status` / `update_task` / `add_comment`, `UpdateStatusIntent`, `IntentHelpers.resolveTask`, and the visual `AddCommentIntent` all collapsed it into `TASK_NOT_FOUND` / "Provide either displayId or taskId", so operators could not tell missing data from store corruption needing display-ID maintenance. Duplicates now report `INTERNAL_ERROR` with the hint `Duplicate task identifier for displayId N` (App Intents — matching the milestone paths and the T-1097 `QueryTasksIntent` fix) or an `isError` result reading `Duplicate task identifier detected for displayId N` (MCP — matching the milestone tools). MCP `query_tasks` uses the same wording instead of leaking `Lookup failed: duplicateDisplayID`. No new error code was introduced. The three MCP call sites now share `resolveTaskArgument`, `UpdateStatusIntent` routes through `IntentHelpers.resolveTask` instead of its own copy of the mapping, and `VisualIntentError` gains a `duplicateIdentifier` case (code `INTERNAL_ERROR`). Regression coverage in `DuplicateTaskDisplayIDIntentTests` and `MCPDuplicateTaskDisplayIDTests`. - The MCP `add_comment` handler now distinguishes a missing required field from a present-but-non-string one for `content` and `authorName` (T-1579). Previously `guard let value = args[key] as? String, !value.isEmpty` collapsed numbers, booleans, arrays, and null into `Missing required argument: content` / `Missing required argument: authorName`, misleading callers into thinking they omitted a field they actually supplied with the wrong type. A present non-string value now returns the field-specific `content must be a string` / `authorName must be a string` before any task resolution or comment creation, matching the hardened pattern used by `update_task_status` after T-1205. Extracted a `requiredString(_:key:)` helper to keep `handleAddComment` within the cyclomatic-complexity limit. Regression coverage added in `MCPAddCommentTypeValidationTests` (numeric/boolean/array/null `content` and `authorName`).

diff --git a/Transit/Transit/Services/DisplayIDMaintenanceService.swift b/Transit/Transit/Services/DisplayIDMaintenanceService.swiftindex d16d174..edc4245 100644--- a/Transit/Transit/Services/DisplayIDMaintenanceService.swift+++ b/Transit/Transit/Services/DisplayIDMaintenanceService.swift@@ -204,10 +204,15 @@ final class DisplayIDMaintenanceService {             )         }         return await reassignMilestoneGroup(displayId: group.displayId, winner: winner, losers: losers)     } +    /// Message surfaced when a loser's committed display ID no longer matches the+    /// scanned value — including when the record is gone. Reported by both the+    /// pre-allocation and post-allocation probes.+    static let staleIdMessage = "Display ID changed since scan"+     /// Message surfaced per group when duplicate cleanup is attempted with iCloud Sync     /// off. Reassignment needs a fresh ID from the CloudKit counter, which is off limits     /// in that mode (T-1797), so the run reports rather than silently doing nothing.     static let cloudSyncInactiveWarning = """         iCloud Sync is off, so the display ID counter is unavailable. Turn iCloud Sync on, \@@ -264,25 +269,28 @@ final class DisplayIDMaintenanceService {      private func reassignTaskLoser(loser: RecordRef, displayId: Int) async -> LoserOutcome {         guard let loserTask = lookup.task(id: loser.id) else {             return .failed(GroupFailure(code: .staleId, message: "Task not found"))         }-        guard let storedId = lookup.storedTaskDisplayId(id: loser.id) else {-            return .failed(GroupFailure(code: .staleId, message: "Display ID changed since scan"))-        }-        if storedId != displayId {-            return .failed(GroupFailure(code: .staleId, message: "Display ID changed since scan"))+        guard lookup.storedTaskDisplayId(id: loser.id) == displayId else {+            return .failed(GroupFailure(code: .staleId, message: Self.staleIdMessage))         }         let newId: Int         do {             // Exclude every committed ID, not just the duplicate being repaired: the             // counter-advance fence above can be undone by a stale counter read, and             // handing back an in-use ID would trade one collision for another (T-1766).             newId = try await taskAllocator.allocateNextID(excluding: { try self.usedDisplayIDs.tasks() })         } catch {             return .failed(GroupFailure(code: .allocationFailed, message: error.localizedDescription))         }+        // Allocation suspends, so the committed-ID probe above can expire while+        // waiting. Re-probe after the final await and adjacent to the write; a+        // skipped counter value is safer than overwriting a peer repair (T-2019).+        guard lookup.storedTaskDisplayId(id: loser.id) == displayId else {+            return .failed(GroupFailure(code: .staleId, message: Self.staleIdMessage))+        }         loserTask.permanentDisplayId = newId         do {             try modelContext.saveOrRollback()         } catch {             return .failed(GroupFailure(code: .saveFailed, message: error.localizedDescription))@@ -320,16 +328,12 @@ final class DisplayIDMaintenanceService {             // Re-fetch to pick up any peer-merged changes since the scan.             guard let loserMilestone = lookup.milestone(id: loser.id) else {                 failure = GroupFailure(code: .staleId, message: "Milestone not found")                 break             }-            guard let storedId = lookup.storedMilestoneDisplayId(id: loser.id) else {-                failure = GroupFailure(code: .staleId, message: "Display ID changed since scan")-                break-            }-            if storedId != displayId {-                failure = GroupFailure(code: .staleId, message: "Display ID changed since scan")+            guard lookup.storedMilestoneDisplayId(id: loser.id) == displayId else {+                failure = GroupFailure(code: .staleId, message: Self.staleIdMessage)                 break             }              let newId: Int             do {@@ -339,10 +343,16 @@ final class DisplayIDMaintenanceService {             } catch {                 failure = GroupFailure(code: .allocationFailed, message: error.localizedDescription)                 break             } +            // `allocateNextID` is the final suspension point. Validate the+            // committed loser again before mutating the scan-time object (T-2019).+            guard lookup.storedMilestoneDisplayId(id: loser.id) == displayId else {+                failure = GroupFailure(code: .staleId, message: Self.staleIdMessage)+                break+            }             let previousId = displayId             loserMilestone.permanentDisplayId = newId             do {                 try modelContext.saveOrRollback()             } catch {

diff --git a/Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swift b/Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swiftindex 87a79c4..e54bf16 100644--- a/Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swift+++ b/Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swift@@ -17,10 +17,213 @@ struct DisplayIDMaintenanceServiceReassignTests {         let taskStore: InMemoryCounterStore         let milestoneStore: InMemoryCounterStore         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+        let gateStore: AllocationGatedCounterStore+        let project: Project+    }++    private func makeGatedEnv(gateTasks: Bool) throws -> GatedTestEnv {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let gateStore = AllocationGatedCounterStore()+        let taskAllocator: DisplayIDAllocator+        let milestoneAllocator: DisplayIDAllocator+        if gateTasks {+            taskAllocator = DisplayIDAllocator(store: gateStore)+            milestoneAllocator = DisplayIDAllocator(store: InMemoryCounterStore())+        } else {+            taskAllocator = DisplayIDAllocator(store: InMemoryCounterStore())+            milestoneAllocator = DisplayIDAllocator(store: gateStore)+        }+        let service = DisplayIDMaintenanceService(+            modelContext: context,+            taskAllocator: taskAllocator,+            milestoneAllocator: milestoneAllocator,+            commentService: CommentService(modelContext: context)+        )+        let project = Project(name: "Test", description: "", gitRepo: nil, colorHex: "#FF0000")+        context.insert(project)+        return GatedTestEnv(+            context: context,+            service: service,+            gateStore: gateStore,+            project: project+        )+    }++    /// Regression for T-2019: a peer update committed while task ID allocation+    /// is suspended must win. Cleanup reports stale-id and emits no false audit.+    @Test func taskPeerUpdateDuringAllocationIsPreservedWithoutAuditComment() async throws {+        let env = try makeGatedEnv(gateTasks: true)+        let loserId = UUID()+        let winner = TransitTask(+            name: "Winner", type: .feature, project: env.project, displayID: .permanent(5)+        )+        winner.creationDate = Date(timeIntervalSince1970: 1000)+        let loser = TransitTask(+            name: "Loser", type: .feature, project: env.project, displayID: .permanent(5)+        )+        loser.id = loserId+        loser.creationDate = Date(timeIntervalSince1970: 2000)+        env.context.insert(winner)+        env.context.insert(loser)+        try env.context.save()++        let peerContext = ModelContext(env.context.container)+        let peerLoser = try #require(try peerContext.fetch(FetchDescriptor<TransitTask>(+            predicate: #Predicate { $0.id == loserId }+        )).first)++        let maintenance = Task { @MainActor in+            await env.service.reassignDuplicates()+        }+        #expect(await env.gateStore.waitUntilAllocationStarts(),+                "Allocation never parked; the counter-call sequence changed")+        do {+            peerLoser.permanentDisplayId = 20+            try peerContext.save()+        } catch {+            await env.gateStore.releaseAllocation()+            _ = await maintenance.value+            throw error+        }+        await env.gateStore.releaseAllocation()++        let result = await maintenance.value+        let group = try #require(result.groups.first(where: { $0.type == .task }))+        #expect(group.failure?.code == .staleId)+        #expect(group.reassignments.isEmpty)+        // 100 was allocated and then deliberately skipped. Also proves the gate+        // parked inside allocation rather than during the counter fence.+        #expect(await env.gateStore.currentNextDisplayID() == 101,+                "Allocation must have completed and its counter value been skipped")++        let probe = ModelContext(env.context.container)+        let storedLoser = try #require(try probe.fetch(FetchDescriptor<TransitTask>(+            predicate: #Predicate { $0.id == loserId }+        )).first)+        #expect(storedLoser.permanentDisplayId == 20, "Peer-assigned task ID must be preserved")+        #expect(try probe.fetch(FetchDescriptor<Transit.Comment>()).isEmpty,+                "Stale cleanup must not emit an audit comment for a change it did not make")+    }++    /// Milestone companion to the gated T-2019 task regression.+    @Test func milestonePeerUpdateDuringAllocationIsPreserved() async throws {+        let env = try makeGatedEnv(gateTasks: false)+        let loserId = UUID()+        let winner = Milestone(name: "Winner", project: env.project, displayID: .permanent(7))+        winner.creationDate = Date(timeIntervalSince1970: 1000)+        let loser = Milestone(name: "Loser", project: env.project, displayID: .permanent(7))+        loser.id = loserId+        loser.creationDate = Date(timeIntervalSince1970: 2000)+        env.context.insert(winner)+        env.context.insert(loser)+        try env.context.save()++        let peerContext = ModelContext(env.context.container)+        let peerLoser = try #require(try peerContext.fetch(FetchDescriptor<Milestone>(+            predicate: #Predicate { $0.id == loserId }+        )).first)++        let maintenance = Task { @MainActor in+            await env.service.reassignDuplicates()+        }+        #expect(await env.gateStore.waitUntilAllocationStarts(),+                "Allocation never parked; the counter-call sequence changed")+        do {+            peerLoser.permanentDisplayId = 30+            try peerContext.save()+        } catch {+            await env.gateStore.releaseAllocation()+            _ = await maintenance.value+            throw error+        }+        await env.gateStore.releaseAllocation()++        let result = await maintenance.value+        let group = try #require(result.groups.first(where: { $0.type == .milestone }))+        #expect(group.failure?.code == .staleId)+        #expect(group.reassignments.isEmpty)+        #expect(await env.gateStore.currentNextDisplayID() == 101,+                "Allocation must have completed and its counter value been skipped")++        let probe = ModelContext(env.context.container)+        let storedLoser = try #require(try probe.fetch(FetchDescriptor<Milestone>(+            predicate: #Predicate { $0.id == loserId }+        )).first)+        #expect(storedLoser.permanentDisplayId == 30, "Peer-assigned milestone ID must be preserved")+    }+     private func makeEnv(         taskCounterStart: Int = 1,         milestoneCounterStart: Int = 1,         clock: @escaping () -> Date = { Date(timeIntervalSince1970: 1_700_000_000) }     ) throws -> TestEnv {

diff --git a/docs/agent-notes/display-id-maintenance.md b/docs/agent-notes/display-id-maintenance.mdindex 3028a04..1e8896a 100644--- a/docs/agent-notes/display-id-maintenance.md+++ b/docs/agent-notes/display-id-maintenance.md@@ -7,9 +7,9 @@ - Reassigned tasks get an agent-authored audit comment through `CommentService`; milestones do not have comment support. - The feature is exposed through Settings (`DataMaintenanceView`), two App Intents, and two gated MCP tools.  ## Current gotchas -- 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. Decision 11's pure `FetchDescriptor` against the main context returned the cached registered instance and missed peer-merged updates (T-1061).-- 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 but a deterministic gate (e.g. an injected slow `CounterStore` parking on an `AsyncStream` continuation) would harden it against scheduler variance.+- 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. - Existing tests mostly cover task-only duplicate scenarios. The UI test seed in `Transit/Transit/UITestScenario.swift` does not currently exercise mixed task+milestone duplicate IDs.

diff --git a/specs/bugfixes/duplicate-cleanup-stale-id-check-expires-during-allocation/report.md b/specs/bugfixes/duplicate-cleanup-stale-id-check-expires-during-allocation/report.mdnew file mode 100644index 0000000..b007cd0--- /dev/null+++ b/specs/bugfixes/duplicate-cleanup-stale-id-check-expires-during-allocation/report.md@@ -0,0 +1,131 @@+# Bugfix Report: Duplicate Cleanup Stale-ID Check Expires During Allocation++**Date:** 2026-08-01+**Status:** Fixed++## Description of the Issue++Duplicate display-ID cleanup verifies that a scanned loser still has the duplicate ID, then awaits asynchronous ID allocation. A peer context or device can commit a different ID while allocation is suspended. When cleanup resumes, it writes the allocated ID through its stale registered object and overwrites the peer change. The task path then adds an audit comment that falsely claims cleanup changed the task from the scanned duplicate ID.++**Reproduction steps:**+1. Scan a task or milestone duplicate group whose loser has committed display ID 5 or 7.+2. Let cleanup pass its committed-store stale-ID probe, then suspend inside `allocateNextID`.+3. Commit display ID 20 or 30 for that loser from a peer context.+4. Resume allocation and observe cleanup overwrite the peer ID; for tasks, observe a false maintenance audit comment.++**Impact:** A maintenance operation intended to repair duplicate IDs can overwrite a newer peer repair on both tasks and milestones. Task history can additionally claim a transition that cleanup did not validly perform.++## Investigation Summary++The investigation followed the four systematic-debugging phases.++### Phase 1: Initial overview++- **Expected:** The committed loser ID is still the scanned duplicate immediately before cleanup mutates and saves it. If it changed, return `stale-id`, preserve the peer value, and create no audit comment.+- **Actual:** The check is valid only before `allocateNextID`; allocation suspends and leaves a check-to-write race.+- **Context:** The race requires a peer/local-store update to land during the allocation await.++### Phase 2: Systematic inspection++- **Data-flow/timing defect:** `DisplayIDMaintenanceService.reassignTaskLoser` probes `storedTaskDisplayId`, awaits `taskAllocator.allocateNextID`, then mutates `loserTask` without a second probe.+- **Matching milestone defect:** `reassignMilestoneGroup` has the same probe-await-mutate sequence using `storedMilestoneDisplayId`.+- **Secondary task defect:** `appendAuditComment` runs after the stale write succeeds, so the race produces a false audit record based on the scan-time ID.+- **Existing safeguards inspected:** T-1061's transient-context probe correctly bypasses the main context cache, but only at the point where it is called. T-1766's used-ID closure prevents allocating an occupied ID; it does not validate ownership of the loser being mutated.++### Phase 3: Root cause analysis++**Defect type:** Async check-to-use race / stale validation.++**Five Whys:**+1. Why is the peer ID overwritten? Cleanup mutates the registered loser after the peer commits a newer value.+2. Why does cleanup permit that mutation? Its stale-ID guard passed before the peer update.+3. Why can the peer update land after the guard? `allocateNextID` is asynchronous and suspends between validation and mutation.+4. Why is the earlier guard treated as sufficient? T-1061 fixed the committed-store read mechanism but did not account for validation expiring across a later await.+5. Why is the audit false? Comment creation is conditioned only on cleanup's save succeeding, not on a committed-state validation immediately before that save.++**Root cause:** The committed loser ID is not re-probed after the final suspension point and immediately before mutation/save.++**Assumption validated:** A transient `ModelContext` is the established way to observe committed store state without the main context's registered-object cache; Decision 12 and the T-1061 regression define this contract.++### Phase 4: Solution and verification plan++- Re-probe `storedTaskDisplayId` immediately after allocation returns and directly before assigning `loserTask.permanentDisplayId`.+- Re-probe `storedMilestoneDisplayId` at the equivalent point.+- On a missing or mismatched committed value, return/report `.staleId` without mutation, save, reassignment entry, or task audit comment. The allocated counter value is intentionally skipped.+- Add deterministic task and milestone tests using a gated counter store that parks the allocation after the maintenance counter fence. Commit a peer ID while parked, then assert `staleId`, no reassignment, preserved peer ID, and no task audit comment.++## Discovered Root Cause++The initial stale-ID check occurs before an asynchronous allocation await. Its result is therefore stale by the time cleanup performs the write. Both record types share this structure, and only the task path compounds it by writing an audit comment after the invalid overwrite.++**Defect type:** Race condition / expired precondition.++**Why it occurred:** The T-1061 fix corrected where committed state was read but did not repeat that read after the subsequent suspension point.++**Contributing factors:** SwiftData's main context keeps the scan-time registered object, while the peer update is visible in the committed store through a transient context.++## Resolution for the Issue++**Changes made:**+- `Transit/Transit/Services/DisplayIDMaintenanceService.swift` — after task allocation returns, re-probe `storedTaskDisplayId` immediately before assigning the new ID. A missing or changed committed ID returns `.staleId` before mutation, save, or audit-comment creation.+- `Transit/Transit/Services/DisplayIDMaintenanceService.swift` — apply the equivalent post-allocation `storedMilestoneDisplayId` probe before milestone mutation/save.+- `Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swift` — add a deterministic counter-store gate plus task and milestone peer-update regressions.++**Approach rationale:** The existing transient-context lookup is already the established SwiftData mechanism for bypassing the main context's registered-object cache. Repeating that probe after `allocateNextID` returns places validation after the final suspension point and directly beside the mutation. There is no `await` between the second probe and save, so MainActor code cannot interleave another local mutation in that final window. The allocated ID is deliberately skipped on a stale result; consuming one counter value is safer than overwriting a peer repair.++**Alternatives considered:**+- **Rely on the pre-allocation probe:** Rejected because its result expires across the allocation await; this is the defect.+- **Hold the allocator's in-process gate through save:** Rejected because the peer writer may be another context/process/device and does not participate in that gate.+- **Add a database compare-and-swap for the model property:** SwiftData exposes no conditional update primitive for this CloudKit-backed model. The adjacent committed-store probe is the smallest compatible safeguard.++## Regression Test++**Test file:** `Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swift`++**Test names:**+- `taskPeerUpdateDuringAllocationIsPreservedWithoutAuditComment`+- `milestonePeerUpdateDuringAllocationIsPreserved`++**What they verify:** `AllocationGatedCounterStore` parks the loser's allocation after the maintenance counter fence. A peer context commits a replacement ID while cleanup is parked. On resume, cleanup must return `staleId`, make no reassignment, preserve the peer ID, and, for tasks, emit no audit comment.++**Run command:** `make test-quick`++## Affected Files++| File | Change |+|------|--------|+| `Transit/Transit/Services/DisplayIDMaintenanceService.swift` | Post-allocation committed-ID probes for task and milestone losers |+| `Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swift` | Deterministic gated task and milestone race regressions |+| `docs/agent-notes/display-id-maintenance.md` | Documents that stale-ID validation must run again after allocation |+| `specs/duplicate-displayid-cleanup/decision_log.md` | Decision 13 records the double probe and the deliberate counter skip |+| `specs/duplicate-displayid-cleanup/implementation.md` | Per-loser walkthrough updated to the two-probe sequence |+| `CHANGELOG.md` | Unreleased T-2019 fix entry |+| `specs/bugfixes/duplicate-cleanup-stale-id-check-expires-during-allocation/report.md` | Investigation, resolution, and verification record |++## Verification++**Automated:**+- [x] Regression tests fail before the fix (`make test-quick`; both gated cases recorded as failed in the xcresult)+- [x] Regression tests pass after the fix+- [x] Full macOS unit suite passes (`make test-quick`: 1,567 tests, 0 failures)+- [x] SwiftLint and repository ownership validation pass (`make lint`)+- [ ] Full iOS/UI validation passes — blocked by unrelated UI test failures described below++**Validation blockers:**+- `make test`: 1,130 passed, 3 failed; every failure was in `TransitUITests` (`testClearAll`, `testEditViewPreservesTaskMilestone`, `testDataMaintenanceGoldenPath`). All unit tests passed.+- `make test-ui`: 15 passed, 6 failed. In addition to the three above, settings tests could not find `dashboard.settingsButton` because it appeared under the toolbar's `More` overflow, and the data-maintenance test encountered duplicated/nested accessibility elements for `dataMaintenance.confirmButton`. Device-service/debugger warnings also appeared during the simulator run. None of these paths or views changed in T-2019.++**Manual verification:** Not applicable; the deterministic gated tests model the required interleaving directly.++## Prevention++- Treat validation before an `await` as expired when the validated state can change externally.+- Put the final committed-state probe after the last suspension point and adjacent to mutation.+- Pin concurrency regressions with explicit gates rather than scheduler timing.++## Related++- T-2019 — this bug.+- Decision 13 in `specs/duplicate-displayid-cleanup/decision_log.md` — the recorded decision, extending Decision 12.+- T-1061 — introduced the transient committed-store stale-ID probe.+- T-1766 — prevents maintenance allocation from returning an already-used display ID.

diff --git a/specs/duplicate-displayid-cleanup/decision_log.md b/specs/duplicate-displayid-cleanup/decision_log.mdindex 841fbaf..d665835 100644--- a/specs/duplicate-displayid-cleanup/decision_log.md+++ b/specs/duplicate-displayid-cleanup/decision_log.md@@ -420,5 +420,53 @@ Just before the stale-ID comparison in each loser path, read the loser's committ - `Transit/Transit/Services/DisplayIDMaintenanceService.swift` — new `storedTaskDisplayId(id:)` / `storedMilestoneDisplayId(id:)` helpers; both reassign paths now call them before the guard comparison. - `Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swift` — regression test `peerUpdatedLoserIsSkippedWithStaleId()`. - `Transit/TransitTests/TestModelContainer.swift` — added `newContainer()` to enable multi-context test setups.  ---++## Decision 13: Re-Probe the Committed Loser ID After Allocation++**Date**: 2026-08-01+**Status**: accepted++### Context++Decision 12 established *how* the stale-ID guard reads committed state: a transient `ModelContext` on the maintenance container, which bypasses the main context's registered-object snapshot. It did not address *when* that read happens. Both reassign paths probed the committed loser ID, then `await`ed `allocateNextID()`, then mutated and saved.++`allocateNextID()` is a suspension point with a CloudKit round-trip, so the probe's result can expire before the write. A peer device or another local context can commit a different `permanentDisplayId` for that loser while allocation is parked. Cleanup then resumes, overwrites the peer value through its scan-time registered object, and — on the task path — appends an audit comment claiming a transition cleanup was no longer entitled to make (T-2019). AC 2.3 requires the re-fetch "immediately before writing a loser's new ID", which the pre-fix sequence did not satisfy.++### Decision++Each loser is probed twice through the same `DisplayIDRecordLookup` helpers: once before allocation, and again after `allocateNextID()` returns, immediately adjacent to the mutation and save with no `await` in between. A missing or mismatched committed value on the second probe reports `stale-id` and performs no mutation, no save, no reassignment entry, and no audit comment. The already-allocated counter value is deliberately skipped.++### Rationale++The final `await` is the only window in which committed state can change without the maintenance service observing it, so validation belongs after it. The pre-allocation probe is retained purely as an early-out: it avoids a CloudKit round-trip — and therefore avoids burning a counter value — when the loser is already known to be stale.++Skipping one counter value is strictly cheaper than overwriting a peer's repair. Display IDs are only required to be unique and increasing (AC 2.2), never dense, and Decision 1 already accepts monotonic counter advance across failed groups.++### Alternatives Considered++- **Rely on the pre-allocation probe alone**: Keep the single existing check - Rejected because its result expires across the allocation await; that is the defect.+- **Hold the allocator's in-process gate through the save**: Extend `DisplayIDAllocator`'s allocation gate to cover the maintenance write - Rejected because the competing writer is another context, process, or device and does not participate in an in-process gate; it would also couple maintenance to allocator internals for no added safety.+- **Database-level compare-and-swap on `permanentDisplayId`**: Make the write conditional on the observed value - Rejected because SwiftData exposes no conditional-update primitive for a CloudKit-backed model.++### Consequences++**Positive:**+- A peer repair landing during allocation is preserved rather than overwritten, on both record types.+- Task history no longer records an audit comment for a reassignment that did not validly occur.+- The implemented sequence now matches AC 2.3's "immediately before writing" wording.++**Negative:**+- A counter value is consumed and never used whenever the second probe fires, so counters can develop small gaps.+- Two transient `ModelContext` constructions per loser instead of one, doubling the cost noted in Decision 12's consequences.+- The probe and the save are still not atomic with respect to CloudKit merges. The race is narrowed to a main-actor-synchronous span rather than eliminated; only another process or device can still interleave.++### Impact++- `Transit/Transit/Services/DisplayIDMaintenanceService.swift` — post-allocation probes in `reassignTaskLoser` and `reassignMilestoneGroup`.+- `Transit/TransitTests/DisplayIDMaintenanceServiceReassignTests.swift` — `AllocationGatedCounterStore` plus the two gated peer-update regressions.+- Supersedes the per-loser step ordering in `design.md` step 5.1 and the matching walkthrough in `implementation.md`.+- Full investigation record: `specs/bugfixes/duplicate-cleanup-stale-id-check-expires-during-allocation/report.md`.++---

diff --git a/specs/duplicate-displayid-cleanup/implementation.md b/specs/duplicate-displayid-cleanup/implementation.mdindex 6b43839..3db67af 100644--- a/specs/duplicate-displayid-cleanup/implementation.md+++ b/specs/duplicate-displayid-cleanup/implementation.md@@ -28,12 +28,13 @@ If you're scripting Transit, the same operations are available as Shortcuts (`Sc  1. Fetch tasks + milestones once. A fetch failure short-circuits to a result envelope with a counter-advance warning on both types (no silent no-op). 2. Reuse the same arrays to compute the per-type sampled max. 3. Per type, call `CounterStore.advanceCounter(toAtLeast: sampledMax + 1)` (CAS-with-retry). A counter-advance failure aborts that type only — the other type still runs. 4. Iterate groups (tasks first, ascending displayId). For each loser:-   - Re-fetch the loser by UUID and check `permanentDisplayId == scanned value` (stale-ID guard, AC 2.3 / Decision 11).+   - Read the loser's committed `permanentDisplayId` through a transient `ModelContext` and check it against the scanned value (stale-ID guard, AC 2.3 / Decision 12).    - `allocateNextID()` via the existing `DisplayIDAllocator` (per-loser CloudKit round-trip).+   - Repeat the committed-ID probe. `allocateNextID` suspends, so the first probe's result can expire; the second runs after the last `await` and immediately before the write (Decision 13). A mismatch records `stale-id`, writes nothing, and leaves the allocated counter value unused.    - Save the new ID. On `save()` failure, `safeRollback()` and record `save-failed`.    - For tasks only, append a `Comment` ("Transit Maintenance" / `isAgent=true` / body containing `T-<old>`, `T-<new>`, ISO date) via `CommentService.addComment(save:)` as a separate save. A comment-save failure becomes a `commentWarning` on the entry — the ID change persists.  The `CounterStore.advanceCounter(toAtLeast:retryLimit:)` extension (default impl on the `CounterStore` protocol) is what makes step 3 race-free: load → if already past target, return; else save with the loaded `expectedChangeTag`; on conflict, retry. A racing writer that already moved the counter past target short-circuits the next iteration.