Merge branch 'main' of github.com:NousResearch/hermes-agent into feat/ink-refactor

tests/tools/test_image_generation.py

@@ -107,16 +107,16 @@ class TestAspectRatioFamily:
     """Nano-banana uses aspect_ratio enum, NOT image_size."""
 
     def test_nano_banana_landscape_uses_aspect_ratio(self, image_tool):
-        p = image_tool._build_fal_payload("fal-ai/nano-banana", "hello", "landscape")
+        p = image_tool._build_fal_payload("fal-ai/nano-banana-pro", "hello", "landscape")
         assert p["aspect_ratio"] == "16:9"
         assert "image_size" not in p
 
     def test_nano_banana_square_uses_aspect_ratio(self, image_tool):
-        p = image_tool._build_fal_payload("fal-ai/nano-banana", "hello", "square")
+        p = image_tool._build_fal_payload("fal-ai/nano-banana-pro", "hello", "square")
         assert p["aspect_ratio"] == "1:1"
 
     def test_nano_banana_portrait_uses_aspect_ratio(self, image_tool):
-        p = image_tool._build_fal_payload("fal-ai/nano-banana", "hello", "portrait")
+        p = image_tool._build_fal_payload("fal-ai/nano-banana-pro", "hello", "portrait")
         assert p["aspect_ratio"] == "9:16"
 
 
@@ -164,13 +164,17 @@ class TestSupportsFilter:
         assert "num_inference_steps" not in p
 
     def test_recraft_has_minimal_payload(self, image_tool):
-        # Recraft supports prompt, image_size, style only.
-        p = image_tool._build_fal_payload("fal-ai/recraft-v3", "hi", "landscape")
-        assert set(p.keys()) <= {"prompt", "image_size", "style"}
+        # Recraft V4 Pro supports prompt, image_size, enable_safety_checker,
+        # colors, background_color (no seed, no style — V4 dropped V3's style enum).
+        p = image_tool._build_fal_payload("fal-ai/recraft/v4/pro/text-to-image", "hi", "landscape")
+        assert set(p.keys()) <= {
+            "prompt", "image_size", "enable_safety_checker",
+            "colors", "background_color",
+        }
 
     def test_nano_banana_never_gets_image_size(self, image_tool):
         # Common bug: translator accidentally setting both image_size and aspect_ratio.
-        p = image_tool._build_fal_payload("fal-ai/nano-banana", "hi", "landscape", seed=1)
+        p = image_tool._build_fal_payload("fal-ai/nano-banana-pro", "hi", "landscape", seed=1)
         assert "image_size" not in p
         assert p["aspect_ratio"] == "16:9"
 
@@ -285,9 +289,9 @@ class TestModelResolution:
     def test_config_wins_over_env_var(self, image_tool, monkeypatch):
         monkeypatch.setenv("FAL_IMAGE_MODEL", "fal-ai/z-image/turbo")
         with patch("hermes_cli.config.load_config",
-                   return_value={"image_gen": {"model": "fal-ai/nano-banana"}}):
+                   return_value={"image_gen": {"model": "fal-ai/nano-banana-pro"}}):
             mid, _ = image_tool._resolve_fal_model()
-        assert mid == "fal-ai/nano-banana"
+        assert mid == "fal-ai/nano-banana-pro"
 
 
 # ---------------------------------------------------------------------------
@@ -387,10 +391,10 @@ class TestManagedGatewayErrorTranslation:
                             lambda gw: mock_managed_client)
 
         with pytest.raises(ValueError) as exc_info:
-            image_tool._submit_fal_request("fal-ai/nano-banana", {"prompt": "x"})
+            image_tool._submit_fal_request("fal-ai/nano-banana-pro", {"prompt": "x"})
 
         msg = str(exc_info.value)
-        assert "fal-ai/nano-banana" in msg
+        assert "fal-ai/nano-banana-pro" in msg
         assert "403" in msg
         assert "FAL_KEY" in msg
         assert "hermes tools" in msg

tests/tools/test_mcp_oauth.py

@@ -431,3 +431,71 @@ class TestBuildOAuthAuthNonInteractive:
 
         assert auth is not None
         assert "no cached tokens found" not in caplog.text.lower()
+
+
+# ---------------------------------------------------------------------------
+# Extracted helper tests (Task 3 of MCP OAuth consolidation)
+# ---------------------------------------------------------------------------
+
+
+def test_build_client_metadata_basic():
+    """_build_client_metadata returns metadata with expected defaults."""
+    from tools.mcp_oauth import _build_client_metadata, _configure_callback_port
+
+    cfg = {"client_name": "Test Client"}
+    _configure_callback_port(cfg)
+    md = _build_client_metadata(cfg)
+
+    assert md.client_name == "Test Client"
+    assert "authorization_code" in md.grant_types
+    assert "refresh_token" in md.grant_types
+
+
+def test_build_client_metadata_without_secret_is_public():
+    """Without client_secret, token endpoint auth is 'none' (public client)."""
+    from tools.mcp_oauth import _build_client_metadata, _configure_callback_port
+
+    cfg = {}
+    _configure_callback_port(cfg)
+    md = _build_client_metadata(cfg)
+    assert md.token_endpoint_auth_method == "none"
+
+
+def test_build_client_metadata_with_secret_is_confidential():
+    """With client_secret, token endpoint auth is 'client_secret_post'."""
+    from tools.mcp_oauth import _build_client_metadata, _configure_callback_port
+
+    cfg = {"client_secret": "shh"}
+    _configure_callback_port(cfg)
+    md = _build_client_metadata(cfg)
+    assert md.token_endpoint_auth_method == "client_secret_post"
+
+
+def test_configure_callback_port_picks_free_port():
+    """_configure_callback_port(0) picks a free port in the ephemeral range."""
+    from tools.mcp_oauth import _configure_callback_port
+
+    cfg = {"redirect_port": 0}
+    port = _configure_callback_port(cfg)
+    assert 1024 < port < 65536
+    assert cfg["_resolved_port"] == port
+
+
+def test_configure_callback_port_uses_explicit_port():
+    """An explicit redirect_port is preserved."""
+    from tools.mcp_oauth import _configure_callback_port
+
+    cfg = {"redirect_port": 54321}
+    port = _configure_callback_port(cfg)
+    assert port == 54321
+    assert cfg["_resolved_port"] == 54321
+
+
+def test_parse_base_url_strips_path():
+    """_parse_base_url drops path components for OAuth discovery."""
+    from tools.mcp_oauth import _parse_base_url
+
+    assert _parse_base_url("https://example.com/mcp/v1") == "https://example.com"
+    assert _parse_base_url("https://example.com") == "https://example.com"
+    assert _parse_base_url("https://host.example.com:8080/api") == "https://host.example.com:8080"
+

tests/tools/test_mcp_oauth_integration.py

@@ -0,0 +1,193 @@
+"""End-to-end integration tests for the MCP OAuth consolidation.
+
+Exercises the full chain — manager, provider subclass, disk watch, 401
+dedup — with real file I/O and real imports (no transport mocks, no
+subprocesses). These are the tests that would catch Cthulhu's original
+BetterStack bug: an external process rewrites the tokens file on disk,
+and the running Hermes session picks up the new tokens on the next auth
+flow without requiring a restart.
+"""
+import asyncio
+import json
+import os
+import time
+
+import pytest
+
+
+pytest.importorskip("mcp.client.auth.oauth2", reason="MCP SDK 1.26.0+ required")
+
+
+@pytest.mark.asyncio
+async def test_external_refresh_picked_up_without_restart(tmp_path, monkeypatch):
+    """Simulate Cthulhu's cron workflow end-to-end.
+
+    1. A running Hermes session has OAuth tokens loaded in memory.
+    2. An external process (cron) writes fresh tokens to disk.
+    3. On the next auth flow, the manager's disk-watch invalidates the
+       in-memory state so the SDK re-reads from storage.
+    4. ``provider.context.current_tokens`` now reflects the new tokens
+       with no process restart required.
+    """
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+
+    from tools.mcp_oauth_manager import MCPOAuthManager, reset_manager_for_tests
+    reset_manager_for_tests()
+
+    token_dir = tmp_path / "mcp-tokens"
+    token_dir.mkdir(parents=True)
+    tokens_file = token_dir / "srv.json"
+    client_info_file = token_dir / "srv.client.json"
+
+    # Pre-seed the baseline state: valid tokens the session loaded at startup.
+    tokens_file.write_text(json.dumps({
+        "access_token": "OLD_ACCESS",
+        "token_type": "Bearer",
+        "expires_in": 3600,
+        "refresh_token": "OLD_REFRESH",
+    }))
+    client_info_file.write_text(json.dumps({
+        "client_id": "test-client",
+        "redirect_uris": ["http://127.0.0.1:12345/callback"],
+        "grant_types": ["authorization_code", "refresh_token"],
+        "response_types": ["code"],
+        "token_endpoint_auth_method": "none",
+    }))
+
+    mgr = MCPOAuthManager()
+    provider = mgr.get_or_build_provider(
+        "srv", "https://example.com/mcp", None,
+    )
+    assert provider is not None
+
+    # The SDK's _initialize reads tokens from storage into memory. This
+    # is what happens on the first http request under normal operation.
+    await provider._initialize()
+    assert provider.context.current_tokens.access_token == "OLD_ACCESS"
+
+    # Now record the baseline mtime in the manager (this happens
+    # automatically via the HermesMCPOAuthProvider.async_auth_flow
+    # pre-hook on the first real request, but we exercise it directly
+    # here for test determinism).
+    await mgr.invalidate_if_disk_changed("srv")
+
+    # EXTERNAL PROCESS: cron rewrites the tokens file with fresh creds.
+    # The old refresh_token has been consumed by this external exchange.
+    future_mtime = time.time() + 1
+    tokens_file.write_text(json.dumps({
+        "access_token": "NEW_ACCESS",
+        "token_type": "Bearer",
+        "expires_in": 3600,
+        "refresh_token": "NEW_REFRESH",
+    }))
+    os.utime(tokens_file, (future_mtime, future_mtime))
+
+    # The next auth flow should detect the mtime change and reload.
+    changed = await mgr.invalidate_if_disk_changed("srv")
+    assert changed, "manager must detect the disk mtime change"
+    assert provider._initialized is False, "_initialized must flip so SDK re-reads storage"
+
+    # Simulate the next async_auth_flow: _initialize runs because _initialized=False.
+    await provider._initialize()
+    assert provider.context.current_tokens.access_token == "NEW_ACCESS"
+    assert provider.context.current_tokens.refresh_token == "NEW_REFRESH"
+
+
+@pytest.mark.asyncio
+async def test_handle_401_deduplicates_concurrent_callers(tmp_path, monkeypatch):
+    """Ten concurrent 401 handlers for the same token should fire one recovery.
+
+    Mirrors Claude Code's pending401Handlers dedup pattern — prevents N MCP
+    tool calls hitting 401 simultaneously from all independently clearing
+    caches and re-reading the keychain (which thrashes the storage and
+    bogs down startup per CC-1096).
+    """
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+
+    from tools.mcp_oauth_manager import MCPOAuthManager, reset_manager_for_tests
+    reset_manager_for_tests()
+
+    token_dir = tmp_path / "mcp-tokens"
+    token_dir.mkdir(parents=True)
+    (token_dir / "srv.json").write_text(json.dumps({
+        "access_token": "TOK",
+        "token_type": "Bearer",
+        "expires_in": 3600,
+    }))
+
+    mgr = MCPOAuthManager()
+    provider = mgr.get_or_build_provider(
+        "srv", "https://example.com/mcp", None,
+    )
+    assert provider is not None
+
+    # Count how many times invalidate_if_disk_changed is called — proxy for
+    # how many actual recovery attempts fire.
+    call_count = 0
+    real_invalidate = mgr.invalidate_if_disk_changed
+
+    async def counting(name):
+        nonlocal call_count
+        call_count += 1
+        return await real_invalidate(name)
+
+    monkeypatch.setattr(mgr, "invalidate_if_disk_changed", counting)
+
+    # Fire 10 concurrent handlers with the same failed token.
+    results = await asyncio.gather(*(
+        mgr.handle_401("srv", "SAME_FAILED_TOKEN") for _ in range(10)
+    ))
+
+    # All callers get the same result (the shared future's resolution).
+    assert all(r == results[0] for r in results), "dedup must return identical result"
+    # Exactly ONE recovery ran — the rest awaited the same pending future.
+    assert call_count == 1, f"expected 1 recovery attempt, got {call_count}"
+
+
+@pytest.mark.asyncio
+async def test_handle_401_returns_false_when_no_provider(tmp_path, monkeypatch):
+    """handle_401 for an unknown server returns False cleanly."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    from tools.mcp_oauth_manager import MCPOAuthManager, reset_manager_for_tests
+    reset_manager_for_tests()
+
+    mgr = MCPOAuthManager()
+    result = await mgr.handle_401("nonexistent", "any_token")
+    assert result is False
+
+
+@pytest.mark.asyncio
+async def test_invalidate_if_disk_changed_handles_missing_file(tmp_path, monkeypatch):
+    """invalidate_if_disk_changed returns False when tokens file doesn't exist."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    from tools.mcp_oauth_manager import MCPOAuthManager, reset_manager_for_tests
+    reset_manager_for_tests()
+
+    mgr = MCPOAuthManager()
+    mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+
+    # No tokens file exists yet — this is the pre-auth state
+    result = await mgr.invalidate_if_disk_changed("srv")
+    assert result is False
+
+
+@pytest.mark.asyncio
+async def test_provider_is_reused_across_reconnects(tmp_path, monkeypatch):
+    """The manager caches providers; multiple reconnects reuse the same instance.
+
+    This is what makes the disk-watch stick across reconnects: tearing down
+    the MCP session and rebuilding it (Task 5's _reconnect_event path) must
+    not create a new provider, otherwise ``last_mtime_ns`` resets and the
+    first post-reconnect auth flow would spuriously "detect" a change.
+    """
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    from tools.mcp_oauth_manager import MCPOAuthManager, reset_manager_for_tests
+    reset_manager_for_tests()
+
+    mgr = MCPOAuthManager()
+    p1 = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+
+    # Simulate a reconnect: _run_http calls get_or_build_provider again
+    p2 = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+
+    assert p1 is p2, "manager must cache the provider across reconnects"

tests/tools/test_mcp_oauth_manager.py

@@ -0,0 +1,141 @@
+"""Tests for the MCP OAuth manager (tools/mcp_oauth_manager.py).
+
+The manager consolidates the eight scattered MCP-OAuth call sites into a
+single object with disk-mtime watch, dedup'd 401 handling, and a provider
+cache. See `tools/mcp_oauth_manager.py` for design rationale.
+"""
+import json
+import os
+import time
+
+import pytest
+
+pytest.importorskip(
+    "mcp.client.auth.oauth2",
+    reason="MCP SDK 1.26.0+ required for OAuth support",
+)
+
+
+def test_manager_is_singleton():
+    """get_manager() returns the same instance across calls."""
+    from tools.mcp_oauth_manager import get_manager, reset_manager_for_tests
+    reset_manager_for_tests()
+    m1 = get_manager()
+    m2 = get_manager()
+    assert m1 is m2
+
+
+def test_manager_get_or_build_provider_caches(tmp_path, monkeypatch):
+    """Calling get_or_build_provider twice with same name returns same provider."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    from tools.mcp_oauth_manager import MCPOAuthManager
+
+    mgr = MCPOAuthManager()
+    p1 = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+    p2 = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+    assert p1 is p2
+
+
+def test_manager_get_or_build_rebuilds_on_url_change(tmp_path, monkeypatch):
+    """Changing the URL discards the cached provider."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    from tools.mcp_oauth_manager import MCPOAuthManager
+
+    mgr = MCPOAuthManager()
+    p1 = mgr.get_or_build_provider("srv", "https://a.example.com/mcp", None)
+    p2 = mgr.get_or_build_provider("srv", "https://b.example.com/mcp", None)
+    assert p1 is not p2
+
+
+def test_manager_remove_evicts_cache(tmp_path, monkeypatch):
+    """remove(name) evicts the provider from cache AND deletes disk files."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    from tools.mcp_oauth_manager import MCPOAuthManager
+
+    # Pre-seed tokens on disk
+    token_dir = tmp_path / "mcp-tokens"
+    token_dir.mkdir(parents=True)
+    (token_dir / "srv.json").write_text(json.dumps({
+        "access_token": "TOK",
+        "token_type": "Bearer",
+    }))
+
+    mgr = MCPOAuthManager()
+    p1 = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+    assert p1 is not None
+    assert (token_dir / "srv.json").exists()
+
+    mgr.remove("srv")
+
+    assert not (token_dir / "srv.json").exists()
+    p2 = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+    assert p1 is not p2
+
+
+def test_hermes_provider_subclass_exists():
+    """HermesMCPOAuthProvider is defined and subclasses OAuthClientProvider."""
+    from tools.mcp_oauth_manager import _HERMES_PROVIDER_CLS
+    from mcp.client.auth.oauth2 import OAuthClientProvider
+
+    assert _HERMES_PROVIDER_CLS is not None
+    assert issubclass(_HERMES_PROVIDER_CLS, OAuthClientProvider)
+
+
+@pytest.mark.asyncio
+async def test_disk_watch_invalidates_on_mtime_change(tmp_path, monkeypatch):
+    """When the tokens file mtime changes, provider._initialized flips False.
+
+    This is the behaviour Claude Code ships as
+    invalidateOAuthCacheIfDiskChanged (CC-1096 / GH#24317) and is the core
+    fix for Cthulhu's external-cron refresh workflow.
+    """
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    from tools.mcp_oauth_manager import MCPOAuthManager, reset_manager_for_tests
+
+    reset_manager_for_tests()
+
+    token_dir = tmp_path / "mcp-tokens"
+    token_dir.mkdir(parents=True)
+    tokens_file = token_dir / "srv.json"
+    tokens_file.write_text(json.dumps({
+        "access_token": "OLD",
+        "token_type": "Bearer",
+    }))
+
+    mgr = MCPOAuthManager()
+    provider = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+    assert provider is not None
+
+    # First call: records mtime (zero -> real) -> returns True
+    changed1 = await mgr.invalidate_if_disk_changed("srv")
+    assert changed1 is True
+
+    # No file change -> False
+    changed2 = await mgr.invalidate_if_disk_changed("srv")
+    assert changed2 is False
+
+    # Touch file with a newer mtime
+    future_mtime = time.time() + 10
+    os.utime(tokens_file, (future_mtime, future_mtime))
+
+    changed3 = await mgr.invalidate_if_disk_changed("srv")
+    assert changed3 is True
+    # _initialized flipped — next async_auth_flow will re-read from disk
+    assert provider._initialized is False
+
+
+def test_manager_builds_hermes_provider_subclass(tmp_path, monkeypatch):
+    """get_or_build_provider returns HermesMCPOAuthProvider, not plain OAuthClientProvider."""
+    from tools.mcp_oauth_manager import (
+        MCPOAuthManager, _HERMES_PROVIDER_CLS, reset_manager_for_tests,
+    )
+    reset_manager_for_tests()
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+
+    mgr = MCPOAuthManager()
+    provider = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+
+    assert _HERMES_PROVIDER_CLS is not None
+    assert isinstance(provider, _HERMES_PROVIDER_CLS)
+    assert provider._hermes_server_name == "srv"
+

tests/tools/test_mcp_reconnect_signal.py

@@ -0,0 +1,57 @@
+"""Tests for the MCPServerTask reconnect signal.
+
+When the OAuth layer cannot recover in-place (e.g., external refresh of a
+single-use refresh_token made the SDK's in-memory refresh fail), the tool
+handler signals MCPServerTask to tear down the current MCP session and
+reconnect with fresh credentials. This file exercises the signal plumbing
+in isolation from the full stdio/http transport machinery.
+"""
+import asyncio
+
+import pytest
+
+
+@pytest.mark.asyncio
+async def test_reconnect_event_attribute_exists():
+    """MCPServerTask has a _reconnect_event alongside _shutdown_event."""
+    from tools.mcp_tool import MCPServerTask
+    task = MCPServerTask("test")
+    assert hasattr(task, "_reconnect_event")
+    assert isinstance(task._reconnect_event, asyncio.Event)
+    assert not task._reconnect_event.is_set()
+
+
+@pytest.mark.asyncio
+async def test_wait_for_lifecycle_event_returns_reconnect():
+    """When _reconnect_event fires, helper returns 'reconnect' and clears it."""
+    from tools.mcp_tool import MCPServerTask
+    task = MCPServerTask("test")
+
+    task._reconnect_event.set()
+    reason = await task._wait_for_lifecycle_event()
+    assert reason == "reconnect"
+    # Should have cleared so the next cycle starts fresh
+    assert not task._reconnect_event.is_set()
+
+
+@pytest.mark.asyncio
+async def test_wait_for_lifecycle_event_returns_shutdown():
+    """When _shutdown_event fires, helper returns 'shutdown'."""
+    from tools.mcp_tool import MCPServerTask
+    task = MCPServerTask("test")
+
+    task._shutdown_event.set()
+    reason = await task._wait_for_lifecycle_event()
+    assert reason == "shutdown"
+
+
+@pytest.mark.asyncio
+async def test_wait_for_lifecycle_event_shutdown_wins_when_both_set():
+    """If both events are set simultaneously, shutdown takes precedence."""
+    from tools.mcp_tool import MCPServerTask
+    task = MCPServerTask("test")
+
+    task._shutdown_event.set()
+    task._reconnect_event.set()
+    reason = await task._wait_for_lifecycle_event()
+    assert reason == "shutdown"

tests/tools/test_mcp_tool_401_handling.py

@@ -0,0 +1,139 @@
+"""Tests for MCP tool-handler auth-failure detection.
+
+When a tool call raises UnauthorizedError / OAuthNonInteractiveError /
+httpx.HTTPStatusError(401), the handler should:
+  1. Ask MCPOAuthManager.handle_401 if recovery is viable.
+  2. If yes, trigger MCPServerTask._reconnect_event and retry once.
+  3. If no, return a structured needs_reauth error so the model stops
+     hallucinating manual refresh attempts.
+"""
+import json
+from unittest.mock import MagicMock
+
+import pytest
+
+
+pytest.importorskip("mcp.client.auth.oauth2")
+
+
+def test_is_auth_error_detects_oauth_flow_error():
+    from tools.mcp_tool import _is_auth_error
+    from mcp.client.auth import OAuthFlowError
+
+    assert _is_auth_error(OAuthFlowError("expired")) is True
+
+
+def test_is_auth_error_detects_oauth_non_interactive():
+    from tools.mcp_tool import _is_auth_error
+    from tools.mcp_oauth import OAuthNonInteractiveError
+
+    assert _is_auth_error(OAuthNonInteractiveError("no browser")) is True
+
+
+def test_is_auth_error_detects_httpx_401():
+    from tools.mcp_tool import _is_auth_error
+    import httpx
+
+    response = MagicMock()
+    response.status_code = 401
+    exc = httpx.HTTPStatusError("unauth", request=MagicMock(), response=response)
+    assert _is_auth_error(exc) is True
+
+
+def test_is_auth_error_rejects_httpx_500():
+    from tools.mcp_tool import _is_auth_error
+    import httpx
+
+    response = MagicMock()
+    response.status_code = 500
+    exc = httpx.HTTPStatusError("oops", request=MagicMock(), response=response)
+    assert _is_auth_error(exc) is False
+
+
+def test_is_auth_error_rejects_generic_exception():
+    from tools.mcp_tool import _is_auth_error
+    assert _is_auth_error(ValueError("not auth")) is False
+    assert _is_auth_error(RuntimeError("not auth")) is False
+
+
+def test_call_tool_handler_returns_needs_reauth_on_unrecoverable_401(monkeypatch, tmp_path):
+    """When session.call_tool raises 401 and handle_401 returns False,
+    handler returns a structured needs_reauth error (not a generic failure)."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+
+    from tools.mcp_tool import _make_tool_handler
+    from tools.mcp_oauth_manager import get_manager, reset_manager_for_tests
+    from mcp.client.auth import OAuthFlowError
+
+    reset_manager_for_tests()
+
+    # Stub server
+    server = MagicMock()
+    server.name = "srv"
+    session = MagicMock()
+
+    async def _call_tool_raises(*a, **kw):
+        raise OAuthFlowError("token expired")
+
+    session.call_tool = _call_tool_raises
+    server.session = session
+    server._reconnect_event = MagicMock()
+    server._ready = MagicMock()
+    server._ready.is_set.return_value = True
+
+    from tools import mcp_tool
+    mcp_tool._servers["srv"] = server
+    mcp_tool._server_error_counts.pop("srv", None)
+
+    # Ensure the MCP loop exists (run_on_mcp_loop needs it)
+    mcp_tool._ensure_mcp_loop()
+
+    # Force handle_401 to return False (no recovery available)
+    mgr = get_manager()
+
+    async def _h401(name, token=None):
+        return False
+
+    monkeypatch.setattr(mgr, "handle_401", _h401)
+
+    try:
+        handler = _make_tool_handler("srv", "tool1", 10.0)
+        result = handler({"arg": "v"})
+        parsed = json.loads(result)
+        assert parsed.get("needs_reauth") is True, f"expected needs_reauth, got: {parsed}"
+        assert parsed.get("server") == "srv"
+        assert "re-auth" in parsed.get("error", "").lower() or "reauth" in parsed.get("error", "").lower()
+    finally:
+        mcp_tool._servers.pop("srv", None)
+        mcp_tool._server_error_counts.pop("srv", None)
+
+
+def test_call_tool_handler_non_auth_error_still_generic(monkeypatch, tmp_path):
+    """Non-auth exceptions still surface via the generic error path, not needs_reauth."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    from tools.mcp_tool import _make_tool_handler
+
+    server = MagicMock()
+    server.name = "srv"
+    session = MagicMock()
+
+    async def _raises(*a, **kw):
+        raise RuntimeError("unrelated")
+
+    session.call_tool = _raises
+    server.session = session
+
+    from tools import mcp_tool
+    mcp_tool._servers["srv"] = server
+    mcp_tool._server_error_counts.pop("srv", None)
+    mcp_tool._ensure_mcp_loop()
+
+    try:
+        handler = _make_tool_handler("srv", "tool1", 10.0)
+        result = handler({"arg": "v"})
+        parsed = json.loads(result)
+        assert "needs_reauth" not in parsed
+        assert "MCP call failed" in parsed.get("error", "")
+    finally:
+        mcp_tool._servers.pop("srv", None)
+        mcp_tool._server_error_counts.pop("srv", None)

tests/tools/test_skills_sync.py

@@ -12,6 +12,7 @@ from tools.skills_sync import (
     _compute_relative_dest,
     _dir_hash,
     sync_skills,
+    reset_bundled_skill,
     MANIFEST_FILE,
     SKILLS_DIR,
 )
@@ -521,3 +522,133 @@ class TestGetBundledDir:
         monkeypatch.setenv("HERMES_BUNDLED_SKILLS", "")
         result = _get_bundled_dir()
         assert result.name == "skills"
+
+
+class TestResetBundledSkill:
+    """Covers reset_bundled_skill() — the escape hatch for the 'user-modified' trap."""
+
+    def _setup_bundled(self, tmp_path):
+        """Create a minimal bundled skills tree with a single 'google-workspace' skill."""
+        bundled = tmp_path / "bundled_skills"
+        (bundled / "productivity" / "google-workspace").mkdir(parents=True)
+        (bundled / "productivity" / "google-workspace" / "SKILL.md").write_text(
+            "---\nname: google-workspace\n---\n# GW v2 (upstream)\n"
+        )
+        return bundled
+
+    def _patches(self, bundled, skills_dir, manifest_file):
+        from contextlib import ExitStack
+        stack = ExitStack()
+        stack.enter_context(patch("tools.skills_sync._get_bundled_dir", return_value=bundled))
+        stack.enter_context(patch("tools.skills_sync.SKILLS_DIR", skills_dir))
+        stack.enter_context(patch("tools.skills_sync.MANIFEST_FILE", manifest_file))
+        return stack
+
+    def test_reset_clears_stuck_user_modified_flag(self, tmp_path):
+        """The core bug repro: copy-pasted bundled restore doesn't un-stick the flag; reset does."""
+        bundled = self._setup_bundled(tmp_path)
+        skills_dir = tmp_path / "user_skills"
+        manifest_file = skills_dir / ".bundled_manifest"
+
+        # Simulate the stuck state: user edited the skill on an older bundled version,
+        # so manifest has an old origin hash that no longer matches anything on disk.
+        dest = skills_dir / "productivity" / "google-workspace"
+        dest.mkdir(parents=True)
+        (dest / "SKILL.md").write_text("---\nname: google-workspace\n---\n# GW v2 (upstream)\n")
+        # Stale origin_hash — from some prior bundled version. User "restored" by pasting
+        # the current bundled contents, so user_hash == current bundled_hash, but manifest
+        # still points at the stale hash → treated as user_modified forever.
+        manifest_file.write_text("google-workspace:STALEHASH000000000000000000000000\n")
+
+        with self._patches(bundled, skills_dir, manifest_file):
+            # Sanity check: without reset, sync would flag it user_modified
+            pre = sync_skills(quiet=True)
+            assert "google-workspace" in pre["user_modified"]
+
+            # Reset (no --restore) should clear the manifest entry and re-baseline
+            result = reset_bundled_skill("google-workspace", restore=False)
+
+            assert result["ok"] is True
+            assert result["action"] == "manifest_cleared"
+
+            # After reset, the manifest should hold the *current* bundled hash
+            manifest_after = _read_manifest()
+            expected = _dir_hash(bundled / "productivity" / "google-workspace")
+            assert manifest_after["google-workspace"] == expected
+        # User's copy was preserved (we didn't delete)
+        assert dest.exists()
+        assert "GW v2" in (dest / "SKILL.md").read_text()
+
+    def test_reset_restore_replaces_user_copy(self, tmp_path):
+        """--restore nukes the user's copy and re-copies the bundled version."""
+        bundled = self._setup_bundled(tmp_path)
+        skills_dir = tmp_path / "user_skills"
+        manifest_file = skills_dir / ".bundled_manifest"
+
+        dest = skills_dir / "productivity" / "google-workspace"
+        dest.mkdir(parents=True)
+        (dest / "SKILL.md").write_text("# heavily edited by user\n")
+        (dest / "my_custom_file.py").write_text("print('user-added')\n")
+        manifest_file.write_text("google-workspace:STALEHASH000000000000000000000000\n")
+
+        with self._patches(bundled, skills_dir, manifest_file):
+            result = reset_bundled_skill("google-workspace", restore=True)
+
+        assert result["ok"] is True
+        assert result["action"] == "restored"
+        # User's custom file should be gone
+        assert not (dest / "my_custom_file.py").exists()
+        # SKILL.md should be the bundled content
+        assert "GW v2 (upstream)" in (dest / "SKILL.md").read_text()
+
+    def test_reset_nonexistent_skill_errors_gracefully(self, tmp_path):
+        """Resetting a skill that's neither bundled nor in the manifest returns a clear error."""
+        bundled = self._setup_bundled(tmp_path)
+        skills_dir = tmp_path / "user_skills"
+        manifest_file = skills_dir / ".bundled_manifest"
+        skills_dir.mkdir(parents=True)
+        manifest_file.write_text("")
+
+        with self._patches(bundled, skills_dir, manifest_file):
+            result = reset_bundled_skill("some-hub-skill", restore=False)
+
+        assert result["ok"] is False
+        assert result["action"] == "not_in_manifest"
+        assert "not a tracked bundled skill" in result["message"]
+
+    def test_reset_restore_when_bundled_removed_upstream(self, tmp_path):
+        """If a skill was removed upstream, --restore should fail with a clear message."""
+        bundled = self._setup_bundled(tmp_path)
+        skills_dir = tmp_path / "user_skills"
+        manifest_file = skills_dir / ".bundled_manifest"
+        dest = skills_dir / "productivity" / "ghost-skill"
+        dest.mkdir(parents=True)
+        (dest / "SKILL.md").write_text("---\nname: ghost-skill\n---\n# Ghost\n")
+        manifest_file.write_text("ghost-skill:OLDHASH00000000000000000000000000\n")
+
+        with self._patches(bundled, skills_dir, manifest_file):
+            result = reset_bundled_skill("ghost-skill", restore=True)
+
+        assert result["ok"] is False
+        assert result["action"] == "bundled_missing"
+
+    def test_reset_no_op_when_already_clean(self, tmp_path):
+        """If manifest has skill but user copy is in-sync, reset still safely clears + re-baselines."""
+        bundled = self._setup_bundled(tmp_path)
+        skills_dir = tmp_path / "user_skills"
+        manifest_file = skills_dir / ".bundled_manifest"
+
+        # Simulate a clean state — do a fresh sync first
+        with self._patches(bundled, skills_dir, manifest_file):
+            sync_skills(quiet=True)
+            pre_manifest = _read_manifest()
+            assert "google-workspace" in pre_manifest
+
+            result = reset_bundled_skill("google-workspace", restore=False)
+
+            assert result["ok"] is True
+            assert result["action"] == "manifest_cleared"
+            # Manifest entry still present (re-baselined), user copy still present
+            post_manifest = _read_manifest()
+            assert "google-workspace" in post_manifest
+        assert (skills_dir / "productivity" / "google-workspace" / "SKILL.md").exists()

tests/tools/test_url_safety.py

@@ -152,6 +152,34 @@ class TestIsSafeUrl:
             # 100.0.0.1 is a global IP, not in CGNAT range
             assert is_safe_url("http://legit-host.example/") is True
 
+    def test_benchmark_ip_blocked_for_non_allowlisted_host(self):
+        with patch("socket.getaddrinfo", return_value=[
+            (2, 1, 6, "", ("198.18.0.23", 0)),
+        ]):
+            assert is_safe_url("https://example.com/file.jpg") is False
+
+    def test_qq_multimedia_hostname_allowed_with_benchmark_ip(self):
+        with patch("socket.getaddrinfo", return_value=[
+            (2, 1, 6, "", ("198.18.0.23", 0)),
+        ]):
+            assert is_safe_url("https://multimedia.nt.qq.com.cn/download?id=123") is True
+
+    def test_qq_multimedia_hostname_exception_is_exact_match(self):
+        with patch("socket.getaddrinfo", return_value=[
+            (2, 1, 6, "", ("198.18.0.23", 0)),
+        ]):
+            assert is_safe_url("https://sub.multimedia.nt.qq.com.cn/download?id=123") is False
+
+    def test_qq_multimedia_hostname_exception_requires_https(self):
+        with patch("socket.getaddrinfo", return_value=[
+            (2, 1, 6, "", ("198.18.0.23", 0)),
+        ]):
+            assert is_safe_url("http://multimedia.nt.qq.com.cn/download?id=123") is False
+
+    def test_qq_multimedia_hostname_dns_failure_still_blocked(self):
+        with patch("socket.getaddrinfo", side_effect=socket.gaierror("Name resolution failed")):
+            assert is_safe_url("https://multimedia.nt.qq.com.cn/download?id=123") is False
+
 
 class TestIsBlockedIp:
     """Direct tests for the _is_blocked_ip helper."""
@@ -159,7 +187,7 @@ class TestIsBlockedIp:
     @pytest.mark.parametrize("ip_str", [
         "127.0.0.1", "10.0.0.1", "172.16.0.1", "192.168.1.1",
         "169.254.169.254", "0.0.0.0", "224.0.0.1", "255.255.255.255",
-        "100.64.0.1", "100.100.100.100", "100.127.255.254",
+        "100.64.0.1", "100.100.100.100", "100.127.255.254", "198.18.0.23",
         "::1", "fe80::1", "fc00::1", "fd12::1", "ff02::1",
         "::ffff:127.0.0.1", "::ffff:169.254.169.254",
     ])

tests/tools/test_web_tools_config.py

@@ -63,38 +63,6 @@ class TestFirecrawlClientConfig:
 
     # ── Configuration matrix ─────────────────────────────────────────
 
-    def test_cloud_mode_key_only(self):
-        """API key without URL → cloud Firecrawl."""
-        with patch.dict(os.environ, {"FIRECRAWL_API_KEY": "fc-test"}):
-            with patch("tools.web_tools.Firecrawl") as mock_fc:
-                from tools.web_tools import _get_firecrawl_client
-                result = _get_firecrawl_client()
-                mock_fc.assert_called_once_with(api_key="fc-test")
-                assert result is mock_fc.return_value
-
-    def test_self_hosted_with_key(self):
-        """Both key + URL → self-hosted with auth."""
-        with patch.dict(os.environ, {
-            "FIRECRAWL_API_KEY": "fc-test",
-            "FIRECRAWL_API_URL": "http://localhost:3002",
-        }):
-            with patch("tools.web_tools.Firecrawl") as mock_fc:
-                from tools.web_tools import _get_firecrawl_client
-                result = _get_firecrawl_client()
-                mock_fc.assert_called_once_with(
-                    api_key="fc-test", api_url="http://localhost:3002"
-                )
-                assert result is mock_fc.return_value
-
-    def test_self_hosted_no_key(self):
-        """URL only, no key → self-hosted without auth."""
-        with patch.dict(os.environ, {"FIRECRAWL_API_URL": "http://localhost:3002"}):
-            with patch("tools.web_tools.Firecrawl") as mock_fc:
-                from tools.web_tools import _get_firecrawl_client
-                result = _get_firecrawl_client()
-                mock_fc.assert_called_once_with(api_url="http://localhost:3002")
-                assert result is mock_fc.return_value
-
     def test_no_config_raises_with_helpful_message(self):
         """Neither key nor URL → ValueError with guidance."""
         with patch("tools.web_tools.Firecrawl"):
@@ -169,18 +137,6 @@ class TestFirecrawlClientConfig:
                     api_url="https://firecrawl-gateway.nousresearch.com",
                 )
 
-    def test_direct_mode_is_preferred_over_tool_gateway(self):
-        """Explicit Firecrawl config should win over the gateway fallback."""
-        with patch.dict(os.environ, {
-            "FIRECRAWL_API_KEY": "fc-test",
-            "TOOL_GATEWAY_DOMAIN": "nousresearch.com",
-        }):
-            with patch("tools.web_tools._read_nous_access_token", return_value="nous-token"):
-                with patch("tools.web_tools.Firecrawl") as mock_fc:
-                    from tools.web_tools import _get_firecrawl_client
-                    _get_firecrawl_client()
-                mock_fc.assert_called_once_with(api_key="fc-test")
-
     def test_nous_auth_token_respects_hermes_home_override(self, tmp_path):
         """Auth lookup should read from HERMES_HOME/auth.json, not ~/.hermes/auth.json."""
         real_home = tmp_path / "real-home"
@@ -275,18 +231,6 @@ class TestFirecrawlClientConfig:
 
     # ── Edge cases ───────────────────────────────────────────────────
 
-    def test_empty_string_key_treated_as_absent(self):
-        """FIRECRAWL_API_KEY='' should not be passed as api_key."""
-        with patch.dict(os.environ, {
-            "FIRECRAWL_API_KEY": "",
-            "FIRECRAWL_API_URL": "http://localhost:3002",
-        }):
-            with patch("tools.web_tools.Firecrawl") as mock_fc:
-                from tools.web_tools import _get_firecrawl_client
-                _get_firecrawl_client()
-                # Empty string is falsy, so only api_url should be passed
-                mock_fc.assert_called_once_with(api_url="http://localhost:3002")
-
     def test_empty_string_key_no_url_raises(self):
         """FIRECRAWL_API_KEY='' with no URL → should raise."""
         with patch.dict(os.environ, {"FIRECRAWL_API_KEY": ""}):