crossmate

commit aec26a1b155a86cf1b268ff9d840ac187006085c
parent 4ea5c9f22df8c81341e0f7d2051ea6dbcba55624
Author: Michael Camilleri <[email protected]>
Date:   Fri,  3 Jul 2026 12:47:08 +0900

Add notes about flaws

Diffstat:
A
Notes/DesignFlaws.md
 | 
1654
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

1 file changed, 1654 insertions(+), 0 deletions(-)
diff --git a/Notes/DesignFlaws.md b/Notes/DesignFlaws.md
@@ -0,0 +1,1654 @@
+# Crossmate Audit
+
+A phased audit of the Crossmate codebase. Each phase covers one conceptual
+piece of the app and is reviewed in its own pass, since the whole codebase is
+too large to review in one sitting.
+
+**Focus for every phase:** correctness, security, performance, and code
+quality/style. Findings that don't fit the phase they're discovered in should
+still be recorded under that phase, with a note pointing at the phase they
+actually belong to.
+
+**Out of scope:** `Crossmake/` (separate offline puzzle-authoring SPM
+package, not part of the app build).
+
+**Status legend:** Not started / In progress / Done
+
+---
+
+## Phase 1 — CloudKit Sync Engine
+
+**Status:** Done
+
+The core of the app: raw `CKSyncEngine`, per-game zones, record
+serialization/merging, and conflict handling. Highest complexity, highest
+blast radius if wrong.
+
+- `Crossmate/Sync/SyncEngine.swift`
+- `Crossmate/Sync/CloudZones.swift`
+- `Crossmate/Sync/CloudQuery.swift`
+- `Crossmate/Sync/RecordApplier.swift`
+- `Crossmate/Sync/RecordBuilder.swift`
+- `Crossmate/Sync/RecordSerializer.swift`
+- `Crossmate/Sync/GridStateMerger.swift`
+- `Crossmate/Sync/Moves.swift`
+- `Crossmate/Sync/MovesUpdater.swift`
+- `Crossmate/Sync/PeerChangeLedger.swift`
+- `Crossmate/Sync/PlayerSelectionPublisher.swift`
+- `Crossmate/Sync/RecentChanges.swift`
+- `Crossmate/Sync/SessionMonitor.swift`
+- `Crossmate/Sync/Presence.swift`
+- `Crossmate/Sync/AuthorIdentity.swift`
+- `Crossmate/Sync/Archive.swift`
+- `Crossmate/Sync/GameArchiver.swift`
+- `Crossmate/Sync/CloudDiagnostics.swift`
+- `Crossmate/Sync/SyncState+Helpers.swift`
+- `Crossmate/Services/CloudService.swift` (sync composition root)
+- `Tests/Unit/Sync/*`, `Tests/Unit/RecordSerializer*Tests.swift`,
+  `Tests/Unit/GridStateMergerTests.swift`, `Tests/Unit/MovesUpdaterTests.swift`,
+  `Tests/Unit/PeerChangeLedgerTests.swift`, `Tests/Unit/RecentChangesTests.swift`,
+  `Tests/Unit/SyncMonitorTests.swift`
+
+### Findings (consolidated from Sonnet 5 and Opus 4.8, reviewed 2026-07-01)
+
+Overall this layer is in good shape: the LWW/etag guards, the
+`serverRecordChanged` re-enqueue handling, the burst/drain routing, and the
+zone-orphaning teardown are carefully reasoned and heavily documented. The
+merge/ledger/publisher helpers are clean and well-tested. The items below are
+the exceptions, ordered by practical priority.
+
+**1. (Security — high) Account push address/secret `Decision`s are adopted
+without verifying they came from the private `account` zone.**
+`RecordSerializer.parseAccountPushAddressDecision` /
+`parseAccountPushSecretDecision` (`RecordSerializer.swift:219`, `:245`)
+validate only the record type/name/kind/payload. They do not check the record's
+zone or database scope. `SyncEngine.handleFetchedRecordZoneChanges` calls them
+for every fetched `Decision` record (`SyncEngine.swift:1514`, `:1517`) on both
+the private and shared engines, then dispatches the surfaced values to
+`onAccountPushAddress` / `onAccountPushSecret` (`SyncEngine.swift:1656`,
+`:1666`).
+
+The push secret is the HMAC key used by `deriveGameAddress`, and the comments
+say it should live only in the private account zone. A writable non-account
+zone can therefore carry a record named `decision-account-pushSecret`, type
+`Decision`, `kind=account`, with attacker-chosen payload/version, and this
+device will adopt it. At minimum this can poison push registration and cause a
+push-delivery DoS. The sibling `nickname` decision already gates on
+`record.recordID.zoneID.zoneName == accountZoneID.zoneName && databaseScope == 0`
+(`RecordSerializer.swift:1153`); account push address/secret adoption should
+use the same private-account-zone guard, either in the parsers or at their fetch
+call site. The `serverRecordChanged` conflict path (`SyncEngine.swift:1889`,
+`:1892`) is less exposed because the server record comes from the account-zone
+write being resolved.
+
+**2. (Correctness/data loss — high) `GameArchiver.promoteRevoked` deletes the
+original game even when archive materialization fails.**
+`GameArchiver.promoteRevoked` discards the result of
+`Archive.materialize(payload, in:)` (`GameArchiver.swift:221`) and then
+unconditionally deletes the original `GameEntity` (`:222-227`).
+`Archive.materialize` returns `nil` if `payload.puzzleSource` is empty
+(`Archive.swift:392`). A cloud archive payload can have an empty puzzle source
+if the `CKAsset` read fails, because `Archive.payload(from:)` converts any
+missing/unreadable asset to `""` (`Archive.swift:352-354`).
+
+Failure scenario: a completed shared game is revoked; the archive record fetches
+but the puzzle source asset is unavailable or unreadable; no replacement row is
+created, yet the original row is deleted. Only delete the original after
+materialization succeeds, or fall back to the local snapshot when the cloud
+payload is incomplete.
+
+**3. (Security/performance — medium) `puzzleSource` CKAssets are read without a
+size cap.**
+`RecordSerializer.applyGameRecord` reads the entire `puzzleSource` asset with
+`String(contentsOf: fileURL, encoding: .utf8)` (`RecordSerializer.swift:870-901`).
+Anyone with write access to a shared game zone can cause every syncing device
+to load that asset fully into memory. Crossword sources are inherently small,
+so enforce a conservative byte limit before decoding and reject or ignore
+oversized assets.
+
+**4. (Performance — medium) Sync actors use blocking Core Data work in async
+paths.**
+The sync layer frequently calls `context.performAndWait` from async actor
+methods, including `SyncEngine.handleFetchedRecordZoneChanges`
+(`SyncEngine.swift:1466`), `MovesUpdater.performFlush`
+(`MovesUpdater.swift:158`), and `PlayerSelectionPublisher`
+(`PlayerSelectionPublisher.swift:203`). This may be a deliberate serialization
+tradeoff, but blocking a Swift concurrency worker during Core Data fetch/save
+work can become a scalability problem during bursty sync across many games.
+Consider a systematic pass to move hot paths to `await context.perform { ... }`
+where ordering constraints allow it. Treat this as a performance audit item
+rather than an immediate correctness bug.
+
+**5. (Performance — low/medium) `RecordBuilder.buildRecord` creates a fresh
+background context per outbound record.**
+`RecordBuilder.buildRecord` creates a new background context at
+`RecordBuilder.swift:48` and performs one lookup/build inside it. Because this
+runs once per pending outbound record, draining a large CKSyncEngine batch
+allocates one Core Data context per record. Hoisting context creation to the
+batch caller would reduce overhead and share the row cache across a drain.
+
+**6. (Resource — low) Archive temp files are left for OS cleanup.**
+`Archive.asset(for:ext:)` writes a new file in `FileManager.default.temporaryDirectory`
+for every archive asset (`Archive.swift:300-306`). `GameArchiver.write` can
+re-save archives across retry/reconcile passes, and no path explicitly removes
+these files after CloudKit has consumed them. This is usually bounded by OS temp
+eviction, but a cleanup hook after successful `CKModifyRecordsOperation` would
+avoid accumulating orphaned archive asset files.
+
+**7. (Correctness/resource — low) Private-DB zone deletion does not share the
+full zone-orphan cleanup path.**
+When a private database zone deletion is fetched, `handleFetchedDatabaseChanges`
+deletes the local `GameEntity` (`SyncEngine.swift:1346-1415`) but does not
+remove queued `pendingRecordZoneChanges` or `pendingPings` for that zone.
+`applyZoneOrphaning` does remove both (`SyncEngine.swift:2033-2054`). This
+should self-heal when the queued send later fails with `.zoneNotFound`, but the
+fetch-side deletion path could use the same helper to avoid a futile retry and
+keep transient state consistent.
+
+**8. (Performance — low) `GameArchiver.write` ensures the archive zone before
+every save.**
+`GameArchiver.write` calls `ensureZone(zoneID)` unconditionally
+(`GameArchiver.swift:139-143`). This is idempotent and correct, but can add an
+avoidable CloudKit round trip on repeated archive reconcile passes. Cache known
+archive zones or tolerate `zoneAlreadyExists` after a create-once attempt.
+
+**9. (Correctness/consistency — low) Equal-timestamp move-cell merges use an
+implicit incoming-wins tie-break.**
+`mergeIncomingMovesCells` replaces the local cell whenever
+`existing.updatedAt <= incoming.updatedAt` (`RecordSerializer.swift:1014-1027`).
+`GridStateMerger.shouldReplace` uses a deterministic tie-break on author and
+device after timestamp equality (`GridStateMerger.swift:78-86`). This merge is
+scoped to a single `(author, device)` moves row, so impact is likely low, but
+aligning the tie-breaks would reduce surprise if the helper is reused.
+
+**10. (Code quality — low) `applyPlayerRecord` drops the whole record when
+`name` is absent.**
+`RecordApplier.applyPlayerRecord` returns early if `record["name"]` is missing
+(`RecordApplier.swift:231`), which also drops selection, presence, read cursor,
+and time-log fields. Today `playerRecord` always writes `name`, so this is a
+future partial-fetch sharp edge rather than a live bug.
+
+**11. (Code quality — low) `MovesUpdater.enqueue(actingAuthorID:)` is a dead
+parameter.**
+`GameMutator` passes `actingAuthorID` into `MovesUpdater.enqueue`
+(`GameMutator.swift:371`), but `MovesUpdater.enqueue` ignores the parameter
+(`MovesUpdater.swift:77`). Journaling attribution is handled separately in
+`GameMutator`, so either remove the parameter or wire it to behavior that needs
+it.
+
+**Non-findings and deferred checks:**
+- `serverRecordChanged` re-enqueue handling appears correct: the
+  `recoverServerChangedSave` and `recoveredSaves`/`decisionWins` paths
+  explicitly re-add `.saveRecord` after adopting server system fields.
+- `CloudQuery` desired-key lists are centralized in `RecordSerializer` and
+  cross-checked against written fields; the old duplicated-list footgun is not
+  present here.
+- `GridStateMerger`'s primary merge logic is deterministic and covered by
+  tests.
+- Record-name parsers reject malformed names rather than force-unwrapping
+  untrusted CloudKit record names.
+- `CloudDiagnostics` does not appear to log tokens, emails, or full record
+  dumps.
+- Re-check account push address/secret adoption and republish behavior in
+  Phase 4 alongside `AccountPushCoordinator`.
+- Re-check non-monotonic `PlayerEntity.readThrough` adoption in Phase 2 when
+  durable read cursor semantics are reviewed.
+- Re-check `RecordSerializer.applyDecisionRecord` friend/nickname/block/
+  encryption-key cases, `Presence` trust boundaries, and `ShareController`
+  join confirmation in Phase 3.
+
+---
+
+## Phase 2 — Persistence & Data Model
+
+**Status:** Done
+
+Core Data model, the local durable store, journal/replay, and the domain
+models layered on top.
+
+- `Crossmate/Persistence/PersistenceController.swift`
+- `Crossmate/Persistence/GameStore.swift`
+- `Crossmate/Persistence/GameMutator.swift`
+- `Crossmate/Persistence/Journal.swift`
+- `Crossmate/Persistence/JournalReplay.swift`
+- `Crossmate/Models/CrossmateModel.xcdatamodeld/`
+- `Crossmate/Models/Game.swift`, `GameEntity+ContentKey.swift`
+- `Crossmate/Models/Square.swift`, `CellMark.swift`
+- `Crossmate/Models/PlayerRoster.swift`, `PlayerSession.swift`,
+  `PlayerSelection.swift`, `PlayerColor.swift`, `PlayerPreferences.swift`
+- `Crossmate/Models/ParticipantSummaries.swift`, `GameCursorStore.swift`,
+  `GameViewedStore.swift`, `TimeLog.swift`, `TipStore.swift`
+- `Tests/Unit/GameStore*Tests.swift`, `GameMutatorTests.swift`,
+  `JournalReplayTests.swift`, `JournalUploadTests.swift`,
+  `PersistenceRecoveryTests.swift`, `GameCursorStoreTests.swift`,
+  `GameViewedStoreTests.swift`, `TimeLogTests.swift`, `TipStoreTests.swift`,
+  `PlayerColorTests.swift`, `PlayerRosterTests.swift`
+
+### Findings (consolidated from Sonnet 5 and Opus 4.8, reviewed 2026-07-01)
+
+No security findings this phase — the model layer doesn't cross a trust
+boundary itself (author-ID/zone validation lives in `RecordSerializer`/
+`RecordApplier`, already covered in Phase 1). The main themes are a silent
+failure path in the move journal (bounded in practice by the journal
+context's merge policy), one clock-reconciliation edge case, and a few spots
+where synchronous Core Data work runs on the main actor / render path despite
+an `async` signature promising otherwise. Both passes agreed on the seven
+items below; the severity of #1 is tempered on the second pass, and #4/#6
+carry a note that the fix is a local index change, not a CloudKit schema
+deploy.
+
+**1. (Correctness — medium) `MovesJournal.persist` silently swallows Core Data
+save failures, which can permanently truncate a finished game's uploaded
+replay journal.** `Journal.swift:404-423` calls `ctx.save()` and, on failure,
+only does `print("MovesJournal: failed to persist entry: \(error)")` plus
+`ctx.rollback()` — no `eventLog`, unlike every other Core Data writer in this
+phase (`GameStore.saveContext`, `PersistenceController.backfillCachedSummaryFields`).
+`MovesJournal` isn't even constructed with an `eventLog` reference
+(`GameStore.swift:566`: `MovesJournal(persistence: persistence)`). This
+matters because the completion/replay-upload path deliberately reads the
+*persisted* Core Data rows through a separate background context rather than
+the in-memory `entries` list — see the doc comment on `flush()`
+(`Journal.swift:394-402`): the Phase 2 upload "reconstructs the asset from
+Core Data on a *separate* background context." A dropped `persist` (e.g. a
+non-optional-relationship validation failure if `entity.game` resolves to nil
+because the game row was concurrently deleted, or any other one-off Core Data
+error) removes that touch from the uploaded Journal asset permanently, with
+nothing beyond a console `print` to explain why a replay is missing a move.
+Severity was reconsidered on the second pass: the `mergeByPropertyStoreTrump`
+policy set in `MovesJournal.init` (`Journal.swift:163`) already neutralises
+the one *known* failure (the 133020 merge conflict), and the residual
+save-failure surface is narrow — the `game` relationship is optional, so a
+concurrently-deleted game just nulls it rather than failing validation, and
+`gameID` is always set. So the permanent-truncation scenario is real but
+uncommon; the silent, un-logged swallow (inconsistent with every other writer
+in the layer) is the core problem regardless of how often it fires. Fix:
+thread `eventLog` into `MovesJournal` and log persist failures at `error`
+level, mirroring the rest of the Persistence layer.
+
+**2. (Correctness — medium) `TimeLog.open`'s crash-reconciliation can
+fabricate solve time on a fast relaunch.** `TimeLog.swift:72-84`. When
+`reconcileStale` fires (first open of a game since launch, healing a session
+left dangling by a previous run's crash), the banked interval end is
+`boundedEnd = min(beatAt + openGrace(4min), openStart + maxSessionCap)` —
+never clamped to `now`. If the user relaunches well under 4 minutes after the
+crash, `boundedEnd` lands *after* the actual relaunch instant, so the sealed
+interval extends past `now` and the immediately-following live session
+(`openStart = now`) starts inside that still-"future" interval;
+`TimeLog.unionSeconds` then merges the overlap, crediting real elapsed time
+plus up to ~4 minutes of dead/crashed time that was never played. The
+existing test (`staleSessionReconciledOnReopen`,
+`Tests/Unit/TimeLogTests.swift:172`) only exercises `now=200_000`, far past
+`beatAt+openGrace=340`, so it never hits this boundary. Fix: also clamp
+`boundedEnd` to `now`.
+
+**3. (Performance — medium) `PlayerRoster.refresh()` blocks the main actor on
+a synchronous Core Data fetch instead of yielding it.** `PlayerRoster.swift:256`.
+`refresh()` is an `async @MainActor` method that creates a background context
+and then calls `ctx.performAndWait { ... }` (not `await ctx.perform`).
+`performAndWait` blocks the calling thread — the main thread here — until the
+background context finishes fetching the `GameEntity`, all its
+`PlayerEntity`/`MovesEntity` rows, and every nicknamed `FriendEntity`.
+`refresh()` is triggered by `.playerRosterShouldRefresh`, posted from
+`RecordApplier`/`SyncEngine` on roster-relevant inbound CloudKit changes,
+which can land mid co-solve session — each such event stalls the UI for the
+fetch's duration rather than freeing it, despite the `async` signature. This
+is a sharper, UI-facing instance of Phase 1 finding #4 (blocking Core Data
+work in async paths). Fix: `await ctx.perform { ... }`. Related render-path
+instance (second pass): `PlayerRoster.solveTime` (`PlayerRoster.swift:107-130`)
+fetches the `GameEntity` plus *every* `PlayerEntity` and JSON-decodes each
+`timeLog` synchronously on each call, and it drives the live header clock. It
+runs on `viewContext` (so a main-thread fetch is at least expected there), but
+it's the same "synchronous Core Data on the render path" theme and belongs in
+this cluster.
+
+**4. (Performance — medium) `GameEntity` has zero `fetchIndex` entries**
+despite `id`, `ckRecordName`, `databaseScope`, and `puzzleResourceID`/`title`
+being hit with exact-match `NSPredicate`s constantly —
+`"id == %@"` appears roughly 90 times in `GameStore.swift` alone (`loadGame`,
+`deleteGame`, `markCompleted`, `setEngagement`/`setNotification`,
+`ensurePushCredentials`, etc.), and `"ckRecordName == %@"` lookups against
+`GameEntity` appear across `RecordApplier`, `SyncEngine`, `RecordBuilder`,
+`RecordSerializer`, `MovesUpdater`, and `PlayerSelectionPublisher`. As the
+local library grows, every one of these becomes a full table scan. Add
+`fetchIndex`es on `id` and `ckRecordName` at minimum
+(`CrossmateModel.xcdatamodel/contents:3-44`). Note (second pass): these are
+Core Data `fetchIndex`es (local SQLite), not CloudKit schema — the app uses
+raw `CKSyncEngine`, not `NSPersistentCloudKitContainer` — so adding them (here
+and in #6) needs only a lightweight migration on next launch and *no*
+Production CloudKit schema deploy.
+
+**5. (Performance — low/medium) `MovesJournal` loads a game's entire journal
+synchronously on the main actor the first time it's touched.**
+`Journal.swift:361-364` (`ensureLoaded`) calls `load(_:)`, which runs an
+unbounded `JournalEntity` fetch via `ctx.performAndWait` (`:376-385`) — and
+`ensureLoaded` is called from `record`, `recordedEntries`, `canUndo`,
+`canRedo`, `planUndo`, and `planRedo`. `canUndo`/`canRedo` are read by the
+toolbar essentially as soon as a puzzle screen renders, so opening a
+heavily-edited or long-undo-history game can briefly stall the UI on this
+one-time-per-session fetch. Consider prefetching asynchronously before the
+puzzle screen becomes interactive, or fetching lazily off-main and updating
+state once ready.
+
+**6. (Performance — low) `MovesEntity`/`PlayerEntity` also lack a
+`ckRecordName` index.** Only composite indexes exist
+(`byGameAndAuthorAndDevice`, `byGameAndAuthor`), but plain
+`"ckRecordName == %@"` lookups against these entities happen in
+`GameStore.ensureMovesEntity` (`GameStore.swift:2769`), `RecordApplier`,
+`RecordSerializer`, `MovesUpdater`, and `PlayerSelectionPublisher`/
+`PlayerNamePublisher` — none benefit from the existing composite indexes.
+
+**7. (Code quality — low) `CellEntity` carries four dead attributes.**
+`ckRecordName`, `ckSystemFields`, `databaseScope`, and `lastSyncedAt`
+(`contents:81-90`) are declared on `CellEntity` but never read or written
+anywhere in the codebase (verified against every `CellEntity(context:`
+construction site). Looks like leftover schema from an earlier
+per-cell-record sync design, since replaced by the Moves-record model
+(`CellEntity` is now purely a derived local cache). Safe to drop.
+
+**Non-findings and deferred checks:**
+- `GameEntity`'s `cells`/`journal`/`moves`/`peerChanges`/`players`
+  relationships are all `deletionRule="Cascade"`, matching
+  `GameStore.resetAllData`'s doc-comment assumption — confirmed directly
+  against the schema, not just the comment.
+- `Game.swift`'s completion-cache bookkeeping (filled/wrong counts, gap-cell
+  exclusion, revealed-cell locking) matches `GameMutatorTests` exactly,
+  including author-preservation on a same-letter rewrite.
+- `CellMark`'s code/decode round-trip is lossless and exhaustive; unknown
+  codes safely default to `.none`.
+- `PlayerColor`'s companion/color assignment hashing is deterministic and
+  array-bounds-safe.
+- The manual `Puzzle.Direction` `rawValue`/`init?(rawValue:)` extension in
+  `PlayerSelection.swift:13-30` (across=0, down=1) is consistent with the
+  independent manual encoding `Journal.swift:447` uses for the same field —
+  checked directly since a mismatch here would silently flip undo/redo
+  cursor direction between the Core Data and JSON-upload paths.
+- No force-unwraps on untrusted/CloudKit-sourced data anywhere in this
+  file set.
+- `GameCursorStore.swift`, `GameViewedStore.swift`, `ParticipantSummaries.swift`:
+  straightforward, no bugs found.
+- `TipStore.markDismissed` re-inserts into an already-dismissed set on a
+  repeat dismiss, firing a redundant (harmless) `UserDefaults` write —
+  not worth a fix on its own. (The working-tree change to this file is
+  catalog-only — four new tips — and doesn't touch this path.)
+- The Phase 1 deferred check on non-monotonic `PlayerEntity.readThrough`
+  adoption resolves as a non-finding: `RecordApplier.swift:285-301` adopts it
+  under last-writer-wins *outside* the `updatedAt` guard deliberately, with a
+  bounded, self-healing dip documented in place (a still-present device
+  re-asserts its lease on draining the inbound close). The separate *local*
+  read watermark `GameEntity.readThroughAt` (`GameStore.swift:1766-1792`) is
+  properly monotonic.
+
+---
+
+## Phase 3 — Friends, Sharing & Invites
+
+**Status:** Done
+
+Friend graph, `CKShare`-based collaboration, invite links, and the two
+Workers that back link resolution and the realtime room.
+
+- `Crossmate/Sync/FriendController.swift`, `FriendZone.swift`
+- `Crossmate/Sync/ShareController.swift`
+- `Crossmate/Services/InviteCoordinator.swift`
+- `Crossmate/Services/ShareLinkRoute.swift`, `ShareLinkShortener.swift`
+- `Crossmate/Models/FriendEntity+DisplayName.swift`,
+  `InviteEntity+DisplayName.swift`
+- `Shared/NicknameDirectory.swift`, `FriendEncryptionKeyDirectory.swift`
+- `Crossmate/Services/KeychainHelper.swift`
+- `Crossmate/Views/Friends/*`
+- `Workers/link-worker.js`, `Workers/room-worker.js`
+- `Tests/Unit/Sync/FriendControllerNicknameReplayTests.swift`,
+  `FriendModelTests.swift`, `FriendZoneTests.swift`, `ShareRoutingTests.swift`,
+  `Tests/Unit/NicknameDirectoryTests.swift`, `ShareLinkRouteTests.swift`,
+  `ShareLinkShortenerTests.swift`
+
+### Findings (consolidated from Sonnet 5 and Opus 4.8, reviewed 2026-07-01)
+
+The dominant theme is the same trust-boundary class Phase 1 flagged: several
+inbound-`Decision` cases in `RecordSerializer.applyDecisionRecord` are adopted
+without checking the record's zone or database scope. Phase 3 makes that gap
+*concretely reachable*: a friendship gives the other party `.readWrite` on the
+`friend-<pairKey>` inbox zone **you own** (private DB, `scope == 0`), so any
+non-blocked friend can write arbitrary `Decision`/`Ping` records into a zone
+your private `CKSyncEngine` fetches. The two properly-gated cases (`name`,
+`nickname`) are the exception, not the rule — `block`, `left`, and
+`encryptionKey` all under-check. The `ShareController` owner gating, the two
+Workers' auth, and the link/token validation are all in good shape (see
+non-findings). The carry-over items are addressed as findings #1, #3, #4 (2)
+and non-findings (3).
+
+**1. (Security — high) `block` and `left` `Decision`s are adopted from any
+fetched zone, so a friend can tamper with your relationships and delete your
+local shared games.** `RecordSerializer.applyDecisionRecord`'s `block` case
+(`RecordSerializer.swift:1055-1074`) and `left` case (`:1075-1090`) apply with
+no zone/scope gate at all — unlike the sibling `nickname` case, which requires
+`record.recordID.zoneID.zoneName == accountZoneID.zoneName && databaseScope == 0`
+(`:1153-1155`). Both are legitimately written only into the private `account`
+zone (`FriendController.setBlocked` and `ShareController.leaveShare` both go
+through `SyncEngine.enqueueDecision`, which targets `accountZoneID`,
+`SyncEngine.swift:918-919`), but `applyDecisionRecord` runs for *every* fetched
+`Decision` on both engines (`SyncEngine.swift:1520-1525`). Concrete exploit: Bob
+is an accepted friend, so he holds `.readWrite` on Alice's inbox zone
+`friend-<pairKey(Alice,Bob)>` (`FriendController.saveZoneWideShare`,
+`FriendController.swift:609-623`). Bob saves a record named
+`decision-block-<Carol>` (`kind=block`, `payload="1"`, a large `version`) into
+that zone. Alice's private engine fetches her own inbox, the `block` case
+adopts it, and Alice's `FriendEntity` for Carol is marked `isBlocked` at Bob's
+version — silently blocking a third party (Alice can no longer invite Carol;
+`FriendController.sendInvite` throws `.friendBlocked`, `:366`) and forcing a
+version fight to undo it. The same write with `decision-left-<gameID>` hits the
+`left` case, which hard-deletes Alice's local participant `GameEntity` for any
+shared game she's in (`:1083-1090`) — a self-healing but disruptive
+kick/DoS. (Bob cannot un-block *himself* this way: blocking downgrades him to
+`.readOnly`, so he loses the inbox write access — the server-side enforcement
+holds. The exposure is tampering with the victim's state toward *third
+parties*, and `left`-driven local deletion.) Fix: gate both cases on the
+private `account` zone + `scope == 0`, exactly as `nickname` does.
+
+**2. (Security — medium) `applyFriendPing` accepts and starts syncing a share
+whose zone name is never checked against the bootstrap payload's `pairKey`.**
+`FriendController.applyFriendPing` validates the *acceptor* against the payload
+(`FriendZone.canAcceptBootstrap` requires
+`pairKey(localAuthorID, payload.ownerAuthorID) == payload.pairKey`,
+`FriendZone.swift:92-96`), but after fetching the share metadata it trusts the
+accepted zone wholesale: `outboxZoneID = metadata.share.recordID.zoneID`
+(`FriendController.swift:196`) with no check that
+`outboxZoneID.zoneName == FriendZone.zoneName(pairKey: payload.pairKey)`. A
+co-player (bootstrap Pings ride a shared *game* zone, so the attacker must
+already share a game) can mint a `CKShare` on a zone he owns but *names*
+`friend-<pairKey(Alice,Carol)>`, then send a `.friend` Ping whose payload sets
+`ownerAuthorID = Bob` and `pairKey = pairKey(Alice,Bob)` (so `canAcceptBootstrap`
+passes). Alice accepts and now syncs, at `scope == 1`, a zone Bob controls whose
+*name* impersonates the Alice–Carol pair. This is the enabler for finding #3,
+and independently means Bob-controlled `Decision`/`Ping` records flow into
+Alice's fetch under a forged pair identity. Fix: after fetching metadata, reject
+the accept unless `outboxZoneID.zoneName == FriendZone.zoneName(pairKey: payload.pairKey)`.
+
+**3. (Security — medium) `encryptionKey` `Decision` adoption omits the
+scope-0 gate its sibling `name` case has, allowing a third party's
+friend-channel key to be poisoned.** The `encryptionKey` case
+(`RecordSerializer.swift:1172-1187`) verifies the zone *name* matches
+`friend-<pairKey(localAuthorID, key)>` but — unlike the `name` case, which also
+requires `databaseScope == 0` (`:1116`) precisely to reject a record seen in a
+zone we merely joined — applies at any scope. Combined with finding #2, Bob (a
+mutual co-player who therefore knows Carol's authorID) shares a zone named
+`friend-<pairKey(Alice,Carol)>`, gets Alice to accept it (scope 1), and writes
+`decision-encryptionKey-<Carol>` carrying an attacker-chosen
+`FriendEncryptionKeyPayload`. Alice's fetch adopts it via
+`FriendEncryptionKeyDirectory.upsert(payload, for: Carol)`. That payload holds
+the `address` and `contentKey` used when Alice later sends Carol an invite push
+(`AccountPushCoordinator.swift:473`) and when the NSE decrypts one
+(`NotificationService.swift:62`), so a poisoned entry misroutes Alice's invite
+push for Carol to an attacker-chosen address and/or breaks decryption — an
+invite-push disclosure/DoS scoped to that pair. Fix: add `databaseScope == 0`
+to the `encryptionKey` gate (fixing #2 also closes the practical path).
+
+**4. (Security — high, fix owned by Phase 4) The friend inbox is the concrete
+writable zone that makes Phase 1 finding #1 (account push address/secret
+adoption) exploitable.** Phase 1 #1 noted that
+`parseAccountPushAddressDecision` / `parseAccountPushSecretDecision`
+(`RecordSerializer.swift:219`, `:245`) validate only record
+type/name/kind/payload and are called for every fetched `Decision`
+(`SyncEngine.swift:1514-1518`) but speculated about "a writable non-account
+zone." Phase 3 confirms one exists and is trivially reachable: any accepted
+friend holds `.readWrite` on your `friend-<pairKey>` inbox in the **private**
+database (`scope == 0`). A friend can therefore save a record named
+`decision-account-pushSecret` (`kind=account`, attacker payload, high `version`)
+into that inbox; your private engine fetches it and surfaces it to
+`onAccountPushSecret`. Because the push secret is the HMAC key from which every
+per-game push address is derived (`RecordSerializer.deriveGameAddress`,
+`:263-273`), adopting a foreign secret lets an attacker DoS push delivery
+across all of the victim's games and, if the attacker knows the secret they
+planted, predict/target the victim's per-game push addresses. This raises the
+practical severity of Phase 1 #1. `applyDecisionRecord`'s own `account`-kind
+handling isn't the issue (there is none); the gap is these two dedicated
+parsers plus their unconditional call site. Phase 4 should gate both parsers
+(or their fetch call site) on the private `account` zone + `scope == 0` when it
+reviews `AccountPushCoordinator`. Recorded here because Phase 3 surfaces the
+attack vector; the adoption/republish code is Phase 4.
+
+**5. (Security/resource — low) An `.invite` Ping's `puzzleSource` is trusted
+and materialised into a playable game before any shared-zone validation, with
+no size cap.** `InviteCoordinator.applyInvitePings` stores
+`payload.puzzleSource` from the friend-zone Ping onto the durable `InviteEntity`
+(`InviteCoordinator.swift:217`), and `acceptInvite` feeds it to
+`cloudService.acceptShare(prefetchedPuzzleSource:)` to build a local game before
+the shared-zone fetch (`:307-313`). The inviter controls this string, and the
+friend-zone `Ping.payload` is an unindexed record field bounded only by
+CloudKit's ~1 MB record limit — the comment (`FriendZone.swift:128-134`)
+assumes "a few KB." A malicious inviter can ship a ~1 MB blob to bloat the
+invitee's `InviteEntity` and force an oversized XD parse on accept. This is the
+friend-invite analogue of Phase 1 finding #3 (`puzzleSource` CKAsset without a
+size cap); the XD-parse hardening itself is Phase 5. Fix: enforce a
+conservative byte cap on `payload.puzzleSource` before storing/using it, and
+treat oversize as "fetch from the shared zone instead."
+
+**6. (Security/resource — low) `room-worker.js` allows unauthenticated room
+creation and has no rate limiting.** `register` (`room-worker.js:81-109`)
+creates a Durable Object and persists a secret for *any* well-formed body
+(`isAcceptableSecret` only requires ≥32 decoded bytes, `:249-258`); the room ID
+is an arbitrary path segment (`roomRouteFromPath`, `:242-245`) fed straight to
+`idFromName`. Anyone can therefore mint unbounded rooms (storage/cost) with no
+auth and no per-IP throttle on `register`/`socket`. Rooms self-expire on idle
+TTL (`alarm` → `deleteAll`, `:191-196`), so the blast radius is bounded in time,
+but a burst is uncapped. The TOFU secret model itself is sound — first-write
+wins, `timingSafeEqual` on re-register, and the secret never travels on a
+connect (`:93-104`, `:125-128`). Fix: consider a lightweight rate limit / proof
+requirement on `register`, or accept the risk explicitly given the TTL bound.
+(In-room `authorID`/`deviceID` are self-asserted query params signed by the
+*shared* room secret, so any room member can connect as any authorID and the
+relay does not bind frame authorship — an engagement/presence spoofing surface
+that belongs to Phase 4's engagement review.)
+
+**7. (Code quality — low) `FriendController.seedOwnNameDecision`'s `scope`
+parameter is effectively fixed.** It is only ever called with `scope: 1`
+(`FriendController.swift:206`), and `establish` doesn't seed a name at all
+(deferred to the acceptor's `applyFriendPing`). Not a bug, but the parameter
+implies a flexibility that doesn't exist; inline it or document the single
+caller.
+
+**8. (Security — medium) A `.decline` Ping is trusted to evict a share
+participant without checking that the decliner was ever invited to the named
+game.** `InviteCoordinator.applyDeclinePing` (`InviteCoordinator.swift:700-
+725`) acts on any inbound `.decline` Ping addressed to the local user and
+calls `ShareController.removeFriendParticipant(fromGameID: ping.gameID,
+userRecordName: ping.authorID)` (`ShareController.swift:211-254`). Both
+`ping.gameID` and `ping.authorID` are plain CKRecord fields
+(`Ping.parseRecord`, `Presence.swift:77-108`) on a record written into the
+*recipient's own inbox* zone — i.e. self-reported by whichever friend wrote
+it, with no check that this particular friend has any connection to the
+named game at all. `removeFriendParticipant` only checks that the local user
+owns the named game (`databaseScope == 0`) and that the named
+`userRecordName` currently holds a seat on *that* game's share; it never
+confirms a matching outstanding invite exists for `(gameID, authorID)`, or
+that the Ping's sender is the one who received that invite. Concretely: any
+established friend — not necessarily a collaborator on the target game — who
+can name a live `(gameID, authorID)` pair for one of the owner's *other*
+shared games (e.g. a former collaborator on that other game, or anyone who
+otherwise learned the pair) can forge a `.decline` Ping into the owner's
+inbox and evict that unrelated participant from a game the forger was never
+part of. The practical bar is knowing a live `(gameID, authorID)` pair, but
+nothing ties the claimed decliner identity to the actual CloudKit writer, and
+every accepted friend has permanent inbox write access (the same durable
+property finding #1 relies on) to attempt it at will. Fix: before freeing the
+seat, cross-check against a durable record of invites this owner actually
+sent for that `(gameID, authorID)` pair rather than trusting the Ping's
+self-reported fields.
+
+**9. (Code quality — low) `ShareController` hardcodes the zone-wide share
+record name instead of reusing the existing constant.**
+`ShareController.zoneWideShareRecordName = "cloudkit.zoneshare"`
+(`ShareController.swift:10`) duplicates the value of the system constant
+`CKRecordNameZoneWideShare`, which `FriendController.swift:521,632` and
+`PlayerRoster.swift:525` already use directly for the identical purpose.
+Harmless today (the literal is correct), but needless duplication that could
+silently drift. Replace with `CKRecordNameZoneWideShare`.
+
+**Non-findings and deferred checks:**
+- **Carry-over #2 resolved:** `nickname`'s account-zone+scope-0 gate
+  (`RecordSerializer.swift:1153-1155`) is *not* the lone exception — `name`
+  carries an equivalent pair-zone-name + `scope == 0` gate (`:1106-1116`), and
+  both are correct (the `name` gate even documents rejecting a scope-1 sighting
+  of the friend's own inbox). The exceptions that *under*-check are `block`,
+  `left` (finding #1) and `encryptionKey` (finding #3).
+- **Carry-over #3 (`ShareController` join confirmation) resolved as a
+  non-finding:** `confirmSeatAfterJoin` / `hasLostSeat`
+  (`ShareController.swift:684-732`) is explicitly *cooperative* — CloudKit can't
+  enforce a participant cap, so an over-cap joiner leaves voluntarily and a
+  transient failure lets the join stand. It reads its own identity from
+  `container.userRecordID()` and the server share, adds no trust boundary, and a
+  non-cooperating client is the documented, accepted residual. Ticket-seat
+  consumption uses correct optimistic-lock retries (`:852-897`).
+- `ShareController`'s owner-only gating is consistent and correct:
+  `prepareShareRecord`, `createShareLink`, `addFriendParticipant`,
+  `removeFriendParticipant`, and `existingShareLink` all guard
+  `entity.databaseScope == 0`; `leaveShare`/`confirmSeatAfterJoin` guard
+  `== 1`. `removeParticipant` refuses to drop the owner (`role != .owner`).
+- The `sessionInvitedAuthorIDs` re-assert on every invite save correctly
+  guards the eventually-consistent-share window without ever *removing* a
+  participant a fetched share carries (`ShareController.swift:44-51`,
+  `:171-195`).
+- `room-worker.js` connect auth is solid: HMAC-SHA256 over
+  `roomID|authorID|deviceID|timestamp|nonce`, ±120 s skew window, single-use
+  nonces with TTL pruning, and `timingSafeEqual` on the signature
+  (`:111-155`). No secret is logged or returned.
+- `link-worker.js` is a safe stateless redirector: the token is constrained to
+  RFC 3986 unreserved chars (`TOKEN_PATTERN`, `:45`) so the target can only ever
+  be `https://www.icloud.com/share/<token>` (no open redirect), title decode is
+  charset-checked → strict-UTF-8 → control-stripped → capped → HTML-escaped at
+  the interpolation site (`:279-301`), and every other interpolated value
+  (`token`, `origin`, `shape`) is charset-validated upstream. `escapeHTML` omits
+  `'`, but every interpolation site is a double-quoted attribute or element text
+  where `'` is inert. Silhouette rendering is bounded to ≤35×35
+  (`parseShape`, `:232`). No ReDoS in the crawler/token patterns.
+- `ShareLinkRoute` / `ShareLinkShortener` token validation mirrors the worker's
+  and rejects percent-encoded slash smuggling (`ShareLinkShortenerTests` line
+  156-165); `iCloudShareURL`'s force-unwrap is safe behind the charset check.
+- Invite tokens are not a Crossmate secret to guess: joining is gated by the
+  underlying `CKShare` acceptance (Apple-enforced), not by link
+  unguessability; the short link carries only the iCloud share token, which is
+  as strong as CloudKit makes it.
+- `KeychainHelper` uses `kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly`
+  (no iCloud keychain sync, sensible for device-local secrets), update-then-add
+  upsert, and surfaces non-`errSecItemNotFound` failures as thrown errors.
+- `NicknameDirectory` / `FriendEncryptionKeyDirectory` are App-Group JSON
+  mirrors rebuilt from Core Data ground truth; empty-save removes the file, and
+  the `@TaskLocal` test override keeps parallel suites isolated. (The *source*
+  of a `FriendEncryptionKeyDirectory` entry is the trust gap in finding #3, not
+  the directory storage itself.)
+- `FriendZone.pairKey` is a symmetric SHA-256 of the sorted author pair
+  (64-hex, deterministic, self-excluding), giving both devices the same zone
+  name without coordination; covered by `FriendZoneTests`.
+- Re-check the `encryptionKey`/push-address/push-secret *adoption* side and the
+  in-room `authorID` self-assertion in Phase 4 alongside
+  `AccountPushCoordinator` and the engagement layer (findings #4, #6).
+
+---
+
+## Phase 4 — Push Notifications & Engagement
+
+**Status:** Done
+
+Push credential registration, badge/session pushes, the realtime engagement
+(co-solving) layer, the notification service extension, and the push Worker.
+
+- `Crossmate/Services/AccountPushCoordinator.swift`, `PushClient.swift`,
+  `PushRequestAuthenticator.swift`, `BadgeCoordinator.swift`
+- `Crossmate/Sync/GamePushCredentials.swift`
+- `Crossmate/Services/SessionPushPlanner.swift`, `SessionCoordinator.swift`
+- `Crossmate/Services/EngagementHost.swift`,
+  `EngagementHostEnvironment.swift`, `EngagementLifecycle.swift`
+- `Crossmate/Sync/EngagementCoordinator.swift`
+- `Crossmate/Services/AnnouncementCenter.swift`,
+  `PlayerNamePublisher.swift`, `DriveMonitor.swift`, `InputMonitor.swift`
+- `Shared/PushPayload.swift`, `PushPayloadCipher.swift`,
+  `NotificationState.swift`, `PuzzleNotificationText.swift`
+- `Crossmate/Models/PuzzleNotificationText+GameEntity.swift`
+- `NotificationService/NotificationService.swift`
+- `Workers/push-worker.js`
+- `Tests/Unit/AccountPushCoordinatorTests.swift`,
+  `AnnouncementCenterTests.swift`, `Tests/Unit/Sync/GamePushCredentialsTests.swift`,
+  `Tests/Unit/Sync/EngagementCoordinatorTests.swift`,
+  `SessionPushPlannerTests.swift`, `PlayerNamePublisherTests.swift`,
+  `NotificationStateTests.swift`, `PuzzleNotificationTextTests.swift`,
+  `PushPayloadTests.swift`, `PushPayloadCipherTests.swift`,
+  `NotificationNavigationBrokerTests.swift`, `OpenPuzzleBannerTests.swift`,
+  `PlayerSessionNavigationTests.swift`
+
+### Findings (consolidated from Sonnet 5 and Opus 4.8, reviewed 2026-07-01)
+
+Both carry-over items from Phase 1/3 resolve *in* this phase, and both are
+concrete, high-severity gaps rather than theoretical ones: the account
+push-secret adoption path (carried since Phase 1) has no defense-in-depth
+anywhere downstream of the ungated parsers, and the engagement relay's
+self-asserted identity (carried since Phase 3) turns out to durably corrupt
+shared grid content, not just spoof presence. Elsewhere the push/crypto core —
+App Attest enrolment, HMAC request signing, AES-GCM payload sealing — is
+sound and both passes independently verified it closely. The recurring
+"blocking Core Data on an async main-actor path" theme from Phases 1/2
+reappears at three new call sites, found independently by both passes.
+
+**1. (Security — high) Account push address/secret `Decision`s are adopted
+with no account-zone gate anywhere in the pipeline, and a friend can hijack
+the HMAC push secret with an equal-version write.** This is the confirmed
+resolution of Phase 1 finding #1 / Phase 3 finding #4; the fix is owned here.
+`RecordSerializer.parseAccountPushAddressDecision` / `parseAccountPushSecretDecision`
+(`RecordSerializer.swift:219-227`, `:245-255`) validate only record
+type/name/kind/payload — never zone or database scope.
+`SyncEngine.handleFetchedRecordZoneChanges`'s `Decision` case
+(`SyncEngine.swift:1513-1519`) calls both parsers for every fetched Decision
+on both engines; `scope` is already in scope two lines later (passed into
+`applyDecisionRecord(..., databaseScope: scope)`) but is never checked before
+appending to `effects.accountPushAddresses`/`accountPushSecrets`. The
+`serverRecordChanged` conflict twin (`SyncEngine.swift:1888-1894`) has the
+same gap, lower-exposure since it only fires on this device's own
+account-zone save attempts.
+
+Tracing the adoption side confirms there is no second line of defense.
+`AccountPushCoordinator.adoptInboundPushAddress` (`AccountPushCoordinator.swift:126-130`)
+unconditionally caches whatever address it's handed and reconciles.
+`adoptInboundPushSecret` (`:140-149`) gates only on
+`version > local || (version == local && secret != current)` — and because a
+`Decision` with no `version` field maps to `decisionBaseVersion` (1), the
+*same* value a fresh local mint uses (`RecordSerializer.swift:235`,
+`AccountPushCoordinator.swift:350`), an attacker doesn't even need a high
+version: a `decision-account-pushSecret` record with no version field and a
+differing payload wins outright under the equal-version "server wins" branch.
+Adoption caches the foreign secret and calls `reconcilePushRegistration`,
+which re-derives every per-game push address via `deriveGameAddress`
+(`RecordSerializer.swift:263`), rewrites local Player rows, and — per
+`updatePushRegistration(republishPlayerRows: true)`
+(`AccountPushCoordinator.swift:168-190`) — re-publishes the poisoned
+derivation into every shared game's synced `Player.pushAddress` field via
+`syncEngine.enqueuePlayer(gameID:authorID:reason:"pushAddress")`
+(`:178-184`), visible to every co-player in each affected game, not just the
+attacker. Combined with Phase 3's confirmed reachable vector (any accepted
+friend holds `.readWrite` on your private-DB `friend-<pairKey>` inbox, which
+your private engine fetches), this DoSes push delivery across the victim's
+entire push surface and, since the attacker chose the secret, lets them
+predict/target the victim's derived push address for any game.
+
+Fix: gate both call sites (`SyncEngine.swift:1513-1519` and `:1888-1894`) on
+`record.recordID.zoneID.zoneName == accountZoneID.zoneName && databaseScope == 0`,
+mirroring the `nickname`/`name` gates already present in
+`applyDecisionRecord` (`RecordSerializer.swift:1106-1116`, `:1153-1155`). The
+call site is the right place — the parsers are also used on the local-save
+reconciliation path where scope/zoneID aren't otherwise threaded through and
+the gate would be redundant. Separately, tighten the equal-version branch so
+a foreign inbox copy with a missing/matching version can never win.
+
+**2. (Security — high) The realtime engagement channel trusts each frame's
+self-asserted `authorID`/`deviceID`/`cellAuthorID`/`updatedAt`, letting a
+room member durably corrupt and misattribute shared grid content — the
+concrete client-side consequence of Phase 3 finding #6's spoofable room
+identity.** `room-worker.js`'s `webSocketMessage` (`room-worker.js:157-170`)
+is a blind byte relay: `peer.send(data)` forwards the raw frame to every
+other connected socket with zero inspection or identity binding — so a
+frame's `authorID`/`cellAuthorID`/`deviceID` fields are entirely
+self-chosen, not even tied to the already-spoofable connect-time query-string
+`authorID` Phase 3 flagged. `EngagementLifecycle.handleEngagementMessage`'s
+`.cellEdit`/`.cellEditBatch` cases (`EngagementLifecycle.swift:346-376`) hand
+the decoded edit straight to `store.applyRealtimeCellEdit(s)` with no
+roster/identity check. `GameStore.applyRealtimeCellEdits`
+(`GameStore.swift:884-953`) only rejects an empty `authorID`/`deviceID` or a
+`deviceID` equal to the *local* device (`:897-902`) — unlike
+`EngagementStore.set` (`EngagementStore.swift:48-65`), which at least
+requires a cursor's `authorID` to resolve to a known roster player color
+before accepting it. The edit's `letter`/`mark`/`updatedAt`/`cellAuthorID`
+are written straight into the same local `MovesEntity` cache that legitimate
+CloudKit-fetched Moves records for that identity populate, using the
+attacker-supplied `updatedAt` unclamped against the local clock.
+
+The escalation: when the real CloudKit Moves record for the impersonated
+`(authorID, deviceID)` eventually syncs, `RecordSerializer.applyMovesRecord`
+merges cell-by-cell against the existing cache via `mergeIncomingMovesCells`
+(`RecordSerializer.swift:1014-1027`, the same tie-break flagged in Phase 1
+finding #9), keeping whichever `updatedAt` is newer. A frame stamped with a
+far-future `updatedAt` therefore wins that per-cell LWW race permanently —
+the genuine author's real, later edits to that exact cell can never overwrite
+it again through incremental sync, on any device that was live-connected to
+the room when the frame arrived. Concrete exploit: Bob, an accepted
+collaborator (so he legitimately holds the room secret), sends a
+`cellEditBatch` claiming `authorID`/`cellAuthorID` = Carol (a real co-player)
+with `updatedAt` far in the future; every live peer applies it, and Carol's
+genuine future keystrokes at that cell never win again on those devices.
+Mitigating factors: presence (`hasPeer`) is derived from `Player.readAt`
+leases in Core Data, not the room (`EngagementLifecycle.swift:116-121`), so
+the spoof can't fake presence; no push is emitted from an inbound frame; and
+the corruption never re-uploads to CloudKit (outbound Moves are always built
+from the local user's own identity), so blast radius is bounded to devices
+live-connected at attack time — but for those devices it is durable, not
+transient.
+
+Fix: have `room-worker.js` stamp the connection's own HMAC-verified
+`authorID`/`deviceID` onto (or strip from) relayed frames rather than passing
+client-chosen identity through untouched; on the client, validate
+`edit.authorID` against the known roster before writing into the durable
+cache (mirroring `EngagementStore.set`'s gate); and/or clamp
+`edit.updatedAt` to `≤ now + small skew` so a spoof can't win LWW durably —
+ideally combined with keeping live-channel content in a short-lived overlay
+that any subsequent real CloudKit fetch unconditionally supersedes, rather
+than merging it into the persisted cache CloudKit's own merge also reads
+from.
+
+**3. (Security/Availability — medium) `push-worker.js` has no rate limiting
+at either the authenticated `/publish` endpoint or the unauthenticated App
+Attest bootstrap endpoints.** Every client-side send throttle
+(`SessionCoordinator.nudgeCooldown` = 60s,
+`AccountPushCoordinator.accountSeenCoalesceWindow` = 30s, etc.) is enforced
+only in `PushClient`/`SessionCoordinator` — nothing in `handlePublish`
+(`push-worker.js:414-497`) limits how often a given `credID`/address may be
+published to. A malicious collaborator already has everything needed to
+bypass the app entirely: `GamePushCredentials` (secret + `credID`) is synced
+into the Game record's `notification` field for every participant
+(`RecordSerializer.swift:413`, `:858-868`), and every other participant's
+derived `pushAddress` is a plain field on the synced `Player` record
+(`RecordSerializer.swift:560-563`, `:624-631`) — both readable directly off
+CloudKit by any collaborator. A co-player can script raw, correctly-signed
+`/publish` calls at unlimited frequency, sending real alert pushes (sound +
+banner) to every co-player with none of the app's UI-layer cooldowns
+applying — the caller is fully App-Attest-authenticated, so this can't be
+dismissed as requiring attacker infrastructure. Separately, `/attest/challenge`
+and `/attest/register` (`push-worker.js:143-162`, `:164-222`) necessarily run
+*before* auth (they bootstrap the key) and take an attacker-chosen
+`deviceID`; `pruneExpired` only sweeps the same `deviceID` prefix
+(`:153`, `:313-323`), so an attacker rotating `deviceID` accumulates
+`appattest-challenge:*` storage rows faster than they TTL out. Both are the
+same class of gap Phase 3 flagged for `room-worker.js`'s open `register`
+(finding #6); rooms/challenges self-expire so the blast radius is
+time-bounded, but a burst is uncapped. Fix: add a per-`credID`/per-address
+sliding-window rate limit on `/publish`, and a lightweight per-IP/per-device
+throttle on the attest bootstrap endpoints.
+
+**4. (Performance — medium) Three new call sites repeat the recurring Phase
+1/2 "blocking Core Data on an async `@MainActor` path" pattern — found
+independently by both review passes.** Each creates a background context and
+calls `ctx.performAndWait { ... }` synchronously from an `@MainActor` chain
+with no intervening `await`, blocking the main thread for the fetch's
+duration despite the enclosing methods being `async`: `SessionCoordinator.pushPlan`
+(`SessionCoordinator.swift:521-553`), invoked from `nudge`,
+`publishJoinPush`, `publishSessionEndPush`, `publishCompletionPush`, and
+`publishReplayPush` — all reachable from the active puzzle's foreground path,
+and the fetch pulls the game plus all its `PlayerEntity` rows;
+`AccountPushCoordinator.friendEncryptionKeyTargets`
+(`AccountPushCoordinator.swift:232-251`), invoked from every
+`reconcilePushRegistration` and `publishInvitePush`; and
+`PlayerNamePublisher.friendZoneTargets` / `upsertPlayerRecord`
+(`PlayerNamePublisher.swift:167`, `:192`), invoked from `fanOut`/
+`publishName(for:)` — the latter on every shared-puzzle open. Fix: convert to
+`await ctx.perform { ... }` throughout.
+
+**5. (Resource — low) Game push credentials are never expired or cleaned up
+after a game is deleted or archived.** `GamePushCredentials` is deliberately
+durable/no-expiry by design (`GamePushCredentials.swift:25`), unlike
+`EngagementRoomCredentials`'s 10-minute TTL, but no client call anywhere
+unregisters a game credential or its bound addresses from `push-worker.js` —
+`PushClient.unregister(bindings:)` exists but is only driven by
+`setAddresses`'s diff against the current live binding set, never by game
+deletion. This slowly accumulates orphaned `gamecred:<uuid>` and
+`addr:<credID>:*` Durable Object storage entries for every game a user ever
+plays and later deletes/archives.
+
+**6. (Correctness/privacy — low) A failed push `unregister` is silently
+forgotten, leaving a stale address registered with the worker.**
+`PushClient.reconcile` (`PushClient.swift:170-203`) awaits
+`unregister(bindings:)`, which catches and swallows its own errors
+(`:448-467`) rather than throwing, then unconditionally advances
+`lastRegistered = signature` (`:196`). If the DELETE fails (transport blip,
+worker 5xx), the removed bindings drop out of `lastRegistered.bindings`, so
+the next reconcile computes an empty `toRemove` and never retries — the
+stale address→token mapping persists on the worker, so after leaving a game
+or switching accounts the device can keep receiving that game's pushes until
+an APNs 410 or token rotation clears it. Fix: only advance `lastRegistered`
+for bindings that actually unregistered, or re-drive removals until
+confirmed — the register side already models this correctly by leaving
+`lastRegistered` unchanged on failure.
+
+**Non-findings and deferred checks:**
+- `PushPayloadCipher` uses AES-GCM correctly (combined nonce‖ciphertext‖tag
+  box, random nonce, ≥32-byte key enforcement) and fails closed to `nil` on
+  any decode/verify failure — no plaintext leak on error
+  (`PushPayloadCipher.swift:26-52`). `NotificationService.swift:52-98`
+  correctly falls back to the generic cleartext body when `enc` is absent or
+  undecryptable, and never force-unwraps worker-sourced fields.
+- `PushRequestAuthenticator`/`push-worker.js`'s App Attest scheme (challenge/
+  attest/assertion, full CBOR/DER/cert-chain parsing, per-device
+  nonce+timestamp binding, `timingSafeEqual` throughout) is sound and matches
+  Apple's documented scheme; `GamePushSigner`'s HMAC game-participation
+  signature (`credID|bodyHash|timestamp|nonce`, with `bodyHash` covering
+  `credID`) is correctly bound so `credID` can't be swapped without breaking
+  App-Attest auth. Considerably more robust than `room-worker.js`'s
+  HMAC-only connect auth reviewed in Phase 3.
+- `push-worker.js`'s game-credential registration and `/publish` *are* gated
+  behind full App Attest auth (checked before routing,
+  `push-worker.js:19-49`) — a meaningful contrast with `room-worker.js`'s
+  fully open `register` (Phase 3 finding #6); only the missing rate limit
+  (finding #3) carries equivalent risk forward.
+- Account-scoped pushes (the `acct-<UUID>` address) are not spoofable
+  without the account zone: the address is random, not secret-derived, and
+  published only into the private account zone
+  (`push-worker.js:405-413`, `:529-563`).
+- `EngagementHost`'s socket connect auth mirrors the Phase-3-approved
+  room-worker connect auth (HMAC over
+  `roomID|authorID|deviceID|timestamp|nonce`, secret never in the URL);
+  `EngagementCoordinator`'s connect/migrate/teardown state machine and
+  stale-connection sweep are clean.
+- `applyGameRecord`'s adoption of the `engagement`/`notification` fields
+  (`RecordSerializer.swift:848-868`) is correctly scoped — these live on the
+  Game record itself, already resolved to the correct per-game zone/entity,
+  so "whoever can write this game's own zone" is the intended trust boundary
+  (the game's own CKShare participants), unlike the account-zone Decision
+  gap in finding #1.
+- `AnnouncementCenter`, `BadgeCoordinator`/`NotificationState`'s
+  `seenAt`/`suppressedUntil` ledger, `PuzzleNotificationText`(+`GameEntity`),
+  `DriveMonitor`, `InputMonitor` are straightforward and correct; no bugs
+  found. No force-unwraps on untrusted CloudKit- or worker-sourced data
+  anywhere in the Phase 4 file set.
+- `GamePushCredentialsTests` confirms the HMAC key-derivation contract
+  (base64url-decoded secret bytes, not UTF-8) matches the worker's
+  `hmacSHA256` exactly — no drift risk.
+- `fromAuthorID` on a push is worker-forwarded and attacker-settable but is
+  display-only: the NSE only rewrites the body when the sealed payload
+  actually decrypts, so a spoofed `fromAuthorID` on an undecryptable push
+  yields the generic body, not a nickname leak.
+- Carry over to **Phase 5**: `DriveMonitor.readSource`/`importFile` reads a
+  user-selected local/iCloud-Drive `.xd`/`.puz` file with no size cap before
+  `PUZToXDConverter.convert` — not a remote trust boundary like Phase 1 #3 /
+  Phase 3 #5, but worth folding into Phase 5's general XD/PUZ parse hardening
+  review.
+- Carry over to **Phase 6**: finding #2's exploit rides the same LWW
+  tie-break flagged in Phase 1 finding #9; a `now`-clamp or monotonic guard
+  there would also harden the CloudKit ingest path, worth revisiting
+  alongside gameplay/grid state in Phase 6.
+- Carry over to **Phase 7**: `NotificationNavigationBroker`/
+  `PlayerSessionNavigation` live in the app-shell composition root, outside
+  this phase's file list; a shallow look shows they only route an
+  already-parsed `gameID` to in-app navigation with no obvious trust-boundary
+  issue, but they deserve a full pass in Phase 7.
+
+---
+
+## Phase 5 — Puzzle Import & Format Conversion
+
+**Status:** Done
+
+Bringing puzzles into the app: NYT auth/fetch, `.puz`/NYT-JSON → XD
+conversion, and the XD format itself.
+
+- `Crossmate/Services/NYTAuthService.swift`, `NYTPuzzleFetcher.swift`,
+  `NYTPuzzleUpgrader.swift`, `NYTToXDConverter.swift`
+- `Crossmate/Services/PUZToXDConverter.swift`, `ImportService.swift`
+- `Crossmate/Models/XD.swift`, `XDFileType.swift`, `XDMarkup.swift`
+- `Crossmate/Models/PuzzleCatalog.swift`, `PuzzleSource.swift`,
+  `Puzzle.swift`
+- `Puzzles/cm-starter/*`, `Puzzles/debug/*`
+- `Tests/Unit/NYTAuthServiceTests.swift`, `NYTPuzzleUpgraderTests.swift`,
+  `NYTToXDConverterTests.swift`, `PUZToXDConverterTests.swift`,
+  `PuzzleCatalogTests.swift`, `XDAcceptTests.swift`, `XDMarkupTests.swift`
+
+### Findings (consolidated from Sonnet 5 and Opus 4.8, reviewed 2026-07-01)
+
+This is the phase where the Phase 1 finding #3 / Phase 3 finding #5 carry-over
+("`puzzleSource` reaches the parser with no size cap") cashes out concretely —
+and the result is sharper than either earlier phase assumed: the parser itself
+has an unmemoized exponential algorithm that a **sub-300-byte** input can drive
+into a multi-hour hang, so a byte-size cap alone would not have closed the
+gap. Both passes found this independently; Sonnet 5 additionally built a
+standalone harness from the real source and empirically measured it. The
+second major finding — a `.puz` rebus-placeholder allocator that both
+misassigns reserved characters and traps on `UInt8` overflow — was also found
+independently by both passes and empirically confirmed. All file:line
+references below were spot-checked directly against the current source.
+
+**1. (Security/Performance — high, empirically confirmed) `XD.segment` is an
+unmemoized exponential backtracking search, reachable from attacker-controlled
+`puzzleSource` on the CloudKit sync path and, via invite acceptance, on the
+main actor.** `XD.segment` (`XD.swift:705-748`) assigns free-length substrings
+from a clue's `~` answer onto a word's still-unsolved cells via plain
+recursion (`recurse(cellIndex:answerIndex:current:)`, `:706-742`). The
+`maxResults` cap (`results.count < maxResults` at `:709`) only prunes *after*
+a complete valid segmentation is found; when the crafted answer text has
+**zero** valid segmentations against a word's already-crossing-fixed cells,
+the full composition tree is exhausted before returning empty. For a run of
+*E* adjacent unsolved cells constrained only by one trailing fixed-letter
+cell, this is `O(2^E)`-ish combinatorial work. Sonnet 5 reproduced this on
+real device hardware from the extracted source: a 2-column synthetic grid (one
+down-word of unconstrained cells + one crossing-fixed cell) measured 2.13s at
+14 free cells / slack 10 (~1.14M compositions) and **32.2s at 16 free cells /
+slack 12** (~17.4M compositions, from a ~250-byte source file), extrapolating
+to minutes-to-hours at 18-20 free cells — all comfortably under 1 KB.
+
+Reachability has no existing mitigation: `RecordSerializer.applyGameRecord`
+calls `XD.parse(source)` on the synced Game record's `puzzleSource` CKAsset
+with no complexity guard (`RecordSerializer.swift:875`) — the same asset
+Phase 1 #3 flagged for size only. Concretely for invites, the exact Phase 3 #5
+vector completes the chain: `InviteCoordinator.acceptInvite`'s
+inviter-controlled `payload.puzzleSource` flows through
+`CloudService.acceptShare(prefetchedPuzzleSource:)`
+(`CloudService.swift:55`, `:82`) into
+`GameStore.constructJoinedGame(gameID:zoneID:source:)`
+(`GameStore.swift:1202`), which calls `try XD.parse(source)` directly at
+`:1209` — and `GameStore` is `@MainActor` (`GameStore.swift:504`), confirmed
+by direct inspection, so this is a **synchronous main-thread hang**: a friend
+sends one crafted invite and the victim's UI freezes solid the moment they tap
+Accept (likely followed by an iOS watchdog kill). The same call also fires at
+ordinary game-open/local `.xd` import. Recursion depth is bounded by word
+length, so this is pure combinatorial-time DoS, not a stack overflow. Fix:
+memoize `segment` on `(cellIndex, answerIndex)` — this turns it into standard
+`O(word.count × answer.count)` word-break DP, since the failing subproblems
+repeat — and/or cap total recursive-call count and free-cell run length ahead
+of a fail-closed error. This finding supersedes the "just add a size cap"
+framing from Phase 1 #3 / Phase 3 #5: a byte cap remains worth adding for
+memory/parse-cost reasons generally (see #5), but it does not bound this
+specific attack.
+
+**2. (Correctness/Security — high, empirically confirmed) `PUZToXDConverter`'s
+rebus-placeholder allocator is keyed by cell index instead of by distinct
+rebus value, so it both misassigns reserved grammar characters at low counts
+and hard-crashes via `UInt8` overflow past ~207 rebus-covered cells.** The
+loop at `PUZToXDConverter.swift:78-87` walks every rebus-covered cell once
+(`for index in 0..<cellCount where isOpen(...)`) and assigns a fresh key from
+`nextRebusKey`, plain (non-`&+=`) `UInt8` arithmetic starting at
+`Character("1").asciiValue!`: `guard rebusKeys[index] == nil else { continue }`
+never fires because `rebusKeys` is keyed by cell **index** and each index is
+visited exactly once — the guard cannot dedupe by *value*, unlike the correct
+value-keyed `NYTToXDConverter.rebusLookup` (`NYTToXDConverter.swift:150-163`,
+whose header comment at `:22-23` explicitly documents having already hit and
+fixed this exact class of bug: "walking ASCII naïvely from `'1'` overflows
+into `=` by the 13th key, which produced an unparseable `==VALUE` entry"). So
+even a `.puz` with only a handful of *distinct* rebus values spread across
+enough cells burns through placeholders one-per-cell: the 13th assigned key is
+`=`, breaking the `Rebus: key=value` grammar into an unparseable entry; the
+16th is `@`, which silently re-parses as a *circled*-cell marker instead of a
+rebus key. At the 207th rebus-covered cell, `nextRebusKey` reaches 256 and
+`nextRebusKey += 1` **traps**, crashing the app outright. Sonnet 5 confirmed
+the crash empirically against a synthetic 256-cell 16×16 `.puz` with only 3
+distinct rebus values mapped across all cells (exit 133, fatal arithmetic-overflow
+trap), proving the bug is cell-count-keyed, not distinct-value-keyed as a
+size-based mitigation might assume. `PUZToXDConverterTests.swift` has no
+rebus/`GRBS`/`RTBL` coverage at all, which is exactly why this shipped
+unnoticed. Reachable via any locally-imported `.puz` (Files app, AirDrop,
+email) — a sufficiently rebus-heavy variety puzzle could trigger the
+`=`/`@` corruption by accident, no malice required. Fix: dedupe by rebus
+*value* into a `[String: Character]` map exactly mirroring
+`NYTToXDConverter.rebusLookup`, reusing its curated, reserved-character-avoiding
+`rebusPlaceholders` set (`NYTToXDConverter.swift:24-30`) instead of raw ASCII
+walking, and use `&+=` or a bounds check regardless.
+
+**2b. (Correctness — medium; NOT found in this audit, discovered 2026-07-01
+while fixing #2 — TODO H2b) `PUZToXDConverter.parseRebus` reads the `GRBS` grid
+byte directly as the `RTBL` key, but `.puz` stores `GRBS = RTBL_key + 1`.** The
+loop at `PUZToXDConverter.swift:338-343` did `let key = Int(grid[gridIndex]);
+table[key]`, treating the 1-indexed grid byte as a 0-indexed table key. The
+`.puz` format reserves `GRBS` byte 0 for "no rebus" and stores `1 + n` for the
+square whose solution is `RTBL` key `n` — confirmed independently by the puz
+FileFormat wiki ("A value of 1+n indicates a rebus square … key n in the RTBL
+section"), puzpy (`self.table[i] = k + 1`), and other implementations. Effect on
+a *real* Across Lite rebus file: a single-value rebus (`RTBL` key 0, `GRBS` byte
+1) looked up the absent `table[1]` and silently dropped every rebus cell — the
+cell fell back to its raw single-letter solution byte; a multi-value rebus
+mis-mapped each cell to the *next* table entry and dropped the last. Silent
+corruption, never a crash, which is why it went unnoticed (and why the #2
+allocator crash was in practice only reachable from a hand-crafted file, since
+no real rebus `.puz` ever populated the rebus map). Fix applied: `table[key - 1]`
+with the grid byte guarded `> 0`; regression test `singleRebusValueDecodes`.
+
+**3. (Security/Correctness — medium) `NYTToXDConverter.convert` doesn't
+validate that `dimensions.width`/`height` are positive before using them,
+crashing on a malformed NYT response; a sibling cell-index access is
+similarly unguarded.** `convert(jsonData:)` extracts `width`/`height` as plain
+`Int` from JSON (`NYTToXDConverter.swift:71-75`) and only checks
+`cells.count == width * height` (`:81-83`) before looping
+`for row in 0..<height { for col in 0..<width { ... } }` (`:173-175`). A
+response with `width: -1, height: -1` and a single-element `cells` array
+satisfies `(-1)*(-1) == 1 == cells.count` and passes every guard, then traps
+with `Fatal error: Range requires lowerBound <= upperBound` — confirmed by
+running the converter against a crafted minimal body. Separately,
+`let answerStr = cellIndices.compactMap { answers[$0] }` (`:241`) subscripts
+`answers` with `clue["cells"]` indices with no bounds check — an
+out-of-range/negative index is a fatal array access — while the sibling
+`acceptedAnswerVariants` correctly guards the same kind of access with
+`answers.indices.contains` (`:330`). NYT's API is comparatively trusted, but
+it's still externally-supplied structure parsed as untrusted input per this
+phase's brief; a corrupted proxy/cache, API regression, or on-device MITM
+crashes the fetch/upgrade flow. Fix: `guard width > 0, height > 0` alongside
+the existing cell-count check, and guard every `answers[$0]` access the same
+way `acceptedAnswerVariants` already does.
+
+**4. (Security/privacy — medium) `NYTPuzzleFetcher` unconditionally logs a
+partial NYT session cookie and full request/response bodies via raw `print`,
+not gated by `#if DEBUG`.** `fetchPuzzle` prints
+`"Cookie header: NYT-S=\(cookie.prefix(40))..."`
+(`NYTPuzzleFetcher.swift:99`, confirmed) — 40 characters of the live session
+cookie that authenticates every NYT request — plus the request URL, full
+response bodies up to 1000-3000 chars, and the entire converted `.xd` source
+(`:97-133`, confirmed), on every single fetch, in release builds, since `print`
+is not stripped. `NYTAuthService.completeSignIn` logs only cookie *names* and
+*domains*, never values (confirmed) — the more careful pattern already exists
+in this file set and simply wasn't applied here. This is a real departure from
+the discipline Phases 1/3/4 verified for `CloudDiagnostics` and the push/crypto
+layers (never log tokens/secrets even truncated); a 40-char cookie prefix
+reaching Console.app/sysdiagnose/MDM log collection/screen-share is a genuine,
+if not catastrophic (NYT-S is an ordinary web session cookie, not a
+cryptographic key), credential-exposure surface. `fetchPuzzleList`, which
+carries the same logging, also has no callers anywhere in `Crossmate/` or
+`Tests/` — dead debug scaffolding left in the shipped file. Fix: delete all of
+these prints (or route a redacted line through the same event-logging pattern
+used elsewhere), and remove the dead `fetchPuzzleList` method or wire it up.
+
+**5. (Performance — low) No length/complexity cap on clue text compounds two
+independently-quadratic-or-worse render/parse paths.** `XDMarkup.segments`
+(`XDMarkup.swift:61-86`) rescans from `i+2` to end-of-string on every
+unterminated markup open (`{` + valid marker with no matching close) and falls
+through to a single-character advance rather than skipping the failed scan —
+clue text with many repeated unterminated sequences (e.g. `{/{/{/{/...`)
+triggers a full rescan per occurrence. `Puzzle.init`'s `parseCrossReferences`
+(`Puzzle.swift:305-336`) runs a lazy-quantified regex over every clue's text at
+construction time, which — per finding #1's reachability — executes on the
+sync worker for attacker-influenced clue text; not exponential, but unbounded
+polynomial cost purely because clue length is unbounded. Both are lower
+severity than finding #1 (clue prose is normally short, and `XDMarkup` runs on
+render rather than the sync/accept hot path) but reinforce the same theme: add
+a conservative byte/length cap on `puzzleSource` and per-clue text before
+parsing, at every boundary identified in Phase 1 #3 / Phase 3 #5 / the Phase 4
+`DriveMonitor` carry-over (`ImportService.importGame`'s uncapped
+`String(contentsOf:)`/`Data(contentsOf:)` at `ImportService.swift:27,29` is the
+same gap). This bounds memory/parse cost generally even though, per finding
+#1, it does not alone bound the exponential case.
+
+**6. (Consistency — low) Bundled/debug `.xd` sources are stamped `CmVer: 3`
+against a current parser version of 7.** Every file under `Puzzles/cm-starter/*`
+and `Puzzles/debug/*` carries `CmVer: 3` while `XD.currentCmVersion == 7`
+(`XD.swift:7`). All 13 bundled/debug puzzles were parsed end-to-end with the
+real parser (not just cross-checked against manifests) and load correctly
+regardless, and grid/`blockMask` consistency against each manifest was also
+verified directly — this is purely a stale version stamp. Worth confirming
+whether bundled-puzzle staleness/refresh logic (Phase 2) treats a `CmVer`
+behind the parser version as meaningful, or whether it should — otherwise
+just re-stamp the bundled sources to track the authoring tool's actual output.
+
+**7. (Code quality — low) `PUZToXDConverter` indexes `Data` with absolute
+offsets that assume a zero-based buffer.** `data[0x2C]`, `asciiString(in:range:)`,
+and `Array(data[solutionStart..<fillStart])` (`PUZToXDConverter.swift:37-52`)
+use literal/absolute offsets into `Data`. Safe with the current caller
+(`Data(contentsOf:)` is zero-based) but would crash on any `Data` slice with a
+non-zero `startIndex`. Normalize with `data.startIndex + offset`.
+
+**Non-findings and deferred checks:**
+- No force-unwraps in `XD.swift` on untrusted input: all regex captures are
+  `Int`-guarded, grid/word walks stay bounds-checked through `isOpen`/`isBlock`,
+  and `Numbering`/segment array accesses stay within grid bounds. The only
+  algorithmic hazard is `segment` itself (finding #1), not a crash.
+- `XDMarkup` degrades safely (no crash) on an unterminated span or an invalid
+  `{` marker — only the perf cost in finding #5.
+- `XD.Numbering.build` and `Puzzle.init`'s independent clue-numbering
+  computation agree (checked directly; a mismatch would silently misnumber
+  clues).
+- `NYTToXDConverter`'s *own* rebus placeholder set correctly avoids every
+  reserved grid/header character and is tested
+  (`manyRebusFillsAvoidReservedKeys`) — the exact hardening `PUZToXDConverter`
+  lacks (finding #2); its header comment even documents having hit and fixed
+  the identical bug class previously.
+- `NYTAuthService` credential handling is sound: cookie/email go through
+  `KeychainHelper` (`kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly`, no
+  iCloud sync, proper upsert — consistent with Phase 3's review of the same
+  helper); `cookieLoadResult`/keychain-load logging records only status and a
+  `hasData` boolean, never cookie content (contrast with finding #4); a locked
+  device correctly maps `errSecInteractionNotAllowed` to
+  `.temporarilyUnavailable` rather than a false sign-out.
+- `NYTAuthService.extractAccountGraphQLConfiguration`'s HTML-scraping regex
+  (`firstJSONStringValue`) is single-pass with mutually-exclusive alternation
+  on first character — no catastrophic backtracking, and it reconstructs a
+  well-formed JSON literal to decode escapes safely rather than hand-parsing
+  them.
+- `NYTPuzzleUpgrader.structuralDivergence` correctly gates source replacement
+  on identical dimensions + block layout + per-cell solution; both
+  `.mismatched`/`.failed` paths preserve the player's existing moves, and
+  `.plan` only fires for owner-side NYT puzzles.
+- `PUZToXDConverter.decodeString`'s CP1252→Latin1→UTF8 fallback chain
+  terminates in a non-optional decode; the non-rebus parsing (dimensions,
+  string table, `GEXT`/circled cells, clue-count reconciliation) validates
+  every offset/length against `data.count` before slicing, and `width`/`height`
+  are single bytes (≤255) so `cellCount` can't overflow.
+- `Puzzle.init`'s loops can't see a mismatched width/height, because `Puzzle`
+  is only ever constructed from an already-successfully-parsed `XD`, whose
+  `width`/`height` are derived from the same grid data its rows were built
+  from.
+- `PuzzleCatalog` manifest loading is defensive (`try?` throughout, skips any
+  bundle directory with an unreadable/undecodable manifest) and reads only
+  app-bundle resources — not a trust boundary, no user input reaches it.
+- `ImportService.importGame` is a thin, straightforward extension dispatcher;
+  its missing size cap is the same Phase 4 `DriveMonitor` carry-over folded
+  into finding #5 above (and, per finding #1, a size cap wouldn't have stopped
+  that specific attack anyway).
+- Nothing new carries forward to Phase 6/7 beyond what's already noted above —
+  the XD/PUZ/NYT parsing surface is self-contained within this file set, and
+  finding #1's exploit path terminates at `GameStore`/`XD.parse` reviewed here,
+  even though its *reachability* runs through Phase 2/3/6 call sites.
+
+---
+
+## Phase 6 — Gameplay & Puzzle UI
+
+**Status:** Done
+
+The actual solving experience: the puzzle screen, custom keyboard, replay,
+and session state.
+
+- `Crossmate/Views/Puzzle/*`
+- `Crossmate/Services/PuzzleSession.swift`, `ReplayLoader.swift`
+- `Crossmate/Models/ReplayControls.swift`, `CheckResult.swift`
+- `Crossmate/Services/GridSilhouette.swift`
+- `Tests/Unit/PuzzleSessionTests.swift`, `ReplayCacheTests.swift`,
+  `ReplayControlsTests.swift`, `GridSilhouetteTests.swift`,
+  `GameSummaryThumbnailTests.swift`, `PendingEditFlagTests.swift`
+
+### Findings (consolidated from Sonnet 5 and Opus 4.8, reviewed 2026-07-01)
+
+This is one of the cleaner phases so far. `GridView`'s single-`Canvas` redesign
+is carefully reasoned about render cost, `PuzzleSession`'s grace-timer/
+background-assertion state machine is cleanly modeled and well-tested, and
+`GridSilhouette`'s codec is defensive with no force-unwraps anywhere in the
+file set. The puzzle UI itself turns out to be a pure, passive consumer of
+already-merged grid state — it does not re-implement or second-guess any
+last-writer-wins tie-break, which resolves the Phase 4 carry-over as a
+non-finding for new gameplay-side logic (see below), though tracing the
+consumers confirms concretely how a Phase 4 finding #2 spoof would surface in
+this layer. The one substantive bug both passes independently zeroed in on
+different corners of is a replay-completeness gap in the app-shell
+composition root that can permanently cache an incomplete scrubber for a
+session; the rest is low-severity robustness/quality, including a recurring
+"redundant full-grid pass" pattern this audit has flagged in earlier phases.
+
+**1. (Correctness — medium) The app-shell's "local-only" replay fast path
+infers a game has no other contributors from an unverified local Core Data
+snapshot, and can permanently cache an empty/incomplete replay for the rest
+of the session.** `CrossmateApp.swift:787-797`:
+```swift
+let entries = store.localJournalEntries(for: gameID)
+let otherDevices = store.contributingDevices(for: gameID)
+    .filter { $0.deviceID != localDeviceID }
+if otherDevices.isEmpty {
+    return .ready(ReplayTimeline(merging: [entries]))
+}
+```
+`contributingDevices(for:)` (`GameStore.swift:2313-2324`) only reads this
+device's already-synced local `MovesEntity` rows — unlike
+`ReplayLoader.loadReplay` (`ReplayLoader.swift:45-120`), which derives
+`expectedDevices` from a live CKQuery and gates on every contributor's journal
+having landed. If this device's Moves catch-up sync for the game hasn't
+finished yet — the common case the first time a device opens an
+already-completed shared game (freshly accepted an invite to a finished
+puzzle, a reinstall, an iCloud restore, or a collaborator returning to a game
+finished while they were away) — `contributingDevices` sees zero remote rows
+even though real collaborators exist, so the shortcut wrongly takes the solo
+branch and builds `ReplayTimeline(merging: [entries])` from this device's own
+(often empty) local journal, bypassing `ReplayAssembler`'s strict-completeness
+gate entirely. That wrong result is cached as `.ready` by
+`ReplayAssembler.memoised` (`JournalReplay.swift:173-189`) into
+`ReplayLoader.replayTimelineCache` (`ReplayLoader.swift:23`); because
+`ReplayControls.load` is a no-op once `.ready` (`ReplayControls.swift:169-174`)
+and the memo short-circuits on every later call for that `gameID`
+(`JournalReplay.swift:180-183`), every subsequent reopen of the puzzle within
+the same app session keeps serving the wrong scrubber even after the real
+Moves records finish syncing in the background — only a full app relaunch
+clears the in-memory cache. Fix: gate the fast path on a hard, durable signal
+that the game was never shared (e.g. `!session.mutator.isShared` /
+`GameEntity.ckShareRecordName == nil`) rather than on the currently-synced
+local Moves cache; for anything shared, always defer to
+`services.replays.loadReplay`, which already handles the true solo-but-shared
+case correctly via its own `expectedDevices` check.
+
+**2. (Correctness — low) A whitespace-only rebus commit writes a phantom
+"filled" cell that silently blocks completion.**
+`PlayerSession.committedRebusValue` (`PlayerSession.swift:488-493`) returns
+the buffer unchanged whenever it contains no non-whitespace character —
+`guard buffer.rangeOfCharacter(from: .whitespacesAndNewlines.inverted) != nil
+else { return buffer }` — so a whitespace-only buffer commits as a
+non-empty, visually-blank cell instead of clearing it. The reachable path is
+the rebus modal's system `TextField` (`PuzzleView.swift:623-637`), which,
+unlike the custom keyboard, permits a space; the blank-looking cell then reads
+as filled to `PuzzleScoreboard.filledCellCount` and `Game.completionState`, so
+a fully-filled grid resolves to `filledWithErrors` with no visibly wrong cell
+for the player to find. This lives in `PlayerSession.swift`, a Phase 2 file,
+but is recorded here since the rebus-modal gameplay path is what surfaces it.
+Fix: treat a whitespace-only committed value as empty, mirroring the
+empty-buffer case.
+
+**3. (Code quality/robustness — low) Three render-path dictionaries built with
+the trapping `Dictionary(uniqueKeysWithValues:)` sit alongside a fourth that
+deliberately defends against duplicate keys.** `GridView.body`'s
+`authorTintByID` (`GridView.swift:42-46`), `PuzzleScoreboard.scores`
+(`PuzzleScoreboard.swift:124`), and `SuccessPanel.contributions`
+(`SuccessPanel.swift:178`) all build a `[String: T]` from
+`roster.entries`/`entries` with `Dictionary(uniqueKeysWithValues:)`, which
+traps on a duplicate `authorID`. `RecentChangeBorders.resolve`, in the same
+`GridView.swift` file, already hedges the equivalent construction with
+`Dictionary(..., uniquingKeysWith: { first, _ in first })`
+(`GridView.swift:687-690`), showing the author has considered duplicates
+possible. Today this is safe by construction —
+`ParticipantSummaries.remoteParticipants` de-duplicates via a `Set` before the
+local entry is prepended (`ParticipantSummaries.swift:59-66`,
+`PlayerRoster.swift:393`) — but the three trapping sites should adopt the same
+`uniquingKeysWith` form for defense-in-depth, since a future change to roster
+assembly would otherwise turn a benign data glitch into a hard crash while a
+puzzle is on screen.
+
+**4. (Code quality — low) `GridSilhouette.decode` accepts over-length /
+trailing-garbage payloads instead of requiring an exact bit count.** `decode`
+checks only `bits.count == storedCount` after `unpackBits` truncates to
+`storedCount` (`GridSilhouette.swift:106-108`); it never rejects a payload
+carrying *more* base64url bytes than the dimensions require, so two distinct
+segments can decode to the same grid and a segment with appended junk still
+decodes successfully. Bounded (≤35×35) and low-impact (only paints a
+link-tap placeholder), but a strict `bytes.count == (storedCount + 7) / 8`
+check would make the codec total and match the encoder's output exactly.
+
+**5. (Performance/code quality — low) The live scoreboard does repeated
+full-grid passes on every keystroke, and duplicates the same
+author-normalization/scoring logic across two files.**
+`PuzzleScoreboard.swift`'s `fillableCellCount` (`:39-43`), `filledCellCount`
+(`:45-56`), `revealedSquareCount` (`:58-69`), and `scores` (`:111-175`) each
+independently loop over every cell — four full-grid passes recomputed on each
+render, one of which (`fillableCellCount`) doesn't depend on mutable state at
+all — and `SuccessPanel.swift`'s `revealedSquareCount` (`:27-40`) and
+`contributions` (`:161-232`) repeat the pattern; `SuccessPanel`'s
+`normalizedAuthorID` (`:252-257`) is a near-verbatim duplicate of
+`PuzzleScoreboard.normalizedAuthorID` (`:177-182`), with the roster-fallback/
+extra-contributor bucketing logic duplicated almost line-for-line between the
+two files. Puzzles are bounded to 35×35, so this is genuinely negligible in
+practice (no Core Data involved, unlike the recurring blocking-fetch theme
+from Phases 1/2/4) — recorded for the redundant-work/duplication pattern, not
+as a performance emergency. Worth consolidating into one shared per-grid-stats
+helper reused by both views.
+
+**Non-findings and deferred checks:**
+- **Phase 4 finding #2 / Phase 1 finding #9 carry-over resolved:** grepped
+  every `updatedAt`/`cellAuthorID`/`letterAuthorID` use across
+  `Views/Puzzle/*` and confirmed the gameplay UI adds no independent
+  clamping, trust check, or tie-break logic of its own — it is a passive
+  renderer of `session.game.squares[r][c]` and replayed `ReplayFrame.cells`.
+  The only `updatedAt` comparison in this file set
+  (`GridView.swift:540`, `RemoteCursorTints.remoteTrackTints`) is cosmetic
+  cursor-track tint precedence, not cell content. `Square.enqueuedAt`,
+  applied in `GameStore.restore` (`GameStore.swift:2467-2520`, Phase 2,
+  covered by this phase's `PendingEditFlagTests.swift`), protects a
+  locally-buffered keystroke from being clobbered by an inbound merge, but
+  that is a separate, correctly-functioning mechanism from the Phase 4 #2
+  spoof path. Tracing the consumers does confirm the concrete UI-visible
+  blast radius of that already-recorded issue, though: a poisoned
+  `cellAuthorID` from a spoofed realtime frame would misattribute the
+  author tint in `GridView` (`GridView.swift:78`, `:91`) and miscount
+  per-player credit in `PuzzleScoreboard.scores` /
+  `SuccessPanel.contributions` (`PuzzleScoreboard.swift:118`,
+  `SuccessPanel.swift:143`, `:173`) — useful color for Phase 4's
+  already-owned fix, not a new gameplay-side bug.
+- `PuzzleSession.swift`'s grace-timer/background-assertion state machine
+  (`scheduleEndPush`/`cancelPendingEndPush`/`fireEndPush`) is correctly
+  serialized with no interleaving `await` between guard and mutation, and its
+  `UIBackgroundTaskIdentifier`/idle bookkeeping matches `PuzzleSessionTests`'s
+  interleaving cases (including expedited-expiration and supersede paths)
+  exactly.
+- Replay data is a trust boundary but is display-only and crash-safe: a
+  malicious co-player's uploaded journal can misrender/misattribute a
+  *finished* game's replay, but never writes back to the live `Game` or to
+  CloudKit, and every consumer of a replayed cursor position is bounds-checked
+  (`Puzzle.swift:366-431`, `GridView.swift:74`, `SuccessPanel.swift:240`,
+  `ReplayControls`'s scrub/playback state machine).
+- `ReplayLoader`'s own CloudKit paths are non-blocking
+  (`store.cachedRemoteJournals`/`cacheRemoteJournals` correctly use
+  `await ctx.perform`, `GameStore.swift:1459-1525`). Its one synchronous
+  main-actor read, `store.localReplaySource` → `movesJournal.recordedEntries`
+  (`ReplayLoader.swift:57`), is **Phase 2 finding #5** reaffirmed from a new
+  call site (fires once on the finish-banner path), not a new finding.
+- `PlayerRoster.refresh`'s `ctx.performAndWait` (`PlayerRoster.swift:256`) is
+  also reached from this phase's UI (colour change in
+  `PuzzleToolbarModifier.swift:218`, `PuzzleLifecycleModifier`) — **Phase 2
+  finding #3** reaffirmed from a new call site, not a new finding.
+- No force-unwraps (`try!`, `as!`, `.first!`/`.last!`, bare postfix `!`)
+  anywhere in `Views/Puzzle/*`, `PuzzleSession.swift`, `ReplayLoader.swift`,
+  `ReplayControls.swift`, `CheckResult.swift`, or `GridSilhouette.swift`.
+  `HardwareKeyboardInputView`'s HID-usage mapping safely rejects any
+  unrecognized `UIKeyCommand.input` via `guard`/`default: return nil`
+  (`HardwareKeyboardInputView.swift:120-171`), and custom `Layout`
+  implementations (`KeyboardRow`, `PuzzleGridLayerLayout`/
+  `PuzzleGridGeometry`) guard degenerate (zero-size/zero-count) input.
+- `PuzzleCommands`/`PuzzleActionTarget` equality is deliberately keyed on
+  `session === && isEnabled` to stop a UIKit menu-builder reentrancy deadlock
+  (`PuzzleCommands.swift:26-28`) — correct as documented, not a staleness bug.
+- `CheckResult` is a trivial two-case enum with no correctness surface;
+  cross-reference hatching (`CellPatterns`/`CrossRefLines`) is pure geometry
+  with `max(2, …)` step floors, no divide-by-zero.
+- `GameSummaryThumbnailTests.swift`, `ReplayCacheTests.swift`, and
+  `PendingEditFlagTests.swift` exercise Phase 2 source (`GameStore.swift`'s
+  `GameSummary`, `cacheRemoteJournals`/`cachedRemoteJournals`, and
+  `restore`'s pending-edit flag) rather than any Phase 6 file directly; each
+  was checked against the corresponding Phase 2 source and found accurate.
+- Carry over to **Phase 7**: `PuzzleView`'s `.task` beat sets
+  `session.recentChanges = loadRecentChanges()` after a 750ms hold
+  (`PuzzleView.swift:229-233`); `loadRecentChanges` is injected from
+  `AppServices` (Phase 7) — worth confirming it isn't a synchronous
+  main-thread Core Data fetch when Phase 7 reviews the composition root,
+  alongside the existing Phase 4 carry-over note on
+  `NotificationNavigationBroker`/`PlayerSessionNavigation`.
+
+---
+
+## Phase 7 — App Shell & General UI
+
+**Status:** Done
+
+Everything else: app-wide composition/DI, general browsing/list/settings
+UI, and cross-cutting diagnostics.
+
+- `Crossmate/Services/AppServices.swift`, `DebuggingMonitors.swift`
+- `Crossmate/Views/Browse/*`, `Views/GameList/*`, `Views/Settings/*`,
+  `Views/Components/*`
+- `Crossmate/CrossmateApp.swift` (app-shell composition root; not listed
+  under any earlier phase, reviewed here for the first time)
+- `Tests/Unit/LogScrubberTests.swift`
+
+### Findings (consolidated from Sonnet 5 and Opus 4.8, reviewed 2026-07-01)
+
+Both passes independently confirmed the two outstanding carry-overs
+(`NotificationNavigationBroker`/"`PlayerSessionNavigation`" from Phase 4,
+`loadRecentChanges` from Phase 6) as non-findings, and both independently
+found the recurring "blocking Core Data on an async `@MainActor` path"
+pattern at new composition-root call sites in `AppServices.swift`. The two
+passes disagreed sharply on one item — Sonnet 5 flagged `RecordEditorView`
+as a high-severity finding, Opus 4.8 assessed the same view as a non-finding
+("a power-user CloudKit-Console substitute, not a cross-trust-boundary
+surface"). This was resolved by direct verification of the source below:
+Sonnet 5's read is correct and Opus 4.8's is not — the view's `scope: .shared`
+option targets `container.sharedCloudDatabase`, i.e. zones *other users*
+have shared with the signed-in user, so it is a cross-trust-boundary surface
+by design, not merely an owner-facing tool.
+
+**1. (Security — high, verified) A raw CloudKit record browser/editor/deleter
+is reachable by any user via a client-side toggle with no other gate, turning
+every trust-boundary gap this audit has flagged in Phases 1/3/4 into a
+one-tap, no-code exploit.** `RecordEditorView.swift` lets the user pick
+`scope: .private` or `.shared`, list every zone in that scope
+(`listZonesForEdit`, `CloudDiagnostics.swift:151-162`), list every record of a
+chosen type in a zone (`queryRecordsForEdit`, `:169-190`), fetch/edit/save any
+record by name (`fetchRecordForEdit`/`saveRecordForEdit`, `:122-145`), and
+delete any record via swipe (`deleteRecordForEdit`, `:195-204`) — all backed
+by nothing but the CloudKit API itself, confirmed directly against the
+source. The only gate is `SettingsView.swift:9`'s
+`@AppStorage("debugMode") private var debugMode = false`, flipped by tapping
+the About footer text four times (`SettingsView.swift:127-133`,
+`easterEggTaps >= 4 { debugMode.toggle() }`) — a runtime UserDefaults flag any
+user can flip, not a `#if DEBUG` compile gate, developer entitlement, or
+server-side check. Once toggled, "Record Editor" appears as an ordinary
+`NavigationLink` in Settings → Debugging (`SettingsView.swift:94-96`).
+
+Critically, `scope: .shared` routes to `container.sharedCloudDatabase`
+(`CloudDiagnostics.swift:126-128`, `:139-141`, `:152-154`, `:176-177`,
+`:199-201`) — CloudKit's shared database, which surfaces every zone *another
+user* has shared with the signed-in account. This is exactly the class of
+writable-but-not-yours zone this audit has repeatedly identified as the
+concrete attack surface: any accepted friend's `friend-<pairKey>` inbox that
+the app itself doesn't expose as browsable (Phase 1 finding #1, Phase 3
+findings #1/#3/#4), and any shared game's zone. With this view, a user needs
+no reverse-engineered client and no custom CloudKit code to carry out those
+findings by hand: pick "Shared", "List zones", pick a `friend-<pairKey>`
+zone, type "Decision" as the record type, "List records in zone", tap a
+`decision-account-pushSecret` or `decision-block-<X>` record (or none —
+create one blind by record name), add/edit a String field, "Save changes".
+The swipe-to-delete action equally lets any participant with zone
+`.readWrite` (which CKShare zone-sharing grants at the zone level, not
+per-record) delete a co-player's `Game`/`Player`/`MovesEntity` records
+outright, bypassing every app-level ownership check. Fix: compile this view
+out of release builds (`#if DEBUG`) rather than gating it behind a
+user-flippable preference; if a Production-facing inspection tool is still
+wanted for support purposes, make it read-only and restrict it to the
+signed-in user's own private-database zones.
+
+**2. (Performance — medium) `AppServices` blocks the main actor with
+`ctx.performAndWait` at three call sites — the same recurring pattern flagged
+in Phases 1, 2, 4, and 6, now at the composition-root layer that wires those
+calls together.** `AppServices.start(appDelegate:)` creates a background
+context and calls `nicknameCtx.performAndWait { FriendEntity
+.rebuildNicknameDirectory(...); GameEntity.rebuildContentKeyDirectory(...) }`
+(`AppServices.swift:678-685`) synchronously on the `@MainActor`-isolated
+`start()`, during cold launch; `rebuildContentKeyDirectory`
+(`GameEntity+ContentKey.swift:13-25`) fetches every `GameEntity` with a
+non-nil `notification` field — effectively every game that has ever minted
+push credentials, not just the small friends table the neighboring comment
+describes. Separately, `reconcilePendingJournalUploads` — called from every
+`runFreshenGameList` (foreground, appear, manual pull-to-refresh, and
+remote-push-driven freshens) — does two more `ctx.performAndWait` blocks on a
+fresh background context (`AppServices.swift:1268-1271`, a `completedAt !=
+nil AND journalUploaded == NO` fetch against `GameEntity`, which per Phase 2
+finding #4 has no `fetchIndex`, so it's a full-table scan; and `:1288-1295`,
+the conditional mark-done save). All three block the calling thread — the
+main actor, since `AppServices` is `@MainActor` with no intervening `await` —
+despite the enclosing methods being `async`. The inconsistency is visible in
+the same file: `logPlayerLeaseSnapshot`, `hasPresentPeer`, `soonestPeerLease`,
+and `presentPeers` (`AppServices.swift:2225-2373`) all correctly use
+`context.perform` wrapped in `withCheckedContinuation` instead of
+`performAndWait` — the non-blocking pattern is already established elsewhere
+in this exact file. Fix: convert all three sites to the same
+`context.perform`/`withCheckedContinuation` (or `await ctx.perform`) pattern.
+
+**3. (Correctness — low) `GameListView`'s "Leave Puzzle" failure is captured
+into a dead `leaveError` state that is never displayed, unlike every sibling
+destructive action.** `leaveShare(game:)` catches a thrown error from
+`shareController.leaveShare(gameID:)` into `@State private var leaveError:
+Error?` (`GameListView.swift:61`, `:777-785`), and grepping the file confirms
+those are the *only* two references to `leaveError` — no `.alert` or
+`Announcement` ever reads it. Every other destructive action in the same file
+(resign, delete, decline) posts an `Announcement` on failure via
+`Self.destructiveActionErrorID`, and the equivalent in-puzzle leave flow
+(`PuzzleView.swift:62,609` → `PuzzleModifiers.swift:364-374`) wires its own
+`leaveError` to a `"Couldn't Leave"` alert — so the pattern is known and used
+correctly one screen over. A network blip or CloudKit error during a Game
+List "Leave" swipe fails completely silently: the row stays in the list with
+no feedback. Fix: post an `Announcement` (or add the missing `.alert`)
+mirroring the in-puzzle leave flow.
+
+**4. (Security — low) `NYTLoginView`'s sign-in-redirect detector uses a
+substring hostname check instead of an exact/suffix match.**
+`NYTWebView.Coordinator.webView(_:decidePolicyFor:)` gates cookie extraction
+on `url.host?.contains("nytimes.com") == true`
+(`NYTLoginView.swift:71`, confirmed) rather than an exact match or
+`hasSuffix(".nytimes.com")` — the standard hostname-validation footgun (it
+would also match `nytimes.com.attacker.tld`). Exploitability is narrow: the
+`WKWebView`'s only navigation entry point is the hardcoded
+`NYTAuthService.loginURL` (an `nytimes.com` URL) with no address bar, so
+reaching a look-alike host would require an actual open redirect on NYT's own
+login flow, and the extracted cookie store only ever contains cookies this
+WebView session itself set. Still a one-line fix worth making since the
+substring check offers no real benefit over an exact/suffix match.
+
+**5. (Performance/code quality — low) The "recent changes" read does a
+redundant, unbounded diagnostic Core Data fetch on every puzzle-open and
+foreground-resume beat.** The injected `loadRecentChanges` closure
+(`CrossmateApp.swift:805-817`, driving `PuzzleView`'s `.task` beat,
+`PuzzleView.swift:232`) and `recaptureRecentChanges`
+(`CrossmateApp.swift:1052-1062`, driving background→foreground resume) each
+call both `store.recentlyChangedCells(...)` (bounded, `changedAt > since`,
+`GameStore.swift:2209-2224`) and `store.recentChangesDiagnosticSummary(...)`
+(`GameStore.swift:2227-2233`) — the latter runs a **second, unbounded** fetch
+of every `PeerChangeEntity` row for the game (predicate is `gameID == %@`
+only, no time filter) purely to build one diagnostic log line. Both run on
+`GameStore.context`, which is `persistence.viewContext`
+(`GameStore.swift:508`) — a main-thread context, so this is the ordinary
+"synchronous Core Data on the render path" class already accepted elsewhere
+(Phase 2 finding #3, Phase 6's `solveTime` non-finding), not a cross-queue
+blocking stall; **this resolves the Phase 6 carry-over** on `loadRecentChanges`
+as a non-finding on the threading question specifically. The genuinely new
+issue is the redundant unbounded diagnostic fetch doubling the ledger read on
+every open/resume. Fix: gate the diagnostic-summary fetch behind an actual
+diagnostics flag, or derive both the summary and the cell map from a single
+fetch.
+
+**6. (Performance — low) `DiagnosticsView` rebuilds and rewrites the entire
+event-log dump on every log event while the screen is open.**
+`diagnosticDump` (`DiagnosticsView.swift:251-273`) string-joins the whole
+`eventLog.entries` buffer (up to 30,000 entries, `DebuggingMonitors.swift:279`)
+as a computed property; `.onChange(of: diagnosticDump)`
+(`DiagnosticsView.swift:142`) forces SwiftUI to recompute that full O(n) join
+on every body evaluation just to diff it, and each change triggers
+`refreshDiagnosticShareFile` → `writeDiagnosticShareFile` (`:155-169`), which
+rewrites the entire multi-MB dump to `temporaryDirectory` atomically. During
+an active co-solve the log churns at ~5 events/sec (documented at
+`DebuggingMonitors.swift:271-273`), so simply having the Diagnostics screen
+open triggers several full rebuilds + full-file disk rewrites per second.
+Bounded to a rarely-open debug screen. Fix: rebuild the share file lazily,
+only when the `ShareLink` is actually invoked.
+
+**7. (Code quality — low) `GameListView` wraps its entire body in a
+`GeometryReader` solely to read container height.** `GameListView.body` opens
+with `GeometryReader { geometry in ... }` (`GameListView.swift:74`) only to
+pass `geometry.size` into `usesRoomierType(for:)`
+(`size.height >= 760 && dynamicTypeSize <= .medium`, `:787-789`) — this
+contradicts the project's documented preference for `containerRelativeFrame`/
+size-class environment values over `GeometryReader`. Functionally harmless
+here (single read, no layout feedback loop), but worth swapping for a
+vertical-size-class or `containerRelativeFrame` check.
+
+**Non-findings and deferred checks:**
+- **Phase 4 carry-over resolved:** `NotificationNavigationBroker`
+  (`CrossmateApp.swift:309-376`) and its siblings `CloudShareAcceptanceBroker`
+  (`:379-406`) and `ShareLinkBroker` (`:416-441`) are `@MainActor` singletons
+  with an identical buffer-until-handler-set pattern to survive the
+  cold-launch race before `RootView.task` wires up handlers.
+  `NotificationNavigationBroker` only routes an already-parsed `gameID` (from
+  `AppDelegate.gameID(from:)`, which validates a UUID string or a
+  `game-<uuid>` zone-name pattern and returns `nil` on anything malformed)
+  into in-app navigation — no trust-boundary issue. "`PlayerSessionNavigation`"
+  turned out not to be a composition-root type at all: it is only the name of
+  `Tests/Unit/PlayerSessionNavigationTests.swift`, which exercises
+  `PlayerSession`'s clue-navigation methods (Phase 2/6 territory) — the
+  original Phase 4 note conflated a test filename with a type. That test
+  file's `rebusCommitPreservesAllWhitespace` case incidentally locks in, as
+  expected behavior, the exact whitespace-only-rebus bug Phase 6 finding #2
+  already flagged (confirmatory, not new).
+- **Phase 6 carry-over resolved:** see finding #5 — `loadRecentChanges` is a
+  synchronous main-thread fetch on `viewContext`, the same accepted class as
+  Phase 2 finding #3, not the background-context-`performAndWait` anti-pattern
+  this audit has been flagging; the redundant unbounded diagnostic fetch it
+  also does is recorded as its own item above.
+- `DebuggingMonitors.swift` (`PerformanceMonitor`, `EventLog`, `SyncMonitor`,
+  `LogScrubber`) is careful and well-tested: `LogScrubber.scrub` (truncates
+  UUIDs, `_`+32-hex/bare-32-hex tokens, and iCloud share-link tokens) runs on
+  the full formatted message at `EventLog.append` (`:320-321`), so
+  `SyncMonitor.recordError`'s `NSError.userInfo` dump (`:415-418`) is scrubbed
+  too, not just the separately-stored description — verified directly against
+  `LogScrubberTests`' cases. One honest caveat: base64/base64url secrets (the
+  account push secret, content/encryption keys) are not hex and would pass
+  through unscrubbed, but nothing in this phase's file set (or elsewhere,
+  outside the already-recorded Phase 5 `NYTPuzzleFetcher` cookie finding) logs
+  those — headroom, not a live leak.
+- No force-unwraps (`try!`, `as!`, `.first!`/`.last!`, bare postfix `!`) on
+  untrusted/CloudKit-sourced or user-controlled data anywhere in scope
+  (confirmed by grep across the full file list); the only `!` sites are on
+  hardcoded constants (`TimeZone(identifier: "America/New_York")!`, two
+  hardcoded `URL(string:)!` links in `AboutView.swift`) or modulo-bounded
+  weekday-symbol lookups in `NYTBrowseView.swift`.
+- The inviter-controlled `invite.gridSilhouette`, decoded via
+  `GridSilhouette.decode` on the Game List and accept path
+  (`GameListView.swift:602`, `:696`), is a real CloudKit trust boundary but
+  was already verified crash-safe and bounded (≤35×35) in Phase 6 non-finding
+  #4; a `nil` decode here just renders no thumbnail.
+- The invite-accept path (`GameListView.accept`, `:693-717`) only forwards the
+  inviter-controlled `shareURL`/`pingRecordName` to
+  `InviteCoordinator.acceptInvite` (Phase 3), which owns validation. The
+  account-control push path (`AppServices.handleAccountControlPush`,
+  `:1714-1774`) adopts `readAt`/`gameID` only from `accountJoined`/
+  `accountSeen` pushes targeting the random, account-private `acct-<UUID>`
+  address (Phase 4 non-finding: not spoofable without the account zone) and
+  self-guards on `senderDeviceID == localDeviceID` — sibling-device-scoped,
+  not a co-player boundary.
+- `GameShareSheet.describe` (`GameShareItem.swift:355-361`) copies a full
+  unscrubbed `nsError.userInfo` dump to the pasteboard on "Copy Error",
+  bypassing `LogScrubber` unlike the shareable diagnostics dump — owner-facing
+  and transient, not raised as a numbered finding, but worth aligning with the
+  scrubber if this ad-hoc error surface is ever meant to be shared further.
+- `PerformanceMonitor`, `EventLog`/`EventLogStore` (bounded 30k entries + 24h
+  retention, debounced off-main persist), `AnnouncementCenter`-driven banners,
+  `NewGameSheet`/`ImportedBrowseView` (thin dispatchers into Phase 5's
+  import/XD path), and the remaining `Views/Components/*` are straightforward
+  with no bugs found. `FriendAvatarView`'s ring-reveal `Task` is properly
+  cancelled on view changes. `ImportedBrowseView`'s uncapped
+  `DriveMonitor.readSource`/`importFile` is the already-recorded Phase 4→5
+  carry-over, not a new finding.
+- Nothing new carries forward — Phase 7 is the last phase in the plan, and
+  every app-shell issue found here terminates in a layer already reviewed
+  (Phase 2 indexing, Phase 4 push scope, Phase 5 import path, Phase 6
+  replay/recent-changes).
+
+---
+
+## Process
+
+Work through phases in order above (roughly risk/complexity order). For each
+phase:
+
+1. Mark it "In progress".
+2. Review the listed files (and anything they pull in that isn't listed
+   elsewhere).
+3. Record findings inline in this file under the phase, or in a linked doc if
+   long.
+4. Mark it "Done" and move to the next phase.