refactor(server): settled state is now server-authored, ending client drift#5462

Layers/ThreadAutoSettleReactor.ts:217

This module owns construction of the only implementation, so the canonical export names are make and layer rather than makeThreadAutoSettleReactorLive / ThreadAutoSettleReactorLive (the *Live suffix is reserved for abstract-port modules with several implementations). Suggest exporting the existing constructor as make and adding export const layer = Layer.effect(ThreadAutoSettleReactor, make()), then updating server.ts and the test import.

No longer relevant as of f5ee144326035566857dea093aa1a5c6cd267497

Fixed in c514c22cc alongside the module consolidation: exports are now make (options-taking) and layer, with server.ts importing the namespace and the test building Layer.effect(ThreadAutoSettleReactor, make()).

The new service is split across Services/ThreadAutoSettleReactor.ts (tag + standalone ThreadAutoSettleReactorShape) and Layers/ThreadAutoSettleReactor.ts (construction + layer). Consider a single canonical module apps/server/src/orchestration/ThreadAutoSettleReactor.ts with the interface declared inline in the Context.Service call, and reference the inferred interface as ThreadAutoSettleReactor["Service"] (the ThreadAutoSettleReactorShape["sweepOnce"] / ["start"] / satisfies annotations in the layer file) instead of keeping a FooShape type.

^{Posted via Macroscope — Effect Service Conventions}

Keeping the Services/Layers split: it matches every other reactor in this package (CheckpointReactor, ThreadDeletionReactor, ProviderCommandReactor all have a Services/ tag+Shape module and a Layers/ implementation). A one-off consolidation here would make this the odd one out.

Reversed my earlier dismissal — the check clarified the convention distinguishes single-implementation modules from abstract ports (the sibling reactors' Services/Layers split predates it). Fixed in c514c22cc: one canonical module at orchestration/ThreadAutoSettleReactor.ts, interface inline in Context.Service, no standalone Shape type, references via ThreadAutoSettleReactor["Service"].

🟡 Medium state/threadSettled.ts:204

effectiveSettled now returns true for a thread that has settledOverride === "settled" but also a queued turn start — a user message no session has adopted yet. In the window between a new user message landing and the server's auto-unsettle projection arriving, the shell still carries the stale "settled" override, no running session, and no pending approval/input, so all three guards pass and the just-reactivated thread classifies as settled. The previous implementation detected this race via hasQueuedTurnStart and a settledAt-versus-latestUserMessageAt comparison to keep the thread visible until the server cleared the override. Consider restoring the queued-turn guard (with the settledAt adjudication) so a freshly sent message isn't hidden before auto-unsettle lands.

🟡 Medium Layers/ThreadAutoSettleReactor.ts:174

A thread whose cached PR state is "open" is never live-verified, so it stays active indefinitely even after the PR is actually merged or closed. peekChangeRequestState returns "open" from a stale cache, resolveAutoSettleVerdict returns skip for any "open" state, and the sweep immediately continues — only "unknown" states ever reach refreshStatus. A cold checkout whose cache still says "open" will never be re-checked, so a merged/closed PR never triggers the settle that "merged"/"closed" would. Consider live-verifying stale "open" states (or treating them like "unknown" after a cooldown) so a merged PR unblocks the settle.

Auto-settle settings ignore capability

Medium Severity

The new threadAutoSettle capability is documented as the gate for the auto-settle settings UI, but BetaSettingsPanel shows and writes threadAutoSettleAfterDays whenever sidebar v2 is on. On servers without the sweep, the control still appears and persists a no-op setting.

^{Reviewed by Cursor Bugbot for commit bfa8139a0d1dfd610a008172e5388685432d9029. Configure here.}

Fixed in bad152b9a: the settings rows now render only when the primary server reports capabilities.threadAutoSettle === true.

Stale open PR blocks settle forever

High Severity

Cached open PR state returns skip, so the sweep never enters the verify-pr path. After the first live lookup caches open, later merges on unwatched checkouts never re-verify despite the reactor’s cooldown comments promising settle within that window. Quiet branch threads can stay active indefinitely after merge.

^{Reviewed by Cursor Bugbot for commit bfa8139a0d1dfd610a008172e5388685432d9029. Configure here.}

📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Use the canonical Effect service module for ThreadAutoSettleReactor.

The service exports ThreadAutoSettleReactorShape from Services/ while its layer implementation lives in Layers/; move both into apps/server/src/orchestration/ThreadAutoSettleReactor.ts, inline the shape in Context.Service, expose ThreadAutoSettleReactor["Service"], make, and layer; and update dependent imports from Services/ and Layers/ to the canonical module.

Source: Pipeline failures

@t3dotgg, agreed. The Services/ and Layers/ split is consistent with the sibling reactors. This PR should not introduce a one-off module convention change. I am withdrawing this finding.

_{You are interacting with an AI system.}

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Refresh cached open PR state for inactive threads.

peekChangeRequestState returns "open" from cache. resolveAutoSettleVerdict then returns skip, so this code never calls refreshStatus for that thread.

If VCS polling has stopped and the PR later merges or closes, the thread remains active indefinitely. Treat cached "open" as unknown for an inactivity candidate, then use the existing cooldown, background-policy gate, and verification budget. Add a focused test where peekStatus returns open and refreshStatus returns merged or closed.

✅ Confirmed as addressed by @t3dotgg

Fixed in bad152b9a as suggested: cached "open" is treated as needing verification (a distinct open-cached state routed through the existing cooldown + background-policy gate + budget), with the focused test where peekStatus returns open and refreshStatus returns merged, plus the inverse where it stays open.

@t3dotgg, verified in bad152b9a.

"open-cached" reaches the existing verification path. The background-policy gate and verification budget remain in place. The focused tests cover both refreshed "merged" settlement and refreshed "open" retention.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

_{You are interacting with an AI system.}

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Preserve an existing disabled auto-settle preference during migration.

A client with persisted sidebarAutoSettleAfterDays: null loses that opt-out when this field is removed. The new server setting then decodes to 3, which enables auto-settlement after upgrade.

Add a one-time migration or define a safe server default that does not convert an explicit disable into enabled behavior. Add coverage for the persisted null case.

Dismissing as intended behavior: there is no canonical per-device value to migrate — the old setting lived independently in each browser's localStorage, the desktop app's file, and mobile hardcoded 3 regardless. A user could simultaneously have null on desktop and 3 in a browser against the same server, and mobile always auto-settled at 3 days. That ambiguity is what this PR removes. The server default of 3 matches what the majority of surfaces already did; users who had disabled it flip one switch in Settings → Beta (and un-settling any thread is one click). A migration would have to pick one arbitrary device's value to win, which is worse than a clean default.

@t3dotgg, agreed. The old value was per-device and could conflict across clients. It cannot provide a valid migration source for the new server-authoritative setting.

Using the three-day default is intentional and matches the existing behavior on most surfaces. I am withdrawing this finding.

_{You are interacting with an AI system.}

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Commit the threshold only after editing completes.

AutoSettleDaysInput calls onCommit for every valid keystroke. Line 114 persists each intermediate value to the server.

If a user changes 3 to 14, a pause after entering 1 can set the policy to one day. The reactor can settle eligible threads before the user enters the final value. Restoring 14 does not restore those threads.

Commit on blur or Enter after validating the full draft value.

✅ Confirmed as addressed by @t3dotgg

Fixed in bad152b9a: the input commits on blur/Enter only, with full-draft validation. Good catch — with the sweep being server-real, a transient "1" while typing "14" would have settled day-old threads irreversibly.

@t3dotgg, confirmed. AutoSettleDaysInput now validates the complete draft and updates threadAutoSettleAfterDays only on blur or Enter. This prevents transient values from reaching the server sweep.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

_{You are interacting with an AI system.}

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Keep auto-settle backdating server-only.

settledAt is now accepted from the public thread.settle command and stored unchanged. Any command caller can select a past, future, or malformed string because IsoDateTime is currently Schema.String. This can corrupt settled-shelf ordering.

• packages/contracts/src/orchestration.ts#L596-L600: remove settledAt from the client-callable thread.settle contract, or introduce a server-only auto-settle command.
• apps/server/src/orchestration/decider.ts#L500-L502: derive the backdated timestamp only from trusted server projection data.
• apps/server/src/orchestration/decider.settled.test.ts#L111-L130: test the trusted server path instead of accepting a caller-provided timestamp.

✅ Confirmed as addressed by @t3dotgg

Fixed in bad152b9a: the command no longer carries settledAt at all. The decider derives the stamp from trusted read-model data (last user message / turn timestamps), and the decider test now covers that derivation instead of a caller-provided timestamp.

@t3dotgg, confirmed. ThreadSettleCommand no longer accepts settledAt. The decider now owns timestamp derivation from trusted read-model activity. This resolves the client-controlled shelf-ordering risk.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

_{You are interacting with an AI system.}

🟡 Medium

https://github.com/pingdotgg/t3code/blob/bad152b9a6363edcebff43d97ee71fbe25a6b6c5/apps/server/src/orchestration/decider.ts#L160

thread.settle persists a future settledAt when the newest user message carries a client-supplied timestamp ahead of the server time. The threadHasQueuedTurnStart guard only blocks settle inside a ±2-minute grace window, so a message timestamp more than two minutes in the future makes that guard pass; the thread is then settled with threadLastActivityAt(thread, occurredAt), which returns the unclamped client timestamp, and that future value controls shelf ordering — placing the settled thread ahead of genuine recent work. Clamp candidate activity timestamps in threadLastActivityAt to occurredAt (or ignore future candidates) so a skewed or malicious client cannot forge shelf position.

Fixed in f5ee14432: settledAt candidates are clamped to the command time (parsed <= occurredAtMs), so a future-stamped client message can no longer forge shelf position — it falls through to the next-newest real candidate or the command time.

🟡 Medium orchestration/decider.ts:163

threadLastActivityAt omits thread.snoozedUntil from its candidates. When a snoozed thread's wake time is the most recent activity, thread.settle stamps settledAt with an older message or turn timestamp instead, placing the thread at the wrong position in the settled shelf ordering. Add thread.snoozedUntil to the candidates array so the wake time is considered.

Fixed in f5ee14432: an ELAPSED snooze wake now counts as a settledAt candidate (the same clamp keeps a still-pending wake out — a future wake is not activity). Matches the sweep's inactivity accounting on the reactor side.

SettledAt misses unhydrated messages

High Severity

thread.settle now derives settledAt from thread.messages and latestTurn, but the engine’s command read model bootstraps every thread with messages: [] and never reloads historical messages. Quiet threads after restart therefore lose user-message activity when stamping. An understated settledAt trips the new client guard (latestUserMessageAt newer than settledAt), so server-settled threads can stay in the active list.

^{Reviewed by Cursor Bugbot for commit bad152b9a6363edcebff43d97ee71fbe25a6b6c5. Configure here.}

Fixed in f5ee14432 — this was the sharpest catch of the round. The command read model indeed boots threads with messages: [] and never rehydrates, so after a restart the decider lost all user-message activity: the queued-turn invariant went blind and settledAt understated, which would trip the client's reactivation guard and strand server-settled threads in the active list. OrchestrationThread now carries the projected latestUserMessageAt stamp (maintained by the projector on message-sent, hydrated by both getSnapshot and getCommandReadModel), and the decider reads it alongside in-memory messages for both the invariant and the stamp.

Revert leaves stale activity stamp

Medium Severity

thread.reverted prunes in-memory messages and resets latestTurn, but never recomputes latestUserMessageAt. The engine decides against that in-memory model, while SQLite is refreshed correctly, so a same-session settle after revert can still see the pre-revert stamp. That can falsely trip the queued-turn settle guard or backdate settledAt to removed work.

^{Reviewed by Cursor Bugbot for commit f5ee144326035566857dea093aa1a5c6cd267497. Configure here.}

Fixed in 12b1dc681: the in-memory revert projection recomputes latestUserMessageAt from retained messages (the SQL side already did), so a same-session settle after revert no longer sees the pre-revert stamp — neither falsely tripping the queued-turn guard nor backdating settledAt to removed work. Covered by an assertion in the existing revert projector test.

Stale empty PR cache settles

Medium Severity

peekChangeRequestState treats a cached status with no PR as authoritative none, and the inactivity path settles without live verification. Only unknown and cached open re-verify. A warm cache that predates opening a PR can therefore inactivity-settle a thread that still has an open PR, which breaks the stated open-PR block.

^{Reviewed by Cursor Bugbot for commit 671e3465b231298b025183a65e60b8f97edc666b. Configure here.}

Fixed in 407c63ea3 (was 0c62906a3 pre-rebase): a cached status with no PR now maps to "unknown" instead of "none" — the cache may predate the PR being opened, so only a live lookup (or a branch mismatch, which a lookup cannot change) may conclude there is no gating PR. Added a test where peekStatus reports no PR but refreshStatus finds an open one: the thread stays active.

Settings failure enables settle

Low Severity

When getSettings fails, autoSettleAfterDays falls back to DEFAULT_THREAD_AUTO_SETTLE_AFTER_DAYS (3) instead of disabling the sweep. A user who set threadAutoSettleAfterDays to null can still get inactivity settles during a settings read failure.

^{Reviewed by Cursor Bugbot for commit 671e3465b231298b025183a65e60b8f97edc666b. Configure here.}

Fixed in 407c63ea3: a failed settings read now resolves autoSettleAfterDays to null (sweep disabled) instead of the 3-day default — hiding threads on a read error is not undone by the next successful sweep, so the failure mode must be inert. Covered by a test with a failing ServerSettingsService mock.

🟠 High orchestration/ThreadAutoSettleReactor.ts:112

peekChangeRequestState treats a cached closed PR state as final and returns "closed", which resolveAutoSettleVerdict immediately settles on. A closed PR can be reopened, so a stale cached closed causes the sweep to settle a thread whose PR is actually open again — hiding work behind a reopened PR without any live verification. Only merged is truly terminal; a cached closed should map to "unknown" (or "open-cached") so the verdict requests a live lookup before settling, the same way cached "open" is handled.

✅ Resolved in 72f1b2fab90273c1dc9de4e2b497d5637228774f

Fixed in 72f1b2fab: cached "closed" now maps to closed-cached, which the verdict routes through the same cooldown-limited live verification as cached "open" — a confirmed close still settles regardless of recency, but a reopened PR is caught before the thread is hidden. Only "merged" remains trusted from cache.

Confirmed—closed-cached correctly routes through live verification before settling, preventing stale cached close states from hiding threads whose PRs have been reopened.

Future timestamps block inactivity settle

Medium Severity

threadLastActivityAt treats client-supplied message/turn timestamps as activity even when they are ahead of server now. The inactivity check then always sees the thread as fresh, so a skewed createdAt can prevent server auto-settle indefinitely. Snooze wakes are already clamped with nowMs, and the decider clamps settledAt candidates the same way, but this path does not.

^{Reviewed by Cursor Bugbot for commit 407c63ea327370d8ecde6c40b9abb9b9437b167d. Configure here.}

Fixed in 72f1b2fab: threadLastActivityAt clamps candidates to the sweep's clock (Math.min with nowMs), and the window comparison applies the same clamp — a future-stamped createdAt counts as activity NOW, so the skew can no longer extend the freshness window indefinitely. Mirrors the decider's settledAt clamp. Test covers both sides: still active within a window of the sweep clock, settles once the sweep clock alone moves a full window past.

🟠 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.

Fixed in 72f1b2fab 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.

orchestration/ThreadAutoSettleReactor.test.ts:79

Reference the inferred service interface as VcsStatusBroadcaster["Service"][...] rather than Parameters<(typeof VcsStatusBroadcaster)["of"]>[0][...] — that is the shape used everywhere else in the server tests and it reads directly.

No longer relevant as of 2693edadb16e01d1b96d3ef838f08e6fe773fc8d

🟡 Medium orchestration/projector.ts:769

thread.reverted recomputes latestUserMessageAt from the capped messages array, so a revert that retains an older checkpoint but whose matching user message was already evicted (by the MAX_THREAD_MESSAGES cap) resets latestUserMessageAt to null. This drops the timestamp even though the reverted history still contains user activity, causing the decider's settle/auto-settle logic to compute timing from incomplete history until rehydration. Consider recomputing from the full pre-cap message set, or preserving the existing thread.latestUserMessageAt when no retained user message is found.

No longer relevant as of 5d03b32c8d36fda48bf5e2178aa0bf5fa0a05bab

Dismissing with reasoning — the null is harmless in exactly the eviction case described, and the suggested preservation would reintroduce a worse bug. When a retained turn's user message has been evicted by the message cap, that message is necessarily OLDER than its own turn's completion, and the revert keeps that turn's checkpoint — so latestTurn.completedAt ≥ the evicted message time. Every consumer of the stamp (the decider's settledAt derivation and last-activity computation, and the sweep's inactivity window) takes the max over the stamp AND the latestTurn timestamps, so a null stamp with an older true value never changes any outcome. The queued-turn guard is also unaffected: an evicted message is far outside its 2-minute grace window. Preserving the pre-revert stamp instead would resurrect the stale-stamp bug fixed earlier in this PR — a reverted-away NEWER message blocking settle/snooze on work that no longer exists.

Live PR verify skips headRef guard

Medium Severity

peekChangeRequestState refuses to act on a cached PR whose headRef does not match the thread branch (stale local/remote pairing after a branch switch), but verifyChangeRequestState returns status.pr?.state after only checking refName. refreshStatus loads local and remote halves concurrently, so a mid-refresh checkout change can still pair a new refName with the previous branch’s PR. That path can auto-settle a thread from another branch’s merge or close.

^{Reviewed by Cursor Bugbot for commit 9763b020cba93dc83ffa893278a41f70317b21c6. Configure here.}

Fixed in 5d03b32c8: the live path now applies the same headRef coherence guard as the cached path — a PR whose headRef is not the thread's branch reports "unknown" instead of settling (or blocking) on another branch's PR, and a later sweep retries.

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.

^{Reviewed by Cursor Bugbot for commit 8b8bec71c5446d3cd404f1a32bf0bef1d89cd895. Configure here.}

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.