mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-01 12:02:05 +00:00
fix(tools): read browser.command_timeout in Camofox HTTP client
The Camofox browser backend hardcoded a 30s HTTP timeout via _DEFAULT_TIMEOUT, ignoring the user's browser.command_timeout config. The main browser_tool path already reads this config via _get_command_timeout(). This commit adds an equivalent _get_command_timeout() to browser_camofox.py that reads browser.command_timeout from config with caching, and switches all HTTP helper methods (_post, _get, _get_raw, _delete) to use it as the default timeout. Fixes #40843
This commit is contained in:
parent
babd9168ba
commit
fe38d50833
2 changed files with 112 additions and 7 deletions
69
tests/tools/test_browser_camofox_timeout.py
Normal file
69
tests/tools/test_browser_camofox_timeout.py
Normal file
|
|
@ -0,0 +1,69 @@
|
|||
"""Tests for browser_camofox._get_command_timeout — config-driven timeout."""
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
class TestCamofoxCommandTimeout:
|
||||
"""Verify that the Camofox HTTP backend reads browser.command_timeout."""
|
||||
|
||||
def test_default_is_30(self):
|
||||
"""When config has no browser.command_timeout, default to 30s."""
|
||||
from tools.browser_camofox import _get_command_timeout
|
||||
|
||||
# Clear cache
|
||||
import tools.browser_camofox as mod
|
||||
mod._cmd_timeout_resolved = False
|
||||
mod._cached_cmd_timeout = None
|
||||
|
||||
with patch("tools.browser_camofox.read_raw_config", return_value={}):
|
||||
assert _get_command_timeout() == 30
|
||||
|
||||
def test_reads_from_config(self):
|
||||
"""Read browser.command_timeout from config.yaml."""
|
||||
from tools.browser_camofox import _get_command_timeout
|
||||
|
||||
import tools.browser_camofox as mod
|
||||
mod._cmd_timeout_resolved = False
|
||||
mod._cached_cmd_timeout = None
|
||||
|
||||
cfg = {"browser": {"command_timeout": 90}}
|
||||
with patch("tools.browser_camofox.read_raw_config", return_value=cfg):
|
||||
assert _get_command_timeout() == 90
|
||||
|
||||
def test_floor_at_5s(self):
|
||||
"""Config values below 5 are clamped to 5."""
|
||||
from tools.browser_camofox import _get_command_timeout
|
||||
|
||||
import tools.browser_camofox as mod
|
||||
mod._cmd_timeout_resolved = False
|
||||
mod._cached_cmd_timeout = None
|
||||
|
||||
cfg = {"browser": {"command_timeout": 1}}
|
||||
with patch("tools.browser_camofox.read_raw_config", return_value=cfg):
|
||||
assert _get_command_timeout() == 5
|
||||
|
||||
def test_cached_after_first_call(self):
|
||||
"""Config is read only once; subsequent calls use cached value."""
|
||||
from tools.browser_camofox import _get_command_timeout
|
||||
|
||||
import tools.browser_camofox as mod
|
||||
mod._cmd_timeout_resolved = False
|
||||
mod._cached_cmd_timeout = None
|
||||
|
||||
mock_read = MagicMock(return_value={"browser": {"command_timeout": 45}})
|
||||
with patch("tools.browser_camofox.read_raw_config", mock_read):
|
||||
_get_command_timeout()
|
||||
_get_command_timeout()
|
||||
mock_read.assert_called_once()
|
||||
|
||||
def test_config_read_error_falls_back(self):
|
||||
"""If config read raises, fall back to 30s."""
|
||||
from tools.browser_camofox import _get_command_timeout
|
||||
|
||||
import tools.browser_camofox as mod
|
||||
mod._cmd_timeout_resolved = False
|
||||
mod._cached_cmd_timeout = None
|
||||
|
||||
with patch("tools.browser_camofox.read_raw_config", side_effect=Exception("no config")):
|
||||
assert _get_command_timeout() == 30
|
||||
|
|
@ -36,7 +36,7 @@ from urllib.parse import SplitResult, urlsplit, urlunsplit
|
|||
|
||||
import requests
|
||||
|
||||
from hermes_cli.config import cfg_get, load_config
|
||||
from hermes_cli.config import cfg_get, load_config, read_raw_config
|
||||
from tools.browser_camofox_state import get_camofox_identity
|
||||
from tools.registry import tool_error
|
||||
|
||||
|
|
@ -46,11 +46,39 @@ logger = logging.getLogger(__name__)
|
|||
# Configuration
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
_DEFAULT_TIMEOUT = 30 # seconds per HTTP request
|
||||
_DEFAULT_TIMEOUT = 30 # fallback when config is unreadable
|
||||
_SNAPSHOT_MAX_CHARS = 80_000 # camofox paginates at this limit
|
||||
_vnc_url: Optional[str] = None # cached from /health response
|
||||
_vnc_url_checked = False # only probe once per process
|
||||
|
||||
# Cached command timeout from config (resolved lazily, like browser_tool)
|
||||
_cached_cmd_timeout: Optional[int] = None
|
||||
_cmd_timeout_resolved = False
|
||||
|
||||
|
||||
def _get_command_timeout() -> int:
|
||||
"""Return ``browser.command_timeout`` from config, falling back to 30s.
|
||||
|
||||
Mirrors :func:`tools.browser_tool._get_command_timeout` so both the
|
||||
local browser path and the Camofox path honour the same config knob.
|
||||
Result is cached after the first call.
|
||||
"""
|
||||
global _cached_cmd_timeout, _cmd_timeout_resolved
|
||||
if _cmd_timeout_resolved:
|
||||
return _cached_cmd_timeout # type: ignore[return-value]
|
||||
|
||||
_cmd_timeout_resolved = True
|
||||
result = _DEFAULT_TIMEOUT
|
||||
try:
|
||||
cfg = read_raw_config()
|
||||
val = cfg_get(cfg, "browser", "command_timeout")
|
||||
if val is not None:
|
||||
result = max(int(val), 5) # floor at 5s
|
||||
except Exception as exc:
|
||||
logger.debug("Could not read browser.command_timeout: %s", exc)
|
||||
_cached_cmd_timeout = result
|
||||
return result
|
||||
|
||||
|
||||
def _auth_headers() -> Dict[str, str]:
|
||||
"""Return Authorization header when CAMOFOX_API_KEY is set."""
|
||||
|
|
@ -356,7 +384,7 @@ def _ensure_tab(task_id: Optional[str], url: str = "about:blank") -> Dict[str, A
|
|||
"listItemId": session["session_key"],
|
||||
"url": url,
|
||||
},
|
||||
timeout=_DEFAULT_TIMEOUT,
|
||||
timeout=_get_command_timeout(),
|
||||
headers=_auth_headers(),
|
||||
)
|
||||
resp.raise_for_status()
|
||||
|
|
@ -393,32 +421,40 @@ def camofox_soft_cleanup(task_id: Optional[str] = None) -> bool:
|
|||
# HTTP helpers
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def _post(path: str, body: dict, timeout: int = _DEFAULT_TIMEOUT) -> dict:
|
||||
def _post(path: str, body: dict, timeout: Optional[int] = None) -> dict:
|
||||
"""POST JSON to camofox and return parsed response."""
|
||||
if timeout is None:
|
||||
timeout = _get_command_timeout()
|
||||
url = f"{get_camofox_url()}{path}"
|
||||
resp = requests.post(url, json=body, timeout=timeout, headers=_auth_headers())
|
||||
resp.raise_for_status()
|
||||
return resp.json()
|
||||
|
||||
|
||||
def _get(path: str, params: dict = None, timeout: int = _DEFAULT_TIMEOUT) -> dict:
|
||||
def _get(path: str, params: dict = None, timeout: Optional[int] = None) -> dict:
|
||||
"""GET from camofox and return parsed response."""
|
||||
if timeout is None:
|
||||
timeout = _get_command_timeout()
|
||||
url = f"{get_camofox_url()}{path}"
|
||||
resp = requests.get(url, params=params, timeout=timeout, headers=_auth_headers())
|
||||
resp.raise_for_status()
|
||||
return resp.json()
|
||||
|
||||
|
||||
def _get_raw(path: str, params: dict = None, timeout: int = _DEFAULT_TIMEOUT) -> requests.Response:
|
||||
def _get_raw(path: str, params: dict = None, timeout: Optional[int] = None) -> requests.Response:
|
||||
"""GET from camofox and return raw response (for binary data)."""
|
||||
if timeout is None:
|
||||
timeout = _get_command_timeout()
|
||||
url = f"{get_camofox_url()}{path}"
|
||||
resp = requests.get(url, params=params, timeout=timeout, headers=_auth_headers())
|
||||
resp.raise_for_status()
|
||||
return resp
|
||||
|
||||
|
||||
def _delete(path: str, body: dict = None, timeout: int = _DEFAULT_TIMEOUT) -> dict:
|
||||
def _delete(path: str, body: dict = None, timeout: Optional[int] = None) -> dict:
|
||||
"""DELETE to camofox and return parsed response."""
|
||||
if timeout is None:
|
||||
timeout = _get_command_timeout()
|
||||
url = f"{get_camofox_url()}{path}"
|
||||
resp = requests.delete(url, json=body, timeout=timeout, headers=_auth_headers())
|
||||
resp.raise_for_status()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue