Sync remote thread review state - #411
Conversation
SergeSerb2
left a comment
SergeSerb2
left a comment
There was a problem hiding this comment.
Blocking: Missing update to displayEquivalent(to:) method and memoization test for the new autoReviewPhase field.
The PR adds server-authoritative remote thread state sync, but without updating displayEquivalent(to:), the field will be silently dropped in upserts. This is a known trap (PR #328/snooze bug) — every new ChatThread field must join both displayEquivalent(to:) comparison and memoization tests.
The PR also expands the fields being synced from remote threads (title, scenery, modelSelection, etc.). Verify all of these are already in displayEquivalent(to:) or add them if needed.
SurgeCode auto-review · model=codex/gpt-5.6-luna · head=deba1cf70a58
| public var hasPendingApproval: Bool | ||
| public var hasPendingUserInput: Bool | ||
| public var hasActionableProposedPlan: Bool | ||
| /// Server-authoritative auto-review lifecycle, preserved separately from |
There was a problem hiding this comment.
blocking: Critical: New field autoReviewPhase added, but displayEquivalent(to:) method is not updated. Without this, upserts will silently drop this field when syncing between Macs.
Add autoReviewPhase to the displayEquivalent(to:) comparison and add a memoization test. Reference: PR #328 (snooze bug) — same root cause.
| sequence: 21, threadID: threadID, autoReviewPhase: "reviewing"))) | ||
|
|
||
| #expect(await backend.debugThreadStatus(threadID: threadID) == .reviewing) | ||
| } |
There was a problem hiding this comment.
blocking: Test covers the detail snapshot case, but missing memoization test that verifies autoReviewPhase is preserved through an upsert cycle (same thread, different shell). This catches the displayEquivalent(to:) issue.
Add a test that calls upsert(chatThread) with an existing shell and verifies the field survives.
|
|
||
| shell.latestTurn = thread.latestTurn | ||
| shell.session = thread.session | ||
| shell.title = thread.title |
There was a problem hiding this comment.
important: The PR now syncs many fields that weren't synced before (title, scenery, modelSelection, runtimeMode, interactionMode, branch, worktreePath, etc.). Verify these are all handled in displayEquivalent(to:). If any were missing before, this expands the data-loss bug to all of them.
| public var hasPendingApproval: Bool | ||
| public var hasPendingUserInput: Bool | ||
| public var hasActionableProposedPlan: Bool | ||
| /// Server-authoritative auto-review lifecycle, preserved separately from |
There was a problem hiding this comment.
nit: Consider using an enum instead of String? for autoReviewPhase to provide type safety and self-document the valid phases (e.g., enum AutoReviewPhase { case idle, reviewing, completed }). Avoids invalid states and makes callers' code more legible.
SergeSerb2
left a comment
SergeSerb2
left a comment
There was a problem hiding this comment.
Adds autoReviewPhase field to ChatThread to preserve server-authoritative auto-review state across remote Mac clients. Field is synced from thread detail to shell state, with test coverage. Critical concern: verify the new autoReviewPhase field has been added to displayEquivalent(to:) method; per tracked issues, fields missing from displayEquivalent silently drop on upserts. Also, PR scope includes many additional property syncs beyond just autoReviewPhase—confirm this is intentional.
SurgeCode auto-review · model=codex/gpt-5.6-luna · head=deba1cf70a58
| public var hasPendingApproval: Bool | ||
| public var hasPendingUserInput: Bool | ||
| public var hasActionableProposedPlan: Bool | ||
| /// Server-authoritative auto-review lifecycle, preserved separately from |
There was a problem hiding this comment.
blocking: New ChatThread field autoReviewPhase must be verified in displayEquivalent(to:) method. The memory trap documents that fields missing from displayEquivalent silently drop on upserts (see display-equivalent-field-trap.md). The comment says this is "server-authoritative", not derived—confirm it belongs in (or is explicitly excluded from) the equivalence check with reasoning in a comment if excluded.
| threadID: threadID, sequence: sequence, source: .detail) | ||
| else { return false } | ||
| guard var shell = threadShellsByID[threadID] else { return false } | ||
|
|
There was a problem hiding this comment.
important: This diff expands shell-state sync to include title, scenery, modelSelection, runtimeMode, interactionMode, executor settings, branch, worktreePath, parentThreadId, createdAt, and latestUserMessageAt—well beyond the autoReviewPhase implied by the PR title. Clarify: is this scope expansion necessary for remote Mac sync to function, or should it be split into a separate PR? If intentional, update the PR body to document the broader sync strategy.
| shell.createdAt = thread.createdAt | ||
| shell.updatedAt = thread.updatedAt | ||
| shell.archivedAt = thread.archivedAt | ||
| if let latestUserMessageAt = thread.messages.last(where: { $0.role == .user })?.createdAt { |
There was a problem hiding this comment.
info: autoReviewPhase is synced and stored but not observed in the diff. Confirm where this field is consumed (UI rendering, status derivation, remote-client logic) so reviewers understand the full picture; if it's stored for future use, a comment noting that would help.
SergeSerb2
left a comment
SergeSerb2
left a comment
There was a problem hiding this comment.
No correctness issues found. The comment-only model change is harmless, and return correctly fixes the multi-statement Swift fixture helper so the remote review-state test compiles.
SurgeCode auto-review · model=codex/gpt-5.6-luna · head=3b064df63314
1 participant
Summary
Area
apps/mac— native macOS appRelease size
size:S— small user-visible fix or contained improvementVerification
pnpm run verify --allpasses (check + typecheck + tests; addslint:mobileand the Swift suite)pnpm run test:macpasses (covered by the full gate)Release notes
Remote-connected Macs now show the same auto-review and thread lifecycle state as the host.