diff --git a/acp_adapter/server.py b/acp_adapter/server.py index 119a08685..aa886cfbd 100644 --- a/acp_adapter/server.py +++ b/acp_adapter/server.py @@ -51,7 +51,7 @@ try: except ImportError: from acp.schema import AuthMethod as AuthMethodAgent # type: ignore[attr-defined] -from acp_adapter.auth import detect_provider, has_provider +from acp_adapter.auth import detect_provider from acp_adapter.events import ( make_message_cb, make_step_cb, @@ -351,9 +351,18 @@ class HermesACPAgent(acp.Agent): ) async def authenticate(self, method_id: str, **kwargs: Any) -> AuthenticateResponse | None: - if has_provider(): - return AuthenticateResponse() - return None + # Only accept authenticate() calls whose method_id matches the + # provider we advertised in initialize(). Without this check, + # authenticate() would acknowledge any method_id as long as the + # server has provider credentials configured — harmless under + # Hermes' threat model (ACP is stdio-only, local-trust), but poor + # API hygiene and confusing if ACP ever grows multi-method auth. + provider = detect_provider() + if not provider: + return None + if not isinstance(method_id, str) or method_id.strip().lower() != provider: + return None + return AuthenticateResponse() # ---- Session management ------------------------------------------------- diff --git a/hermes_cli/models.py b/hermes_cli/models.py index 1e5abb97e..ae5421795 100644 --- a/hermes_cli/models.py +++ b/hermes_cli/models.py @@ -688,6 +688,31 @@ def _openrouter_model_is_free(pricing: Any) -> bool: return False +def _openrouter_model_supports_tools(item: Any) -> bool: + """Return True when the model's ``supported_parameters`` advertise tool calling. + + hermes-agent is tool-calling-first — every provider path assumes the model + can invoke tools. Models that don't advertise ``tools`` in their + ``supported_parameters`` (e.g. image-only or completion-only models) cannot + be driven by the agent loop and would fail at the first tool call. + + **Permissive when the field is missing.** Some OpenRouter-compatible gateways + (Nous Portal, private mirrors, older catalog snapshots) don't populate + ``supported_parameters`` at all. Treat that as "unknown capability → allow" + so the picker doesn't silently empty for those users. Only hide models + whose ``supported_parameters`` is an explicit list that omits ``tools``. + + Ported from Kilo-Org/kilocode#9068. + """ + if not isinstance(item, dict): + return True + params = item.get("supported_parameters") + if not isinstance(params, list): + # Field absent / malformed / None — be permissive. + return True + return "tools" in params + + def fetch_openrouter_models( timeout: float = 8.0, *, @@ -730,6 +755,11 @@ def fetch_openrouter_models( live_item = live_by_id.get(preferred_id) if live_item is None: continue + # Hide models that don't advertise tool-calling support — hermes-agent + # requires it and surfacing them leads to immediate runtime failures + # when the user selects them. Ported from Kilo-Org/kilocode#9068. + if not _openrouter_model_supports_tools(live_item): + continue desc = "free" if _openrouter_model_is_free(live_item.get("pricing")) else "" curated.append((preferred_id, desc)) diff --git a/tests/acp/test_server.py b/tests/acp/test_server.py index 5893d7907..61db3f9fb 100644 --- a/tests/acp/test_server.py +++ b/tests/acp/test_server.py @@ -95,19 +95,37 @@ class TestInitialize: class TestAuthenticate: @pytest.mark.asyncio - async def test_authenticate_with_provider_configured(self, agent, monkeypatch): + async def test_authenticate_with_matching_method_id(self, agent, monkeypatch): monkeypatch.setattr( - "acp_adapter.server.has_provider", - lambda: True, + "acp_adapter.server.detect_provider", + lambda: "openrouter", ) resp = await agent.authenticate(method_id="openrouter") assert isinstance(resp, AuthenticateResponse) + @pytest.mark.asyncio + async def test_authenticate_is_case_insensitive(self, agent, monkeypatch): + monkeypatch.setattr( + "acp_adapter.server.detect_provider", + lambda: "openrouter", + ) + resp = await agent.authenticate(method_id="OpenRouter") + assert isinstance(resp, AuthenticateResponse) + + @pytest.mark.asyncio + async def test_authenticate_rejects_mismatched_method_id(self, agent, monkeypatch): + monkeypatch.setattr( + "acp_adapter.server.detect_provider", + lambda: "openrouter", + ) + resp = await agent.authenticate(method_id="totally-invalid-method") + assert resp is None + @pytest.mark.asyncio async def test_authenticate_without_provider(self, agent, monkeypatch): monkeypatch.setattr( - "acp_adapter.server.has_provider", - lambda: False, + "acp_adapter.server.detect_provider", + lambda: None, ) resp = await agent.authenticate(method_id="openrouter") assert resp is None diff --git a/tests/hermes_cli/test_models.py b/tests/hermes_cli/test_models.py index fc86caeeb..ea2f3057f 100644 --- a/tests/hermes_cli/test_models.py +++ b/tests/hermes_cli/test_models.py @@ -88,6 +88,131 @@ class TestFetchOpenRouterModels: assert models == OPENROUTER_MODELS + def test_filters_out_models_without_tool_support(self, monkeypatch): + """Models whose supported_parameters omits 'tools' must not appear in the picker. + + hermes-agent is tool-calling-first — surfacing a non-tool model leads to + immediate runtime failures when the user selects it. Ported from + Kilo-Org/kilocode#9068. + """ + class _Resp: + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def read(self): + # opus-4.6 advertises tools → kept + # nano-image has explicit supported_parameters that OMITS tools → dropped + # qwen3.6-plus advertises tools → kept + return ( + b'{"data":[' + b'{"id":"anthropic/claude-opus-4.6","pricing":{"prompt":"0.000015","completion":"0.000075"},' + b'"supported_parameters":["temperature","tools","tool_choice"]},' + b'{"id":"google/gemini-3-pro-image-preview","pricing":{"prompt":"0.00001","completion":"0.00003"},' + b'"supported_parameters":["temperature","response_format"]},' + b'{"id":"qwen/qwen3.6-plus","pricing":{"prompt":"0.000000325","completion":"0.00000195"},' + b'"supported_parameters":["tools","temperature"]}' + b']}' + ) + + # Include the image-only id in the curated list so it has a chance to be surfaced. + monkeypatch.setattr( + _models_mod, + "OPENROUTER_MODELS", + [ + ("anthropic/claude-opus-4.6", ""), + ("google/gemini-3-pro-image-preview", ""), + ("qwen/qwen3.6-plus", ""), + ], + ) + monkeypatch.setattr(_models_mod, "_openrouter_catalog_cache", None) + with patch("hermes_cli.models.urllib.request.urlopen", return_value=_Resp()): + models = fetch_openrouter_models(force_refresh=True) + + ids = [mid for mid, _ in models] + assert "anthropic/claude-opus-4.6" in ids + assert "qwen/qwen3.6-plus" in ids + # Image-only model advertised supported_parameters WITHOUT tools → must be dropped. + assert "google/gemini-3-pro-image-preview" not in ids + + def test_permissive_when_supported_parameters_missing(self, monkeypatch): + """Models missing the supported_parameters field keep appearing in the picker. + + Some OpenRouter-compatible gateways (Nous Portal, private mirrors, older + catalog snapshots) don't populate supported_parameters. Treating missing + as 'unknown → allow' prevents the picker from silently emptying on + those gateways. + """ + class _Resp: + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def read(self): + # No supported_parameters field at all on either entry. + return ( + b'{"data":[' + b'{"id":"anthropic/claude-opus-4.6","pricing":{"prompt":"0.000015","completion":"0.000075"}},' + b'{"id":"qwen/qwen3.6-plus","pricing":{"prompt":"0.000000325","completion":"0.00000195"}}' + b']}' + ) + + monkeypatch.setattr(_models_mod, "_openrouter_catalog_cache", None) + with patch("hermes_cli.models.urllib.request.urlopen", return_value=_Resp()): + models = fetch_openrouter_models(force_refresh=True) + + ids = [mid for mid, _ in models] + assert "anthropic/claude-opus-4.6" in ids + assert "qwen/qwen3.6-plus" in ids + + +class TestOpenRouterToolSupportHelper: + """Unit tests for _openrouter_model_supports_tools (Kilo port #9068).""" + + def test_tools_in_supported_parameters(self): + from hermes_cli.models import _openrouter_model_supports_tools + assert _openrouter_model_supports_tools( + {"id": "x", "supported_parameters": ["temperature", "tools"]} + ) is True + + def test_tools_missing_from_supported_parameters(self): + from hermes_cli.models import _openrouter_model_supports_tools + assert _openrouter_model_supports_tools( + {"id": "x", "supported_parameters": ["temperature", "response_format"]} + ) is False + + def test_supported_parameters_absent_is_permissive(self): + """Missing field → allow (so older / non-OR gateways still work).""" + from hermes_cli.models import _openrouter_model_supports_tools + assert _openrouter_model_supports_tools({"id": "x"}) is True + + def test_supported_parameters_none_is_permissive(self): + from hermes_cli.models import _openrouter_model_supports_tools + assert _openrouter_model_supports_tools({"id": "x", "supported_parameters": None}) is True + + def test_supported_parameters_malformed_is_permissive(self): + """Malformed (non-list) value → allow rather than silently drop.""" + from hermes_cli.models import _openrouter_model_supports_tools + assert _openrouter_model_supports_tools( + {"id": "x", "supported_parameters": "tools,temperature"} + ) is True + + def test_non_dict_item_is_permissive(self): + from hermes_cli.models import _openrouter_model_supports_tools + assert _openrouter_model_supports_tools(None) is True + assert _openrouter_model_supports_tools("anthropic/claude-opus-4.6") is True + + def test_empty_supported_parameters_list_drops_model(self): + """Explicit empty list → no tools → drop.""" + from hermes_cli.models import _openrouter_model_supports_tools + assert _openrouter_model_supports_tools( + {"id": "x", "supported_parameters": []} + ) is False + class TestFindOpenrouterSlug: def test_exact_match(self):