Pre-push review: T-1959 teaching change-detection duplicate UUIDs

diff --git a/Packages/AsterismCore/Sources/AsterismCore/LibraryRepository+ComposedTeaching.swift b/Packages/AsterismCore/Sources/AsterismCore/LibraryRepository+ComposedTeaching.swiftindex 4e014c6..d0ecc19 100644--- a/Packages/AsterismCore/Sources/AsterismCore/LibraryRepository+ComposedTeaching.swift+++ b/Packages/AsterismCore/Sources/AsterismCore/LibraryRepository+ComposedTeaching.swift@@ -382,6 +382,17 @@ extension LibraryRepository {      /// Whether reapplying the composed outcome would change any Entry or Work     /// value (Req 7.1 `.noChanges` detection). Pure comparison, no mutation.+    ///+    /// **Detection and the commit resolve duplicates the same way**+    /// (Decision 12, T-1959). This once collapsed its own fetch to one row per+    /// UUID with `uniquingKeysWith: { a, _ in a }` over a descriptor carrying no+    /// sort, so under a duplicated Entry or Work UUID — tolerated and+    /// non-quarantining since Q12 — `.noChanges` was decided by whichever row+    /// the store happened to return first, while the apply path resolved the+    /// group deterministically and wrote every row of it. It buckets through+    /// `entryGroups`/`workGroups` now, on the same arguments+    /// `applyComposedOutcome` passes, and answers "changed" when **any** row of+    /// a logical record differs.     private static func composedOutcomeChangesState(         context: ModelContext, hostname: String, outcome: ComposedTeachingOutcome,         titleRuleID: UUID, titleVersion: Int, titleDefinition: PatternDefinition,@@ -448,6 +459,13 @@ extension LibraryRepository {                 case .protected, .ambiguous, .noChange:                     break                 case .reuse(let id), .claim(let id):+                    // The assignment's **UUID**, never which row of the group+                    // the pointer names (Decision 12). The apply path pins it to+                    // `group.representative`, but a to-one pointer moving+                    // between rows of one logical record changes nothing any+                    // reader can observe — every presentation unions the group —+                    // so reporting it as a change would make Recalculate commit+                    // over a library where no derived value is stale.                     if entry.work?.id != id { return true }                     // The assignment also refreshes the target Work's parsed                     // title (Req 3.21), so a stale name is a change even when

diff --git a/Packages/AsterismCore/Tests/AsterismCoreTests/ComposedRecalculationTests.swift b/Packages/AsterismCore/Tests/AsterismCoreTests/ComposedRecalculationTests.swiftindex cc2c2ea..4ac2d07 100644--- a/Packages/AsterismCore/Tests/AsterismCoreTests/ComposedRecalculationTests.swift+++ b/Packages/AsterismCore/Tests/AsterismCoreTests/ComposedRecalculationTests.swift@@ -135,6 +135,101 @@ struct ComposedRecalculationTests {         }     } +    // MARK: - Change detection sees the rows the commit writes (T-1959, Decision 12)++    @Test("Recalculation repairs a stale Entry twin an in-sync representative hides")+    func recalcRepairsStaleEntryTwin() async throws {+        let fixture = try ComposedRecalcFixture()+        let shared = UUID()+        let url = "https://ex.com/read?id=42&chapter=7"+        try fixture.seedUntaught([(shared, "Chapter 7 - Real Work", url)])+        try await teach(fixture)++        // A second row for the same Entry UUID, as sync produces one: identical+        // capture evidence, none of the derived values the teach wrote. Its+        // `modifiedAt` is later than the taught row's, so `GroupOrdering` makes+        // the **in-sync** row the representative and the stale one the twin —+        // the shape a winner-resolved change detector reports `.noChanges` over+        // while the commit behind it would write every row.+        try fixture.seed { context in+            let twin = Entry(+                id: shared, captureTitle: "Chapter 7 - Real Work", captureTitleSource: .host,+                rawURLString: url, hostname: host, entryIdentityKey: url,+                timestamp: Date(timeIntervalSince1970: 1))+            twin.conservativeIdentityKey = url+            twin.identityBasis = .conservative+            twin.editCitations { $0.identity = .rawURL }+            twin.modifiedAt = Date(timeIntervalSince1970: 1_900_000_000)+            context.insert(twin)+        }++        fixture.save.resetCounts()+        let contract = try await fixture.repository.previewRecalculation(hostname: host)+        let result = try await fixture.repository.commitRecalculation(contract)+        guard case .committed = result else { Issue.record("expected committed, got \(result)"); return }+        #expect(fixture.save.successCount == 1)++        let context = fixture.freshContext()+        let rows = try context.fetch(FetchDescriptor<Entry>()).filter { $0.id == shared }+        #expect(rows.count == 2)+        // Every row, not just the representative (Req 2.7).+        #expect(rows.allSatisfy { $0.citationValues.identity.keyVersion == 2 })+        #expect(rows.allSatisfy { $0.chapterSequence == "7" })+        #expect(rows.allSatisfy { $0.chapterTitle == "Chapter 7" })+        #expect(rows.allSatisfy { $0.work != nil })++        // And with the twin repaired, the next recalculation has nothing to do.+        let second = try await fixture.repository.previewRecalculation(hostname: host)+        guard case .noChanges = try await fixture.repository.commitRecalculation(second) else {+            Issue.record("expected noChanges on the second recalculation"); return+        }+    }++    @Test("Recalculation repairs a stale Work twin an in-sync representative hides")+    func recalcRepairsStaleWorkTwin() async throws {+        let fixture = try ComposedRecalcFixture()+        try fixture.seedUntaught([(UUID(), "Chapter 7 - Real Work", "https://ex.com/read?id=42&chapter=7")])+        try await teach(fixture)++        // The Work counterpart: a second row for the same Work UUID whose+        // membership for this hostname carries no rule-derived URL identity. It+        // is created later than the taught row, so it is the twin rather than+        // the representative, and it differs in nothing a reader authored — so+        // the group is split, not torn, and teaching stays open on it.+        var workID: UUID?+        try fixture.seed { context in+            guard let taught = (try? context.fetch(FetchDescriptor<Work>()))?.first else { return }+            workID = taught.id+            let site = (try? context.fetch(FetchDescriptor<Site>()))?.first { $0.hostname == host }+            let twin = Work.createCarrying(+                in: context, id: taught.id, title: taught.displayTitle, hostname: host,+                site: site, identity: nil,+                timestamp: Date(timeIntervalSince1970: 1_900_000_000))+            twin.lastParsedTitle = taught.lastParsedTitle+            twin.titleProvenanceRaw = taught.titleProvenanceRaw+        }+        let shared = try #require(workID)++        fixture.save.resetCounts()+        let contract = try await fixture.repository.previewRecalculation(hostname: host)+        let result = try await fixture.repository.commitRecalculation(contract)+        guard case .committed = result else { Issue.record("expected committed, got \(result)"); return }+        #expect(fixture.save.successCount == 1)++        let context = fixture.freshContext()+        let rows = try context.fetch(FetchDescriptor<Work>()).filter { $0.id == shared }+        #expect(rows.count == 2)+        let identities = rows.map { $0.membership(for: host)?.urlIdentity }+        #expect(identities.allSatisfy { $0 != nil })+        #expect(Set(identities.map { $0 ?? "" }).count == 1)+        #expect(rows.allSatisfy { $0.membership(for: host)?.urlIdentityState == .rule })++        let second = try await fixture.repository.previewRecalculation(hostname: host)+        guard case .noChanges = try await fixture.repository.commitRecalculation(second) else {+            Issue.record("expected noChanges on the second recalculation"); return+        }+    }+     @Test("Recalculation never creates or retires a rule")     func recalcReusesRuleVersions() async throws {         let fixture = try ComposedRecalcFixture()

diff --git a/specs/bugfixes/teaching-change-detection-duplicate-uuids/report.md b/specs/bugfixes/teaching-change-detection-duplicate-uuids/report.mdnew file mode 100644index 0000000..fe62b54--- /dev/null+++ b/specs/bugfixes/teaching-change-detection-duplicate-uuids/report.md@@ -0,0 +1,167 @@+# Bugfix Report: Teaching Change-Detection Resolves Duplicate UUIDs Differently From The Commit++**Date:** 2026-08-29+**Status:** Fixed+**Ticket:** T-1959++## Description of the Issue++`composedOutcomeChangesState` decides Recalculate's `.noChanges` short-circuit:+it re-derives every Entry and Work value the commit would write and answers+whether any of them differs from what is stored. It built its own by-UUID+collapse of its fetch —++```swift+let entriesByID = Dictionary(entries.map { ($0.id, $0) }, uniquingKeysWith: { a, _ in a })+let worksByID   = Dictionary(works.map   { ($0.id, $0) }, uniquingKeysWith: { a, _ in a })+```++— over a `FetchDescriptor` carrying no sort descriptor, so the row it compared+against was whichever the store happened to return first. The apply path in the+same file resolved the same records deterministically and wrote **every** row of+them (Req 2.7).++**Reproduction steps:**+1. Teach a site so its Entries carry rule-derived identity keys, chapter+   sequences and Work assignments.+2. Let sync deliver a second row for one of those Entry UUIDs (a tolerated,+   non-quarantining state per Q12) carrying the pre-teach conservative values.+3. Run Recalculate.+4. Observe: the commit reports `.noChanges` when the comparison happened to land+   on the in-sync row, and repairs the twin when it happened to land on the stale+   one — the same store, the same rules, two answers.++**Impact:** Medium. Recalculate is the reader's route to repairing a site. A+false `.noChanges` says there is nothing to repair while the twin stays stale,+and every later recalculation makes the same mistake for the same reason. Only+reachable while a duplicate Entry or Work UUID exists, which is what+`library-integrity-tolerance` made a tolerated rather than a fatal state.++## Investigation Summary++- **Symptoms examined:** ticket T-1959, raised during the pre-push review of+  `feature/library-integrity-tolerance`. Line numbers were from July 2026, so the+  code was located by grepping for `uniquingKeysWith`.+- **Code inspected:** `LibraryRepository+ComposedTeaching.swift`+  (`composedOutcomeChangesState`, `applyComposedOutcome`,+  `buildComposedTeachingBasis`), `LibraryRepository+Groups.swift`+  (`entryGroups`/`workGroups`, `EntryGroup`/`WorkGroup`), `GroupOrdering.swift`+  (row order, authored content), `LibraryDiagnostics.quarantineMap()`.+- **Hypotheses tested:**+  - *The collapse is still there* — it is not. `git log -S uniquingKeysWith` on+    the file shows it introduced by `0e536a5` (unified teaching composition) and+    removed by `610c080` (duplicate reconciliation, M4c), which replaced it with+    `entryGroups`/`workGroups` while retiring the winner maps across the write+    paths (Q69 of `duplicate-reconciliation`).+  - *A residual divergence remains* — one does, and it is deliberate rather than+    accidental: detection compares `entry.work?.id`, while apply pins the pointer+    to `group.representative`. See Decision 12.+  - *Detection could still be winner-resolved somewhere* — checked every branch+    of the function against the corresponding branch of the apply path; both now+    iterate `group.rows`.++## Discovered Root Cause++A comparison that resolved a duplicate set to one arbitrary row, sitting in front+of a write that addressed every row of it. The two were never the negation of+each other, and nothing named which of them was right.++**Defect type:** Non-determinism — an unordered fetch collapsed with a+first-one-wins rule, used to decide whether a write runs at all.++**Why it occurred:** The code was written under unified teaching composition+(`0e536a5`), when a duplicated Entry or Work UUID failed the library open, so the+path could not be entered with duplicates present and the collapse was+unreachable in practice. `library-integrity-tolerance` made that state tolerated,+which made the collapse reachable. The same shape as T-1956: latent code whose+bug the tolerance work exposed rather than introduced.++**Contributing factors:** the winner-map retirement in `610c080` was enforced by+deleting helpers so the compiler would enumerate their callers, and this function+never called them — it built its own map. Q69 caught it by inspection.++## Resolution for the Issue++The routing half of the fix landed with the group refactor in `610c080`: the+collapse is gone and both sides bucket through `entryGroups`/`workGroups` with+identical arguments. What was still missing is what T-1959 asks for — the design+answer, stated rather than inherited, and tests that hold the code to it.++**Changes made:**+- `Packages/AsterismCore/Sources/AsterismCore/LibraryRepository+ComposedTeaching.swift`+  — documents the rule on `composedOutcomeChangesState` (detection and the commit+  resolve duplicates the same way; any row differing is a change) and states the+  pointer-row carve-out where the comparison is made.+- `specs/library-integrity-tolerance/decision_log.md` — Decision 12, the full+  entry answering winner-resolution versus any-row.+- `Packages/AsterismCore/Tests/AsterismCoreTests/ComposedRecalculationTests.swift`+  — two regression tests.+- `specs/library-integrity-tolerance/implementation.md` — the Potential Issues+  bullet listing T-1959 as open now records it as resolved.++**Approach rationale:** detection must be the exact negation of the commit. The+commit writes every row of a logical record, so a comparison that clears after+one row claims a repair the next commit would perform is not needed. The union is+also the conservative direction of error: over-reporting costs one save that+changes nothing and then genuinely answers `.noChanges`, while under-reporting+leaves a stale row no route repairs.++**Alternatives considered:** see Decision 12 — winner-resolved detection, keeping+the arbitrary collapse, treating the representative re-pin as a change, and+dropping the short-circuit entirely.++## Regression Test++**Test file:** `Packages/AsterismCore/Tests/AsterismCoreTests/ComposedRecalculationTests.swift`+**Test names:** `recalcRepairsStaleEntryTwin`, `recalcRepairsStaleWorkTwin`++**What they verify:** a duplicated Entry (and Work) UUID whose *representative*+row is in sync and whose twin is stale. Recalculate must report `.committed`,+repair every row, and answer `.noChanges` only on the following run. Both are+constructed so the stale row is deliberately **not** the representative under+`GroupOrdering`, which makes them a deterministic pin rather than an+order-dependent one.++**Red/green check:** with the two comparison loops in+`composedOutcomeChangesState` narrowed to `[group.representative]` — a+winner-resolved detector, the rejected alternative — both tests fail reporting+`.noChanges`. Restored, both pass.++**Run command:** `make test-core CORE_TEST='ComposedRecalculationTests'`++## Affected Files++| File | Change |+|------|--------|+| `Packages/AsterismCore/Sources/AsterismCore/LibraryRepository+ComposedTeaching.swift` | Documents the shared-resolution rule and the pointer carve-out |+| `Packages/AsterismCore/Tests/AsterismCoreTests/ComposedRecalculationTests.swift` | Two regression tests for stale twins behind an in-sync representative |+| `specs/library-integrity-tolerance/decision_log.md` | Decision 12 |+| `specs/library-integrity-tolerance/implementation.md` | T-1959 recorded as resolved |++## Verification++**Automated:**+- [x] Regression tests pass (and fail under the rejected alternative)+- [x] `make test-core` passes+- [x] `make verify-identity` passes (it is a `test-core` prerequisite)++**Manual verification:** none — the state is reachable only through sync, and the+suite constructs it directly.++## Prevention++- A comparison that gates a write must resolve its rows through the same helper+  the write does, with the same arguments. A function building its own+  `Dictionary(..., uniquingKeysWith:)` over a fetch with no sort is the smell.+- Deleting a helper so the compiler enumerates its callers does not reach code+  that re-implemented it inline. A grep for `uniquingKeysWith` over the write+  paths is the cheap periodic check.+- New `.noChanges`-style short-circuits should be pinned by a test whose stale row+  is deliberately not the representative, so a winner-resolved regression fails+  deterministically instead of intermittently.++## Related++- Decision 12 and Q12 in `specs/library-integrity-tolerance/decision_log.md`+- Q69 and Decision 9 in `specs/duplicate-reconciliation/decision_log.md`+- T-1956 (the sibling latent defect the same review found)

diff --git a/specs/library-integrity-tolerance/decision_log.md b/specs/library-integrity-tolerance/decision_log.mdindex 66eb286..87145f0 100644--- a/specs/library-integrity-tolerance/decision_log.md+++ b/specs/library-integrity-tolerance/decision_log.md@@ -672,3 +672,124 @@ retired — a new task adding a `refreshDiagnostics` signpost and a device UI te to drive it.  ---++## Decision 12: `.noChanges` Detection Compares Every Row Of A Logical Record, Not A Resolved Winner++**Date**: 2026-08-29+**Status**: accepted++### Context++`composedOutcomeChangesState` decides Recalculate's `.noChanges` short-circuit:+it re-derives every Entry and Work value the commit would write and answers+whether any of them differs from what is stored. It used to build its own+by-UUID collapse of its fetch —+`Dictionary(entries.map { ($0.id, $0) }, uniquingKeysWith: { a, _ in a })`, and+the same for Works — over a `FetchDescriptor` carrying no sort descriptor. Under+a duplicated Entry or Work UUID, which Q12 makes a tolerated, non-quarantining+state, that keeps whichever row the store returned first: an arbitrary answer,+not a resolved one. The apply path eleven lines below resolved the same records+deterministically and wrote **every** row of them (Req 2.7). So the comparison+and the commit could see different rows: a teaching commit could report no+changes while the rows it would write did differ, or the reverse (T-1959).++The routing half of the fix is not contested — both sides must resolve+duplicates identically. What had to be settled is *which* answer they share.+Detection can compare against the resolved winner of each duplicate set, or it+can report a change when any row of the set differs. The two disagree exactly on+the shape sync produces most often: an in-sync row beside a stale twin.++The code as it stands already takes the second answer — the group refactor that+retired the winner maps (Q69 of `duplicate-reconciliation`, which names this+function) rewrote it along with the write paths, replacing the collapse with+`entryGroups`/`workGroups`. It arrived as a consequence of that refactor rather+than as an answer to this question, and was never stated as a rule. Decision 9+of this spec is the precedent for stating one: resolution is split by purpose,+and each caller's purpose is named rather than inherited.++### Decision++Change detection buckets its rows through `entryGroups`/`workGroups` with the+same arguments `applyComposedOutcome` passes, and reports a change when **any**+row of a logical record differs from what the outcome would write. Which+physical row a to-one assignment pointer names is explicitly **not** a+difference: the comparison is `entry.work?.id != id`, and the apply path's pin+to `group.representative` may move the pointer without that counting as a+change.++### Rationale++The rule that makes detection correct is that it must be the exact negation of+the commit. The commit writes every row of a group, so a comparison that clears+after inspecting one row is answering a different question from the one the+caller asked: with a stale twin present, `.noChanges` is a claim that a repair+the very next commit would perform is not needed. That is the failure mode with+a reader behind it — Recalculate is how someone repairs a site, and a false+`.noChanges` tells them there is nothing to repair.++The union is also the conservative direction of error. Its cost when wrong is a+commit that rewrites values already equal — one save, no reader-visible change,+and the second recalculation then genuinely answers `.noChanges`. Winner-only's+cost when wrong is a stale row that no route repairs, since every subsequent+recalculation makes the same mistake for the same reason.++The pointer carve-out is where "any row differs" would over-report rather than+under-report. Every presentation of a Work unions the Entries of every row of+its group, so an assignment pointing at a twin and one pointing at the+representative are indistinguishable to a reader; treating the re-pin as a change+would make Recalculate commit on a library where nothing derived is stale, and+the re-pin belongs to reconciliation anyway. The carve-out is narrow and stated+where the comparison is made rather than left to be inferred.++### Alternatives Considered++- **Share the winner resolution**: compare only each group's representative,+  the row `GroupOrdering` puts first - Rejected because the write is not+  winner-only. It makes `.noChanges` a statement about one row and the commit a+  statement about all of them, so a stale twin behind an in-sync representative+  is invisible to the only route that would repair it. Both new regression tests+  fail under it.+- **Keep the arbitrary first-row collapse**: no change at all - Rejected as the+  bug. It is neither answer: it agrees with the winner on some runs and with the+  union on others, so no test can pin it and no reader can predict it.+- **Report the representative re-pin as a change too**: a stricter reading of+  "detection is the negation of the commit" - Rejected because it would make+  `.noChanges` unreachable for any split group whose pointer happens to name the+  twin, committing a save the reader cannot observe every time they recalculate.+- **Drop the short-circuit and always commit**: no detection to get wrong -+  Rejected: Req 7.1 asks for `.noChanges`, and always committing writes+  `modifiedAt` across the hostname's rows on every recalculation, which is a+  sync fan-out and a false "changed" signal to every other device.++### Consequences++**Positive:**+- Detection and the commit answer over the same rows by construction: both call+  the same two bucketing helpers with the same arguments, so drift means+  deleting a call the compiler will name.+- Recalculate repairs a stale twin instead of reporting there is nothing to+  repair — the case sync actually produces.+- The answer is device-independent. `GroupOrdering` is a total order over synced+  fields (Q36 of `data-model-cleanups`), where the retired `RecordResolutionOrder`+  ended on the device-local `PersistentIdentifier`.++**Negative:**+- Detection costs one derivation per **row** rather than per record. Split groups+  are rare and duplicate sets are small, so the multiplier is near 1 in practice,+  but the worst case is bounded by row count rather than record count.+- A group whose rows disagree only in a value the outcome does not write still+  reports `.noChanges`, and the commit that a *different* difference triggers+  will then converge it. Recalculate is not a convergence pass and this decision+  does not make it one.+- The pointer carve-out is a documented asymmetry: one thing the commit writes is+  deliberately not compared. It is correct for the reason stated, and it is the+  kind of exception that needs re-justifying if `entry.work` ever stops being+  read through the group.++### Impact++`LibraryRepository+ComposedTeaching.swift` (`composedOutcomeChangesState`),+Req 7.1, and `ComposedRecalculationTests` — which pins both halves with a stale+Entry twin and a stale Work twin behind an in-sync representative.++---

diff --git a/specs/library-integrity-tolerance/implementation.md b/specs/library-integrity-tolerance/implementation.mdindex ed7c6c1..12171e5 100644--- a/specs/library-integrity-tolerance/implementation.md+++ b/specs/library-integrity-tolerance/implementation.md@@ -842,8 +842,10 @@ store that got that way by accident.   state quarantines and a quarantined site already takes the conservative path.   Correct, but it makes part of Req 5.4 measure work that is not happening. - **Two commit sites still roll back on any diagnosis** rather than comparing-  pre and post (T-1956), and change detection resolves duplicates differently-  from the commit that follows it (T-1959).+  pre and post (T-1956). Change detection resolving duplicates differently from+  the commit that follows it (T-1959) is **resolved**: it buckets through the+  same `entryGroups`/`workGroups` the apply path uses and reports a change when+  any row of a logical record differs (Decision 12).  ---