Repository navigation
fix(acp): keep OMP replies that follow its reasoning - #26658
Conversation
|
Problem. OMP gives a reply's reasoning and its answer the same message identifier. Orca put both into one text row, threw the answer away when the row switched from reasoning to answer, and still marked the turn successful, so the user saw "Worked for Ns" with no answer. What changes for the user. OMP answers that follow its reasoning now appear in the chat under the "Reasoning" row and stay in the saved chat. Nothing new is shown on screen. What was fixed. Reasoning and answer get separate text rows, while both stay with the turn that first produced their message. Late text for a message whose turn already finished is refused instead of leaking into the next prompt. Refused text produces a main-process log line with identifiers only. A fresh adversarial review found that splitting the rows alone could let a late answer move to the next prompt; that was fixed by keeping turn ownership per provider message. Fresh architecture and implementation reviews after that found no remaining in-scope problem. Deferred / limits. Ownership is kept in memory for the 128 most recent provider messages per chat process; after eviction or a restart that link is gone. A provider reusing one message identifier for two different turns is outside this fix. Verified.
Not verified. Live Linux, Windows, SSH and mobile. A Windows OMP recording reproduces the original bug through the real translator, but it is not a live run of this fix. App restart (as opposed to renderer reload) was not exercised live; the database close-and-reopen test covers saved rows. |
…lost-reply # Conflicts: # config/scripts/vitest-sqlite-runtime-files.mjs # src/main/acp/acp-timeline-translator.ts
📝 WalkthroughWalkthroughACP text chunks now use separate identities for assistant and reasoning channels. The timeline tracks message ownership across turns and rejects identified text from settled turns. The structured lane reports dropped text through its logger. New tests cover provider message ownership, stream identity, dropped-text diagnostics, OMP recordings, and journal reopening. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The fix keeps OMP replies that follow reasoning and prevents late text from moving to another turn. The remaining risk is minor: drop diagnostics log provider message identifiers unmasked. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 10 files. (2 skipped: 2 unsupported.)
✨ 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 |
![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
🧹 Nitpick comments (1)
src/main/acp/acp-text-message-ownership.test.ts (1)
104-109: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert answer ownership per turn. Both tests can pass when one turn contains the expected answer text and another completed turn has no answer.
src/main/acp/acp-text-message-ownership.test.ts#L104-L109: assert thatNext answerbelongs to the second turn.src/main/acp/acp-structured-omp-recordings.test.ts#L112-L112: compare each prompt's recorded answer with rows linked to its turn.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
676a6181-0915-44fa-915b-b2ffda602301
📒 Files selected for processing (12)
config/scripts/vitest-sqlite-runtime-files.mjssrc/main/acp/acp-session-update.tssrc/main/acp/acp-structured-acquire.tssrc/main/acp/acp-structured-lane.tssrc/main/acp/acp-structured-omp-recordings.test.tssrc/main/acp/acp-text-message-ownership.test.tssrc/main/acp/acp-text-stream-identity.test.tssrc/main/acp/acp-timeline-translator.tssrc/main/acp/acp-turn-messages.tssrc/main/acp/fixtures/omp-v17-reply-after-thought.jsonlsrc/main/acp/fixtures/omp-v17-reply-without-thought.jsonlsrc/main/native-chat/agent-session-timeline/provider-timeline-assembler-test-support.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| providerSessionId: this.deps.providerSessionId, | ||
| agent: this.deps.agent, | ||
| generation: this.deps.generation, | ||
| ...drop |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Redact provider message IDs in text-drop diagnostics. When an ACP agent places private text in messageId, acpNamedTextKey includes that value in itemId, and this spread sends it unchanged to console and span attributes. Log a bounded hash or masked identifier instead. Keep the full key for internal row identity. Based on learnings, log masked or truncated identifiers rather than unredacted provider data.
🧰 Tools
🪛 ast-grep (0.45.3)
[warning] 172-179: Avoid logging sensitive data
Context: this.logger.warn('ACP text chunk rejected by its message stream', {
scope: acp-text-${drop.reason},
sessionId: this.deps.sessionId,
providerSessionId: this.deps.providerSessionId,
agent: this.deps.agent,
generation: this.deps.generation,
...drop
})
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data-typescript)
Sources: Learnings, Linters/SAST tools
ELI5
OMP could finish a reply while Orca showed only its reasoning. OMP gives the reasoning and answer the same message identifier; Orca treated them as one text stream, silently discarded the answer, and still showed a successful turn. Answers that follow reasoning now appear in chat and stay saved when the chat reopens.
What Changed
Why
The mistake was at the provider translation boundary. Keeping separate channel items there preserves the shared journal writer’s rule that a text row cannot change from reasoning into an answer. Message ownership is kept in bounded, process-local state for the translator's lifetime, without adding persistent fields or rewriting saved chats.
Linked Issue
Found while investigating the OMP reasoning-only reply observed in #26502. Independently reproduced from Windows OMP + Gemini frames in #26587 (
omp-message-collision-repro.json): the same provider identifier appeared on reasoning and answer, and the answer was discarded despite a successful finish. That second-platform evidence exercises the real translator and assembler with a fake admitting sink; it is not live Windows validation of this fix.Visual Proof
Before: the original #26502 chat finishes with “Worked for 3s” and no answer. The provider’s saved history contained the answer; raw wire for that historical turn was unavailable.
After: OMP 17.0.5 in a hidden, isolated laptop rig answered a reasoning prompt. The chat shows the "Reasoning" row and the answer in the same turn:
The saved chat (read directly from the rig's database) has the reasoning and the answer as two separate rows from the same OMP message, then the turn finishing successfully. After a renderer reload (not an app restart) the same chat still showed the exact answer in the page text. The screenshot taken after the reload is not attached: a first-run "Jump to a worktree" tip covered the chat in it, so that step is shown by the page text and the saved rows, not by an image.
This live check ran on a local, never-pushed test build that merged this branch at
3f52e6fe00awith #26502 atcb0556199314(build403242994a66), not on this branch alone. The final head20072b9fea2adds only a merge of currentmain; the change itself is identical. A second prompt in that run was never sent: the tip dialog blocked the Send click, and no submission reached the app, so it says nothing about this fix.Testing
Scrubbed real OMP 17.0.5 recordings run through the complete protocol runtime, adapter, assembler and on-disk journal. The saved answer matches all recorded answer chunks; closing the database and opening a fresh journal preserves the complete message rows.
Synthetic interleaving and late-answer regressions cover OMP, OpenCode and Grok, including completion, cancellation, and a subsequent prompt. The diagnostic test also verifies that a failing logger cannot prevent settlement. No cross-channel identifier reuse was found in the saved Grok/OpenCode recordings audited; synthetic cases protect their shared translation paths.
Validation at
3f52e6fe00a: 8 targeted files / 61 tests, the scoped main-process typecheck, all 22 pre-push repository checks, and every selected CI job passed. After the final merge ofmain(head20072b9fea2; two import/test-list conflicts with #26544, which changes a different part of the same file): 9 targeted ACP files / 77 tests, the scoped main-process typecheck, and all 22 pre-push checks pass. CI on the final head is classified in the review comment. Skipped jobs are not counted as validation. Live Linux, Windows, SSH and mobile validation has not been performed. This change adds no remote wire fields, paths, platform-specific logic or mobile-facing surface.Review
A fresh adversarial review found that splitting the text items could let a late answer escape its original turn. The approved correction retains ownership at the provider message level. Fresh architecture and implementation reviews found no in-scope blocker after the correction.
Agent skill upstream boundary
Notes
Ownership retains at most 128 recent provider messages for one translator. Eviction or a fresh translator removes that correlation; this is not durable protection against arbitrary historical live duplicates. Normal history replay remains ignored and saved rows remain readable. Reusing one provider message identifier for different actual turns is an existing provider-contract limitation, outside this fix.
Checklist