diff --git a/holmes/core/models.py b/holmes/core/models.py index 9dee59fe9..7bc19db60 100644 --- a/holmes/core/models.py +++ b/holmes/core/models.py @@ -124,7 +124,6 @@ class ToolApprovalDecision(BaseModel): save_prefixes: Optional[List[str]] = None # Prefixes to remember for session feedback: Optional[str] = None # User feedback when denying a tool call decision: Optional[Dict[str, Any]] = None # Structured decision data (e.g. OAuth callback) - edit_command: Optional[str] = None # If set, replaces the tool call's "command" argument before execution class OAuthCallbackRequest(BaseModel): diff --git a/holmes/core/tool_calling_llm.py b/holmes/core/tool_calling_llm.py index d3e034c19..ba9131663 100644 --- a/holmes/core/tool_calling_llm.py +++ b/holmes/core/tool_calling_llm.py @@ -324,26 +324,6 @@ def _execute_tool_decisions( ) if not tool_result: - if tool_decision.edit_command is not None: - try: - edited_params = json.loads(tool_call.function.arguments or "{}") - except json.JSONDecodeError: - edited_params = {} - edited_params["command"] = tool_decision.edit_command - edited_arguments = json.dumps(edited_params) - tool_call.function.arguments = edited_arguments - # Persist the edited command in the conversation history so - # subsequent turns see the command that was actually executed. - msg_tool_calls = messages[ - tool_call_with_decision.message_index - ].get("tool_calls", []) - for original_tool_call in msg_tool_calls: - if original_tool_call.get("id") == tool_call.id: - original_function = original_tool_call.get("function") or {} - original_function["arguments"] = edited_arguments - original_tool_call["function"] = original_function - break - tool_result = self._invoke_llm_tool_call( tool_to_call=tool_call, previous_tool_calls=[], diff --git a/tests/core/conversations_worker/integration/test_conversation_integration.py b/tests/core/conversations_worker/integration/test_conversation_integration.py index 40b583006..06c63f7e0 100644 --- a/tests/core/conversations_worker/integration/test_conversation_integration.py +++ b/tests/core/conversations_worker/integration/test_conversation_integration.py @@ -212,179 +212,6 @@ def test_approval_pause_and_resume(self, supabase_fx: SupabaseFixture): f"Approval + answer should compact multiple prior rows; got {stats}" ) - def test_approval_with_edit_command(self, supabase_fx: SupabaseFixture): - """When the user approves a pending tool call with an `edit_command` - override, the worker must execute the edited command (not the - original) and the edited command must appear both in the - TOOL_RESULT event and in the conversation history attached to - the AI_ANSWER_END terminal event.""" - verification_code = "HOLMES_INTEG_EDIT_42_X9K2M" - edited_command = f"echo {verification_code}" - - # Turn 1: force a bash call by asking for an URL that needs the shell. - # We don't care what command Holmes picks because we'll override it. - conv = supabase_fx.create_conversation( - ask=( - "Run the bash command `curl -sf -H 'Authorization: ApiKey ENV_KEY' " - "https://example.invalid/no-op || echo done` to confirm a simple " - "shell works. You MUST use the bash tool." - ), - title="integ: tool-approval-edit-command", - enable_tool_approval=True, - ) - cid = conv["conversation_id"] - supabase_fx.wait_for_terminal(cid, request_sequence=1, timeout=120) - - terminal1 = supabase_fx.find_terminal_event(cid) - assert terminal1 is not None - assert terminal1["event"] == "approval_required" - pending = terminal1["data"].get("pending_approvals") or [] - assert len(pending) > 0, "Must have at least one pending approval" - - # Sanity: the original command Holmes picked is NOT our verification - # code, so any later sighting can only come from the edit override. - for p in pending: - original_cmd = (p.get("params") or {}).get("command", "") - assert verification_code not in original_cmd, ( - f"verification code leaked into original command: {original_cmd}" - ) - - # Turn 2: approve, but override the bash command for every pending - # call. Only the bash tool understands "command", so we only set - # edit_command on bash decisions. - tool_decisions = [] - for p in pending: - decision = { - "tool_call_id": p["tool_call_id"], - "approved": True, - "save_prefixes": None, - "feedback": None, - } - if p.get("tool_name") == "bash": - decision["edit_command"] = edited_command - tool_decisions.append(decision) - - # The test is only meaningful if a bash tool call was pending and we - # actually attached an edit_command override to its decision. Fail - # loudly if not, so the cause is obvious instead of surfacing as a - # confusing StopIteration / "edited command not found" later on. - edited_id = next( - (p["tool_call_id"] for p in pending if p.get("tool_name") == "bash"), - None, - ) - assert edited_id is not None, ( - f"Expected a bash tool call in pending approvals, got: " - f"{[p.get('tool_name') for p in pending]}" - ) - assert any("edit_command" in d for d in tool_decisions), ( - "Expected at least one tool_decision to carry an edit_command " - f"override; built decisions: {tool_decisions}" - ) - - now_iso = datetime.now(timezone.utc).isoformat() - followup = supabase_fx.post_followup( - conversation_id=cid, - events=[ - { - "event": "user_message", - "data": { - "tool_decisions": tool_decisions, - "enable_tool_approval": True, - }, - "ts": now_iso, - } - ], - ) - result = supabase_fx.wait_for_terminal( - cid, request_sequence=followup["request_sequence"], timeout=180 - ) - assert result["status"] == "completed" - - # Holmes may issue further tool calls before producing ai_answer_end - # (the unfamiliar verification string can encourage extra - # investigation). Auto-approve any follow-up tool calls verbatim - # until we reach ai_answer_end. - for _ in range(5): - term = supabase_fx.find_terminal_event(cid) - assert term is not None - if term["event"] == "ai_answer_end": - break - assert term["event"] == "approval_required", ( - f"Unexpected terminal event {term['event']}" - ) - more_pending = (term.get("data") or {}).get("pending_approvals") or [] - assert more_pending, "approval_required without pending approvals" - now_iso = datetime.now(timezone.utc).isoformat() - followup = supabase_fx.post_followup( - conversation_id=cid, - events=[ - { - "event": "user_message", - "data": { - "tool_decisions": [ - { - "tool_call_id": p["tool_call_id"], - "approved": True, - "save_prefixes": None, - "feedback": None, - } - for p in more_pending - ], - "enable_tool_approval": True, - }, - "ts": now_iso, - } - ], - ) - result = supabase_fx.wait_for_terminal( - cid, request_sequence=followup["request_sequence"], timeout=180 - ) - assert result["status"] == "completed" - tool_result_ev = None - for row in supabase_fx.get_events(cid): - for ev in row.get("events") or []: - if ( - ev.get("event") == "tool_calling_result" - and (ev.get("data") or {}).get("tool_call_id") == edited_id - ): - tool_result_ev = ev - assert tool_result_ev is not None, ( - "tool_calling_result for edited tool call not found" - ) - result_params = ( - (tool_result_ev.get("data") or {}).get("result") or {} - ).get("params") or {} - assert result_params.get("command") == edited_command, ( - f"TOOL_RESULT params.command must be the edited command, " - f"got: {result_params!r}" - ) - - # The ai_answer_end terminal event includes the conversation_history - # ("messages"). The assistant message that originally requested the - # bash call must now reflect the edited command. - terminal2 = supabase_fx.find_terminal_event(cid) - assert terminal2 is not None and terminal2["event"] == "ai_answer_end" - history = (terminal2.get("data") or {}).get("messages") or [] - found_edited = False - for msg in history: - if msg.get("role") != "assistant": - continue - for tc in msg.get("tool_calls") or []: - if tc.get("id") != edited_id: - continue - args_raw = (tc.get("function") or {}).get("arguments") or "{}" - try: - args = json.loads(args_raw) - except json.JSONDecodeError: - args = {} - if args.get("command") == edited_command: - found_edited = True - assert found_edited, ( - "ai_answer_end conversation_history must contain the edited command " - f"on tool_call {edited_id}" - ) - - # --------------------------------------------------------------------------- # 4. Stop conversation (ConversationReassignedError) # --------------------------------------------------------------------------- diff --git a/tests/test_edit_command_removed.py b/tests/test_edit_command_removed.py new file mode 100644 index 000000000..b1ec04dee --- /dev/null +++ b/tests/test_edit_command_removed.py @@ -0,0 +1,102 @@ +"""Regression: `edit_command` was removed from the tool-approval flow. + +This test pins the backwards-compat guarantee: + +1. Older clients that still POST `edit_command` in their `tool_decisions` + payload are silently accepted - Pydantic v2's default `extra="ignore"` + drops the unknown field at deserialization. No 422, no warning. +2. The executed tool sees the ORIGINAL command from the assistant + tool_call. The edit substitution is gone - the LLM-proposed command + is what runs. +""" + +import json +from unittest.mock import MagicMock + +from holmes.core.models import ToolApprovalDecision, ToolCallResult +from holmes.core.tool_calling_llm import ToolCallingLLM +from holmes.core.tools import StructuredToolResult, StructuredToolResultStatus + + +def _build_ai() -> ToolCallingLLM: + return ToolCallingLLM( + tool_executor=MagicMock(), + max_steps=5, + llm=MagicMock(), + tool_results_dir=None, + ) + + +def _make_messages(tool_call_id: str, original_command: str) -> list: + return [ + {"role": "user", "content": "do something"}, + { + "role": "assistant", + "content": "I'll run a command", + "tool_calls": [ + { + "id": tool_call_id, + "type": "function", + "function": { + "name": "bash", + "arguments": json.dumps({"command": original_command}), + }, + "pending_approval": True, + } + ], + }, + ] + + +def test_edit_command_in_payload_is_silently_dropped_by_pydantic(): + """An old client still sending `edit_command` must not break — Pydantic + drops the unknown field at the boundary.""" + raw_payload = { + "tool_call_id": "tc1", + "approved": True, + "edit_command": "rm -rf /tmp/foo", + } + decision = ToolApprovalDecision.model_validate(raw_payload) + + assert decision.tool_call_id == "tc1" + assert decision.approved is True + assert not hasattr(decision, "edit_command") + + +def test_original_command_runs_even_when_edit_command_was_sent(): + """An old client POSTing `edit_command="rm -rf /tmp/foo"` over an + approved `command="ls"` tool_call must see `ls` run — not the + substituted command. The edit substitution is gone.""" + ai = _build_ai() + original = "ls" + messages = _make_messages("tc1", original) + + captured = {} + + def fake_invoke(*, tool_to_call, **kwargs): + captured["arguments"] = tool_to_call.function.arguments + params = json.loads(tool_to_call.function.arguments) + return ToolCallResult( + tool_call_id=tool_to_call.id, + tool_name=tool_to_call.function.name, + description="mocked", + result=StructuredToolResult( + status=StructuredToolResultStatus.SUCCESS, + data="ok", + params=params, + ), + ) + + ai._invoke_llm_tool_call = MagicMock(side_effect=fake_invoke) + + decision = ToolApprovalDecision.model_validate( + { + "tool_call_id": "tc1", + "approved": True, + "edit_command": "rm -rf /tmp/foo", + } + ) + ai._execute_tool_decisions(messages=messages, tool_decisions=[decision]) + + assert "arguments" in captured, "_invoke_llm_tool_call was not called" + assert json.loads(captured["arguments"])["command"] == original diff --git a/tests/test_tool_decision_edit_command.py b/tests/test_tool_decision_edit_command.py deleted file mode 100644 index 825c11f36..000000000 --- a/tests/test_tool_decision_edit_command.py +++ /dev/null @@ -1,180 +0,0 @@ -"""Unit tests for the `edit_command` field on `ToolApprovalDecision`. - -When an approved tool decision carries `edit_command`, the worker must: - -1. Execute the tool with the edited command instead of the original. -2. Persist the edited command back into the conversation history (the - assistant message's `tool_calls`) so subsequent turns and the - ANSWER_END message reflect what actually ran. -3. Emit the edited command in the TOOL_RESULT stream event. -""" - -import json -from unittest.mock import MagicMock - -from holmes.core.models import ToolApprovalDecision, ToolCallResult -from holmes.core.tool_calling_llm import ToolCallingLLM -from holmes.core.tools import StructuredToolResult, StructuredToolResultStatus -from holmes.utils.stream import StreamEvents - - -def _make_messages(tool_call_id: str, original_command: str) -> list: - return [ - {"role": "user", "content": "do something"}, - { - "role": "assistant", - "content": "I'll run a command", - "tool_calls": [ - { - "id": tool_call_id, - "type": "function", - "function": { - "name": "bash", - "arguments": json.dumps({"command": original_command}), - }, - "pending_approval": True, - } - ], - }, - ] - - -def _build_ai() -> ToolCallingLLM: - return ToolCallingLLM( - tool_executor=MagicMock(), - max_steps=5, - llm=MagicMock(), - tool_results_dir=None, - ) - - -def test_edit_command_replaces_command_in_executed_tool_call(): - ai = _build_ai() - original = "kubectl delete pod dangerous" - edited = "kubectl get pods -n default" - messages = _make_messages("tc1", original) - - captured = {} - - def fake_invoke(*, tool_to_call, **kwargs): - captured["arguments"] = tool_to_call.function.arguments - params = json.loads(tool_to_call.function.arguments) - return ToolCallResult( - tool_call_id=tool_to_call.id, - tool_name=tool_to_call.function.name, - description="mocked", - result=StructuredToolResult( - status=StructuredToolResultStatus.SUCCESS, - data="ok", - params=params, - ), - ) - - ai._invoke_llm_tool_call = MagicMock(side_effect=fake_invoke) - - decision = ToolApprovalDecision( - tool_call_id="tc1", approved=True, edit_command=edited - ) - ai._execute_tool_decisions(messages=messages, tool_decisions=[decision]) - - assert "arguments" in captured, "_invoke_llm_tool_call was not called" - assert json.loads(captured["arguments"])["command"] == edited - - -def test_edit_command_persisted_in_conversation_history(): - ai = _build_ai() - original = "kubectl delete pod dangerous" - edited = "kubectl get pods -n default" - messages = _make_messages("tc1", original) - - def fake_invoke(*, tool_to_call, **kwargs): - params = json.loads(tool_to_call.function.arguments) - return ToolCallResult( - tool_call_id=tool_to_call.id, - tool_name=tool_to_call.function.name, - description="mocked", - result=StructuredToolResult( - status=StructuredToolResultStatus.SUCCESS, - data="ok", - params=params, - ), - ) - - ai._invoke_llm_tool_call = MagicMock(side_effect=fake_invoke) - - decision = ToolApprovalDecision( - tool_call_id="tc1", approved=True, edit_command=edited - ) - updated_messages, _events = ai._execute_tool_decisions( - messages=messages, tool_decisions=[decision] - ) - - assistant_msg = updated_messages[1] - persisted = json.loads(assistant_msg["tool_calls"][0]["function"]["arguments"]) - assert persisted["command"] == edited - # And the pending_approval flag was cleared so it isn't replayed next turn. - assert "pending_approval" not in assistant_msg["tool_calls"][0] - - -def test_edit_command_appears_in_tool_result_stream_event(): - ai = _build_ai() - edited = "kubectl get pods -n default" - messages = _make_messages("tc1", "kubectl delete pod dangerous") - - def fake_invoke(*, tool_to_call, **kwargs): - params = json.loads(tool_to_call.function.arguments) - return ToolCallResult( - tool_call_id=tool_to_call.id, - tool_name=tool_to_call.function.name, - description="mocked", - result=StructuredToolResult( - status=StructuredToolResultStatus.SUCCESS, - data="ok", - params=params, - ), - ) - - ai._invoke_llm_tool_call = MagicMock(side_effect=fake_invoke) - - decision = ToolApprovalDecision( - tool_call_id="tc1", approved=True, edit_command=edited - ) - _messages, events = ai._execute_tool_decisions( - messages=messages, tool_decisions=[decision] - ) - - tool_results = [e for e in events if e.event == StreamEvents.TOOL_RESULT] - assert len(tool_results) == 1 - assert tool_results[0].data["result"]["params"]["command"] == edited - - -def test_edit_command_ignored_when_decision_is_rejected(): - """A denied decision must not mutate arguments or run the tool, even if - edit_command is present.""" - ai = _build_ai() - original = "kubectl delete pod dangerous" - messages = _make_messages("tc1", original) - - ai._invoke_llm_tool_call = MagicMock() - - decision = ToolApprovalDecision( - tool_call_id="tc1", - approved=False, - edit_command="kubectl get pods", - feedback="no", - ) - updated_messages, events = ai._execute_tool_decisions( - messages=messages, tool_decisions=[decision] - ) - - # The tool was never invoked. - ai._invoke_llm_tool_call.assert_not_called() - # The original command remains in conversation history. - persisted = json.loads( - updated_messages[1]["tool_calls"][0]["function"]["arguments"] - ) - assert persisted["command"] == original - # A denial TOOL_RESULT was still emitted. - tool_results = [e for e in events if e.event == StreamEvents.TOOL_RESULT] - assert len(tool_results) == 1 - assert tool_results[0].data["result"]["status"] == "error"