Merge main into bb/gui.

Resolve merge conflicts while preserving bb/gui dashboard paths and STT provider support.

tests/tools/test_browser_chromium_check.py

@@ -41,6 +41,16 @@ class TestChromiumSearchRoots:
 
 
 class TestChromiumInstalled:
+    def test_true_when_plain_chromium_on_path(self, monkeypatch):
+        monkeypatch.delenv("AGENT_BROWSER_EXECUTABLE_PATH", raising=False)
+        monkeypatch.setattr(
+            bt.shutil,
+            "which",
+            lambda name: "/usr/bin/chromium" if name == "chromium" else None,
+        )
+
+        assert bt._chromium_installed() is True
+
     def test_true_when_chromium_dir_present(self, monkeypatch, tmp_path):
         monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", str(tmp_path))
         (tmp_path / "chromium-1208").mkdir()
@@ -108,4 +118,3 @@ class TestRunBrowserCommandChromiumGuard:
     """
 
 
-

tests/tools/test_clarify_gateway.py

@@ -205,3 +205,23 @@ class TestGatewayTextIntercept:
         pending2 = cm.get_pending_for_session("sk")
         assert pending2 is not None
         assert pending2.clarify_id == "first"
+    def test_text_fallback_enables_awaiting_text_for_multi_choice(self):
+        """When base send_clarify renders choices as text, mark_awaiting_text
+        is called so the gateway text-intercept can capture the reply."""
+        from tools import clarify_gateway as cm
+
+        entry = cm.register("id-tf", "sk-tf", "Pick one", ["A", "B", "C"])
+        # Initially, multi-choice does NOT await text (button path)
+        assert entry.awaiting_text is False
+
+        # After the base send_clarify text fallback calls mark_awaiting_text:
+        flipped = cm.mark_awaiting_text("id-tf")
+        assert flipped is True
+
+        # Now get_pending_for_session should find it
+        pending = cm.get_pending_for_session("sk-tf")
+        assert pending is not None
+        assert pending.clarify_id == "id-tf"
+        
+        # Clean up
+        cm.clear_session("sk-tf")

tests/tools/test_clipboard.py

@@ -39,6 +39,7 @@ from cli import _should_auto_attach_clipboard_image_on_paste
 
 FAKE_PNG = b"\x89PNG\r\n\x1a\n" + b"\x00" * 100
 FAKE_BMP = b"BM" + b"\x00" * 100
+FAKE_JPEG = b"\xff\xd8\xff\xe0" + b"\x00" * 100
 
 
 # ═════════════════════════════════════════════════════════════════════════
@@ -393,9 +394,53 @@ class TestWaylandSave:
             if "stdout" in kw and hasattr(kw["stdout"], "write"):
                 kw["stdout"].write(FAKE_BMP)
             return MagicMock(returncode=0)
+
+        def fake_convert(path):
+            assert path == dest
+            path.write_bytes(FAKE_PNG)
+            return True
+
+        with patch("hermes_cli.clipboard.subprocess.run", side_effect=fake_run):
+            with patch("hermes_cli.clipboard._convert_to_png", side_effect=fake_convert):
+                assert _wayland_save(dest) is True
+
+    def test_jpeg_extraction_converts_to_real_png(self, tmp_path):
+        dest = tmp_path / "out.png"
+
+        def fake_run(cmd, **kw):
+            if "--list-types" in cmd:
+                return MagicMock(stdout="image/jpeg\ntext/plain\n", returncode=0)
+            if "stdout" in kw and hasattr(kw["stdout"], "write"):
+                kw["stdout"].write(FAKE_JPEG)
+            return MagicMock(returncode=0)
+
+        def fake_convert(path):
+            assert path == dest
+            path.write_bytes(FAKE_PNG)
+            return True
+
+        with patch("hermes_cli.clipboard.subprocess.run", side_effect=fake_run):
+            with patch("hermes_cli.clipboard._convert_to_png", side_effect=fake_convert) as mock_convert:
+                assert _wayland_save(dest) is True
+
+        mock_convert.assert_called_once_with(dest)
+        assert dest.read_bytes() == FAKE_PNG
+
+    def test_non_png_conversion_failure_cleans_up(self, tmp_path):
+        dest = tmp_path / "out.png"
+
+        def fake_run(cmd, **kw):
+            if "--list-types" in cmd:
+                return MagicMock(stdout="image/jpeg\n", returncode=0)
+            if "stdout" in kw and hasattr(kw["stdout"], "write"):
+                kw["stdout"].write(FAKE_JPEG)
+            return MagicMock(returncode=0)
+
         with patch("hermes_cli.clipboard.subprocess.run", side_effect=fake_run):
             with patch("hermes_cli.clipboard._convert_to_png", return_value=True):
-                assert _wayland_save(dest) is True
+                assert _wayland_save(dest) is False
+
+        assert not dest.exists()
 
     def test_no_image_types(self, tmp_path):
         dest = tmp_path / "out.png"

tests/tools/test_computer_use.py

@@ -591,6 +591,67 @@ class TestRunAgentMultimodalHelpers:
             for p in cleaned["content"]
         )
 
+    def test_computer_use_image_result_becomes_error_for_text_only_model(self):
+        from run_agent import AIAgent
+
+        agent = object.__new__(AIAgent)
+        agent.provider = "deepseek"
+        agent.model = "deepseek-v4-pro"
+        result = {
+            "_multimodal": True,
+            "content": [
+                {"type": "text", "text": "screen captured"},
+                {"type": "image_url", "image_url": {"url": "data:image/png;base64,x"}},
+            ],
+            "text_summary": "screen captured",
+        }
+
+        with patch.object(agent, "_model_supports_vision", return_value=False):
+            content = agent._tool_result_content_for_active_model("computer_use", result)
+
+        parsed = json.loads(content)
+        assert "computer_use returned screenshot/image content" in parsed["error"]
+        assert parsed["text_summary"] == "screen captured"
+        assert "image_url" not in content
+
+    def test_computer_use_image_result_preserved_for_vision_model(self):
+        from run_agent import AIAgent
+
+        agent = object.__new__(AIAgent)
+        result = {
+            "_multimodal": True,
+            "content": [
+                {"type": "text", "text": "screen captured"},
+                {"type": "image_url", "image_url": {"url": "data:image/png;base64,x"}},
+            ],
+        }
+
+        with patch.object(agent, "_model_supports_vision", return_value=True):
+            content = agent._tool_result_content_for_active_model("computer_use", result)
+
+        assert content is result["content"]
+        assert any(part.get("type") == "image_url" for part in content)
+
+    def test_other_multimodal_tool_uses_text_summary_for_text_only_model(self):
+        from run_agent import AIAgent
+
+        agent = object.__new__(AIAgent)
+        agent.provider = "custom"
+        agent.model = "text-only"
+        result = {
+            "_multimodal": True,
+            "content": [
+                {"type": "text", "text": "analysis text"},
+                {"type": "image_url", "image_url": {"url": "data:image/png;base64,x"}},
+            ],
+            "text_summary": "analysis summary",
+        }
+
+        with patch.object(agent, "_model_supports_vision", return_value=False):
+            content = agent._tool_result_content_for_active_model("vision_analyze", result)
+
+        assert content == "analysis summary"
+
 
 # ---------------------------------------------------------------------------
 # Universality: does the schema work without Anthropic?

tests/tools/test_cronjob_tools.py

@@ -122,6 +122,27 @@ class TestCronjobRequirements:
 
         assert check_cronjob_requirements() is False
 
+    @pytest.mark.parametrize("false_like_value", ["0", "false", "no", "off"])
+    def test_rejects_false_like_interactive_env(self, monkeypatch, false_like_value):
+        monkeypatch.setenv("HERMES_INTERACTIVE", false_like_value)
+        monkeypatch.delenv("HERMES_GATEWAY_SESSION", raising=False)
+        monkeypatch.delenv("HERMES_EXEC_ASK", raising=False)
+        assert check_cronjob_requirements() is False
+
+    @pytest.mark.parametrize(
+        "var_name",
+        ["HERMES_INTERACTIVE", "HERMES_GATEWAY_SESSION", "HERMES_EXEC_ASK"],
+    )
+    @pytest.mark.parametrize("false_like_value", ["0", "false", "no", "off"])
+    def test_rejects_false_like_any_session_env(
+        self, monkeypatch, var_name, false_like_value
+    ):
+        """All three session env vars share the same truthy semantics."""
+        for v in ("HERMES_INTERACTIVE", "HERMES_GATEWAY_SESSION", "HERMES_EXEC_ASK"):
+            monkeypatch.delenv(v, raising=False)
+        monkeypatch.setenv(var_name, false_like_value)
+        assert check_cronjob_requirements() is False
+
 
 class TestUnifiedCronjobTool:
     @pytest.fixture(autouse=True)

tests/tools/test_image_generation_env.py

@@ -37,3 +37,62 @@ def test_fal_key_empty_is_unset(monkeypatch):
     )
 
     assert image_generation_tool.check_fal_api_key() is False
+
+
+# ---------------------------------------------------------------------------
+# Actionable setup message when no FAL backend is reachable.
+# Regression for the silent-drop UX gap described in issue #2543.
+# ---------------------------------------------------------------------------
+
+
+def test_no_backend_message_mentions_fal_signup_and_plugins(monkeypatch):
+    from tools import image_generation_tool
+
+    monkeypatch.setattr(
+        image_generation_tool, "managed_nous_tools_enabled", lambda: False
+    )
+
+    msg = image_generation_tool._build_no_backend_setup_message()
+
+    assert "FAL_KEY" in msg
+    assert "https://fal.ai" in msg
+    # Plugin pointer so users on a stale image_gen.provider know where to look.
+    assert "hermes tools" in msg or "hermes plugins" in msg
+
+
+def test_no_backend_message_mentions_managed_gateway_when_enabled(monkeypatch):
+    from tools import image_generation_tool
+
+    monkeypatch.setattr(
+        image_generation_tool, "managed_nous_tools_enabled", lambda: True
+    )
+
+    msg = image_generation_tool._build_no_backend_setup_message()
+
+    assert "managed FAL gateway" in msg
+    assert "Nous account" in msg or "hermes setup" in msg
+
+
+def test_image_generate_tool_returns_actionable_error_when_no_backend(monkeypatch):
+    """End-to-end: handler must surface the actionable message, not a bare string."""
+    import json
+
+    from tools import image_generation_tool
+
+    monkeypatch.setattr(
+        image_generation_tool, "fal_key_is_configured", lambda: False
+    )
+    monkeypatch.setattr(
+        image_generation_tool, "_resolve_managed_fal_gateway", lambda: None
+    )
+    monkeypatch.setattr(
+        image_generation_tool, "managed_nous_tools_enabled", lambda: False
+    )
+
+    result = json.loads(
+        image_generation_tool.image_generate_tool(prompt="a cat")
+    )
+
+    assert result["success"] is False
+    assert "https://fal.ai" in result["error"]
+    assert "FAL_KEY" in result["error"]

tests/tools/test_lazy_deps.py

@@ -226,3 +226,182 @@ class TestIsAvailable:
         monkeypatch.setitem(ld.LAZY_DEPS, "test.miss", ("zzzfake>=1",))
         monkeypatch.setattr(ld, "_is_satisfied", lambda spec: False)
         assert ld.is_available("test.miss") is False
+
+
+# ---------------------------------------------------------------------------
+# Version-aware _is_satisfied (Piece B — "stale pin" detection)
+#
+# The original implementation returned True the moment the package name
+# was importable, ignoring the spec's version range. That meant pin bumps
+# in LAZY_DEPS never propagated to users who already lazy-installed the
+# backend at an older version. _is_satisfied now parses the spec and
+# checks the installed version against the constraint.
+# ---------------------------------------------------------------------------
+
+
+class TestIsSatisfiedVersionAware:
+    def _fake_version(self, monkeypatch, installed_versions: dict):
+        """Patch importlib.metadata.version() inside lazy_deps."""
+        from importlib.metadata import PackageNotFoundError
+
+        def _version(pkg):
+            if pkg in installed_versions:
+                return installed_versions[pkg]
+            raise PackageNotFoundError(pkg)
+
+        # Patch at the import site lazy_deps uses (inside the function).
+        import importlib.metadata as _md
+        monkeypatch.setattr(_md, "version", _version)
+
+    def test_exact_pin_match_returns_true(self, monkeypatch):
+        self._fake_version(monkeypatch, {"honcho-ai": "2.0.1"})
+        assert ld._is_satisfied("honcho-ai==2.0.1") is True
+
+    def test_exact_pin_mismatch_returns_false(self, monkeypatch):
+        # Installed 2.0.0, spec requires 2.0.1 → False (needs upgrade).
+        self._fake_version(monkeypatch, {"honcho-ai": "2.0.0"})
+        assert ld._is_satisfied("honcho-ai==2.0.1") is False
+
+    def test_range_within_returns_true(self, monkeypatch):
+        self._fake_version(monkeypatch, {"slack-bolt": "1.27.0"})
+        assert ld._is_satisfied("slack-bolt>=1.18.0,<2") is True
+
+    def test_range_above_returns_false(self, monkeypatch):
+        # Installed too new for the upper bound.
+        self._fake_version(monkeypatch, {"slack-bolt": "2.0.0"})
+        assert ld._is_satisfied("slack-bolt>=1.18.0,<2") is False
+
+    def test_range_below_returns_false(self, monkeypatch):
+        self._fake_version(monkeypatch, {"slack-bolt": "1.0.0"})
+        assert ld._is_satisfied("slack-bolt>=1.18.0,<2") is False
+
+    def test_package_not_installed_returns_false(self, monkeypatch):
+        self._fake_version(monkeypatch, {})
+        assert ld._is_satisfied("anthropic==0.86.0") is False
+
+    def test_bare_package_name_presence_is_enough(self, monkeypatch):
+        # No version constraint — presence alone counts as satisfied.
+        self._fake_version(monkeypatch, {"somepkg": "1.0.0"})
+        assert ld._is_satisfied("somepkg") is True
+
+    def test_extras_block_in_spec_is_stripped(self, monkeypatch):
+        # mautrix[encryption]==0.21.0 — the [encryption] block must not
+        # confuse the specifier parser.
+        self._fake_version(monkeypatch, {"mautrix": "0.21.0"})
+        assert ld._is_satisfied("mautrix[encryption]==0.21.0") is True
+
+    def test_extras_block_mismatch_returns_false(self, monkeypatch):
+        self._fake_version(monkeypatch, {"mautrix": "0.20.0"})
+        assert ld._is_satisfied("mautrix[encryption]==0.21.0") is False
+
+
+# ---------------------------------------------------------------------------
+# active_features + refresh_active_features (Piece A — hermes update wiring)
+# ---------------------------------------------------------------------------
+
+
+class TestActiveFeatures:
+    def test_no_packages_installed_returns_empty(self, monkeypatch):
+        monkeypatch.setattr(ld, "_is_present", lambda spec: False)
+        assert ld.active_features() == []
+
+    def test_finds_features_with_at_least_one_package_installed(self, monkeypatch):
+        # Pretend only honcho-ai is installed; nothing else.
+        monkeypatch.setattr(
+            ld, "_is_present",
+            lambda spec: ld._pkg_name_from_spec(spec) == "honcho-ai",
+        )
+        active = ld.active_features()
+        assert "memory.honcho" in active
+        # Backends the user never enabled stay quiet.
+        assert "memory.hindsight" not in active
+        assert "platform.slack" not in active
+
+    def test_multi_package_feature_active_if_any_present(self, monkeypatch):
+        # platform.slack has 3 packages; only one needs to be present
+        # for the feature to count as active (user activated it before,
+        # one transitive may have been uninstalled separately).
+        monkeypatch.setattr(
+            ld, "_is_present",
+            lambda spec: ld._pkg_name_from_spec(spec) == "slack-bolt",
+        )
+        assert "platform.slack" in ld.active_features()
+
+
+class TestRefreshActiveFeatures:
+    def test_no_active_features_returns_empty(self, monkeypatch):
+        monkeypatch.setattr(ld, "active_features", lambda: [])
+        assert ld.refresh_active_features() == {}
+
+    def test_already_current_is_noop(self, monkeypatch):
+        monkeypatch.setattr(ld, "active_features", lambda: ["test.feat"])
+        monkeypatch.setitem(ld.LAZY_DEPS, "test.feat", ("zzzfake==1.0.0",))
+        monkeypatch.setattr(ld, "_is_satisfied", lambda spec: True)
+        # If pip were called, this would fail loudly.
+        monkeypatch.setattr(
+            ld, "_venv_pip_install",
+            lambda *a, **kw: pytest.fail("pip should not be called"),
+        )
+        result = ld.refresh_active_features()
+        assert result == {"test.feat": "current"}
+
+    def test_stale_pin_triggers_reinstall(self, monkeypatch):
+        monkeypatch.setattr(ld, "active_features", lambda: ["test.feat"])
+        monkeypatch.setitem(ld.LAZY_DEPS, "test.feat", ("zzzfake==2.0.0",))
+        # First _is_satisfied check (in feature_missing) says no; after
+        # install, post-install check says yes.
+        states = iter([False, True])
+        monkeypatch.setattr(ld, "_is_satisfied", lambda spec: next(states))
+        monkeypatch.setattr(ld, "_allow_lazy_installs", lambda: True)
+        monkeypatch.setattr(
+            ld, "_venv_pip_install",
+            lambda specs, **kw: ld._InstallResult(True, "ok", ""),
+        )
+        result = ld.refresh_active_features()
+        assert result == {"test.feat": "refreshed"}
+
+    def test_install_failure_recorded_not_raised(self, monkeypatch):
+        # A failed refresh must NOT raise out of hermes update.
+        monkeypatch.setattr(ld, "active_features", lambda: ["test.feat"])
+        monkeypatch.setitem(ld.LAZY_DEPS, "test.feat", ("zzzfake==2.0.0",))
+        monkeypatch.setattr(ld, "_is_satisfied", lambda spec: False)
+        monkeypatch.setattr(ld, "_allow_lazy_installs", lambda: True)
+        monkeypatch.setattr(
+            ld, "_venv_pip_install",
+            lambda specs, **kw: ld._InstallResult(
+                False, "", "ERROR: PyPI 404 quarantine"
+            ),
+        )
+        result = ld.refresh_active_features()
+        assert "test.feat" in result
+        assert result["test.feat"].startswith("failed:")
+        assert "404 quarantine" in result["test.feat"]
+
+    def test_lazy_installs_disabled_marked_skipped(self, monkeypatch):
+        # security.allow_lazy_installs=false → don't error, mark skipped
+        # so hermes update can render "respecting your config" message.
+        monkeypatch.setattr(ld, "active_features", lambda: ["test.feat"])
+        monkeypatch.setitem(ld.LAZY_DEPS, "test.feat", ("zzzfake==2.0.0",))
+        monkeypatch.setattr(ld, "_is_satisfied", lambda spec: False)
+        monkeypatch.setattr(ld, "_allow_lazy_installs", lambda: False)
+        result = ld.refresh_active_features()
+        assert "test.feat" in result
+        assert result["test.feat"].startswith("skipped:")
+
+    def test_mixed_results_returns_per_feature_status(self, monkeypatch):
+        monkeypatch.setattr(ld, "active_features", lambda: ["a.ok", "b.fail"])
+        monkeypatch.setitem(ld.LAZY_DEPS, "a.ok", ("pkga==1.0",))
+        monkeypatch.setitem(ld.LAZY_DEPS, "b.fail", ("pkgb==1.0",))
+        # a.ok: already satisfied → "current"
+        # b.fail: missing + install fails → "failed:"
+        def fake_satisfied(spec):
+            return ld._pkg_name_from_spec(spec) == "pkga"
+        monkeypatch.setattr(ld, "_is_satisfied", fake_satisfied)
+        monkeypatch.setattr(ld, "_allow_lazy_installs", lambda: True)
+        monkeypatch.setattr(
+            ld, "_venv_pip_install",
+            lambda specs, **kw: ld._InstallResult(False, "", "nope"),
+        )
+        result = ld.refresh_active_features()
+        assert result["a.ok"] == "current"
+        assert result["b.fail"].startswith("failed:")

tests/tools/test_managed_server_tool_support.py

@@ -1,178 +0,0 @@
-"""
-Tests for ManagedServer / tool-parser integration.
-
-Validates that:
-1. The installed atroposlib API still matches Hermes's expectations
-2. Hermes's parser registry remains compatible with ManagedServer parsing
-3. HermesAgentBaseEnv wires the selected parser into ServerManager correctly
-
-These tests verify the contract between hermes-agent's environments/ code
-and atroposlib's ManagedServer. They detect API incompatibilities early.
-"""
-
-import inspect
-import sys
-from pathlib import Path
-
-import pytest
-
-sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
-
-try:
-    import atroposlib  # noqa: F401
-except ImportError:
-    pytest.skip("atroposlib not installed", allow_module_level=True)
-
-
-class TestManagedServerAPI:
-    """Test that ManagedServer's API matches what hermes-agent expects."""
-
-    def test_managed_server_init_signature(self):
-        """ManagedServer should accept tool_call_parser parameter."""
-        from atroposlib.envs.server_handling.managed_server import ManagedServer
-
-        sig = inspect.signature(ManagedServer.__init__)
-        params = list(sig.parameters.keys())
-
-        # Core params that must exist
-        assert "self" in params
-        assert "server" in params
-        assert "tokenizer" in params
-        assert "track_tree" in params
-
-        # tool_call_parser — required for tool_call_support branch
-        # If this fails, atroposlib hasn't been updated to tool_call_support
-        has_tool_parser = "tool_call_parser" in params
-        if not has_tool_parser:
-            pytest.skip(
-                "ManagedServer does not have tool_call_parser param — "
-                "baseline atroposlib (pre tool_call_support branch)"
-            )
-
-    def test_server_manager_managed_server_signature(self):
-        """ServerManager.managed_server() should accept tool_call_parser."""
-        from atroposlib.envs.server_handling.server_manager import ServerManager
-
-        sig = inspect.signature(ServerManager.managed_server)
-        params = list(sig.parameters.keys())
-
-        assert "self" in params
-        assert "tokenizer" in params
-
-        has_tool_parser = "tool_call_parser" in params
-        if not has_tool_parser:
-            pytest.skip(
-                "ServerManager.managed_server() does not have tool_call_parser param — "
-                "baseline atroposlib (pre tool_call_support branch)"
-            )
-
-    def test_managed_server_chat_template_kwargs(self):
-        """ManagedServer should have CHAT_TEMPLATE_KWARGS for forwarding tools/thinking."""
-        from atroposlib.envs.server_handling.managed_server import ManagedServer
-
-        if not hasattr(ManagedServer, "CHAT_TEMPLATE_KWARGS"):
-            pytest.skip(
-                "ManagedServer does not have CHAT_TEMPLATE_KWARGS — "
-                "baseline atroposlib (pre tool_call_support branch)"
-            )
-
-        kwargs = ManagedServer.CHAT_TEMPLATE_KWARGS
-        assert "tools" in kwargs, "tools must be in CHAT_TEMPLATE_KWARGS"
-
-    def test_no_get_logprobs_method(self):
-        """get_logprobs should be removed in tool_call_support branch."""
-        from atroposlib.envs.server_handling.managed_server import ManagedServer
-
-        # In baseline, get_logprobs exists. In tool_call_support, it's removed.
-        # We just note the state — not a hard fail either way.
-        has_get_logprobs = hasattr(ManagedServer, "get_logprobs")
-        if has_get_logprobs:
-            pytest.skip(
-                "ManagedServer still has get_logprobs — baseline atroposlib"
-            )
-
-
-class TestParserCompatibility:
-    """Test that hermes-agent's parsers match ManagedServer's expectations."""
-
-    def test_parser_parse_returns_correct_format(self):
-        """
-        ManagedServer expects parser.parse(text) -> (content, tool_calls)
-        where tool_calls is a list of objects with .id, .function.name, .function.arguments
-        """
-        from environments.tool_call_parsers import get_parser
-
-        parser = get_parser("hermes")
-        text = '<tool_call>{"name": "terminal", "arguments": {"command": "ls"}}</tool_call>'
-        content, tool_calls = parser.parse(text)
-
-        assert tool_calls is not None
-        assert len(tool_calls) == 1
-
-        tc = tool_calls[0]
-        # ManagedServer accesses these attrs directly
-        assert hasattr(tc, "id")
-        assert hasattr(tc, "function")
-        assert hasattr(tc.function, "name")
-        assert hasattr(tc.function, "arguments")
-
-    def test_parser_no_tools_returns_none(self):
-        """ManagedServer checks `if parsed_tool_calls:` — None should be falsy."""
-        from environments.tool_call_parsers import get_parser
-
-        parser = get_parser("hermes")
-        content, tool_calls = parser.parse("Just text, no tools")
-        assert tool_calls is None
-
-    def test_parser_content_is_string_or_none(self):
-        """ManagedServer uses `parsed_content or ""` — must be str or None."""
-        from environments.tool_call_parsers import get_parser
-
-        parser = get_parser("hermes")
-
-        # With tool calls
-        text = '<tool_call>{"name": "terminal", "arguments": {"command": "ls"}}</tool_call>'
-        content, _ = parser.parse(text)
-        assert content is None or isinstance(content, str)
-
-        # Without tool calls
-        content2, _ = parser.parse("Just text")
-        assert isinstance(content2, str)
-
-
-class TestBaseEnvCompatibility:
-    """Test that hermes_base_env.py's tool-parser wiring matches the current API."""
-
-    def test_hermes_base_env_sets_server_manager_tool_parser(self):
-        """Hermes wires parser selection through ServerManager.tool_parser."""
-        import ast
-
-        base_env_path = Path(__file__).parent.parent.parent / "environments" / "hermes_base_env.py"
-        source = base_env_path.read_text()
-        tree = ast.parse(source)
-
-        found_assignment = False
-        for node in ast.walk(tree):
-            if isinstance(node, ast.Assign):
-                for target in node.targets:
-                    if isinstance(target, ast.Attribute) and target.attr == "tool_parser":
-                        parent = target.value
-                        if (
-                            isinstance(parent, ast.Attribute)
-                            and parent.attr == "server"
-                            and isinstance(parent.value, ast.Name)
-                            and parent.value.id == "self"
-                        ):
-                            found_assignment = True
-
-        assert found_assignment, (
-            "hermes_base_env.py should set self.server.tool_parser from config.tool_call_parser"
-        )
-
-    def test_hermes_base_env_uses_config_tool_call_parser(self):
-        """Verify hermes_base_env uses the config field rather than a local parser instance."""
-        base_env_path = Path(__file__).parent.parent.parent / "environments" / "hermes_base_env.py"
-        source = base_env_path.read_text()
-
-        assert 'tool_call_parser: str = Field(' in source
-        assert 'self.server.tool_parser = config.tool_call_parser' in source

tests/tools/test_mcp_tool.py

@@ -1592,6 +1592,40 @@ class TestReconnection:
 
         asyncio.run(_test())
 
+    def test_initial_oauth_failure_does_not_retry(self):
+        """Initial OAuth failures stop immediately to avoid repeated browser prompts."""
+        from tools.mcp_tool import MCPServerTask
+
+        run_count = 0
+        target_server = None
+        oauth_error = RuntimeError("Token exchange failed (400): Unknown client_id")
+
+        original_run_stdio = MCPServerTask._run_stdio
+
+        async def patched_run_stdio(self_srv, config):
+            nonlocal run_count, target_server
+            run_count += 1
+            if target_server is not self_srv:
+                return await original_run_stdio(self_srv, config)
+            raise oauth_error
+
+        async def _test():
+            nonlocal target_server
+            server = MCPServerTask("oauth_srv")
+            target_server = server
+
+            with patch.object(MCPServerTask, "_run_stdio", patched_run_stdio), \
+                 patch("tools.mcp_tool._is_auth_error", return_value=True), \
+                 patch("asyncio.sleep", new_callable=AsyncMock) as mock_sleep:
+                await server.run({"command": "test"})
+
+            assert run_count == 1
+            assert server._error is oauth_error
+            assert server._ready.is_set()
+            assert mock_sleep.await_count == 0
+
+        asyncio.run(_test())
+
 
 # ---------------------------------------------------------------------------
 # Configurable timeouts

tests/tools/test_process_registry.py

@@ -865,3 +865,138 @@ class TestProcessToolHandler:
         from tools.process_registry import _handle_process
         result = json.loads(_handle_process({"action": "unknown_action"}))
         assert "error" in result
+
+
+# =========================================================================
+# format_process_notification + drain_notifications (shared helpers)
+# =========================================================================
+
+from tools.process_registry import format_process_notification
+
+
+def test_format_completion_event():
+    evt = {
+        "type": "completion",
+        "session_id": "proc_abc",
+        "command": "sleep 5",
+        "exit_code": 0,
+        "output": "done",
+    }
+    result = format_process_notification(evt)
+    assert "[IMPORTANT: Background process proc_abc completed" in result
+    assert "exit code 0" in result
+    assert "Command: sleep 5" in result
+    assert "Output:\ndone]" in result
+
+
+def test_format_watch_match_event():
+    evt = {
+        "type": "watch_match",
+        "session_id": "proc_xyz",
+        "command": "tail -f log",
+        "pattern": "ERROR",
+        "output": "ERROR: disk full",
+        "suppressed": 0,
+    }
+    result = format_process_notification(evt)
+    assert 'watch pattern "ERROR"' in result
+    assert "Matched output:\nERROR: disk full" in result
+
+
+def test_format_watch_match_with_suppressed():
+    evt = {
+        "type": "watch_match",
+        "session_id": "proc_xyz",
+        "command": "tail -f log",
+        "pattern": "WARN",
+        "output": "WARN: low mem",
+        "suppressed": 3,
+    }
+    result = format_process_notification(evt)
+    assert "3 earlier matches were suppressed" in result
+
+
+def test_format_watch_disabled_event():
+    evt = {
+        "type": "watch_disabled",
+        "message": "Watch disabled for proc_xyz: too many matches",
+    }
+    result = format_process_notification(evt)
+    assert "[IMPORTANT: Watch disabled for proc_xyz" in result
+
+
+def test_format_returns_none_for_empty_event():
+    evt = {}
+    result = format_process_notification(evt)
+    assert result is not None
+    assert "unknown" in result
+
+
+def test_drain_notifications_returns_pending_events():
+    from tools.process_registry import process_registry
+
+    while not process_registry.completion_queue.empty():
+        process_registry.completion_queue.get_nowait()
+
+    process_registry.completion_queue.put({
+        "type": "completion",
+        "session_id": "proc_drain1",
+        "command": "echo hi",
+        "exit_code": 0,
+        "output": "hi",
+    })
+    process_registry.completion_queue.put({
+        "type": "watch_match",
+        "session_id": "proc_drain2",
+        "command": "tail -f x",
+        "pattern": "ERR",
+        "output": "ERR found",
+        "suppressed": 0,
+    })
+
+    try:
+        results = process_registry.drain_notifications()
+        assert len(results) == 2
+        assert results[0][0]["session_id"] == "proc_drain1"
+        assert "proc_drain1 completed" in results[0][1]
+        assert results[1][0]["session_id"] == "proc_drain2"
+        assert "watch pattern" in results[1][1]
+    finally:
+        while not process_registry.completion_queue.empty():
+            process_registry.completion_queue.get_nowait()
+        process_registry._completion_consumed.discard("proc_drain1")
+        process_registry._completion_consumed.discard("proc_drain2")
+
+
+def test_drain_notifications_skips_consumed():
+    from tools.process_registry import process_registry
+
+    while not process_registry.completion_queue.empty():
+        process_registry.completion_queue.get_nowait()
+
+    process_registry._completion_consumed.add("proc_consumed")
+    process_registry.completion_queue.put({
+        "type": "completion",
+        "session_id": "proc_consumed",
+        "command": "echo done",
+        "exit_code": 0,
+        "output": "done",
+    })
+
+    try:
+        results = process_registry.drain_notifications()
+        assert len(results) == 0
+    finally:
+        process_registry._completion_consumed.discard("proc_consumed")
+        while not process_registry.completion_queue.empty():
+            process_registry.completion_queue.get_nowait()
+
+
+def test_drain_notifications_empty_queue():
+    from tools.process_registry import process_registry
+
+    while not process_registry.completion_queue.empty():
+        process_registry.completion_queue.get_nowait()
+
+    results = process_registry.drain_notifications()
+    assert results == []

tests/tools/test_registry.py

@@ -5,7 +5,7 @@ import threading
 from pathlib import Path
 from unittest.mock import patch
 
-from tools.registry import ToolRegistry, discover_builtin_tools
+from tools.registry import ToolRegistry, _module_registers_tools, discover_builtin_tools
 
 
 def _dummy_handler(args, **kwargs):
@@ -289,43 +289,19 @@ class TestCheckFnExceptionHandling:
 
 
 class TestBuiltinDiscovery:
-    def test_matches_previous_manual_builtin_tool_set(self):
-        expected = {
-            "tools.browser_cdp_tool",
-            "tools.browser_dialog_tool",
-            "tools.browser_tool",
-            "tools.clarify_tool",
-            "tools.code_execution_tool",
-            "tools.computer_use_tool",
-            "tools.cronjob_tools",
-            "tools.delegate_tool",
-            "tools.discord_tool",
-            "tools.feishu_doc_tool",
-            "tools.feishu_drive_tool",
-            "tools.file_tools",
-            "tools.homeassistant_tool",
-            "tools.image_generation_tool",
-            "tools.kanban_tools",
-            "tools.memory_tool",
-            "tools.mixture_of_agents_tool",
-            "tools.process_registry",
-            "tools.rl_training_tool",
-            "tools.send_message_tool",
-            "tools.session_search_tool",
-            "tools.skill_manager_tool",
-            "tools.skills_tool",
-            "tools.terminal_tool",
-            "tools.todo_tool",
-            "tools.tts_tool",
-            "tools.vision_tools",
-            "tools.web_tools",
-            "tools.yuanbao_tools",
-        }
+    def test_discovers_all_real_self_registering_builtin_tool_modules(self):
+        tools_dir = Path(__file__).resolve().parents[2] / "tools"
+        expected = [
+            f"tools.{path.stem}"
+            for path in sorted(tools_dir.glob("*.py"))
+            if path.name not in {"__init__.py", "registry.py", "mcp_tool.py"}
+            and _module_registers_tools(path)
+        ]
 
         with patch("tools.registry.importlib.import_module"):
-            imported = discover_builtin_tools(Path(__file__).resolve().parents[2] / "tools")
+            imported = discover_builtin_tools(tools_dir)
 
-        assert set(imported) == expected
+        assert imported == expected
 
     def test_imports_only_self_registering_modules(self, tmp_path):
         tools_dir = tmp_path / "tools"

tests/tools/test_rl_training_tool.py

@@ -1,142 +0,0 @@
-"""Tests for rl_training_tool.py — file handle lifecycle and cleanup.
-
-Verifies that _stop_training_run properly closes log file handles,
-terminates processes, and handles edge cases on failure paths.
-Inspired by PR #715 (0xbyt4).
-"""
-
-from unittest.mock import MagicMock
-
-import pytest
-
-from tools.rl_training_tool import RunState, _stop_training_run
-
-
-def _make_run_state(**overrides) -> RunState:
-    """Create a minimal RunState for testing."""
-    defaults = {
-        "run_id": "test-run-001",
-        "environment": "test_env",
-        "config": {},
-    }
-    defaults.update(overrides)
-    return RunState(**defaults)
-
-
-class TestStopTrainingRunFileHandles:
-    """Verify that _stop_training_run closes log file handles stored as attributes."""
-
-    def test_closes_all_log_file_handles(self):
-        state = _make_run_state()
-        files = {}
-        for attr in ("api_log_file", "trainer_log_file", "env_log_file"):
-            fh = MagicMock()
-            setattr(state, attr, fh)
-            files[attr] = fh
-
-        _stop_training_run(state)
-
-        for attr, fh in files.items():
-            fh.close.assert_called_once()
-            assert getattr(state, attr) is None
-
-    def test_clears_file_attrs_to_none(self):
-        state = _make_run_state()
-        state.api_log_file = MagicMock()
-
-        _stop_training_run(state)
-
-        assert state.api_log_file is None
-
-    def test_close_exception_does_not_propagate(self):
-        """If a file handle .close() raises, it must not crash."""
-        state = _make_run_state()
-        bad_fh = MagicMock()
-        bad_fh.close.side_effect = OSError("already closed")
-        good_fh = MagicMock()
-        state.api_log_file = bad_fh
-        state.trainer_log_file = good_fh
-
-        _stop_training_run(state)  # should not raise
-
-        bad_fh.close.assert_called_once()
-        good_fh.close.assert_called_once()
-
-    def test_handles_missing_file_attrs(self):
-        """RunState without log file attrs should not crash."""
-        state = _make_run_state()
-        # No log file attrs set at all — getattr(..., None) should handle it
-        _stop_training_run(state)  # should not raise
-
-
-class TestStopTrainingRunProcesses:
-    """Verify that _stop_training_run terminates processes correctly."""
-
-    def test_terminates_running_processes(self):
-        state = _make_run_state()
-        for attr in ("api_process", "trainer_process", "env_process"):
-            proc = MagicMock()
-            proc.poll.return_value = None  # still running
-            setattr(state, attr, proc)
-
-        _stop_training_run(state)
-
-        for attr in ("api_process", "trainer_process", "env_process"):
-            getattr(state, attr).terminate.assert_called_once()
-
-    def test_does_not_terminate_exited_processes(self):
-        state = _make_run_state()
-        proc = MagicMock()
-        proc.poll.return_value = 0  # already exited
-        state.api_process = proc
-
-        _stop_training_run(state)
-
-        proc.terminate.assert_not_called()
-
-    def test_handles_none_processes(self):
-        state = _make_run_state()
-        # All process attrs are None by default
-        _stop_training_run(state)  # should not raise
-
-    def test_handles_mixed_running_and_exited_processes(self):
-        state = _make_run_state()
-        # api still running
-        api = MagicMock()
-        api.poll.return_value = None
-        state.api_process = api
-        # trainer already exited
-        trainer = MagicMock()
-        trainer.poll.return_value = 0
-        state.trainer_process = trainer
-        # env is None
-        state.env_process = None
-
-        _stop_training_run(state)
-
-        api.terminate.assert_called_once()
-        trainer.terminate.assert_not_called()
-
-
-class TestStopTrainingRunStatus:
-    """Verify status transitions in _stop_training_run."""
-
-    def test_sets_status_to_stopped_when_running(self):
-        state = _make_run_state(status="running")
-        _stop_training_run(state)
-        assert state.status == "stopped"
-
-    def test_does_not_change_status_when_failed(self):
-        state = _make_run_state(status="failed")
-        _stop_training_run(state)
-        assert state.status == "failed"
-
-    def test_does_not_change_status_when_pending(self):
-        state = _make_run_state(status="pending")
-        _stop_training_run(state)
-        assert state.status == "pending"
-
-    def test_no_crash_with_no_processes_and_no_files(self):
-        state = _make_run_state()
-        _stop_training_run(state)  # should not raise
-        assert state.status == "pending"

tests/tools/test_skills_tool.py

@@ -1076,3 +1076,168 @@ Do the legacy thing.
         assert result["setup_needed"] is False
         assert result["missing_required_environment_variables"] == []
         assert result["readiness_status"] == "available"
+
+
+class TestSkillViewCollisionDetection:
+    """Regression tests for skill_view name collision handling.
+
+    When a skill name resolves to multiple paths across the local skills
+    dir and external_dirs, skill_view must refuse to guess. Silent
+    shadowing — where ``/skills`` shows the local version but
+    ``skill_view`` loads the external one — is the bug class this guards
+    against. Reproduces with `skills.external_dirs` registered in
+    config.yaml and a same-name skill nested under a category locally.
+
+    Adapted from a regression suite originally proposed by @polkn in PR
+    #6136 (which used local-first precedence). The collision-refusal
+    behavior preserves the same protection without silently picking a
+    side, and gives the user an actionable hint (use the categorized
+    path) to recover.
+    """
+
+    def _patch_dirs(self, local_dir, external_dirs):
+        """Patch SKILLS_DIR (module-level) and get_external_skills_dirs at source."""
+        return (
+            patch("tools.skills_tool.SKILLS_DIR", local_dir),
+            patch(
+                "agent.skill_utils.get_external_skills_dirs",
+                return_value=list(external_dirs),
+            ),
+        )
+
+    def test_nested_local_collides_with_top_level_external(self, tmp_path):
+        """The original bug scenario: nested local + top-level external,
+        same name. Now refuses with both paths surfaced."""
+        local_dir = tmp_path / "local"
+        external_dir = tmp_path / "external"
+        local_dir.mkdir()
+        external_dir.mkdir()
+
+        _make_skill(
+            local_dir,
+            "explore-codebase",
+            category="foundations/runtime",
+            body="LOCAL VERSION",
+        )
+        _make_skill(external_dir, "explore-codebase", body="EXTERNAL VERSION")
+
+        p1, p2 = self._patch_dirs(local_dir, [external_dir])
+        with p1, p2:
+            raw = skill_view("explore-codebase")
+
+        result = json.loads(raw)
+        assert result["success"] is False
+        assert "Ambiguous skill name 'explore-codebase'" in result["error"]
+        assert "matches" in result
+        assert len(result["matches"]) == 2
+        # Both paths surfaced
+        assert any("foundations/runtime" in p for p in result["matches"])
+        assert any("external" in p for p in result["matches"])
+        assert "hint" in result
+
+    def test_top_level_local_collides_with_external(self, tmp_path):
+        """Top-level local + top-level external with the same name also
+        refuses — same-name shadowing is ambiguous regardless of nesting."""
+        local_dir = tmp_path / "local"
+        external_dir = tmp_path / "external"
+        local_dir.mkdir()
+        external_dir.mkdir()
+
+        _make_skill(local_dir, "shared-name", body="LOCAL VERSION")
+        _make_skill(external_dir, "shared-name", body="EXTERNAL VERSION")
+
+        p1, p2 = self._patch_dirs(local_dir, [external_dir])
+        with p1, p2:
+            raw = skill_view("shared-name")
+
+        result = json.loads(raw)
+        assert result["success"] is False
+        assert "Ambiguous" in result["error"]
+        assert len(result["matches"]) == 2
+
+    def test_collision_resolvable_via_categorized_path(self, tmp_path):
+        """User can recover from a collision by passing the full
+        categorized path — the bare name is ambiguous, the path is not."""
+        local_dir = tmp_path / "local"
+        external_dir = tmp_path / "external"
+        local_dir.mkdir()
+        external_dir.mkdir()
+
+        _make_skill(
+            local_dir,
+            "explore-codebase",
+            category="foundations/runtime",
+            body="LOCAL VERSION",
+        )
+        _make_skill(external_dir, "explore-codebase", body="EXTERNAL VERSION")
+
+        p1, p2 = self._patch_dirs(local_dir, [external_dir])
+        with p1, p2:
+            raw = skill_view("foundations/runtime/explore-codebase")
+
+        result = json.loads(raw)
+        assert result["success"] is True
+        assert "LOCAL VERSION" in result["content"]
+
+    def test_external_skill_resolves_when_no_collision(self, tmp_path):
+        """External-only skills still resolve normally when there's no
+        local skill of the same name."""
+        local_dir = tmp_path / "local"
+        external_dir = tmp_path / "external"
+        local_dir.mkdir()
+        external_dir.mkdir()
+
+        _make_skill(external_dir, "external-only", body="EXTERNAL BODY")
+
+        p1, p2 = self._patch_dirs(local_dir, [external_dir])
+        with p1, p2:
+            raw = skill_view("external-only")
+
+        result = json.loads(raw)
+        assert result["success"] is True
+        assert "EXTERNAL BODY" in result["content"]
+
+    def test_two_externals_same_name_also_refuse(self, tmp_path):
+        """Collision detection is symmetric — two external dirs with
+        same-name skills also trigger the refusal."""
+        local_dir = tmp_path / "local"
+        ext_a = tmp_path / "ext_a"
+        ext_b = tmp_path / "ext_b"
+        local_dir.mkdir()
+        ext_a.mkdir()
+        ext_b.mkdir()
+
+        _make_skill(ext_a, "pr", body="EXT_A VERSION")
+        _make_skill(ext_b, "pr", body="EXT_B VERSION")
+
+        p1, p2 = self._patch_dirs(local_dir, [ext_a, ext_b])
+        with p1, p2:
+            raw = skill_view("pr")
+
+        result = json.loads(raw)
+        assert result["success"] is False
+        assert "Ambiguous" in result["error"]
+        assert len(result["matches"]) == 2
+
+    def test_local_only_skill_loads_normally(self, tmp_path):
+        """Sanity: a single local skill (no external collision) loads
+        without any error."""
+        local_dir = tmp_path / "local"
+        external_dir = tmp_path / "external"
+        local_dir.mkdir()
+        external_dir.mkdir()
+
+        _make_skill(
+            local_dir,
+            "my-skill",
+            category="foundations/runtime",
+            body="LOCAL BODY",
+        )
+
+        p1, p2 = self._patch_dirs(local_dir, [external_dir])
+        with p1, p2:
+            raw = skill_view("my-skill")
+
+        result = json.loads(raw)
+        assert result["success"] is True
+        assert "LOCAL BODY" in result["content"]

tests/tools/test_tool_call_parsers.py

@@ -1,274 +0,0 @@
-"""
-Tests for environments/tool_call_parsers/ — client-side tool call parsers.
-
-These parsers extract structured tool_calls from raw model output text.
-Used in Phase 2 (VLLM/generate) where the server returns raw tokens.
-"""
-
-import json
-import sys
-from pathlib import Path
-
-import pytest
-
-# Ensure repo root is importable
-sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
-
-try:
-    from environments.tool_call_parsers import (
-        ParseResult,
-        ToolCallParser,
-        get_parser,
-        list_parsers,
-    )
-except ImportError:
-    pytest.skip("atroposlib not installed", allow_module_level=True)
-
-
-# ─── Registry tests ─────────────────────────────────────────────────────
-
-class TestParserRegistry:
-    def test_list_parsers_returns_nonempty(self):
-        parsers = list_parsers()
-        assert len(parsers) > 0
-
-    def test_hermes_parser_registered(self):
-        parsers = list_parsers()
-        assert "hermes" in parsers
-
-    def test_get_parser_returns_instance(self):
-        parser = get_parser("hermes")
-        assert isinstance(parser, ToolCallParser)
-
-    def test_get_parser_unknown_raises(self):
-        with pytest.raises(KeyError):
-            get_parser("nonexistent_parser_xyz")
-
-    def test_all_registered_parsers_instantiate(self):
-        """Every registered parser should be importable and instantiable."""
-        for name in list_parsers():
-            parser = get_parser(name)
-            assert isinstance(parser, ToolCallParser)
-            assert hasattr(parser, "parse")
-
-
-# ─── Hermes parser tests ────────────────────────────────────────────────
-
-class TestHermesParser:
-    @pytest.fixture
-    def parser(self):
-        return get_parser("hermes")
-
-    def test_no_tool_call(self, parser):
-        text = "Hello, I can help you with that."
-        content, tool_calls = parser.parse(text)
-        assert content == text
-        assert tool_calls is None
-
-    def test_single_tool_call(self, parser):
-        text = '<tool_call>{"name": "terminal", "arguments": {"command": "ls -la"}}</tool_call>'
-        content, tool_calls = parser.parse(text)
-        assert tool_calls is not None
-        assert len(tool_calls) == 1
-        assert tool_calls[0].function.name == "terminal"
-        args = json.loads(tool_calls[0].function.arguments)
-        assert args["command"] == "ls -la"
-
-    def test_tool_call_with_surrounding_text(self, parser):
-        text = 'Let me check that for you.\n<tool_call>{"name": "terminal", "arguments": {"command": "pwd"}}</tool_call>'
-        content, tool_calls = parser.parse(text)
-        assert tool_calls is not None
-        assert len(tool_calls) == 1
-        assert tool_calls[0].function.name == "terminal"
-        # Content should have the surrounding text
-        if content is not None:
-            assert "check that" in content or content.strip() != ""
-
-    def test_multiple_tool_calls(self, parser):
-        text = (
-            '<tool_call>{"name": "terminal", "arguments": {"command": "ls"}}</tool_call>\n'
-            '<tool_call>{"name": "read_file", "arguments": {"path": "test.py"}}</tool_call>'
-        )
-        content, tool_calls = parser.parse(text)
-        assert tool_calls is not None
-        assert len(tool_calls) == 2
-        names = {tc.function.name for tc in tool_calls}
-        assert "terminal" in names
-        assert "read_file" in names
-
-    def test_tool_call_ids_are_unique(self, parser):
-        text = (
-            '<tool_call>{"name": "terminal", "arguments": {"command": "ls"}}</tool_call>\n'
-            '<tool_call>{"name": "terminal", "arguments": {"command": "pwd"}}</tool_call>'
-        )
-        _, tool_calls = parser.parse(text)
-        assert tool_calls is not None
-        ids = [tc.id for tc in tool_calls]
-        assert len(ids) == len(set(ids)), "Tool call IDs must be unique"
-
-    def test_empty_string(self, parser):
-        content, tool_calls = parser.parse("")
-        assert tool_calls is None
-
-    def test_malformed_json_in_tool_call(self, parser):
-        text = '<tool_call>not valid json</tool_call>'
-        content, tool_calls = parser.parse(text)
-        # Should either return None tool_calls or handle gracefully
-        # (implementation may vary — some parsers return error tool calls)
-
-    def test_truncated_tool_call(self, parser):
-        """Test handling of unclosed tool_call tag (model truncated mid-generation)."""
-        text = '<tool_call>{"name": "terminal", "arguments": {"command": "ls -la"}'
-        content, tool_calls = parser.parse(text)
-        # Parser should handle truncated output gracefully
-        # Either parse it successfully or return None
-
-
-# ─── Parse result contract tests (applies to ALL parsers) ───────────────
-
-class TestParseResultContract:
-    """Ensure all parsers conform to the ParseResult contract."""
-
-    @pytest.fixture(params=["hermes"])  # Add more as needed
-    def parser(self, request):
-        return get_parser(request.param)
-
-    def test_returns_tuple_of_two(self, parser):
-        result = parser.parse("hello world")
-        assert isinstance(result, tuple)
-        assert len(result) == 2
-
-    def test_no_tools_returns_none_tool_calls(self, parser):
-        content, tool_calls = parser.parse("Just plain text, no tools.")
-        assert tool_calls is None
-        assert content is not None
-
-    def test_tool_calls_are_proper_objects(self, parser):
-        """When tool calls are found, they should be ChatCompletionMessageToolCall objects."""
-        # Use hermes format since that's universal
-        text = '<tool_call>{"name": "terminal", "arguments": {"command": "echo hi"}}</tool_call>'
-        content, tool_calls = parser.parse(text)
-        if tool_calls is not None:
-            for tc in tool_calls:
-                assert hasattr(tc, "id")
-                assert hasattr(tc, "function")
-                assert hasattr(tc.function, "name")
-                assert hasattr(tc.function, "arguments")
-                assert tc.id is not None
-                assert isinstance(tc.function.name, str)
-                assert isinstance(tc.function.arguments, str)
-
-
-# ─── DeepSeek V3 parser tests ───────────────────────────────────────────
-
-class TestDeepSeekV3Parser:
-    @pytest.fixture
-    def parser(self):
-        return get_parser("deepseek_v3")
-
-    def test_no_tool_call(self, parser):
-        text = "Hello, how can I help you?"
-        content, tool_calls = parser.parse(text)
-        assert content == text
-        assert tool_calls is None
-
-    def test_single_tool_call(self, parser):
-        text = (
-            '<｜tool▁calls▁begin｜><｜tool▁call▁begin｜>function<｜tool▁sep｜>get_weather\n'
-            '```json\n{"city": "London"}\n```<｜tool▁call▁end｜><｜tool▁calls▁end｜>'
-        )
-        content, tool_calls = parser.parse(text)
-        assert tool_calls is not None
-        assert len(tool_calls) == 1
-        assert tool_calls[0].function.name == "get_weather"
-        args = json.loads(tool_calls[0].function.arguments)
-        assert args["city"] == "London"
-
-    def test_multiple_tool_calls(self, parser):
-        text = (
-            '<｜tool▁calls▁begin｜>'
-            '<｜tool▁call▁begin｜>function<｜tool▁sep｜>get_weather\n'
-            '```json\n{"city": "London"}\n```<｜tool▁call▁end｜>'
-            '<｜tool▁call▁begin｜>function<｜tool▁sep｜>get_time\n'
-            '```json\n{"timezone": "UTC"}\n```<｜tool▁call▁end｜>'
-            '<｜tool▁calls▁end｜>'
-        )
-        content, tool_calls = parser.parse(text)
-        assert tool_calls is not None
-        assert len(tool_calls) == 2, f"Expected 2 tool calls, got {len(tool_calls)}"
-        names = [tc.function.name for tc in tool_calls]
-        assert "get_weather" in names
-        assert "get_time" in names
-
-    def test_tool_call_with_preceding_text(self, parser):
-        text = (
-            'Let me check that for you.\n'
-            '<｜tool▁calls▁begin｜><｜tool▁call▁begin｜>function<｜tool▁sep｜>terminal\n'
-            '```json\n{"command": "ls"}\n```<｜tool▁call▁end｜><｜tool▁calls▁end｜>'
-        )
-        content, tool_calls = parser.parse(text)
-        assert tool_calls is not None
-        assert len(tool_calls) == 1
-
-
-# ─── Mistral parser tests ───────────────────────────────────────────────
-
-class TestMistralParser:
-    @pytest.fixture
-    def parser(self):
-        return get_parser("mistral")
-
-    def test_no_tool_call(self, parser):
-        text = "Hello, how can I help you?"
-        content, tool_calls = parser.parse(text)
-        assert content == text
-        assert tool_calls is None
-
-    def test_pre_v11_single_tool_call(self, parser):
-        text = '[TOOL_CALLS] [{"name": "func", "arguments": {"key": "val"}}]'
-        content, tool_calls = parser.parse(text)
-        assert tool_calls is not None
-        assert len(tool_calls) == 1
-        assert tool_calls[0].function.name == "func"
-        args = json.loads(tool_calls[0].function.arguments)
-        assert args["key"] == "val"
-
-    def test_pre_v11_nested_json(self, parser):
-        text = '[TOOL_CALLS] [{"name": "func", "arguments": {"nested": {"deep": true}}}]'
-        content, tool_calls = parser.parse(text)
-        assert tool_calls is not None
-        assert len(tool_calls) == 1
-        assert tool_calls[0].function.name == "func"
-        args = json.loads(tool_calls[0].function.arguments)
-        assert args["nested"]["deep"] is True
-
-    def test_v11_single_tool_call(self, parser):
-        text = '[TOOL_CALLS]get_weather{"city": "London"}'
-        content, tool_calls = parser.parse(text)
-        assert tool_calls is not None
-        assert len(tool_calls) == 1
-        assert tool_calls[0].function.name == "get_weather"
-        args = json.loads(tool_calls[0].function.arguments)
-        assert args["city"] == "London"
-
-    def test_v11_multiple_tool_calls(self, parser):
-        text = '[TOOL_CALLS]func1{"a": 1}[TOOL_CALLS]func2{"b": 2}'
-        content, tool_calls = parser.parse(text)
-        assert tool_calls is not None
-        assert len(tool_calls) == 2
-        names = [tc.function.name for tc in tool_calls]
-        assert "func1" in names
-        assert "func2" in names
-
-    def test_preceding_text_preserved(self, parser):
-        text = 'Hello[TOOL_CALLS]func{"a": 1}'
-        content, tool_calls = parser.parse(text)
-        assert content == "Hello"
-        assert tool_calls is not None
-        assert len(tool_calls) == 1
-        assert tool_calls[0].function.name == "func"
-
-    def test_malformed_json_fallback(self, parser):
-        text = "[TOOL_CALLS] not valid json"
-        content, tool_calls = parser.parse(text)
-        assert tool_calls is None

tests/tools/test_transcription.py

@@ -8,11 +8,16 @@ import json
 import os
 import tempfile
 from pathlib import Path
+from types import SimpleNamespace
 from unittest.mock import MagicMock, patch, mock_open
 
 import pytest
 
 
+def _fake_faster_whisper_module(mock_model):
+    return SimpleNamespace(WhisperModel=MagicMock(return_value=mock_model))
+
+
 # ---------------------------------------------------------------------------
 # Provider selection
 # ---------------------------------------------------------------------------
@@ -137,8 +142,9 @@ class TestTranscribeLocal:
         mock_model = MagicMock()
         mock_model.transcribe.return_value = ([mock_segment], mock_info)
 
+        fake_fw = _fake_faster_whisper_module(mock_model)
         with patch("tools.transcription_tools._HAS_FASTER_WHISPER", True), \
-             patch("faster_whisper.WhisperModel", return_value=mock_model), \
+             patch.dict("sys.modules", {"faster_whisper": fake_fw}), \
              patch("tools.transcription_tools._local_model", None):
             from tools.transcription_tools import _transcribe_local
             result = _transcribe_local(str(audio_file), "base")
@@ -300,7 +306,8 @@ class TestNormalizeLocalModel:
                  }), \
                  patch("tools.transcription_tools._local_model", None), \
                  patch("tools.transcription_tools._local_model_name", None), \
-                 patch("faster_whisper.WhisperModel", return_value=mock_model) as mock_cls:
+                 patch.dict("sys.modules", {"faster_whisper": _fake_faster_whisper_module(mock_model)}):
+                mock_cls = __import__("faster_whisper").WhisperModel
                 from tools.transcription_tools import transcribe_audio
                 transcribe_audio(audio_file)
                 # WhisperModel must NOT have been called with "whisper-1"

tests/tools/test_transcription_dotenv_fallback.py

@@ -181,7 +181,15 @@ class TestTranscribeCallSitesReadDotenv:
         assert seen_keys == ["mistral-dotenv-key"]
 
     def test_transcribe_xai_forwards_dotenv_key(self):
+        """xAI STT now resolves credentials through ``tools.xai_http`` so the
+        OAuth bearer wins when present and ``XAI_API_KEY`` is the fallback.
+        Patch the resolver's ``get_env_value`` to simulate a dotenv-only key
+        and confirm it reaches the HTTP call. The per-call-site
+        ``transcription_tools.get_env_value`` is still consulted for the
+        ``XAI_STT_BASE_URL`` override (covered by ``test_custom_base_url``).
+        """
         from tools import transcription_tools as tt
+        from tools import xai_http
 
         captured: dict = {}
 
@@ -194,15 +202,12 @@ class TestTranscribeCallSitesReadDotenv:
             response.json.return_value = {"text": "hello"}
             return response
 
-        # get_env_value is consulted for both XAI_API_KEY and XAI_STT_BASE_URL.
-        # Return the key for the first call, None for base-url override
-        # (so it defaults to the module-level XAI_STT_BASE_URL).
         def fake_get_env_value(name, default=None):
             if name == "XAI_API_KEY":
                 return "xai-dotenv-key"
             return None
 
-        with patch.object(tt, "get_env_value", side_effect=fake_get_env_value), \
+        with patch.object(xai_http, "get_env_value", side_effect=fake_get_env_value), \
              patch("requests.post", side_effect=fake_post), \
              patch("builtins.open", MagicMock()):
             result = tt._transcribe_xai("/tmp/fake.mp3", "grok-stt")

tests/tools/test_transcription_tools.py

@@ -1522,3 +1522,44 @@ class TestTranscribeAudioElevenLabsDispatch:
             transcribe_audio(sample_ogg, model="scribe_v2")
 
         assert mock_elevenlabs.call_args[0][1] == "scribe_v2"
+
+
+# Shell safety — shlex.split on auto-detected templates
+# ============================================================================
+class TestShellSafety:
+    def test_auto_detected_template_is_shlex_safe(self, monkeypatch):
+        """Auto-detected whisper command should be safely splittable."""
+        import shlex
+        monkeypatch.delenv("HERMES_LOCAL_STT_COMMAND", raising=False)
+        monkeypatch.setattr(
+            "tools.transcription_tools._find_whisper_binary",
+            lambda: "/usr/bin/whisper",
+        )
+        from tools.transcription_tools import _get_local_command_template
+        template = _get_local_command_template()
+        assert template is not None
+        cmd = template.format(
+            input_path=shlex.quote("/tmp/test.wav"),
+            output_dir=shlex.quote("/tmp/out"),
+            language=shlex.quote("en"),
+            model=shlex.quote("base"),
+        )
+        parts = shlex.split(cmd)
+        assert parts[0] == "/usr/bin/whisper"
+        assert "/tmp/test.wav" in parts
+
+    def test_env_var_template_uses_shell_path(self, monkeypatch):
+        """When HERMES_LOCAL_STT_COMMAND is set, use_shell should be True."""
+        import os
+        from tools.transcription_tools import LOCAL_STT_COMMAND_ENV
+        monkeypatch.setenv(LOCAL_STT_COMMAND_ENV, "whisper {input_path} | tee log.txt")
+        use_shell = bool(os.getenv(LOCAL_STT_COMMAND_ENV, "").strip())
+        assert use_shell is True
+
+    def test_no_env_var_uses_list_mode(self, monkeypatch):
+        """When no env var is set, use_shell should be False."""
+        import os
+        from tools.transcription_tools import LOCAL_STT_COMMAND_ENV
+        monkeypatch.delenv(LOCAL_STT_COMMAND_ENV, raising=False)
+        use_shell = bool(os.getenv(LOCAL_STT_COMMAND_ENV, "").strip())
+        assert use_shell is False

tests/tools/test_tts_dotenv_fallback.py

@@ -57,7 +57,12 @@ class TestDotenvFallbackPerProvider:
             mock_import.return_value.assert_called_once_with(api_key="el-dotenv-key")
 
     def test_xai_reads_dotenv_key(self, tmp_path):
+        """xAI TTS now resolves credentials through ``tools.xai_http``; the
+        dotenv fallback contract from #17140 is preserved by patching the
+        resolver's ``get_env_value`` rather than ``tts_tool.get_env_value``.
+        """
         from tools import tts_tool
+        from tools import xai_http
 
         captured: dict = {}
 
@@ -69,7 +74,7 @@ class TestDotenvFallbackPerProvider:
             response.raise_for_status = MagicMock()
             return response
 
-        with patch.object(tts_tool, "get_env_value", return_value="xai-dotenv-key"), \
+        with patch.object(xai_http, "get_env_value", return_value="xai-dotenv-key"), \
              patch("requests.post", side_effect=fake_post):
             tts_tool._generate_xai_tts("hi", str(tmp_path / "out.mp3"), {})
 

tests/tools/test_tts_kittentts.py

@@ -3,7 +3,6 @@
 import json
 from unittest.mock import MagicMock, patch
 
-import numpy as np
 import pytest
 
 
@@ -27,7 +26,7 @@ def mock_kittentts_module():
     """Inject a fake kittentts + soundfile module that return stub objects."""
     fake_model = MagicMock()
     # 24kHz float32 PCM at ~2s of silence
-    fake_model.generate.return_value = np.zeros(48000, dtype=np.float32)
+    fake_model.generate.return_value = [0.0] * 48000
     fake_cls = MagicMock(return_value=fake_model)
     fake_kittentts = MagicMock()
     fake_kittentts.KittenTTS = fake_cls

tests/tools/test_tts_speed.py

@@ -8,7 +8,12 @@ import pytest
 
 @pytest.fixture(autouse=True)
 def clean_env(monkeypatch):
-    for key in ("OPENAI_API_KEY", "MINIMAX_API_KEY", "HERMES_SESSION_PLATFORM"):
+    for key in (
+        "OPENAI_API_KEY",
+        "MINIMAX_API_KEY",
+        "MINIMAX_GROUP_ID",
+        "HERMES_SESSION_PLATFORM",
+    ):
         monkeypatch.delenv(key, raising=False)
 
 
@@ -110,37 +115,126 @@ class TestOpenaiTtsSpeed:
 
 
 # ---------------------------------------------------------------------------
-# MiniMax TTS (new API: raw audio, no speed/voice_setting)
+# MiniMax TTS (t2a_v2 endpoint: nested voice_setting/audio_setting,
+# JSON response with hex-encoded audio.  Falls back to the legacy
+# text_to_speech endpoint shape when the base_url points at it.)
 # ---------------------------------------------------------------------------
 
-class TestMinimaxTtsSpeed:
-    def _run(self, tts_config, tmp_path, monkeypatch):
-        monkeypatch.setenv("MINIMAX_API_KEY", "test-key")
-        mock_response = MagicMock()
-        mock_response.status_code = 200
-        mock_response.headers = {"Content-Type": "audio/mpeg"}
-        mock_response.content = b"\x00\x01\x02\x03"
 
-        # requests is imported locally inside _generate_minimax_tts
-        with patch("requests.post", return_value=mock_response) as mock_post:
+def _hex_response(payload_audio: bytes = b"\x00\x01\x02\x03"):
+    """Build a mock response shaped like a successful t2a_v2 reply."""
+    mock_response = MagicMock()
+    mock_response.status_code = 200
+    mock_response.headers = {"Content-Type": "application/json"}
+    mock_response.json.return_value = {
+        "data": {"audio": payload_audio.hex(), "status": 2},
+        "base_resp": {"status_code": 0, "status_msg": "success"},
+    }
+    return mock_response
+
+
+class TestMinimaxTtsT2aV2:
+    """Default path: base_url contains 't2a_v2'."""
+
+    def _run(self, tts_config, tmp_path, monkeypatch, response=None):
+        monkeypatch.setenv("MINIMAX_API_KEY", "test-key")
+        resp = response if response is not None else _hex_response()
+        with patch("requests.post", return_value=resp) as mock_post:
             from tools.tts_tool import _generate_minimax_tts
             output = _generate_minimax_tts("Hello", str(tmp_path / "out.mp3"), tts_config)
         return mock_post, output
 
-    def test_simple_payload(self, tmp_path, monkeypatch):
-        """New API uses flat payload with model, text, voice_id."""
+    def test_nested_payload(self, tmp_path, monkeypatch):
+        """Default endpoint uses nested voice_setting / audio_setting."""
+        mock_post, _ = self._run({}, tmp_path, monkeypatch)
+        payload = mock_post.call_args[1]["json"]
+        assert payload["model"] == "speech-02-hd"
+        assert payload["text"] == "Hello"
+        assert "voice_setting" in payload
+        assert payload["voice_setting"]["voice_id"] == "English_expressive_narrator"
+        assert "audio_setting" in payload
+        assert payload["audio_setting"]["format"] == "mp3"
+        # Don't send flat top-level voice_id alongside nested voice_setting.
+        assert "voice_id" not in payload
+
+    def test_decodes_hex_audio(self, tmp_path, monkeypatch):
+        """t2a_v2 hex-encoded audio is decoded and written verbatim."""
+        _, output = self._run({}, tmp_path, monkeypatch)
+        with open(output, "rb") as f:
+            assert f.read() == b"\x00\x01\x02\x03"
+
+    def test_default_url_is_t2a_v2(self, tmp_path, monkeypatch):
+        """Default base URL points at the live t2a_v2 endpoint."""
+        mock_post, _ = self._run({}, tmp_path, monkeypatch)
+        url = mock_post.call_args[0][0]
+        assert "t2a_v2" in url
+        assert "api.minimax.io" in url
+
+    def test_group_id_from_config(self, tmp_path, monkeypatch):
+        """group_id from config attaches as ?GroupId=<id>."""
+        mock_post, _ = self._run({"minimax": {"group_id": "G123"}}, tmp_path, monkeypatch)
+        url = mock_post.call_args[0][0]
+        assert "GroupId=G123" in url
+
+    def test_group_id_from_env(self, tmp_path, monkeypatch):
+        """MINIMAX_GROUP_ID env var attaches as ?GroupId=<id>."""
+        monkeypatch.setenv("MINIMAX_GROUP_ID", "G456")
+        mock_post, _ = self._run({}, tmp_path, monkeypatch)
+        url = mock_post.call_args[0][0]
+        assert "GroupId=G456" in url
+
+    def test_group_id_already_in_url_left_alone(self, tmp_path, monkeypatch):
+        """If user already set GroupId in base_url, don't double-append it."""
+        cfg = {"minimax": {
+            "base_url": "https://api.minimax.io/v1/t2a_v2?GroupId=PRESET",
+            "group_id": "IGNORED",
+        }}
+        mock_post, _ = self._run(cfg, tmp_path, monkeypatch)
+        url = mock_post.call_args[0][0]
+        assert url.count("GroupId=") == 1
+        assert "GroupId=PRESET" in url
+
+    def test_api_error_raises(self, tmp_path, monkeypatch):
+        """Non-zero base_resp.status_code surfaces as RuntimeError."""
+        resp = MagicMock()
+        resp.status_code = 200
+        resp.headers = {"Content-Type": "application/json"}
+        resp.json.return_value = {
+            "data": {"audio": "", "status": 1},
+            "base_resp": {"status_code": 2013, "status_msg": "invalid voice"},
+        }
+        with pytest.raises(RuntimeError, match="2013"):
+            self._run({}, tmp_path, monkeypatch, response=resp)
+
+
+class TestMinimaxTtsLegacyTextToSpeech:
+    """Legacy path: caller pins base_url to the old text_to_speech endpoint."""
+
+    LEGACY_URL = "https://api.minimax.chat/v1/text_to_speech"
+
+    def _run(self, tts_config, tmp_path, monkeypatch):
+        monkeypatch.setenv("MINIMAX_API_KEY", "test-key")
+        cfg = dict(tts_config)
+        cfg.setdefault("minimax", {})["base_url"] = self.LEGACY_URL
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+        mock_response.headers = {"Content-Type": "audio/mpeg"}
+        mock_response.content = b"\x00\x01\x02\x03"
+        with patch("requests.post", return_value=mock_response) as mock_post:
+            from tools.tts_tool import _generate_minimax_tts
+            output = _generate_minimax_tts("Hello", str(tmp_path / "out.mp3"), cfg)
+        return mock_post, output
+
+    def test_flat_payload(self, tmp_path, monkeypatch):
+        """Legacy endpoint keeps the flat {model, text, voice_id} shape."""
         mock_post, _ = self._run({}, tmp_path, monkeypatch)
         payload = mock_post.call_args[1]["json"]
-        assert "model" in payload
-        assert "text" in payload
         assert "voice_id" in payload
         assert "voice_setting" not in payload
         assert "audio_setting" not in payload
-        assert "stream" not in payload
 
     def test_writes_raw_audio(self, tmp_path, monkeypatch):
-        """New API returns raw bytes written directly to file."""
+        """Legacy endpoint returns raw bytes written directly to file."""
         _, output = self._run({}, tmp_path, monkeypatch)
-        assert output == str(tmp_path / "out.mp3")
         with open(output, "rb") as f:
             assert f.read() == b"\x00\x01\x02\x03"

tests/tools/test_url_safety.py

@@ -22,6 +22,14 @@ class TestIsSafeUrl:
         ]):
             assert is_safe_url("https://example.com/image.png") is True
 
+    def test_ftp_scheme_blocked(self):
+        """Only http/https should be allowed for fetch tools."""
+        assert is_safe_url("ftp://example.com/file.txt") is False
+
+    def test_missing_scheme_blocked(self):
+        """Bare host/path should be rejected to avoid ambiguous handling."""
+        assert is_safe_url("example.com/path") is False
+
     def test_localhost_blocked(self):
         with patch("socket.getaddrinfo", return_value=[
             (2, 1, 6, "", ("127.0.0.1", 0)),

tests/tools/test_video_generation_dispatch.py

@@ -0,0 +1,126 @@
+"""Tests for the unified ``video_generate`` tool dispatch surface."""
+
+from __future__ import annotations
+
+import json
+from typing import Any, Dict, List, Optional
+
+import pytest
+
+from agent import video_gen_registry
+from agent.video_gen_provider import VideoGenProvider
+
+
+@pytest.fixture(autouse=True)
+def _reset_registry():
+    video_gen_registry._reset_for_tests()
+    yield
+    video_gen_registry._reset_for_tests()
+
+
+class _RecordingProvider(VideoGenProvider):
+    """Captures the kwargs the tool layer hands it."""
+
+    def __init__(self, name: str = "fake"):
+        self._name = name
+        self.last_kwargs: Dict[str, Any] = {}
+
+    @property
+    def name(self) -> str:
+        return self._name
+
+    def list_models(self) -> List[Dict[str, Any]]:
+        return [{"id": "model-a"}]
+
+    def default_model(self) -> Optional[str]:
+        return "model-a"
+
+    def generate(self, prompt, **kwargs):
+        self.last_kwargs = {"prompt": prompt, **kwargs}
+        modality = "image" if kwargs.get("image_url") else "text"
+        return {
+            "success": True,
+            "video": "https://example.com/v.mp4",
+            "model": kwargs.get("model") or "model-a",
+            "prompt": prompt,
+            "modality": modality,
+            "aspect_ratio": kwargs.get("aspect_ratio", ""),
+            "duration": kwargs.get("duration") or 0,
+            "provider": self._name,
+        }
+
+
+class _RaisingProvider(VideoGenProvider):
+    @property
+    def name(self) -> str:
+        return "raises"
+
+    def generate(self, prompt, **kwargs):
+        raise RuntimeError("boom")
+
+
+class TestUnifiedDispatch:
+    def _run(self, args: Dict[str, Any], *, configured: Optional[str] = None) -> Dict[str, Any]:
+        from tools import video_generation_tool
+        import hermes_cli.plugins as plugins_module
+
+        saved = video_generation_tool._read_configured_video_provider
+        video_generation_tool._read_configured_video_provider = lambda: configured  # type: ignore
+        saved_discover = plugins_module._ensure_plugins_discovered
+        plugins_module._ensure_plugins_discovered = lambda *_a, **_k: None  # type: ignore
+        try:
+            raw = video_generation_tool._handle_video_generate(args)
+        finally:
+            video_generation_tool._read_configured_video_provider = saved  # type: ignore
+            plugins_module._ensure_plugins_discovered = saved_discover  # type: ignore
+        return json.loads(raw)
+
+    def test_no_provider_returns_clear_error(self):
+        result = self._run({"prompt": "a dog"})
+        assert result["success"] is False
+        assert result["error_type"] == "no_provider_configured"
+
+    def test_unknown_provider_returns_clear_error(self):
+        result = self._run({"prompt": "a dog"}, configured="ghost")
+        assert result["success"] is False
+        assert result["error_type"] == "provider_not_registered"
+
+    def test_text_to_video_routes_without_image_url(self):
+        provider = _RecordingProvider("rec")
+        video_gen_registry.register_provider(provider)
+        result = self._run({"prompt": "a happy dog"})
+        assert result["success"] is True
+        assert result["modality"] == "text"
+        assert "image_url" not in provider.last_kwargs
+        assert provider.last_kwargs["aspect_ratio"] == "16:9"
+        assert provider.last_kwargs["resolution"] == "720p"
+
+    def test_image_to_video_routes_with_image_url(self):
+        provider = _RecordingProvider("rec")
+        video_gen_registry.register_provider(provider)
+        result = self._run({
+            "prompt": "animate this",
+            "image_url": "https://example.com/img.png",
+        })
+        assert result["success"] is True
+        assert result["modality"] == "image"
+        assert provider.last_kwargs["image_url"] == "https://example.com/img.png"
+
+    def test_prompt_required(self):
+        provider = _RecordingProvider("rec")
+        video_gen_registry.register_provider(provider)
+        result = self._run({"prompt": "", "image_url": "https://example.com/i.png"})
+        assert "error" in result
+        assert "prompt" in result["error"].lower()
+
+    def test_provider_exception_caught(self):
+        video_gen_registry.register_provider(_RaisingProvider())
+        result = self._run({"prompt": "x"})
+        assert result["success"] is False
+        assert result["error_type"] == "provider_exception"
+
+    def test_operation_field_not_in_schema(self):
+        """Make sure we removed the operation field from the schema."""
+        from tools.video_generation_tool import VIDEO_GENERATE_SCHEMA
+        assert "operation" not in VIDEO_GENERATE_SCHEMA["parameters"]["properties"]
+        assert "video_url" not in VIDEO_GENERATE_SCHEMA["parameters"]["properties"]

tests/tools/test_video_generation_dynamic_schema.py

@@ -0,0 +1,153 @@
+"""Tests for the dynamic schema builder under the simplified surface."""
+
+from __future__ import annotations
+
+from typing import Any, Dict, List, Optional
+
+import pytest
+import yaml
+
+from agent import video_gen_registry
+from agent.video_gen_provider import VideoGenProvider
+
+
+@pytest.fixture(autouse=True)
+def _reset_registry():
+    video_gen_registry._reset_for_tests()
+    yield
+    video_gen_registry._reset_for_tests()
+
+
+@pytest.fixture
+def cfg_home(tmp_path, monkeypatch):
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    return tmp_path
+
+
+def _write_cfg(home, cfg: dict):
+    (home / "config.yaml").write_text(yaml.safe_dump(cfg))
+
+
+class _BothModalitiesProvider(VideoGenProvider):
+    """Supports both text-to-video AND image-to-video (the common case)."""
+
+    @property
+    def name(self) -> str:
+        return "both"
+
+    def is_available(self) -> bool:
+        return True
+
+    def list_models(self) -> List[Dict[str, Any]]:
+        return [{"id": "family-a", "modalities": ["text", "image"]}]
+
+    def default_model(self) -> Optional[str]:
+        return "family-a"
+
+    def capabilities(self) -> Dict[str, Any]:
+        return {
+            "modalities": ["text", "image"],
+            "aspect_ratios": ["16:9", "9:16"],
+            "resolutions": ["720p", "1080p"],
+            "min_duration": 1,
+            "max_duration": 15,
+            "supports_audio": True,
+            "supports_negative_prompt": True,
+            "max_reference_images": 0,
+        }
+
+    def generate(self, prompt, **kwargs):
+        return {"success": True}
+
+
+class _ImageOnlyProvider(VideoGenProvider):
+    """Backend with only image-to-video support (rare but possible)."""
+
+    @property
+    def name(self) -> str:
+        return "img-only"
+
+    def is_available(self) -> bool:
+        return True
+
+    def list_models(self) -> List[Dict[str, Any]]:
+        return [{"id": "img-only-v1", "modalities": ["image"]}]
+
+    def default_model(self) -> Optional[str]:
+        return "img-only-v1"
+
+    def capabilities(self) -> Dict[str, Any]:
+        return {"modalities": ["image"], "min_duration": 1, "max_duration": 10}
+
+    def generate(self, prompt, **kwargs):
+        return {"success": True}
+
+
+class TestDynamicSchemaBuilder:
+    def test_no_config_says_so(self, cfg_home):
+        from tools.video_generation_tool import _build_dynamic_video_schema
+
+        desc = _build_dynamic_video_schema()["description"]
+        assert "No video backend is configured" in desc
+        assert "hermes tools" in desc
+
+    def test_does_not_mention_edit_or_extend(self, cfg_home):
+        """The simplified surface only does text→video and image→video.
+        The description must not mention edit/extend anywhere."""
+        from tools.video_generation_tool import _build_dynamic_video_schema, _GENERIC_DESCRIPTION
+
+        desc = _build_dynamic_video_schema()["description"]
+        # Block words that would suggest functionality we removed
+        assert "edit" not in desc.lower() or "audio" in desc.lower()  # 'audio' contains 'audi' not 'edit'
+        # Stronger: no occurrence of the words "edit" or "extend" as standalone
+        for forbidden in (" edit ", " edits ", " extend ", " extends "):
+            assert forbidden not in desc.lower(), f"description leaks '{forbidden.strip()}'"
+        # Sanity: the generic blurb itself is also clean
+        for forbidden in ("edit", "extend"):
+            assert forbidden not in _GENERIC_DESCRIPTION.lower()
+
+    def test_both_modalities_advertises_auto_routing(self, cfg_home):
+        from tools.video_generation_tool import _build_dynamic_video_schema
+
+        _write_cfg(cfg_home, {"video_gen": {"provider": "both"}})
+        video_gen_registry.register_provider(_BothModalitiesProvider())
+
+        import hermes_cli.plugins as plugins_module
+        saved = plugins_module._ensure_plugins_discovered
+        plugins_module._ensure_plugins_discovered = lambda *a, **k: None
+        try:
+            desc = _build_dynamic_video_schema()["description"]
+        finally:
+            plugins_module._ensure_plugins_discovered = saved
+
+        assert "Active backend: Both" in desc
+        assert "text-to-video" in desc and "image-to-video" in desc
+        assert "routes automatically" in desc
+        # operations bullet is gone
+        assert "operations supported" not in desc
+
+    def test_image_only_model_warns_about_required_image_url(self, cfg_home):
+        from tools.video_generation_tool import _build_dynamic_video_schema
+
+        _write_cfg(cfg_home, {"video_gen": {"provider": "img-only"}})
+        video_gen_registry.register_provider(_ImageOnlyProvider())
+
+        import hermes_cli.plugins as plugins_module
+        saved = plugins_module._ensure_plugins_discovered
+        plugins_module._ensure_plugins_discovered = lambda *a, **k: None
+        try:
+            desc = _build_dynamic_video_schema()["description"]
+        finally:
+            plugins_module._ensure_plugins_discovered = saved
+
+        assert "image-to-video only" in desc
+        assert "image_url is REQUIRED" in desc
+
+    def test_builder_wired_into_registry(self):
+        from tools.registry import discover_builtin_tools, registry
+
+        discover_builtin_tools()
+        entry = registry._tools["video_generate"]
+        assert entry.dynamic_schema_overrides is not None
+        out = entry.dynamic_schema_overrides()
+        assert "description" in out

tests/tools/test_video_generation_tool_surface_matrix.py

@@ -0,0 +1,253 @@
+"""Tool-surface routing matrix: every (provider, model, modality) combo.
+
+This is the integration test for the question Teknium asked: regardless
+of which provider+model the user picks and whether they pass an
+image_url or not, does the tool surface route correctly to the right
+endpoint with the right payload shape?
+
+Drives ``_handle_video_generate(args)`` end-to-end — config write →
+config read → registry lookup → provider.generate() → outbound HTTP/SDK
+call. Stubs fal_client and httpx so we observe routing without hitting
+the network.
+"""
+
+from __future__ import annotations
+
+import asyncio
+import json
+import types
+from typing import Any, Dict, List, Optional
+
+import pytest
+import yaml
+
+
+@pytest.fixture(autouse=True)
+def _reset_registry():
+    from agent import video_gen_registry
+    video_gen_registry._reset_for_tests()
+    yield
+    video_gen_registry._reset_for_tests()
+
+
+@pytest.fixture
+def matrix_env(tmp_path, monkeypatch):
+    """Set up HERMES_HOME, stub fal_client + httpx, force plugin discovery."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    monkeypatch.setenv("FAL_KEY", "test-key")
+    monkeypatch.setenv("XAI_API_KEY", "test-key")
+
+    fal_calls: List[Dict[str, Any]] = []
+    xai_calls: List[Dict[str, Any]] = []
+
+    # fal_client stub
+    fake_fal = types.ModuleType("fal_client")
+    def _subscribe(endpoint, arguments=None, with_logs=False):
+        fal_calls.append({"endpoint": endpoint, "arguments": arguments})
+        return {"video": {"url": f"https://fake-fal/{endpoint.replace('/','_')}.mp4"}}
+    fake_fal.subscribe = _subscribe  # type: ignore
+    monkeypatch.setitem(__import__("sys").modules, "fal_client", fake_fal)
+
+    # httpx stub for xAI
+    import httpx
+    class _Resp:
+        def __init__(self, p, s=200):
+            self.status_code = s
+            self._p = p
+            self.text = json.dumps(p)
+        def raise_for_status(self):
+            if self.status_code >= 400:
+                raise httpx.HTTPStatusError("err", request=None, response=self)  # type: ignore
+        def json(self):
+            return self._p
+    class _Client:
+        async def __aenter__(self): return self
+        async def __aexit__(self, *a): return None
+        async def post(self, url, headers=None, json=None, timeout=None):
+            xai_calls.append({"url": url, "json": json})
+            return _Resp({"request_id": "req-1"})
+        async def get(self, url, headers=None, timeout=None):
+            return _Resp({
+                "status": "done",
+                "video": {"url": "https://xai-cdn/out.mp4", "duration": 8},
+                "model": "grok-imagine-video",
+            })
+    import plugins.video_gen.xai as xai_plugin
+    monkeypatch.setattr(xai_plugin.httpx, "AsyncClient", lambda: _Client())
+    async def _no_sleep(*a, **k): return None
+    monkeypatch.setattr(asyncio, "sleep", _no_sleep)
+
+    # Reset FAL plugin's lazy fal_client cache so it picks up the stub
+    from plugins.video_gen import fal as fal_plugin
+    fal_plugin._fal_client = None
+
+    # Force discovery
+    from hermes_cli.plugins import _ensure_plugins_discovered
+    _ensure_plugins_discovered(force=True)
+
+    return tmp_path, fal_calls, xai_calls
+
+
+def _invoke_tool(home, cfg: dict, args: dict) -> dict:
+    """Write config, invoke the registered tool handler, return parsed JSON."""
+    (home / "config.yaml").write_text(yaml.safe_dump(cfg))
+    import hermes_cli.config as cfg_mod
+    if hasattr(cfg_mod, "_invalidate_load_config_cache"):
+        cfg_mod._invalidate_load_config_cache()
+
+    from tools.registry import registry
+    handler = registry._tools["video_generate"].handler
+    return json.loads(handler(args))
+
+
+# ─────────────────────────────────────────────────────────────────────────
+# FAL: every family × {text-only, text+image}
+# ─────────────────────────────────────────────────────────────────────────
+
+# We parametrize over the catalog so the test discovers new families
+# automatically. If someone adds 'sora-2' to FAL_FAMILIES, this matrix
+# picks it up — no test changes needed beyond confirming the endpoints.
+def _all_fal_families():
+    from plugins.video_gen.fal import FAL_FAMILIES
+    return list(FAL_FAMILIES.keys())
+
+
+@pytest.mark.parametrize("family_id", _all_fal_families())
+def test_fal_text_only_routes_to_text_endpoint(matrix_env, family_id):
+    home, fal_calls, _ = matrix_env
+    from plugins.video_gen.fal import FAL_FAMILIES
+
+    result = _invoke_tool(
+        home,
+        {"video_gen": {"provider": "fal", "model": family_id}},
+        {"prompt": "a dog running"},
+    )
+
+    assert result["success"] is True, f"{family_id}: {result.get('error')}"
+    assert result["modality"] == "text"
+    assert result["provider"] == "fal"
+
+    # Outbound endpoint must be the family's text endpoint
+    assert len(fal_calls) == 1
+    endpoint = fal_calls[0]["endpoint"]
+    assert endpoint == FAL_FAMILIES[family_id]["text_endpoint"]
+
+    # Payload must NOT contain any image-shaped key
+    payload = fal_calls[0]["arguments"] or {}
+    image_keys = [k for k in payload if "image" in k and "url" in k]
+    assert not image_keys, f"{family_id} text-only leaked image keys: {image_keys}"
+
+
+@pytest.mark.parametrize("family_id", _all_fal_families())
+def test_fal_text_plus_image_routes_to_image_endpoint(matrix_env, family_id):
+    home, fal_calls, _ = matrix_env
+    from plugins.video_gen.fal import FAL_FAMILIES
+
+    result = _invoke_tool(
+        home,
+        {"video_gen": {"provider": "fal", "model": family_id}},
+        {"prompt": "animate this dog", "image_url": "https://example.com/dog.png"},
+    )
+
+    assert result["success"] is True, f"{family_id}: {result.get('error')}"
+    assert result["modality"] == "image"
+    assert result["provider"] == "fal"
+
+    # Outbound endpoint must be the family's image endpoint
+    assert len(fal_calls) == 1
+    endpoint = fal_calls[0]["endpoint"]
+    assert endpoint == FAL_FAMILIES[family_id]["image_endpoint"]
+
+    # Payload must contain the right image key (may be image_url or
+    # start_image_url depending on the family's image_param_key)
+    payload = fal_calls[0]["arguments"] or {}
+    expected_image_key = FAL_FAMILIES[family_id].get("image_param_key") or "image_url"
+    assert payload.get(expected_image_key) == "https://example.com/dog.png", (
+        f"{family_id} text+image missing {expected_image_key} in payload "
+        f"(keys: {sorted(payload.keys())})"
+    )
+
+
+# ─────────────────────────────────────────────────────────────────────────
+# xAI: text-only / text+image both go to /videos/generations
+# (xAI uses one endpoint with an optional 'image' field, not separate URLs)
+# ─────────────────────────────────────────────────────────────────────────
+
+def test_xai_text_only_via_tool_surface(matrix_env):
+    home, _, xai_calls = matrix_env
+
+    result = _invoke_tool(
+        home,
+        {"video_gen": {"provider": "xai"}},
+        {"prompt": "a dog running"},
+    )
+    assert result["success"] is True
+    assert result["modality"] == "text"
+    assert result["provider"] == "xai"
+
+    assert len(xai_calls) == 1
+    assert xai_calls[0]["url"].endswith("/videos/generations")
+    payload = xai_calls[0]["json"] or {}
+    assert "image" not in payload
+    assert "reference_images" not in payload
+
+
+def test_xai_text_plus_image_via_tool_surface(matrix_env):
+    home, _, xai_calls = matrix_env
+
+    result = _invoke_tool(
+        home,
+        {"video_gen": {"provider": "xai"}},
+        {"prompt": "animate this", "image_url": "https://example.com/img.png"},
+    )
+    assert result["success"] is True
+    assert result["modality"] == "image"
+    assert result["provider"] == "xai"
+
+    assert len(xai_calls) == 1
+    assert xai_calls[0]["url"].endswith("/videos/generations")
+    payload = xai_calls[0]["json"] or {}
+    assert payload["image"] == {"url": "https://example.com/img.png"}
+
+
+# ─────────────────────────────────────────────────────────────────────────
+# tool-level `model` arg overrides config
+# ─────────────────────────────────────────────────────────────────────────
+
+def test_tool_model_arg_overrides_config(matrix_env):
+    """When the tool call passes model=, it wins over video_gen.model in config."""
+    home, fal_calls, _ = matrix_env
+
+    # Config picks pixverse-v6, but tool call says veo3.1
+    result = _invoke_tool(
+        home,
+        {"video_gen": {"provider": "fal", "model": "pixverse-v6"}},
+        {"prompt": "a dog", "model": "veo3.1"},
+    )
+
+    assert result["success"] is True
+    assert result["model"] == "veo3.1"
+    # Outbound endpoint reflects the override, not config
+    assert fal_calls[0]["endpoint"] == "fal-ai/veo3.1"
+
+
+def test_tool_model_arg_with_image_url_routes_to_override_image_endpoint(matrix_env):
+    """model= override on text+image goes to the override family's image endpoint."""
+    home, fal_calls, _ = matrix_env
+
+    result = _invoke_tool(
+        home,
+        {"video_gen": {"provider": "fal", "model": "pixverse-v6"}},
+        {
+            "prompt": "animate this",
+            "image_url": "https://example.com/i.png",
+            "model": "kling-v3-4k",
+        },
+    )
+
+    assert result["success"] is True
+    assert result["model"] == "kling-v3-4k"
+    assert fal_calls[0]["endpoint"] == "fal-ai/kling-video/v3/4k/image-to-video"
+    # Kling 4K uses start_image_url
+    assert fal_calls[0]["arguments"].get("start_image_url") == "https://example.com/i.png"
+    assert "image_url" not in fal_calls[0]["arguments"]

tests/tools/test_web_providers.py

@@ -20,50 +20,121 @@ import pytest
 
 
 class TestWebProviderABCs:
-    """The ABCs enforce the interface contract."""
+    """The unified WebSearchProvider ABC enforces the interface contract.
 
-    def test_cannot_instantiate_search_provider(self):
-        from tools.web_providers.base import WebSearchProvider
+    After PR #25182, all seven providers are subclasses of
+    :class:`agent.web_search_provider.WebSearchProvider`. The legacy
+    in-tree ABCs at ``tools.web_providers.base`` (separate
+    ``WebSearchProvider`` + ``WebExtractProvider``) were deleted in the
+    same PR — providers now advertise capabilities via
+    ``supports_search() / supports_extract() / supports_crawl()`` flags.
+    """
+
+    def test_cannot_instantiate_abc_directly(self):
+        from agent.web_search_provider import WebSearchProvider
 
         with pytest.raises(TypeError):
             WebSearchProvider()  # type: ignore[abstract]
 
-    def test_cannot_instantiate_extract_provider(self):
-        from tools.web_providers.base import WebExtractProvider
-
-        with pytest.raises(TypeError):
-            WebExtractProvider()  # type: ignore[abstract]
-
-    def test_concrete_search_provider_works(self):
-        from tools.web_providers.base import WebSearchProvider
+    def test_concrete_search_only_provider_works(self):
+        from agent.web_search_provider import WebSearchProvider
 
         class Dummy(WebSearchProvider):
-            def provider_name(self) -> str:
+            @property
+            def name(self) -> str:
                 return "dummy"
-            def is_configured(self) -> bool:
+
+            @property
+            def display_name(self) -> str:
+                return "Dummy Search"
+
+            def is_available(self) -> bool:
                 return True
+
+            def supports_search(self) -> bool:
+                return True
+
             def search(self, query: str, limit: int = 5) -> Dict[str, Any]:
                 return {"success": True, "data": {"web": []}}
 
         d = Dummy()
-        assert d.provider_name() == "dummy"
-        assert d.is_configured() is True
+        assert d.name == "dummy"
+        assert d.display_name == "Dummy Search"
+        assert d.is_available() is True
+        assert d.supports_search() is True
+        assert d.supports_extract() is False  # default
+        assert d.supports_crawl() is False  # default
         assert d.search("test")["success"] is True
 
-    def test_concrete_extract_provider_works(self):
-        from tools.web_providers.base import WebExtractProvider
+    def test_concrete_multi_capability_provider_works(self):
+        from agent.web_search_provider import WebSearchProvider
 
-        class Dummy(WebExtractProvider):
-            def provider_name(self) -> str:
+        class Dummy(WebSearchProvider):
+            @property
+            def name(self) -> str:
                 return "dummy"
-            def is_configured(self) -> bool:
+
+            @property
+            def display_name(self) -> str:
+                return "Dummy Multi"
+
+            def is_available(self) -> bool:
                 return True
-            def extract(self, urls: List[str], **kwargs) -> Dict[str, Any]:
-                return {"success": True, "data": [{"url": urls[0], "content": "x"}]}
+
+            def supports_search(self) -> bool:
+                return True
+
+            def supports_extract(self) -> bool:
+                return True
+
+            def supports_crawl(self) -> bool:
+                return True
+
+            def search(self, query: str, limit: int = 5) -> Dict[str, Any]:
+                return {"success": True, "data": {"web": []}}
+
+            def extract(self, urls: List[str], **kwargs: Any) -> List[Dict[str, Any]]:
+                return [{"url": urls[0], "content": "x"}]
+
+            def crawl(self, url: str, **kwargs: Any) -> Dict[str, Any]:
+                return {"results": [{"url": url, "content": "x"}]}
 
         d = Dummy()
-        assert d.provider_name() == "dummy"
-        assert d.extract(["https://example.com"])["success"] is True
+        assert d.supports_search() is True
+        assert d.supports_extract() is True
+        assert d.supports_crawl() is True
+        assert d.extract(["https://example.com"])[0]["url"] == "https://example.com"
+        assert d.crawl("https://example.com")["results"][0]["url"] == "https://example.com"
+
+    def test_search_only_provider_skips_extract_and_crawl(self):
+        """Search-only providers don't have to implement extract() / crawl()."""
+        from agent.web_search_provider import WebSearchProvider
+
+        class SearchOnly(WebSearchProvider):
+            @property
+            def name(self) -> str:
+                return "search-only"
+
+            @property
+            def display_name(self) -> str:
+                return "Search Only"
+
+            def is_available(self) -> bool:
+                return True
+
+            def supports_search(self) -> bool:
+                return True
+
+            def search(self, query: str, limit: int = 5) -> Dict[str, Any]:
+                return {"success": True, "data": {"web": []}}
+
+        # Should instantiate fine — extract/crawl have default
+        # supports_*() returning False and aren't required to be
+        # overridden when not advertised.
+        s = SearchOnly()
+        assert s.supports_search() is True
+        assert s.supports_extract() is False
+        assert s.supports_crawl() is False
 
 
 # ---------------------------------------------------------------------------
@@ -192,3 +263,72 @@ class TestWebSearchUsesSearchBackend:
 
         assert len(called_with) > 0
         assert called_with[0][0] == "search"
+
+
+class TestUnconfiguredErrorEnvelopeParity:
+    """Regression tests for PR #25182: the post-migration dispatcher must
+    emit the same top-level error envelope as pre-migration main when no
+    web backend is configured.
+
+    Plugin-level error wrapping is correct for in-flight errors (per-page
+    SDK exceptions, scrape timeouts) but PRE-FLIGHT configuration errors
+    must surface at the top level so function-calling models that check
+    ``result.get("error")`` detect the failure cleanly.
+    """
+
+    def _clear_web_creds(self, monkeypatch):
+        for k in (
+            "BRAVE_SEARCH_API_KEY",
+            "SEARXNG_URL",
+            "TAVILY_API_KEY",
+            "EXA_API_KEY",
+            "PARALLEL_API_KEY",
+            "FIRECRAWL_API_KEY",
+            "FIRECRAWL_API_URL",
+            "FIRECRAWL_GATEWAY_URL",
+            "TOOL_GATEWAY_DOMAIN",
+        ):
+            monkeypatch.delenv(k, raising=False)
+
+    def test_unconfigured_search_emits_top_level_error(self, monkeypatch):
+        """``web_search_tool`` with no creds returns ``{"error": "Error searching web: ..."}``
+        — matching main's ``tool_error()`` envelope, not a per-result shape.
+        """
+        import json
+        from tools import web_tools
+
+        self._clear_web_creds(monkeypatch)
+        # Reset firecrawl client cache so the unconfigured state is re-evaluated
+        monkeypatch.setattr(web_tools, "_firecrawl_client", None, raising=False)
+        monkeypatch.setattr(web_tools, "_firecrawl_client_config", None, raising=False)
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {})
+
+        result = json.loads(web_tools.web_search_tool("hello world", limit=3))
+        assert "error" in result, f"expected top-level 'error' key, got {result}"
+        # ``Error searching web:`` prefix comes from web_tools' top-level except handler
+        assert "Error searching web:" in result["error"]
+        assert "FIRECRAWL_API_KEY" in result["error"]
+        # No per-result burying
+        assert "results" not in result
+
+    def test_unconfigured_crawl_emits_top_level_error(self, monkeypatch):
+        """``web_crawl_tool`` with no creds returns ``{"success": False, "error": "web_crawl requires Firecrawl..."}``
+        — the dispatcher gates on ``provider.is_available()`` BEFORE
+        delegating to the plugin so pre-config errors don't get wrapped
+        into ``results[]``.
+        """
+        import asyncio
+        import json
+        from tools import web_tools
+
+        self._clear_web_creds(monkeypatch)
+        monkeypatch.setattr(web_tools, "_firecrawl_client", None, raising=False)
+        monkeypatch.setattr(web_tools, "_firecrawl_client_config", None, raising=False)
+        monkeypatch.setattr(web_tools, "_load_web_config", lambda: {})
+
+        result = json.loads(asyncio.run(web_tools.web_crawl_tool("https://example.com", use_llm_processing=False)))
+        assert result.get("success") is False
+        assert "error" in result, f"expected top-level 'error' key, got {result}"
+        assert "web_crawl requires Firecrawl" in result["error"]
+        # Crucially: no per-page burying
+        assert "results" not in result

tests/tools/test_web_providers_brave_free.py

@@ -1,8 +1,8 @@
 """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
+- BraveFreeWebSearchProvider.is_available() env var gating
+- BraveFreeWebSearchProvider.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
@@ -17,34 +17,34 @@ from unittest.mock import MagicMock, patch
 
 
 # ---------------------------------------------------------------------------
-# BraveFreeSearchProvider unit tests
+# BraveFreeWebSearchProvider 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
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
+        assert BraveFreeWebSearchProvider().is_available() 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
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
+        assert BraveFreeWebSearchProvider().is_available() 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
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
+        assert BraveFreeWebSearchProvider().is_available() is False
 
     def test_provider_name(self):
-        from tools.web_providers.brave_free import BraveFreeSearchProvider
-        assert BraveFreeSearchProvider().provider_name() == "brave-free"
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
+        assert BraveFreeWebSearchProvider().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)
+        from agent.web_search_provider import WebSearchProvider
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
+        assert issubclass(BraveFreeWebSearchProvider, WebSearchProvider)
 
 
 class TestBraveFreeProviderSearch:
@@ -68,10 +68,10 @@ class TestBraveFreeProviderSearch:
 
     def test_happy_path_normalizes_results(self, monkeypatch):
         monkeypatch.setenv("BRAVE_SEARCH_API_KEY", "BSAkey123")
-        from tools.web_providers.brave_free import BraveFreeSearchProvider
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
 
         with patch("httpx.get", return_value=self._mock_resp(self._SAMPLE_RESPONSE)):
-            result = BraveFreeSearchProvider().search("test query", limit=5)
+            result = BraveFreeWebSearchProvider().search("test query", limit=5)
 
         assert result["success"] is True
         web = result["data"]["web"]
@@ -82,7 +82,7 @@ class TestBraveFreeProviderSearch:
     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
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
 
         captured = {}
 
@@ -93,7 +93,7 @@ class TestBraveFreeProviderSearch:
             return self._mock_resp({"web": {"results": []}})
 
         with patch("httpx.get", side_effect=fake_get):
-            BraveFreeSearchProvider().search("q", limit=5)
+            BraveFreeWebSearchProvider().search("q", limit=5)
 
         assert captured["url"] == "https://api.search.brave.com/res/v1/web/search"
         assert captured["headers"].get("X-Subscription-Token") == "BSAkey123"
@@ -103,7 +103,7 @@ class TestBraveFreeProviderSearch:
     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
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
 
         captured = {}
 
@@ -112,26 +112,26 @@ class TestBraveFreeProviderSearch:
             return self._mock_resp({"web": {"results": []}})
 
         with patch("httpx.get", side_effect=fake_get):
-            BraveFreeSearchProvider().search("q", limit=100)
+            BraveFreeWebSearchProvider().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
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
 
         with patch("httpx.get", return_value=self._mock_resp(self._SAMPLE_RESPONSE)):
-            result = BraveFreeSearchProvider().search("q", limit=2)
+            result = BraveFreeWebSearchProvider().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
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
 
         with patch("httpx.get", return_value=self._mock_resp({"web": {"results": []}})):
-            result = BraveFreeSearchProvider().search("nothing", limit=5)
+            result = BraveFreeWebSearchProvider().search("nothing", limit=5)
 
         assert result["success"] is True
         assert result["data"]["web"] == []
@@ -139,10 +139,10 @@ class TestBraveFreeProviderSearch:
     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
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
 
         with patch("httpx.get", return_value=self._mock_resp({})):
-            result = BraveFreeSearchProvider().search("q", limit=5)
+            result = BraveFreeWebSearchProvider().search("q", limit=5)
 
         assert result["success"] is True
         assert result["data"]["web"] == []
@@ -150,14 +150,14 @@ class TestBraveFreeProviderSearch:
     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
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
 
         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)
+            result = BraveFreeWebSearchProvider().search("q", limit=5)
 
         assert result["success"] is False
         assert "429" in result["error"]
@@ -165,19 +165,19 @@ class TestBraveFreeProviderSearch:
     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
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
 
         with patch("httpx.get", side_effect=httpx.RequestError("boom")):
-            result = BraveFreeSearchProvider().search("q", limit=5)
+            result = BraveFreeWebSearchProvider().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
+        from plugins.web.brave_free.provider import BraveFreeWebSearchProvider
 
-        result = BraveFreeSearchProvider().search("q", limit=5)
+        result = BraveFreeWebSearchProvider().search("q", limit=5)
         assert result["success"] is False
         assert "BRAVE_SEARCH_API_KEY" in result["error"]
 

tests/tools/test_web_providers_ddgs.py

@@ -1,8 +1,8 @@
 """Tests for the DuckDuckGo (ddgs) web search provider.
 
 Covers:
-- DDGSSearchProvider.is_configured() — reflects package importability
-- DDGSSearchProvider.search() — happy path, missing package, runtime error
+- DDGSWebSearchProvider.is_available() — reflects package importability
+- DDGSWebSearchProvider.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
@@ -40,21 +40,21 @@ def _install_fake_ddgs(monkeypatch, *, text_results=None, text_raises=None):
 
 
 # ---------------------------------------------------------------------------
-# DDGSSearchProvider unit tests
+# DDGSWebSearchProvider 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
+        # Drop any cached ``plugins.web.ddgs.provider`` so is_configured re-imports ddgs fresh
+        monkeypatch.delitem(sys.modules, "plugins.web.ddgs.provider", raising=False)
+        from plugins.web.ddgs.provider import DDGSWebSearchProvider
+        assert DDGSWebSearchProvider().is_available() 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)
+        monkeypatch.delitem(sys.modules, "plugins.web.ddgs.provider", raising=False)
         # Block the import so ``import ddgs`` raises ImportError even if the package is actually installed
         import builtins
         orig_import = builtins.__import__
@@ -65,17 +65,17 @@ class TestDDGSProviderIsConfigured:
             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
+        from plugins.web.ddgs.provider import DDGSWebSearchProvider
+        assert DDGSWebSearchProvider().is_available() is False
 
     def test_provider_name(self):
-        from tools.web_providers.ddgs import DDGSSearchProvider
-        assert DDGSSearchProvider().provider_name() == "ddgs"
+        from plugins.web.ddgs.provider import DDGSWebSearchProvider
+        assert DDGSWebSearchProvider().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)
+        from agent.web_search_provider import WebSearchProvider
+        from plugins.web.ddgs.provider import DDGSWebSearchProvider
+        assert issubclass(DDGSWebSearchProvider, WebSearchProvider)
 
 
 class TestDDGSProviderSearch:
@@ -85,9 +85,9 @@ class TestDDGSProviderSearch:
             {"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
+        from plugins.web.ddgs.provider import DDGSWebSearchProvider
 
-        result = DDGSSearchProvider().search("q", limit=5)
+        result = DDGSWebSearchProvider().search("q", limit=5)
 
         assert result["success"] is True
         web = result["data"]["web"]
@@ -99,9 +99,9 @@ class TestDDGSProviderSearch:
         _install_fake_ddgs(monkeypatch, text_results=[
             {"title": "A", "url": "https://a.example.com", "body": "desc A"},
         ])
-        from tools.web_providers.ddgs import DDGSSearchProvider
+        from plugins.web.ddgs.provider import DDGSWebSearchProvider
 
-        result = DDGSSearchProvider().search("q", limit=5)
+        result = DDGSWebSearchProvider().search("q", limit=5)
 
         assert result["success"] is True
         assert result["data"]["web"][0]["url"] == "https://a.example.com"
@@ -111,16 +111,16 @@ class TestDDGSProviderSearch:
             {"title": f"R{i}", "href": f"https://r{i}.example.com", "body": ""}
             for i in range(10)
         ])
-        from tools.web_providers.ddgs import DDGSSearchProvider
+        from plugins.web.ddgs.provider import DDGSWebSearchProvider
 
-        result = DDGSSearchProvider().search("q", limit=3)
+        result = DDGSWebSearchProvider().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)
+        monkeypatch.delitem(sys.modules, "plugins.web.ddgs.provider", raising=False)
         import builtins
         orig_import = builtins.__import__
 
@@ -130,25 +130,25 @@ class TestDDGSProviderSearch:
             return orig_import(name, *args, **kwargs)
 
         monkeypatch.setattr(builtins, "__import__", blocked_import)
-        from tools.web_providers.ddgs import DDGSSearchProvider
+        from plugins.web.ddgs.provider import DDGSWebSearchProvider
 
-        result = DDGSSearchProvider().search("q", limit=5)
+        result = DDGSWebSearchProvider().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
+        from plugins.web.ddgs.provider import DDGSWebSearchProvider
 
-        result = DDGSSearchProvider().search("q", limit=5)
+        result = DDGSWebSearchProvider().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
+        from plugins.web.ddgs.provider import DDGSWebSearchProvider
 
-        result = DDGSSearchProvider().search("nothing", limit=5)
+        result = DDGSWebSearchProvider().search("nothing", limit=5)
         assert result["success"] is True
         assert result["data"]["web"] == []
 

tests/tools/test_web_providers_searxng.py

@@ -1,8 +1,8 @@
 """Tests for the SearXNG web search provider.
 
 Covers:
-- SearXNGSearchProvider.is_configured() env var gating
-- SearXNGSearchProvider.search() — happy path, HTTP error, request error, bad JSON
+- SearXNGWebSearchProvider.is_available() env var gating
+- SearXNGWebSearchProvider.search() — happy path, HTTP error, request error, bad JSON
 - Result normalization (title, url, description, position)
 - Score-based sorting and limit truncation
 - _is_backend_available("searxng") integration
@@ -19,38 +19,38 @@ import pytest
 
 
 # ---------------------------------------------------------------------------
-# SearXNGSearchProvider unit tests
+# SearXNGWebSearchProvider unit tests
 # ---------------------------------------------------------------------------
 
 
 class TestSearXNGSearchProviderIsConfigured:
     def test_configured_when_url_set(self, monkeypatch):
         monkeypatch.setenv("SEARXNG_URL", "http://localhost:8080")
-        from tools.web_providers.searxng import SearXNGSearchProvider
-        assert SearXNGSearchProvider().is_configured() is True
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
+        assert SearXNGWebSearchProvider().is_available() is True
 
     def test_not_configured_when_url_missing(self, monkeypatch):
         monkeypatch.delenv("SEARXNG_URL", raising=False)
-        from tools.web_providers.searxng import SearXNGSearchProvider
-        assert SearXNGSearchProvider().is_configured() is False
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
+        assert SearXNGWebSearchProvider().is_available() is False
 
     def test_not_configured_when_url_empty_string(self, monkeypatch):
         monkeypatch.setenv("SEARXNG_URL", "   ")
-        from tools.web_providers.searxng import SearXNGSearchProvider
-        assert SearXNGSearchProvider().is_configured() is False
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
+        assert SearXNGWebSearchProvider().is_available() is False
 
     def test_provider_name(self):
-        from tools.web_providers.searxng import SearXNGSearchProvider
-        assert SearXNGSearchProvider().provider_name() == "searxng"
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
+        assert SearXNGWebSearchProvider().name == "searxng"
 
     def test_implements_web_search_provider(self):
-        from tools.web_providers.base import WebSearchProvider
-        from tools.web_providers.searxng import SearXNGSearchProvider
-        assert issubclass(SearXNGSearchProvider, WebSearchProvider)
+        from agent.web_search_provider import WebSearchProvider
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
+        assert issubclass(SearXNGWebSearchProvider, WebSearchProvider)
 
 
 class TestSearXNGSearchProviderSearch:
-    """Happy path and error handling for SearXNGSearchProvider.search()."""
+    """Happy path and error handling for SearXNGWebSearchProvider.search()."""
 
     _SAMPLE_RESPONSE = {
         "results": [
@@ -69,11 +69,11 @@ class TestSearXNGSearchProviderSearch:
 
     def test_happy_path_returns_normalized_results(self, monkeypatch):
         monkeypatch.setenv("SEARXNG_URL", "http://localhost:8080")
-        from tools.web_providers.searxng import SearXNGSearchProvider
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
         mock_resp = self._make_mock_response(self._SAMPLE_RESPONSE)
 
         with patch("httpx.get", return_value=mock_resp):
-            result = SearXNGSearchProvider().search("test query", limit=5)
+            result = SearXNGWebSearchProvider().search("test query", limit=5)
 
         assert result["success"] is True
         web = result["data"]["web"]
@@ -86,7 +86,7 @@ class TestSearXNGSearchProviderSearch:
     def test_results_sorted_by_score_descending(self, monkeypatch):
         """Results should be sorted by score before limit is applied."""
         monkeypatch.setenv("SEARXNG_URL", "http://localhost:8080")
-        from tools.web_providers.searxng import SearXNGSearchProvider
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
         unordered = {
             "results": [
                 {"title": "Low",  "url": "https://low.example.com",  "content": "", "score": 0.1},
@@ -97,7 +97,7 @@ class TestSearXNGSearchProviderSearch:
         mock_resp = self._make_mock_response(unordered)
 
         with patch("httpx.get", return_value=mock_resp):
-            result = SearXNGSearchProvider().search("query", limit=5)
+            result = SearXNGWebSearchProvider().search("query", limit=5)
 
         assert result["success"] is True
         assert result["data"]["web"][0]["title"] == "High"
@@ -106,33 +106,33 @@ class TestSearXNGSearchProviderSearch:
 
     def test_limit_is_respected(self, monkeypatch):
         monkeypatch.setenv("SEARXNG_URL", "http://localhost:8080")
-        from tools.web_providers.searxng import SearXNGSearchProvider
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
         mock_resp = self._make_mock_response(self._SAMPLE_RESPONSE)
 
         with patch("httpx.get", return_value=mock_resp):
-            result = SearXNGSearchProvider().search("query", limit=2)
+            result = SearXNGWebSearchProvider().search("query", limit=2)
 
         assert result["success"] is True
         assert len(result["data"]["web"]) == 2
 
     def test_position_is_one_indexed(self, monkeypatch):
         monkeypatch.setenv("SEARXNG_URL", "http://localhost:8080")
-        from tools.web_providers.searxng import SearXNGSearchProvider
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
         mock_resp = self._make_mock_response(self._SAMPLE_RESPONSE)
 
         with patch("httpx.get", return_value=mock_resp):
-            result = SearXNGSearchProvider().search("query", limit=5)
+            result = SearXNGWebSearchProvider().search("query", limit=5)
 
         positions = [r["position"] for r in result["data"]["web"]]
         assert positions == [1, 2, 3]
 
     def test_empty_results(self, monkeypatch):
         monkeypatch.setenv("SEARXNG_URL", "http://localhost:8080")
-        from tools.web_providers.searxng import SearXNGSearchProvider
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
         mock_resp = self._make_mock_response({"results": []})
 
         with patch("httpx.get", return_value=mock_resp):
-            result = SearXNGSearchProvider().search("nothing", limit=5)
+            result = SearXNGWebSearchProvider().search("nothing", limit=5)
 
         assert result["success"] is True
         assert result["data"]["web"] == []
@@ -140,7 +140,7 @@ class TestSearXNGSearchProviderSearch:
     def test_missing_score_falls_back_to_zero(self, monkeypatch):
         """Results without a score field should sort to the bottom."""
         monkeypatch.setenv("SEARXNG_URL", "http://localhost:8080")
-        from tools.web_providers.searxng import SearXNGSearchProvider
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
         data = {
             "results": [
                 {"title": "No score", "url": "https://noscore.example.com", "content": ""},
@@ -150,7 +150,7 @@ class TestSearXNGSearchProviderSearch:
         mock_resp = self._make_mock_response(data)
 
         with patch("httpx.get", return_value=mock_resp):
-            result = SearXNGSearchProvider().search("query", limit=5)
+            result = SearXNGWebSearchProvider().search("query", limit=5)
 
         assert result["success"] is True
         # Has score should sort first (0.8 > 0)
@@ -159,14 +159,14 @@ class TestSearXNGSearchProviderSearch:
     def test_http_error_returns_failure(self, monkeypatch):
         import httpx
         monkeypatch.setenv("SEARXNG_URL", "http://localhost:8080")
-        from tools.web_providers.searxng import SearXNGSearchProvider
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
 
         mock_resp = MagicMock()
         mock_resp.status_code = 500
         http_err = httpx.HTTPStatusError("500", request=MagicMock(), response=mock_resp)
 
         with patch("httpx.get", side_effect=http_err):
-            result = SearXNGSearchProvider().search("query", limit=5)
+            result = SearXNGWebSearchProvider().search("query", limit=5)
 
         assert result["success"] is False
         assert "500" in result["error"]
@@ -174,26 +174,26 @@ class TestSearXNGSearchProviderSearch:
     def test_request_error_returns_failure(self, monkeypatch):
         import httpx
         monkeypatch.setenv("SEARXNG_URL", "http://localhost:8080")
-        from tools.web_providers.searxng import SearXNGSearchProvider
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
 
         with patch("httpx.get", side_effect=httpx.RequestError("connection refused")):
-            result = SearXNGSearchProvider().search("query", limit=5)
+            result = SearXNGWebSearchProvider().search("query", limit=5)
 
         assert result["success"] is False
         assert "localhost:8080" in result["error"] or "connection" in result["error"].lower()
 
     def test_missing_url_returns_failure(self, monkeypatch):
         monkeypatch.delenv("SEARXNG_URL", raising=False)
-        from tools.web_providers.searxng import SearXNGSearchProvider
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
 
-        result = SearXNGSearchProvider().search("query", limit=5)
+        result = SearXNGWebSearchProvider().search("query", limit=5)
         assert result["success"] is False
         assert "SEARXNG_URL" in result["error"]
 
     def test_trailing_slash_stripped_from_url(self, monkeypatch):
         """Base URL trailing slash should not produce double-slash in endpoint."""
         monkeypatch.setenv("SEARXNG_URL", "http://localhost:8080/")
-        from tools.web_providers.searxng import SearXNGSearchProvider
+        from plugins.web.searxng.provider import SearXNGWebSearchProvider
         mock_resp = self._make_mock_response({"results": []})
 
         calls = []
@@ -202,7 +202,7 @@ class TestSearXNGSearchProviderSearch:
             return mock_resp
 
         with patch("httpx.get", side_effect=capture_get):
-            SearXNGSearchProvider().search("query", limit=5)
+            SearXNGWebSearchProvider().search("query", limit=5)
 
         assert calls[0] == "http://localhost:8080/search", f"Got: {calls[0]}"
 

tests/tools/test_web_tools_config.py

@@ -485,15 +485,28 @@ class TestWebSearchSchema:
     def test_web_search_clamps_limit_before_backend_call(self):
         import tools.web_tools
 
-        with patch("tools.web_tools._get_backend", return_value="parallel"), \
-             patch("tools.web_tools._parallel_search", return_value={"success": True, "data": {"web": []}}) as mock_search, \
+        # After the web-provider plugin migration, _parallel_search lives in
+        # plugins.web.parallel.provider.ParallelWebSearchProvider.search; the
+        # tool dispatcher resolves a provider from the registry and calls
+        # provider.search(query, limit). Mock the provider lookup so we can
+        # assert the limit is clamped before reaching the backend.
+        fake_search = MagicMock(return_value={"success": True, "data": {"web": []}})
+        fake_provider = MagicMock(
+            name="ParallelWebSearchProvider",
+            supports_search=MagicMock(return_value=True),
+        )
+        fake_provider.search = fake_search
+        fake_provider.name = "parallel"
+
+        with patch("tools.web_tools._get_search_backend", return_value="parallel"), \
+             patch("agent.web_search_registry.get_provider", return_value=fake_provider), \
              patch("tools.interrupt.is_interrupted", return_value=False), \
              patch.object(tools.web_tools._debug, "log_call"), \
              patch.object(tools.web_tools._debug, "save"):
             result = json.loads(tools.web_tools.web_search_tool("docs", limit=500))
 
         assert result == {"success": True, "data": {"web": []}}
-        mock_search.assert_called_once_with("docs", 100)
+        fake_search.assert_called_once_with("docs", 100)
 
 
 class TestWebSearchErrorHandling:
@@ -502,11 +515,19 @@ class TestWebSearchErrorHandling:
     def test_search_error_response_does_not_expose_diagnostics(self):
         import tools.web_tools
 
-        firecrawl_client = MagicMock()
-        firecrawl_client.search.side_effect = RuntimeError("boom")
+        # After the web-provider plugin migration, the firecrawl client lives
+        # at plugins.web.firecrawl.provider._get_firecrawl_client. We mock the
+        # registry's get_provider to return a fake provider whose .search()
+        # raises so we can verify error sanitization.
+        fake_provider = MagicMock(
+            name="FirecrawlWebSearchProvider",
+            supports_search=MagicMock(return_value=True),
+        )
+        fake_provider.search.side_effect = RuntimeError("boom")
+        fake_provider.name = "firecrawl"
 
-        with patch("tools.web_tools._get_backend", return_value="firecrawl"), \
-             patch("tools.web_tools._get_firecrawl_client", return_value=firecrawl_client), \
+        with patch("tools.web_tools._get_search_backend", return_value="firecrawl"), \
+             patch("agent.web_search_registry.get_provider", return_value=fake_provider), \
              patch("tools.interrupt.is_interrupted", return_value=False), \
              patch.object(tools.web_tools._debug, "log_call") as mock_log_call, \
              patch.object(tools.web_tools._debug, "save"):

tests/tools/test_website_policy.py

@@ -350,11 +350,16 @@ def test_browser_navigate_allows_when_shared_file_missing(monkeypatch, tmp_path)
 @pytest.mark.asyncio
 async def test_web_extract_short_circuits_blocked_url(monkeypatch):
     from tools import web_tools
+    from plugins.web.firecrawl import provider as firecrawl_provider
 
     # Allow test URLs past SSRF check so website policy is what gets tested
     monkeypatch.setattr(web_tools, "is_safe_url", lambda url: True)
+    # The per-URL website-policy gate moved into the firecrawl plugin's
+    # extract() during the web-provider migration. Patch it at the new
+    # location; the dispatcher-level gate (used by web_crawl_tool's
+    # pre-flight) still lives on tools.web_tools.
     monkeypatch.setattr(
-        web_tools,
+        firecrawl_provider,
         "check_website_access",
         lambda url: {
             "host": "blocked.test",
@@ -364,11 +369,13 @@ async def test_web_extract_short_circuits_blocked_url(monkeypatch):
         },
     )
     monkeypatch.setattr(
-        web_tools,
+        firecrawl_provider,
         "_get_firecrawl_client",
         lambda: pytest.fail("firecrawl should not run for blocked URL"),
     )
     monkeypatch.setattr("tools.interrupt.is_interrupted", lambda: False)
+    # Force the firecrawl plugin to be the active extract provider.
+    monkeypatch.setenv("FIRECRAWL_API_KEY", "fake-key")
 
     result = json.loads(await web_tools.web_extract_tool(["https://blocked.test"], use_llm_processing=False))
 
@@ -398,6 +405,7 @@ def test_check_website_access_fails_open_on_malformed_config(tmp_path, monkeypat
 @pytest.mark.asyncio
 async def test_web_extract_blocks_redirected_final_url(monkeypatch):
     from tools import web_tools
+    from plugins.web.firecrawl import provider as firecrawl_provider
 
     # Allow test URLs past SSRF check so website policy is what gets tested
     monkeypatch.setattr(web_tools, "is_safe_url", lambda url: True)
@@ -424,9 +432,12 @@ async def test_web_extract_blocks_redirected_final_url(monkeypatch):
                 },
             }
 
-    monkeypatch.setattr(web_tools, "check_website_access", fake_check)
-    monkeypatch.setattr(web_tools, "_get_firecrawl_client", lambda: FakeFirecrawlClient())
+    # After the web-provider migration, the per-URL gate + firecrawl client
+    # live in the plugin. Patch both at the plugin location.
+    monkeypatch.setattr(firecrawl_provider, "check_website_access", fake_check)
+    monkeypatch.setattr(firecrawl_provider, "_get_firecrawl_client", lambda: FakeFirecrawlClient())
     monkeypatch.setattr("tools.interrupt.is_interrupted", lambda: False)
+    monkeypatch.setenv("FIRECRAWL_API_KEY", "fake-key")
 
     result = json.loads(await web_tools.web_extract_tool(["https://allowed.test"], use_llm_processing=False))
 
@@ -443,6 +454,9 @@ async def test_web_crawl_short_circuits_blocked_url(monkeypatch):
     monkeypatch.setenv("FIRECRAWL_API_KEY", "fake-key")
     # Allow test URLs past SSRF check so website policy is what gets tested
     monkeypatch.setattr(web_tools, "is_safe_url", lambda url: True)
+    # The dispatcher-level (seed-URL) policy gate still lives on web_tools.
+    # No per-page gate runs in this test because the dispatcher returns
+    # immediately when the seed is blocked, before delegating to the plugin.
     monkeypatch.setattr(
         web_tools,
         "check_website_access",
@@ -453,10 +467,13 @@ async def test_web_crawl_short_circuits_blocked_url(monkeypatch):
             "message": "Blocked by website policy",
         },
     )
+    # If the dispatcher ever reaches the firecrawl plugin's crawl(), the test
+    # fails — pin the plugin module's client lookup so we'd notice.
+    from plugins.web.firecrawl import provider as firecrawl_provider
     monkeypatch.setattr(
-        web_tools,
+        firecrawl_provider,
         "_get_firecrawl_client",
-        lambda: pytest.fail("firecrawl should not run for blocked crawl URL"),
+        lambda: pytest.fail("firecrawl plugin should not run for blocked crawl URL"),
     )
     monkeypatch.setattr("tools.interrupt.is_interrupted", lambda: False)
 
@@ -469,13 +486,17 @@ async def test_web_crawl_short_circuits_blocked_url(monkeypatch):
 @pytest.mark.asyncio
 async def test_web_crawl_blocks_redirected_final_url(monkeypatch):
     from tools import web_tools
+    from plugins.web.firecrawl import provider as firecrawl_provider
 
-    # web_crawl_tool checks for Firecrawl env before website policy
+    # Force the firecrawl plugin to be the active crawl provider.
     monkeypatch.setenv("FIRECRAWL_API_KEY", "fake-key")
     # Allow test URLs past SSRF check so website policy is what gets tested
     monkeypatch.setattr(web_tools, "is_safe_url", lambda url: True)
 
     def fake_check(url):
+        # Dispatcher seed-URL gate (web_tools.check_website_access call)
+        # and plugin per-page gate (firecrawl_provider.check_website_access
+        # call) both flow through this single fake_check.
         if url == "https://allowed.test":
             return None
         if url == "https://blocked.test/final":
@@ -501,8 +522,13 @@ async def test_web_crawl_blocks_redirected_final_url(monkeypatch):
                 ]
             }
 
+    # After PR #25182 follow-up: per-page policy gate lives in
+    # plugins.web.firecrawl.provider.crawl(). Patch the gate + client at
+    # the plugin location. The dispatcher-level (seed) gate also reads
+    # web_tools.check_website_access — patch both.
     monkeypatch.setattr(web_tools, "check_website_access", fake_check)
-    monkeypatch.setattr(web_tools, "_get_firecrawl_client", lambda: FakeCrawlClient())
+    monkeypatch.setattr(firecrawl_provider, "check_website_access", fake_check)
+    monkeypatch.setattr(firecrawl_provider, "_get_firecrawl_client", lambda: FakeCrawlClient())
     monkeypatch.setattr("tools.interrupt.is_interrupted", lambda: False)
 
     result = json.loads(await web_tools.web_crawl_tool("https://allowed.test", use_llm_processing=False))