Skip to content

fix(runtime): group parallel tool calls in history summarizer replay - #3049

Closed
1625567290 wants to merge 2 commits into
apache:mainfrom
1625567290:fix/runtime-summarizer-parallel-tool-calls
Closed

fix(runtime): group parallel tool calls in history summarizer replay#3049
1625567290 wants to merge 2 commits into
apache:mainfrom
1625567290:fix/runtime-summarizer-parallel-tool-calls

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

replayPlanItemsToModelMessages emitted every tool_call replay item as its own assistant message. Parallel calls therefore looked like:

assistant [call A]
assistant [call B]
tool [result A]
tool [result B]

Strict OpenAI-compatible providers reject that (DeepSeek 400: previous tool_calls not yet answered). Compaction then fail-opens as provider_error.

Consecutive tool_call items are now one assistant message with N tool-call parts, followed by N tool messages. Sequential rounds that already have answers between them stay separate. The primary replay materializer is unchanged.

Fixes #3030

Verification

  • npx tsx --test packages/runtime/src/__tests__/history-compact-summarizer.test.ts — 10/10
  • New cases: two parallel reads → one assistant + two tool messages; sequential rounds stay two assistants
  • Both cases assert every assistant tool-call id is answered by the immediately following tool messages

Not run: live DeepSeek generateText. The unit test locks the rejected message shape.

Checklist

  • Tests cover the change and fail without it
  • Focused lint/typecheck and the affected suites pass locally
  • Full workspace lint/format/typecheck

Does this PR entail a change in behavior?

  • Yes — history summarizer requests over parallel tool calls now use one assistant message

Consecutive tool_call replay items are one assistant step. Emitting
each as its own assistant message leaves the previous tool_calls
unanswered and is rejected by strict OpenAI-compatible providers.

Fixes apache#3030

Generated-by: Grok
AssistantContent can be a string, and tool parts include approval
responses. The parallel-call grouping assertions need the same
narrowing the rest of this file already uses.

Generated-by: Grok
@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Problem solved

replayPlanItemsToModelMessages now groups consecutive parallel tool_call items into one assistant message. It emits one tool message for each result. Sequential tool-call rounds remain separate. This prevents strict OpenAI-compatible providers from rejecting replayed messages and allows compaction to continue.

Source of truth

This change extends the existing summarizer replay path. It aligns that path with the invariant already used by the primary materializer. The primary materializer remains unchanged. No parallel replay path was added.

Scope and complexity

The indexed iteration and explicit index advancement are necessary to group consecutive tool calls without changing text, tool-result, or skipped thinking-item handling. The change is the smallest coherent solution for the replay defect.

Simplification opportunities

No code or tests can be removed without weakening behavior or regression coverage. The tests cover both parallel grouping and sequential separation, including tool-call ID matching.

Risks and validation

The main risk is incorrect grouping or mismatched tool-call IDs during replay. Focused tests pass. Full workspace checks and live DeepSeek verification were not run.

Walkthrough

The history summarizer now groups consecutive tool calls into one assistant message. Tests verify matching tool-result ordering and preserve separate assistant messages for sequential tool-call rounds.

Changes

History replay

Layer / File(s) Summary
Group consecutive tool calls
packages/runtime/src/history-compact-summarizer.ts
Index-based replay combines consecutive tool calls into one assistant message and preserves tool-result and skipped-thinking handling.
Validate replay ordering
packages/runtime/src/__tests__/history-compact-summarizer.test.ts
Tests verify parallel calls, matching results, strict sequencing, and separate sequential rounds.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🟡 Moderate · up to 1715c

Tool calls separated by a thinking item can still be replayed as separate assistant messages, leaving an earlier call unanswered and causing strict providers to reject history summarization requests. The PR is not merge-ready until this interleaving is grouped and covered by a regression test.

Sequence Diagram(s)

sequenceDiagram
  participant ReplayPlan
  participant HistorySummarizer
  participant OpenAICompatibleProvider
  ReplayPlan->>HistorySummarizer: provide tool-call replay items
  HistorySummarizer->>OpenAICompatibleProvider: send one assistant message with parallel tool calls
  HistorySummarizer->>OpenAICompatibleProvider: send matching tool-result messages
Loading

Suggested reviewers: astro-han, are404

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the runtime fix for grouping parallel tool calls in history summarizer replay.
Description check ✅ Passed The description includes the required summary, issue reference, verification results, checklist, behavior change, and unrun checks.
Linked Issues check ✅ Passed The implementation and tests satisfy issue #3030 by grouping consecutive tool calls, preserving sequential rounds, and leaving the primary replay path unchanged.
Out of Scope Changes check ✅ Passed The changes are limited to the summarizer replay logic and regression tests required for parallel tool-call handling.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 24a0d4f7-0003-4b4d-9903-e1502a1411d1

📥 Commits

Reviewing files that changed from the base of the PR and between e388557 and 1715ce4.

📒 Files selected for processing (2)
  • packages/runtime/src/__tests__/history-compact-summarizer.test.ts
  • packages/runtime/src/history-compact-summarizer.ts

Comment on lines +155 to +159
while (index < items.length) {
const next = items[index];
if (next?.kind !== 'tool_call') break;
calls.push(next);
index += 1;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Continue grouping across skipped thinking items.

If a thinking item occurs between tool calls in one tool step, Line 157 ends the group. Lines 183-185 then remove that boundary. The replay emits adjacent assistant tool-call messages, so the first call remains unanswered and strict providers can reject the request.

Skip thinking items while collecting calls. Add a regression case with tool_call, thinking, tool_call, and matching results.

Proposed fix
       while (index < items.length) {
         const next = items[index];
+        if (next?.kind === 'thinking') {
+          index += 1;
+          continue;
+        }
         if (next?.kind !== 'tool_call') break;
         calls.push(next);
         index += 1;

As per path instructions, “Review the diff adversarially against the problem it claims to solve.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
while (index < items.length) {
const next = items[index];
if (next?.kind !== 'tool_call') break;
calls.push(next);
index += 1;
while (index < items.length) {
const next = items[index];
if (next?.kind === 'thinking') {
index += 1;
continue;
}
if (next?.kind !== 'tool_call') break;
calls.push(next);
index += 1;

Source: Path instructions

@1625567290

Copy link
Copy Markdown
Contributor Author

Closing in favor of #3038. That PR groups by step and also covers interleaved tool results; our adjacency grouping is the weaker of the two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(runtime): history summarizer emits parallel tool calls as separate assistant messages, rejected by strict OpenAI-compatible providers

1 participant