Camofox HTTP client command timeout fixes

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #54729 fix(camofox): repair broken browser backend (listItemId + auth + timeout + stale-tab)

  merged to main

  files: hermes_cli/config.py, tests/tools/test_browser_camofox_auth.py, tests/tools/test_browser_camofox_ensure_tab.py, tests/tools/test_browser_camofox_persistence.py, tests/tools/test_browser_camofox_timeout.py, tools/browser_camofox.py

  Show the diff

  • hermes_cli/config.py

    @@ -3549,6 +3549,15 @@ def _ensure_hermes_home_managed(home: Path):
             "password": [redacted:secret-assignment]
             "category": "tool",
         },
    +    "CAMOFOX_API_KEY": {
    +        "description": "Optional bearer token sent as Authorization header to a remote/authenticated Camofox server",
    +        "prompt": "Camofox API key",
    +        "url": "https://github.com/jo-inc/camofox-browser",
    +        "tools": ["browser_navigate", "browser_click"],
    +        "password": True,
    +        "category": "tool",
    +        "advanced": True,
    +    },
         "FAL_KEY": {
             "description": "FAL API key for image and video generation",
             "prompt": "FAL API key",

  • tests/tools/test_browser_camofox_auth.py

    @@ -0,0 +1,111 @@
    +"""Tests that Camofox browser sends Authorization header when CAMOFOX_API_KEY is set.
    +
    +Regression test for https://github.com/NousResearch/hermes-agent/issues/20476
    +"""
    +
    +import json
    +from unittest.mock import MagicMock, patch
    +
    +import pytest
    +
    +from tools.browser_camofox import (
    +    _auth_headers,
    +    camofox_back,
    +    camofox_click,
    +    camofox_close,
    +    camofox_navigate,
    +    camofox_press,
    +    camofox_scroll,
    +    camofox_snapshot,
    +    camofox_type,
    +)
    +
    +
    +def _mock_response(status=200, json_data=None):
    +    resp = MagicMock()
    +    resp.status_code = status
    +    resp.json.return_value = json_data or {}
    +    resp.content = b"\x89PNG\r\n\x1a\nfake"
    +    resp.raise_for_status = MagicMock()
    +    return resp
    +
    +
    +class TestAuthHeaders:
    +    """Unit tests for _auth_headers() helper."""
    +
    +    def test_empty_when_no_key(self, monkeypatch):
    +        monkeypatch.delenv("CAMOFOX_API_KEY", raising=False)
    +        assert _auth_headers() == {}
    +
    +    def test_bearer_when_key_set(self, monkeypatch):
    +        monkeypatch.setenv("CAMOFOX_API_KEY", "test-secret-123")
    +        assert _auth_headers() == {"[redacted:auth-header]"}
    +
    +    def test_empty_when_key_blank(self, monkeypatch):
    +        monkeypatch.setenv("CAMOFOX_API_KEY", "   ")
    +        assert _auth_headers() == {}
    +
    +
    +class TestAuthHeadersSent:
    +    """Verify all HTTP call sites include auth headers when CAMOFOX_API_KEY is set."""
    +
    +    @pytest.fixture(autouse=True)
    +    def _set_key(self, monkeypatch):
    +        monkeypatch.setenv("CAMOFOX_URL", "http://localhost:9377")
    +        monkeypatch.setenv("CAMOFOX_API_KEY", "my-api-key")
    +
    +    @patch("tools.browser_camofox.requests.post")
    +    def test_ensure_tab_sends_auth(self, mock_post):
    +        mock_post.return_value = _mock_response(json_data={"tabId": "t1"})
    +        camofox_navigate("https://example.com", task_id="auth_test_1")
    +        _, kwargs = mock_post.call_args
    +        assert kwargs["headers"] == {"[redacted:auth-header]"}
    +
    +    @patch("tools.browser_camofox.requests.post")
    +    def test_post_sends_auth(self, mock_post):
    +        mock_post.return_value = _mock_response(json_data={"tabId": "t2"})
    +        camofox_navigate("https://example.com", task_id="auth_test_2")
    +        mock_post.return_value = _mock_response(json_data={"ok": True, "url": "https://x.com"})
    +        camofox_navigate("https://x.com", task_id="auth_test_2")
    +        # The second call is a POST to /tabs/{tabId}/navigate
    +        last_call = mock_post.call_args_list[-1]
    +        assert last_call.kwargs.get("headers") == {"[redacted:auth-header]"}
    +
    +    @patch("tools.browser_camofox.requests.post")
    +    @patch("tools.browser_camofox.requests.get")
    +    def test_get_sends_auth(self, mock_get, mock_post):
    +        mock_post.return_value = _mock_response(json_data={"tabId": "t3"})
    +        camofox_navigate("https://example.com", task_id="auth_test_3")
    +        mock_get.return_value = _mock_response(json_data={
    +            "snapshot": '- heading "Hello"',
    +            "refsCount": 1,
    +        })
    +        camofox_snapshot(task_id="auth_test_3")
    +        _, kwargs = mock_get.call_args
    +        assert kwargs["headers"] == {"[redacted:auth-header]"}
    +
    +    @patch("tools.browser_camofox.requests.post")
    +    @patch("tools.browser_camofox.requests.delete")
    +    def test_delete_sends_auth(self, mock_delete, mock_post):
    +        mock_post.return_value = _mock_response(json_data={"tabId": "t4"})
    +        camofox_navigate("https://example.com", task_id="auth_test_4")
    +        mock_delete.return_value = _mock_response(json_data={"ok": True})
    +        camofox_close(task_id="auth_test_4")
    +        _, kwargs = mock_delete.call_args
    +        assert kwargs["headers"] == {"[redacted:auth-header]"}
    +
    +
    +class TestNoAuthHeadersWhenKeyUnset:
    +    """Verify HTTP calls send empty headers when CAMOFOX_API_KEY is not set."""
    +
    +    @pytest.fixture(autouse=True)
    +    def _unset_key(self, monkeypatch):
    +        monkeypatch.setenv("CAMOFOX_URL", "http://localhost:9377")
    +        monkeypatch.delenv("CAMOFOX_API_KEY", raising=False)
    +
    +    @patch("tools.browser_camofox.requests.post")
    +    def test_no_auth_on_tab_creation(self, mock_post):
    +        mock_post.return_value = _mock_response(json_data={"tabId": "t5"})
    +        camofox_navigate("https://example.com", task_id="noauth_test_1")
    +        _, kwargs = mock_post.call_args
    +        assert kwargs.get("headers") == {}

  • tests/tools/test_browser_camofox_ensure_tab.py

    @@ -0,0 +1,66 @@
    +"""Regression test: _ensure_tab must send ``listItemId`` (not ``sessionKey``).
    +
    +The Camoufox REST API server requires ``listItemId`` in the ``POST /tabs``
    +body.  A previous version sent ``sessionKey`` which caused a 400 Bad Request
    +on every ``browser_navigate`` call.  See issue #37960.
    +"""
    +
    +from unittest.mock import patch, MagicMock
    +
    +
    +def test_ensure_tab_sends_list_item_id():
    +    """POST /tabs body must contain ``listItemId``, not ``sessionKey``."""
    +    # Import the module under test
    +    from tools import browser_camofox as mod
    +
    +    fake_session = {
    +        "user_id": "hermes_test123",
    +        "tab_id": None,
    +        "session_key": "task_my-session",
    +        "managed": False,
    +        "adopt_existing_tab": False,
    +    }
    +
    +    mock_response = MagicMock()
    +    mock_response.json.return_value = {"tabId": "tab-42"}
    +    mock_response.raise_for_status = MagicMock()
    +
    +    with patch.object(mod, "_get_session", return_value=fake_session), \
    +         patch.object(mod, "get_camofox_url", return_value="http://localhost:9377"), \
    +         patch("tools.browser_camofox.requests.post", return_value=mock_response) as mock_post:
    +        result = mod._ensure_tab("test-task", url="https://example.com")
    +
    +    # Verify the POST was called
    +    mock_post.assert_called_once()
    +    call_kwargs = mock_post.call_args
    +    body = call_kwargs.kwargs.get("json") or call_kwargs[1].get("json")
    +
    +    # Core assertion: listItemId present, sessionKey absent
    +    assert "listItemId" in body, f"Expected 'listItemId' in POST body, got: {body}"
    +    assert "sessionKey" not in body, f"'sessionKey' should not be in POST body: {body}"
    +    assert body["listItemId"] == "task_my-session"
    +    assert body["userId"] == "hermes_test123"
    +    assert body["url"] == "https://example.com"
    +
    +    # Verify tab_id was set from response
    +    assert result["tab_id"] == "tab-42"
    +
    +
    +def test_ensure_tab_skips_creation_when_tab_exists():
    +    """If session already has a tab_id, no POST should be made."""
    +    from tools import browser_camofox as mod
    +
    +    fake_session = {
    +        "user_id": "hermes_test123",
    +        "tab_id": "existing-tab",
    +        "session_key": "task_my-session",
    +        "managed": False,
    +    }
    +
    +    with patch.object(mod, "_get_session", return_value=fake_session), \
    +         patch("tools.browser_camofox.requests.post") as mock_post:
    +        result = mod._ensure_tab("test-task")
    +
    +    # No POST should be made — tab already exists
    +    mock_post.assert_not_called()
    +    assert result["tab_id"] == "existing-tab"

  • tests/tools/test_browser_camofox_persistence.py

    @@ -151,7 +151,7 @@ def test_navigate_uses_stable_identity(self, tmp_path, monkeypatch):
     
             requests_seen = []
     
    -        def _capture_post(url, json=None, timeout=None):
    +        def _capture_post(url, json=None, timeout=None, headers=None):
                 requests_seen.append(json)
                 return _mock_response(
                     json_data={"tabId": "tab-1", "url": "https://example.com"}
    @@ -171,7 +171,7 @@ def test_navigate_reuses_identity_after_close(self, tmp_path, monkeypatch):
     
             requests_seen = []
     
    -        def _capture_post(url, json=None, timeout=None):
    +        def _capture_post(url, json=None, timeout=None, headers=None):
                 requests_seen.append(json)
                 return _mock_response(
                     json_data={"tabId": f"tab-{len(requests_seen)}", "url": "https://example.com"}

  • tests/tools/test_browser_camofox_timeout.py

    @@ -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

  • tools/browser_camofox.py

    @@ -36,7 +36,7 @@
     
     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,47 @@
     # 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."""
    +    key = os.getenv("CAMOFOX_API_KEY", "").strip()
    +    if key:
    +        return {"Authorization": f"Bearer {key}"}
    +    return {}
    +
     
     def get_camofox_url() -> str:
         """Return the configured Camofox server URL, or empty string."""
    @@ -345,10 +381,11 @@ def _ensure_tab(task_id: Optional[str], url: str = "about:blank") -> Dict[str, A
             f"{base}/tabs",
             json={
                 "userId": session["user_id"],
    -            "sessionKey": session["session_key"],
    +            "listItemId": session["session_key"],
                 "url": url,
             },
    -        timeout=_DEFAULT_TIMEOUT,
    +        timeout=_get_command_timeout(),
    +        headers=_auth_headers(),
         )
         resp.raise_for_status()
         data = resp.json()
    @@ -384,34 +421,42 @@ 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)
    +    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)
    +    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)
    +    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)
    +    resp = requests.delete(url, json=body, timeout=timeout, headers=_auth_headers())
         resp.raise_for_status()
         return resp.json()
     
    @@ -430,12 +475,25 @@ def camofox_navigate(url: str, task_id: Optional[str] = None) -> str:
                 session = _ensure_tab(task_id, browser_url)
                 data = {"ok": True, "url": browser_url}
             else:
    -            # Navigate existing tab
    -            data = _post(
    -                f"/tabs/{session['tab_id']}/navigate",
    -                {"userId": session["user_id"], "url": browser_url},
    -                timeout=60,
    -            )
    +            # Navigate existing tab — recover from stale tab 404
    +            try:
    +                data = _post(
    +                    f"/tabs/{session['tab_id']}/navigate",
    +                    {"userId": session["user_id"], "url": browser_url},
    +                    timeout=60,
    +                )
    +            except requests.HTTPError as e:
    +                if e.response is not None and e.response.status_code == 404:
    +                    logger.warning(
    +                        "Camofox tab %s returned 404 — tab was garbage collected. "
    +                        "Creating a fresh tab.",
    +                        session["tab_id"],
    +                    )
    +                    session["tab_id"] = None
    +                    session = _ensure_tab(task_id, browser_url)
    +                    data = {"ok": True, "url": browser_url}
    +                else:
    +                    raise
             result = {
                 "success": True,
                 "url": data.get("url", browser_url),