feat(desktop): support manual context compaction in Side Conversations - #4532
Conversation
5f3797a to
d0515a3
Compare
me2seeks
left a comment
There was a problem hiding this comment.
Review
Method: traced the compaction flow end to end — panel → dispatchQuoteCompanionInput → useQuoteCompanion.compact() → the Workbar port → the existing sessions.compact IPC (no new protocol surface) — and followed contextCompactionOutcome through runtime-kernel → canonical-turn-snapshot → session-projector → applyOwnedEvent for all three terminal states (completed / failed / interrupted). CI test is green on this head; the branch merges cleanly into main.
The projector hunk is the right root-cause fix: the kernel's live complete event already carried the outcome, and only the snapshot-synthesized path dropped it — which is exactly the path an asynchronous Side Conversation compaction is observed through. The new test pins it.
Findings
P2 — The async compaction exclusivity lock leaks when the compaction turn terminates without an outcome. use-quote-companion.ts:289-298 clears compactionRequestInFlightRef only on a complete event carrying contextCompactionOutcome. Two plausible paths never produce that event: the Host restarting mid-compaction classifies the run as interrupted → the projector synthesizes abort; a kernel-level failure of the compaction run itself → error. Neither carries an outcome (canonical-turn-snapshot.ts reads the outcome only for completed runs). When that happens, compactionRequestInFlightRef stays true forever: send() (:797) and compact() (:750) refuse permanently, and the panel only recovers by closing it. The existing tests cover an RPC throw and a finished result, but not a terminal error/abort for the compaction turn. Smallest sound fix: in applyOwnedEvent, also match terminal error/abort events against compactionTurnIdRef.current, clear both refs, and report through onContextCompactionError; add a regression test for "Host interruption settles an async companion compaction and releases the lock".
P3 — Refused /compact is silent. dispatchQuoteCompanionInput intercepts the exact command before the streaming/steer split, and compact() returns false when the fork is running, preparing, or archived — the draft survives (good), but the user gets no signal why nothing happened. A localized toast on the refusal paths would close the loop.
P3 — Narrow double-/compact race. If the first compaction's terminal event arrives before its RPC resolves and a second /compact fires inside that window, the first call's continuation can overwrite compactionTurnIdRef and swallow the second compaction's outcome presentation. Consequence is a missing toast, not stuck state; noting for the record.
Production code that can be deleted
None identified — the feature rides the existing Session-scoped compaction capability and IPC channel; the helpers stay feature-private (d0515a3).
Low-quality tests to delete or replace
None identified. The new tests assert behavior: dispatch order before steering, exclusivity until the terminal event, retry after failure, and rejection without consuming staged quotes.
Review-relevant risks
User-visible behavior change (a new command path in Side Conversations) and an additive field on a synthesized Session event in the Runtime Host projection contract. Per CONTRIBUTING.md, material changes in protected areas require independent human review; this automated review is not an approval. Desktop-only scope is documented; CLI/TUI /compact is unchanged.
Conclusion
- The solution is optimal for the actual problem: existing capability, existing channel, root-cause projector fix.
- Deletable production code: none identified.
- Deletable/replaceable tests: none identified.
- Deeper refactor: not required.
- Merge readiness: after the P2 lock-leak is fixed (small, localized) or an explicit plan exists; everything else is ready.
- Residual risks: compaction availability remains subject to the backend capability checks already enforced by the Host; that is inherited, not introduced.
d24c02f to
d2d6d58
Compare
|
Thanks for the review. I rebased the PR onto the current
The focused Desktop Side Conversation suite passes 38/38, the Runtime Host projector suite passes 20/20, and the PR CI has been restarted on this rebased head. The remaining silent-refusal note is non-blocking and unchanged. |
d2d6d58 to
869b3f8
Compare
Add exact /compact handling for idle committed Side Conversations, routing compaction to the companion Session and preserving existing feedback and draft ownership. Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
869b3f8 to
967dac9
Compare
me2seeks
left a comment
There was a problem hiding this comment.
Re-reviewed the rebased head (967dac9) against the current main. Verified end to end:
- P2 lock-leak (previous review) — fixed.
applyOwnedEventnow treats terminalabortand non-recoverableerrorevents for the tracked compaction turnId as compaction terminals, releasing the in-flight fence and reporting throughonContextCompactionError. Regression test "releases an async companion compaction after a Host interruption" covers it. - P3 double-/compact race — fixed. Terminal events are fenced by the exact compaction
turnId; a Host terminal arriving before the compact RPC response is buffered inpendingCompactionTerminalRefand settled only after the response proves identity. Covered by "does not settle a pending companion compaction from another turn outcome". - Projector root-cause fix is minimal and correct. Snapshot-synthesized
completeevents now forwardcontextCompactionOutcome(packages/runtime-host/src/adapter/session-projector.ts), pinned by the new projector test; failed/interrupted turns surface through the pre-existingerror/abortbranches. - No new protocol surface — rides the existing
sessions:compactIPC; Workbar port, desktop adapter, fake services, and storybook bridge are consistent. - Guards in
compact()(mounted / archived / exact model choice / in-flight / submit lock / pending admission / active turn / runningTurnIds) plus thesend()interception keep draft, quotes, and attachments intact on every refusal path.
Local verification on this exact diff: runtime-host build clean + session-projector 20/20, desktop quote-companion suite 38/38, workbar-services-adapter 2/2, desktop typecheck clean, renderer architecture scanner clean, git diff --check clean. The earlier CI desktop-release-targets failure was environmental (node:test IPC deserialization against a spawned electron-builder child; passes locally under Node 24 and 26, and the lane is green on this head's CI).
Non-blocking notes, unchanged from the previous round: the companion compaction presentation duplicates the app-shell one deliberately (feature-private boundary), a refused /compact is silent (author acknowledged), and the failed-outcome toast uses the fallback copy rather than the Host reason — consistent with the existing shell semantics.
Summary
/compactcommand in idle, committed Side Conversations./compactto the model or touching the source Session.Implementation
completeevents preservecontextCompactionOutcome, allowing asynchronous Side Conversation compaction to settle and release its lock.Tests
git diff --check: passing.owned Host exits promptly after its first connection closesfailure; the complete Runtime Host suite rerun passed 1,567 / 12 skipped.Platform limitations
/compactbehavior is unchanged.Fixes #4521