fix(telegram): reuse preview message across tool-use rounds to prevent inter-tool text leak#32890
fix(telegram): reuse preview message across tool-use rounds to prevent inter-tool text leak#32890Sid-Qin wants to merge 1 commit into
Conversation
…t text leak When an LLM response involved multiple tool-use rounds, onAssistantMessageStart archived the current streaming preview and called forceNewMessage() to create a new one for each round. Each round's intermediate text became an independent visible Telegram message before the finally block could delete them. The fix removes the archive+forceNewMessage logic so all tool-use rounds share the same preview message. Intermediate text is overwritten by the next round, and only the final reply text is visible to the user. Closes openclaw#32535 Made-with: Cursor
Greptile SummaryThis PR fixes a UX regression (#32535) where multiple separate Telegram messages were sent for intermediate LLM narration text during multi-tool-call turns. The fix is targeted and correct: the Key observations:
All 49 tests pass. The changes are safe to merge once the two identified items are addressed: clarifying the stale comment and strengthening the test coverage. Confidence Score: 4/5
Last reviewed commit: 7c87c67 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c87c67431
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // forceNewMessage(), creating a visible message per round. | ||
| resetDraftLaneState(answerLane); | ||
| // New assistant message boundary: this lane now tracks a fresh preview lifecycle. | ||
| finalizedPreviewByLane.answer = false; |
There was a problem hiding this comment.
Keep finalized preview flag when reusing same message
Now that assistant rounds reuse a single preview message, resetting finalizedPreviewByLane.answer on every onAssistantMessageStart can delete an already-finalized user reply on a later media-only round. In the sequence “text final (preview edited) -> assistant restart -> media-only final”, cleanup treats the answer lane as unfinalized and calls stream.clear(), and clear() deletes the current preview message ID, so the previously finalized text disappears from chat.
Useful? React with 👍 / 👎.
| // First final edits the single preview message; remaining finals are delivered normally. | ||
| expect(editMessageTelegram).toHaveBeenNthCalledWith( | ||
| 1, | ||
| 123, | ||
| 1001, | ||
| "Message A final", | ||
| expect.any(Object), | ||
| ); |
There was a problem hiding this comment.
The test verifies that the first final edits the preview, but it doesn't assert how Message B and C finals are delivered. The code comment claims they are "delivered normally" but without an explicit assertion, a future regression where those finals are accidentally dropped would pass silently.
After Message A final, finalizedPreviewByLane.answer becomes true, routing subsequent finals through deliverReplies instead of edit-via-preview. Consider adding an assertion to guard against silent regressions:
| // First final edits the single preview message; remaining finals are delivered normally. | |
| expect(editMessageTelegram).toHaveBeenNthCalledWith( | |
| 1, | |
| 123, | |
| 1001, | |
| "Message A final", | |
| expect.any(Object), | |
| ); | |
| // First final edits the single preview message; remaining finals are delivered normally. | |
| expect(editMessageTelegram).toHaveBeenNthCalledWith( | |
| 1, | |
| 123, | |
| 1001, | |
| "Message A final", | |
| expect.any(Object), | |
| ); | |
| // Messages B and C finals should be delivered via deliverReplies since the | |
| // single preview was already finalized by Message A. | |
| expect(deliverReplies).toHaveBeenCalled(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/telegram/bot-message-dispatch.test.ts
Line: 475-482
Comment:
The test verifies that the first final edits the preview, but it doesn't assert how Message B and C finals are delivered. The code comment claims they are "delivered normally" but without an explicit assertion, a future regression where those finals are accidentally dropped would pass silently.
After Message A final, `finalizedPreviewByLane.answer` becomes true, routing subsequent finals through `deliverReplies` instead of edit-via-preview. Consider adding an assertion to guard against silent regressions:
```suggestion
// First final edits the single preview message; remaining finals are delivered normally.
expect(editMessageTelegram).toHaveBeenNthCalledWith(
1,
123,
1001,
"Message A final",
expect.any(Object),
);
// Messages B and C finals should be delivered via deliverReplies since the
// single preview was already finalized by Message A.
expect(deliverReplies).toHaveBeenCalled();
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/telegram/bot-message-dispatch.ts
Line: 271
Comment:
The comment references "forceNewMessage decision" but this field is no longer used for that purpose after this PR's changes. It's now used for finalization tracking (checked on line 527) and reasoning lane splitting (line 610). Update the comment to reflect its actual purpose.
```suggestion
// Mark that we've received streaming content for finalization tracking.
```
How can I resolve this? If you propose a fix, please make it concise. |
Summary
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Security Impact (required)
Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Updated 2 existing tests and verified all 49 tests pass.
Human Verification (required)
Compatibility / Migration
Failure Recovery (if this breaks)
Risks and Mitigations