diff --git a/agent/auxiliary_client.py b/agent/auxiliary_client.py index c47c3a4a1d26..c6e00340e7e1 100644 --- a/agent/auxiliary_client.py +++ b/agent/auxiliary_client.py @@ -102,7 +102,7 @@ OpenAI = _OpenAIProxy() # module-level name, resolves lazily on call/isinstance from agent.credential_pool import load_pool from hermes_cli.config import get_hermes_home from hermes_constants import OPENROUTER_BASE_URL -from utils import base_url_host_matches, base_url_hostname, normalize_proxy_env_vars +from utils import base_url_host_matches, base_url_hostname, model_forces_max_completion_tokens, normalize_proxy_env_vars logger = logging.getLogger(__name__) @@ -4300,13 +4300,15 @@ def get_auxiliary_extra_body() -> dict: return _nous_extra_body() if auxiliary_is_nous else {} -def auxiliary_max_tokens_param(value: int) -> dict: +def auxiliary_max_tokens_param(value: int, *, model: Optional[str] = None) -> dict: """Return the correct max tokens kwarg for the auxiliary client's provider. - + OpenRouter and local models use 'max_tokens'. Direct OpenAI with newer - models (gpt-4o, o-series, gpt-5+) requires 'max_completion_tokens'. + models (gpt-4o, gpt-4.1, gpt-5+, o-series) requires 'max_completion_tokens'. The Codex adapter translates max_tokens internally, so we use max_tokens - for it as well. + for it as well. Pass ``model`` so third-party OpenAI-compatible endpoints + fronting the newer families are also recognised — URL-only detection + misses the case where a custom base URL serves e.g. ``gpt-5.4``. """ custom_base = _current_custom_base_url() or_key = os.getenv("OPENROUTER_API_KEY") @@ -4316,6 +4318,9 @@ def auxiliary_max_tokens_param(value: int) -> dict: and _read_nous_auth() is None and base_url_hostname(custom_base) in {"api.openai.com", "api.githubcopilot.com"}): return {"max_completion_tokens": value} + # ...and for any caller serving a newer OpenAI-family model by name. + if model_forces_max_completion_tokens(model): + return {"max_completion_tokens": value} return {"max_tokens": value} diff --git a/run_agent.py b/run_agent.py index c717c66c178d..5465bb9ae2de 100644 --- a/run_agent.py +++ b/run_agent.py @@ -196,7 +196,7 @@ from agent.tool_dispatch_helpers import ( _extract_error_preview, _trajectory_normalize_msg, # noqa: F401 # re-exported for tests that `from run_agent import _trajectory_normalize_msg` ) -from utils import atomic_json_write, base_url_host_matches, base_url_hostname, is_truthy_value +from utils import atomic_json_write, base_url_host_matches, base_url_hostname, is_truthy_value, model_forces_max_completion_tokens @@ -1253,13 +1253,24 @@ class AIAgent: def _max_tokens_param(self, value: int) -> dict: """Return the correct max tokens kwarg for the current provider. - OpenAI's newer models (gpt-4o, o-series, gpt-5+) require - 'max_completion_tokens'. Azure OpenAI also requires - 'max_completion_tokens' for gpt-5.x models served via the - OpenAI-compatible endpoint. OpenRouter, local models, and older + OpenAI's newer models (gpt-4o, gpt-4.1, gpt-5+, o-series) require + 'max_completion_tokens'. Azure OpenAI and GitHub Copilot also require + 'max_completion_tokens' for those families served via their + OpenAI-compatible endpoints. OpenRouter, local models, and older OpenAI models use 'max_tokens'. + + The check is URL-first (api.openai.com / Azure / Copilot all use the + new kwarg), then falls back to a model-name check so third-party + OpenAI-compatible endpoints fronting those models are recognised — + URL-only detection misses that case and silently sends the wrong + kwarg, which the upstream model rejects with a 400. """ - if self._is_direct_openai_url() or self._is_azure_openai_url() or self._is_github_copilot_url(): + if ( + self._is_direct_openai_url() + or self._is_azure_openai_url() + or self._is_github_copilot_url() + or model_forces_max_completion_tokens(self.model) + ): return {"max_completion_tokens": value} return {"max_tokens": value} diff --git a/tests/agent/test_auxiliary_client.py b/tests/agent/test_auxiliary_client.py index 68355482fe09..86b310ac82c3 100644 --- a/tests/agent/test_auxiliary_client.py +++ b/tests/agent/test_auxiliary_client.py @@ -3791,3 +3791,82 @@ class TestAuxUnhealthyCache: ) # After the 402, OpenRouter is in the unhealthy cache. assert _is_provider_unhealthy("openrouter") is True + + +# ── auxiliary_max_tokens_param ────────────────────────────────────────────── + + +class TestAuxiliaryMaxTokensParam: + """Verify the kwarg emitted by ``auxiliary_max_tokens_param`` across + URL / provider / model-name combinations. Regression cover: a custom + OpenAI-compatible endpoint serving ``gpt-5.x`` was silently getting + ``max_tokens`` and 400-ing on ``unsupported_parameter``.""" + + def test_direct_openai_returns_max_completion_tokens(self): + with ( + patch("agent.auxiliary_client._current_custom_base_url", + return_value="https://api.openai.com/v1"), + patch("agent.auxiliary_client._read_nous_auth", return_value=None), + ): + assert auxiliary_max_tokens_param(4096) == {"max_completion_tokens": 4096} + + def test_local_endpoint_without_model_uses_max_tokens(self): + with ( + patch("agent.auxiliary_client._current_custom_base_url", + return_value="http://localhost:11434/v1"), + patch("agent.auxiliary_client._read_nous_auth", return_value=None), + ): + assert auxiliary_max_tokens_param(4096) == {"max_tokens": 4096} + + def test_openrouter_api_key_present_keeps_max_tokens_without_model_hint(self, monkeypatch): + monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-v1-test") + with ( + patch("agent.auxiliary_client._current_custom_base_url", + return_value="https://openrouter.ai/api/v1"), + patch("agent.auxiliary_client._read_nous_auth", return_value=None), + ): + assert auxiliary_max_tokens_param(4096) == {"max_tokens": 4096} + + # Model-name fallback — this is the regression guard. + + def test_custom_endpoint_serving_gpt5_uses_max_completion_tokens(self): + """Third-party gateway + gpt-5.x: name-based detection must kick in.""" + with ( + patch("agent.auxiliary_client._current_custom_base_url", + return_value="https://my-gateway.example.com/v1"), + patch("agent.auxiliary_client._read_nous_auth", return_value=None), + ): + assert auxiliary_max_tokens_param(4096, model="gpt-5.4") == { + "max_completion_tokens": 4096 + } + + def test_openrouter_serving_gpt4o_uses_max_completion_tokens(self, monkeypatch): + monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-v1-test") + with ( + patch("agent.auxiliary_client._current_custom_base_url", + return_value="https://openrouter.ai/api/v1"), + patch("agent.auxiliary_client._read_nous_auth", return_value=None), + ): + assert auxiliary_max_tokens_param(4096, model="openai/gpt-4o-mini") == { + "max_completion_tokens": 4096 + } + + def test_custom_endpoint_serving_classic_llama_keeps_max_tokens(self): + with ( + patch("agent.auxiliary_client._current_custom_base_url", + return_value="https://my-gateway.example.com/v1"), + patch("agent.auxiliary_client._read_nous_auth", return_value=None), + ): + assert auxiliary_max_tokens_param(4096, model="llama3-70b") == { + "max_tokens": 4096 + } + + def test_empty_model_falls_back_to_url_only(self): + """No model hint → only the URL-based rule applies.""" + with ( + patch("agent.auxiliary_client._current_custom_base_url", + return_value="https://my-gateway.example.com/v1"), + patch("agent.auxiliary_client._read_nous_auth", return_value=None), + ): + assert auxiliary_max_tokens_param(4096, model="") == {"max_tokens": 4096} + assert auxiliary_max_tokens_param(4096, model=None) == {"max_tokens": 4096} diff --git a/tests/run_agent/test_run_agent.py b/tests/run_agent/test_run_agent.py index d215c7b193ad..9d97693aa7f6 100644 --- a/tests/run_agent/test_run_agent.py +++ b/tests/run_agent/test_run_agent.py @@ -5063,6 +5063,41 @@ class TestMaxTokensParam: result = agent._max_tokens_param(4096) assert result == {"max_completion_tokens": 4096} + # ── Model-name fallback for non-openai.com endpoints serving newer families ── + + def test_returns_max_completion_tokens_for_gpt5_on_custom_endpoint(self, agent): + """Custom OpenAI-compatible endpoint serving gpt-5.x must also use + max_completion_tokens — otherwise the server 400s on max_tokens.""" + agent.base_url = "https://my-gateway.example.com/v1" + agent.model = "gpt-5.4" + result = agent._max_tokens_param(4096) + assert result == {"max_completion_tokens": 4096} + + def test_returns_max_completion_tokens_for_gpt4o_on_openrouter(self, agent): + agent.base_url = "https://openrouter.ai/api/v1" + agent.model = "openai/gpt-4o-mini" + result = agent._max_tokens_param(4096) + assert result == {"max_completion_tokens": 4096} + + def test_returns_max_completion_tokens_for_o1_on_custom_endpoint(self, agent): + agent.base_url = "https://custom.example.com/v1" + agent.model = "o1-preview" + result = agent._max_tokens_param(4096) + assert result == {"max_completion_tokens": 4096} + + def test_returns_max_tokens_for_classic_gpt4_on_openrouter(self, agent): + """Classic gpt-4 (non-omni) still uses max_tokens. Don't over-match.""" + agent.base_url = "https://openrouter.ai/api/v1" + agent.model = "openai/gpt-4-turbo" + result = agent._max_tokens_param(4096) + assert result == {"max_tokens": 4096} + + def test_returns_max_tokens_for_llama_on_local(self, agent): + agent.base_url = "http://localhost:11434/v1" + agent.model = "llama3" + result = agent._max_tokens_param(4096) + assert result == {"max_tokens": 4096} + class TestGpt5ApiModeRouting: """Verify provider-specific GPT-5 API-mode routing.""" diff --git a/tests/test_model_forces_max_completion_tokens.py b/tests/test_model_forces_max_completion_tokens.py new file mode 100644 index 000000000000..196dcc5095f7 --- /dev/null +++ b/tests/test_model_forces_max_completion_tokens.py @@ -0,0 +1,137 @@ +"""Targeted tests for ``utils.model_forces_max_completion_tokens``. + +This helper decides whether a given model name requires the newer +``max_completion_tokens`` kwarg (rather than the legacy ``max_tokens``) on +``/v1/chat/completions``. It protects against the 400 ``unsupported_parameter`` +error seen when third-party OpenAI-compatible endpoints serve gpt-4o / 4.1 / +5.x / o-series models by name and the caller only checks the URL host. +""" + +from __future__ import annotations + +from utils import model_forces_max_completion_tokens + + +# ─── Positive cases: families that require max_completion_tokens ──────────── + + +class TestPositiveCases: + def test_gpt_5_bare(self): + assert model_forces_max_completion_tokens("gpt-5") is True + + def test_gpt_5_point_release(self): + # The case the user actually hit — gpt-5.4 on a custom OpenAI-compatible + # endpoint was being sent max_tokens and getting 400 back. + assert model_forces_max_completion_tokens("gpt-5.4") is True + + def test_gpt_5_mini(self): + assert model_forces_max_completion_tokens("gpt-5-mini") is True + + def test_gpt_5_nano(self): + assert model_forces_max_completion_tokens("gpt-5-nano") is True + + def test_gpt_4o(self): + assert model_forces_max_completion_tokens("gpt-4o") is True + + def test_gpt_4o_mini(self): + assert model_forces_max_completion_tokens("gpt-4o-mini") is True + + def test_gpt_4_1(self): + assert model_forces_max_completion_tokens("gpt-4.1") is True + + def test_gpt_4_1_mini(self): + assert model_forces_max_completion_tokens("gpt-4.1-mini") is True + + def test_o1(self): + assert model_forces_max_completion_tokens("o1") is True + + def test_o1_preview(self): + assert model_forces_max_completion_tokens("o1-preview") is True + + def test_o1_mini(self): + assert model_forces_max_completion_tokens("o1-mini") is True + + def test_o3(self): + assert model_forces_max_completion_tokens("o3") is True + + def test_o3_mini(self): + assert model_forces_max_completion_tokens("o3-mini") is True + + def test_o4_mini(self): + # Future-proofing — o4 is already listed publicly. + assert model_forces_max_completion_tokens("o4-mini") is True + + +# ─── Negative cases: older or non-OpenAI families still use max_tokens ────── + + +class TestNegativeCases: + def test_gpt_3_5_turbo(self): + assert model_forces_max_completion_tokens("gpt-3.5-turbo") is False + + def test_gpt_4(self): + # Classic gpt-4 (non-omni) still uses max_tokens on chat completions. + assert model_forces_max_completion_tokens("gpt-4") is False + + def test_gpt_4_turbo(self): + assert model_forces_max_completion_tokens("gpt-4-turbo") is False + + def test_claude_family(self): + assert model_forces_max_completion_tokens("claude-3-opus") is False + assert model_forces_max_completion_tokens("claude-sonnet-4-6") is False + + def test_llama_family(self): + assert model_forces_max_completion_tokens("llama3") is False + assert model_forces_max_completion_tokens("llama-3-70b-instruct") is False + + def test_mistral_family(self): + assert model_forces_max_completion_tokens("mistral-7b-instruct") is False + + def test_qwen_family(self): + assert model_forces_max_completion_tokens("qwen2.5-72b") is False + + def test_deepseek_family(self): + assert model_forces_max_completion_tokens("deepseek-chat") is False + + +# ─── Edge cases ───────────────────────────────────────────────────────────── + + +class TestEdgeCases: + def test_empty_string(self): + assert model_forces_max_completion_tokens("") is False + + def test_none(self): + assert model_forces_max_completion_tokens(None) is False # type: ignore[arg-type] + + def test_whitespace_only(self): + assert model_forces_max_completion_tokens(" ") is False + + def test_case_insensitive(self): + assert model_forces_max_completion_tokens("GPT-5.4") is True + assert model_forces_max_completion_tokens("Gpt-4o-Mini") is True + assert model_forces_max_completion_tokens("O3-MINI") is True + + def test_leading_trailing_whitespace(self): + assert model_forces_max_completion_tokens(" gpt-5 ") is True + + def test_vendor_prefix_stripped(self): + # OpenRouter-style "vendor/model" names should match the tail. + assert model_forces_max_completion_tokens("openai/gpt-5.4") is True + assert model_forces_max_completion_tokens("openai/gpt-4o-mini") is True + assert model_forces_max_completion_tokens("openai/o3-mini") is True + + def test_vendor_prefix_with_non_matching_tail(self): + assert model_forces_max_completion_tokens("openai/gpt-3.5-turbo") is False + assert model_forces_max_completion_tokens("anthropic/claude-3-opus") is False + + def test_fake_prefix_not_matched(self): + # "o-series-but-not-really" doesn't start with o1/o3/o4. + assert model_forces_max_completion_tokens("omni-chat") is False + # "ox" isn't an o-series model, and "olive" / "opus" shouldn't collide. + assert model_forces_max_completion_tokens("ox-large") is False + assert model_forces_max_completion_tokens("opus-3") is False + + def test_gpt_5_substring_in_middle_not_matched(self): + # Only a prefix should match — "local-gpt-5-clone" is a different model. + assert model_forces_max_completion_tokens("local-gpt-5-clone") is False diff --git a/utils.py b/utils.py index b2c9f1e83ff2..4ab94f18612a 100644 --- a/utils.py +++ b/utils.py @@ -355,6 +355,44 @@ def base_url_hostname(base_url: str) -> str: return (parsed.hostname or "").lower().rstrip(".") +# ─── Model Capability Detection ────────────────────────────────────────────── + + +def model_forces_max_completion_tokens(model: str) -> bool: + """Return True for model families that require ``max_completion_tokens``. + + OpenAI's newer families reject ``max_tokens`` on /v1/chat/completions with + HTTP 400 ``unsupported_parameter`` — the caller must send + ``max_completion_tokens`` instead. This covers: + + - ``gpt-4o`` / ``gpt-4o-mini`` / ``gpt-4o-*`` + - ``gpt-4.1`` / ``gpt-4.1-*`` + - ``gpt-5`` / ``gpt-5.x`` / ``gpt-5-*`` + - ``o1`` / ``o1-*`` + - ``o3`` / ``o3-*`` + - ``o4`` / ``o4-*`` + + Handles vendor prefixes like ``openai/gpt-5.4`` by stripping to the tail. + The URL-based check (``base_url_hostname == "api.openai.com"``) misses + third-party OpenAI-compatible endpoints (custom OpenAI gateways, + OpenRouter) that front these models and enforce the same parameter + constraint, so name-based detection is required as a fallback. + """ + m = (model or "").strip().lower() + if not m: + return False + if "/" in m: + m = m.rsplit("/", 1)[-1] + return ( + m.startswith("gpt-4o") + or m.startswith("gpt-4.1") + or m.startswith("gpt-5") + or m.startswith("o1") + or m.startswith("o3") + or m.startswith("o4") + ) + + def base_url_host_matches(base_url: str, domain: str) -> bool: """Return True when the base URL's hostname is ``domain`` or a subdomain.