-
Notifications
You must be signed in to change notification settings - Fork 181
Add tool description to tool start event #880
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
Conversation
WalkthroughAdds tool-specific metadata to START_TOOL events in the streaming path (call_stream) by parsing tool arguments, resolving the tool, computing a parameterized description, and including params and description in the emitted stream payload. No changes to invocation, error handling, or non-streaming logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant L as LLM
participant O as Orchestrator (call_stream)
participant R as ToolRegistry
participant S as StreamSink
participant T as ToolExecutor
L->>O: Emit tool calls (name, id, arguments)
loop For each tool call
O->>O: Parse arguments -> tool_params (json)
O->>R: Resolve tool by name
R-->>O: Tool reference
O->>O: Compute description via get_parameterized_one_liner(tool_params)
O-->>S: START_TOOL {id, tool_name, params, description}
O->>T: Schedule/execute tool
T-->>O: Tool result/stream
O-->>S: TOOL_OUTPUT / END_TOOL
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 (
|
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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
holmes/core/tool_calling_llm.py (2)
701-715: Optional: DRY parsing/description logic with _invoke_toolThe new argument parsing and description resolution duplicates logic in _invoke_tool (exception handling, hasattr checks). Consider extracting a small helper (e.g., _extract_tool_metadata(tool_call) -> tuple[name, params_dict, description_str]) and reuse in both places to avoid divergence.
I can draft this helper and wire it in both call_stream and _invoke_tool if you want.
710-715: Consider redacting params before streaming for securityYou now emit raw params on START_TOOL. If params may contain tokens, secrets, or PII, this increases exposure surface. You already sanitize for description via get_parameterized_one_liner; consider applying similar sanitization to the streamed params (or feature-flag it).
If sanitize_params is available from holmes.core.tools, you could apply it here before yielding.
Would you like me to add opt-in redaction (e.g., via a settings flag) and wire sanitize_params here?
📜 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(1 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
🧬 Code Graph Analysis (1)
holmes/core/tool_calling_llm.py (3)
holmes/core/tools_utils/tool_executor.py (1)
get_tool_by_name(60-64)holmes/core/tools.py (2)
get_parameterized_one_liner(178-179)get_parameterized_one_liner(205-212)holmes/utils/stream.py (2)
StreamMessage(18-20)StreamEvents(10-15)
⏰ 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). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (1)
holmes/core/tool_calling_llm.py (1)
710-715: Verify downstream consumers handle new fields and possible None descriptionWe’ve added two new keys—
paramsanddescription—to the START_TOOL event payload in holmes/core/tool_calling_llm.py (lines 709–715). A quick search didn’t reveal any in-repo consumers ofStreamEvents.START_TOOL, so it’s unclear whether UI components, analytics pipelines or other downstream handlers expect a fixed schema or non-nulldescription.Please manually verify that all consumers of this event:
- Properly deserialize the new
paramsfield- Safely handle a
null/Nonedescription
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.
Guard against invalid JSON and custom tool calls in streaming path
Two issues:
- json.loads on t.function.arguments is unguarded and can raise, breaking streaming.
- Assumes t.function exists; custom tool calls (non-function) will raise AttributeError. _invoke_tool correctly guards via hasattr; mirror that here.
Apply this safer parsing and resolution:
- tool_params = ( - json.loads(t.function.arguments) if t.function.arguments else {} - ) - tool = self.tool_executor.get_tool_by_name(t.function.name) - description = ( - tool.get_parameterized_one_liner(tool_params) if tool else None - ) + tool_name = None + tool_params = None + description = None + if hasattr(t, "function"): + tool_name = t.function.name + args_str = getattr(t.function, "arguments", None) + if args_str: + try: + parsed = json.loads(args_str) + tool_params = parsed if isinstance(parsed, dict) else {} + except Exception: + logging.warning( + f"Failed to parse arguments for tool: {tool_name}. args: {args_str}" + ) + tool_params = {} + tool_obj = self.tool_executor.get_tool_by_name(tool_name) + if tool_obj: + try: + description = tool_obj.get_parameterized_one_liner(tool_params or {}) + except Exception: + logging.exception( + f"Failed to compute parameterized description for tool: {tool_name}" + ) + else: + logging.error(f"Unsupported custom tool call: {t}") + tool_name = "unknown"
📝 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.
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.
Use computed tool_name and normalized params in START_TOOL payload
Follow-up to the parsing fix: rely on the precomputed variables and avoid direct t.function access. Also normalize params to a dict to keep payload shape stable.
- data={ - "tool_name": t.function.name, - "id": t.id, - "params": tool_params, - "description": description, - }, + data={ + "tool_name": tool_name, + "id": t.id, + "params": tool_params or {}, + "description": description, + },
📝 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.
🤖 Prompt for AI Agents
In holmes/core/tool_calling_llm.py around lines 710 to 715, the START_TOOL
payload is using t.function.name and raw tool_params directly; update it to use
the precomputed tool_name variable and a normalized params dict (e.g., ensure
params are converted/validated to a plain dict called normalized_params) so the
payload shape stays stable; replace "tool_name": t.function.name with
"tool_name": tool_name and "params": tool_params with "params":
normalized_params (or ensure tool_params is normalized to a dict before use).
No description provided.