Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions agent/auxiliary_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -3420,13 +3420,14 @@ def resolve_provider_client(
# with grok-4.3 configured gets grok-4.3 for title generation
# instead of silently dropping to whatever Step-2 fallback (#31845).
#
# Each provider branch below sees a non-empty ``model`` whenever the
# user has *anything* configured — no provider-specific empty-model
# guards needed. When the user has NOTHING configured (fresh install,
# main_model also empty), the branches still hit their own
# missing-credentials returns and ``_resolve_auto`` falls through to
# the Step-2 chain as before.
if not model:
# Each explicit provider branch below sees a non-empty ``model`` whenever
# the user has *anything* configured — no provider-specific empty-model
# guards needed. The ``auto`` branch is the exception: it must let
# ``_resolve_auto(main_runtime=...)`` choose the matched provider/model pair
# from the live runtime. Falling back to ``_read_main_model()`` here can
# cross a stale config/default model (e.g. Opus) onto a newly failed-over
# provider client (e.g. Codex), producing provider+model mismatches.
if provider != "auto" and not model:
model = _get_aux_model_for_provider(provider) or _read_main_model() or model

def _needs_codex_wrap(client_obj, base_url_str: str, model_str: str) -> bool:
Expand Down
58 changes: 58 additions & 0 deletions tests/agent/test_auxiliary_main_first.py
Original file line number Diff line number Diff line change
Expand Up @@ -540,3 +540,61 @@ def test_runtime_provider_without_model_does_not_borrow_global_model(self):
# Opus model was NOT borrowed onto the codex provider.
mock_resolve.assert_not_called()
assert client is None


class TestResolveProviderClientAutoRuntimeModel:
"""The outer resolver must not reintroduce stale main-config models."""

def test_auto_provider_does_not_let_stale_config_model_override_runtime_model(self):
"""``provider=auto`` uses ``_resolve_auto``'s runtime pair, not ``_read_main_model``.

Regression for a production compression failure after mid-session
fallback: ``_resolve_auto(main_runtime=...)`` correctly selected the
Codex runtime pair (openai-codex / gpt-5.5), but
``resolve_provider_client`` had already filled ``model`` from stale
process/config state (claude-opus-4-8) and then returned Codex client +
stale Opus model. Codex rejected that wire shape with HTTP 400.
"""
codex_client = MagicMock()

with patch(
"agent.auxiliary_client._get_aux_model_for_provider", return_value=None,
), patch(
"agent.auxiliary_client._read_main_model", return_value="claude-opus-4-8",
), patch(
"agent.auxiliary_client._resolve_auto", return_value=(codex_client, "gpt-5.5"),
) as mock_resolve_auto:
from agent.auxiliary_client import resolve_provider_client

client, model = resolve_provider_client(
"auto",
None,
False,
main_runtime={"provider": "openai-codex", "model": "gpt-5.5"},
)

assert client is codex_client
mock_resolve_auto.assert_called_once_with(
main_runtime={"provider": "openai-codex", "model": "gpt-5.5"}
)
assert model == "gpt-5.5"
assert model != "claude-opus-4-8"

def test_auto_provider_preserves_explicit_model_override(self):
"""A real caller-supplied model still wins over the auto-resolved model."""
codex_client = MagicMock()

with patch(
"agent.auxiliary_client._resolve_auto", return_value=(codex_client, "gpt-5.5"),
):
from agent.auxiliary_client import resolve_provider_client

client, model = resolve_provider_client(
"auto",
"gpt-5.4-mini",
False,
main_runtime={"provider": "openai-codex", "model": "gpt-5.5"},
)

assert client is codex_client
assert model == "gpt-5.4-mini"
Comment on lines +583 to +600

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Second test does not assert main_runtime forwarding to _resolve_auto

test_auto_provider_preserves_explicit_model_override verifies that a caller-supplied model wins over the auto-resolved one, which is correct. However, it doesn't assert that _resolve_auto was actually called with the right main_runtime. If a future refactor accidentally dropped main_runtime from the inner call, this test would still pass. The first test (test_auto_provider_does_not_let_stale_config_model_override_runtime_model) includes mock_resolve_auto.assert_called_once_with(main_runtime=...) — adding the same assertion here would close the gap.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Loading