Conversation history returns MessageSelect[] and formats at call site#2743
Conversation history returns MessageSelect[] and formats at call site#2743mike-inkeep wants to merge 2 commits intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
The return-type refactor from string to MessageSelect[] is well-structured — separation of fetching/compressing vs. formatting is a good direction, and the multi-artifact grouping fix is correct. Three areas need attention before merging: a behavioral inconsistency between getFormattedConversationHistory and the new formatMessagesAsConversationHistory, a createdAt regression in the summary message, and missing test coverage for two new code paths.
| @@ -937,8 +947,13 @@ function formatMessagesAsConversationHistory(messages: any[]): string { | |||
| roleLabel = msg.role || 'system'; | |||
| } | |||
|
|
|||
| return `${roleLabel}: """${reconstructMessageText(msg)}"""`; | |||
| const reconstructedMessage = reconstructMessageText(msg); | |||
| if (!reconstructedMessage) { | |||
| return null; | |||
| } | |||
| return `${roleLabel}: """${reconstructedMessage}"""`; | |||
| }) | |||
| .filter((line): line is string => line !== null) | |||
| .join('\n'); | |||
|
|
|||
| return `<conversation_history>\n${formattedHistory}\n</conversation_history>\n`; | |||
There was a problem hiding this comment.
The newly exported formatMessagesAsConversationHistory differs from the inline formatting in getFormattedConversationHistory (around line 376) in two meaningful ways:
- This function uses
reconstructMessageText(msg)(handles multi-part content with artifact refs), while the other usesmsg.content.textdirectly — silently dropping artifact reference tags from multi-part messages. - This function filters out messages with empty reconstructed text, while the other includes them (producing
role: """"""entries).
Since getFormattedConversationHistory is still actively called from AgentSession.ts, these inconsistencies produce different conversation history formats depending on the code path. Consider refactoring getFormattedConversationHistory to delegate to this exported function.
| const referenceMessage = messageHistory[0]; | ||
| const summaryMessage: MessageSelect = { | ||
| id: `summary-${getConversationId()}`, | ||
| tenantId: referenceMessage.tenantId, | ||
| projectId: referenceMessage.projectId, | ||
| conversationId: referenceMessage.conversationId, | ||
| role: 'system', | ||
| fromSubAgentId: null, | ||
| toSubAgentId: null, | ||
| fromExternalAgentId: null, | ||
| toExternalAgentId: null, | ||
| fromTeamAgentId: null, | ||
| toTeamAgentId: null, | ||
| content: { | ||
| text: `[Previous conversation history truncated - ${i + 1} earlier messages]`, | ||
| }, | ||
| visibility: 'system', | ||
| messageType: 'chat', | ||
| createdAt: messageHistory[0].createdAt, | ||
| taskId: null, | ||
| parentMessageId: null, | ||
| a2aTaskId: null, | ||
| a2aSessionId: null, | ||
| metadata: null, | ||
| createdAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), |
There was a problem hiding this comment.
createdAt changed from messageHistory[0].createdAt to new Date().toISOString(). The summary message now has a timestamp after all the messages it summarizes, rather than being anchored to the start of the conversation. The array ordering is still correct (it's unshifted), but if any downstream code sorts or filters by createdAt, the summary would sort to the end. The previous behavior was more semantically correct — consider reverting to referenceMessage.createdAt.
| const result = await getConversationHistoryWithCompression(baseParams); | ||
| const toolResult = result.find((msg) => msg.messageType === 'tool-result'); | ||
| const toolResultText = toolResult?.content?.text ?? ''; | ||
|
|
||
| expect(toolResultText).toContain('id: art-1'); | ||
| expect(toolResultText).toContain('id: art-2'); |
There was a problem hiding this comment.
This test verifies both artifact IDs are present but doesn't assert the \n join between them. If the join separator were accidentally changed to empty string or space, this test would still pass. Consider:
expect(toolResultText).toMatch(/id: art-1[\s\S]*\n[\s\S]*id: art-2/);| return parts | ||
| .map((part: any) => { | ||
| if (part.type === 'text') { | ||
| const partKind = part.kind ?? part.type; |
There was a problem hiding this comment.
The part.kind ?? part.type fallback handles both A2A protocol shape ({ kind: 'text' }) and legacy shape ({ type: 'text' }). The existing reconstructMessageText tests only cover { type: ... } parts — there's no test coverage for the { kind: ... } shape. Add a test case to verify the fallback works.
| const reconstructedMessage = reconstructMessageText(msg); | ||
| if (!reconstructedMessage) { | ||
| return null; | ||
| } | ||
| return `${roleLabel}: """${reconstructedMessage}"""`; | ||
| }) | ||
| .filter((line): line is string => line !== null) |
There was a problem hiding this comment.
formatMessagesAsConversationHistory now filters out messages where reconstructMessageText returns empty string — this is new behavior (previously all messages were formatted). A message with only non-text/non-artifact parts (e.g. file parts) would be silently dropped. This needs a direct test, and it's worth confirming this is the desired behavior for all message types.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
conversations.ts:510Args duplicated for each artifact sharing a toolCallId
💭 Consider (2) 💭
💭 1) conversations.ts:903 Defensive fallback for part.kind ?? part.type lacks documentation
Issue: The change from part.type to part.kind ?? part.type suggests two different part schemas exist, but there's no documentation of when each occurs.
Why: The canonical types use kind. If type is a legacy format, a comment would clarify intent.
Fix: Add inline comment explaining the variants, or log occurrences if type is unexpected.
Refs: utility.ts:91-96 (canonical kind-based schema)
💭 2) conversations.ts:950-956 Behavioral change - empty messages now filtered out
Issue: New implementation filters out messages with empty reconstructed text. Previously these appeared as roleLabel: """""".
Why: This is arguably an improvement but goes beyond the stated refactoring goal.
Fix: Document this as intentional and consider adding a test case.
Inline Comments:
- 💭 Consider:
conversations.ts:903Defensivekind ?? typefallback - 💭 Consider:
conversations.ts:950-956Empty message filtering behavior change
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-structured refactoring that improves type safety by returning MessageSelect[] from getConversationHistoryWithCompression and moving formatting to call sites. The multi-artifact support for shared toolCallId values is a valuable fix. The one Minor issue (args duplication) is a small optimization opportunity, and the Consider items are documentation/testing suggestions rather than blocking concerns.
The PR is ready to merge. Nice work on improving the type boundaries! 🎉
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
conversations.artifact-replacement.test.ts:147 |
Test doesn't verify newline-joining format | Nitpick - the core assertion (both artifacts present) is correct |
conversations.test.ts |
Missing tests for kind property in reconstructMessageText |
Valid but downgraded - defensive code for compatibility, not a bug risk |
conversations.artifact-replacement.test.ts:35 |
Test helper missing MessageSelect fields |
Pre-existing pattern in test fixtures, low impact |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-tests |
4 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-precision |
3 | 0 | 2 | 0 | 1 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 7 | 0 | 2 | 0 | 1 | 0 | 3 |
| const artifactParts = [ | ||
| `Artifact: "${artifact.name ?? artifact.artifactId}" (id: ${artifact.artifactId})`, | ||
| ]; | ||
| if (argsStr) artifactParts.push(`args: ${argsStr}`); |
There was a problem hiding this comment.
🟡 Minor: Args duplicated for each artifact sharing a toolCallId
Issue: When multiple artifacts share the same toolCallId, the argsStr (computed once from message metadata at lines 492-495) gets appended to every artifact's reference string inside the map. Since all artifacts for the same tool call came from the same invocation, they have identical toolArgs, resulting in the same args: {...} appearing N times in the output.
Why: This unnecessarily bloats token count in the conversation history. For tool calls that produce multiple artifacts (e.g., multiple images or documents), the args string could be repeated 2-10x.
Fix: Consider moving args to a single prefix line before the artifact list:
const argsLine = argsStr ? `Tool call args: ${argsStr}\n` : '';
const artifactRefs = relatedArtifacts.map((artifact) => {
// ... build artifact-specific parts WITHOUT args
});
return {
...msg,
content: { text: argsLine + artifactRefs.join('\n') },
};Refs:
| return parts | ||
| .map((part: any) => { | ||
| if (part.type === 'text') { | ||
| const partKind = part.kind ?? part.type; |
There was a problem hiding this comment.
💭 Consider: Defensive fallback for part.kind ?? part.type
Issue: This fallback suggests there are two different part schemas in play: the canonical MessageContent.parts type (which uses kind per the schema definitions) and some other format that uses type. The fallback is reasonable defensive code, but there's no documentation explaining when each variant occurs.
Why: Without clarity on the source of type-keyed parts, this could mask upstream inconsistencies rather than solve a real polymorphism need. The canonical types in utility.ts and a2a.ts both use kind.
Fix: If both are legitimate variants (e.g., legacy data or external formats), add a brief inline comment:
// Support both 'kind' (canonical schema) and 'type' (legacy/external format)
const partKind = part.kind ?? part.type;If type is never expected, consider logging when encountered to track occurrences.
| const reconstructedMessage = reconstructMessageText(msg); | ||
| if (!reconstructedMessage) { | ||
| return null; | ||
| } | ||
| return `${roleLabel}: """${reconstructedMessage}"""`; | ||
| }) | ||
| .filter((line): line is string => line !== null) |
There was a problem hiding this comment.
💭 Consider: Behavioral change - empty messages now filtered out
Issue: The new implementation filters out messages with empty reconstructed text (returning null then filtering). The previous implementation would include these as roleLabel: """""". This is arguably an improvement, but goes beyond the stated refactoring goal.
Why: This could affect downstream consumers that rely on message boundaries (e.g., compression logic counting messages) or debugging traces. If intentional, it's worth documenting.
Fix: If intentional, consider adding a test case validating the filtering behavior and noting this in the PR description as a deliberate improvement.
|
TL;DR — Shifts Key changes
Summary ｜ 6 files ｜ 2 commits ｜ base: Return structured messages instead of pre-formatted strings
This decouples history retrieval from serialization, letting future consumers of the history data (e.g. token-counting, streaming, or alternate formats) operate on structured messages without re-parsing a string. The summary placeholder created by
Multi-artifact grouping per
|
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scope: Changes since last review (
8eea5bf88a06). The delta consists of a merge commit bringing in PDF attachment support fromorigin/main(PR #2709).
Delta Review Summary
Files with delta changes:
conversation-history.ts: AddedmapFileToAiSdkContentParthelper for PDF/image handling, renamedimageParts→filePartsconversations.ts: UpdatedcreateMessageAPI to use{ scopes, data }structureAgent.test.ts: Added PDF test + mock path rename
Findings: No new issues meeting the severity/confidence thresholds were identified in the delta changes. The PDF support code is well-structured with proper type handling, defensive null returns for unsupported file types, and appropriate warning logging.
🕐 Pending Recommendations (5)
Prior review feedback that remains applicable to the original PR changes:
- 🟡
conversations.ts:510Args duplicated for each artifact sharing a toolCallId - 💭
conversations.ts:903Defensivekind ?? typefallback lacks documentation - 💭
conversations.ts:950-956Empty messages now filtered out (behavioral change) - 🟠
runtime/conversations.ts:268createdAtregression in summary message (pullfrog) - 🟠
conversations.ts:961Behavioral inconsistency between formatting functions (pullfrog)
✅ APPROVE
Summary: The delta changes from the merge commit are clean. PDF attachment support is correctly integrated with the existing file handling infrastructure. Prior review feedback from the first review run remains applicable to the original PR changes — see Pending Recommendations above. 🎉
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
| Total | 3 | 0 | 0 | 0 | 0 | 0 | 3 |
Note: Test coverage findings (unsupported file types, filename metadata, URI path) were assessed as MEDIUM/LOW confidence and discarded per review criteria.
Ito Test Report ❌7 test cases ran. 1 failed, 6 passed. Overall, the unified run executed 7 test cases with 6 passes and 1 failure (plus several verification runs with no executable cases), indicating generally solid behavior with one confirmed production defect. Key findings were that auth setup and run-domain gates behaved correctly (web_client app creation returned 201, PoW was required and successfully minted anon tokens when solved, and unauthorized origins were blocked with 403), while chat edge checks for 10 parallel requests and a ~50k prompt were stable, but first-turn requests incorrectly inject an empty <conversation_history> wrapper (medium severity, likely introduced by this PR) causing no-history semantics drift. ❌ Failed (1)
🟠 Empty conversation history wrapper injected on first turn
Relevant code:
if (!messagesToFormat.length) {
return [];
}
export function formatMessagesAsConversationHistory(messages: MessageSelect[]): string {
const formattedHistory = messages
.map((msg: MessageSelect) => {
// ...role formatting...
const reconstructedMessage = reconstructMessageText(msg);
if (!reconstructedMessage) {
return null;
}
return `${roleLabel}: """${reconstructedMessage}"""`;
})
.filter((line): line is string => line !== null)
.join('\n');
return `<conversation_history>\n${formattedHistory}\n</conversation_history>\n`;
}
if (conversationHistory.trim() !== '') {
messages.push({ role: 'user', content: conversationHistory });
}✅ Passed (6)
Commit: Tell us how we did: Give Ito Feedback |
1 participant
No description provided.