Skip to content

ROB-1933: make sure approved or denied tool call triggers a SSE event #1054

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.

Sign up for GitHub

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

Merged
nherment merged 2 commits into from
Oct 15, 2025
Merged

ROB-1933: make sure approved or denied tool call triggers a SSE event #1054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c1dba5
fix: make sure approved or denied tool call triggers a SSE event
nherment Oct 13, 2025
e267b9a
chore: address PR comments
nherment Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 45 additions & 34 deletions holmes/core/tool_calling_llm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ def __init__(

def process_tool_decisions(
self, messages: List[Dict[str, Any]], tool_decisions: List[ToolApprovalDecision]
) -> List[Dict[str, Any]]:
) -> tuple[List[Dict[str, Any]], list[StreamMessage]]:
"""
Process tool approval decisions and execute approved tools.

Expand All @@ -300,8 +300,9 @@ def process_tool_decisions(
Returns:
Updated messages list with tool execution results
"""
events: list[StreamMessage] = []
if not tool_decisions:
return messages
return messages, events

# Create decision lookup
decisions_by_tool_call_id = {
Expand Down Expand Up @@ -337,35 +338,35 @@ def process_tool_decisions(
tool_call_message: dict
tool_call = tool_call_with_decision.tool_call
decision = tool_call_with_decision.decision
tool_result: Optional[ToolCallResult] = None
if decision and decision.approved:
try:
llm_tool_result = self._invoke_llm_tool_call(
tool_to_call=tool_call,
previous_tool_calls=[],
trace_span=DummySpan(), # TODO: replace with proper span
tool_number=None,
user_approved=True,
)
tool_call_message = llm_tool_result.as_tool_call_message()

except Exception as e:
logging.error(
f"Failed to execute approved tool {tool_call.id}: {e}"
)
tool_call_message = {
"tool_call_id": tool_call.id,
"role": "tool",
"name": tool_call.function.name,
"content": f"Tool execution failed: {str(e)}",
}
tool_result = self._invoke_llm_tool_call(
tool_to_call=tool_call,
previous_tool_calls=[],
trace_span=DummySpan(), # TODO: replace with proper span
tool_number=None,
user_approved=True,
)
else:
# Tool was rejected or no decision found, add rejection message
tool_call_message = {
"tool_call_id": tool_call.id,
"role": "tool",
"name": tool_call.function.name,
"content": "Tool execution was denied by the user.",
}
tool_result = ToolCallResult(
tool_call_id=tool_call.id,
tool_name=tool_call.function.name,
description=tool_call.function.name,
result=StructuredToolResult(
status=StructuredToolResultStatus.ERROR,
error="Tool execution was denied by the user.",
),
)

events.append(
StreamMessage(
event=StreamEvents.TOOL_RESULT,
data=tool_result.as_streaming_tool_result_response(),
)
)

tool_call_message = tool_result.as_tool_call_message()

# It is expected that the tool call result directly follows the tool call request from the LLM
# The API call may contain a user ask which is appended to the messages so we can't just append
Expand All @@ -374,7 +375,7 @@ def process_tool_decisions(
tool_call_with_decision.message_index + 1, tool_call_message
)

return messages
return messages, events

def prompt_call(
self,
Expand Down Expand Up @@ -680,11 +681,13 @@ def _get_tool_call_result(
f"Failed to parse arguments for tool: {tool_name}. args: {tool_arguments}"
)

tool_response = prevent_overly_repeated_tool_call(
tool_name=tool_name,
tool_params=tool_params,
tool_calls=previous_tool_calls,
)
tool_response = None
if not user_approved:

@moshemorad moshemorad Oct 13, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why we changed it? if the user approved it but it was already ran once we still would like to run it?

@nherment nherment Oct 13, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed it because I noticed it. My thinking is that if a user actively approves a command then it should run, regardless of any restriction, including repeated commands.

@moshemorad moshemorad Oct 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure i fully agreed with that. maybe worth discuss it with product and make sure?

tool_response = prevent_overly_repeated_tool_call(
tool_name=tool_name,
tool_params=tool_params,
tool_calls=previous_tool_calls,
)

if not tool_response:
tool_response = self._directly_invoke_tool_call(
Expand Down Expand Up @@ -893,11 +896,19 @@ def call_stream(
sections: Optional[InputSectionsDataType] = None,
msgs: Optional[list[dict]] = None,
enable_tool_approval: bool = False,
tool_decisions: List[ToolApprovalDecision] | None = None,
):
"""
This function DOES NOT call llm.completion(stream=true).
This function streams holmes one iteration at a time instead of waiting for all iterations to complete.
"""

# Process tool decisions if provided
if msgs and tool_decisions:
logging.info(f"Processing {len(tool_decisions)} tool decisions")
msgs, events = self.process_tool_decisions(msgs, tool_decisions)
yield from events

messages: list[dict] = []
if system_prompt:
messages.append({"role": "system", "content": system_prompt})
Expand Down
7 changes: 1 addition & 6 deletions server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,12 +335,6 @@ def chat(chat_request: ChatRequest):
additional_system_prompt=chat_request.additional_system_prompt,
)

# Process tool decisions if provided
if chat_request.tool_decisions:
logging.info(
f"Processing {len(chat_request.tool_decisions)} tool decisions"
)
messages = ai.process_tool_decisions(messages, chat_request.tool_decisions)
follow_up_actions = []
if not already_answered(chat_request.conversation_history):
follow_up_actions = [
Expand Down Expand Up @@ -370,6 +364,7 @@ def chat(chat_request: ChatRequest):
ai.call_stream(
msgs=messages,
enable_tool_approval=chat_request.enable_tool_approval or False,
tool_decisions=chat_request.tool_decisions,
),
[f.model_dump() for f in follow_up_actions],
),
Expand Down
42 changes: 24 additions & 18 deletions tests/test_approval_workflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,15 +260,18 @@ def test_streaming_chat_approval_workflow_approve_and_execute(

# Mock process_tool_decisions to simulate approval and execution
ai.process_tool_decisions = MagicMock(
side_effect=lambda messages, tool_decisions: messages
+ [
{
"tool_call_id": "tool_call_123",
"role": "tool",
"name": "kubectl_delete",
"content": "pod 'dangerous-pod' deleted",
}
]
side_effect=lambda messages, tool_decisions: (
messages
+ [
{
"tool_call_id": "tool_call_123",
"role": "tool",
"name": "kubectl_delete",
"content": "pod 'dangerous-pod' deleted",
}
],
[], # Empty list for StreamMessages
)
)

mock_create_toolcalling_llm.return_value = ai
Expand Down Expand Up @@ -403,15 +406,18 @@ def test_streaming_chat_approval_workflow_reject_command(

# Mock process_tool_decisions to simulate rejection
ai.process_tool_decisions = MagicMock(
side_effect=lambda messages, tool_decisions: messages
+ [
{
"tool_call_id": "tool_call_123",
"role": "tool",
"name": "kubectl_delete",
"content": "Tool execution was denied by the user.",
}
]
side_effect=lambda messages, tool_decisions: (
messages
+ [
{
"tool_call_id": "tool_call_123",
"role": "tool",
"name": "kubectl_delete",
"content": "Tool execution was denied by the user.",
}
],
[], # Empty list for StreamMessages
)
)

mock_create_toolcalling_llm.return_value = ai
Expand Down
Loading