refactor(server): settled state is now server-authored, ending client drift - #5462
refactor(server): settled state is now server-authored, ending client drift#5462t3dotgg wants to merge 1 commit into
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.github.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two Effect service convention violations in the new ThreadAutoSettleReactor service. The rest of apps/server/src already follows the canonical single-module make + layer shape (e.g. vcs/VcsStatusBroadcaster.ts, background/BackgroundPolicy.ts), so the new service is the outlier here. Everything else in the diff (namespace subpath imports, dependency acquisition via yield* Foo, pure-config options, test-only Layer.succeed/Layer.mock seams, VcsStatusBroadcaster.peekStatus addition, contracts/settings moves) looks consistent with the conventions.
Posted via Macroscope — Effect Service Conventions
![cursor[bot]](https://avatars.github.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR fundamentally changes settled-state handling from client-derived to server-authored, introducing a new periodic reactor and schema changes. The architectural scope—new server infrastructure, capability flags, and settings migration—plus an unresolved High severity finding about shell schema compatibility warrant human review. You can customize Macroscope's approvability policy. Learn more. |
bfa8139 to
c2b77f2
Compare
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/server/src/orchestration/Layers/ThreadAutoSettleReactor.ts`:
- Around line 169-185: The inactivity-candidate flow around
resolveAutoSettleVerdict must treat a cached "open" result from
peekChangeRequestState as "unknown" so it reaches the existing verification
path. Preserve the cooldown, background-policy gate, and verifyBudget checks,
then use verifyChangeRequestState to refresh and settle when the PR is merged or
closed. Add a focused test covering peekStatus returning open and refreshStatus
returning merged or closed.
In `@apps/server/src/orchestration/Services/ThreadAutoSettleReactor.ts`:
- Around line 19-35: Move ThreadAutoSettleReactor and its layer implementation
into the canonical orchestration/ThreadAutoSettleReactor module, inline
ThreadAutoSettleReactorShape in Context.Service, and export the service type,
make, and layer members there. Update all consumers to import
ThreadAutoSettleReactor from the canonical module instead of the Services/ or
Layers/ modules, removing the obsolete split definitions.
In `@apps/web/src/components/settings/BetaSettingsPanel.tsx`:
- Around line 112-114: Update the AutoSettleDaysInput usage in BetaSettingsPanel
so updateServerSettings is not called for every valid keystroke; commit the
fully validated draft threshold only on blur or Enter, preserving the existing
threadAutoSettleAfterDays setting update once editing completes.
In `@packages/contracts/src/orchestration.ts`:
- Around line 596-600: Keep auto-settle backdating server-only: in
packages/contracts/src/orchestration.ts:596-600, remove settledAt from the
client-callable thread.settle contract or provide a separate server-only
auto-settle command; in apps/server/src/orchestration/decider.ts:500-502, derive
the timestamp exclusively from trusted server projection data; in
apps/server/src/orchestration/decider.settled.test.ts:111-130, update coverage
to exercise the trusted server path without accepting a caller-provided
timestamp.
In `@packages/contracts/src/settings.ts`:
- Around line 536-541: The default for threadAutoSettleAfterDays must preserve
clients that previously persisted sidebarAutoSettleAfterDays: null instead of
enabling auto-settlement with 3 days. Add a one-time migration that carries the
explicit null forward, or change the server default to a disabled-safe value,
and add coverage verifying the persisted null case remains disabled after
decoding.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 732a9a94-b034-4f54-a494-b6616cb226ed
📒 Files selected for processing (33)
apps/desktop/src/settings/DesktopClientSettings.test.tsapps/mobile/src/features/home/HomeScreen.tsxapps/mobile/src/features/threads/ThreadNavigationSidebar.tsxapps/mobile/src/features/threads/thread-list-v2-items.tsxapps/mobile/src/features/threads/threadListV2.test.tsapps/mobile/src/features/threads/threadListV2.tsapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/src/environment/ServerEnvironment.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/OrchestrationReactor.test.tsapps/server/src/orchestration/Layers/OrchestrationReactor.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsapps/server/src/orchestration/Layers/ThreadAutoSettleReactor.test.tsapps/server/src/orchestration/Layers/ThreadAutoSettleReactor.tsapps/server/src/orchestration/Services/ThreadAutoSettleReactor.tsapps/server/src/orchestration/autoSettle.test.tsapps/server/src/orchestration/autoSettle.tsapps/server/src/orchestration/decider.settled.test.tsapps/server/src/orchestration/decider.tsapps/server/src/server.tsapps/server/src/vcs/VcsStatusBroadcaster.tsapps/web/src/components/ChatView.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/SidebarV2.tsxapps/web/src/components/settings/BetaSettingsPanel.tsxapps/web/src/hooks/useNowMinute.tspackages/client-runtime/src/state/threadSettled.test.tspackages/client-runtime/src/state/threadSettled.tspackages/contracts/src/environment.tspackages/contracts/src/orchestration.tspackages/contracts/src/settings.test.tspackages/contracts/src/settings.ts
💤 Files with no reviewable changes (3)
- apps/desktop/src/settings/DesktopClientSettings.test.ts
- apps/mobile/src/features/threads/thread-list-v2-items.tsx
- apps/web/src/hooks/useNowMinute.ts
github-actions
Bot
added
size:XXL
12b1dc6 to
671e346
Compare
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
671e346 to
407c63e
Compare
09f046c to
5199aca
Compare
| return yield* updateCachedStatus(cwd, local, remote); | ||
| }); | ||
|
|
||
| const peekStatus: VcsStatusBroadcaster["Service"]["peekStatus"] = Effect.fn( |
There was a problem hiding this comment.
🟠 High vcs/VcsStatusBroadcaster.ts:346
peekStatus merges independently cached local and remote halves without verifying they describe the same checkout. When refreshLocalStatusCore updates only cached.local after a branch switch, the stale cached.remote from the previous branch remains in the cache. mergeGitStatusParts pairs the new branch name from local with the old branch's PR data from remote, so the auto-settle reactor can see a new branch alongside a merged/closed PR from the prior branch and incorrectly settle an active thread. Consider invalidating the remote half when the local checkout identity changes, or adding a coherence check before merging in peekStatus.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/vcs/VcsStatusBroadcaster.ts around line 346:
`peekStatus` merges independently cached local and remote halves without verifying they describe the same checkout. When `refreshLocalStatusCore` updates only `cached.local` after a branch switch, the stale `cached.remote` from the previous branch remains in the cache. `mergeGitStatusParts` pairs the new branch name from local with the old branch's PR data from remote, so the auto-settle reactor can see a new branch alongside a merged/closed PR from the prior branch and incorrectly settle an active thread. Consider invalidating the remote half when the local checkout identity changes, or adding a coherence check before merging in `peekStatus`.
There was a problem hiding this comment.
Fixed in 72f1b2f at the consumer: peekChangeRequestState now requires the cached PR's headRef to equal the thread's branch, so a stale local/remote pairing after a branch switch (new refName + previous branch's PR) maps to "unknown" and live-verifies instead of settling. I kept the fix in the sweep rather than changing peekStatus/cache invalidation because the streaming path already tolerates the transient mismatch (rows re-render when the remote half refreshes) and the sweep is the only consumer that acts irreversibly on the merged view.
2693eda to
ffb4e20
Compare
There was a problem hiding this comment.
One convention finding in the new server-side auto-settle code: the test harness references the service interface through Parameters<typeof Tag["of"]>[0] instead of the canonical Tag["Service"]. The reactor module itself now follows the canonical layout (single orchestration/ThreadAutoSettleReactor.ts, inline interface in Context.Service, real make, layer, all dependencies acquired via yield*), so the earlier layout/naming findings are resolved.
Posted via Macroscope — Effect Service Conventions
ffb4e20 to
9763b02
Compare
Settled classification used to be re-derived per client (inactivity window, PR state, clock hacks), with real drift between web and mobile. The server is now the single author of settled state: a ThreadAutoSettleReactor sweep dispatches thread.settle for quiet threads and merged/closed PRs, the auto-settle window moved to ServerSettings, and clients just read settledOverride. Includes the review-hardening rounds: settledAt derived in the decider from read-model activity (unforgeable, restart-safe via a projected latestUserMessageAt stamp, revert-consistent, clock-skew clamped), only live-confirmed PR states act as settle authority (cached open/closed/no-PR re-verify with cooldown), fail-safe settings reads, and capability-gated settings UI with blur-committed input. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5d03b32 to
8b8bec7
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8b8bec7. Configure here.
| // array: the engine's command read model boots threads with no message | ||
| // bodies, and the decider needs this stamp for settle invariants and the | ||
| // settledAt derivation. Optional for pre-existing payloads. | ||
| latestUserMessageAt: Schema.optional(Schema.NullOr(IsoDateTime)), |
There was a problem hiding this comment.
Shell stamp breaks old servers
High Severity
latestUserMessageAt is required on OrchestrationThreadShell, unlike neighboring lifecycle fields that use Schema.optional for old-server and cached-snapshot interop. The full OrchestrationThread marks the same field optional. New clients decoding shells from older servers or pre-upgrade persisted snapshots missing the key can fail the shell stream or cache hydrate, which breaks the thread list despite the PR’s graceful-degradation goal for older servers.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8b8bec7. Configure here.
There was a problem hiding this comment.
False positive — the required latestUserMessageAt on OrchestrationThreadShell (L464) is pre-existing and byte-identical to main (main L459); this PR does not touch it, so old-server interop is unchanged. The field this PR ADDS is on the full OrchestrationThread detail model (L404), and that one is Schema.optional for exactly the interop reason you describe.
1 participant
"Settled" was split across the stack: the server stored a user override while every client re-derived the actual classification from an inactivity window, per-row PR state, and clock heuristics. The copies had drifted — mobile hardcoded the 3-day window web made configurable, sorted the settled shelf by a different key, and inverted the capability-gate default — so the same thread could be settled on one device and active on another.
Now the server is the single author of settled state and clients just read
settledOverride:ThreadAutoSettleReactorsweeps once a minute and dispatches the existingthread.settlecommand for threads that qualify: quiet past the inactivity window, or on a merged/closed PR. All existing decider invariants and the activity-driven auto-unsettle apply unchanged, so a raced sweep can never hide live work.ServerSettings.threadAutoSettleAfterDays— one value per environment, same shelf on every device.settledAtto the thread's last activity, and both platforms now sort the settled shelf bysettledAt— fixing the ordering drift.ServerSettings.threadAutoSettleOnMerge, applied by the sweep. Clients keep a smallchangeRequestAutoSettleshelper for display only (Woke-pill suppression). Mobile's device-local copy of the toggle is removed — settle policy has no per-device knobs.effectiveSettledderivation (window/PR/clock inputs, the "serverAdjudicated" clock-skew hack), the per-row PR-state lift-up machinery on web and mobile, web'suseNowMinutehook, and mobile's hardcoded window.effectiveSettledis now a plain override read with a blocked-work guard.Old servers never emit the override, so their threads simply stay active — same graceful degradation as before, minus a capability check per row.
Built by Claude Fable 5 via Claude Code.
Note
High Risk
Changes core thread-list behavior and settlement timing across server and all clients; incorrect sweep or
settledAtderivation could hide active work or settle threads users still care about.Overview
Thread settlement is now server-authored so web, mobile, and desktop no longer disagree on whether a thread is settled.
A new
ThreadAutoSettleReactorruns periodic sweeps using pure policy inautoSettle.ts: inactivity (threadAutoSettleAfterDays), merged/closed PR rules (threadAutoSettleOnMerge), and cooldown-limited VCS/PR verification viaVcsStatusBroadcaster.peekStatus/refreshStatus. Qualifying threads getthread.settledispatched server-side; the decider derivessettledAtfrom last activity (includinglatestUserMessageAton the read model) instead of settle time.Clients stop re-deriving settled state:
effectiveSettledis asettledOverrideread plus local guards (live session, pending input, user message newer thansettledAt). Removed are client-side inactivity/PR/clock partitioning, per-row PR state lift-up on lists,useNowMinutefor settle, and mobile/desktopsidebarAutoSettle*/autoSettleOnMergepreferences. Auto-settle knobs move to server settings (threadAutoSettleAfterDays,threadAutoSettleOnMerge), gated by athreadAutoSettlecapability; settled shelf ordering aligns onsettledAtacross platforms.Reviewed by Cursor Bugbot for commit 8b8bec7. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Move thread settled state from client-side heuristics to server-authored settlement
ThreadAutoSettleReactoron the server that periodically sweeps threads and dispatches settle commands based on inactivity windows and PR merge state read fromServerSettings.effectiveSettledinthreadSettled.tsis rewritten to only classify a thread as settled when the server has setsettledOverride = 'settled'and no newer user message exists; all client-side inactivity/PR-state paths are removed.threadAutoSettleAfterDays,threadAutoSettleOnMerge) move fromClientSettingstoServerSettings; the web settings panel now reads/writes server-scoped fields and only renders when the server advertises thethreadAutoSettlecapability.thread.settledevents now stampsettledAtwith the thread's last recorded activity time rather than the command dispatch time, affecting shelf ordering.changeRequestStateByKey,autoSettleOnMerge, or wall-clocknowinto thread list partitioning.effectiveSettledno longer accepts any options object; all callers must be updated, and threads will not appear settled until the server reactor marks them.Macroscope summarized 8b8bec7.
Summary by CodeRabbit
New Features
Improvements