ROB-1933: issues with SaaS approval flow - #1037
Conversation
WalkthroughIntroduces ToolCallWithDecision and refactors tool-call approval flow in holmes/core/tool_calling_llm.py to collect assistant tool_calls with decisions, propagate user_approved through execution, unify insertion of tool_call messages, and add a lookup helper. holmes/core/conversations.py has only whitespace edits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Orchestrator as ToolCallingLLM
participant LLM as Chat Model
participant Decider as Approval Decisions
participant Tools as LLM Tool(s)
User->>Orchestrator: call()/call_stream(messages)
Orchestrator->>LLM: send conversation
LLM-->>Orchestrator: assistant message with tool_calls
Note over Orchestrator: Collect assistant tool_calls<br/>Pair with decisions → ToolCallWithDecision[]
Orchestrator->>Decider: fetch decisions by tool_call_id
Decider-->>Orchestrator: approve/reject per tool_call
alt Approved
loop each approved ToolCallWithDecision
Orchestrator->>Orchestrator: find_assistant_tool_call_request(tool_call_id)
Orchestrator->>Tools: _invoke_llm_tool_call(..., user_approved=true)
Tools-->>Orchestrator: ToolCallResult
Orchestrator->>Orchestrator: append tool_call message with result (after originating message)
end
else Rejected
Orchestrator->>Orchestrator: append rejection message after originating assistant message
end
Orchestrator-->>User: updated messages (including tool_call results or rejections)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
holmes/core/tool_calling_llm.py (3)
321-324: Create a custom exception class for better error handling.As per coding guidelines and static analysis hints, avoid creating generic exceptions with string messages inline. This makes error handling less precise and harder to test.
Based on static analysis hints, consider creating a custom exception class:
class ToolDecisionError(Exception): """Raised when tool decisions cannot be processed correctly.""" passThen update the code:
if not pending_tool_calls: - error_message = f"Received {len(tool_decisions)} tool decisions but no pending approvals found" - logging.error(error_message) - raise Exception(error_message) + error_message = f"Received {len(tool_decisions)} tool decisions but no pending approvals found" + logging.error(error_message) + raise ToolDecisionError(error_message)
342-351: Improve exception handling specificity and logging.Based on static analysis hints, the exception handling here could be more specific, and
logging.exceptionshould be used instead oflogging.errorfor better stack trace capture.Apply these improvements:
except Exception as e: - logging.error( - f"Failed to execute approved tool {tool_call_with_decision.tool_call.id}: {e}" - ) + logging.exception( + f"Failed to execute approved tool {tool_call_with_decision.tool_call.id}" + ) tool_call_message = { "tool_call_id": tool_call_with_decision.tool_call.id, "role": "tool", "name": tool_call_with_decision.tool_call.function.name, - "content": f"Tool execution failed: {str(e)}", + "content": f"Tool execution failed: {e!s}", }
1103-1116: Improve exception handling in helper method.Based on static analysis hints, avoid generic exceptions with long inline messages.
Apply these improvements:
def find_assistant_tool_call_request( self, tool_call_id: str, messages: list[dict[str, Any]] ) -> dict[str, Any]: + """Find a tool call request in the conversation history by tool_call_id. + + Args: + tool_call_id: The ID of the tool call to find + messages: List of conversation messages + + Returns: + The tool call dictionary + + Raises: + ToolCallNotFoundError: If the tool call is not found in the conversation + """ for message in messages: if message.get("role") == "assistant": for tool_call in message.get("tool_calls", []): if tool_call.get("id") == tool_call_id: return tool_call - # Should not happen unless there is a bug. - # If we are here - raise Exception( - f"Failed to find assistant request for a tool_call in conversation history. tool_call_id={tool_call_id}" - ) + raise ToolCallNotFoundError( + f"Tool call not found in conversation history: {tool_call_id}" + )Define the custom exception at the module level:
class ToolCallNotFoundError(Exception): """Raised when a tool call cannot be found in the conversation history.""" pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
holmes/core/conversations.py(1 hunks)holmes/core/tool_calling_llm.py(10 hunks)server.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
server.pyholmes/core/tool_calling_llm.pyholmes/core/conversations.py
🧬 Code graph analysis (2)
holmes/core/tool_calling_llm.py (2)
holmes/core/models.py (2)
ToolApprovalDecision(177-181)as_tool_call_message(30-36)holmes/core/tracing.py (1)
DummySpan(101-122)
holmes/core/conversations.py (1)
holmes/utils/global_instructions.py (1)
add_global_instructions_to_user_prompt(10-20)
🪛 Ruff (0.13.3)
holmes/core/tool_calling_llm.py
324-324: Create your own exception
(TRY002)
342-342: Do not catch blind exception: Exception
(BLE001)
343-345: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
350-350: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1114-1116: Create your own exception
(TRY002)
1114-1116: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (5)
holmes/core/conversations.py (1)
374-381: LGTM! Conditional user message append prevents empty entries.This change correctly ensures that user messages are only appended when
askhas a truthy value, preventing empty orNonemessages from entering the conversation history. This aligns well with the structured tool-call approval flow introduced in this PR.holmes/core/tool_calling_llm.py (3)
260-264: LGTM! Well-structured model for pairing tool calls with decisions.The
ToolCallWithDecisionclass provides a clean way to associate tool calls with their approval decisions and track their position in the message history.
652-652: LGTM! Theuser_approvedparameter propagates approval state correctly.The addition of the
user_approvedparameter to_get_tool_call_resultand_invoke_llm_tool_callproperly threads the approval state through the tool execution pipeline, enabling tools to behave differently based on whether they were pre-approved or user-approved.Also applies to: 674-674, 719-719, 752-752
1075-1093: LGTM! Tool approval marking logic is clear and correct.The code properly iterates through approval-required tools, locates their corresponding tool_calls in the message history using the helper method, and marks them with
pending_approval=Truebefore ending the stream.However, the debug print on line 1093 should be replaced with logging:
- print(f"***FINAL MESSAGES***\n{json.dumps(messages[1:])}") + logging.debug(f"Final messages:\n{json.dumps(messages[1:])}")server.py (1)
328-329: Replace print with logging and clarify global instructions intent.Apply this diff in
server.pyat lines 327–329:- global_instructions = None # dal.get_global_instructions_for_account() - print(chat_request.model_dump_json()) + global_instructions = None # TODO: temporarily disabled for SaaS approval flow testing + logging.debug(f"Chat request: {chat_request.model_dump_json()}")Confirm whether disabling
get_global_instructions_for_account()here is intentional and temporary, or if it should be re-enabled.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
holmes/core/tool_calling_llm.py (1)
303-320: Preserve tool-result insertion order for multiple tool_calls in the same assistant message.Repeated insert at message_index + 1 reverses order when a single assistant message has multiple tool_calls. Use a per-message offset to keep original order.
Apply this diff:
@@ - for tool_call_with_decision in pending_tool_calls: + # Track how many tool results we've inserted per assistant message, + # so multiple results for the same message preserve original order. + inserted_per_message: dict[int, int] = {} + for tool_call_with_decision in pending_tool_calls: @@ - messages.insert( - tool_call_with_decision.message_index + 1, tool_call_message - ) + msg_idx = tool_call_with_decision.message_index + offset = inserted_per_message.get(msg_idx, 0) + insertion_idx = msg_idx + 1 + offset + messages.insert(insertion_idx, tool_call_message) + inserted_per_message[msg_idx] = offset + 1Also add a brief inline comment above the reversed(range(...)) loop to explain that reverse iteration avoids index shifting issues when inserting after the assistant message.
Also applies to: 326-364
🧹 Nitpick comments (3)
holmes/core/tool_calling_llm.py (3)
321-325: Use a dedicated exception type for decision/message mismatch.Avoid bare Exception; define a specific error for clarity and Ruff compliance.
Apply this diff:
- if not pending_tool_calls: - error_message = f"Received {len(tool_decisions)} tool decisions but no pending approvals found" - logging.error(error_message) - raise Exception(error_message) + if not pending_tool_calls: + error_message = f"Received {len(tool_decisions)} tool decisions but no pending approvals found" + logging.error(error_message) + raise ToolApprovalMismatchError(error_message)Add this exception class near the top of the module (e.g., after TRUNCATION_NOTICE):
class ToolApprovalMismatchError(Exception): """Tool decisions supplied but no pending approvals were found in messages."""As per static analysis hints
339-348: Prefer logging.exception and explicit conversion for error paths.Use logging.exception to capture stack trace and explicit conversion in the user-facing string.
Apply this diff:
- except Exception as e: - logging.error( - f"Failed to execute approved tool {tool_call_with_decision.tool_call.id}: {e}" - ) + except Exception as e: + logging.exception( + "Failed to execute approved tool %s", + tool_call_with_decision.tool_call.id, + ) tool_call_message = { "tool_call_id": tool_call_with_decision.tool_call.id, "role": "tool", "name": tool_call_with_decision.tool_call.function.name, - "content": f"Tool execution failed: {str(e)}", + "content": f"Tool execution failed: {e!s}", }As per static analysis hints
1102-1116: Raise a custom exception when tool_call not found; minor perf note.Use a specific exception; optional: consider caching id→tool_call lookup if this is on a hot path.
Apply this diff:
- raise Exception( - f"Failed to find assistant request for a tool_call in conversation history. tool_call_id={tool_call_id}" - ) + raise ToolCallRequestNotFoundError( + f"Assistant request for tool_call_id={tool_call_id} not found in conversation history." + )Add this exception class near the top of the module:
class ToolCallRequestNotFoundError(Exception): """Assistant tool_call request not found in conversation history."""As per static analysis hints
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
holmes/core/conversations.py(1 hunks)holmes/core/tool_calling_llm.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/core/conversations.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/core/tool_calling_llm.py
🧬 Code graph analysis (1)
holmes/core/tool_calling_llm.py (2)
holmes/core/models.py (2)
ToolApprovalDecision(177-181)as_tool_call_message(30-36)holmes/core/tracing.py (1)
DummySpan(101-122)
🪛 Ruff (0.13.3)
holmes/core/tool_calling_llm.py
324-324: Create your own exception
(TRY002)
339-339: Do not catch blind exception: Exception
(BLE001)
340-342: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
347-347: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1113-1115: Create your own exception
(TRY002)
1113-1115: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (4)
holmes/core/tool_calling_llm.py (4)
260-264: New ToolCallWithDecision model looks good.Clear fields and typing; fits usage below.
653-676: Propagating user_approved into tool execution: LGTM.Correctly threads through to invocation context.
720-721: Default user_approved=False in _invoke_llm_tool_call: LGTM.Backward compatible and explicit.
1075-1080: Marking assistant tool_calls with pending_approval: LGTM.Correctly tags the original tool_call in messages for later processing.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
holmes/core/tool_calling_llm.py (1)
303-319: Document the reverse iteration pattern.As noted in previous reviews, the reverse iteration ensures that when tool results are inserted at
message_index + 1, higher indices are processed first, preventing index shifts that would affect subsequent insertions.Add a brief inline comment above the loop:
+ # Iterate in reverse so that inserting tool results at message_index + 1 + # doesn't shift indices of earlier messages for i in reversed(range(len(messages))):
🧹 Nitpick comments (4)
holmes/core/tool_calling_llm.py (4)
321-324: Use a custom exception class.The generic
Exceptionmakes it harder to handle this specific error case programmatically.Based on coding guidelines, create a custom exception:
+class NoToolApprovalsFoundError(Exception): + """Raised when tool decisions are provided but no pending approvals are found.""" + pass + # Then in process_tool_decisions: if not pending_tool_calls: - error_message = f"Received {len(tool_decisions)} tool decisions but no pending approvals found" - logging.error(error_message) - raise Exception(error_message) + logging.error(f"Received {len(tool_decisions)} tool decisions but no pending approvals found") + raise NoToolApprovalsFoundError( + f"Received {len(tool_decisions)} tool decisions but no pending approvals found" + )
341-350: Improve exception logging.Use
logging.exceptionto automatically capture the stack trace, and avoid f-string conversion warnings.Apply this diff:
except Exception as e: - logging.error( - f"Failed to execute approved tool {tool_call.id}: {e}" - ) + logging.exception( + "Failed to execute approved tool %s: %s", + tool_call.id, + str(e), + ) tool_call_message = { "tool_call_id": tool_call.id, "role": "tool", "name": tool_call.function.name, - "content": f"Tool execution failed: {str(e)}", + "content": f"Tool execution failed: {e!s}", }
1104-1106: Add return type hint.Per coding guidelines, type hints are required for all functions.
Apply this diff:
def find_assistant_tool_call_request( - self, tool_call_id: str, messages: list[dict[str, Any]] - ) -> dict[str, Any]: + self, tool_call_id: str, messages: list[dict[str, Any]] + ) -> dict[str, Any]: + """ + Locate a specific tool_call in the conversation history by its ID. + + Args: + tool_call_id: The ID of the tool call to find + messages: The conversation message history + + Returns: + The tool_call dictionary from the assistant's message + + Raises: + Exception: If the tool call is not found in the message history + """
1115-1117: Use a custom exception class.The generic
Exceptionwith a long message makes error handling less precise.Create a custom exception:
+class ToolCallNotFoundError(Exception): + """Raised when a tool call cannot be found in message history.""" + pass + # Then in find_assistant_tool_call_request: - raise Exception( - f"Failed to find assistant request for a tool_call in conversation history. tool_call_id={tool_call_id}" - ) + raise ToolCallNotFoundError( + f"Tool call not found in conversation history: {tool_call_id}" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/tool_calling_llm.py(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/core/tool_calling_llm.py
🧬 Code graph analysis (1)
holmes/core/tool_calling_llm.py (2)
holmes/core/models.py (2)
ToolApprovalDecision(177-181)as_tool_call_message(30-36)holmes/core/tracing.py (1)
DummySpan(101-122)
🪛 Ruff (0.13.3)
holmes/core/tool_calling_llm.py
324-324: Create your own exception
(TRY002)
341-341: Do not catch blind exception: Exception
(BLE001)
342-344: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
349-349: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1115-1117: Create your own exception
(TRY002)
1115-1117: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (3)
holmes/core/tool_calling_llm.py (3)
260-264: LGTM!The
ToolCallWithDecisiondata structure cleanly encapsulates the relationship between a tool call, its location in the message history, and its approval decision. This design improves type safety and makes the approval workflow more maintainable.
655-677: LGTM!The
user_approvedparameter is cleanly propagated through the tool invocation chain with appropriate defaults. This enables tools to distinguish between automatically approved and user-approved executions.Also applies to: 722-755
1077-1081: LGTM!The refactor to use
find_assistant_tool_call_requestimproves code organization and eliminates duplication in locating tool calls within the message history.
2 participants
CLI approval flow was also manually verified after these changes