Fix tool call numbering to maintain sequential order across iterations - #885
Conversation
WalkthroughIntroduces a tool_number_offset to ensure sequential tool_number values across batches and in both non-streaming (call) and streaming (call_stream) modes, incrementing the offset after each completed batch of tool invocations. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Caller
participant L as ToolCallingLLM
participant T as Tool(s)
rect rgb(245,248,255)
note over L: Initialize tool_number_offset = 0
U->>L: call(...) or call_stream(...)
alt Non-streaming
loop For each batch
L->>T: invoke(tool, tool_number = offset + local_index)
T-->>L: result
note over L: After batch completes<br/>offset += batch_tool_count
end
else Streaming
loop For each streamed batch
L->>T: invoke(tool, tool_number = offset + local_index)
T-->>L: partial result(s)
note over L: After batch completes<br/>offset += batch_tool_count
end
end
L-->>U: aggregated result(s)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
holmes/core/tool_calling_llm.py (3)
406-408: Sequential tool-number offset update: correct; prefer basing on futures count and add a debug range logUsing the batch size to advance the offset is right. To make this resilient to any future change that might skip scheduling some tools, increment by the number of scheduled futures and add a small debug log for traceability.
Apply this diff:
- # Update the tool number offset for the next iteration - tool_number_offset += len(tools_to_call) + # Update the tool number offset for the next iteration + num_tools_in_batch = len(futures) + tool_number_offset += num_tools_in_batch + logging.debug( + f"Assigned tool_number range {tool_number_offset - num_tools_in_batch + 1}..{tool_number_offset} for iteration {i}" + )
723-725: Increment offset after batch completion: base on scheduled futures and add debugSame nit as the non-streaming path: advance by the number of scheduled futures and emit a helpful debug line.
Apply this diff:
- # Update the tool number offset for the next iteration - tool_number_offset += len(tools_to_call) + # Update the tool number offset for the next iteration + num_tools_in_batch = len(futures) + tool_number_offset += num_tools_in_batch + logging.debug( + f"Assigned tool_number range {tool_number_offset - num_tools_in_batch + 1}..{tool_number_offset} for iteration {i}" + )
702-703: Surfacetool_numberin streaming eventsVerified findings:
- The only
invokedefinition is inholmes/core/tools_utils/tool_executor.py:49, which currently accepts only(tool_name: str, params: dict)and does not include atool_numberparameter.- There are no remaining references to
tool_indexin the codebase.- No existing streaming consumers (Python or JS/TS) handle
tool_numberinSTART_TOOLorTOOL_RESULTevents.Recommended optional refactoring:
holmes/core/tools_utils/tool_executor.py (line 49):
Changedef invoke(self, tool_name: str, params: dict) -> StructuredToolResult:to
def invoke(self, tool_name: str, params: dict, tool_number: int) -> StructuredToolResult:and propagate
tool_numberinto the underlying tool call.holmes/core/tool_calling_llm.py:
• After submitting each tool invocation, yield aStreamMessage(START_TOOL)withdata={"tool_name": t.function.name, "id": t.id, "tool_number": assigned_tool_number}• When emitting
TOOL_RESULT, include"tool_number": assigned_tool_numberin the payload.Downstream clients/UI:
Ensure any consumers ofSTART_TOOLandTOOL_RESULTextract and display the newtool_numberfield so tool calls are consistently ordered in the UI/telemetry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
holmes/core/tool_calling_llm.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Type hints are required (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
holmes/core/tool_calling_llm.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (1)
holmes/core/tool_calling_llm.py (1)
608-609: Initialize tool_number_offset in streaming path — LGTMThis correctly resets numbering per streaming session and aligns with the non-streaming behavior.
2 participants
Before
After