Add session context compaction flow - #166
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a CompactionResult API and richer preserved-state handling, records compaction events with replay/rehydration that replaces compacted prefixes with a summary event, and integrates manual/provider and model-switch compaction flows and UI into the TUI with tests. ChangesAgent Compaction Metadata and State Preservation
Session Event Replay and Rehydration
TUI Model-Switch Compaction Gate and Session Compaction Command
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/tui/command_center.go (1)
155-159: 💤 Low valueUnused policy implementation flagged by linter.
The
noopModelSwitchCompactionPolicytype and itsBeforeModelSwitchmethod are unused. Consider one of:
- Remove if not needed
- Use in tests or production code
- Document its purpose with a comment if it's intentional for future use or external injection
🤖 Prompt for 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. In `@internal/tui/command_center.go` around lines 155 - 159, The noopModelSwitchCompactionPolicy type and its BeforeModelSwitch(modelSwitchCompactionRequest) modelSwitchCompactionDecision method are unused and should either be removed or documented/used; remove the noopModelSwitchCompactionPolicy declaration and its method if it's not required, or alternatively add a comment explaining it’s an intentional no-op for future injection or wire it into the codebase (e.g., use noopModelSwitchCompactionPolicy where a model switch compaction policy is required) or add tests that instantiate noopModelSwitchCompactionPolicy to justify keeping it.Source: Linters/SAST tools
🤖 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 `@internal/sessions/replay.go`:
- Around line 181-187: RecordCompaction currently permits an empty
input.Plan.SessionID which lets stray compaction plans be recorded; update the
validation in RecordCompaction to require that input.Plan.SessionID is non-empty
and exactly equals the provided sessionID (i.e., reject when
strings.TrimSpace(input.Plan.SessionID) == "" or when it != sessionID) and
return a clear error (referencing RecordCompaction and input.Plan.SessionID)
when the check fails.
- Around line 230-243: RehydrateEvents currently decodes every EventCompaction
in the events slice but only keeps the last result; change it to find the last
compaction event first and decode only that one to avoid failing on malformed
historical payloads: in function RehydrateEvents, instead of decoding inside the
forward loop, scan events from the end (or track the index of the last
EventCompaction) to locate the final compaction event and then call
decodeCompactionPayload just once for that event, assigning index and payload
only from that single decode.
---
Nitpick comments:
In `@internal/tui/command_center.go`:
- Around line 155-159: The noopModelSwitchCompactionPolicy type and its
BeforeModelSwitch(modelSwitchCompactionRequest) modelSwitchCompactionDecision
method are unused and should either be removed or documented/used; remove the
noopModelSwitchCompactionPolicy declaration and its method if it's not required,
or alternatively add a comment explaining it’s an intentional no-op for future
injection or wire it into the codebase (e.g., use
noopModelSwitchCompactionPolicy where a model switch compaction policy is
required) or add tests that instantiate noopModelSwitchCompactionPolicy to
justify keeping it.
🪄 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: 99a93d00-d203-4b52-aad6-e8b69c4067aa
📒 Files selected for processing (14)
internal/agent/compaction.gointernal/agent/compaction_metadata_test.gointernal/agent/compaction_preserve.gointernal/agent/compaction_preserve_test.gointernal/sessions/exec_session.gointernal/sessions/replay.gointernal/sessions/replay_test.gointernal/tui/command_center.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/options.gointernal/tui/session.gointernal/tui/session_controls.gointernal/tui/session_controls_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/model.go (1)
1454-1461:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
compactInFlightguard allows queued prompts to launch during compaction.
launchQueuedMessageIfReadychecksm.pendingand other modal states but notm.compactInFlight. If a prompt is queued while a run is active, then the run finishes and/compactis invoked, the queued message will launch during compaction whenagentResponseMsgcalls this function at line 786. This bypasses the guard at lines 1190–1196 and could corrupt compaction state or session/transcript snapshots.🔒 Proposed fix to add compactInFlight check
func (m model) launchQueuedMessageIfReady() (model, tea.Cmd) { - if !m.hasQueuedMessage() || m.pending || m.exiting || m.pendingPermission != nil || m.pendingAskUser != nil || m.pendingSpecReview != nil { + if !m.hasQueuedMessage() || m.pending || m.compactInFlight || m.exiting || m.pendingPermission != nil || m.pendingAskUser != nil || m.pendingSpecReview != nil { return m, nil }🤖 Prompt for 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. In `@internal/tui/model.go` around lines 1454 - 1461, The launchQueuedMessageIfReady function lacks a guard for compaction and can start queued prompts during a compactInFlight; update launchQueuedMessageIfReady to include m.compactInFlight in its initial conditional (alongside m.pending, m.exiting, m.pendingPermission, m.pendingAskUser, m.pendingSpecReview) so it returns without launching when a compaction is in flight, preventing queued prompts from running during compaction and corrupting compaction/session state.
🤖 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.
Outside diff comments:
In `@internal/tui/model.go`:
- Around line 1454-1461: The launchQueuedMessageIfReady function lacks a guard
for compaction and can start queued prompts during a compactInFlight; update
launchQueuedMessageIfReady to include m.compactInFlight in its initial
conditional (alongside m.pending, m.exiting, m.pendingPermission,
m.pendingAskUser, m.pendingSpecReview) so it returns without launching when a
compaction is in flight, preventing queued prompts from running during
compaction and corrupting compaction/session state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e2b16134-3580-475b-a0a4-c0a9352580d1
📒 Files selected for processing (3)
internal/tui/model.gointernal/tui/session_controls.gointernal/tui/session_controls_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tui/session_controls.go
- internal/tui/session_controls_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/session_controls.go (1)
228-232:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPropagate the cleared session snapshot when reload fails after compaction.
Once Line 489 records the compaction, the persisted session is already mutated. If Line 498 then fails,
compactActiveSessionclearsm.sessionEventsin its local copy, but Line 231 drops that snapshot and returns only the error. The live model can then keep pre-compaction context in memory and re-send history that was just compacted away. This is the same persisted-vs-in-memory contract thathandleRewindCommandexplicitly protects earlier in this file.Possible fix
return func() tea.Msg { next, result, err := m.compactActiveSession() if err != nil { - return compactResultMsg{err: err} + return compactResultMsg{ + err: err, + activeSession: next.activeSession, + sessionEvents: append([]sessions.Event{}, next.sessionEvents...), + transcript: append([]transcriptRow{}, next.transcript...), + hasSessionSnapshot: true, + } } return compactResultMsg{ result: result, activeSession: next.activeSession,Also applies to: 498-501
🤖 Prompt for 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. In `@internal/tui/session_controls.go` around lines 228 - 232, compactActiveSession currently clears the in-memory m.sessionEvents but when it returns an error the code at the anonymous function (return func() tea.Msg) drops that cleared snapshot and only returns compactResultMsg{err: err}, leaving the live model with stale events; modify the contract so compactActiveSession (and callers at the other failing-return sites noted) returns the cleared session snapshot and extend compactResultMsg to carry that snapshot (e.g., a sessionEvents or sessionSnapshot field) and populate it even on error, then ensure the caller applies that snapshot to m.sessionEvents when handling the compactResultMsg (similar to handleRewindCommand's persisted-vs-in-memory update).
🤖 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 `@internal/tui/session_controls.go`:
- Around line 391-395: Update the UI message returned by the function that
builds the compressing session text (the return that currently joins
"Compressing session", "Keep typing - messages will queue and send after
compression finishes.", and m.compactAnimationLine()) to accurately reflect that
submissions are blocked during compaction; replace the second line with copy
such as "You can keep editing your draft; submissions are disabled until
compression finishes." so the text matches the actual behavior (referencing the
same return and m.compactAnimationLine()).
---
Outside diff comments:
In `@internal/tui/session_controls.go`:
- Around line 228-232: compactActiveSession currently clears the in-memory
m.sessionEvents but when it returns an error the code at the anonymous function
(return func() tea.Msg) drops that cleared snapshot and only returns
compactResultMsg{err: err}, leaving the live model with stale events; modify the
contract so compactActiveSession (and callers at the other failing-return sites
noted) returns the cleared session snapshot and extend compactResultMsg to carry
that snapshot (e.g., a sessionEvents or sessionSnapshot field) and populate it
even on error, then ensure the caller applies that snapshot to m.sessionEvents
when handling the compactResultMsg (similar to handleRewindCommand's
persisted-vs-in-memory update).
🪄 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: dae57d8a-f5e6-4780-b1c5-a325e4332f9d
📒 Files selected for processing (3)
internal/tui/rendering.gointernal/tui/session_controls.gointernal/tui/session_controls_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/session_controls_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sessions/replay_test.go (1)
342-351: Confirm CompactionPayload “minimal” fields are sufficient forRehydrateEvents
RehydrateEventsonly requirespayload.Summaryto be non-empty (decodeCompactionPayload), andrehydrateEventsWithCompactiondecides what to skip usingpayload.CompactableEvents(byref.ID). It falls back topayload.CompactedThroughSequenceonly whenCompactableEventsis empty—so omitted fields likePreserveLast,PromptChars,Truncated,PreservedEvents, etc. won’t affect this test’s outcome.
Optional: add a test covering the fallback path by using a payload withCompactableEvents: []and a non-zeroCompactedThroughSequence.🤖 Prompt for 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. In `@internal/sessions/replay_test.go` around lines 342 - 351, The test's CompactionPayload can be minimal because RehydrateEvents (via decodeCompactionPayload) only requires payload.Summary to be non-empty and rehydrateEventsWithCompaction uses payload.CompactableEvents (by ref.ID) to decide what to skip; ensure the CompactionPayload in the test includes a non-empty Summary and the desired CompactableEvents (as in the existing entry) and you may optionally add a separate test case where CompactableEvents is empty and CompactedThroughSequence is set to exercise the fallback path used by rehydrateEventsWithCompaction.
🤖 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.
Nitpick comments:
In `@internal/sessions/replay_test.go`:
- Around line 342-351: The test's CompactionPayload can be minimal because
RehydrateEvents (via decodeCompactionPayload) only requires payload.Summary to
be non-empty and rehydrateEventsWithCompaction uses payload.CompactableEvents
(by ref.ID) to decide what to skip; ensure the CompactionPayload in the test
includes a non-empty Summary and the desired CompactableEvents (as in the
existing entry) and you may optionally add a separate test case where
CompactableEvents is empty and CompactedThroughSequence is set to exercise the
fallback path used by rehydrateEventsWithCompaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aeb20e88-34c9-4134-b098-e55731d7e5be
📒 Files selected for processing (4)
internal/sessions/replay.gointernal/sessions/replay_test.gointernal/tui/session_controls.gointernal/tui/session_controls_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sessions/replay.go
- internal/tui/session_controls_test.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Thanks for this — solid, well-tested feature, and the JSON-block approach to lossless preserved state is well-reasoned. Requesting changes on one correctness item that affects a core path (resume/fork); the rest are non-blocking suggestions.
Blocking
Resume/fork can now hard-fail where it previously couldn't
exec_session.go switches PrepareExec from ReadEvents → ReadRehydratedEvents (both the fork and resume paths). RehydrateEvents → decodeCompactionPayload returns an error on a malformed / empty-summary latest EventCompaction (replay.go), and that error propagates straight out of PrepareExec. So a single bad compaction event makes the entire session unresumable and unforkable — a failure mode ReadEvents never had.
RecordCompaction validates before writing, so it shouldn't happen on the happy path. But a torn tail (which the store already hardens against elsewhere — see b28a1e7 / ea2920a) or an event written by a different build would brick resume for that session. Please degrade gracefully: on decode failure, log and fall back to the raw events rather than failing the whole prepare. A regression test that resumes a session whose latest compaction payload is malformed would lock this in.
Non-blocking
Brittle cross-module string coupling (silent degradation)
loadedToolEntriesFromOutputkeys off the literal"Loaded "prefix +"Full schemas follow"substring of thetool_searchresult text.projectInstructionBlockkeys off"# "+" instructions for "+ the<INSTRUCTIONS>/</INSTRUCTIONS>tags.
These hard-code the rendered output format of other subsystems. If that wording/template ever changes, preservation silently stops working — and the tests won't catch it because they embed the same literals. Consider exporting these as shared constants from the producing packages (or documenting the contract) so a format change breaks at the source.
Only commandPrompt is blocked during in-flight compaction
handleSubmit blocks new prompts while compactInFlight, but /model, /mode, and /rewind still execute. When the background compactResultMsg lands it overwrites activeSession, sessionEvents, and transcript from its snapshot — which could clobber an intervening /rewind or reset a model-switch note. Consider blocking session-mutating commands while compacting, or guarding the snapshot-apply against an intervening session change.
Model-switch guard is a confusing half-feature
requestCompactionBeforeModelSwitch increments compactRequests, prints "Context compaction requested…", and aborts the switch — but never actually triggers compaction. The user must manually /compact then re-run /model. The message ("unchanged until compaction can run") implies something automatic will happen when nothing does. Either wire it to enqueue the actual compaction, or reword to make the manual step explicit.
Apparently-unused duplicate API
ReadReplayEvents and ReplayEvents are exact aliases of ReadRehydratedEvents / RehydrateEvents and don't appear to be called anywhere in this PR. If there's no caller, drop them.
Nits
projectInstructionBlockshadows the builtinclose— rename tocloseIdx.Compacted: len(events) < len(beforeEvents)+1is a fragile "did anything change" check; the plan already knows viaplan.CompactableCount— prefer deriving from that.- Two token estimators now coexist (
estimateTokensvsestimateTranscriptTokens); a one-line comment noting they're intentionally separate would help.
Tests & security
Coverage is strong and adversarial (malformed earlier payload, markdown round-trip, repeated-compaction carry-forward, TUI flows). Worth adding the two cases above: a malformed latest compaction payload through PrepareExec, and a /model or /rewind issued while compactInFlight. No new security exposure — preserved state is verbatim content that ReadEvents already returned, redaction still applies at display, and the summarizer prompt asks to omit secrets.
|
@gnanam1990 fixed the blocking resume/fork issue in What changed:
Validation:
Please re-review when you get a chance. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-reviewed after b9b740b. My blocking item — resume/fork hard-failing when the latest compaction payload is malformed — is resolved: readExecContextEvents now falls back to raw ReadEvents when rehydration fails, with a regression test covering both resume and fork. That's exactly the graceful degradation I asked for. CodeRabbit had already approved and CI is green, so lifting my requested changes.
A few non-blocking follow-ups remain (tracked in #173), none of which should hold up merge:
- String literals in
compaction_preserve.gocouple preserved-state parsing to other subsystems' rendered output (silent-degradation risk). ReadReplayEvents/ReplayEventsinreplay.goare unused aliases.projectInstructionBlockshadows the builtinclose.- The resume-fallback
log.Printfcan smudge the TUI frame; consider a quieter sink.
Nice work on the compaction flow. Approving.
2 participants
Summary
/compactin the TUI to perform real session compaction, using the active provider for a compact summary when available, and adds a conservative model-switch compaction guard.Why
Long Zero sessions already had automatic agent compaction, but manual/session replay compaction was still mostly a shell. This makes
/compactaffect the actual future context path instead of only reporting status.Validation
go test ./...go vet ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokeNotes
ReadEvents; prompt replay usesReadRehydratedEvents/ReadReplayEventsso compacted raw events are not re-sent./compactrecords only the returned summary; no-provider sessions use a deterministic fallback summary.Summary by CodeRabbit
New Features
Bug Fixes
Tests