diff --git a/src/benchflow/acp/runtime.py b/src/benchflow/acp/runtime.py index a4cd802a8..84e27348d 100644 --- a/src/benchflow/acp/runtime.py +++ b/src/benchflow/acp/runtime.py @@ -28,6 +28,7 @@ from benchflow.acp.selection import selected_acp_transport from benchflow.acp.types import McpServerSpec from benchflow.acp.watchdog import IdleWatchdog +from benchflow.agents.codex_config import apply_codex_launch_config from benchflow.agents.protocol import ACPSessionAdapter from benchflow.agents.providers import ( find_provider, @@ -329,6 +330,8 @@ def _model_selection_owned_by_env( agent: str, model: str | None, agent_env: dict[str, str], + *, + launch_config_owns_model: bool = False, ) -> bool: """Return True when launch/env config should own model selection. @@ -350,7 +353,9 @@ def _model_selection_owned_by_env( # fall back to their own defaults. if agent_env.get("BENCHFLOW_LITELLM_MODEL_VIA_ENV") in {"1", "true", "True"}: mapped_model_env = agent_cfg.env_mapping.get("BENCHFLOW_PROVIDER_MODEL") - return bool(mapped_model_env and agent_env.get(mapped_model_env)) + if mapped_model_env and agent_env.get(mapped_model_env): + return True + return launch_config_owns_model if agent_env.get("BENCHFLOW_LITELLM_MODEL_ALIAS"): return False provider = find_provider(model) @@ -495,11 +500,18 @@ async def _configure_acp_session( model: str | None, agent_env: dict[str, str], reasoning_effort: str | None, + launch_config_owns_model: bool = False, ) -> None: agent_cfg = AGENTS.get(agent) - effort_in_model_id = False + effort_configured = False - if model and _model_selection_owned_by_env(agent, model, agent_env): + if model and _model_selection_owned_by_env( + agent, + model, + agent_env, + launch_config_owns_model=launch_config_owns_model, + ): + effort_configured = bool(reasoning_effort and launch_config_owns_model) logger.info( f"Skipping ACP model configuration for {agent} — launch/env config owns model selection" ) @@ -508,7 +520,7 @@ async def _configure_acp_session( acp_model_id = _select_acp_model_id( acp_model_input, agent, session, reasoning_effort ) - effort_in_model_id = bool( + effort_configured = bool( reasoning_effort and agent == "codex-acp" and _codex_reasoning_effort(acp_model_id) == reasoning_effort @@ -538,12 +550,11 @@ async def _configure_acp_session( if not reasoning_effort: return - if effort_in_model_id: + if effort_configured: # The effort already rides the selected ``model[effort]`` id (codex), - # so there is nothing further to configure. + # or its launch config, so there is nothing further to configure. logger.info( - f"Reasoning effort {reasoning_effort!r} applied via the model id " - f"for {agent}" + f"Reasoning effort {reasoning_effort!r} applied with model selection for {agent}" ) return if not agent_cfg or not agent_cfg.acp_effort_config_id: @@ -591,6 +602,12 @@ async def connect_acp( Retries with exponential backoff on ConnectionError (Daytona SSH storms). """ + agent_env, launch_config_owns_model = apply_codex_launch_config( + agent, + agent_env, + model=model, + reasoning_effort=reasoning_effort, + ) agent_env = await _prepare_openhands_direct_execution( env, agent=agent, @@ -687,6 +704,7 @@ async def connect_acp( model=model, agent_env=agent_env, reasoning_effort=reasoning_effort, + launch_config_owns_model=launch_config_owns_model, ) await enforce_agent_egress_firewall(env, sandbox_user, agent_env) except Exception: diff --git a/src/benchflow/agents/codex_config.py b/src/benchflow/agents/codex_config.py index 7dc27e69f..42260750e 100644 --- a/src/benchflow/agents/codex_config.py +++ b/src/benchflow/agents/codex_config.py @@ -5,11 +5,33 @@ import json from typing import Any +from benchflow.providers.litellm_config import safe_model_alias + CODEX_CONFIG_ENV = "CODEX_CONFIG" CODEX_DEFAULT_AUTH_REQUEST_ENV = "DEFAULT_AUTH_REQUEST" CODEX_MODEL_PROVIDER_ENV = "MODEL_PROVIDER" _CODEX_PROVIDER_ID_PREFIX = "benchflow-" +_LITELLM_MODEL_VIA_ENV = "BENCHFLOW_LITELLM_MODEL_VIA_ENV" +_PROVIDER_MODEL_ENV = "BENCHFLOW_PROVIDER_MODEL" + + +def _parse_codex_config( + raw_config: str | None, *, strict: bool = False +) -> dict[str, Any] | None: + if not raw_config: + return {} + try: + config = json.loads(raw_config) + except (json.JSONDecodeError, TypeError) as exc: + if strict: + raise ValueError(f"{CODEX_CONFIG_ENV} must be valid JSON") from exc + return None + if not isinstance(config, dict): + if strict: + raise ValueError(f"{CODEX_CONFIG_ENV} must decode to a JSON object") + return None + return config def codex_provider_id(provider_name: str | None) -> str: @@ -29,19 +51,8 @@ def apply_codex_provider_config( strict: bool = False, ) -> None: """Create or update Codex's model provider entry in ``agent_env``.""" - raw_config = agent_env.get(CODEX_CONFIG_ENV) - if not raw_config: - config: dict[str, Any] = {} - else: - try: - config = json.loads(raw_config) - except json.JSONDecodeError as exc: - if strict: - raise ValueError(f"{CODEX_CONFIG_ENV} must be valid JSON") from exc - return - if not isinstance(config, dict): - if strict: - raise ValueError(f"{CODEX_CONFIG_ENV} must decode to a JSON object") + config = _parse_codex_config(agent_env.get(CODEX_CONFIG_ENV), strict=strict) + if config is None: return provider_id = ( @@ -74,6 +85,36 @@ def apply_codex_provider_config( ) +def apply_codex_launch_config( + agent: str, + agent_env: dict[str, str], + *, + model: str | None, + reasoning_effort: str | None, +) -> tuple[dict[str, str], bool]: + """Apply launch-owned effort and report whether config owns model selection.""" + if agent != "codex-acp": + return agent_env, False + config = _parse_codex_config(agent_env.get(CODEX_CONFIG_ENV)) + provider_model = agent_env.get(_PROVIDER_MODEL_ENV) + owns_model = bool( + model + and agent_env.get(_LITELLM_MODEL_VIA_ENV) in {"1", "true", "True"} + and provider_model + and provider_model == safe_model_alias(model) + and config is not None + and config.get("model") == provider_model + ) + if not owns_model or not reasoning_effort: + return agent_env, owns_model + + assert config is not None + updated_env = dict(agent_env) + config["model_reasoning_effort"] = reasoning_effort + updated_env[CODEX_CONFIG_ENV] = json.dumps(config, separators=(",", ":")) + return updated_env, True + + def _apply_codex_default_auth_request( agent_env: dict[str, str], *, diff --git a/tests/agents/test_codex_config.py b/tests/agents/test_codex_config.py new file mode 100644 index 000000000..77f17d77b --- /dev/null +++ b/tests/agents/test_codex_config.py @@ -0,0 +1,63 @@ +"""Codex launch configuration ownership tests.""" + +import json + +import pytest + +from benchflow.agents.codex_config import apply_codex_launch_config + + +@pytest.mark.parametrize( + "raw_config", + [None, "{", "[]", "{}", '{"model":"another-model"}'], + ids=["missing", "malformed", "non-object", "missing-model", "mismatch"], +) +def test_launch_config_rejects_missing_invalid_or_mismatched_model(raw_config): + """Guards PR #1076: only exact valid Codex config owns model selection.""" + agent_env = { + "BENCHFLOW_PROVIDER_MODEL": "benchflow-openai-gpt-5.4-mini", + "BENCHFLOW_LITELLM_MODEL_VIA_ENV": "1", + } + if raw_config is not None: + agent_env["CODEX_CONFIG"] = raw_config + + updated_env, owns_model = apply_codex_launch_config( + "codex-acp", agent_env, model="openai/gpt-5.4-mini", reasoning_effort="high" + ) + + assert updated_env is agent_env + assert not owns_model + + +def test_launch_config_applies_effort_to_exact_model(): + """Guards PR #1076: launch-owned model carries requested effort.""" + agent_env = { + "BENCHFLOW_PROVIDER_MODEL": "benchflow-openai-gpt-5.4-mini", + "BENCHFLOW_LITELLM_MODEL_VIA_ENV": "1", + "CODEX_CONFIG": '{"model":"benchflow-openai-gpt-5.4-mini"}', + } + + updated_env, owns_model = apply_codex_launch_config( + "codex-acp", agent_env, model="openai/gpt-5.4-mini", reasoning_effort="high" + ) + + assert owns_model + assert json.loads(updated_env["CODEX_CONFIG"])["model_reasoning_effort"] == "high" + assert updated_env is not agent_env + assert "model_reasoning_effort" not in agent_env["CODEX_CONFIG"] + + +def test_launch_config_rejects_alias_for_a_different_requested_model(): + """Guards PR #1076: stale proxy aliases cannot claim requested-model ownership.""" + agent_env = { + "BENCHFLOW_PROVIDER_MODEL": "benchflow-openai-gpt-5.4-mini", + "BENCHFLOW_LITELLM_MODEL_VIA_ENV": "1", + "CODEX_CONFIG": '{"model":"benchflow-openai-gpt-5.4-mini"}', + } + + updated_env, owns_model = apply_codex_launch_config( + "codex-acp", agent_env, model="openai/gpt-5.5", reasoning_effort="high" + ) + + assert updated_env is agent_env + assert not owns_model diff --git a/tests/test_acp_model_config_dispatch.py b/tests/test_acp_model_config_dispatch.py index 26f53f805..bf3f60a4e 100644 --- a/tests/test_acp_model_config_dispatch.py +++ b/tests/test_acp_model_config_dispatch.py @@ -11,6 +11,7 @@ """ import contextlib +import json from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -43,10 +44,12 @@ def _make_mocks(config_options=None, model_state=None): @contextlib.contextmanager def _runtime_patches(mock_acp): with ( - patch("benchflow.acp.runtime.ContainerTransport", return_value=MagicMock()), + patch( + "benchflow.acp.runtime.ContainerTransport", return_value=MagicMock() + ) as transport, patch("benchflow.acp.runtime.ACPClient", return_value=mock_acp), ): - yield + yield transport async def _connect( @@ -54,7 +57,7 @@ async def _connect( ): from benchflow.acp.runtime import connect_acp - with _runtime_patches(mock_acp): + with _runtime_patches(mock_acp) as transport: await connect_acp( env=AsyncMock(), agent=agent, @@ -67,6 +70,7 @@ async def _connect( agent_cwd="/app", reasoning_effort=reasoning_effort, ) + return transport @pytest.mark.asyncio @@ -81,11 +85,11 @@ async def test_codex_with_only_fastmode_option_uses_set_model(tmp_path): @pytest.mark.asyncio -async def test_codex_litellm_alias_uses_bare_model_for_set_model(tmp_path): +async def test_codex_litellm_config_mismatch_uses_bare_model_for_set_model(tmp_path): """Codex validates set_model against its own model catalog, not proxy aliases. - This guards against a false-green CI path where BenchFlow recorded the - requested model but codex-acp fell back to its own default at request time. + Guards PR #1076 against a false green where BenchFlow records the requested + model but codex-acp falls back to its own default at request time. """ mock_acp = _make_mocks( config_options=[{"id": "fast-mode"}], @@ -97,19 +101,48 @@ async def test_codex_litellm_alias_uses_bare_model_for_set_model(tmp_path): "currentModelId": "gpt-5.5[medium]", }, ) + agent_env = { + "BENCHFLOW_PROVIDER_MODEL": "benchflow-openai-gpt-5.4-mini", + LITELLM_MODEL_ALIAS_ENV: "benchflow-openai-gpt-5.4-mini", + LITELLM_MODEL_VIA_ENV: "1", + "CODEX_CONFIG": '{"model":"another-model"}', + } await _connect( mock_acp, agent="codex-acp", model="openai/gpt-5.4-mini", tmp_path=tmp_path, + agent_env=agent_env, + ) + + mock_acp.set_model.assert_awaited_once_with("gpt-5.4-mini[medium]") + mock_acp.set_config_option.assert_not_awaited() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("reasoning_effort", [None, "high"]) +async def test_codex_litellm_config_owns_model_selection(tmp_path, reasoning_effort): + """Guards PR #1076: exact launch config owns Codex model and effort.""" + alias = "benchflow-openai-gpt-5.4" + mock_acp = _make_mocks(config_options=[{"id": "fast-mode"}]) + transport = await _connect( + mock_acp, + agent="codex-acp", + model="openai/gpt-5.4", + tmp_path=tmp_path, + reasoning_effort=reasoning_effort, agent_env={ - "BENCHFLOW_PROVIDER_MODEL": "benchflow-openai-gpt-5.4-mini", - LITELLM_MODEL_ALIAS_ENV: "benchflow-openai-gpt-5.4-mini", + "BENCHFLOW_PROVIDER_MODEL": alias, + LITELLM_MODEL_ALIAS_ENV: alias, LITELLM_MODEL_VIA_ENV: "1", + "CODEX_CONFIG": f'{{"model":"{alias}"}}', }, ) - mock_acp.set_model.assert_awaited_once_with("gpt-5.4-mini[medium]") + launch_env = transport.call_args.kwargs["env"] + config = json.loads(launch_env["CODEX_CONFIG"]) + assert config.get("model_reasoning_effort") == reasoning_effort + mock_acp.set_model.assert_not_awaited() mock_acp.set_config_option.assert_not_awaited()