fix(runtime): group parallel tool calls into one summarizer assistant message - #3038
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for fixing the strict-provider failure at the shared summarizer replay seam. Grouping parallel calls here is the right direction, but the current grouping still ends too early when calls and results are interleaved.
The Runtime history can legitimately contain:
call A
call B
result A
call C
result B
result C
There is already a production-seam fixture for this ordering in ai-sdk-backend.test.ts (replays interleaved parallel RuntimeEvent tool calls as one provider tool-call block).
Because openToolCalls is cleared on the first result, this PR materializes that history as:
assistant [A, B]
tool [result A]
assistant [C]
tool [result B]
tool [result C]
A strict OpenAI-compatible provider can still reject this: the second assistant message appears before every call from the first assistant message has a result.
Could we keep the entire assistant step open and buffer its results until the step boundary, using stepId and the set of unsettled calls rather than adjacency alone? The cleanest long-term shape would be to share the primary replay materializer’s step-grouping invariant, so the summarizer does not maintain a second, weaker interpretation of the same history.
The new test is useful; extending it with the existing interleaved ordering, plus a case proving that different steps are not merged, should cover the missing behavior.
中文对照
感谢在共享的 summarizer replay 边界修复严格 provider 的失败问题。在这里合并并行调用的方向是正确的,但当调用和结果交错出现时,当前分组仍会过早结束。
Runtime 历史可能合法地按以下顺序记录:
call A
call B
result A
call C
result B
result C
ai-sdk-backend.test.ts 已有命中生产调用链的测试覆盖这种顺序:replays interleaved parallel RuntimeEvent tool calls as one provider tool-call block。
由于代码在第一个 result 出现时就清空 openToolCalls,这个 PR 会把上述历史转换成:
assistant [A, B]
tool [result A]
assistant [C]
tool [result B]
tool [result C]
严格的 OpenAI-compatible provider 仍可能拒绝该序列,因为第一条 assistant message 中的全部调用得到结果前,第二条 assistant message 已经出现。
建议保持整个 assistant step 打开并缓存结果,使用 stepId 和尚未完成的调用集合判断边界,而不是仅依赖事件是否相邻。长期更干净的方案是复用 primary replay materializer 的 step grouping invariant,避免 summarizer 维护另一套更弱的历史解释。
新增测试是有价值的;建议加入现有的交错顺序,并补充不同 step 不应被合并的用例。
AI-assisted review disclosure: Codex helped inspect the diff and reproduce the message sequence. I reviewed the source, evidence, and severity and made the final judgment.
|
Addressed in c9d7112. Grouping is now step-based with results buffered to the step boundary, as you suggested:
Tests added per your list: the interleaved ordering from the primary materializer's fixture ( Agreed the cleanest long-term shape is sharing the primary materializer's step-grouping invariant instead of a second interpretation here — that refactor (extracting the step merge out of the materializer's flush) felt out of proportion for this fix, so I kept it as a mirror with the same semantics and would follow up if you want the extraction. Full |
|
Reviewed against the #3029 incident repro. Step-based grouping with results deferred to the step boundary is the correct invariant and strictly stronger than the adjacency-based draft we had in #3040 (which missed the interleaved-result ordering). The stamped-stepId / legacy-settledness membership logic matches the primary materializer. No blocking issues found. One non-blocking observation: a cross-step call arriving while a previous step still has unsettled calls (with differing stamped stepIds) would flush the previous step's partial results and open a new assistant message. That ordering is already-malformed replay input that the primary path rejects as well, so it is out of scope for this fix. FYI: we are removing our own #3030 fix from #3040 and deferring to this PR. |
…erseded by apache#3038) PR apache#3038 (fix/summarizer-parallel-tool-calls) fixes apache#3030 with a stronger step-based grouping that also handles interleaved results (call A, call B, result A, call C, result B, result C). Our adjacency-based merge missed that case, so drop the apache#3030 portion here and keep this PR focused on apache#3029 (write-gate summary validation) only.
|
Thanks for cross-checking against the incident repro and for deferring #3040's variant. Confirming your non-blocking observation matches what my probe pass found: a cross-step call with a differing stamped stepId arriving while the previous step is unsettled flushes the partial results and opens a new message — that input is already-malformed replay (the primary path rejects it too), and the pre-fix code produced an equally invalid shape for it, so it stays out of scope here. Generated-by: Claude Code |
… message replayPlanItemsToModelMessages emitted every tool_call replay item as its own assistant message, so a step with parallel tool calls produced a second assistant message while the first's tool calls were still unanswered. Strict OpenAI-compatible providers reject that shape (DeepSeek 400: an assistant message with tool_calls must be followed by tool messages responding to each tool_call_id), so history compaction could never succeed for sessions on such providers — the provider_error fail-open loop behind the apache#3029 incident. Collect a run of tool calls uninterrupted by text or results into one assistant content array, mirroring the step-merge invariant the primary replay path gets from the materializer. Skipped thinking items do not break the run. The primary replay path is untouched. Fixes apache#3030 Generated-by: Claude Code
Review follow-up: adjacency-based grouping closed the assistant message at the first tool_result, so the production-legal interleaved ordering (call A, call B, result A, call C, result B, result C) still emitted a second assistant message while call B was unanswered — the same strict provider rejection this PR set out to fix. Group by assistant step instead: a step's calls share one assistant message and its results are buffered to the step boundary. Membership follows the stamped stepId when both sides carry one; legacy items without a stepId join while the open step still has unsettled calls, which is exactly the interleaving case, and a call arriving after the open step fully settled opens its own message so distinct steps are never merged. Generated-by: Claude Code
c9d7112 to
99ce715
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughSummaryThis PR fixes history summarization for parallel tool calls. The primary replay path remains unchanged. This change extends the existing history summarization path instead of creating a parallel replay path. The solution is coherent and targeted. Regression tests cover parallel calls, interleaved calls, separate settled steps, stamped The main risk is changed message grouping in summarized histories. This improves compatibility with strict OpenAI-compatible providers but may affect consumers that depend on the previous message shape. The full Review-relevant risksThe current diff changes user-visible model-message history behavior. Material changes in this area require independent human review under repository policy. No public API, security, licensing, release, or governance effect was identified in the current diff. The person performing the merge must review the final diff. A maintainer makes the final determination. WalkthroughThe history summarizer now groups tool calls by assistant step, buffers related results, supports interleaved calls, and preserves separate legacy steps. Tests cover parallel calls, explicit step IDs, and result ordering. ChangesTool-call history grouping
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change groups parallel tool calls into one assistant message so compacted histories remain acceptable to strict providers. It is localized and covered by regression tests, with no actionable merge-blocking risk remaining beyond normal checks. Sequence Diagram(s)sequenceDiagram
participant PlanItems
participant replayPlanItemsToModelMessages
participant ModelMessages
PlanItems->>replayPlanItemsToModelMessages: replay tool calls and results
replayPlanItemsToModelMessages->>replayPlanItemsToModelMessages: group calls and buffer results
replayPlanItemsToModelMessages->>ModelMessages: emit one assistant tool-call message
replayPlanItemsToModelMessages->>ModelMessages: emit ordered tool-result messages
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
M4n5ter
left a comment
There was a problem hiding this comment.
LGTM. I have manually reviewed the final diff and the validation evidence, and I approve this change.
…xtures Same rebase-only treatment as the provider-native fixture: apache#3038's grouping tests stub free-form text this PR's validation rejects by design; they assert on the grouped messages, not the returned text. Generated-by: Claude Code
…xtures Same rebase-only treatment as the provider-native fixture: apache#3038's grouping tests stub free-form text this PR's validation rejects by design; they assert on the grouped messages, not the returned text. Generated-by: Claude Code
…xtures Same rebase-only treatment as the provider-native fixture: apache#3038's grouping tests stub free-form text this PR's validation rejects by design; they assert on the grouped messages, not the returned text. Generated-by: Claude Code
Summary
Fixes #3030.
replayPlanItemsToModelMessagesmapped everytool_callreplay item to its own assistant message, so a step with parallel tool calls producedStrict OpenAI-compatible providers 400 on that shape, so mid-turn history compaction could never succeed on such providers — the
provider_errorfail-open loop that preceded the #3029 incident.What changed: grouping is step-based with results buffered to the step boundary, mirroring the step-merge invariant the primary replay path gets from the materializer (
model-history.ts). Membership follows the stampedstepIdwhen both sides carry one; legacy items without astepIdjoin while the open step still has unsettled calls, and a call arriving after the open step fully settled opens its own message. Results for calls in the open step are deferred and emitted in arrival order at the step boundary, so interleaved orderings likecall A, call B, result A, call C, result B, result Cmaterialize asassistant [A, B, C] → tool A → tool B → tool C. Skippedthinkingitems do not break a step. The primary replay path is untouched.Verification
@maka/runtimesuite passes locally (re-verified after rebasing ontomainpast feat(runtime): add Codex server-side history compaction #2993).AI use
Select exactly one:
Tool(s) and scope: Claude Code authored the fix, the tests, and this description under human direction and review; commits carry
Generated-by: Claude Codetrailers.Checklist
Does this PR entail a change in behavior?