From d39a031329b4613ee44e504bfd5bd0059f9db8f0 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 19 Aug 2026 20:17:07 -0500 Subject: [PATCH] fix(nous): stop dropping "thinking off" on Portal models that can honor it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reasoning: {enabled: false} is the only shape the Portal honors, and the profile refused to send it for every model. Sending nothing means the upstream default instead, which on a thinking-first route like deepseek/deepseek-v4-pro (catalog: default_effort high) is thinking ON — so turning thinking off kept billing reasoning tokens on every turn. The blanket omission was over-broad. The Portal only rejects a disable on reasoning-mandatory routes ("Reasoning is mandatory for this model"), which its catalog flags per model, so that flag now gates the omission. Models the catalog can't speak to keep the old behavior rather than risk the 400. extra_body.thinking, DeepSeek's own disable shape, is not forwarded upstream by the Portal and is not an option here. --- plugins/model-providers/nous/__init__.py | 39 +++++- .../agent/transports/test_chat_completions.py | 7 +- .../model_providers/test_nous_profile.py | 123 ++++++++++++++++++ tests/providers/test_transport_parity.py | 2 +- 4 files changed, 165 insertions(+), 6 deletions(-) create mode 100644 tests/plugins/model_providers/test_nous_profile.py diff --git a/plugins/model-providers/nous/__init__.py b/plugins/model-providers/nous/__init__.py index df1c1f4674..e179e5c1b2 100644 --- a/plugins/model-providers/nous/__init__.py +++ b/plugins/model-providers/nous/__init__.py @@ -65,20 +65,53 @@ class NousProfile(ProviderProfile): body["provider"] = provider_preferences return body + @staticmethod + def _cannot_disable_reasoning(model: str | None) -> bool: + """True when a disable can't safely be sent for *model*. + + Reasoning-mandatory routes answer ``reasoning: {enabled: false}`` + with HTTP 400 ("Reasoning is mandatory for this model"), so the + catalog's ``mandatory`` flag decides. Cache-only, and an unknown + model (catalog cold, unlisted, or unreachable) also answers True: + a cold first turn errs toward the old omit-everything behavior + rather than risking a 400. + """ + try: + from hermes_cli.models import ( + nous_model_reasoning_capabilities, + warm_nous_reasoning_caps_async, + ) + + caps = nous_model_reasoning_capabilities(model) + if caps is None: + warm_nous_reasoning_caps_async() + return True + except Exception: + return True + return bool(caps.get("mandatory")) + def build_api_kwargs_extras( self, *, reasoning_config: dict | None = None, supports_reasoning: bool = False, + model: str | None = None, **context, ) -> tuple[dict[str, Any], dict[str, Any]]: - """Nous: passes full reasoning_config, but OMITS when disabled.""" + """Nous: passes the full reasoning_config, disable included. + + The Portal honors ``reasoning: {enabled: false}`` — it is the only + wire shape that does. Sending nothing means the *upstream* default, + which for a thinking-first model like ``deepseek/deepseek-v4-pro`` + (catalog: ``default_effort: high``) is thinking ON, so omitting a + disable silently ignored the user's "thinking off". + """ extra_body = {} if supports_reasoning: if reasoning_config is not None: rc = dict(reasoning_config) - if rc.get("enabled") is False: - pass # Nous omits reasoning when disabled + if rc.get("enabled") is False and self._cannot_disable_reasoning(model): + pass # route rejects a disable — let the model think else: extra_body["reasoning"] = rc else: diff --git a/tests/agent/transports/test_chat_completions.py b/tests/agent/transports/test_chat_completions.py index afa756bf50..059322ff41 100644 --- a/tests/agent/transports/test_chat_completions.py +++ b/tests/agent/transports/test_chat_completions.py @@ -262,7 +262,7 @@ class TestChatCompletionsBuildKwargs: ) assert kw["extra_body"]["reasoning"] == {"enabled": True, "effort": "medium"} - def test_nous_omits_disabled_reasoning(self, transport): + def test_nous_omits_disabled_reasoning_for_unknown_model(self, transport): from providers import get_provider_profile profile = get_provider_profile("nous") msgs = [{"role": "user", "content": "Hi"}] @@ -272,7 +272,10 @@ class TestChatCompletionsBuildKwargs: supports_reasoning=True, reasoning_config={"enabled": False}, ) - # Nous rejects enabled=false; reasoning omitted entirely + # Not a Portal model id, so the catalog can't rule out a + # reasoning-mandatory route (which 400s on a disable) — omit. + # tests/plugins/model_providers/test_nous_profile.py covers the + # catalog-known cases where the disable IS forwarded. assert "reasoning" not in kw.get("extra_body", {}) def test_ollama_num_ctx(self, transport): diff --git a/tests/plugins/model_providers/test_nous_profile.py b/tests/plugins/model_providers/test_nous_profile.py new file mode 100644 index 0000000000..68d738dcc2 --- /dev/null +++ b/tests/plugins/model_providers/test_nous_profile.py @@ -0,0 +1,123 @@ +"""Unit tests for the Nous Portal profile's reasoning wiring. + +The Portal honors ``reasoning: {enabled: false}`` — it is the only wire shape +that does, and ``extra_body.thinking`` is not forwarded upstream. The profile +used to drop a disable for every model, which on a thinking-first route like +``deepseek/deepseek-v4-pro`` (catalog: ``default_effort: high``) meant the +upstream default applied and "thinking off" burned reasoning tokens anyway. + +A disable is still dropped for reasoning-mandatory routes, which answer +``reasoning: {enabled: false}`` with HTTP 400, and for models the catalog +can't speak to — an unknown model errs toward the old behavior rather than +risking a 400 on a cold first turn. + +These tests pin that contract without going live. +""" + +from __future__ import annotations + +import pytest + + +@pytest.fixture +def nous_profile(): + """Resolve the registered Nous profile through the real discovery path.""" + # ``model_tools`` triggers plugin discovery on import, which is what + # registers the Nous profile in the global provider registry. + import model_tools # noqa: F401 + import providers + + profile = providers.get_provider_profile("nous") + assert profile is not None, "nous provider profile must be registered" + return profile + + +@pytest.fixture +def portal_catalog(monkeypatch): + """Prime the Portal reasoning-capability cache with known entries.""" + import hermes_cli.models as models_mod + + monkeypatch.setattr(models_mod, "_nous_reasoning_caps_failed_at", None) + monkeypatch.setattr(models_mod, "_nous_reasoning_caps_cache", { + "deepseek/deepseek-v4-pro": { + "supports_reasoning": True, + "supported_efforts": ["xhigh", "high"], + "mandatory": False, + }, + "arcee-ai/trinity-large-thinking": { + "supports_reasoning": True, + "supported_efforts": None, + "mandatory": True, + }, + }) + + +class TestNousReasoningWireShape: + """``build_api_kwargs_extras`` produces the Portal's wire format.""" + + def test_disable_reaches_optional_reasoning_model(self, nous_profile, portal_catalog): + """The knob the user set is the knob the Portal receives.""" + extra_body, top_level = nous_profile.build_api_kwargs_extras( + reasoning_config={"enabled": False}, + supports_reasoning=True, + model="deepseek/deepseek-v4-pro", + ) + assert extra_body == {"reasoning": {"enabled": False}} + assert top_level == {} + + def test_disable_dropped_for_mandatory_reasoning_model(self, nous_profile, portal_catalog): + """Mandatory routes 400 on a disable — send nothing instead.""" + extra_body, _ = nous_profile.build_api_kwargs_extras( + reasoning_config={"enabled": False}, + supports_reasoning=True, + model="arcee-ai/trinity-large-thinking", + ) + assert "reasoning" not in extra_body + + def test_disable_dropped_for_unknown_model(self, nous_profile, portal_catalog): + """Unlisted / cold catalog → fail safe, never risk the 400.""" + extra_body, _ = nous_profile.build_api_kwargs_extras( + reasoning_config={"enabled": False}, + supports_reasoning=True, + model="private/unlisted-route", + ) + assert "reasoning" not in extra_body + + @pytest.mark.parametrize( + "model", + ["deepseek/deepseek-v4-pro", "arcee-ai/trinity-large-thinking", "private/unlisted-route"], + ) + def test_enabled_config_always_forwarded(self, nous_profile, portal_catalog, model): + """Mandatory-ness only gates the disable; an enable always ships.""" + extra_body, _ = nous_profile.build_api_kwargs_extras( + reasoning_config={"enabled": True, "effort": "high"}, + supports_reasoning=True, + model=model, + ) + assert extra_body["reasoning"] == {"enabled": True, "effort": "high"} + + def test_no_config_defaults_to_medium(self, nous_profile, portal_catalog): + extra_body, _ = nous_profile.build_api_kwargs_extras( + reasoning_config=None, + supports_reasoning=True, + model="deepseek/deepseek-v4-pro", + ) + assert extra_body["reasoning"] == {"enabled": True, "effort": "medium"} + + def test_nothing_emitted_without_reasoning_support(self, nous_profile, portal_catalog): + extra_body, top_level = nous_profile.build_api_kwargs_extras( + reasoning_config={"enabled": False}, + supports_reasoning=False, + model="deepseek/deepseek-v4-pro", + ) + assert extra_body == {} + assert top_level == {} + + def test_caller_config_not_mutated(self, nous_profile, portal_catalog): + cfg = {"enabled": False} + nous_profile.build_api_kwargs_extras( + reasoning_config=cfg, + supports_reasoning=True, + model="deepseek/deepseek-v4-pro", + ) + assert cfg == {"enabled": False} diff --git a/tests/providers/test_transport_parity.py b/tests/providers/test_transport_parity.py index e6befad371..11a2b52d38 100644 --- a/tests/providers/test_transport_parity.py +++ b/tests/providers/test_transport_parity.py @@ -105,7 +105,7 @@ class TestOpenRouterParity: class TestNousParity: - """Nous: product tags, reasoning, omit when disabled.""" + """Nous: product tags, reasoning passthrough (disable included).""" def test_tags(self, transport): from agent.portal_tags import nous_portal_tags