Repository navigation
feat(acp): compact Grok, OpenCode and OMP chats - #26502
Conversation
|
Problem. In Grok, OpenCode and OMP chats, typing What changes for users. How. Orca sends Fixed during review. A stale Grok capability test ( Deferred. OpenCode's slash menu only lists commands the agent advertises, so Verified live.
Final head Limits.
Tests and CI.
QA incidents, kept for the record.
No main merge or PR merge was performed. |
…action # Conflicts: # src/main/acp/acp-dialects/omp-dialect.ts # src/main/acp/acp-launch-specs.ts # src/shared/structured-agent-session-status-block.ts
A /compact the agent refused for sign-in now records the same notSignedIn fact as an ordinary send, so the chat shows sign-in guidance and the next send starts a new agent that reads the new login.
📝 WalkthroughWalkthroughThe change adds Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to A refused /compact command can leave its chat turn unfinished. Settle that failure before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
…action # Conflicts: # src/main/acp/acp-dialects/omp-dialect.ts
…r main's growth Main's OMP per-channel message identity grew acp-timeline-translator.ts past max-lines with this PR's compaction handling. The running compaction's frame folding moves beside its ending in acp-compaction-turn.ts, and the compacting-prompt lookup into AcpPromptTurns; behaviour is unchanged.
Conflict in acp-structured-lane.ts: keeps main's child-work lane and AcpTimelineEvent type; re-adds only this PR's beginCommand/forgetCommand. Main's Grok subagent rows (#26508) left acp-timeline-translator.ts at its 300-line limit, so this PR's compaction hooks move out of it: the running compaction now opens, absorbs its frames and ends inside AcpPromptTurns (which takes the dialect and agent name instead of an injected flag), the translator's openPrompt takes the command turn instead of a separate openCompaction, and the substantive-update list moves to acp-session-update.ts. Behaviour is unchanged.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
8ad6b024-2a44-4a7f-a54b-1189d73b8573
📒 Files selected for processing (9)
src/main/acp/acp-compaction-turn.tssrc/main/acp/acp-dialects/acp-dialect.tssrc/main/acp/acp-prompt-turns.tssrc/main/acp/acp-session-update.tssrc/main/acp/acp-structured-agent-definitions.tssrc/main/acp/acp-structured-lane.tssrc/main/acp/acp-structured-session-adapter.tssrc/main/acp/acp-structured-turns.tssrc/main/acp/acp-timeline-translator.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| return [ | ||
| { | ||
| type: 'item.close', | ||
| item: `compaction:${turn}`, | ||
| body: { | ||
| kind: 'status', | ||
| tone: 'warning', | ||
| text, | ||
| presentation: AGENT_SESSION_COMPACTION_SKIPPED_PRESENTATION | ||
| }, | ||
| join | ||
| }, | ||
| end('success') | ||
| ] | ||
| } | ||
| const words = reply?.detail ?? input.failureDetail ?? compaction.failureDetail | ||
| const detail = words === undefined ? undefined : providerDiagnostic(words, 'person') | ||
| const verdict = structuredCompactionOutcome({ | ||
| // An agent that ends `/compact` normally compacted, unless its reply says otherwise. | ||
| compacted: stopReason === 'end_turn' && reply === undefined, | ||
| interruptRequested: stopReason === 'cancelled', | ||
| failed: detail ? { detail } : {} | ||
| }) | ||
| if (verdict.outcome === 'cancellation') { | ||
| return [end('cancellation')] | ||
| } | ||
| // The same fact a signed-out send records, so its guidance shows and the child is replaced. | ||
| const failure = | ||
| verdict.failure && input.notSignedIn | ||
| ? agentSessionFailureFact('notSignedIn', detail ? { detail } : {}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n 'structuredCompactionOutcome|failureDetail|absorbCompaction' src/main/acp src/shared/agent-session-compaction.ts
sed -n '1,180p' src/shared/agent-session-compaction.ts
sed -n '85,125p' src/main/acp/acp-compaction-turn.tsRepository: stablyai/orca
Length of output: 5519
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- outcome helper references ---'
rg -n -F --glob '*.ts' -- 'structuredCompactionOutcome' src test tests 2>/dev/null || true
printf '%s\n' '--- outcome helper source ---'
helper=$(rg -l -F --glob '*.ts' -- 'export function structuredCompactionOutcome' src)
printf '%s\n' "$helper"
sed -n '1,240p' "$helper"
printf '%s\n' '--- compaction turn source ---'
sed -n '1,190p' src/main/acp/acp-compaction-turn.ts
printf '%s\n' '--- ACP translator caller ---'
sed -n '90,145p' src/main/acp/acp-timeline-translator.ts
sed -n '215,320p' src/main/acp/acp-timeline-translator.ts
printf '%s\n' '--- prompt owner ---'
sed -n '1,130p' src/main/acp/acp-prompt-turns.ts
printf '%s\n' '--- focused tests ---'
rg -n -i --glob '*.{ts,tsx}' -- 'compaction|failureDetail|Context compacted|Nothing to compact|structuredCompactionOutcome' test tests src 2>/dev/null | head -240Repository: stablyai/orca
Length of output: 41958
🏁 Script executed:
printf '%s\n' '--- ACP translator frame path ---'
nl -ba src/main/acp/acp-timeline-translator.ts | sed -n '185,275p'
printf '%s\n' '--- ACP compaction settlement tests ---'
nl -ba src/main/acp/acp-structured-compaction.test.ts | sed -n '110,335p'
printf '%s\n' '--- dialect reply contracts and implementations ---'
nl -ba src/main/acp/acp-dialects/acp-dialect.ts | sed -n '45,105p'
rg -n -F --glob '*.ts' -- 'compactionReply' src/main/acp/acp-dialectsRepository: stablyai/orca
Length of output: 17865
Make extension failures override the default end_turn success.
When an ACP extension supplies failureDetail and the prompt ends with end_turn without a parsed reply, acpCompactionEnd passes both compacted: true and failed: { detail }. structuredCompactionOutcome checks compacted first, so the turn reports successful compaction instead of the failure.
Suggested fix
- compacted: stopReason === 'end_turn' && reply === undefined,
+ compacted: stopReason === 'end_turn' && reply === undefined && detail === undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return [ | |
| { | |
| type: 'item.close', | |
| item: `compaction:${turn}`, | |
| body: { | |
| kind: 'status', | |
| tone: 'warning', | |
| text, | |
| presentation: AGENT_SESSION_COMPACTION_SKIPPED_PRESENTATION | |
| }, | |
| join | |
| }, | |
| end('success') | |
| ] | |
| } | |
| const words = reply?.detail ?? input.failureDetail ?? compaction.failureDetail | |
| const detail = words === undefined ? undefined : providerDiagnostic(words, 'person') | |
| const verdict = structuredCompactionOutcome({ | |
| // An agent that ends `/compact` normally compacted, unless its reply says otherwise. | |
| compacted: stopReason === 'end_turn' && reply === undefined, | |
| interruptRequested: stopReason === 'cancelled', | |
| failed: detail ? { detail } : {} | |
| }) | |
| if (verdict.outcome === 'cancellation') { | |
| return [end('cancellation')] | |
| } | |
| // The same fact a signed-out send records, so its guidance shows and the child is replaced. | |
| const failure = | |
| verdict.failure && input.notSignedIn | |
| ? agentSessionFailureFact('notSignedIn', detail ? { detail } : {}) | |
| return [ | |
| { | |
| type: 'item.close', | |
| item: `compaction:${turn}`, | |
| body: { | |
| kind: 'status', | |
| tone: 'warning', | |
| text, | |
| presentation: AGENT_SESSION_COMPACTION_SKIPPED_PRESENTATION | |
| }, | |
| join | |
| }, | |
| end('success') | |
| ] | |
| } | |
| const words = reply?.detail ?? input.failureDetail ?? compaction.failureDetail | |
| const detail = words === undefined ? undefined : providerDiagnostic(words, 'person') | |
| const verdict = structuredCompactionOutcome({ | |
| // An agent that ends `/compact` normally compacted, unless its reply says otherwise. | |
| compacted: stopReason === 'end_turn' && reply === undefined && detail === undefined, | |
| interruptRequested: stopReason === 'cancelled', | |
| failed: detail ? { detail } : {} | |
| }) | |
| if (verdict.outcome === 'cancellation') { | |
| return [end('cancellation')] | |
| } | |
| // The same fact a signed-out send records, so its guidance shows and the child is replaced. | |
| const failure = | |
| verdict.failure && input.notSignedIn | |
| ? agentSessionFailureFact('notSignedIn', detail ? { detail } : {}) |
Conflict in acp-launch-specs.ts: keeps main's modelDiscovery on the Grok, OpenCode and OMP specs and re-adds only this PR's `compaction: true`. Main's model catalog work (#26407) left acp-structured-session-adapter.ts at its 300-line limit, so the adapter's `compact` now delegates in one expression and AcpStructuredTurns.compact returns the admission receipt itself; the adapter's two imports from acp-structured-connection are joined and releaseAcquisition drops a return annotation close() already gives. Behaviour is unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
e462d281-7e94-4531-8546-167e7e8fed4d
📒 Files selected for processing (4)
src/main/acp/acp-dialects/acp-dialect.tssrc/main/acp/acp-launch-specs.tssrc/main/acp/acp-structured-session-adapter.tssrc/main/acp/acp-structured-turns.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| lane.forgetCommand(command.turnId) | ||
| throw error | ||
| } | ||
| return { state: 'accepted', providerIdentity: null } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Settle a refused /compact command turn.
If the agent refuses the prompt before starting it, run calls reject. The command’s message ID is not in unsettled, so reject does nothing. The host has already received accepted admission, and this path emits no command result or turn end. Handle that refusal as a command failure so the turn can finish.
…utput rig Main's #26502 made Grok declare compaction; the rest-test rig adapter has no compact, so the registry refused it before any recovery case ran.
* fix: preserve streamed turn duration after host restart * test: run restart duration regression in Node * fix: preserve provider receipt time in journal updates * test: verify provider receipt in Claude ownership assertion * fix(native-chat): time interrupted turns from real provider output, not flushes Shared-timeline checkpoints stamped the coalescer flush or retry time as the provider's latest proof of life, so a turn whose provider died before a delayed flush settled with an inflated duration after a host restart. Carry each delta's receipt time through the coalescer and timeline lane. * fix(native-chat): derive interrupted-turn duration from saved output A reply cut off by a server restart could show "Worked for 0s" because recovery ended the turn at the last heartbeat or the reply's first timestamp. End it at the latest time this owner's output was saved to chat history instead, derived in the existing journal fold from the row save time. Drops the per-frame receipt plumbing through every provider and buffer. * fix(chat): exclude host recovery revisions from provider activity Mark journal-open and connection-cleanup revisions with the existing recovery annotation so an interrupted turn does not include host downtime. Preserve buffered provider output and advance restart fixtures to the restart clock. * fix(native-chat): interrupt turns a replaced server left running When the managed server died mid-reply and a new one started on the same host, the cut-off turn settled `unverifiable` with no end whenever the old provider's identity could not be proven gone, and nothing ever revised it: the chat showed no turn status and the sidebar said "unconfirmed". The new server opens the chat records holding the host's instance lock, so every runtime that wrote them before is gone, and every provider it held lost its pipes with it. Record that at load (the fences those runtimes granted, the loaded owner's last renewal, how its runtime ended) and let the stale settle end those runtimes' running or unverifiable turns as interrupted at the owner's last saved output or renewal, with the existing Orca-stop row. Lease adjudication and the live / unverifiable / exited owner verdicts are unchanged. * fix(native-chat): require the record store for any journal open that settles Only an acquisition's open may skip it; a reader open without the store would leave a replaced server's turns unverifiable. * fix(native-chat): say a remote host's Orca restarted, not that it was closed A reply cut off when a remote host's Orca server stopped read "Orca on <host> was closed while this response was in progress." Nobody closed anything there: the server stopped and Orca started it again before the row could show. For a chat on a remote environment, cause quit now reads "Orca on <host> restarted while this response was in progress." The desktop's own quit keeps "was closed". Renderer-only; no wire change. * fix(native-chat): say a remote host's Orca stopped, true for a shared desktop too * test(native-chat): register Grok without compaction in the recovery-output rig Main's #26502 made Grok declare compaction; the rest-test rig adapter has no compact, so the registry refused it before any recovery case ran. * chore(native-chat): say a replaced runtime also revises an unverifiable turn
* fix: preserve streamed turn duration after host restart * test: run restart duration regression in Node * fix: preserve provider receipt time in journal updates * test: verify provider receipt in Claude ownership assertion * fix(native-chat): time interrupted turns from real provider output, not flushes Shared-timeline checkpoints stamped the coalescer flush or retry time as the provider's latest proof of life, so a turn whose provider died before a delayed flush settled with an inflated duration after a host restart. Carry each delta's receipt time through the coalescer and timeline lane. * fix(native-chat): derive interrupted-turn duration from saved output A reply cut off by a server restart could show "Worked for 0s" because recovery ended the turn at the last heartbeat or the reply's first timestamp. End it at the latest time this owner's output was saved to chat history instead, derived in the existing journal fold from the row save time. Drops the per-frame receipt plumbing through every provider and buffer. * fix(chat): exclude host recovery revisions from provider activity Mark journal-open and connection-cleanup revisions with the existing recovery annotation so an interrupted turn does not include host downtime. Preserve buffered provider output and advance restart fixtures to the restart clock. * fix(native-chat): interrupt turns a replaced server left running When the managed server died mid-reply and a new one started on the same host, the cut-off turn settled `unverifiable` with no end whenever the old provider's identity could not be proven gone, and nothing ever revised it: the chat showed no turn status and the sidebar said "unconfirmed". The new server opens the chat records holding the host's instance lock, so every runtime that wrote them before is gone, and every provider it held lost its pipes with it. Record that at load (the fences those runtimes granted, the loaded owner's last renewal, how its runtime ended) and let the stale settle end those runtimes' running or unverifiable turns as interrupted at the owner's last saved output or renewal, with the existing Orca-stop row. Lease adjudication and the live / unverifiable / exited owner verdicts are unchanged. * fix(native-chat): require the record store for any journal open that settles Only an acquisition's open may skip it; a reader open without the store would leave a replaced server's turns unverifiable. * fix(native-chat): say a remote host's Orca restarted, not that it was closed A reply cut off when a remote host's Orca server stopped read "Orca on <host> was closed while this response was in progress." Nobody closed anything there: the server stopped and Orca started it again before the row could show. For a chat on a remote environment, cause quit now reads "Orca on <host> restarted while this response was in progress." The desktop's own quit keeps "was closed". Renderer-only; no wire change. * fix(native-chat): say a remote host's Orca stopped, true for a shared desktop too * test(native-chat): register Grok without compaction in the recovery-output rig Main's #26502 made Grok declare compaction; the rest-test rig adapter has no compact, so the registry refused it before any recovery case ran. * chore(native-chat): say a replaced runtime also revises an unverifiable turn (cherry picked from commit b44a579)
1 participant
ELI5
Grok, OpenCode and OMP chats could not run their native
/compactcommand through Orca. During review, an OMP conversation that was too short to compact also appeared to finish with an empty “Worked for 0s” turn: the warning existed, but the transcript folded it away.You can now compact these chats and keep chatting. Successful compaction shows “Context compacted.” OMP’s “Nothing to compact” and “Already compacted” outcomes show their actual warning reason, including after reopening or paging through history.
What Changed
Orca sends
/compactas an ordinary prompt through the existing Agent Client Protocol connection, inside the command turn the chat already owns. Its response settles that turn through the existing queue, Stop and process-exit handling. Compaction reply text becomes one outcome row; tool and context updates still flow through.Skipped outcomes carry their own presentation hint, so visibility does not depend on loading the original command on the same history page. Previously saved OMP outcomes derive that hint from their own reserved identity. Ordinary warnings keep their existing folding behavior. A skipped command does not reset context usage or claim successful compaction.
Why
Invoking native compaction through an ordinary prompt follows the common pattern. Reusing the command’s existing owner avoids another scheduler or a persistent flag. Giving the outcome its own meaning removes the page-boundary failure instead of exempting warnings whenever a nearby command is found.
Differences from the common pattern:
/compactmust be typed. Follow-up: include the host-supported command in that menu; no separate ticket exists.Linked Issue
Follow-up to #25845 and #26401; no separate issue.
Visual Proof
Earlier laptop evidence on
537a7b45003: Grok and OpenCode each show “Context compacted.” These screenshots predate the OMP visibility correction; their follow-ups were not captured.Before the OMP correction, the installed 17.0.5 agent’s skipped command appeared as an empty collapsed “Worked for 0s” turn:
On final head
cb055619931, OMP 17.0.5 now shows its skipped reason with the work turn still collapsed:OMP success and follow-up, rendered on a local integration build (
403242994a66): this PR's headcb055619931merged with draft #26658's head3f52e6fe00ain a disposable QA checkout, never pushed. #26658 was needed because ordinary OMP replies could vanish when reasoning and answer share an id; its branch also carries its own main ancestry. So this is not standalone evidence for this PR, and this PR does not fix ordinary reply delivery. In one OMP 17.0.5 chat opened from New tab, six long chapter prompts were sent, then/compactonce, then one follow-up:The saved journal records one
/compactsubmission, one “Context compacted” outcome and the command turn completed as success; the follow-up answered “blue cedar”. Limits: OMP 17.0.5 answered/compactin about 100 ms and the next turn's context estimate was slightly higher (48.3k to 52.2k), the same shape as the native 17.0.5 success recording (instant, token increase, real saved entry); OMP's own session file was not inspected. The model also repeated the marker inside chapter 6, so the follow-up shows the chat stays usable after compaction rather than proving chapter-1 memory survived it.Final head
9b579925d7b. After QA, main was merged once (754b21c0cfd, three conflicts resolved by keeping main's code and re-adding only this PR's compaction lines). A fresh review of that merge found that main's new sign-in handling did not reach/compact: a signed-out/compactshowed "Compaction failed: Authentication required", and main's restart of a signed-out agent never fired, so/compactcould keep failing after signing in.059ccbde009records the same "not signed in" result an ordinary message records, so the sign-in guidance shows and the restart happens. Then main was merged again (bfa8c848626, after #26658 and #26666 landed): the one conflict kept main's new shared prompt-error rule and re-added only this PR's compaction line, and052f3656f78moved this PR's compaction handling out of the timeline translator intoacp-compaction-turn.tsbecause main's per-channel reply identity pushed that file over the line limit (same behaviour, reviewed). A third main merge (f87dd3b6570, after #26508 Grok subagents) kept main's subagent handling and moved this PR's compaction bookkeeping intoacp-prompt-turns.tsto stay under the translator's line limit (same behaviour, reviewed);996ea5018a8merges current main once more with no conflicts. A fourth main merge (9b579925d7b, after #26587 and #26407) kept main's model-discovery settings, re-added onlycompaction: true, and madecompactreturn its receipt from the turn runner to keep the session adapter under its line limit (same behaviour, reviewed). The live screenshots above were taken before these commits; the later changes are covered by tests, not live QA.Testing
Real native recordings cover OMP 17.0.5 and 18.2.7: two short prompts and a fresh chat both return the skipped reason, while sufficient default conversation history produces an actual saved compaction entry. Version 17.0.5 reported a 548-token increase; these results establish completion, not guaranteed token savings. Version 18.2.7 used a checksum-verified isolated official binary without replacing the installed version.
On the prepared final code, 211 targeted tests passed. A fresh reviewer independently passed 177 tests, including the actual 200-item history reader with the command omitted, older-page loading, saved-journal reload, turn ownership, ordinary-warning controls and usage preservation. Queued Node, renderer and CLI typechecks passed. Immediate prepush static checks passed 22/22. Earlier draft CI on
cb055619931: 32 checks, 15 passed, 17 skipped, 0 failed. CI on final head9b579925d7b(runs 37905775276, 37905775284, 37905775301, 37905775712): all 32 checks finished, 17 passed, 15 skipped by workflow rules, 0 failed. Earlier ready-for-review CI on059ccbde009(run): 35 checks, 17 passed, 18 skipped, 0 failed after one rerun of a flaky remote-server file-permission test this PR does not touch. Skipped jobs provide no coverage: Git compatibility, Codex index-heal contract, xterm patch sync, shell contracts, orcad browser provider, managed hooks on Node 18, e2e, real IME, mobile web app bundle, real WSL terminal, unit_selection_evidence, test, four headless Linux server jobs, headless persistence and mobile tests on main.Earlier lifecycle review passed 76 tests and four temporary full-host scenarios: queued-message handover, acknowledged Stop, bounded recovery from hanging cancellation, and process exit followed by another message. Temporary tests were archived outside the PR and removed.
Windows, Linux, SSH, mobile and older peers were checked in source review, not exercised live. No new journal kind, field, protocol operation or migration was introduced; the existing open presentation string carries the hint. Opaque legacy identities are conservatively left unchanged. Two synthetic late-event sequences remain unproven with real providers. Grok and OpenCode compaction screenshots precede the OMP visibility correction; their post-compaction follow-ups were not verified.
Review
Fresh architecture and lifecycle reviews covered the original command implementation. Review fixed a stale Grok capability assertion and the OMP warning visibility failure. A fresh final review and the readiness checklist without counsel accepted the outcome-owned design and exact source hashes. No main merge or PR merge was performed.
Agent skill upstream boundary
Checklist