-
Notifications
You must be signed in to change notification settings - Fork 45
fix(agent-runtime): consolidate streaming text content blocks for storage #425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,8 +67,40 @@ export class MessageConverter { | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Merge adjacent text content blocks into a single block. | ||
| * Non-text blocks act as natural boundaries and are never merged. | ||
| */ | ||
| static consolidateContentBlocks(blocks: MessageContentBlock[]): MessageContentBlock[] { | ||
| const result: MessageContentBlock[] = []; | ||
| for (const block of blocks) { | ||
| const prev = result[result.length - 1]; | ||
| if ( | ||
| prev && prev.type === ContentBlockType.Text && block.type === ContentBlockType.Text | ||
| ) { | ||
| const prevText = prev as TextContentBlock; | ||
| const curText = block as TextContentBlock; | ||
| prevText.text = { | ||
| value: prevText.text.value + curText.text.value, | ||
| annotations: [...prevText.text.annotations, ...curText.text.annotations], | ||
| }; | ||
| } else if (block.type === ContentBlockType.Text) { | ||
| const curText = block as TextContentBlock; | ||
| result.push({ | ||
| ...curText, | ||
| text: { ...curText.text, annotations: [...curText.text.annotations] }, | ||
| }); | ||
| } else { | ||
| result.push({ ...block }); | ||
| } | ||
| } | ||
| return result; | ||
| } | ||
|
Comment on lines
+70
to
+98
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Annotation position indices are not adjusted during text concatenation. The consolidation logic concatenates This will cause any downstream consumers that rely on annotation positions (e.g., for highlighting, links, or citations) to reference incorrect character ranges in the consolidated text. Please verify whether annotation indices are consumed downstream and need adjustment: #!/bin/bash
# Search for usages of annotations in the codebase
rg -n -C3 'annotations' --type=ts -g '!**/test/**' -g '!**/node_modules/**'🤖 Prompt for AI Agents |
||
|
|
||
| /** | ||
| * Extract MessageObjects and accumulated usage from AgentStreamMessage objects. | ||
| * Adjacent text content blocks from streaming chunks are consolidated into | ||
| * a single message with merged text, matching the OpenAI Assistants API behavior. | ||
| */ | ||
| static extractFromStreamMessages( | ||
| messages: AgentStreamMessage[], | ||
|
|
@@ -77,14 +109,14 @@ export class MessageConverter { | |
| output: MessageObject[]; | ||
| usage?: RunUsage; | ||
| } { | ||
| const output: MessageObject[] = []; | ||
| const allBlocks: MessageContentBlock[] = []; | ||
| let promptTokens = 0; | ||
| let completionTokens = 0; | ||
| let hasUsage = false; | ||
|
|
||
| for (const msg of messages) { | ||
| if (msg.message) { | ||
| output.push(MessageConverter.toMessageObject(msg.message, runId)); | ||
| allBlocks.push(...MessageConverter.toContentBlocks(msg.message)); | ||
| } | ||
| if (msg.usage) { | ||
| hasUsage = true; | ||
|
|
@@ -93,6 +125,20 @@ export class MessageConverter { | |
| } | ||
| } | ||
|
|
||
| const consolidated = MessageConverter.consolidateContentBlocks(allBlocks); | ||
|
|
||
| const output: MessageObject[] = consolidated.length > 0 | ||
| ? [{ | ||
| id: newMsgId(), | ||
| object: AgentObjectType.ThreadMessage, | ||
| createdAt: nowUnix(), | ||
| runId, | ||
| role: MessageRole.Assistant, | ||
| status: MessageStatus.Completed, | ||
| content: consolidated, | ||
| }] | ||
| : []; | ||
|
|
||
| let usage: RunUsage | undefined; | ||
| if (hasUsage) { | ||
| usage = { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
elseblock assumes that everyMessageContentBlockhas atextproperty. WhileMessageContentBlockis currently only defined asTextContentBlock, the comments and the logic in this function suggest it is intended to handle other types (like images) as natural boundaries. If a non-text block is encountered, accessingblock.text.annotationswill cause a runtime crash. You should check the block type before attempting to deep-copy thetextandannotationsproperties.