mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-13 16:14:10 +00:00
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.
This commit is contained in:
1 parent
ff10fcca0f
commit
f2760a9138
4 files changed
+247
-39
No files matched your search
@@ -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():
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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"],
|
||||
|
||||
@@ -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"]
|
||||
Reference in new issue
Block a user