From f2760a9138cbc061bd77865b8d2f2456c4a9698e Mon Sep 17 00:00:00 2001 From: arc53-machine <232052973+arc53-machine@users.noreply.github.com> Date: Tue, 29 Sep 2026 16:45:55 +0100 Subject: [PATCH] Refuse gated tool calls in research steps instead of running them Research steps ran each tool call straight through the executor, never asking check_pause, so approval-gated actions (including a public-link visitor's wiki edits) and an outside caller's writes on the owner's accounts ran unchecked. A step can't pause, so it now checks each call first and answers any pause with a refusal the model reads, journaled like a headless denial. Headless and research runs share the journaling. --- docsgpt/agents/research_agent.py | 99 +++++++++++++++++---- docsgpt/agents/tool_executor.py | 30 +++++++ docsgpt/llm/handlers/base.py | 24 +---- tests/agents/test_research_agent.py | 133 ++++++++++++++++++++++++++++ 4 files changed, 247 insertions(+), 39 deletions(-) diff --git a/docsgpt/agents/research_agent.py b/docsgpt/agents/research_agent.py index 859bc872..c59f1513 100644 --- a/docsgpt/agents/research_agent.py +++ b/docsgpt/agents/research_agent.py @@ -6,7 +6,7 @@ from typing import Any, Dict, Generator, List, Optional from docsgpt import tracing from docsgpt.agents.base import BaseAgent -from docsgpt.agents.tool_executor import ToolExecutor +from docsgpt.agents.tool_executor import ToolExecutor, journal_refused_call from docsgpt.agents.tools.graph_search import add_graph_search_tool from docsgpt.agents.tools.internal_search import add_internal_search_tool from docsgpt.agents.tools.wiki import add_wiki_tool @@ -582,24 +582,31 @@ class ResearchAgent(BaseAgent): search_returned_empty = False for call in tool_calls: - gen = executor.execute( - tools_dict, call, self.llm.__class__.__name__ - ) - result = None - call_id = None - while True: - try: - event = next(gen) - # Log tool_call status events instead of discarding them - if isinstance(event, dict) and event.get("type") == "tool_call": - logger.debug( - "Tool %s status: %s", - event.get("data", {}).get("action_name", ""), - event.get("data", {}).get("status", ""), - ) - except StopIteration as e: - result, call_id = e.value - break + # A step runs inside one turn and nobody can answer a pause here, + # so a call that would pause (approval, a connection, the client, + # an outside caller's write on the owner's account) is refused. + refusal = self._refuse_paused_call(tools_dict, call, executor) + if refusal is not None: + result, call_id = refusal + else: + gen = executor.execute( + tools_dict, call, self.llm.__class__.__name__ + ) + result = None + call_id = None + while True: + try: + event = next(gen) + # Log tool_call status events instead of discarding them + if isinstance(event, dict) and event.get("type") == "tool_call": + logger.debug( + "Tool %s status: %s", + event.get("data", {}).get("action_name", ""), + event.get("data", {}).get("status", ""), + ) + except StopIteration as e: + result, call_id = e.value + break # Detect empty search results for refinement is_search = "search" in (call.name or "").lower() @@ -636,6 +643,60 @@ class ResearchAgent(BaseAgent): return messages, search_returned_empty + def _refuse_paused_call( + self, tools_dict: Dict, call, executor: ToolExecutor + ) -> Optional[tuple[str, str]]: + """Refuse a call ``check_pause`` would pause on, as a headless run does. + + Args: + tools_dict: The step's tools. + call: The model's tool call. + executor: The run's executor. + + Returns: + ``(tool result, call id)`` when the call is refused, else None. + """ + pause_info = executor.check_pause( + tools_dict, call, self.llm.__class__.__name__ + ) + if not pause_info: + return None + pause_type = pause_info.get("pause_type") + if pause_type == "headless_denied": + reason = pause_info.get("deny_reason") or "This tool can't run here." + result = f"Tool denied: {reason}" + journal_error = f"headless: {reason}" if executor.headless else f"denied: {reason}" + if executor.headless: + executor.headless_denials.append(pause_info) + elif pause_info.get("connection_required"): + result = ( + "Tool not run: its service needs to be connected first, and a " + "research step can't wait for that." + ) + journal_error = "research: connection required" + elif pause_type == "requires_client_execution": + result = ( + "Tool not run: it runs in the user's app, which a research step " + "can't reach." + ) + journal_error = "research: client-side tool" + else: + result = ( + "Tool not run: this action needs the user's approval, which a " + "research step can't ask for. Tell the user it needs their " + "approval in a regular chat." + ) + journal_error = "research: approval required" + logger.info( + "research_step_tool_refused", + extra={ + "action_name": pause_info.get("action_name"), + "pause_type": pause_type, + }, + ) + journal_refused_call(executor, pause_info, journal_error) + return result, pause_info["call_id"] + def _collect_step_sources(self): """Register the search tools' docs (internal search and graph pages) with CitationManager.""" for doc in self._search_tool_docs(): diff --git a/docsgpt/agents/tool_executor.py b/docsgpt/agents/tool_executor.py index a78ebe3e..532d06a3 100644 --- a/docsgpt/agents/tool_executor.py +++ b/docsgpt/agents/tool_executor.py @@ -483,6 +483,36 @@ def _mark_failed( logger.exception("tool_call_attempts failed-write failed for %s", call_id) +def journal_refused_call(executor: Any, pause_info: Dict, error: str) -> None: + """Journal a tool call that was refused instead of paused, as failed. + + A headless run and a research step can't pause for anyone, so a call + ``check_pause`` would pause on is answered with a refusal. Journaling it + keeps the refusal visible to the reconciler and tool analytics. + + Args: + executor: The run's ``ToolExecutor``. + pause_info: What ``check_pause`` returned for the call. + error: The failure recorded on the journal row. + """ + if _record_proposed( + pause_info["call_id"], + pause_info["tool_name"], + pause_info["action_name"], + pause_info.get("arguments") or {}, + tool_id=pause_info.get("tool_id"), + message_id=getattr(executor, "message_id", None), + user_id=getattr(executor, "user", None), + agent_id=getattr(executor, "agent_id", None), + ): + _mark_failed( + pause_info["call_id"], + error, + message_id=getattr(executor, "message_id", None), + user_id=getattr(executor, "user", None), + ) + + class ToolExecutor: """Handles tool discovery, preparation, and execution. diff --git a/docsgpt/llm/handlers/base.py b/docsgpt/llm/handlers/base.py index 1d432f81..f495773c 100644 --- a/docsgpt/llm/handlers/base.py +++ b/docsgpt/llm/handlers/base.py @@ -1191,27 +1191,11 @@ class LLMHandler(ABC): ) if hasattr(agent.tool_executor, "headless_denials"): agent.tool_executor.headless_denials.append(pause_info) - from docsgpt.agents.tool_executor import ( - _mark_failed, - _record_proposed, - ) + from docsgpt.agents.tool_executor import journal_refused_call - if _record_proposed( - pause_info["call_id"], - pause_info["tool_name"], - pause_info["action_name"], - pause_info.get("arguments") or {}, - tool_id=pause_info.get("tool_id"), - message_id=agent.tool_executor.message_id, - user_id=agent.tool_executor.user, - agent_id=agent.tool_executor.agent_id, - ): - _mark_failed( - pause_info["call_id"], - f"headless: {deny_reason}", - message_id=agent.tool_executor.message_id, - user_id=agent.tool_executor.user, - ) + journal_refused_call( + agent.tool_executor, pause_info, f"headless: {deny_reason}" + ) denied_data = { "tool_name": pause_info["tool_name"], "call_id": pause_info["call_id"], diff --git a/tests/agents/test_research_agent.py b/tests/agents/test_research_agent.py index 79b899e7..aeae1e95 100644 --- a/tests/agents/test_research_agent.py +++ b/tests/agents/test_research_agent.py @@ -1676,3 +1676,136 @@ class TestGenInnerTraceSpans: llm_spans = [s for s in trace.spans if s.kind == tracing.KIND_LLM] assert llm_spans[0].parent_id == by_name["research step 1"].id assert by_name["research step 1"].previews["query"] == "s1" + + +# ===================================================================== +# Research steps honour approval and outside-caller gates +# ===================================================================== + + +@pytest.mark.unit +class TestResearchStepPauseGates: + """A research step can't pause for anyone, so a gated call is refused, not run.""" + + def _run(self, agent_base_params, mock_llm_handler, executor, tools_dict, calls, monkeypatch): + from docsgpt.agents import tool_executor as te_mod + + # Journal writes need a database; the refusal still goes through them. + monkeypatch.setattr(te_mod, "_record_proposed", lambda *a, **kw: False) + agent = ResearchAgent(tool_executor=executor, **agent_base_params) + executor.prepare_tools_for_llm(tools_dict) + ran = [] + + def gen_execute(tools, tc, llm_class): + ran.append(tc.name) + yield {"type": "tool_call", "data": {"action_name": tc.name, "status": "pending"}} + return (f"ran {tc.name}", tc.id) + + executor.execute = gen_execute + mock_llm_handler.create_tool_message = Mock( + side_effect=lambda call, result: {"role": "tool", "tool_call_id": call.id, "content": str(result)} + ) + messages, _ = agent._execute_step_tools_with_refinement( + calls, tools_dict, [{"role": "user", "content": "q"}], executor, False + ) + results = {m["tool_call_id"]: m["content"] for m in messages if m.get("role") == "tool"} + return ran, results, messages + + def test_public_link_wiki_write_is_refused_and_view_still_runs( + self, agent_base_params, mock_llm, mock_llm_handler, mock_llm_creator, mock_llm_handler_creator, monkeypatch, + ): + from docsgpt.agents.tool_executor import ToolExecutor + from docsgpt.agents.tools.wiki import add_wiki_tool + from docsgpt.llm.handlers.base import ToolCall + + tools_dict = {} + add_wiki_tool( + tools_dict, + {"source_id": "w1", "source_owner_id": "visitor", "user": "visitor", "approval_required": True}, + ) + executor = ToolExecutor(user="visitor", decoded_token={"sub": "visitor"}, public_link_caller=True) + calls = [ + ToolCall(id="c1", name="wiki_create", arguments={"path": "/x.md", "content": "x"}), + ToolCall(id="c2", name="wiki_view", arguments={"path": "/"}), + ] + ran, results, messages = self._run( + agent_base_params, mock_llm_handler, executor, tools_dict, calls, monkeypatch + ) + assert ran == ["wiki_view"] + assert "approval" in results["c1"].lower() and "not run" in results["c1"].lower() + assert results["c2"] == "ran wiki_view" + # Each call, refused or not, still gets its assistant tool_call turn. + assert [m["tool_calls"][0]["id"] for m in messages if m.get("role") == "assistant"] == ["c1", "c2"] + + def test_external_caller_owner_credential_write_is_refused_read_runs( + self, agent_base_params, mock_llm, mock_llm_handler, mock_llm_creator, mock_llm_handler_creator, monkeypatch, + ): + from docsgpt.agents.tool_executor import ToolExecutor + from docsgpt.llm.handlers.base import ToolCall + + params = {"type": "object", "properties": {"text": {"type": "string"}}} + tools_dict = { + "tool-1": { + "id": "tool-1", + "name": "telegram", + "user_id": "owner", + "config": {"encrypted_credentials": "secret"}, + "actions": [ + {"name": "send_message", "description": "Send", "active": True, "parameters": params}, + {"name": "get_updates", "description": "Read", "active": True, "parameters": params}, + ], + } + } + executor = ToolExecutor(user="owner", decoded_token={"sub": "owner"}, external_caller=True) + calls = [ + ToolCall(id="c1", name="send_message", arguments={"text": "hi"}), + ToolCall(id="c2", name="get_updates", arguments={"text": "x"}), + ] + ran, results, _ = self._run(agent_base_params, mock_llm_handler, executor, tools_dict, calls, monkeypatch) + assert ran == ["get_updates"] + assert "can't take this action" in results["c1"] + assert results["c2"] == "ran get_updates" + + def test_approval_required_action_is_refused_for_the_owner_too( + self, agent_base_params, mock_llm, mock_llm_handler, mock_llm_creator, mock_llm_handler_creator, monkeypatch, + ): + from docsgpt.agents.tool_executor import ToolExecutor + from docsgpt.llm.handlers.base import ToolCall + + tools_dict = { + "tool-2": { + "id": "tool-2", + "name": "api_tool_like", + "user_id": "owner", + "config": {}, + "actions": [ + {"name": "delete_all", "description": "Delete", "active": True, "require_approval": True, + "parameters": {"type": "object", "properties": {}}}, + ], + } + } + executor = ToolExecutor(user="owner", decoded_token={"sub": "owner"}) + calls = [ToolCall(id="c1", name="delete_all", arguments={})] + ran, results, _ = self._run(agent_base_params, mock_llm_handler, executor, tools_dict, calls, monkeypatch) + assert ran == [] + assert "approval" in results["c1"].lower() + + def test_headless_denial_is_recorded( + self, agent_base_params, mock_llm, mock_llm_handler, mock_llm_creator, mock_llm_handler_creator, monkeypatch, + ): + from docsgpt.agents.tool_executor import ToolExecutor + from docsgpt.llm.handlers.base import ToolCall + + tools_dict = { + "tool-2": { + "id": "tool-2", "name": "x", "user_id": "owner", "config": {}, + "actions": [{"name": "delete_all", "description": "Delete", "active": True, + "require_approval": True, "parameters": {"type": "object", "properties": {}}}], + } + } + executor = ToolExecutor(user="owner", decoded_token={"sub": "owner"}, headless=True) + calls = [ToolCall(id="c1", name="delete_all", arguments={})] + ran, results, _ = self._run(agent_base_params, mock_llm_handler, executor, tools_dict, calls, monkeypatch) + assert ran == [] + assert results["c1"].startswith("Tool denied") + assert [d["action_name"] for d in executor.headless_denials] == ["delete_all"]