Merge branch 'main' of github.com:NousResearch/hermes-agent into bb/gui

# Conflicts:
#	tui_gateway/server.py

tests/tools/test_browser_ssrf_local.py

@@ -106,6 +106,62 @@ class TestPreNavigationSsrf:
 
         assert result["success"] is True
 
+    # -- Always-blocked floor: hybrid routing bypass regression (#16234) -------
+
+    # Hybrid-routing feature flips auto_local_this_nav=True for private URLs,
+    # which previously short-circuited _is_safe_url() entirely. An agent
+    # running on EC2/GCP/Azure could navigate to 169.254.169.254 via the
+    # spawned local Chromium sidecar and read IAM credentials via
+    # browser_snapshot. The always-blocked floor must fire regardless of
+    # routing.
+    IMDS_URLS = [
+        "http://169.254.169.254/latest/meta-data/",      # AWS / GCP / Azure / DO / Oracle
+        "http://169.254.169.253/metadata/instance",        # Azure IMDS wire server
+        "http://169.254.170.2/v2/credentials",             # AWS ECS task metadata
+        "http://100.100.100.200/latest/meta-data/",        # Alibaba Cloud
+        "http://metadata.google.internal/computeMetadata/v1/",  # GCP hostname
+    ]
+
+    @pytest.mark.parametrize("imds_url", IMDS_URLS)
+    def test_cloud_blocks_imds_even_when_routing_to_local_sidecar(
+        self, monkeypatch, _common_patches, imds_url
+    ):
+        """Hybrid routing must not let cloud metadata endpoints through."""
+        monkeypatch.setattr(browser_tool, "_is_local_backend", lambda: False)
+        monkeypatch.setattr(browser_tool, "_allow_private_urls", lambda: False)
+        # Simulate hybrid routing kicking in for this URL (what happens on
+        # main pre-fix — cloud provider configured, _url_is_private → True,
+        # so the session key routes to a local Chromium sidecar).
+        monkeypatch.setattr(browser_tool, "_is_local_sidecar_key", lambda key: True)
+        # _is_safe_url would catch IMDS, but pre-fix it never ran. Force
+        # it to return True here so the test is specifically pinning the
+        # always-blocked floor as an independent gate.
+        monkeypatch.setattr(browser_tool, "_is_safe_url", lambda url: True)
+
+        result = json.loads(browser_tool.browser_navigate(imds_url))
+
+        assert result["success"] is False
+        assert "cloud metadata endpoint" in result["error"]
+
+    def test_cloud_allows_ordinary_private_url_via_sidecar(
+        self, monkeypatch, _common_patches
+    ):
+        """Hybrid routing still works for ordinary private URLs — floor
+        must be narrow enough to not break the PR #16136 feature."""
+        monkeypatch.setattr(browser_tool, "_is_local_backend", lambda: False)
+        monkeypatch.setattr(browser_tool, "_allow_private_urls", lambda: False)
+        monkeypatch.setattr(browser_tool, "_is_local_sidecar_key", lambda key: True)
+        monkeypatch.setattr(browser_tool, "_is_safe_url", lambda url: False)
+
+        for private in (
+            "http://127.0.0.1:8080/dashboard",
+            "http://192.168.1.1/admin",
+            "http://10.0.0.5/",
+            "http://myservice.local/",
+        ):
+            result = json.loads(browser_tool.browser_navigate(private))
+            assert result["success"] is True, f"Unexpected block for {private}: {result}"
+
 
 # ---------------------------------------------------------------------------
 # _is_local_backend() unit tests
@@ -236,6 +292,32 @@ class TestPostRedirectSsrf:
         assert result["success"] is True
         assert result["url"] == final
 
+    # -- Always-blocked floor: redirect to IMDS via hybrid sidecar (#16234) ----
+
+    def test_cloud_blocks_redirect_to_imds_even_via_sidecar(
+        self, monkeypatch, _common_patches
+    ):
+        """Redirect to a cloud metadata endpoint is blocked regardless of
+        routing — even the hybrid local sidecar path can't return IMDS
+        content to the agent."""
+        imds_final = "http://169.254.169.254/latest/meta-data/"
+        monkeypatch.setattr(browser_tool, "_is_local_backend", lambda: False)
+        monkeypatch.setattr(browser_tool, "_allow_private_urls", lambda: False)
+        monkeypatch.setattr(browser_tool, "_is_local_sidecar_key", lambda key: True)
+        # _is_safe_url would catch it on main; force True to pin the
+        # always-blocked floor as an independent gate.
+        monkeypatch.setattr(browser_tool, "_is_safe_url", lambda url: True)
+        monkeypatch.setattr(
+            browser_tool,
+            "_run_browser_command",
+            lambda *a, **kw: _make_browser_result(url=imds_final),
+        )
+
+        result = json.loads(browser_tool.browser_navigate(self.PUBLIC_URL))
+
+        assert result["success"] is False
+        assert "cloud metadata endpoint" in result["error"]
+
 
 class TestAllowPrivateUrlsConfig:
     @pytest.fixture(autouse=True)

tests/tools/test_delegate_composite_toolsets.py

@@ -0,0 +1,46 @@
+"""Tests for composite toolset expansion in delegate_task intersection."""
+
+import unittest
+from unittest.mock import patch
+
+from tools.delegate_tool import _expand_parent_toolsets
+
+
+class TestExpandParentToolsets(unittest.TestCase):
+    """Verify _expand_parent_toolsets recognises individual toolsets within composites."""
+
+    def test_composite_hermes_cli_expands_web(self):
+        """hermes-cli includes web_search/web_extract → 'web' should be in expansion."""
+        expanded = _expand_parent_toolsets({"hermes-cli"})
+        self.assertIn("web", expanded)
+        self.assertIn("terminal", expanded)
+        self.assertIn("browser", expanded)
+        # Original composite is preserved
+        self.assertIn("hermes-cli", expanded)
+
+    def test_individual_toolset_unchanged(self):
+        """When parent already uses individual toolsets, expansion keeps them."""
+        expanded = _expand_parent_toolsets({"web", "terminal"})
+        self.assertIn("web", expanded)
+        self.assertIn("terminal", expanded)
+
+    def test_empty_parent_toolsets(self):
+        expanded = _expand_parent_toolsets(set())
+        self.assertEqual(expanded, set())
+
+    def test_unknown_toolset_passthrough(self):
+        """Unknown toolset names pass through without error."""
+        expanded = _expand_parent_toolsets({"nonexistent-toolset-xyz"})
+        self.assertIn("nonexistent-toolset-xyz", expanded)
+
+    def test_intersection_with_expanded_composite(self):
+        """End-to-end: requesting ['web'] from parent with ['hermes-cli'] yields ['web']."""
+        parent_toolsets = {"hermes-cli"}
+        expanded = _expand_parent_toolsets(parent_toolsets)
+        toolsets = ["web"]
+        child_toolsets = [t for t in toolsets if t in expanded]
+        self.assertEqual(child_toolsets, ["web"])
+
+
+if __name__ == "__main__":
+    unittest.main()

tests/tools/test_discord_tool.py

@@ -175,6 +175,12 @@ class TestDiscordServerValidation:
         assert "error" in result
         assert "channel_id" in result["error"]
 
+    def test_missing_required_message_id_for_delete(self, monkeypatch):
+        monkeypatch.setenv("DISCORD_BOT_TOKEN", "test-token")
+        result = json.loads(discord_admin_handler(action="delete_message", channel_id="11"))
+        assert "error" in result
+        assert "message_id" in result["error"]
+
     def test_missing_multiple_params(self, monkeypatch):
         monkeypatch.setenv("DISCORD_BOT_TOKEN", "test-token")
         result = json.loads(discord_admin_handler(action="add_role"))
@@ -407,10 +413,10 @@ class TestListPins:
 
 
 # ---------------------------------------------------------------------------
-# Actions: pin_message / unpin_message
+# Actions: pin_message / unpin_message / delete_message
 # ---------------------------------------------------------------------------
 
-class TestPinUnpin:
+class TestPinUnpinDelete:
     @patch("tools.discord_tool._discord_request")
     def test_pin_message(self, mock_req, monkeypatch):
         monkeypatch.setenv("DISCORD_BOT_TOKEN", "test-token")
@@ -425,6 +431,16 @@ class TestPinUnpin:
         mock_req.return_value = None
         result = json.loads(discord_admin_handler(action="unpin_message", channel_id="11", message_id="500"))
         assert result["success"] is True
+        mock_req.assert_called_once_with("DELETE", "/channels/11/pins/500", "test-token")
+
+    @patch("tools.discord_tool._discord_request")
+    def test_delete_message(self, mock_req, monkeypatch):
+        monkeypatch.setenv("DISCORD_BOT_TOKEN", "test-token")
+        mock_req.return_value = None
+        result = json.loads(discord_admin_handler(action="delete_message", channel_id="11", message_id="500"))
+        assert result["success"] is True
+        assert "deleted" in result["message"]
+        mock_req.assert_called_once_with("DELETE", "/channels/11/messages/500", "test-token")
 
 
 # ---------------------------------------------------------------------------
@@ -586,6 +602,7 @@ class TestRegistration:
         desc = entry.schema["description"]
         assert "list_guilds()" in desc
         assert "add_role(guild_id, user_id, role_id)" in desc
+        assert "delete_message(channel_id, message_id)" in desc
         # Core actions should NOT be in admin description
         assert "fetch_messages(" not in desc
         assert "create_thread(" not in desc

tests/tools/test_dockerfile_node_modules_perms.py

@@ -0,0 +1,39 @@
+"""contract test: dockerfile chowns runtime node_modules trees to hermes
+
+regression guard for #18800. the container drops privileges to the hermes
+user (uid 10000) in entrypoint.sh, then the TUI launcher's
+_tui_need_npm_install() trips on every startup (see the
+npm_config_install_links=false comment in the Dockerfile) and runs
+`npm install` in /opt/hermes/ui-tui. that install fails with EACCES unless
+the runtime node_modules trees are owned by hermes.
+"""
+from __future__ import annotations
+
+from pathlib import Path
+
+REPO_ROOT = Path(__file__).resolve().parents[2]
+DOCKERFILE = REPO_ROOT / "Dockerfile"
+
+
+def test_dockerfile_chowns_runtime_node_modules_to_hermes_user() -> None:
+    text = DOCKERFILE.read_text()
+
+    chown_lines = [
+        line for line in text.splitlines()
+        if "chown" in line and "hermes:hermes" in line
+    ]
+    assert chown_lines, (
+        "Dockerfile must contain a chown -R hermes:hermes for the runtime "
+        "node_modules trees; see #18800"
+    )
+
+    chown_block = "\n".join(chown_lines)
+
+    # both runtime-mutable trees must be passed to the chown command.
+    # /opt/hermes/web is intentionally excluded: it is build-time only,
+    # because HERMES_WEB_DIST points at hermes_cli/web_dist for runtime.
+    for required_path in ("/opt/hermes/ui-tui", "/opt/hermes/node_modules"):
+        assert required_path in chown_block, (
+            f"{required_path} must be passed to a chown -R hermes:hermes "
+            f"command in the Dockerfile (see #18800)"
+        )

tests/tools/test_dockerfile_pid1_reaping.py

@@ -106,8 +106,15 @@ def test_dockerfile_entrypoint_routes_through_the_init(dockerfile_text):
 
 
 def test_dockerfile_installs_tui_dependencies(dockerfile_text):
+    # The TUI workspace manifests must be present so ``npm install`` can
+    # resolve dependencies. The bundled ``hermes-ink`` workspace package is
+    # now COPIED into the image as a whole tree (not just its lockfile)
+    # because it's referenced as a ``file:`` workspace dependency from
+    # ``ui-tui/package.json`` — copying the tree avoids npm stopping at a
+    # bare ``package.json`` shell.
     assert "ui-tui/package.json" in dockerfile_text
-    assert "ui-tui/packages/hermes-ink/package-lock.json" in dockerfile_text
+    assert "ui-tui/package-lock.json" in dockerfile_text
+    assert "ui-tui/packages/hermes-ink/" in dockerfile_text
     assert any(
         "ui-tui" in step and "npm" in step and (" install" in step or " ci" in step)
         for step in _run_steps(dockerfile_text)
@@ -122,16 +129,17 @@ def test_dockerfile_builds_tui_assets(dockerfile_text):
 
 
 def test_dockerfile_materializes_local_tui_ink_package(dockerfile_text):
-    assert any(
-        "ui-tui" in step
-        and "node_modules/@hermes/ink" in step
-        and "packages/hermes-ink" in step
-        and "rm -rf packages/hermes-ink/node_modules" in step
-        and "npm install --omit=dev" in step
-        and "--prefix node_modules/@hermes/ink" in step
-        and "rm -rf node_modules/@hermes/ink/node_modules/react" in step
-        and "await import('@hermes/ink')" in step
-        for step in _run_steps(dockerfile_text)
+    # ``hermes-ink`` is a bundled workspace package referenced from
+    # ``ui-tui/package.json`` via ``file:`` — not pulled from the npm
+    # registry. The contract this test pins is just that the image
+    # actually carries the package source so ``await import('@hermes/ink')``
+    # can resolve at runtime; the previous, much pickier assertion (manual
+    # ``rm -rf`` + ``npm install --omit=dev --prefix node_modules/@hermes/ink``)
+    # baked in implementation details of an older materialisation flow that
+    # was simplified once npm workspaces handled the resolution natively.
+    assert "ui-tui/packages/hermes-ink/" in dockerfile_text, (
+        "Dockerfile must COPY the bundled hermes-ink workspace package "
+        "so ``await import('@hermes/ink')`` resolves at runtime."
     )
 
 

tests/tools/test_kanban_tools.py

@@ -214,6 +214,61 @@ def test_heartbeat_without_note(worker_env):
     assert d["ok"] is True
 
 
+def test_heartbeat_extends_claim_expires(worker_env):
+    """The kanban_heartbeat tool MUST extend claim_expires, not just
+    update last_heartbeat_at — otherwise long-running workers loop the
+    heartbeat tool diligently and still get reclaimed by
+    release_stale_claims at DEFAULT_CLAIM_TTL_SECONDS.
+
+    Regression test for the bug where _handle_heartbeat called
+    heartbeat_worker but never heartbeat_claim, so claim_expires sat
+    static while last_heartbeat_at advanced.
+    """
+    import time as _time
+    from hermes_cli import kanban_db as kb
+    from tools import kanban_tools as kt
+
+    # Rewind claim_expires into the past so any forward movement is
+    # unambiguous (avoids time.sleep flakiness).
+    conn = kb.connect()
+    try:
+        conn.execute(
+            "UPDATE tasks SET claim_expires = ? WHERE id = ?",
+            (1, worker_env),
+        )
+        conn.commit()
+        before = conn.execute(
+            "SELECT claim_expires FROM tasks WHERE id = ?", (worker_env,)
+        ).fetchone()["claim_expires"]
+    finally:
+        conn.close()
+    assert before == 1
+
+    out = kt._handle_heartbeat({"note": "still alive"})
+    assert json.loads(out).get("ok") is True
+
+    conn = kb.connect()
+    try:
+        after = conn.execute(
+            "SELECT claim_expires FROM tasks WHERE id = ?", (worker_env,)
+        ).fetchone()["claim_expires"]
+    finally:
+        conn.close()
+
+    now = int(_time.time())
+    # claim_expires should be roughly now + DEFAULT_CLAIM_TTL_SECONDS.
+    # We assert a generous floor (now + half the default TTL) to keep the
+    # test stable against future TTL changes.
+    assert after > before, (
+        f"claim_expires did not advance ({before} -> {after}); workers "
+        f"would be reclaimed at TTL despite heartbeating"
+    )
+    assert after >= now + (kb.DEFAULT_CLAIM_TTL_SECONDS // 2), (
+        f"claim_expires={after} is suspiciously close to now={now}; "
+        f"expected at least now + {kb.DEFAULT_CLAIM_TTL_SECONDS // 2}"
+    )
+
+
 def test_comment_happy_path(worker_env):
     from tools import kanban_tools as kt
     out = kt._handle_comment({

tests/tools/test_mcp_cancelled_error_propagation.py

@@ -0,0 +1,92 @@
+"""Regression tests for ``MCPServerTask.run`` + ``asyncio.CancelledError``.
+
+Background
+==========
+On Python 3.11+, ``asyncio.CancelledError`` inherits from ``BaseException``
+rather than ``Exception``, so a bare ``except Exception`` does NOT catch it.
+``MCPServerTask.run`` had a broad ``except Exception`` around the transport
+loop which meant a task cancellation (gateway restart, explicit
+``task.cancel()``) caused the reconnect loop to exit silently — the MCP
+server stayed dead until Hermes was restarted. See #9930.
+
+The fix adds an explicit ``except asyncio.CancelledError: raise`` BEFORE
+the broad catch so cancellation propagates cleanly to asyncio's task
+machinery and ``MCPServerTask.shutdown()``'s ``await self._task`` completes
+without hanging the reconnect loop.
+"""
+
+from __future__ import annotations
+
+import asyncio
+from unittest.mock import patch
+
+import pytest
+
+
+async def _hanging_run(self, cfg):
+    """Stand-in transport that hangs forever so we can cancel it."""
+    await asyncio.sleep(3600)
+
+
+class TestCancelledErrorPropagation:
+    def test_cancelled_error_is_not_swallowed_by_except_exception(self):
+        """CancelledError raised inside the transport call must re-raise
+        so the reconnect loop terminates cleanly on cancel — not stay wedged."""
+        from tools.mcp_tool import MCPServerTask
+
+        server = MCPServerTask("cancel-test")
+
+        async def drive():
+            with patch.object(MCPServerTask, "_run_stdio", _hanging_run), \
+                 patch.object(MCPServerTask, "_is_http", lambda self: False):
+                task = asyncio.create_task(server.run({"command": "fake"}))
+                # Let the run loop enter the try/except and start awaiting.
+                await asyncio.sleep(0.05)
+                task.cancel()
+                # The fix guarantees the task completes (either via
+                # CancelledError propagation or clean exit) rather than
+                # hanging forever.
+                try:
+                    await asyncio.wait_for(task, timeout=2.0)
+                except asyncio.CancelledError:
+                    return "cancelled_cleanly"
+                except asyncio.TimeoutError:
+                    # If we hit this, the reconnect loop swallowed the cancel
+                    # and stayed wedged — the exact #9930 bug.
+                    task.cancel()
+                    try:
+                        await task
+                    except Exception:
+                        pass
+                    return "wedged"
+                return "clean_return"
+
+        outcome = asyncio.run(drive())
+        assert outcome in ("cancelled_cleanly", "clean_return"), (
+            f"MCPServerTask.run wedged on cancel (outcome={outcome}) — "
+            f"#9930 regression"
+        )
+
+    def test_shutdown_completes_promptly_when_task_is_cancelled(self):
+        """``shutdown()`` falls through to ``task.cancel()`` + ``await self._task``
+        after a grace period. That cancel must unwedge the reconnect loop —
+        otherwise ``await self._task`` hangs indefinitely."""
+        from tools.mcp_tool import MCPServerTask
+
+        server = MCPServerTask("shutdown-cancel-test")
+
+        async def drive():
+            with patch.object(MCPServerTask, "_run_stdio", _hanging_run), \
+                 patch.object(MCPServerTask, "_is_http", lambda self: False):
+                server._task = asyncio.ensure_future(server.run({"command": "fake"}))
+                await asyncio.sleep(0.05)
+                server._shutdown_event.set()
+                server._task.cancel()
+                try:
+                    await asyncio.wait_for(server._task, timeout=2.0)
+                except (asyncio.CancelledError, asyncio.TimeoutError):
+                    pass
+                return server._task.done()
+
+        done = asyncio.run(drive())
+        assert done, "MCPServerTask did not finish after cancel — #9930 regression"

tests/tools/test_mcp_empty_error_message.py

@@ -0,0 +1,89 @@
+"""Regression tests for MCP error messages when str(exc) is empty.
+
+Issue #19417: ClosedResourceError (and similar exceptions raised without a
+message argument) produced ``MCP call failed: ClosedResourceError: `` with
+nothing after the colon, making debugging impossible.
+
+Fix: ``_exc_str()`` falls back to ``repr(exc)`` when ``str(exc)`` is empty.
+"""
+
+import json
+from types import SimpleNamespace
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from tools.mcp_tool import _exc_str, _sanitize_error
+
+
+# ---------------------------------------------------------------------------
+# _exc_str unit tests
+# ---------------------------------------------------------------------------
+
+
+class _EmptyMessageError(Exception):
+    """Exception whose __str__ returns empty string (like anyio.ClosedResourceError)."""
+
+    def __str__(self):
+        return ""
+
+
+class _NormalError(Exception):
+    pass
+
+
+def test_exc_str_returns_str_when_nonempty():
+    exc = _NormalError("something broke")
+    assert _exc_str(exc) == "something broke"
+
+
+def test_exc_str_falls_back_to_repr_when_str_empty():
+    exc = _EmptyMessageError()
+    result = _exc_str(exc)
+    assert result != ""
+    assert "_EmptyMessageError" in result
+
+
+def test_exc_str_falls_back_to_repr_for_whitespace_only():
+    """str(exc) that is only whitespace should also trigger the repr fallback."""
+    exc = Exception("   ")
+    result = _exc_str(exc)
+    # After strip(), the text is empty, so repr is used
+    assert result.strip() != ""
+
+
+def test_exc_str_handles_closedresource_like_exception():
+    """Simulate anyio.ClosedResourceError which has no message."""
+    # Replicate the real anyio.ClosedResourceError behavior
+    exc = type("ClosedResourceError", (Exception,), {"__str__": lambda self: ""})()
+    result = _exc_str(exc)
+    assert "ClosedResourceError" in result
+    assert result != ""
+
+
+# ---------------------------------------------------------------------------
+# Integration: error message format in _sanitize_error
+# ---------------------------------------------------------------------------
+
+
+def test_error_message_not_empty_when_exc_has_no_message():
+    """The formatted error string should always contain the exception class name."""
+    exc = _EmptyMessageError()
+    error_msg = _sanitize_error(
+        f"MCP call failed: {type(exc).__name__}: {_exc_str(exc)}"
+    )
+    assert "ClosedResourceError" not in error_msg or "_EmptyMessageError" in error_msg
+    # The key invariant: the message must not end with ": "
+    assert not error_msg.endswith(": ")
+    # And it must contain the exception type name
+    assert "_EmptyMessageError" in error_msg
+
+
+def test_error_message_preserves_normal_exception_text():
+    """Normal exceptions should still show their message text."""
+    exc = _NormalError("connection refused")
+    error_msg = _sanitize_error(
+        f"MCP call failed: {type(exc).__name__}: {_exc_str(exc)}"
+    )
+    assert "connection refused" in error_msg
+    assert "_NormalError" in error_msg

tests/tools/test_mcp_image_content.py

@@ -0,0 +1,138 @@
+"""Regression tests for MCP ImageContent block handling.
+
+Background
+==========
+MCP tool results may include ``ImageContent`` blocks (screenshots from
+Playwright / Blockbench / Puppeteer / any server that returns renders).
+The tool result handler in ``tools/mcp_tool.py`` used to iterate content
+blocks looking only for ``block.text`` — image blocks were silently dropped
+and the agent saw an empty result. Distilled from @c3115644151's PR #17915
+and @gnanirahulnutakki's PR #10848 (both too stale to cherry-pick); this
+test file locks in #10848's approach of plumbing the bytes through
+Hermes' existing ``cache_image_from_bytes`` so a ``MEDIA:<path>`` tag
+goes back to the agent and through to messaging adapters that render
+images natively.
+"""
+
+from __future__ import annotations
+
+import base64
+from types import SimpleNamespace
+from unittest.mock import patch
+
+import pytest
+
+
+def _png_bytes():
+    """Return a minimal valid PNG byte sequence.
+
+    Hermes' ``cache_image_from_bytes`` has a format-sniff guard that rejects
+    non-image payloads — use a real PNG signature so the test exercises the
+    full pipeline instead of the reject path.
+    """
+    # 1x1 transparent PNG
+    return base64.b64decode(
+        "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII="
+    )
+
+
+class TestMimeExtension:
+    def test_maps_jpeg_variants_to_jpg(self):
+        from tools.mcp_tool import _mcp_image_extension_for_mime_type
+        assert _mcp_image_extension_for_mime_type("image/jpeg") == ".jpg"
+        assert _mcp_image_extension_for_mime_type("image/jpg") == ".jpg"
+        assert _mcp_image_extension_for_mime_type("IMAGE/JPEG") == ".jpg"
+        assert _mcp_image_extension_for_mime_type("image/jpeg; charset=utf-8") == ".jpg"
+
+    def test_png_falls_through_to_mimetypes(self):
+        from tools.mcp_tool import _mcp_image_extension_for_mime_type
+        assert _mcp_image_extension_for_mime_type("image/png") == ".png"
+
+    def test_unknown_defaults_to_png(self):
+        from tools.mcp_tool import _mcp_image_extension_for_mime_type
+        assert _mcp_image_extension_for_mime_type("") == ".png"
+        assert _mcp_image_extension_for_mime_type("image/unheard-of-format") == ".png"
+
+
+class TestCacheMcpImageBlock:
+    def test_returns_media_tag_for_valid_image_block(self, tmp_path, monkeypatch):
+        """A well-formed ImageContent block with valid PNG bytes caches
+        to the image dir and the helper returns a ``MEDIA:<path>`` tag."""
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        from tools.mcp_tool import _cache_mcp_image_block
+
+        block = SimpleNamespace(
+            data=base64.b64encode(_png_bytes()).decode("ascii"),
+            mimeType="image/png",
+        )
+        tag = _cache_mcp_image_block(block)
+        assert tag.startswith("MEDIA:"), f"expected MEDIA: tag, got {tag!r}"
+        # The cached file should be in Hermes' image cache dir
+        from gateway.platforms.base import get_image_cache_dir
+        cache_dir = str(get_image_cache_dir().resolve())
+        assert tag.startswith(f"MEDIA:{cache_dir}"), (
+            f"cached file not under HERMES_HOME image cache dir. "
+            f"tag={tag!r}, cache_dir={cache_dir!r}"
+        )
+        # And it should exist + have the PNG bytes
+        path = tag[len("MEDIA:"):]
+        with open(path, "rb") as fh:
+            assert fh.read() == _png_bytes()
+
+    def test_returns_empty_when_block_is_not_an_image(self, tmp_path, monkeypatch):
+        """Non-image MIME types shouldn't trigger caching."""
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        from tools.mcp_tool import _cache_mcp_image_block
+
+        block = SimpleNamespace(
+            data=base64.b64encode(b"some bytes").decode("ascii"),
+            mimeType="application/pdf",
+        )
+        assert _cache_mcp_image_block(block) == ""
+
+    def test_returns_empty_when_block_has_no_data(self, tmp_path, monkeypatch):
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        from tools.mcp_tool import _cache_mcp_image_block
+
+        block = SimpleNamespace(data=None, mimeType="image/png")
+        assert _cache_mcp_image_block(block) == ""
+
+    def test_returns_empty_on_malformed_base64(self, tmp_path, monkeypatch):
+        """A server that sends garbage base64 shouldn't crash the handler —
+        we log and drop the block, letting any text blocks still come through."""
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        from tools.mcp_tool import _cache_mcp_image_block
+
+        block = SimpleNamespace(
+            data="!!!not-base64!!!",
+            mimeType="image/png",
+        )
+        assert _cache_mcp_image_block(block) == ""
+
+    def test_returns_empty_when_bytes_dont_look_like_an_image(self, tmp_path, monkeypatch):
+        """``cache_image_from_bytes`` has a format sniff; if the claimed
+        ``image/png`` is actually an HTML error page, the cache raises and
+        we log + drop rather than propagate."""
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        from tools.mcp_tool import _cache_mcp_image_block
+
+        block = SimpleNamespace(
+            data=base64.b64encode(b"<html>error</html>").decode("ascii"),
+            mimeType="image/png",
+        )
+        assert _cache_mcp_image_block(block) == ""
+
+    def test_handles_jpeg(self, tmp_path, monkeypatch):
+        """JPEG signature should also be accepted."""
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        from tools.mcp_tool import _cache_mcp_image_block
+
+        # minimal JPEG SOI marker + filler
+        jpeg = b"\xff\xd8\xff\xe0" + b"\x00" * 100 + b"\xff\xd9"
+        block = SimpleNamespace(
+            data=base64.b64encode(jpeg).decode("ascii"),
+            mimeType="image/jpeg",
+        )
+        tag = _cache_mcp_image_block(block)
+        assert tag.startswith("MEDIA:")
+        assert tag.endswith(".jpg"), f"expected .jpg extension, got {tag!r}"

tests/tools/test_mcp_oauth.py

@@ -2,6 +2,8 @@
 
 import json
 import os
+import stat
+import sys
 from io import BytesIO
 from pathlib import Path
 from unittest.mock import patch, MagicMock, AsyncMock
@@ -50,6 +52,37 @@ class TestHermesTokenStorage:
         data = json.loads(token_path.read_text())
         assert data["access_token"] == "abc123"
 
+    @pytest.mark.skipif(sys.platform.startswith("win"), reason="POSIX mode bits not enforced on Windows")
+    def test_token_file_created_with_0o600(self, tmp_path, monkeypatch):
+        """Tokens must land on disk at 0o600 with no umask-default exposure window.
+
+        Regression for the TOCTOU race where ``write_text`` + post-write
+        ``chmod`` briefly left credentials at the process umask (commonly
+        0o644 = world-readable) before tightening to owner-only. Mirrors
+        the fix shipped for ``agent/google_oauth.py`` in #19673.
+        """
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        storage = HermesTokenStorage("perm-test-server")
+
+        import asyncio
+        mock_token = MagicMock()
+        mock_token.model_dump.return_value = {
+            "access_token": "secret-abc",
+            "token_type": "Bearer",
+            "refresh_token": "secret-ref",
+        }
+        asyncio.run(storage.set_tokens(mock_token))
+
+        token_path = tmp_path / "mcp-tokens" / "perm-test-server.json"
+        assert token_path.exists()
+        mode = stat.S_IMODE(token_path.stat().st_mode)
+        assert mode == 0o600, f"token file mode {oct(mode)} != 0o600 — TOCTOU race regressed"
+
+        parent_mode = stat.S_IMODE(token_path.parent.stat().st_mode)
+        assert parent_mode == 0o700, (
+            f"token parent dir mode {oct(parent_mode)} != 0o700 — siblings can traverse"
+        )
+
     def test_roundtrip_client_info(self, tmp_path, monkeypatch):
         monkeypatch.setenv("HERMES_HOME", str(tmp_path))
         storage = HermesTokenStorage("test-server")

tests/tools/test_mcp_oauth_metadata.py

@@ -0,0 +1,213 @@
+"""Tests for OAuth server metadata persistence across process restarts.
+
+Covers:
+- :class:`HermesTokenStorage` ``.meta.json`` roundtrip (save / load / remove)
+- The production manager provider
+  (:class:`tools.mcp_oauth_manager.HermesMCPOAuthProvider`) restoring metadata
+  on cold-load init and persisting metadata at the end of ``async_auth_flow``.
+
+Context
+=======
+The MCP SDK discovers OAuth server metadata (``token_endpoint``, etc.)
+on-demand and keeps it in memory only. Without disk persistence a restart
+forces the SDK to fall back to guessing ``{server_url}/token``, which returns
+404 on most real providers and triggers a full browser re-auth even when the
+refresh token is still valid. These tests lock in the disk persistence
+layer so refresh across restarts stays quiet.
+"""
+
+from __future__ import annotations
+
+import asyncio
+import json
+from unittest.mock import AsyncMock, MagicMock, patch
+
+import pytest
+
+from mcp.shared.auth import OAuthMetadata
+
+from tools.mcp_oauth import HermesTokenStorage
+from tools.mcp_oauth_manager import _HERMES_PROVIDER_CLS
+
+
+def _make_metadata(token_endpoint: str = "https://auth.example.com/oauth/token") -> OAuthMetadata:
+    return OAuthMetadata.model_validate(
+        {
+            "issuer": "https://auth.example.com",
+            "authorization_endpoint": "https://auth.example.com/oauth/authorize",
+            "token_endpoint": token_endpoint,
+            "response_types_supported": ["code"],
+        }
+    )
+
+
+# ---------------------------------------------------------------------------
+# HermesTokenStorage metadata roundtrip
+# ---------------------------------------------------------------------------
+
+
+class TestMetadataStorage:
+    def test_save_and_load_roundtrip(self, tmp_path, monkeypatch):
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        storage = HermesTokenStorage("example-server")
+
+        meta = _make_metadata()
+        storage.save_oauth_metadata(meta)
+
+        meta_path = tmp_path / "mcp-tokens" / "example-server.meta.json"
+        assert meta_path.exists()
+
+        loaded = storage.load_oauth_metadata()
+        assert loaded is not None
+        assert str(loaded.token_endpoint) == "https://auth.example.com/oauth/token"
+        assert str(loaded.issuer).rstrip("/") == "https://auth.example.com"
+
+    def test_load_missing_returns_none(self, tmp_path, monkeypatch):
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        storage = HermesTokenStorage("nonexistent")
+        assert storage.load_oauth_metadata() is None
+
+    def test_load_corrupt_returns_none(self, tmp_path, monkeypatch):
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        storage = HermesTokenStorage("corrupt-server")
+
+        # Write something that doesn't validate as OAuthMetadata
+        meta_path = storage._meta_path()
+        meta_path.parent.mkdir(parents=True, exist_ok=True)
+        meta_path.write_text(json.dumps({"issuer": "not-a-url", "wrong_field": 123}))
+
+        assert storage.load_oauth_metadata() is None
+
+    def test_remove_deletes_meta_file(self, tmp_path, monkeypatch):
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        storage = HermesTokenStorage("cleanup-server")
+
+        storage.save_oauth_metadata(_make_metadata())
+        assert storage._meta_path().exists()
+
+        storage.remove()
+        assert not storage._meta_path().exists()
+
+
+# ---------------------------------------------------------------------------
+# Manager-path provider (HermesMCPOAuthProvider) — production code path
+# ---------------------------------------------------------------------------
+
+
+def _manager_provider_with_context(storage: HermesTokenStorage, **context_attrs):
+    """Build an uninitialized manager provider with a mocked context.
+
+    Bypasses the full OAuthClientProvider init so we can exercise the
+    override logic in isolation.
+    """
+    if _HERMES_PROVIDER_CLS is None:
+        pytest.skip("MCP SDK auth not available")
+    provider = _HERMES_PROVIDER_CLS.__new__(_HERMES_PROVIDER_CLS)
+    provider._hermes_server_name = context_attrs.get("server_name", "srv")
+    context = MagicMock()
+    context.storage = storage
+    context.oauth_metadata = context_attrs.get("oauth_metadata")
+    context.current_tokens = context_attrs.get("current_tokens")
+    context.server_url = context_attrs.get("server_url", "https://example.com")
+    context.update_token_expiry = MagicMock()
+    provider.context = context
+    return provider
+
+
+class TestManagerOAuthProviderMetadata:
+    def test_initialize_restores_metadata_from_disk(self, tmp_path, monkeypatch):
+        """Cold-load: if we have no in-memory metadata but disk has some, restore it."""
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        storage = HermesTokenStorage("mgr-srv")
+        storage.save_oauth_metadata(_make_metadata("https://mgr.example.com/token"))
+        provider = _manager_provider_with_context(storage, oauth_metadata=None)
+
+        with patch.object(
+            _HERMES_PROVIDER_CLS.__bases__[0], "_initialize", new=AsyncMock()
+        ):
+            asyncio.run(provider._initialize())
+
+        assert provider.context.oauth_metadata is not None
+        assert str(provider.context.oauth_metadata.token_endpoint) == \
+            "https://mgr.example.com/token"
+
+    def test_initialize_skips_restore_when_in_memory_present(self, tmp_path, monkeypatch):
+        """If SDK already has metadata in memory, don't overwrite from disk."""
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        storage = HermesTokenStorage("mgr-srv2")
+        storage.save_oauth_metadata(_make_metadata("https://disk.example.com/token"))
+        in_memory = _make_metadata("https://memory.example.com/token")
+
+        provider = _manager_provider_with_context(storage, oauth_metadata=in_memory)
+
+        with patch.object(
+            _HERMES_PROVIDER_CLS.__bases__[0], "_initialize", new=AsyncMock()
+        ):
+            asyncio.run(provider._initialize())
+
+        assert str(provider.context.oauth_metadata.token_endpoint) == \
+            "https://memory.example.com/token"
+
+    def test_persist_metadata_if_changed_writes_on_first_discover(self, tmp_path, monkeypatch):
+        """When nothing on disk yet, persist what the SDK discovered in-memory."""
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        storage = HermesTokenStorage("persist-srv")
+        assert storage.load_oauth_metadata() is None
+
+        discovered = _make_metadata("https://discovered.example.com/token")
+        provider = _manager_provider_with_context(storage, oauth_metadata=discovered)
+
+        provider._persist_oauth_metadata_if_changed()
+
+        loaded = storage.load_oauth_metadata()
+        assert loaded is not None
+        assert str(loaded.token_endpoint) == "https://discovered.example.com/token"
+
+    def test_persist_metadata_noop_when_unchanged(self, tmp_path, monkeypatch):
+        """No-op write when disk already matches in-memory metadata."""
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        storage = HermesTokenStorage("noop-srv")
+        meta = _make_metadata("https://same.example.com/token")
+        storage.save_oauth_metadata(meta)
+
+        provider = _manager_provider_with_context(storage, oauth_metadata=meta)
+
+        with patch.object(
+            HermesTokenStorage, "save_oauth_metadata"
+        ) as save_spy:
+            provider._persist_oauth_metadata_if_changed()
+            save_spy.assert_not_called()
+
+    def test_async_auth_flow_persists_on_completion(self, tmp_path, monkeypatch):
+        """End-to-end: running the wrapped auth_flow persists discovered metadata."""
+        monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+        storage = HermesTokenStorage("flow-srv")
+        provider = _manager_provider_with_context(
+            storage,
+            oauth_metadata=_make_metadata("https://flow.example.com/token"),
+            server_name="flow-srv",
+        )
+
+        async def fake_parent_flow(self, request):
+            if False:
+                yield  # pragma: no cover -- make this an async generator
+            return
+
+        manager = MagicMock()
+        manager.invalidate_if_disk_changed = AsyncMock(return_value=False)
+
+        with patch.object(
+            _HERMES_PROVIDER_CLS.__bases__[0],
+            "async_auth_flow",
+            new=fake_parent_flow,
+        ), patch("tools.mcp_oauth_manager.get_manager", return_value=manager):
+            async def drive():
+                gen = provider.async_auth_flow(MagicMock())
+                async for _ in gen:
+                    pass
+
+            asyncio.run(drive())
+
+        loaded = storage.load_oauth_metadata()
+        assert loaded is not None
+        assert str(loaded.token_endpoint) == "https://flow.example.com/token"

tests/tools/test_mcp_sse_transport.py

@@ -0,0 +1,209 @@
+"""Regression tests for SSE transport in ``MCPServerTask._run_http``.
+
+Covers fixes distilled from @amiller's PR #5981 that couldn't be cherry-picked
+due to stale-branch divergence:
+
+1. ``sse_read_timeout`` is set to 300s (not the tool timeout). SSE servers
+   commonly hold the stream idle for minutes between events; a 60s read
+   timeout drops the connection after the first slow stretch. Original
+   observation: Router Teamwork / Supermemory on Cloudflare Workers dropping
+   at ~60s idle.
+
+2. OAuth auth is forwarded to ``sse_client`` when configured. Previously the
+   code built ``_oauth_auth`` but never passed it to the SSE path, so SSE MCP
+   servers behind OAuth 2.1 PKCE would silently fail with 401s.
+"""
+
+from __future__ import annotations
+
+import asyncio
+from unittest.mock import AsyncMock, MagicMock, patch
+
+import pytest
+
+
+async def _noop_initialize():
+    return None
+
+
+def _build_server_with_sse(oauth: bool = False):
+    """Stand up an MCPServerTask configured for SSE transport, with mocks
+    threaded through so ``_run_http`` can enter the SSE branch without a
+    real network call."""
+    from tools.mcp_tool import MCPServerTask
+
+    server = MCPServerTask("sse-test")
+    server._auth_type = "oauth" if oauth else ""
+    server._sampling = None
+    return server
+
+
+@pytest.fixture
+def patch_sse_client():
+    """Replace ``sse_client`` with a MagicMock that records its kwargs.
+
+    Returns the mock so tests can assert how ``_run_http`` called it.
+    """
+    captured_kwargs: dict = {}
+
+    class _FakeStream:
+        def __init__(self):
+            self._read = AsyncMock()
+            self._write = AsyncMock()
+
+        async def __aenter__(self):
+            return (self._read, self._write)
+
+        async def __aexit__(self, *a):
+            return False
+
+    def fake_sse_client(**kwargs):
+        captured_kwargs.clear()
+        captured_kwargs.update(kwargs)
+        return _FakeStream()
+
+    class _FakeSession:
+        def __init__(self, *args, **kwargs):
+            pass
+
+        async def __aenter__(self):
+            mock_session = MagicMock()
+            mock_session.initialize = AsyncMock()
+            return mock_session
+
+        async def __aexit__(self, *a):
+            return False
+
+    with patch("tools.mcp_tool.sse_client", new=fake_sse_client), \
+         patch("tools.mcp_tool.ClientSession", new=_FakeSession):
+        yield captured_kwargs
+
+
+class TestSSEReadTimeout:
+    def test_sse_read_timeout_is_300s_not_tool_timeout(self, patch_sse_client):
+        """``sse_read_timeout`` must be 300s regardless of the configured
+        ``timeout``. Using the tool timeout (60s default) causes Cloudflare-
+        Workers-style SSE MCP servers to drop the connection at ~60s idle."""
+        from tools.mcp_tool import MCPServerTask
+
+        server = _build_server_with_sse()
+
+        async def drive():
+            with patch.object(MCPServerTask, "_wait_for_lifecycle_event",
+                              new=AsyncMock(return_value="shutdown")), \
+                 patch.object(MCPServerTask, "_discover_tools", new=AsyncMock()):
+                try:
+                    await asyncio.wait_for(
+                        server._run_http({
+                            "url": "https://example.com/mcp/sse",
+                            "transport": "sse",
+                            "timeout": 60,
+                        }),
+                        timeout=2.0,
+                    )
+                except (asyncio.TimeoutError, StopAsyncIteration, Exception):
+                    pass
+
+        asyncio.run(drive())
+
+        assert patch_sse_client.get("sse_read_timeout") == 300.0, (
+            f"sse_read_timeout = {patch_sse_client.get('sse_read_timeout')} "
+            f"(expected 300.0) — SSE idle disconnect regression"
+        )
+
+    def test_sse_read_timeout_still_300s_when_tool_timeout_is_large(self, patch_sse_client):
+        """Even if user sets a large ``timeout``, ``sse_read_timeout`` stays
+        decoupled — it's a transport-level budget for inter-event silence,
+        not a per-call budget."""
+        from tools.mcp_tool import MCPServerTask
+
+        server = _build_server_with_sse()
+
+        async def drive():
+            with patch.object(MCPServerTask, "_wait_for_lifecycle_event",
+                              new=AsyncMock(return_value="shutdown")), \
+                 patch.object(MCPServerTask, "_discover_tools", new=AsyncMock()):
+                try:
+                    await asyncio.wait_for(
+                        server._run_http({
+                            "url": "https://example.com/mcp/sse",
+                            "transport": "sse",
+                            "timeout": 600,
+                        }),
+                        timeout=2.0,
+                    )
+                except (asyncio.TimeoutError, StopAsyncIteration, Exception):
+                    pass
+
+        asyncio.run(drive())
+
+        assert patch_sse_client.get("sse_read_timeout") == 300.0
+
+
+class TestSSEOAuthForwarding:
+    def test_sse_client_receives_oauth_auth_when_configured(self, patch_sse_client):
+        """If ``_auth_type == 'oauth'``, ``sse_client`` must receive the
+        constructed OAuth provider via ``auth=``. Previously the provider
+        was built but never forwarded to the SSE path."""
+        from tools.mcp_tool import MCPServerTask
+
+        server = _build_server_with_sse(oauth=True)
+        fake_oauth_provider = MagicMock(name="fake_oauth_provider")
+        fake_manager = MagicMock()
+        fake_manager.get_or_build_provider.return_value = fake_oauth_provider
+
+        async def drive():
+            with patch.object(MCPServerTask, "_wait_for_lifecycle_event",
+                              new=AsyncMock(return_value="shutdown")), \
+                 patch.object(MCPServerTask, "_discover_tools", new=AsyncMock()), \
+                 patch("tools.mcp_oauth_manager.get_manager", return_value=fake_manager):
+                try:
+                    await asyncio.wait_for(
+                        server._run_http({
+                            "url": "https://example.com/mcp/sse",
+                            "transport": "sse",
+                            "auth": "oauth",
+                            "timeout": 60,
+                        }),
+                        timeout=2.0,
+                    )
+                except (asyncio.TimeoutError, StopAsyncIteration, Exception):
+                    pass
+
+        asyncio.run(drive())
+
+        assert "auth" in patch_sse_client, (
+            "sse_client was NOT called with auth= — SSE OAuth forwarding regressed"
+        )
+        assert patch_sse_client["auth"] is fake_oauth_provider
+
+    def test_sse_client_omits_auth_when_no_oauth_configured(self, patch_sse_client):
+        """Without OAuth, ``sse_client`` should not receive an ``auth=`` kwarg.
+        Passing ``None`` would be equally fine but the current code path only
+        sets it when configured — lock that in."""
+        from tools.mcp_tool import MCPServerTask
+
+        server = _build_server_with_sse(oauth=False)
+
+        async def drive():
+            with patch.object(MCPServerTask, "_wait_for_lifecycle_event",
+                              new=AsyncMock(return_value="shutdown")), \
+                 patch.object(MCPServerTask, "_discover_tools", new=AsyncMock()):
+                try:
+                    await asyncio.wait_for(
+                        server._run_http({
+                            "url": "https://example.com/mcp/sse",
+                            "transport": "sse",
+                            "timeout": 60,
+                        }),
+                        timeout=2.0,
+                    )
+                except (asyncio.TimeoutError, StopAsyncIteration, Exception):
+                    pass
+
+        asyncio.run(drive())
+
+        assert "auth" not in patch_sse_client, (
+            f"sse_client was called with auth= when no OAuth was configured: "
+            f"{patch_sse_client!r}"
+        )

tests/tools/test_mcp_tool.py

@@ -547,6 +547,43 @@ class TestRunOnMCPLoopInterrupts:
             mcp_mod._mcp_loop = old_loop
             mcp_mod._mcp_thread = old_thread
 
+    def test_timeout_reports_elapsed_and_configured_timeout(self):
+        import tools.mcp_tool as mcp_mod
+
+        loop = asyncio.new_event_loop()
+        thread = threading.Thread(target=loop.run_forever, daemon=True)
+        thread.start()
+
+        cancelled = threading.Event()
+
+        async def _slow_call():
+            try:
+                await asyncio.sleep(5)
+                return "done"
+            except asyncio.CancelledError:
+                cancelled.set()
+                raise
+
+        old_loop = mcp_mod._mcp_loop
+        old_thread = mcp_mod._mcp_thread
+        mcp_mod._mcp_loop = loop
+        mcp_mod._mcp_thread = thread
+
+        try:
+            with pytest.raises(TimeoutError, match=r"MCP call timed out after .*configured timeout: 0.2s"):
+                mcp_mod._run_on_mcp_loop(_slow_call(), timeout=0.2)
+
+            deadline = time.time() + 2
+            while time.time() < deadline and not cancelled.is_set():
+                time.sleep(0.05)
+            assert cancelled.is_set()
+        finally:
+            loop.call_soon_threadsafe(loop.stop)
+            thread.join(timeout=2)
+            loop.close()
+            mcp_mod._mcp_loop = old_loop
+            mcp_mod._mcp_thread = old_thread
+
 
 # ---------------------------------------------------------------------------
 # Tool registration (discovery + register)

tests/tools/test_mcp_tool_session_expired.py

@@ -53,6 +53,17 @@ def test_is_session_expired_detects_session_terminated():
     assert _is_session_expired_error(RuntimeError("Session terminated")) is True
 
 
+def test_is_session_expired_detects_stale_pipe_and_closed_transport_variants():
+    """Stdio/AnyIO stale-pipe failures usually surface as closed-resource
+    or broken-pipe text, not an HTTP session-expired JSON-RPC error."""
+    from tools.mcp_tool import _is_session_expired_error
+    assert _is_session_expired_error(RuntimeError("ClosedResourceError")) is True
+    assert _is_session_expired_error(RuntimeError("closed resource in MCP child")) is True
+    assert _is_session_expired_error(RuntimeError("transport is closed")) is True
+    assert _is_session_expired_error(RuntimeError("Broken pipe while writing request")) is True
+    assert _is_session_expired_error(RuntimeError("End of file from MCP server")) is True
+
+
 def test_is_session_expired_is_case_insensitive():
     """Match uses lower-cased comparison so servers that emit the
     message in different cases (SDK formatter quirks) still trigger."""

tests/tools/test_mcp_utility_capability_gating.py

@@ -0,0 +1,175 @@
+"""Regression tests for capability-gated MCP utility schema registration.
+
+Background
+==========
+For every connected MCP server, hermes-agent used to register four "utility"
+tool schemas (``mcp_<server>_list_resources``, ``read_resource``,
+``list_prompts``, ``get_prompt``) regardless of whether the server actually
+advertises those capabilities. The old gate used ``hasattr(server.session,
+method)`` which always returned True because ``mcp.ClientSession`` defines
+all four methods on the class — independent of what the remote server
+supports.
+
+Tools-only servers like ``@upstash/context7-mcp`` advertise
+``{\"tools\": {\"listChanged\": true}}`` in their ``initialize`` response —
+no ``prompts`` or ``resources`` keys — and they return JSON-RPC
+``-32601 Method not found`` for ``prompts/list``, ``prompts/get``,
+``resources/list``, ``resources/read``. The model would try the stubs,
+get the error, and incorrectly conclude the MCP server was broken.
+
+The fix captures the ``InitializeResult`` from
+``await session.initialize()`` into ``MCPServerTask.initialize_result``
+and gates utility schema registration on the advertised
+``capabilities.resources`` / ``capabilities.prompts`` sub-objects. See
+#18051 for the reporter's repro (Context7) and analysis.
+"""
+
+from __future__ import annotations
+
+from types import SimpleNamespace
+from unittest.mock import MagicMock
+
+import pytest
+
+
+def _make_init_result(*, resources: bool, prompts: bool):
+    """Build a fake ``InitializeResult`` whose ``capabilities`` sub-object
+    matches a server that advertises exactly the given capability set.
+
+    MCP spec shape: ``capabilities.resources`` / ``capabilities.prompts``
+    are non-None iff the server implements the corresponding request
+    family. We mirror that with ``SimpleNamespace`` because the real SDK
+    models are pydantic and we don't want the test to couple to pydantic
+    versioning.
+    """
+    caps_attrs: dict = {"tools": SimpleNamespace(listChanged=True)}
+    caps_attrs["resources"] = SimpleNamespace(listChanged=True) if resources else None
+    caps_attrs["prompts"] = SimpleNamespace(listChanged=True) if prompts else None
+    return SimpleNamespace(capabilities=SimpleNamespace(**caps_attrs))
+
+
+def _make_fake_server(*, initialize_result):
+    """Build a stand-in ``MCPServerTask`` that exposes just the fields
+    ``_select_utility_schemas`` inspects: ``name``, ``session``,
+    ``initialize_result``.
+
+    A plain ``MCPServerTask`` uses ``__slots__`` and needs an asyncio
+    loop for the ``Event``/``Lock`` init — overkill for unit scope.
+    """
+    server = MagicMock()
+    server.name = "test-server"
+    # session must satisfy the legacy ``hasattr`` fallback too
+    server.session = MagicMock(
+        spec=["list_resources", "read_resource", "list_prompts", "get_prompt"]
+    )
+    server.initialize_result = initialize_result
+    return server
+
+
+def _handler_keys(selected):
+    return {entry["handler_key"] for entry in selected}
+
+
+class TestCapabilityGatedRegistration:
+    def test_tools_only_server_gets_no_utility_schemas(self):
+        """Context7-shaped server (tools only, no prompts / resources) should
+        get zero utility stubs registered — this is the exact scenario
+        from the #18051 bug report."""
+        from tools.mcp_tool import _select_utility_schemas
+
+        server = _make_fake_server(
+            initialize_result=_make_init_result(resources=False, prompts=False)
+        )
+        selected = _select_utility_schemas("context7", server, {})
+        assert _handler_keys(selected) == set(), (
+            f"tools-only server should have zero utility stubs, got "
+            f"{_handler_keys(selected)}"
+        )
+
+    def test_resources_only_server_gets_resource_stubs_only(self):
+        from tools.mcp_tool import _select_utility_schemas
+
+        server = _make_fake_server(
+            initialize_result=_make_init_result(resources=True, prompts=False)
+        )
+        selected = _select_utility_schemas("res-only", server, {})
+        assert _handler_keys(selected) == {"list_resources", "read_resource"}
+
+    def test_prompts_only_server_gets_prompt_stubs_only(self):
+        from tools.mcp_tool import _select_utility_schemas
+
+        server = _make_fake_server(
+            initialize_result=_make_init_result(resources=False, prompts=True)
+        )
+        selected = _select_utility_schemas("prompt-only", server, {})
+        assert _handler_keys(selected) == {"list_prompts", "get_prompt"}
+
+    def test_fully_capable_server_gets_all_four_stubs(self):
+        from tools.mcp_tool import _select_utility_schemas
+
+        server = _make_fake_server(
+            initialize_result=_make_init_result(resources=True, prompts=True)
+        )
+        selected = _select_utility_schemas("full", server, {})
+        assert _handler_keys(selected) == {
+            "list_resources", "read_resource", "list_prompts", "get_prompt",
+        }
+
+
+class TestConfigFilterStillApplies:
+    """Per-server config flags ``tools.resources: false`` / ``tools.prompts: false``
+    must continue to override even when the server DOES advertise the capability."""
+
+    def test_config_disables_resources_even_when_advertised(self):
+        from tools.mcp_tool import _select_utility_schemas
+
+        server = _make_fake_server(
+            initialize_result=_make_init_result(resources=True, prompts=True)
+        )
+        selected = _select_utility_schemas(
+            "full-but-filtered",
+            server,
+            {"tools": {"resources": False}},
+        )
+        assert _handler_keys(selected) == {"list_prompts", "get_prompt"}
+
+    def test_config_disables_prompts_even_when_advertised(self):
+        from tools.mcp_tool import _select_utility_schemas
+
+        server = _make_fake_server(
+            initialize_result=_make_init_result(resources=True, prompts=True)
+        )
+        selected = _select_utility_schemas(
+            "full-but-filtered",
+            server,
+            {"tools": {"prompts": False}},
+        )
+        assert _handler_keys(selected) == {"list_resources", "read_resource"}
+
+
+class TestLegacyFallback:
+    """When ``initialize_result`` is missing (older test fixtures or code
+    paths that haven't captured it yet), fall back to the legacy hasattr
+    check so pre-existing tests and servers keep working."""
+
+    def test_no_initialize_result_falls_back_to_hasattr_check(self):
+        from tools.mcp_tool import _select_utility_schemas
+
+        server = _make_fake_server(initialize_result=None)
+        # With the legacy fallback, session.spec includes all four methods,
+        # so all four stubs should register (old behavior).
+        selected = _select_utility_schemas("legacy", server, {})
+        assert _handler_keys(selected) == {
+            "list_resources", "read_resource", "list_prompts", "get_prompt",
+        }
+
+    def test_no_initialize_result_respects_session_spec(self):
+        """Legacy fallback still filters by ``hasattr(session, method)``, so
+        a session whose spec lacks a method is correctly skipped."""
+        from tools.mcp_tool import _select_utility_schemas
+
+        server = _make_fake_server(initialize_result=None)
+        # Override session to a spec that only has list_resources
+        server.session = MagicMock(spec=["list_resources"])
+        selected = _select_utility_schemas("legacy-partial", server, {})
+        assert _handler_keys(selected) == {"list_resources"}

tests/tools/test_memory_tool_schema.py

@@ -0,0 +1,49 @@
+"""Schema-shape tests for the built-in memory tool.
+
+The memory tool previously used ``allOf: [{if: ..., then: {required: ...}}]``
+at the top level of ``parameters`` to hint per-action required fields.  That
+form was:
+
+  1. Ignored by every provider (Chat Completions doesn't honour ``if/then``
+     on function schemas), so it never actually enforced anything.
+  2. **Rejected outright by strict backends** — OpenAI's Codex endpoint
+     (``chatgpt.com/backend-api/codex``, gpt-5.x) returns
+     ``Invalid schema for function 'memory': schema must have type 'object'
+     and not have 'oneOf'/'anyOf'/'allOf'/'enum'/'not' at the top level``.
+
+We now rely on the runtime handler (``memory_tool()`` in ``tools/memory_tool.py``)
+to validate required fields per action and return actionable error messages.
+These tests guard the schema against regressing back to a shape strict
+backends reject.
+"""
+
+import json
+
+from tools.memory_tool import MEMORY_SCHEMA
+
+
+_FORBIDDEN_TOP_LEVEL_KEYS = ("allOf", "anyOf", "oneOf", "enum", "not")
+
+
+def test_memory_schema_has_no_forbidden_top_level_combinators():
+    """OpenAI's Codex backend rejects these at the top level of parameters."""
+    params = MEMORY_SCHEMA["parameters"]
+    for key in _FORBIDDEN_TOP_LEVEL_KEYS:
+        assert key not in params, (
+            f"top-level {key!r} in memory tool parameters will break the "
+            "Codex backend (chatgpt.com/backend-api/codex). Per-action "
+            "required-field checks belong in the runtime handler, not the schema."
+        )
+
+
+def test_memory_schema_is_well_formed():
+    params = MEMORY_SCHEMA["parameters"]
+    assert params["type"] == "object"
+    assert params["required"] == ["action", "target"]
+    # Nested ``enum`` on property values is fine — only top-level is forbidden.
+    assert params["properties"]["action"]["enum"] == ["add", "replace", "remove"]
+    assert params["properties"]["target"]["enum"] == ["memory", "user"]
+
+
+def test_memory_schema_is_json_serializable():
+    json.dumps(MEMORY_SCHEMA)

tests/tools/test_schema_sanitizer.py

@@ -302,3 +302,61 @@ def test_strip_none_returns_zero():
     tools, stripped = strip_pattern_and_format(None)
     assert tools is None
     assert stripped == 0
+
+
+def test_top_level_allof_stripped_for_codex_backend_compat():
+    """OpenAI Codex backend rejects top-level allOf/oneOf/anyOf/enum/not."""
+    tools = [_tool("memory", {
+        "type": "object",
+        "properties": {
+            "action": {"type": "string", "enum": ["add", "replace"]},
+            "content": {"type": "string"},
+        },
+        "required": ["action"],
+        "allOf": [
+            {
+                "if": {"properties": {"action": {"const": "add"}}, "required": ["action"]},
+                "then": {"required": ["content"]},
+            },
+        ],
+    })]
+    out = sanitize_tool_schemas(tools)
+    params = out[0]["function"]["parameters"]
+    assert "allOf" not in params
+    # Properties and required survive.
+    assert params["required"] == ["action"]
+    assert "content" in params["properties"]
+
+
+def test_top_level_oneof_anyof_enum_not_stripped():
+    """All five forbidden top-level combinators are dropped."""
+    tools = [_tool("t", {
+        "type": "object",
+        "properties": {"x": {"type": "string"}},
+        "oneOf": [{"required": ["x"]}],
+        "anyOf": [{"required": ["x"]}],
+        "enum": ["bogus-top-level"],
+        "not": {"required": ["y"]},
+    })]
+    out = sanitize_tool_schemas(tools)
+    params = out[0]["function"]["parameters"]
+    for key in ("oneOf", "anyOf", "enum", "not"):
+        assert key not in params, f"{key} should be stripped from top level"
+
+
+def test_nested_allof_preserved():
+    """Combinators inside a property's schema are preserved (only top is strict)."""
+    tools = [_tool("t", {
+        "type": "object",
+        "properties": {
+            "config": {
+                "type": "object",
+                "properties": {"mode": {"type": "string"}},
+                "allOf": [{"required": ["mode"]}],
+            },
+        },
+    })]
+    out = sanitize_tool_schemas(tools)
+    nested = out[0]["function"]["parameters"]["properties"]["config"]
+    assert "allOf" in nested
+    assert nested["allOf"] == [{"required": ["mode"]}]

tests/tools/test_send_message_tool.py

@@ -140,6 +140,7 @@ class TestSendMessageTool:
             "hello",
             thread_id="17585",
             media_files=[],
+            force_document=False,
         )
 
     def test_display_label_target_resolves_via_channel_directory(self, tmp_path):
@@ -178,6 +179,7 @@ class TestSendMessageTool:
             "hello",
             thread_id="17585",
             media_files=[],
+            force_document=False,
         )
 
     def test_mirror_receives_current_session_user_id(self):
@@ -483,7 +485,7 @@ class TestSendToPlatformChunking:
 
         sent_calls = []
 
-        async def fake_send(token, chat_id, message, media_files=None, thread_id=None, disable_link_previews=False):
+        async def fake_send(token, chat_id, message, media_files=None, thread_id=None, disable_link_previews=False, force_document=False):
             sent_calls.append(media_files or [])
             return {"success": True, "platform": "telegram", "chat_id": chat_id, "message_id": str(len(sent_calls))}
 

tests/tools/test_skill_usage.py

@@ -1,12 +1,21 @@
 """Tests for tools/skill_usage.py — sidecar telemetry + provenance filtering."""
 
 import json
+import multiprocessing as mp
 import os
 from pathlib import Path
 
 import pytest
 
 
+def _bump_view_many(hermes_home: str, skill_name: str, iterations: int) -> None:
+    os.environ["HERMES_HOME"] = hermes_home
+    from tools.skill_usage import bump_view
+
+    for _ in range(iterations):
+        bump_view(skill_name)
+
+
 @pytest.fixture
 def skills_home(tmp_path, monkeypatch):
     """Isolated HERMES_HOME with a clean skills/ dir for each test."""
@@ -139,6 +148,30 @@ def test_bumps_do_not_corrupt_other_skills(skills_home):
     assert get_record("skill-b")["use_count"] == 1
 
 
+def test_concurrent_bump_view_preserves_all_updates(skills_home):
+    from tools.skill_usage import get_record
+
+    process_count = 6
+    iterations = 25
+    ctx = mp.get_context("spawn")
+    processes = [
+        ctx.Process(
+            target=_bump_view_many,
+            args=(str(skills_home), "shared-skill", iterations),
+        )
+        for _ in range(process_count)
+    ]
+
+    for process in processes:
+        process.start()
+    for process in processes:
+        process.join(timeout=20)
+
+    for process in processes:
+        assert process.exitcode == 0
+    assert get_record("shared-skill")["view_count"] == process_count * iterations
+
+
 # ---------------------------------------------------------------------------
 # State transitions
 # ---------------------------------------------------------------------------

tests/tools/test_url_safety.py

@@ -5,6 +5,7 @@ from unittest.mock import patch
 
 from tools.url_safety import (
     is_safe_url,
+    is_always_blocked_url,
     _is_blocked_ip,
     _global_allow_private_urls,
     _reset_allow_private_cache,
@@ -407,3 +408,69 @@ class TestAllowPrivateUrlsIntegration:
         """Empty URLs are still blocked."""
         monkeypatch.setenv("HERMES_ALLOW_PRIVATE_URLS", "true")
         assert is_safe_url("") is False
+
+
+class TestIsAlwaysBlockedUrl:
+    """The always-blocked floor — cloud metadata only, narrower than is_safe_url."""
+
+    # -- The sentinel set that must always block --------------------------------
+
+    @pytest.mark.parametrize("url", [
+        "http://169.254.169.254/latest/meta-data/",            # AWS / GCP / Azure / DO / Oracle
+        "http://169.254.169.253/metadata/instance",              # Azure IMDS wire server
+        "http://169.254.170.2/v2/credentials",                   # AWS ECS task metadata
+        "http://100.100.100.200/latest/meta-data/",              # Alibaba Cloud
+        "http://169.254.42.1/",                                  # Any /16 link-local
+    ])
+    def test_literal_imds_ips_always_blocked(self, url):
+        """Literal IMDS IPs and the /16 link-local range always block."""
+        assert is_always_blocked_url(url) is True
+
+    def test_gcp_metadata_hostname_always_blocked_even_without_dns(self):
+        """metadata.google.internal blocks by hostname, no DNS needed."""
+        with patch("socket.getaddrinfo", side_effect=socket.gaierror("nope")):
+            assert is_always_blocked_url("http://metadata.google.internal/") is True
+
+    def test_hostname_resolving_to_imds_always_blocked(self):
+        """Attacker-controlled hostname resolving to IMDS still blocks."""
+        with patch("socket.getaddrinfo", return_value=[
+            (2, 1, 6, "", ("169.254.169.254", 0)),
+        ]):
+            assert is_always_blocked_url("http://attacker-controlled.example.com/") is True
+
+    # -- Things the floor must NOT block ----------------------------------------
+
+    def test_public_url_not_blocked(self):
+        assert is_always_blocked_url("https://example.com/path") is False
+
+    @pytest.mark.parametrize("url", [
+        "http://127.0.0.1:8080/",
+        "http://192.168.1.1/",
+        "http://10.0.0.5/",
+        "http://172.16.0.1/",
+        "http://100.64.0.1/",  # CGNAT — blocked by is_safe_url but not by the floor
+    ])
+    def test_ordinary_private_urls_not_in_floor(self, url):
+        """Floor is narrower than is_safe_url — ordinary private URLs pass."""
+        assert is_always_blocked_url(url) is False
+
+    def test_dns_failure_not_in_floor(self):
+        """DNS failure on a non-sentinel hostname = not always-blocked.
+
+        Caller's ordinary fail-closed path (is_safe_url) handles that case.
+        """
+        with patch("socket.getaddrinfo", side_effect=socket.gaierror("fail")):
+            assert is_always_blocked_url("http://nonexistent.example.com/") is False
+
+    def test_empty_url_not_in_floor(self):
+        """Empty URL falls through — caller decides what to do with a malformed URL."""
+        assert is_always_blocked_url("") is False
+
+    def test_malformed_url_not_in_floor(self):
+        """Parse errors don't claim always-blocked status."""
+        assert is_always_blocked_url("not a url at all") is False
+
+    def test_floor_ignores_allow_private_urls_toggle(self, monkeypatch):
+        """security.allow_private_urls can NOT unblock cloud metadata."""
+        monkeypatch.setenv("HERMES_ALLOW_PRIVATE_URLS", "true")
+        assert is_always_blocked_url("http://169.254.169.254/") is True

tests/tools/test_web_providers_brave_free.py

@@ -0,0 +1,275 @@
+"""Tests for the Brave Search (free tier) web search provider.
+
+Covers:
+- BraveFreeSearchProvider.is_configured() env var gating
+- BraveFreeSearchProvider.search() — happy path, HTTP error, request error, bad JSON
+- Result normalization (title, url, description, position)
+- Limit truncation + Brave's count cap (20)
+- _is_backend_available("brave-free") integration
+- _get_backend() recognizes "brave-free" as a valid configured backend
+- check_web_api_key() includes brave-free in availability check
+- web_extract / web_crawl return search-only errors when brave-free is active
+"""
+from __future__ import annotations
+
+import json
+from unittest.mock import MagicMock, patch
+
+
+# ---------------------------------------------------------------------------
+# BraveFreeSearchProvider unit tests
+# ---------------------------------------------------------------------------
+
+
+class TestBraveFreeProviderIsConfigured:
+    def test_configured_when_key_set(self, monkeypatch):
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+        assert BraveFreeSearchProvider().is_configured() is True
+
+    def test_not_configured_when_key_missing(self, monkeypatch):
+        monkeypatch.delenv("BRAVE_SEARCH_API_KEY", raising=False)
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+        assert BraveFreeSearchProvider().is_configured() is False
+
+    def test_not_configured_when_key_whitespace(self, monkeypatch):
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "   ")
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+        assert BraveFreeSearchProvider().is_configured() is False
+
+    def test_provider_name(self):
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+        assert BraveFreeSearchProvider().provider_name() == "brave-free"
+
+    def test_implements_web_search_provider(self):
+        from tools.web_providers.base import WebSearchProvider
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+        assert issubclass(BraveFreeSearchProvider, WebSearchProvider)
+
+
+class TestBraveFreeProviderSearch:
+    _SAMPLE_RESPONSE = {
+        "web": {
+            "results": [
+                {"title": "A", "url": "https://a.example.com", "description": "desc A"},
+                {"title": "B", "url": "https://b.example.com", "description": "desc B"},
+                {"title": "C", "url": "https://c.example.com", "description": "desc C"},
+            ]
+        }
+    }
+
+    @staticmethod
+    def _mock_resp(json_data, status_code=200):
+        m = MagicMock()
+        m.status_code = status_code
+        m.json.return_value = json_data
+        m.raise_for_status = MagicMock()
+        return m
+
+    def test_happy_path_normalizes_results(self, monkeypatch):
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+
+        with patch("httpx.get", return_value=self._mock_resp(self._SAMPLE_RESPONSE)):
+            result = BraveFreeSearchProvider().search("test query", limit=5)
+
+        assert result["success"] is True
+        web = result["data"]["web"]
+        assert len(web) == 3
+        assert web[0] == {"title": "A", "url": "https://a.example.com", "description": "desc A", "position": 1}
+        assert web[2]["position"] == 3
+
+    def test_sends_subscription_token_header_and_count(self, monkeypatch):
+        """Brave uses X-Subscription-Token; count maps from limit."""
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+
+        captured = {}
+
+        def fake_get(url, **kwargs):
+            captured["url"] = url
+            captured["headers"] = kwargs.get("headers", {})
+            captured["params"] = kwargs.get("params", {})
+            return self._mock_resp({"web": {"results": []}})
+
+        with patch("httpx.get", side_effect=fake_get):
+            BraveFreeSearchProvider().search("q", limit=5)
+
+        assert captured["url"] == "https://api.search.brave.com/res/v1/web/search"
+        assert captured["headers"].get("X-Subscription-Token") == "BSAkey123"
+        assert captured["params"].get("q") == "q"
+        assert captured["params"].get("count") == 5
+
+    def test_count_is_capped_at_20(self, monkeypatch):
+        """Brave caps count at 20 — limit above that clamps."""
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+
+        captured = {}
+
+        def fake_get(url, **kwargs):
+            captured["params"] = kwargs.get("params", {})
+            return self._mock_resp({"web": {"results": []}})
+
+        with patch("httpx.get", side_effect=fake_get):
+            BraveFreeSearchProvider().search("q", limit=100)
+
+        assert captured["params"].get("count") == 20
+
+    def test_limit_is_respected_client_side(self, monkeypatch):
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+
+        with patch("httpx.get", return_value=self._mock_resp(self._SAMPLE_RESPONSE)):
+            result = BraveFreeSearchProvider().search("q", limit=2)
+
+        assert result["success"] is True
+        assert len(result["data"]["web"]) == 2
+
+    def test_empty_results(self, monkeypatch):
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+
+        with patch("httpx.get", return_value=self._mock_resp({"web": {"results": []}})):
+            result = BraveFreeSearchProvider().search("nothing", limit=5)
+
+        assert result["success"] is True
+        assert result["data"]["web"] == []
+
+    def test_missing_web_key_returns_empty(self, monkeypatch):
+        """Responses without a ``web`` block should produce an empty result set, not crash."""
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+
+        with patch("httpx.get", return_value=self._mock_resp({})):
+            result = BraveFreeSearchProvider().search("q", limit=5)
+
+        assert result["success"] is True
+        assert result["data"]["web"] == []
+
+    def test_http_error_returns_failure(self, monkeypatch):
+        import httpx
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+
+        bad = MagicMock()
+        bad.status_code = 429
+        err = httpx.HTTPStatusError("429", request=MagicMock(), response=bad)
+
+        with patch("httpx.get", side_effect=err):
+            result = BraveFreeSearchProvider().search("q", limit=5)
+
+        assert result["success"] is False
+        assert "429" in result["error"]
+
+    def test_request_error_returns_failure(self, monkeypatch):
+        import httpx
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+
+        with patch("httpx.get", side_effect=httpx.RequestError("boom")):
+            result = BraveFreeSearchProvider().search("q", limit=5)
+
+        assert result["success"] is False
+        assert "boom" in result["error"] or "Brave" in result["error"]
+
+    def test_missing_key_returns_failure(self, monkeypatch):
+        monkeypatch.delenv("BRAVE_SEARCH_API_KEY", raising=False)
+        from tools.web_providers.brave_free import BraveFreeSearchProvider
+
+        result = BraveFreeSearchProvider().search("q", limit=5)
+        assert result["success"] is False
+        assert "BRAVE_SEARCH_API_KEY" in result["error"]
+
+
+# ---------------------------------------------------------------------------
+# Integration: _is_backend_available / _get_backend / check_web_api_key
+# ---------------------------------------------------------------------------
+
+
+class TestBraveFreeBackendWiring:
+    def test_is_backend_available_true_when_key_set(self, monkeypatch):
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        from tools.web_tools import _is_backend_available
+        assert _is_backend_available("brave-free") is True
+
+    def test_is_backend_available_false_when_key_missing(self, monkeypatch):
+        monkeypatch.delenv("BRAVE_SEARCH_API_KEY", raising=False)
+        from tools.web_tools import _is_backend_available
+        assert _is_backend_available("brave-free") is False
+
+    def test_configured_backend_accepted(self, monkeypatch):
+        from tools import web_tools
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {"backend": "brave-free"})
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        assert web_tools._get_backend() == "brave-free"
+
+    def test_auto_detect_picks_brave_free_when_only_key_set(self, monkeypatch):
+        from tools import web_tools
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {})
+        for key in ("FIRECRAWL_API_KEY", "FIRECRAWL_API_URL", "PARALLEL_API_KEY",
+                    "TAVILY_API_KEY", "EXA_API_KEY", "SEARXNG_URL"):
+            monkeypatch.delenv(key, raising=False)
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        monkeypatch.setattr(web_tools, "_is_tool_gateway_ready", lambda: False)
+        monkeypatch.setattr(web_tools, "_ddgs_package_importable", lambda: False)
+        assert web_tools._get_backend() == "brave-free"
+
+    def test_brave_free_does_not_override_paid_provider(self, monkeypatch):
+        """Tavily (higher priority) should win in auto-detect."""
+        from tools import web_tools
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {})
+        for key in ("FIRECRAWL_API_KEY", "FIRECRAWL_API_URL", "PARALLEL_API_KEY", "EXA_API_KEY", "SEARXNG_URL"):
+            monkeypatch.delenv(key, raising=False)
+        monkeypatch.setenv("TAVILY_API_KEY", "tvly")
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        monkeypatch.setattr(web_tools, "_is_tool_gateway_ready", lambda: False)
+        assert web_tools._get_backend() == "tavily"
+
+    def test_check_web_api_key_true_when_brave_free_configured(self, monkeypatch):
+        from tools import web_tools
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {"backend": "brave-free"})
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        assert web_tools.check_web_api_key() is True
+
+
+# ---------------------------------------------------------------------------
+# brave-free is search-only: web_extract / web_crawl return clear errors
+# ---------------------------------------------------------------------------
+
+
+class TestBraveFreeSearchOnlyErrors:
+    def test_web_extract_returns_search_only_error(self, monkeypatch):
+        import asyncio
+        from tools import web_tools
+
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {"backend": "brave-free"})
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        monkeypatch.setattr(web_tools, "_is_tool_gateway_ready", lambda: False)
+        monkeypatch.setattr("tools.interrupt.is_interrupted", lambda: False, raising=False)
+
+        result_str = asyncio.get_event_loop().run_until_complete(
+            web_tools.web_extract_tool(["https://example.com"])
+        )
+        result = json.loads(result_str)
+        assert result["success"] is False
+        assert "search-only" in result["error"].lower()
+        assert "brave" in result["error"].lower()
+
+    def test_web_crawl_returns_search_only_error(self, monkeypatch):
+        import asyncio
+        from tools import web_tools
+
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {"backend": "brave-free"})
+        monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
+        monkeypatch.setattr(web_tools, "_is_tool_gateway_ready", lambda: False)
+        monkeypatch.setattr(web_tools, "check_firecrawl_api_key", lambda: False)
+        monkeypatch.setattr("tools.interrupt.is_interrupted", lambda: False, raising=False)
+
+        result_str = asyncio.get_event_loop().run_until_complete(
+            web_tools.web_crawl_tool("https://example.com")
+        )
+        result = json.loads(result_str)
+        assert result["success"] is False
+        assert "search-only" in result["error"].lower()
+        assert "brave" in result["error"].lower()

tests/tools/test_web_providers_ddgs.py

@@ -0,0 +1,246 @@
+"""Tests for the DuckDuckGo (ddgs) web search provider.
+
+Covers:
+- DDGSSearchProvider.is_configured() — reflects package importability
+- DDGSSearchProvider.search() — happy path, missing package, runtime error
+- Result normalization (title, url, description, position)
+- _is_backend_available("ddgs") / _get_backend() integration
+- web_extract / web_crawl return search-only errors when ddgs is active
+"""
+from __future__ import annotations
+
+import json
+import sys
+import types
+from unittest.mock import MagicMock
+
+
+def _install_fake_ddgs(monkeypatch, *, text_results=None, text_raises=None):
+    """Install a stub ``ddgs`` module in sys.modules for the duration of a test.
+
+    ``text_results``: iterable of dicts to yield from DDGS().text(...).
+    ``text_raises``: if set, DDGS().text raises this exception instead.
+    """
+    fake = types.ModuleType("ddgs")
+
+    class _FakeDDGS:
+        def __enter__(self):
+            return self
+        def __exit__(self, *_a):
+            return False
+        def text(self, query, max_results=5):
+            if text_raises is not None:
+                raise text_raises
+            for hit in (text_results or []):
+                yield hit
+
+    fake.DDGS = _FakeDDGS
+    monkeypatch.setitem(sys.modules, "ddgs", fake)
+    return fake
+
+
+# ---------------------------------------------------------------------------
+# DDGSSearchProvider unit tests
+# ---------------------------------------------------------------------------
+
+
+class TestDDGSProviderIsConfigured:
+    def test_configured_when_package_importable(self, monkeypatch):
+        _install_fake_ddgs(monkeypatch)
+        # Drop any cached ``tools.web_providers.ddgs`` so is_configured re-imports ddgs fresh
+        monkeypatch.delitem(sys.modules, "tools.web_providers.ddgs", raising=False)
+        from tools.web_providers.ddgs import DDGSSearchProvider
+        assert DDGSSearchProvider().is_configured() is True
+
+    def test_not_configured_when_package_missing(self, monkeypatch):
+        monkeypatch.delitem(sys.modules, "ddgs", raising=False)
+        monkeypatch.delitem(sys.modules, "tools.web_providers.ddgs", raising=False)
+        # Block the import so ``import ddgs`` raises ImportError even if the package is actually installed
+        import builtins
+        orig_import = builtins.__import__
+
+        def blocked_import(name, *args, **kwargs):
+            if name == "ddgs":
+                raise ImportError("blocked for test")
+            return orig_import(name, *args, **kwargs)
+
+        monkeypatch.setattr(builtins, "__import__", blocked_import)
+        from tools.web_providers.ddgs import DDGSSearchProvider
+        assert DDGSSearchProvider().is_configured() is False
+
+    def test_provider_name(self):
+        from tools.web_providers.ddgs import DDGSSearchProvider
+        assert DDGSSearchProvider().provider_name() == "ddgs"
+
+    def test_implements_web_search_provider(self):
+        from tools.web_providers.base import WebSearchProvider
+        from tools.web_providers.ddgs import DDGSSearchProvider
+        assert issubclass(DDGSSearchProvider, WebSearchProvider)
+
+
+class TestDDGSProviderSearch:
+    def test_happy_path_normalizes_results(self, monkeypatch):
+        _install_fake_ddgs(monkeypatch, text_results=[
+            {"title": "A", "href": "https://a.example.com", "body": "desc A"},
+            {"title": "B", "href": "https://b.example.com", "body": "desc B"},
+            {"title": "C", "href": "https://c.example.com", "body": "desc C"},
+        ])
+        from tools.web_providers.ddgs import DDGSSearchProvider
+
+        result = DDGSSearchProvider().search("q", limit=5)
+
+        assert result["success"] is True
+        web = result["data"]["web"]
+        assert len(web) == 3
+        assert web[0] == {"title": "A", "url": "https://a.example.com", "description": "desc A", "position": 1}
+        assert web[2]["position"] == 3
+
+    def test_accepts_url_key_as_fallback_for_href(self, monkeypatch):
+        _install_fake_ddgs(monkeypatch, text_results=[
+            {"title": "A", "url": "https://a.example.com", "body": "desc A"},
+        ])
+        from tools.web_providers.ddgs import DDGSSearchProvider
+
+        result = DDGSSearchProvider().search("q", limit=5)
+
+        assert result["success"] is True
+        assert result["data"]["web"][0]["url"] == "https://a.example.com"
+
+    def test_limit_is_respected(self, monkeypatch):
+        _install_fake_ddgs(monkeypatch, text_results=[
+            {"title": f"R{i}", "href": f"https://r{i}.example.com", "body": ""}
+            for i in range(10)
+        ])
+        from tools.web_providers.ddgs import DDGSSearchProvider
+
+        result = DDGSSearchProvider().search("q", limit=3)
+
+        assert result["success"] is True
+        assert len(result["data"]["web"]) == 3
+
+    def test_missing_package_returns_failure(self, monkeypatch):
+        monkeypatch.delitem(sys.modules, "ddgs", raising=False)
+        monkeypatch.delitem(sys.modules, "tools.web_providers.ddgs", raising=False)
+        import builtins
+        orig_import = builtins.__import__
+
+        def blocked_import(name, *args, **kwargs):
+            if name == "ddgs":
+                raise ImportError("blocked for test")
+            return orig_import(name, *args, **kwargs)
+
+        monkeypatch.setattr(builtins, "__import__", blocked_import)
+        from tools.web_providers.ddgs import DDGSSearchProvider
+
+        result = DDGSSearchProvider().search("q", limit=5)
+        assert result["success"] is False
+        assert "ddgs" in result["error"].lower()
+
+    def test_runtime_error_returns_failure(self, monkeypatch):
+        _install_fake_ddgs(monkeypatch, text_raises=RuntimeError("rate limited 202"))
+        from tools.web_providers.ddgs import DDGSSearchProvider
+
+        result = DDGSSearchProvider().search("q", limit=5)
+        assert result["success"] is False
+        assert "rate limited" in result["error"] or "failed" in result["error"].lower()
+
+    def test_empty_results(self, monkeypatch):
+        _install_fake_ddgs(monkeypatch, text_results=[])
+        from tools.web_providers.ddgs import DDGSSearchProvider
+
+        result = DDGSSearchProvider().search("nothing", limit=5)
+        assert result["success"] is True
+        assert result["data"]["web"] == []
+
+
+# ---------------------------------------------------------------------------
+# Integration: _is_backend_available / _get_backend / check_web_api_key
+# ---------------------------------------------------------------------------
+
+
+class TestDDGSBackendWiring:
+    def test_is_backend_available_true_when_package_importable(self, monkeypatch):
+        from tools import web_tools
+        monkeypatch.setattr(web_tools, "_ddgs_package_importable", lambda: True)
+        assert web_tools._is_backend_available("ddgs") is True
+
+    def test_is_backend_available_false_when_package_missing(self, monkeypatch):
+        from tools import web_tools
+        monkeypatch.setattr(web_tools, "_ddgs_package_importable", lambda: False)
+        assert web_tools._is_backend_available("ddgs") is False
+
+    def test_configured_backend_accepted(self, monkeypatch):
+        from tools import web_tools
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {"backend": "ddgs"})
+        monkeypatch.setattr(web_tools, "_ddgs_package_importable", lambda: True)
+        assert web_tools._get_backend() == "ddgs"
+
+    def test_ddgs_trails_paid_providers_in_auto_detect(self, monkeypatch):
+        """Exa (priority) should win over ddgs in auto-detect."""
+        from tools import web_tools
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {})
+        for key in ("FIRECRAWL_API_KEY", "FIRECRAWL_API_URL", "PARALLEL_API_KEY",
+                    "TAVILY_API_KEY", "SEARXNG_URL", "BRAVE_SEARCH_API_KEY"):
+            monkeypatch.delenv(key, raising=False)
+        monkeypatch.setenv("EXA_API_KEY", "exa-key")
+        monkeypatch.setattr(web_tools, "_is_tool_gateway_ready", lambda: False)
+        monkeypatch.setattr(web_tools, "_ddgs_package_importable", lambda: True)
+        assert web_tools._get_backend() == "exa"
+
+    def test_auto_detect_picks_ddgs_as_last_resort(self, monkeypatch):
+        from tools import web_tools
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {})
+        for key in ("FIRECRAWL_API_KEY", "FIRECRAWL_API_URL", "PARALLEL_API_KEY",
+                    "TAVILY_API_KEY", "EXA_API_KEY", "SEARXNG_URL", "BRAVE_SEARCH_API_KEY"):
+            monkeypatch.delenv(key, raising=False)
+        monkeypatch.setattr(web_tools, "_is_tool_gateway_ready", lambda: False)
+        monkeypatch.setattr(web_tools, "_ddgs_package_importable", lambda: True)
+        assert web_tools._get_backend() == "ddgs"
+
+    def test_check_web_api_key_true_when_ddgs_configured(self, monkeypatch):
+        from tools import web_tools
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {"backend": "ddgs"})
+        monkeypatch.setattr(web_tools, "_ddgs_package_importable", lambda: True)
+        assert web_tools.check_web_api_key() is True
+
+
+# ---------------------------------------------------------------------------
+# ddgs is search-only: web_extract / web_crawl return clear errors
+# ---------------------------------------------------------------------------
+
+
+class TestDDGSSearchOnlyErrors:
+    def test_web_extract_returns_search_only_error(self, monkeypatch):
+        import asyncio
+        from tools import web_tools
+
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {"backend": "ddgs"})
+        monkeypatch.setattr(web_tools, "_ddgs_package_importable", lambda: True)
+        monkeypatch.setattr(web_tools, "_is_tool_gateway_ready", lambda: False)
+        monkeypatch.setattr("tools.interrupt.is_interrupted", lambda: False, raising=False)
+
+        result_str = asyncio.get_event_loop().run_until_complete(
+            web_tools.web_extract_tool(["https://example.com"])
+        )
+        result = json.loads(result_str)
+        assert result["success"] is False
+        assert "search-only" in result["error"].lower()
+        assert "duckduckgo" in result["error"].lower() or "ddgs" in result["error"].lower()
+
+    def test_web_crawl_returns_search_only_error(self, monkeypatch):
+        import asyncio
+        from tools import web_tools
+
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {"backend": "ddgs"})
+        monkeypatch.setattr(web_tools, "_ddgs_package_importable", lambda: True)
+        monkeypatch.setattr(web_tools, "_is_tool_gateway_ready", lambda: False)
+        monkeypatch.setattr(web_tools, "check_firecrawl_api_key", lambda: False)
+        monkeypatch.setattr("tools.interrupt.is_interrupted", lambda: False, raising=False)
+
+        result_str = asyncio.get_event_loop().run_until_complete(
+            web_tools.web_crawl_tool("https://example.com")
+        )
+        result = json.loads(result_str)
+        assert result["success"] is False
+        assert "search-only" in result["error"].lower()
+        assert "duckduckgo" in result["error"].lower() or "ddgs" in result["error"].lower()