diff --git a/agent/agent_init.py b/agent/agent_init.py index f608e95084..97b3349123 100644 --- a/agent/agent_init.py +++ b/agent/agent_init.py @@ -23,6 +23,7 @@ from typing import Any, Callable, Dict, List, Optional from urllib.parse import parse_qs, urlparse, urlunparse from agent.context_compressor import ContextCompressor +from agent.agent_init_fallback import _fallback_entries, _init_fallback_chain, recompute_init_fallback_api_mode from agent.agent_runtime_helpers import _ra from agent.iteration_budget import IterationBudget, normalize_budget_warning_ratio from agent.memory_manager import StreamingContextScrubber @@ -899,6 +900,7 @@ def _routed_client_kwargs(agent, fallback_model, _provider_timeout) -> Optional[ return None agent.provider = agent.requested_provider = _fb["provider"] agent.model = _fb_model or _fb["model"] + recompute_init_fallback_api_mode(agent, _fb_client) return _client_kwargs_from_routed(_fb_client, _provider_timeout) # A burned credential pool (#119533) is otherwise indistinguishable from missing config, # so name it even when no fallback entries are configured. @@ -1088,38 +1090,6 @@ def _client_kwargs_from_routed(client, timeout) -> Dict[str, Any]: return kwargs -def _fallback_entries(fallback_model) -> List[Dict[str, Any]]: - """Normalize legacy single-dict ``fallback_model`` / list ``fallback_providers``.""" - if isinstance(fallback_model, dict): - fallback_model = [fallback_model] - if not isinstance(fallback_model, list): - return [] - return [ - f for f in fallback_model if isinstance(f, dict) and f.get("provider") and f.get("model") - ] - - -def _init_fallback_chain(agent, fallback_model): - # Stable pool-entry identity: OAuth refreshes can replace the token before a failed - # request is recovered, so the key value alone can't attribute the failure. - from agent.agent_runtime_helpers import sync_credential_pool_entry_id - sync_credential_pool_entry_id(agent) - - # Ordered backups tried when the primary is exhausted (legacy single-dict or list). - agent._fallback_chain = _fallback_entries(fallback_model) - agent._fallback_index = 0 - agent._fallback_activated = getattr(agent, "_fallback_activated", False) - # Legacy attribute kept for backward compat (tests, external callers) - agent._fallback_model = agent._fallback_chain[0] if agent._fallback_chain else None - chain = agent._fallback_chain - if chain and not agent.quiet_mode: - labels = [f"{f['model']} ({f['provider']})" for f in chain] - if len(chain) == 1: - print(f"🔄 Fallback model: {labels[0]}") - else: - print(f"🔄 Fallback chain ({len(chain)} providers): " + " → ".join(labels)) - - def _load_tools(agent, enabled_toolsets, disabled_toolsets): # A feature that left core for a catalog plugin (Home Assistant) is installed for a home that # used it, once per process, before discovery so its tools are in this agent's snapshot. diff --git a/agent/agent_init_fallback.py b/agent/agent_init_fallback.py new file mode 100644 index 0000000000..157138225c --- /dev/null +++ b/agent/agent_init_fallback.py @@ -0,0 +1,61 @@ +"""Init-time fallback chain: entry normalization, chain setup, and the fallback's api_mode.""" + +from __future__ import annotations + +import logging +from typing import Any, Dict, List + +logger = logging.getLogger("run_agent") + + +def _fallback_entries(fallback_model) -> List[Dict[str, Any]]: + """Normalize legacy single-dict ``fallback_model`` / list ``fallback_providers``.""" + if isinstance(fallback_model, dict): + fallback_model = [fallback_model] + if not isinstance(fallback_model, list): + return [] + return [ + f for f in fallback_model if isinstance(f, dict) and f.get("provider") and f.get("model") + ] + + +def _init_fallback_chain(agent, fallback_model): + # Stable pool-entry identity: OAuth refreshes can replace the token before a failed + # request is recovered, so the key value alone can't attribute the failure. + from agent.agent_runtime_helpers import sync_credential_pool_entry_id + sync_credential_pool_entry_id(agent) + + # Ordered backups tried when the primary is exhausted (legacy single-dict or list). + agent._fallback_chain = _fallback_entries(fallback_model) + agent._fallback_index = 0 + agent._fallback_activated = getattr(agent, "_fallback_activated", False) + # Legacy attribute kept for backward compat (tests, external callers) + agent._fallback_model = agent._fallback_chain[0] if agent._fallback_chain else None + chain = agent._fallback_chain + if chain and not agent.quiet_mode: + labels = [f"{f['model']} ({f['provider']})" for f in chain] + if len(chain) == 1: + print(f"🔄 Fallback model: {labels[0]}") + else: + print(f"🔄 Fallback chain ({len(chain)} providers): " + " → ".join(labels)) + + +def recompute_init_fallback_api_mode(agent, fb_client) -> None: + """Give an init-time fallback its own api_mode instead of the unreachable primary's. + + A Nous primary configured with ``api_mode: codex_responses`` otherwise leaves a Copilot + gpt-5-mini fallback on the Responses API, which silently drops reasoning content (#46527). + The per-turn path (``try_activate_fallback``) already recomputes via + ``_fallback_api_mode_resolved``; this keeps init in agreement, and runs before the + ``_primary_runtime`` snapshot so ``restore_primary_runtime`` replays the recomputed mode. + A detection failure keeps the primary's mode and never blocks the fallback itself. + """ + from agent.chat_completion_helpers import _fallback_api_mode_resolved + try: + agent.api_mode = _fallback_api_mode_resolved( + agent, agent.provider, agent.model, str(getattr(fb_client, "base_url", "") or "")) + except Exception: + logger.debug("Init-time fallback api_mode detection failed for %s", agent.provider, exc_info=True) + return + if hasattr(agent, "_transport_cache"): + agent._transport_cache.clear() diff --git a/agent/transports/codex.py b/agent/transports/codex.py index 3cbb0b632b..82c7def219 100644 --- a/agent/transports/codex.py +++ b/agent/transports/codex.py @@ -591,7 +591,15 @@ def _reasoning_fields( elif enabled: if is_github_responses: if params.get("github_reasoning_extra") is not None: - fields["reasoning"] = params["github_reasoning_extra"] + # Request a reasoning summary so the Responses API actually returns + # reasoning text. Without ``summary: "auto"`` the GitHub/Copilot + # /responses endpoint returns reasoning items with no summary, so Hermes + # persists empty reasoning/thinking content for every Copilot model on + # the Responses API (gpt-5.4, gpt-5, ...) — the same loss the non-GitHub + # branch below already avoids. Verified against api.githubcopilot.com + # /responses: the summary is only emitted when summary="auto" is sent. + # See #46527. + fields["reasoning"] = {**params["github_reasoning_extra"], "summary": "auto"} else: fields["reasoning"] = {"effort": effort, "summary": "auto"} fields["include"] = include diff --git a/hermes_cli/runtime_provider.py b/hermes_cli/runtime_provider.py index 2e1f9d2630..74ba470939 100644 --- a/hermes_cli/runtime_provider.py +++ b/hermes_cli/runtime_provider.py @@ -224,6 +224,18 @@ def _effective_model(model_cfg: Dict[str, Any], target_model: Optional[str]) -> def _copilot_runtime_api_mode(model_cfg: Dict[str, Any], api_key: str, *, target_model: Optional[str] = None) -> str: configured_mode = _configured_api_mode("copilot", model_cfg) if configured_mode: + # A stale/incompatible explicit ``codex_responses`` must not override Copilot's hard + # per-model requirement. Copilot serves ``gpt-5-mini`` on chat completions only; a + # Responses-API request there silently succeeds but returns no reasoning/thinking + # content (#46527). The stale value commonly survives an ordinary flow: the Copilot + # OAuth setup writes ``api_mode: codex_responses`` (correct for the gpt-5.4-mini it + # initially selects), then the user switches to gpt-5-mini without the mode being + # recomputed. The check is the pure-regex Copilot exception — no network call; a + # legitimate ``codex_responses`` for GPT-5 variants like gpt-5.4-mini is still honored. + model_name = str(_effective_model(model_cfg, target_model)).strip() + if (configured_mode == "codex_responses" and model_name + and not _models._should_use_copilot_responses_api(model_name)): + return "chat_completions" return configured_mode # Use the model being resolved, not the persisted default: a Claude MoA slot inheriting # codex_responses from a GPT-5 default fails with "model ... does not support Responses API". diff --git a/tests/agent/test_init_fallback_on_exhausted_pool.py b/tests/agent/test_init_fallback_on_exhausted_pool.py index 53897ce8d0..98bcb53f57 100644 --- a/tests/agent/test_init_fallback_on_exhausted_pool.py +++ b/tests/agent/test_init_fallback_on_exhausted_pool.py @@ -131,3 +131,79 @@ def test_init_openrouter_exhausted_without_chain_keeps_generic_error(): fallback_model=[{"provider": "openrouter", "model": "poolside/laguna-s-2.1:free"}], ) + + +def test_init_fallback_recomputes_api_mode_for_copilot_gpt5_mini(): + """#46527: Nous primary (api_mode=codex_responses, no creds) -> Copilot gpt-5-mini + fallback must end up with api_mode=chat_completions, not codex_responses. + + Copilot's gpt-5-mini is the documented exception that uses Chat Completions, + not the Responses API. Before the fix, ``agent.api_mode`` stayed + ``"codex_responses"`` after the init-time fallback activated, so every request + for the fallback model went out via the Responses API and silently returned no + reasoning/thinking content. + """ + fb_client = _mock_client(base_url="https://api.githubcopilot.com") + + def fake_resolve(provider, model=None, raw_codex=False, + explicit_base_url=None, explicit_api_key=None): + if provider == "copilot": + return fb_client, "gpt-5-mini" + return None, None # primary (nous) has no usable credentials + + with patch("agent.auxiliary_client.resolve_provider_client", side_effect=fake_resolve), \ + patch("model_tools.get_tool_definitions", return_value=_make_tool_defs()), \ + patch("model_tools.check_toolset_requirements", return_value={}), \ + patch("agent.process_bootstrap.OpenAI", return_value=MagicMock()): + + agent = AIAgent( + provider="nous", + model="gpt-5.4-mini", + api_mode="codex_responses", + api_key=None, + base_url=None, + quiet_mode=True, + skip_context_files=True, + skip_memory=True, + fallback_model=[{"provider": "copilot", "model": "gpt-5-mini"}], + ) + + assert agent.provider == "copilot" + assert agent.model == "gpt-5-mini" + assert agent._fallback_activated is True + assert agent.api_mode == "chat_completions" + + +def test_init_fallback_keeps_codex_responses_for_openai_codex(): + """#46527: Nous primary (api_mode=codex_responses, no creds) -> openai-codex + fallback must keep api_mode=codex_responses (openai-codex always uses the + Responses API).""" + fb_client = _mock_client(base_url="https://chatgpt.com/backend-api/codex") + + def fake_resolve(provider, model=None, raw_codex=False, + explicit_base_url=None, explicit_api_key=None): + if provider == "openai-codex": + return fb_client, "gpt-5.4-mini" + return None, None + + with patch("agent.auxiliary_client.resolve_provider_client", side_effect=fake_resolve), \ + patch("model_tools.get_tool_definitions", return_value=_make_tool_defs()), \ + patch("model_tools.check_toolset_requirements", return_value={}), \ + patch("agent.process_bootstrap.OpenAI", return_value=MagicMock()): + + agent = AIAgent( + provider="nous", + model="gpt-5.4-mini", + api_mode="codex_responses", + api_key=None, + base_url=None, + quiet_mode=True, + skip_context_files=True, + skip_memory=True, + fallback_model=[{"provider": "openai-codex", "model": "gpt-5.4-mini"}], + ) + + assert agent.provider == "openai-codex" + assert agent.model == "gpt-5.4-mini" + assert agent._fallback_activated is True + assert agent.api_mode == "codex_responses" diff --git a/tests/agent/transports/test_codex_transport.py b/tests/agent/transports/test_codex_transport.py index 64c1efc009..5ed2d539c2 100644 --- a/tests/agent/transports/test_codex_transport.py +++ b/tests/agent/transports/test_codex_transport.py @@ -53,7 +53,7 @@ class TestCodexBuildKwargs: reasoning_config=reasoning, github_reasoning_extra=ReasoningParamsMixin._github_models_reasoning_extra_body(agent), ) - assert kw.get("reasoning") == ({"effort": expected} if expected else None) + assert kw.get("reasoning") == ({"effort": expected, "summary": "auto"} if expected else None) def test_astra_direct_request_applies_model_contract_after_overrides(self, transport): kw = transport.build_kwargs( diff --git a/tests/agent/transports/test_codex_transport_github_reasoning.py b/tests/agent/transports/test_codex_transport_github_reasoning.py new file mode 100644 index 0000000000..40f6329bda --- /dev/null +++ b/tests/agent/transports/test_codex_transport_github_reasoning.py @@ -0,0 +1,25 @@ +"""GitHub/Copilot Responses reasoning fields on the Codex transport. + +Regression for #46527: the GitHub Responses branch must request ``summary: "auto"`` so +Copilot returns reasoning text. Without it, reasoning items come back with no summary and +Hermes persists no reasoning/thinking content. +""" + +import pytest + +from agent.transports import get_transport + + +@pytest.fixture +def transport(): + import agent.transports.codex # noqa: F401 + return get_transport("codex_responses") + + +def test_github_responses_requests_reasoning_summary(transport): + kw = transport.build_kwargs( + model="gpt-5.4", messages=[{"role": "user", "content": "Hi"}], tools=[], + is_github_responses=True, + github_reasoning_extra={"effort": "medium"}, + ) + assert kw.get("reasoning") == {"effort": "medium", "summary": "auto"} diff --git a/tests/fixtures/provider_wire_snapshot.json b/tests/fixtures/provider_wire_snapshot.json index e313644a4f..2835c60042 100644 --- a/tests/fixtures/provider_wire_snapshot.json +++ b/tests/fixtures/provider_wire_snapshot.json @@ -2773,7 +2773,8 @@ "model": "gpt-5.5", "parallel_tool_calls": true, "reasoning": { - "effort": "high" + "effort": "high", + "summary": "auto" }, "store": false, "timeout": 1800.0, @@ -2877,7 +2878,8 @@ "model": "gpt-5.5", "parallel_tool_calls": true, "reasoning": { - "effort": "medium" + "effort": "medium", + "summary": "auto" }, "store": false, "timeout": 1800.0, diff --git a/tests/hermes_cli/test_copilot_runtime_api_mode_stale.py b/tests/hermes_cli/test_copilot_runtime_api_mode_stale.py new file mode 100644 index 0000000000..9302c65693 --- /dev/null +++ b/tests/hermes_cli/test_copilot_runtime_api_mode_stale.py @@ -0,0 +1,53 @@ +"""Regression tests for #46527: Copilot primary api_mode resolution must not +honor a stale/incompatible explicit ``api_mode``. + +The gateway resolves and snapshots the primary provider/model at startup via +``resolve_runtime_provider`` -> ``_copilot_runtime_api_mode``. When the config +carries a stale ``api_mode: codex_responses`` (e.g. the Copilot OAuth setup +wrote it for the ``gpt-5.4-mini`` it initially selects, then the user switched +to ``gpt-5-mini``) the old code honored the stale ``codex_responses`` verbatim. +That dispatches gpt-5-mini through the Responses API — which Copilot serves on +chat completions only — and the response comes back with no reasoning/thinking +content (the symptom in #46527). +""" +from hermes_cli.runtime_provider import _copilot_runtime_api_mode + + +def test_stale_codex_responses_vetoed_for_gpt5_mini(): + """gpt-5-mini + stale api_mode=codex_responses must resolve to chat_completions.""" + cfg = {"provider": "copilot", "default": "gpt-5-mini", "api_mode": "codex_responses"} + assert _copilot_runtime_api_mode(cfg, api_key="x") == "chat_completions" + + +def test_explicit_codex_responses_kept_for_gpt5_variant(): + """gpt-5.4-mini legitimately uses the Responses API on Copilot — an explicit + codex_responses must still be honored (no over-correction).""" + cfg = {"provider": "copilot", "default": "gpt-5.4-mini", "api_mode": "codex_responses"} + assert _copilot_runtime_api_mode(cfg, api_key="x") == "codex_responses" + + +def test_no_explicit_mode_derives_chat_completions_for_gpt5_mini(): + """Without an explicit api_mode, gpt-5-mini derives chat_completions (unchanged).""" + cfg = {"provider": "copilot", "default": "gpt-5-mini"} + assert _copilot_runtime_api_mode(cfg, api_key="x") == "chat_completions" + + +def test_explicit_chat_completions_unchanged(): + """An explicit chat_completions stays chat_completions for gpt-5-mini.""" + cfg = {"provider": "copilot", "default": "gpt-5-mini", "api_mode": "chat_completions"} + assert _copilot_runtime_api_mode(cfg, api_key="x") == "chat_completions" + + +def test_stale_mode_from_different_provider_not_honored(): + """When the config's provider doesn't match copilot, the explicit mode is + not honored at all (existing _provider_supports_explicit_api_mode guard), + so gpt-5-mini still derives chat_completions.""" + cfg = {"provider": "nous", "default": "gpt-5-mini", "api_mode": "codex_responses"} + assert _copilot_runtime_api_mode(cfg, api_key="x") == "chat_completions" + + +def test_target_model_beats_stale_default_mode(): + """A /model switch to gpt-5-mini must not inherit a stale codex_responses + recorded for the persisted default.""" + cfg = {"provider": "copilot", "default": "gpt-5.4-mini", "api_mode": "codex_responses"} + assert _copilot_runtime_api_mode(cfg, api_key="x", target_model="gpt-5-mini") == "chat_completions"