Merge 995416edda into 6da7315aa3

strix/runtime/tool_server.py

@@ -68,6 +68,29 @@ class ToolExecutionResponse(BaseModel):
     error: str | None = None
 
 
+# Extra seconds granted on top of an explicit tool-level ``timeout`` kwarg so
+# the sandbox wait_for envelope does not race the tool's own internal timer.
+_TOOL_TIMEOUT_BUFFER_SECONDS = 30
+
+
+def _resolve_request_timeout(default_timeout: int, tool_kwargs: dict[str, Any]) -> int:
+    """Return the wait_for envelope to apply to a single tool request.
+
+    When a tool call passes an explicit ``timeout`` kwarg that exceeds the
+    sandbox default (``--timeout`` / ``STRIX_SANDBOX_EXECUTION_TIMEOUT``),
+    grow the outer ``asyncio.wait_for`` so the tool's own timer runs to
+    completion plus a small buffer for tool setup/teardown. Otherwise keep
+    the sandbox default as the hard cap — it still protects against runaway
+    tools that do not honour their own ``timeout`` contract.
+    """
+    tool_timeout = tool_kwargs.get("timeout")
+    if isinstance(tool_timeout, bool):
+        return default_timeout
+    if isinstance(tool_timeout, (int, float)) and tool_timeout > 0:
+        return max(default_timeout, int(tool_timeout) + _TOOL_TIMEOUT_BUFFER_SECONDS)
+    return default_timeout
+
+
 async def _run_tool(agent_id: str, tool_name: str, kwargs: dict[str, Any]) -> Any:
     from strix.tools.argument_parser import convert_arguments
     from strix.tools.context import set_current_agent_id
@@ -96,9 +119,11 @@ async def execute_tool(
         if not old_task.done():
             old_task.cancel()
 
+    effective_timeout = _resolve_request_timeout(REQUEST_TIMEOUT, request.kwargs)
     task = asyncio.create_task(
         asyncio.wait_for(
-            _run_tool(agent_id, request.tool_name, request.kwargs), timeout=REQUEST_TIMEOUT
+            _run_tool(agent_id, request.tool_name, request.kwargs),
+            timeout=effective_timeout,
         )
     )
     agent_tasks[agent_id] = task
@@ -111,7 +136,13 @@ async def execute_tool(
         return ToolExecutionResponse(error="Cancelled by newer request")
 
     except TimeoutError:
-        return ToolExecutionResponse(error=f"Tool timed out after {REQUEST_TIMEOUT}s")
+        return ToolExecutionResponse(
+            error=(
+                f"Tool timed out after {effective_timeout}s. "
+                "Raise STRIX_SANDBOX_EXECUTION_TIMEOUT (default 120) for longer scans, "
+                "or pass a larger `timeout` kwarg on the tool call."
+            )
+        )
 
     except ValidationError as e:
         return ToolExecutionResponse(error=f"Invalid arguments: {e}")

tests/runtime/test_tool_server_timeout.py

@@ -0,0 +1,120 @@
+"""Unit + functional tests for request-timeout resolution in the sandbox tool server.
+
+Regression coverage for #436 — ``terminal_execute`` timing out at the sandbox
+hard cap even when the caller passed a longer explicit ``timeout`` kwarg.
+"""
+
+from __future__ import annotations
+
+import importlib.util
+import sys
+import time
+from pathlib import Path
+from typing import TYPE_CHECKING, Any
+
+import pytest
+from fastapi.security import HTTPAuthorizationCredentials
+
+from strix.tools import registry
+
+
+if TYPE_CHECKING:
+    from types import ModuleType
+
+
+TOOL_SERVER_PATH = Path(__file__).resolve().parents[2] / "strix" / "runtime" / "tool_server.py"
+
+
+@pytest.fixture
+def tool_server(monkeypatch: pytest.MonkeyPatch) -> ModuleType:
+    """Load ``strix.runtime.tool_server`` under its import-time guards.
+
+    The module requires ``STRIX_SANDBOX_MODE=true`` and reads ``sys.argv`` at
+    import time. We load it under a unique module name via ``importlib.util``
+    and register it with ``monkeypatch.setitem`` so the entry is torn down
+    between tests — keeping sibling tests clean.
+    """
+
+    monkeypatch.setenv("STRIX_SANDBOX_MODE", "true")
+    monkeypatch.setattr(
+        sys,
+        "argv",
+        ["tool_server", "--token", "test-token", "--port", "0"],
+    )
+
+    spec = importlib.util.spec_from_file_location(
+        "strix.runtime._tool_server_under_test", TOOL_SERVER_PATH
+    )
+    assert spec is not None
+    assert spec.loader is not None
+    module = importlib.util.module_from_spec(spec)
+    monkeypatch.setitem(sys.modules, spec.name, module)
+    spec.loader.exec_module(module)
+    return module
+
+
+class TestResolveRequestTimeout:
+    def test_no_timeout_kwarg_uses_default(self, tool_server: ModuleType) -> None:
+        assert tool_server._resolve_request_timeout(120, {}) == 120
+        assert tool_server._resolve_request_timeout(120, {"command": "ls"}) == 120
+
+    def test_small_tool_timeout_keeps_default_hard_cap(self, tool_server: ModuleType) -> None:
+        # Inner tool asks for 10s; sandbox default (120) still dominates.
+        assert tool_server._resolve_request_timeout(120, {"timeout": 10}) == 120
+
+    def test_large_tool_timeout_grows_envelope_by_buffer(self, tool_server: ModuleType) -> None:
+        # The actual #436 case: agent passes a 600s timeout, sandbox should grow
+        # the wait_for envelope to 600s + buffer instead of killing at 120.
+        buffer = tool_server._TOOL_TIMEOUT_BUFFER_SECONDS
+        assert tool_server._resolve_request_timeout(120, {"timeout": 600}) == 600 + buffer
+
+    def test_float_timeout_is_accepted(self, tool_server: ModuleType) -> None:
+        buffer = tool_server._TOOL_TIMEOUT_BUFFER_SECONDS
+        assert tool_server._resolve_request_timeout(120, {"timeout": 300.0}) == 300 + buffer
+
+    def test_zero_or_negative_timeout_falls_back_to_default(self, tool_server: ModuleType) -> None:
+        assert tool_server._resolve_request_timeout(120, {"timeout": 0}) == 120
+        assert tool_server._resolve_request_timeout(120, {"timeout": -1}) == 120
+
+    def test_bool_is_not_treated_as_timeout(self, tool_server: ModuleType) -> None:
+        # ``bool`` is a subclass of ``int``; guard against ``True`` being read
+        # as ``1 second`` (or ``False`` disabling the envelope).
+        assert tool_server._resolve_request_timeout(120, {"timeout": True}) == 120
+        assert tool_server._resolve_request_timeout(120, {"timeout": False}) == 120
+
+    def test_non_numeric_timeout_is_ignored(self, tool_server: ModuleType) -> None:
+        assert tool_server._resolve_request_timeout(120, {"timeout": None}) == 120
+        assert tool_server._resolve_request_timeout(120, {"timeout": "forever"}) == 120
+        assert tool_server._resolve_request_timeout(120, {"timeout": ["a"]}) == 120
+
+
+async def test_timeout_error_message_is_actionable(
+    tool_server: ModuleType, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """When a tool exceeds the effective envelope, the response error mentions
+    ``STRIX_SANDBOX_EXECUTION_TIMEOUT`` so issue-reporters like #436 can
+    self-discover the configuration knob."""
+
+    # Shrink the envelope to keep the test fast.
+    monkeypatch.setattr(tool_server, "REQUEST_TIMEOUT", 1)
+
+    def _slow_tool(**_kwargs: Any) -> dict[str, Any]:
+        time.sleep(3)
+        return {"ok": True}
+
+    _slow_tool.__name__ = "__slow_test_tool__"
+    monkeypatch.setitem(registry._tools_by_name, "__slow_test_tool__", _slow_tool)
+
+    creds = HTTPAuthorizationCredentials(scheme="Bearer", credentials="test-token")
+    request = tool_server.ToolExecutionRequest(
+        agent_id="agent-x",
+        tool_name="__slow_test_tool__",
+        kwargs={},
+    )
+
+    response = await tool_server.execute_tool(request, creds)
+
+    assert response.result is None
+    assert response.error is not None
+    assert "timed out after 1s" in response.error
+    assert "STRIX_SANDBOX_EXECUTION_TIMEOUT" in response.error