fix(turn-change): aggregate per-turn changes across all assistants (#428)#435
Conversation
Display limit stays at 2 MB (changes block is not an IDE diff viewer); restore limit goes up so Word/PDF/PPT typical files remain undoable. Updates two existing test thresholds that used 2 MB+1 to trigger the oversize branch.
Verifies that with display limit at 2 MB and restore limit at 20 MB: - 3 MB file is restorable but not expandable - 20 MB+ file has restoreAvailable=false
…istants - TurnChange.aggregateTurn collapses every assistant in a user turn (parentID match) into one Display, merging same-path edits and dropping white-busy files - aggregateTurnUndo / aggregateTurnRedo apply per-message mutations under the session lock; default aborts on conflict, force=true skips conflicting messages and reports skipped[] - MutationResult/Schema gain optional skipped[] for partial-success reporting Closes the SessionTurn findLast bug surface where only the last assistant's changes were visible.
- GET /session/:id/turn/:userMessageID/changes returns the aggregated display
- POST .../undo and .../redo accept { force? } and reuse the session lock; on applied, broadcast file-changed events
- HTTP-level e2e covers happy path, conflict-blocked default, force=true skip behavior
… flow - SessionTurn drops the findLast lookup; aggregates per user-turn via the new turn-change endpoint and waits for every child assistant to complete before showing the action - Mark large+unrestorable files inline; show an "undone" badge and switch the action label to "Reapply" when the turn is undone - Add a conflict confirm dialog (Skip and apply / Cancel) before retrying with force=true; on applied-with-skipped, persist a yellow notice on the changes block - New i18n keys for unrestorable, undone, reapply, confirm dialog, and skipped notice (zh uses 爪印 voice: 不可撤回 / 已撤销 / 重新应用)
Two assistants editing the same file in one user turn (e.g. +161-126 then +31-20) used to undo as conflict because preflight checked every message's expected state against frozen real disk. Carry a per-path virtual state through the loop so each message's expected_current is matched against the prior step's target, exactly the scenario reported in #428.
- collapseRestoreFiles now keys by absolute path so two assistants writing different files with the same basename do not merge. - aggregateTurnInternal switches to "any-state" availability so a partially-undone turn keeps both undo and redo actionable. - mutateTurn no longer aborts when one actionable message fails; instead it appends to skipped[] and keeps going for the rest. - When all surviving display rows collapse to white-busy after partial mutate, synthesize an applied result with empty files. - Track the exact paths actually written and pass them to publishTurnChangeFiles so skipped files do not emit File events. Adds three regression tests covering the basename collision, mixed-state availability, and the mutatedPaths exclusion contract.
- mutateTurn now rolls back already-mutated assistants when force=false and a later actionable mutate fails. Previously a TOCTOU could leave the turn half-applied while the user saw a blocked toast. - turnChangeFetch catches network errors and JSON parse failures, so a thrown HTTP error in mutateTurnChange no longer becomes an unhandled rejection and the user sees the standard blocked toast instead. - Retry path for null-display fetch now stores its setTimeout in a per- key map cleared on cleanup, and keeps the cache key so a second event burst can no longer race a duplicate GET for the same turn. - Stop mutating the response body in place when attaching skippedCount; spread into a fresh display object before storing.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR implements server-side aggregation of turn-level file changes to resolve issue ChangesTurn-Level Change Aggregation & Undo/Redo
Sequence DiagramsequenceDiagram
participant User
participant Client as Client<br/>(MessageTimeline)
participant Fetch as Turn-Fetch<br/>Module
participant Server as Server<br/>(SessionRoutes)
participant Service as TurnChange<br/>Service
participant FileSystem as FileSystem
User->>Client: View session turn with multiple assistant messages
activate Client
Client->>Fetch: turnFetchSignature(assistants)<br/>→ stable signature from completed targets
Fetch->>Fetch: turnFetchTargets() groups by parentID,<br/>waits for all siblings completed
Fetch-->>Client: signature updated
Client->>Server: GET /session/:id/turn/:userMessageID/changes
activate Server
Server->>Service: aggregateTurn()
activate Service
Service->>Service: listAssistantsForUser()<br/>collapseRestoreFiles()
Service->>Service: Merge displays across all assistants<br/>for userMessageID
Service-->>Server: Aggregated TurnChangeDisplay
deactivate Service
Server-->>Client: Aggregated changes (all files, all assistants)
deactivate Server
Client->>Client: Render cumulative changes<br/>(no longer just last message)
deactivate Client
User->>Client: Click "Undo" on turn
activate Client
Client->>Server: POST /session/:id/turn/:userMessageID/changes/undo
activate Server
Server->>Service: aggregateTurnUndo()
activate Service
Service->>Service: preflightTurn()<br/>Check virtual file state, detect conflicts
alt No conflicts
Service->>Service: mutateTurn()<br/>Call undo for each ordered assistant
Service->>FileSystem: Remove applied changes
Service-->>Server: { status: "applied", mutatedPaths, skipped: [] }
else Conflicts detected (force=false)
Service-->>Server: { status: "blocked", reason: "conflict", skipped: [...] }
else Force applied (force=true)
Service->>Service: mutateTurn(force=true)<br/>Skip conflicted, undo safe changes
Service->>FileSystem: Remove non-conflicted changes
Service-->>Server: { status: "applied", mutatedPaths, skipped: [conflicts] }
end
deactivate Service
Server->>Server: publishTurnChangeFiles(display, mutatedPaths)<br/>Emit only affected paths
Server-->>Client: Mutation result with skipped/mutatedPaths
deactivate Server
Client->>Client: Update turnChanges state<br/>Recompute undoAvailable/redoAvailable<br/>Display skipped notices if any
deactivate Client
User->>User: Observes cumulative undo result<br/>with per-file conflict resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 36 minutes and 59 seconds.Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces aggregated turn changes, allowing users to undo or redo all assistant edits within a single user turn. It includes backend logic for preflight checks, conflict detection, and forced application, along with frontend updates to handle turn-level actions and display skipped files. A review comment points out that the rollback logic in mutateTurn silently suppresses errors during individual message mutations, which could lead to an inconsistent state.
…e parent Previously the cache key was sessionID:userMessageID, so once the first assistant under a turn completed and a GET fired, later siblings under the same parent would not trigger a re-fetch and the aggregated display silently dropped their changes. The signature now incorporates each assistant id + completedAt, so any new sibling completion produces a new key and triggers re-fetch. Logic extracted to turn-change-fetch.ts so the cache decision can be unit tested without SolidJS.
When mutateTurn aborts non-force, the per-message reverse rollback used to swallow blocked results via .catch(() => undefined). If a rollback itself failed (e.g. file changed externally between forward write and reverse), the disk was left half-rolled-back but the response carried the original failure's reason and files, hiding the inconsistency. mutateTurn now collects dirty paths from each blocked reverse mutate and returns reason "rollback_failed" with the affected files when any rollback step did not complete. UI maps the new reason to a dedicated toast that asks the user to inspect the listed files manually.
force-partial leaves a turn with both hasApplied and hasUndone. The single inline button continues in the original direction (undo) by design; the redo path for a mixed state is intentionally not exposed. Recovery UX for mixed state is tracked as follow-up.
Reverse mutate can return blocked with files: [] from the catch branch (unexpected throw) or from internal early returns like restore_missing. The previous loop iterated over empty files, leaving dirty[] empty, so the outer call returned the original blocked reason and hid the rollback failure entirely. Now a blocked reverse without file-level detail records a message-level dirty entry, ensuring the outer call returns rollback_failed and the UI warns the user about the partially-rolled-back message.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/opencode/test/session/turn-change-aggregate.test.ts (1)
48-564: 🏗️ Heavy liftMove this suite onto
testEffect(...)/it.live(...).These cases drive
TurnChange’s Effect services against the real filesystem, so they should use the repo’s Effect-aware test harness instead of plain Bun tests. That avoids ad hoc runtime setup and keeps cleanup semantics aligned with the rest of the test suite.As per coding guidelines, "Use
testEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows" and "Useit.live(...)when the test depends on real time, filesystem mtimes, child processes, git, locks, or other live OS behavior."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/session/turn-change-aggregate.test.ts` around lines 48 - 564, The test suite in turn-change-aggregate.test.ts exercises TurnChange's effectful filesystem behavior; wrap the affected describe blocks (those calling TurnChange.aggregateTurn, TurnChange.aggregateTurnUndo, TurnChange.aggregateTurnRedo, TurnChange.undo, and any filesystem setup/cleanup) with the repo's Effect-aware harness by converting tests to testEffect(...).it.live(...) (importing testEffect from test/lib/effect.ts) so they run under the Effect test harness and use it.live for real-OS behavior; update each test declaration to use testEffect(...) and replace plain async test(...) calls with it.live(...) (preserving the existing async bodies and assertions) and remove any ad-hoc lifecycle so cleanup follows the effect harness conventions.packages/opencode/test/server/turn-change-aggregate-routes.test.ts (1)
54-189: 🏗️ Heavy liftUse the Effect test harness for these live route cases.
This file exercises Effect-backed session/server flows with live filesystem and git state, so it should go through
testEffect(...)/it.live(...)instead of raw Buntest(...). That keeps runtime wiring and cleanup consistent with the rest ofpackages/opencode/test.As per coding guidelines, "Use
testEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows" and "Useit.live(...)when the test depends on real time, filesystem mtimes, child processes, git, locks, or other live OS behavior."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/server/turn-change-aggregate-routes.test.ts` around lines 54 - 189, The two tests use live filesystem/git and should be converted to the Effect test harness: replace Bun's test(...) with testEffect(...) or it.live(...) from test/lib/effect.ts, import the helper(s) at the top, and ensure the async test function body remains unchanged (including calls to resetDatabase, tmpdir, Instance.provide, SessionNs.create, makeUser, makeAssistant, TurnChange.recordWrite/finalize, and Server.Default().app and app.request). Update the test declarations for both "GET aggregates two assistant edits, undo applies and redo restores" and "POST undo without force is blocked on conflict; force=true applies and reports skipped" to use the effect harness so the runtime wiring/cleanup and live OS behavior (git, fs, child processes) are handled correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/src/pages/session/message-timeline.tsx`:
- Around line 475-478: The overflow label is hardcoded as
"+{conflictPaths.length - 6} more" in the Show block and must be localized;
update the JSX inside the Show that renders "+{conflictPaths.length - 6} more"
to call the translation function (e.g. language.t) with a key like
"session.conflicts.more" and pass the count (conflictPaths.length - 6) as a
variable so the string is translated and pluralized correctly; locate the Show
block around the conflictPaths usage in message-timeline.tsx (the div with class
"px-3 py-1.5 text-12-regular text-text-weak border-t border-border-base") and
replace the hardcoded text with language.t(...) referencing the new i18n key.
- Around line 329-333: The pending turn-change retry timers in
turnChangeRetryTimers are only cleared on unmount via onCleanup, so retries can
fire after sessionID() has changed and target the wrong session; modify the
logic that schedules retries to also clear all timers whenever sessionID()
changes (e.g., add a reactive watcher/createEffect that iterates
turnChangeRetryTimers.values(), calls clearTimeout on each, and then
turnChangeRetryTimers.clear()), and apply the same change to the other retry
scheduling block referenced (the code around the retry callback used at the
other location) so no stale timers remain when sessionID() switches.
In `@packages/opencode/src/session/turn-change.ts`:
- Around line 650-667: collapseRestoreFiles is merging RestoreRow.data by
basename only, leaving displayPath ambiguous when different absolute paths share
the same basename; after building merged (Map keyed by row.data.path) run a
second disambiguation pass to recompute unique displayPath values across the
entire returned RestoreFile[] using absolute-path uniqueness for the whole user
turn (i.e., call the same uniqueness algorithm used by nextDisplayPath but
seeded with absolute paths of all merged files) and set each
RestoreFile.displayPath accordingly; apply the same second-pass disambiguation
to the analogous aggregation function around lines 697-710 so turn-level Changes
lists are unambiguous.
- Around line 849-865: In preflightTurn (in
packages/opencode/src/session/turn-change.ts) the blocked array currently turns
all non-permission failures into a generic "conflict" when building skipped
entries; change the logic so that if any blocked item has reason
"restore_unavailable" (i.e., target state is non-restorable/oversized/binary)
you immediately treat the entire turn as fatal and return/emit an
unsupported_size result instead of collapsing to "conflict" or continuing to
process other messages; adjust the same handling in the analogous block around
the code referenced at lines 894-897 so mutateTurn()/aggregate undo-redo cannot
partially apply changes when a non-restorable file is present. Ensure you
reference the blocked array, messageID/skipped construction, and
preflightTurn/mutateTurn locations when applying the change.
---
Nitpick comments:
In `@packages/opencode/test/server/turn-change-aggregate-routes.test.ts`:
- Around line 54-189: The two tests use live filesystem/git and should be
converted to the Effect test harness: replace Bun's test(...) with
testEffect(...) or it.live(...) from test/lib/effect.ts, import the helper(s) at
the top, and ensure the async test function body remains unchanged (including
calls to resetDatabase, tmpdir, Instance.provide, SessionNs.create, makeUser,
makeAssistant, TurnChange.recordWrite/finalize, and Server.Default().app and
app.request). Update the test declarations for both "GET aggregates two
assistant edits, undo applies and redo restores" and "POST undo without force is
blocked on conflict; force=true applies and reports skipped" to use the effect
harness so the runtime wiring/cleanup and live OS behavior (git, fs, child
processes) are handled correctly.
In `@packages/opencode/test/session/turn-change-aggregate.test.ts`:
- Around line 48-564: The test suite in turn-change-aggregate.test.ts exercises
TurnChange's effectful filesystem behavior; wrap the affected describe blocks
(those calling TurnChange.aggregateTurn, TurnChange.aggregateTurnUndo,
TurnChange.aggregateTurnRedo, TurnChange.undo, and any filesystem setup/cleanup)
with the repo's Effect-aware harness by converting tests to
testEffect(...).it.live(...) (importing testEffect from test/lib/effect.ts) so
they run under the Effect test harness and use it.live for real-OS behavior;
update each test declaration to use testEffect(...) and replace plain async
test(...) calls with it.live(...) (preserving the existing async bodies and
assertions) and remove any ad-hoc lifecycle so cleanup follows the effect
harness conventions.
🪄 Autofix (Beta)
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: 2f1654ba-a58f-4db9-8aab-d4a8c9d2a148
📒 Files selected for processing (15)
packages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/turn-change-fetch.tspackages/app/test/session/turn-change-fetch.test.tspackages/opencode/src/server/instance/session.tspackages/opencode/src/session/turn-change.tspackages/opencode/test/server/turn-change-aggregate-routes.test.tspackages/opencode/test/session/turn-change-aggregate.test.tspackages/opencode/test/session/turn-change.test.tspackages/ui/src/components/session-turn-changes.tspackages/ui/src/components/session-turn.csspackages/ui/src/components/session-turn.tsxpackages/ui/src/i18n/en.tspackages/ui/src/i18n/zh.ts
- Cancel pending turn-change retry timers and clear fetched-turn cache when
the active session changes, so a stale 500 ms retry from the previous
session cannot land on a freshly opened one.
- Move the conflict dialog overflow label ("+N more") behind an i18n key
(`ui.sessionTurn.turnChanges.confirmListMore`) for both en and zh.
- Add a second-pass disambiguation over aggregated restore files so two
sibling assistants editing different opaque external paths that share a
basename do not collide on `path: "config.json"` in the turn display.
- Treat `restore_unavailable` as fatal `unsupported_size` in
`preflightTurn`, matching the single-message `mutate()` behavior so an
aggregated undo/redo with one oversized/binary file blocks the whole
turn instead of partially mutating other assistant messages with
force=true.
- Cover the two backend cases with regression tests in
`turn-change-aggregate.test.ts`.
9bcbc55 to
cdce256
Compare
Summary
parentID === userMessageIDinto one turn-level Changes block; the SessionTurn UI now shows the union of file edits with one Undo / Reapply button instead of dropping earlier rounds.forcesemantics (default abort on conflict,force=trueskips the conflicting message and reportsskipped[]).force=false, and UI error / retry resilience.Why
Issue #428: SessionTurn used
findLastover messages so any user prompt that triggered multiple assistant rounds (compaction, tool retries, multi-tool plans) would silently lose earlier file changes from the Changes block. Users could no longer review or undo most of what the agent had written.Related Issue
Closes #428.
Human Review Status
Pending. Two rounds of crosscheck (Claude Opus + Codex) drove the refinement; final merge call belongs to a human review.
Review Focus
packages/opencode/src/session/turn-change.ts—aggregateTurnInternal,preflightTurnvirtual disk chaining,mutateTurnrollback / skip semantics,mutatedPathscontract.packages/opencode/src/server/instance/session.ts— new/turn/:userMessageID/changes[/undo|/redo]routes andpublishTurnChangeFilesexclusion viamutatedPaths.packages/app/src/pages/session/message-timeline.tsx— fetch error path, retry timer cleanup, response body immutability.packages/ui/src/components/session-turn.tsx+session-turn.css+ i18n — aggregate rendering, conflict confirm dialog, skipped notice.Risk Notes
/turn-change/:messageID/{undo,redo}endpoints are unchanged for compatibility.skipped[]array; UI renders a yellow notice and only publishes File events for paths that were actually written.How To Verify
UI verification: started a session with two assistant rounds touching different files plus one shared file, observed the SessionTurn Changes block listing all three with one Undo. Triggered a conflict by tampering one file on disk; the confirm dialog appeared, force-undo applied the rest, the yellow skipped notice rendered.
Screenshots or Recordings
Not attached (manual verification only). The visual change reuses the existing Changes block layout; no new component or styling beyond the skipped-notice surface and the conflict confirm dialog.
Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style