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 8 commits 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.githubusercontent.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.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. This PR introduces a new server-side auto-settle reactor and moves settled state derivation from clients to the server - a significant architectural and runtime behavior change. Additionally, there are unresolved review comments including a high-severity concern about cache coherence that could incorrectly settle active threads after branch switches. You can customize Macroscope's approvability policy. Learn more. |
bfa8139 to
c2b77f2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.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
| // Days of inactivity before the server auto-settles a thread; null disables | ||
| // auto-settle. Server-side (not a client setting) so every client sees the | ||
| // same settled shelf — the server derives settled state, clients only read it. | ||
| threadAutoSettleAfterDays: Schema.NullOr(ThreadAutoSettleAfterDays).pipe( |
There was a problem hiding this comment.
🟡 Medium src/settings.ts:539
threadAutoSettleAfterDays defaults to 3 days for every existing server, but the auto-settle interval was previously a client setting. An existing user who disabled auto-settle (persisted null) or set a custom interval will silently revert to three days after upgrading, and the server can auto-settle threads contrary to their saved preference. A migration that reads the former client-side value and seeds the new server-side field is needed before applying this default.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/contracts/src/settings.ts around line 539:
`threadAutoSettleAfterDays` defaults to `3` days for every existing server, but the auto-settle interval was previously a client setting. An existing user who disabled auto-settle (persisted `null`) or set a custom interval will silently revert to three days after upgrading, and the server can auto-settle threads contrary to their saved preference. A migration that reads the former client-side value and seeds the new server-side field is needed before applying this default.
There was a problem hiding this comment.
🟢 Low
threadListV2Items passes ${nowMinute}:00.000Z as snoozeLabelNow to buildThreadListV2ListItems, which computes each snoozed row's snoozeWakeLabelText (the wake countdown). The useMemo dependency array omits nowMinute, and after this PR nowMinute was also removed from the threadListV2Layout dependencies — so a minute tick no longer invalidates either memo. Expanded snoozed rows keep a stale snoozeWakeLabelText until some other list state changes, so the countdown stops updating each minute. Add nowMinute back to the threadListV2Items dependency array.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/home/HomeScreen.tsx around line 666:
`threadListV2Items` passes `${nowMinute}:00.000Z` as `snoozeLabelNow` to `buildThreadListV2ListItems`, which computes each snoozed row's `snoozeWakeLabelText` (the wake countdown). The `useMemo` dependency array omits `nowMinute`, and after this PR `nowMinute` was also removed from the `threadListV2Layout` dependencies — so a minute tick no longer invalidates either memo. Expanded snoozed rows keep a stale `snoozeWakeLabelText` until some other list state changes, so the countdown stops updating each minute. Add `nowMinute` back to the `threadListV2Items` dependency array.
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
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 407c63e. Configure here.
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.
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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- settledAt is now derived in the decider from the thread's last recorded activity; the thread.settle command no longer accepts a caller-supplied timestamp, so shelf ordering cannot be forged. - A cached "open" PR no longer blocks auto-settle forever: the sweep maps it to "open-cached", which triggers the cooldown-limited live verification the unknown state already used. Only a live-confirmed open PR holds a quiet thread active. - The per-sweep verification budget is only consumed by lookups that actually ran; cooldown-suppressed calls no longer starve other checkouts. - effectiveSettled keeps a stale "settled" override from hiding a thread with a user message newer than settledAt (queued-turn reactivation window), using shell-only comparisons. - The auto-settle settings UI is gated on the threadAutoSettle capability and the days input commits on blur/Enter instead of per keystroke — transient values now drive a real server sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The command read model boots threads with an empty messages array, so the decider now reads the projected latestUserMessageAt stamp (newly carried on OrchestrationThread and maintained by the projector) alongside in-memory messages. Without it, a server restart erased user-message activity from both the queued-turn invariant and the settledAt stamp — an understated settledAt would then trip the client's reactivation guard and strand server-settled threads in the active list. - settledAt candidates are clamped to the command time so a client-supplied future message timestamp cannot forge shelf position. - An elapsed snooze wake now counts as activity for the settledAt stamp, matching the sweep's inactivity accounting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ct conventions Single canonical module with the interface inline in Context.Service (no standalone *Shape type) and make/layer exports instead of the *Live suffix, per the Effect service conventions check. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ts and bad data - thread.reverted now recomputes the in-memory latestUserMessageAt from the retained messages, matching the SQL pipeline: a revert that removes the newest user message must not block settle/snooze on a message that no longer exists or backdate settledAt to reverted-away work. - A malformed user-message createdAt no longer poisons the decider's newest-message reduction (Math.max with NaN), which would have silently disabled the queued-turn settle guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… settings reads - A cached status with no PR no longer settles a quiet thread on the inactivity path: the cache may predate the PR being opened, so it now maps to "unknown" and requires the same live verification as a cached "open". Only a live lookup (or a branch mismatch, which a lookup cannot change) may conclude there is no gating PR. - A failed settings read now disables the sweep instead of falling back to the 3-day default, so a user with auto-settle turned off cannot get inactivity settles during a read failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hority - A cached "closed" no longer settles directly: a closed PR can be reopened, so it maps to closed-cached and gets the same cooldown-limited live verification as cached "open" before the settle fires. Only "merged" is terminal enough to trust from cache. - peekChangeRequestState now requires the cached PR's headRef to match the thread's branch: the broadcaster caches local and remote halves independently, so after a branch switch the merged view can pair the new refName with the previous branch's PR — never settle on another branch's merge. - threadLastActivityAt clamps future-stamped candidates to the sweep's clock, so a skewed client createdAt can no longer hold a thread "fresh" forever and block inactivity settle (mirrors the decider's settledAt clamp). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Main's new chat-header action menu (#5592) shipped against the old client-side effectiveSettled and the removed sidebarAutoSettleAfterDays client setting. The menu now reads the settledOverride like every other surface, and the ChangeRequestStateLike prop threading through ChatView → ChatHeader → useThreadActionMenu is gone — PR state no longer feeds settled classification anywhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
72f1b2f to
b132fb0
Compare
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.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 lifecycle and list behavior across web, mobile, and orchestration; incorrect sweep or PR verification could hide active work or settle threads users still care about.
Overview
Settled threads are now decided on the server, not re-derived on each client from inactivity, PR state, and ticking clocks. Clients read
settledOverride(with small guards for pending work and messages newer thansettledAt).A new
ThreadAutoSettleReactorruns about once a minute: it evaluates threads withresolveAutoSettleVerdict/autoSettle.ts(inactivity window, merged/closed PR, open-PR blocks, snooze/pin/session guards) and dispatchesthread.settle. PR checks useVcsStatusBroadcaster.peekStatusplus cooldown-limited liverefreshStatuswhen background policy allows.threadAutoSettleAfterDaysmoves from client settings toServerSettings; Beta UI writes server settings and is gated onthreadAutoSettle.settledAtis derived from last activity on settle (not command time).latestUserMessageAtis projected for decider/sweep logic. Web/mobile drop PR lift-up,useNowMinute, and client-sideeffectiveSettledwindow/PR logic; list v2 partitions useeffectiveSettled(thread)and sort settled rows bysettledAt.Reviewed by Cursor Bugbot for commit b132fb0. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Move thread auto-settle from client-side inference to server-authored settled state
ThreadAutoSettleReactoron the server that periodically sweeps unsettled threads and dispatchesthread.settlebased on inactivity and PR state, replacing client-side clock/PR-state-based settlement logic.thread.settledecider now derivessettledAtfrom the thread's last recorded activity (viathreadLastActivityAt) rather than command time, preventing auto-settled threads from floating to the top of the settled shelf.effectiveSettledinclient-runtimeis simplified to reflect only server-authoredsettledOverride, droppingautoSettleAfterDays,changeRequestState, and wall-clock inputs; a guard keeps a thread active iflatestUserMessageAtis newer thansettledAt.effectiveSettled(thread)with no options.threadAutoSettleAfterDaysmoves from client settings to server settings (default 3, null to disable), exposed inServerSettingsand gated in the UI behind a newthreadAutoSettleserver capability flag.settledAt(falling back toupdatedAt) rather thanlatestUserMessageAt; threads settled before this deploy will havesettledAtequal to command time rather than last activity.Macroscope summarized b132fb0.
Summary by CodeRabbit
New Features
Improvements