feat: inline tool summaries with sequential dedup#2
Conversation
Root cause: checkOverflow() only checked textBuffer.Len() against the 3000-char threshold, but the rendered message also includes accumulated tool completion summaries. For turns with many tool calls (50+), the tool summary section alone could push the rendered output beyond Slack's chat.update limit (~4000 chars). When chat.update rejected with msg_too_long, the update was silently dropped, leaving the previous (shorter) intermediate content visible -- causing apparent truncation. Two-layer fix: 1. checkOverflow() now uses estimateRenderedLen() which accounts for text buffer + tool summaries + header + footer. Also called after tool_done events (previously only on text_delta). 2. Outbound queue now handles msg_too_long errors gracefully: truncates text at a newline boundary and retries once, instead of silently dropping the message. Adds TestCoalescer_OverflowFromManyTools and TestTruncateForSlack.
Replace the old textBuffer + completedTools approach with an interleaved segment stream. Tool completion summaries now appear inline where they occurred in the text flow, rather than being collected at the bottom. Sequential identical tool descriptions are collapsed into a single line with a count (e.g., 'Reading `handler.go` ×3'). Only consecutive identical entries are deduped; non-adjacent occurrences remain separate. Key changes: - New segment type (segText/segTool) replaces separate textBuffer and completedTools fields - appendToolSegment() handles sequential dedup by checking the previous segment - renderMessage() walks segments in order, rendering text and tools inline - Directive extraction still works (concatenates all text segments) - FinalMessageText extraction walks segments backwards from the end Tests added: - TestCoalescer_SequentialToolDedup: verifies ×3 and ×2 counts for consecutive identical tools with a different tool between batches - TestCoalescer_InlineToolOrdering: verifies tool appears between the text segments that precede and follow it
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c79fa52960
ℹ️ 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".
| // For simplicity, when directives are present we extract clean text once | ||
| // and render it followed by tools (since directive extraction operates on | ||
| // the concatenated text). When no directives, render segment by segment. | ||
| if directiveResult != nil && directiveResult.CleanText != "" { |
There was a problem hiding this comment.
When render.ExtractDirectives removes all directive text, directiveResult.CleanText can be empty, but this condition falls through to the non-directive rendering path and re-renders the original text segments (including raw switchboard blocks). That regresses prior behavior where valid directives were removed from visible text and only rendered as blocks/fallback; users will now see internal directive JSON in Slack whenever a message is directive-only or directives strip all textual content.
Useful? React with 👍 / 👎.
| sc.textBuffer.Reset() | ||
| sc.textBuffer.WriteString(e.Text) | ||
| // Replace clears ALL text segments and replaces with a single one. | ||
| sc.segments = sc.segments[:0] |
There was a problem hiding this comment.
text_replace is defined as replacing the text buffer, but this now resets the entire interleaved segments stream, which also drops previously completed tool summaries. If a text_replace arrives after one or more tool_done events, those tool entries disappear from subsequent updates/final output, causing an incomplete and misleading progress transcript.
Useful? React with 👍 / 👎.
1 participant
Summary
Stacked on PR #1. Two changes:
1. Fix: Slack message truncation from tool summary overflow (
b694029)checkOverflow()now usesestimateRenderedLen()accounting for text + tools + header/footer (was only checking raw text buffer length)checkOverflow()called aftertool_doneevents (was only ontext_delta)msg_too_longerrors trigger truncate-and-retry instead of silent drop2. Feat: inline tool summaries with sequential dedup (
1988ffd)Replaces the old
textBuffer+completedToolsmodel with an interleaved[]segmentstream. Tool summaries now render inline where they occurred in the text flow:Sequential dedup: consecutive identical tool descriptions collapse into one line with a count (×N). Non-adjacent identical entries remain separate.
Tests added
TestCoalescer_OverflowFromManyTools— overflow split with many unique toolsTestTruncateForSlack/TestIsMsgTooLong— outbound truncation fallbackTestCoalescer_SequentialToolDedup— ×3 and ×2 counts with interleaved different toolTestCoalescer_InlineToolOrdering— tool appears between surrounding text segments