Make settled_at host-authoritative (settle teardown, step 0) - #1069
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change prevents phone peers from applying host-authoritative session settle fields. iOS stores pending settle intents locally, separates settle and snooze command handling, and applies pending overlays across session reads and active-session projections. ChangesSession settlement synchronization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/services/sync/syncHostService.ts`:
- Around line 7628-7637: Replace the isMobileChangesetPeer(peer) check with
isMobilePeer(peer) in the settle-authority filtering logic so paired phones
cannot bypass the host-authoritative column guard; then run the ADE CLI
typecheck, tests, and build.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 602bb4ea-4c66-4c9b-a49b-6dfd548ac881
⛔ Files ignored due to path filters (7)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojdocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/crdt-model.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/settle-teardown-design.mdis excluded by!docs/**
📒 Files selected for processing (7)
apps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ios/ADE/Services/Database.swiftapps/ios/ADE/Services/PendingSessionSettleStates.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkRootScreen+Actions.swiftapps/ios/ADETests/PendingSessionSettleStatesTests.swift
|
@codex review Switched the settle-column guard from The earlier |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e02f8e0290
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let intent = intents[session.id], intent.isSatisfied(by: session) else { continue } | ||
| intents.removeValue(forKey: session.id) |
There was a problem hiding this comment.
When two lifecycle commands overlap, a newer inverse intent can already match the stale row left from before the first command. For example, starting settle on an unsettled row and then starting unsettle causes the next localSessions() read to prune the newer intent immediately because the database still has settled_at = nil; the first command's changeset can then render the row settled while the user's later unsettle is still pending. Confirmation needs to distinguish a post-command database update from the baseline state rather than relying on value equality alone.
Useful? React with 👍 / 👎.
|
@codex review Fixed, and reproduced first. You were right that value equality alone is the wrong confirmation rule: with the settle overlay showing the row as settled, the menu offers Unsettle, so the newer intent's target ( An intent now records the row as it stood when its command was sent and is confirmed only once the row has actually moved from that baseline and matches. Nil baseline falls back to value equality.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44cd002b25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if PendingSessionSettleIntent.normalized(session.settleOverride) == "settled" { | ||
| next.settleOverride = nil | ||
| } |
There was a problem hiding this comment.
When a row has an "active" override and the user taps Settle followed by Unsettle before the first changeset arrives, this branch evaluates the stale database row and preserves "active". The preceding settle command clears that override host-side, so processing the commands in tap order actually ends with no override; meanwhile the replacement overlay resurrects the keep-active pin and exposes the wrong state/actions until replication catches up. The unsettle intent needs to derive this branch from the state presented when the command was issued (including the replaced pending intent), not directly from the stale raw row.
Useful? React with 👍 / 👎.
|
@codex review Fixed. Confirmed your reading of the host ordering: the settle clears the pin unconditionally, the unsettle then preserves whatever is left, so the pair really does end with no override — and reading the live row was resurrecting a pin that was already on its way out. An intent now records the override as it was presented when its command was issued (the raw row with any intent it replaced already applied over it), and the unsettle branch follows that. A standalone unsettle over a genuine pin still shows the pin, since there the presented value and the row agree — covered by a second test so that case can't regress. This is the third finding in the same overlapping-command window; if you see any remaining path where an intent reasons from the raw row instead of the presented state, I'd rather sweep the whole class now than trade another round. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6507acc54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let movedSinceCommand = intent.baseline == nil || intent.sawRowChange | ||
| guard movedSinceCommand, intent.isSatisfied(by: session) else { continue } | ||
| intents.removeValue(forKey: session.id) |
There was a problem hiding this comment.
The movement bit only proves that some settle-family mutation occurred after the latest intent began, not that the latest command was applied. With settle → unsettle → settle issued before replication catches up, the first settle changes the row away from the third command's baseline and already satisfies the third settle, so this removes the overlay; when the intervening unsettle then replicates, the UI incorrectly shows the session unsettled until the final settle arrives. Confirmation needs a host revision or other marker associated with the latest command rather than any post-baseline movement.
Useful? React with 👍 / 👎.
| if !canSendLiveRequests() { | ||
| pendingSessionSettleStates.holdBackstop(now: now) | ||
| } |
There was a problem hiding this comment.
When a settle has been queued offline for more than 20 seconds, reconnecting makes canSendLiveRequests() true immediately, so the first read stops holding its deadline and expires the overlay. This can happen before replay because startInitialHydrationTask runs performInitialHydration—including refreshWorkSessions and its database notification—before flushing pending operations; the success-path holdBackstop therefore runs too late. Rebase pending deadlines when the connection becomes reachable or before initial hydration/queue draining so a still-queued settle does not briefly snap back.
Useful? React with 👍 / 👎.
|
@codex review Both fixed, and I took the first one as a signal to stop adding epicycles. Unattributable overlap. You're right that movement proves only that some mutation replicated. The marker you're asking for doesn't exist on the phone — the host's lifecycle revision is host-local, and adding a replicated one is exactly step 1 of this initiative, not step 0. So rather than a fourth heuristic, the overlay now declines to guess: an intent that replaced one still in flight is unconfirmable, keeps showing what the user last asked for, and yields to replicated truth at the backstop once the run has converged. Trailing the truth briefly beats confidently showing the wrong command's result. Reconnect ordering. Confirmed — 27 iOS tests, including |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 162ce730f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard !intent.replacedInFlight, movedSinceCommand, intent.isSatisfied(by: session) else { continue } | ||
| intents.removeValue(forKey: session.id) | ||
| } | ||
| intents = intents.filter { now.timeIntervalSince($0.value.startedAt) < PendingSessionSettleStates.staleAfter } |
There was a problem hiding this comment.
When a host command succeeds but its confirming changeset never arrives, and no unrelated database or UI event occurs afterward, this expiry is never evaluated: prunePendingSessionSettleStates runs only from localSessions/localSession, while the projection bump is scheduled only after a read has already pruned the intent. The immediate post-command reload happens before 20 seconds, so a quiet screen can retain the optimistic settle state indefinitely despite the intended bounded backstop. Schedule a delayed prune/repaint when an intent begins or when its deadline is rebased.
Useful? React with 👍 / 👎.
|
@codex review Fixed. You were right that this one wasn't merely theoretical: There's now a sweep timer armed when an intent begins and re-armed whenever deadlines are rebased. It reads through the chokepoint (so it prunes and repaints), re-arms while any intent is in flight, stops when the map empties, and is cancelled on teardown. Note on process: this is past this lane's soft 5-iteration cap, and the loop would normally force-finalize past review at this point. I'm not doing that — every round here has produced a real, reproducible defect, and forcing past a genuine finding isn't something I'll do. Happy to keep going as long as you keep finding real ones. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74d8fd83a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard !intent.replacedInFlight, movedSinceCommand, intent.isSatisfied(by: session) else { continue } | ||
| intents.removeValue(forKey: session.id) | ||
| } | ||
| intents = intents.filter { now.timeIntervalSince($0.value.startedAt) < PendingSessionSettleStates.staleAfter } |
There was a problem hiding this comment.
If the device clock changes while an intent is pending—for example after an NTP correction or a manual time adjustment—this wall-clock comparison can expire a fresh overlay immediately when time jumps forward, or preserve it far beyond the promised 20-second backstop when time jumps backward. Store and compare a monotonic uptime/deadline for staleness while retaining Date only for the displayed settle timestamp.
Useful? React with 👍 / 👎.
|
@codex review Fixed. Deadlines now use Worth noting this file already had the precedent — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e93646f9cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return | ||
| } | ||
| pendingSessionSettleBackstopTask = Task { @MainActor [weak self] in | ||
| try? await Task.sleep(nanoseconds: UInt64(PendingSessionSettleStates.staleAfter * 1_000_000_000) + 250_000_000) |
There was a problem hiding this comment.
When a connected host takes more than 20.25 seconds to answer a lifecycle command, this sweep expires the intent even though performCommandRequest still allows the request to run for the default 30-second timeout. Because startedAtUptime is captured before sendCommand, the session snaps back to the replicated state while the command remains valid and then flips again if the host succeeds before 30 seconds; a long host operation also leaves progressively less time for its confirming changeset. Hold or rebase the backstop while the request is outstanding, or start its countdown after the command response.
Useful? React with 👍 / 👎.
|
@codex review Fixed. A 20s backstop against a 30s request timeout was just inconsistent — the window is meant to bound waiting for the changeset, not the round trip, so it now restarts when the command is answered rather than when it's sent. Token-scoped, so a slow command can't extend an intent the user has since replaced; there's a test for that case too, since it's the obvious way this fix could go wrong. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 640f0d0bc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try? await Task.sleep(nanoseconds: UInt64(PendingSessionSettleStates.staleAfter * 1_000_000_000) + 250_000_000) | ||
| guard let self, !Task.isCancelled else { return } | ||
| self.pendingSessionSettleBackstopTask = nil | ||
| // Reading through the chokepoint prunes and repaints if anything expired. | ||
| _ = self.localSessions() |
There was a problem hiding this comment.
When a live lifecycle request takes 20.25–30 seconds, the timer armed by beginPendingSessionSettle still wakes here while sendSessionLifecycleCommand is awaiting its response, calls localSessions(), and expires the intent because the connection remains live and its timestamp still reflects send time. Restarting the backstop after the response cannot restore an intent this sweep already removed. The fresh evidence after the claimed fix is that the send-time sweep remains armed at line 9333; cancel or hold that sweep while the matching request is outstanding.
Useful? React with 👍 / 👎.
|
@codex review Fixed properly this time. You were right that restarting the window after the response was the wrong shape — the sweep armed at send fired mid-await and removed the intent, and a restarted window can't bring back something already gone. An intent is now unexpirable while its own request is outstanding, and the window starts when the response arrives; neither Three existing tests were asserting expiry on intents whose request had never answered — that's no longer the contract, so they answer first now. Kept the test proving a late answer from a replaced command can't extend the newer intent. Unrelated: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c26d29156
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pendingSessionSettleStates.restartBackstop( | ||
| for: trimmed, | ||
| token: token, | ||
| uptime: ProcessInfo.processInfo.systemUptime | ||
| ) |
There was a problem hiding this comment.
When sendCommand returns its queued: true sentinel for an offline or timed-out lifecycle command, this unconditionally marks the intent as answered even though the host has not processed it. On reconnect, the single 20-second rebase at the start of performInitialHydration can expire while hydration—or the subsequent replay request with its 30-second timeout—is still running, so the row snaps back before the queued settle finishes. Fresh evidence beyond the earlier reconnect finding is that queued replay never restores awaitingResponse; preserve that state until flushPendingOperations receives the replay response.
Useful? React with 👍 / 👎.
|
@codex review Fixed. Agreed the The lifecycle send now reports whether the host actually answered; a queued command leaves its intent outstanding, and |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c0a8120d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pendingSessionSettleStates.markAnswered( | ||
| for: sessionId, | ||
| uptime: ProcessInfo.processInfo.systemUptime | ||
| ) |
There was a problem hiding this comment.
When an offline settle-family command is replayed and the host rejects it—for example, a dismiss-and-settle after the pending prompt has already changed—the error path at lines 18779-18785 removes the queued operation without reaching this state transition or clearing its intent. That intent therefore remains awaitingResponse == true, which PendingSessionSettleStates.prune retains unconditionally, so iOS can continue painting a state the host rejected indefinitely; associate the queued operation with its intent and retire that intent on terminal replay errors.
Useful? React with 👍 / 👎.
| for sessionId in queuedLifecycleSessionIds(action: operation.action, args: args) { | ||
| pendingSessionSettleStates.markAnswered( | ||
| for: sessionId, | ||
| uptime: ProcessInfo.processInfo.systemUptime | ||
| ) |
There was a problem hiding this comment.
When two settle-family commands for the same session are queued offline, such as settle followed by unsettle, the queue drains them in append order but this lookup identifies the pending intent only by session ID. Completion of the first replay therefore marks the newer unsettle intent as answered and starts its 20-second window before that command's own replay begins; if the second replay takes longer than 20 seconds within its valid 30-second request timeout, the overlay expires mid-request. Fresh evidence after the queued-sentinel fix is that replay completion still carries no operation ID or intent token, so it should only transition the intent created by that exact queued command.
Useful? React with 👍 / 👎.
|
@codex review Both fixed, and they had one root: the durable queue entry carried no reference back to the intent that created it. The queued sentinel now returns its Tests for both: Worth saying: this replay path only exists because of the offline-queue interaction, and you've now found five real defects in it across successive rounds. If the next pass surfaces another, I'd rather cut the offline-hold behaviour entirely for step 0 — reverting to "a queued settle simply expires at the backstop" — than keep growing state to chase correctness in a path whose whole purpose is a cosmetic flicker. Interested in whether you'd read that as the safer shape. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a92a14ec0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| record["ok"] == nil, | ||
| record["queued"] as? Bool == true | ||
| else { return nil } | ||
| return (record["commandId"] as? String).flatMap(normalizedLifecycleSessionId) |
There was a problem hiding this comment.
When a settle-family command is queued offline or after a live timeout, sendSessionLifecycleCommand calls the sendCommand implementation at lines 18854–18922, whose two queue branches still return only ["queued": true] and, in the offline branch, enqueue a separately generated ID. Consequently this extraction always returns nil, so the caller treats the queued operation as a live answer, starts the 20-second backstop, and never attaches an ID that replay success or rejection can match. The fresh evidence beyond the earlier queued-intent findings is that the added commandId was returned only by performCommandRequestSafe, which this lifecycle path does not use; return the actual enqueued operation ID from sendCommand instead.
Useful? React with 👍 / 👎.
|
@codex review Confirmed and fixed — this one was my mistake, not a new design flaw. I added Both queue branches of that sender now return the enqueued id, with the offline branch generating it explicitly. On my earlier offer to cut the offline-hold behaviour: I'm keeping it, since this round was a mechanical error rather than the design failing again, and the machinery is now correct and covered. But I'll note the complexity it added honestly in the summary. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
`terminal_sessions` is a CRR table, and iOS wrote `settled_at` into its own replica optimistically before sending the settle command. That write replicates upstream carrying no host lifecycle revision, so it can win a CRDT merge against a host that REJECTED the settle — filing a live session as done. It is a guard defeated by a merge rather than by a caller, which is why it has to close before the settle-writer chokepoint (step 1) can promise anything. iOS no longer writes the settle columns at all. `updateSessionLifecycle` becomes `updateSessionSnoozeOverlay`, scoped to the snooze/woke columns, which keep their optimistic write plus rollback because they guard no host decision. Instant feedback for settle now comes from `PendingSessionSettleStates`: a local, non-persisted overlay applied when session rows are read, resolved when the host's changeset confirms it, when the command fails, or by a bounded staleness backstop. `settle_source` is left out — no iOS surface reads it. Removing the write only fixes new builds, so the host enforces the rule: `syncHostService` drops inbound `terminal_sessions.settled_at` / `settle_override` / `settle_source` from PHONE peers, alongside the existing `sync_cluster_state` filter. Per-column and silent — the rest of the batch still applies and still acks ok, since a rejected ack would stall the peer's outbound cursor. Desktop peers are exempt on purpose: they run the same `sessionService` chokepoint, so their settle writes are host-decided and must keep replicating. A pre-fix phone now diverges locally instead of corrupting the host, and self-heals on the next `refreshWorkSessions`. Recorded as amendment 6 in the design doc.
…split
Three review rounds against the step 0 change. The findings that mattered:
**The overlay was bypassed where it mattered most.** `refreshActiveSessionsAndSnapshot`
read the database directly, so the widget, Live Activity, and Activity drawer
kept showing a just-settled chat as a live agent — and worse, beginning a settle
bumped the projection, which is precisely what re-ran that blind reader. Fixed by
making the overlay a real read chokepoint (`localSessions` / `localSession(id:)`)
and routing every UI-feeding read through it. Two raw reads remain and both are
deliberate: the pre-command snapshot the snooze rollback restores, and an
existence check the overlay cannot affect.
**A settle taken offline un-settled itself after 20 seconds.** `session.settleSessions`
is queueable, so offline it returns `{queued: true}` and waits — for minutes. The
staleness backstop aged it out on wall clock, so the row snapped back to unsettled
and then settled again when the queue drained. `holdBackstop` now holds the
deadline while the host is unreachable, making `staleAfter` a budget of reachable
time. Sampled at each read, not integrated — documented, and the safe direction.
**`sendSessionLifecycleCommand` was two functions in one coat.** Settle and snooze
apply completely different optimism (a local overlay vs. a replicating column
write) and no caller ever mixes them. Split into a core that takes a `rollback`
closure plus `sendSessionSettleCommand` / `sendSessionSnoozeCommand`. Guard
ordering and error paths are unchanged.
Also: `String??` pairs became a `Kind` enum; the overlay is dropped on unpair, not
just project switch (an unreachable host would otherwise hold it for the app's
life); `.unsettle` now predicts the host's `settle_override` branch, which is
decidable from the row, so a session settled purely by a pin finally shows
feedback; rollback clears are scoped by token so a slow command's failure cannot
retire a newer intent.
Two bugs caught in the fixes themselves: change-detection that reported the
offline re-stamp as a resolution (would have looped the repaint), and a
satisfaction rule I verified against the host SQL rather than assuming.
Host side: `isPhonePeer` for the asymmetric rule, tests moved into the existing
`inbound changeset_batch guards` block beside the `sync_cluster_state` sibling
they mirror, and the shared outbound helper restored. Docs reconciled — §3c-i
past-tensed, the "rollback becomes unnecessary" prediction corrected (it survived,
scoped to snooze), and §5 step 0 marked landed.
The overlay type was already covered exhaustively, but the defect that actually shipped lived in the WIRING: `refreshActiveSessionsAndSnapshot` read the database directly, so a chat the user had just settled kept reporting as a live agent on the lock-screen widget, the Live Activity, and the Activity drawer. A test of the struct alone cannot see that. Adds three DB-backed tests through a `#if DEBUG` seam — `fetchSessions` and `fetchSession(id:)` overlay the row while the database stays untouched (nothing can replicate), and a settled chat leaves `activeSessions`. The last one was verified against the pre-fix behavior: reverting the read to `database.fetchSessions()` fails it with "a settle the user just tapped must not keep reporting as a live agent". Without that check the test would have been vacuous. Writing it also corrected the fixture rather than the code: a mid-stream chat deliberately does NOT leave the roster, because a declared settle is honored only at rest (`WorkSessionCanonicalState`). The test now uses the at-rest state a user actually settles from. Docs: the sync docs described host authority only at TABLE granularity, so the per-column rule had nowhere to live. `sync-and-multi-device/README.md` gains a host-authoritative-columns section, `crdt-model.md` gains the merge-semantics consequence (a merge is not a caller, so a host-side check cannot guard a replicated column), and `ARCHITECTURE.md`'s `terminal_sessions` row now names the authorship rule. Also corrects an overstatement in my own earlier wording: "no replica can defeat the revision guard" → "a phone replica cannot", since desktop peers still author these columns by design.
The offline hold kept a queued settle's overlay alive while unreachable, but the last hold was stamped at the last read BEFORE reconnect — which can be minutes earlier, since nothing reads the projection while offline. The first reachable read would then expire the overlay and flip the row back to unsettled moments before the flushed command's changeset landed, which is the exact flicker the hold exists to prevent. Re-stamp the deadlines as the pending-operation queue flushes: the command is going out now, so it gets a full window to be answered. Docs: the pre-fix-phone heal is a full replace within the active project's lanes, not a per-row merge — so the host's value always wins, and rows in a project the phone has not activated stay stale until it is.
…ration Review finding (CodeRabbit, verified): the settle-column guard used `isMobileChangesetPeer`, which reads only the `hello` metadata the peer sends about itself, so a paired phone claiming `deviceType: "desktop"` would have slipped past it. `isMobilePeer` already exists and is stronger: for a record-backed peer (`paired` / `account`) it resolves through the PAIRING RECORD the host stored at pairing time, falling back to self-declared metadata only for bootstrap-token peers. Switching to it closes the spoofing gap the design doc had recorded as an accepted limitation, so the docs are corrected too rather than left claiming the weaker guarantee. Still a compatibility guard rather than a hard boundary — a bootstrap peer is classified from what it says about itself — and the complete closure remains step 1's host-local lifecycle revision. The docs say exactly that.
Review finding (Codex, P2, verified and reproduced): two lifecycle commands can overlap, and value equality alone confirms the wrong one. The settle overlay makes a row read as settled, so the row menu offers Unsettle and the user can tap it before the settle has landed. The newer unsettle intent wants `settled_at = nil` — which is exactly what the stale row still holds — so the next read confirmed and retired it immediately. The first command's changeset then painted the row settled while the user's later unsettle was still in flight. An intent now records the row as it stood when its command was sent, and is confirmed only once the row has actually MOVED from that baseline and matches what was asked for. A nil baseline (row unknown at begin) falls back to value equality, which is all there is to go on. Pinned by `testAReplacementIntentSurvivesUntilTheRowActuallyMoves`, verified non-vacuous: forcing the movement gate true fails it on both the pre-landing and post-first-command assertions.
Review finding (Codex, P2, verified): the same overlapping-command window, one layer down. A row with an `"active"` keep-active pin is settleable, so the user can tap Settle and then Unsettle before the settle lands. `.unsettle` chose its override branch by reading the LIVE row, which still carries the pin — so the overlay resurrected a keep-active pin and offered the wrong actions. Host-side the two commands run in order: the settle clears the pin unconditionally, then the unsettle preserves whatever is left, so the run actually ends with no override. An intent now records the override as it was PRESENTED when its command was issued — the raw row with any intent it replaced already applied over it — and the unsettle branch follows that. A standalone unsettle over a real pin still shows the pin, because there the presented value and the row agree. Two tests: the overlapping case, and the standalone case that must not regress.
…t reconnect Two review findings (Codex, P2 each), both verified. **An earlier command's changeset could confirm a later one.** Movement off the baseline proves only that *some* settle-family mutation replicated, not that the latest command applied. With `settle → unsettle → settle` issued inside one replication window, the first settle both moves the row and matches the third intent, so it retired the overlay — then the intervening unsettle replicated and the row flipped. The fix Codex asks for is a per-command marker, and the phone has none: the host's lifecycle revision is host-local, and inventing a replicated one is step 1's job, not step 0's. So the overlay stops guessing instead. An intent that replaced one still in flight is deliberately unconfirmable — it keeps showing what the user last asked for and yields to replicated truth at the backstop, by which point the run has converged. Briefly trailing the truth beats confidently showing the wrong command's result. **A queued settle could still snap back on reconnect.** `performInitialHydration` reads (and posts a database change) before `flushPendingOperations` reaches its success-path hold, and reconnect makes `canSendLiveRequests()` true immediately — so a settle queued longer than `staleAfter` expired in the gap. Deadlines are now rebased at the top of hydration, the earliest point the connection is usable. This also closes the "best-effort, not deterministic" caveat the earlier revalidation had accepted.
Review finding (Codex, P2, verified): `staleAfter` was a promise the code did not keep. `prune` runs only from a session read, and reads are driven by database changes. A command whose confirming changeset never arrives produces no database change — so on a quiet screen nothing ever re-evaluated the deadline and the overlay could persist indefinitely. The immediate post-command reload happens well inside the window, so it does not help. Adds the timer that enforces it: a one-shot sweep armed when an intent begins and re-armed whenever deadlines are rebased, which reads through the chokepoint (so it prunes and repaints) and re-arms while any intent is still in flight. It stops as soon as the map empties, and is cancelled on teardown. An earlier revalidation had logged this as Low and accepted it as "bounded". It was not bounded, and calling it that in the docs was wrong — hence fixing rather than re-accepting it.
Review finding (Codex, P2, verified): the backstop compared wall-clock `Date`s, so an NTP correction or a manual time change could expire a fresh overlay the instant time jumped forward, or hold one far past the promised window when it jumped back. Deadlines now use `ProcessInfo.processInfo.systemUptime`. The displayed settle timestamp stays a real `Date` — that one is shown to the user, not measured with. This follows the precedent already in this file: `backgroundedAtUptime` is deliberately monotonic for the same reason, with a comment about a device whose clock moves backward during suspension. The tests now advance a numeric uptime rather than a wall clock, which also makes them independent of the machine date.
Review finding (Codex, P2, verified): a 20s backstop against a 30s request timeout is inconsistent. The window started at send, so a connected host that took longer than `staleAfter` to answer had the overlay expired underneath a command that was still perfectly valid — the row snapped back to replicated state and then flipped again when the host succeeded. A long host operation also left progressively less of the window for the confirming changeset. `staleAfter` bounds the wait for the CHANGESET, not the round trip, so the countdown now restarts when the command is answered. Token-scoped, so a slow command cannot extend an intent the user has since replaced — covered by its own test alongside the restart itself.
Review finding (Codex, P2, verified): the previous fix restarted the window after the response, but the sweep armed at send still fired mid-await and removed the intent — and restarting a window cannot restore an intent that is already gone. An intent is now unexpirable while its own request is outstanding, and the window starts when the response arrives. Neither `prune` nor the sweep can retire it in between. Bounded by the request's own timeout: a failure throws and the rollback clears the intent. Three existing tests were asserting expiry on intents whose request had never answered, which is no longer the contract — they now answer first. Added a test for the outstanding-request case itself, and kept the one proving a late answer from a replaced command cannot extend the newer intent.
Review finding (Codex, P2, verified): `{queued: true}` is durable acceptance by
this device, not an answer from the host — it has not seen the command at all.
Marking the intent answered started a window that could expire during reconnect,
while hydration or the replay request (with its own longer timeout) was still
running, snapping the row back before the queued settle finished.
The lifecycle send now reports whether the host actually answered, and a queued
command leaves its intent outstanding. `flushPendingOperations` marks exactly the
session ids carried by the replayed command as answered and starts their window
then, so the state is restored by the replay rather than never.
… host refuses Two review findings (Codex, P2 each, verified), both from one missing link: the durable queue entry carried no reference back to the intent that created it. **A refused replay left the overlay painting forever.** The terminal-error path removed the queued operation without touching the intent, which stayed `awaitingResponse` — and an outstanding intent is deliberately unexpirable. A dismiss-and-settle the host rejects on replay would have shown a refused state indefinitely. **A replay resolved the wrong intent.** With two commands for one session queued together, they drain in append order, but completion was matched on session id alone — so the first replay started the second intent's window before its own replay had begun, and a replay slower than `staleAfter` (inside its valid timeout) expired the overlay mid-request. The queued sentinel now carries its `commandId`, the intent records it, and both replay outcomes are matched on that id: success answers exactly that intent, terminal failure retires it.
…ally uses Review finding (Codex, P2, verified): the previous commit added `commandId` to `performCommandRequestSafe`, which the lifecycle path does not call. The sender it does use returned a bare `["queued": true]` — and its offline branch did not even pass an id, letting `enqueueOperation` generate one nobody could see. So the extraction always returned nil, the whole operation-binding fix was dead code, and a queued command was once again treated as answered: window started, no id attached, nothing for replay success or rejection to match. A fix in the wrong function reads as a fix and is worse than none, because the tests around it still pass. Both queue branches of that sender now return the enqueued id, with the offline branch generating it explicitly rather than leaving it implicit.
94e4867 to
e7b6f32
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile has paused reviews on this repository — it used its 100 free open-source review credits for this billing period. Reviews resume automatically on September 7. To continue before then, an organization admin can keep reviews running past the free credits — those bill as normal usage.
1 participant
Settle currently writes a lifecycle column and stops nothing, and the plan for making it stop work (settle-teardown design) rests on a revision-guarded write. This is step 0, the precondition that makes such a guard mean anything.
The problem
terminal_sessionsis a cr-sqlite CRR table. iOS wrotesettled_atinto its own replica optimistically before sending the host command, so the write replicated upstream carrying no host lifecycle revision. If the host rejected a settle, the phone's row merged in and settled the session anyway.That is a guard defeated by a merge, not by a caller — no amount of host-side checking closes it, which is why it has to land before the chokepoint (step 1) can promise anything.
What changed
iOS stops writing the settle columns.
Database.updateSessionLifecyclebecomesupdateSessionSnoozeOverlayand can no longer expresssettled_at/settle_override/settle_source— a compile error, not a convention. Optimistic responsiveness is preserved byPendingSessionSettleStates, a purely local, non-persisted overlay applied at a session read chokepoint (localSessions()/localSession(id:)), resolved when the host's changeset confirms it, on command failure, or by a bounded staleness backstop.settle_sourceis excluded — no iOS surface reads it.The snooze overlay keeps its optimistic write + rollback. Those columns guard no host decision, so a merge cannot defeat one. The design doc predicted the rollback would become dead code; it did not, because it is shared with snooze. That prediction is corrected in the doc rather than silently diverged from.
The host enforces the rule against older phones. Removing the write only fixes new builds, so
syncHostServicedrops inboundterminal_sessions.settled_at/settle_override/settle_sourcefrom phone peers. Per-column and silent: the rest of the batch applies and the batch still acksok, because a rejecting ack would stall the peer's outbound cursor. Desktop peers are exempt on purpose — they run the samesessionServicechokepoint. A pre-fix phone now diverges locally instead of corrupting the host, and heals on the nextrefreshWorkSessions.This is a compatibility guard, not a security boundary —
isMobileChangesetPeerreads the peer's self-declared metadata. The real closure is step 1's host-local revision. The doc says so plainly.CRR optimistic-write sweep
The settle write was found via an existing code comment naming the hazard, so I swept
apps/iosfor siblings of the same class rather than fixing one instance:settled_at/settle_override/settle_sourcesnoozed_until,snoozed_at,woke_*)pinned/title/manually_namedrenameSession(local write, no host command)purgeRetiredTerminalSessions(replicating DELETE)tool_type = 'run-shell', same cascades)All other
terminal_sessionswriters run undershouldCaptureLocalChanges = falseand do not replicate.Review rounds
Three
/qualityrounds plus a commit-bound revalidation. The findings worth naming:refreshActiveSessionsAndSnapshotread the database directly, so a just-settled chat kept reporting as a live agent on the lock-screen widget, Live Activity, and Activity drawer — and beginning a settle woke that blind reader. Fixed with the read chokepoint.Tests
PendingSessionSettleOverlayWiringTestswas verified against the pre-fix behavior — reverting the read todatabase.fetchSessions()fails it with "a settle the user just tapped must not keep reporting as a live agent". Without that check it would have been vacuous.Writing it also corrected the fixture rather than the code: a mid-stream chat deliberately does not leave the roster, because a declared settle is honored only at rest.
DatabaseService+SyncService, no mocks)inbound changeset_batch guardsblock, beside thesync_cluster_stateguard they mirrorParity
Map/Setfiltering — no paths, processes, IPC, or native surface.windows-foundationalready runssyncHostService.test.tsonwindows-latest, and desktop's source is a one-line re-export of the changed implementation.ok, advances the cursor, and heals; new-phone/old-host has nothing to filter and is bounded by the backstop. Capability gating unchanged.docs/logging.mdexplicitly excludes.Known and bounded
Four Low findings accepted, not fixed: the flush-path hold re-stamps all in-flight intents rather than only the queued session's; it is best-effort rather than deterministic on the reconnect path; a drained-but-declined settle gets one extra backstop window; and the flush-path behavior itself is untested. Each is bounded at ≤20s and none is a correctness defect. The deterministic alternative would add durable-queue decoding to a per-CRDT-tick read path — more risk than the tail it removes.
Next: step 1 (settle-writer chokepoint + lifecycle revision) and step 2 (settling state + abort rule) ship as their own PRs. All ten
settled_atmutation sites are confirmed to live insessionService.tsand nowhere else.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
The PR makes session settlement host-authoritative by replacing iOS CRR lifecycle writes with a local pending-state overlay and filtering legacy phone-authored settle columns at the host.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
Reviews (12): Last reviewed commit: "fix(ios): return the queue id from the s..." | Re-trigger Greptile
Context used:
ade codeTUI