From b8d327dc41f756a5a17523032e02cbc7297a891b Mon Sep 17 00:00:00 2001
From: Xubin Ren <xubinrencs@gmail.com>
Date: Fri, 17 Apr 2026 12:24:06 +0000
Subject: [PATCH] test + docs: lock should_execute_tools guard semantics
 (#3220)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two small follow-ups to the guard:

1. Fix the should_execute_tools docstring so it matches the actual code.
   The previous version said "Only execute when finish_reason explicitly
   signals tool intent" but the code also accepts finish_reason == "stop".
   Explain why (some compliant providers emit "stop" with legitimate tool
   calls — openai_compat_provider.py already mirrors this at lines ~633 /
   ~678 where ("tool_calls", "stop") are both treated as the terminal
   tool-call state). Without this, a strict "tool_calls"-only guard would
   regress 15 existing runner tests that construct LLMResponse with
   tool_calls but no explicit finish_reason (default = "stop").

2. Add tests/providers/test_llm_response.py. This locks the three cases:
   - no tool calls                  -> never executes
   - tool calls + "tool_calls"/stop -> executes
   - tool calls + refusal / content_filter / error / length / ... -> blocked

   These are exactly the boundary cases the #3220 fix is about; without a
   test here a future refactor could silently revert the guard.

Body + tests only, no behavior change beyond the existing PR's intent.

Made-with: Cursor
---
 nanobot/providers/base.py            | 17 +++++----
 tests/providers/test_llm_response.py | 57 ++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 8 deletions(-)
 create mode 100644 tests/providers/test_llm_response.py

diff --git a/nanobot/providers/base.py b/nanobot/providers/base.py
index 27ea1e18..f017bd3c 100644
--- a/nanobot/providers/base.py
+++ b/nanobot/providers/base.py
@@ -75,17 +75,18 @@ class LLMResponse:
     def should_execute_tools(self) -> bool:
         """Check if tool calls should be executed (guards against gateway injection).
 
-        Only execute when finish_reason explicitly signals tool intent.
-        Tool calls under any other finish_reason (refusal, content_filter, error, etc.)
-        are treated as anomalous and should not be executed.
+        Executes only when ``has_tool_calls`` is true and ``finish_reason`` is one of
+        the known-good signals: ``"tool_calls"`` (explicit intent) or ``"stop"`` (some
+        compliant providers emit ``stop`` for legitimate tool calls; existing paths in
+        ``openai_compat_provider`` already treat both as the tool-call terminal state).
+
+        Tool calls under any other ``finish_reason`` (e.g. ``refusal``, ``content_filter``,
+        ``error``) are treated as anomalous — typically injected by non-compliant API
+        gateways — and are skipped.
         """
         if not self.has_tool_calls:
             return False
-        if self.finish_reason == "tool_calls":
-            return True
-        if self.finish_reason == "stop":
-            return True
-        return False
+        return self.finish_reason in ("tool_calls", "stop")
 
 
 @dataclass(frozen=True)
diff --git a/tests/providers/test_llm_response.py b/tests/providers/test_llm_response.py
new file mode 100644
index 00000000..ca9644dc
--- /dev/null
+++ b/tests/providers/test_llm_response.py
@@ -0,0 +1,57 @@
+"""Regression tests for ``LLMResponse.should_execute_tools`` (#3220).
+
+The agent used to execute tool calls whenever ``has_tool_calls`` was true, regardless
+of ``finish_reason``. Non-compliant API gateways that inject empty / bogus tool calls
+under ``refusal`` / ``content_filter`` / ``error`` pushed the agent into a tight loop
+until ``max_iterations`` fired. ``should_execute_tools`` is the single guard that
+every tool-execution site now funnels through.
+"""
+
+from __future__ import annotations
+
+import pytest
+
+from nanobot.providers.base import LLMResponse, ToolCallRequest
+
+
+def _response(finish_reason: str, *, with_tool_call: bool = True) -> LLMResponse:
+    tool_calls = (
+        [ToolCallRequest(id="call_1", name="list_dir", arguments={"path": "."})]
+        if with_tool_call
+        else []
+    )
+    return LLMResponse(content=None, tool_calls=tool_calls, finish_reason=finish_reason)
+
+
+class TestShouldExecuteTools:
+    def test_no_tool_calls_never_executes(self) -> None:
+        # No tool calls present -> guard must reject regardless of finish_reason.
+        for reason in ("tool_calls", "stop", "length", "error", "refusal", "content_filter"):
+            resp = _response(reason, with_tool_call=False)
+            assert resp.should_execute_tools is False, f"rejected for finish_reason={reason!r}"
+
+    def test_tool_calls_with_tool_calls_reason_executes(self) -> None:
+        # The canonical case: provider explicitly signals tool intent.
+        resp = _response("tool_calls")
+        assert resp.has_tool_calls is True
+        assert resp.should_execute_tools is True
+
+    def test_tool_calls_with_stop_reason_executes(self) -> None:
+        # Some compliant providers emit "stop" together with tool_calls; the
+        # guard must accept this to avoid breaking real tool-calling flows.
+        # See openai_compat_provider.py:~633,678 where ("tool_calls", "stop")
+        # are both treated as terminal tool-call states.
+        resp = _response("stop")
+        assert resp.should_execute_tools is True
+
+    @pytest.mark.parametrize(
+        "anomalous_reason",
+        ["refusal", "content_filter", "error", "length", "function_call", ""],
+    )
+    def test_tool_calls_under_anomalous_reason_blocked(self, anomalous_reason: str) -> None:
+        # This is the #3220 bug: gateways injecting tool_calls under any of these
+        # finish_reasons must not cause execution. Blocking here is what prevents
+        # the infinite empty tool-call loop.
+        resp = _response(anomalous_reason)
+        assert resp.has_tool_calls is True
+        assert resp.should_execute_tools is False