From 4b6a969df2387e69ce67f68443a2ec3a44568171 Mon Sep 17 00:00:00 2001 From: Ziheng Zhang <142805986+MuXinCG2004@users.noreply.github.com> Date: Tue, 9 Jun 2026 01:38:13 +0800 Subject: [PATCH] refactor(agent): make create_cli_agent(config=, chat_model=) pure (#267) * refactor(agent): make create_cli_agent(config=, chat_model=) pure Re-applies the #183 purity refactor on top of the observation-memory lifecycle that landed in #259, integrating the two cleanly. create_cli_agent gains a pure path: when both `config` and `chat_model` are passed it builds the agent entirely from locals and writes none of the cached module globals (`_config`, `_chat_model`, `_chat_model_key`, `_EvoScientist_agent`). `/model` commits the switch via `set_active_config` / `set_chat_model_instance` only after a successful build, so a failed rebuild leaves the session on the original model (replaces the old snapshot/restore rollback). Supporting changes: - Extract `set_active_config` (write-half of `_ensure_config`), `_apply_env_from_config`, `_build_chat_model`, and `set_chat_model_instance`. - Thread `cfg` / `chat_model` through `_get_default_middleware`, `_build_base_kwargs`, `load_mcp_and_build_kwargs`, `_maybe_swap_async_subagents`, and `_inject_subagent_middleware` so the pure path never falls back to the global-writing `_ensure_config()` / `_ensure_chat_model()`. - Integrate with #259's memory middleware: subagent context-editing middleware binds the threaded `chat_model`, and the configured system prompt / memory controls read the threaded `cfg` (new threading vs the original #183, required because #259 made these paths read config). - Consolidate `cfg` resolution to one `cfg if cfg is not None else _ensure_config()` at the top of each kwargs builder, matching the pattern already used in the other config-aware helpers. * fix(agent): keep pure tool selector off global cache * fix(model): apply config switch in place to preserve reference integrity --------- Co-authored-by: Xi Zhang <106144707+X-iZhang@users.noreply.github.com> Co-authored-by: X-iZhang --- EvoScientist/EvoScientist.py | 172 ++++++++-- EvoScientist/cli/agent.py | 5 + EvoScientist/commands/implementation/model.py | 69 ++-- tests/test_auxiliary_model.py | 58 ++++ tests/test_model_command.py | 301 +++++++++--------- uv.lock | 2 +- 6 files changed, 379 insertions(+), 228 deletions(-) diff --git a/EvoScientist/EvoScientist.py b/EvoScientist/EvoScientist.py index 753f0c8..40cb366 100644 --- a/EvoScientist/EvoScientist.py +++ b/EvoScientist/EvoScientist.py @@ -81,18 +81,52 @@ _EvoScientist_agent = None # ============================================================================= +def set_active_config(cfg) -> None: + """Commit *cfg* as the active module config. + + Public commit path for callers (e.g. ``/model``) that built an agent on + the pure ``create_cli_agent(config=..., chat_model=...)`` path and now + want it to become the session-wide active config. This is the write half + of ``_ensure_config(cfg)`` extracted so the pure path can defer the commit + until the agent has been built successfully. + """ + global _config + _config = cfg + apply_config_to_env(cfg) + + +def _apply_env_from_config(cfg) -> None: + """Apply *cfg*'s API-key env vars without caching it as ``_config``. + + ``apply_config_to_env`` is set-if-unset (guards on ``not + os.environ.get(...)``), so this is idempotent and safe to call on the pure + path, where no module globals may be written. + """ + apply_config_to_env(cfg) + + def _ensure_config(config=None): """Return cached config. If *config* is passed, cache and use it.""" - global _config if config is not None: - _config = config - apply_config_to_env(_config) + set_active_config(config) if _config is None: - _config = get_effective_config() - apply_config_to_env(_config) + set_active_config(get_effective_config()) return _config +def _build_chat_model(cfg): + """Build a chat model from *cfg* without writing any module globals. + + Pure-construction counterpart to ``_ensure_chat_model``: used by ``/model`` + to verify a switch before committing, and threaded into + ``create_cli_agent(chat_model=...)`` so the new agent binds the requested + model without touching the cached ``_chat_model``. + """ + from .llm import get_chat_model + + return get_chat_model(model=cfg.model, provider=cfg.provider) + + def _replace_chat_model(instance, key: tuple[str | None, str | None]) -> None: """Install a new chat model and propagate the related invariants. @@ -118,15 +152,10 @@ def _ensure_chat_model(): into the new agent without requiring callers to interleave ``set_chat_model()`` calls in any particular order. """ - from .llm import get_chat_model - cfg = _ensure_config() key = (cfg.model, cfg.provider) if _chat_model is None or _chat_model_key != key: - _replace_chat_model( - get_chat_model(model=cfg.model, provider=cfg.provider), - key, - ) + _replace_chat_model(_build_chat_model(cfg), key) return _chat_model @@ -180,6 +209,18 @@ def set_chat_model(model: str, provider: str | None = None): return _chat_model +def set_chat_model_instance(instance, key: tuple[str | None, str | None]) -> None: + """Commit an already-built chat model *instance* as the active model. + + Companion to ``set_active_config`` for the pure path: installs a model that + ``_build_chat_model`` already constructed (e.g. during a ``/model`` verify) + without rebuilding it, keeping ``_chat_model`` / ``_chat_model_key`` / + ``_EvoScientist_agent`` in sync via ``_replace_chat_model``. Unlike + ``set_chat_model``, the caller owns the ``(model, provider)`` *key*. + """ + _replace_chat_model(instance, key) + + # ============================================================================= # MCP caching # ============================================================================= @@ -245,12 +286,18 @@ def _inject_subagent_middleware( subs: list[dict], *, workspace_dir: str | Path | None = None, + cfg=None, + chat_model=None, ) -> None: """Ensure every subagent gets error handling and context management middleware. Without this, subagent tool errors are caught by LangGraph's default ToolNode handler which produces terse messages without tracebacks or retry guidance — reducing the subagent's ability to self-recover. + + *chat_model*, when provided, is forwarded to the subagents' + ``create_context_editing_middleware`` so the pure ``create_cli_agent`` + path doesn't fall back to the global-writing ``_ensure_chat_model()``. """ from .middleware import ( ContextOverflowMapperMiddleware, @@ -262,7 +309,7 @@ def _inject_subagent_middleware( create_runtime_context_middleware, ) - cfg = _ensure_config() + cfg = cfg if cfg is not None else _ensure_config() memory_controls = MemoryControls.from_config(cfg) memory_dir = str(_paths_mod.MEMORIES_DIR) for sa in subs: @@ -280,9 +327,10 @@ def _inject_subagent_middleware( ), ) middleware = [ - # No ``model=`` — subagents share the main agent's model, - # so defer to the factory's ``_ensure_chat_model()`` fallback. - create_context_editing_middleware(), + # Subagents share the main agent's model: use the threaded + # ``chat_model`` on the pure path, else defer to the factory's + # ``_ensure_chat_model()`` fallback (when ``chat_model=None``). + create_context_editing_middleware(chat_model), create_runtime_context_middleware(), ToolErrorHandlerMiddleware(), ContextOverflowMapperMiddleware(), @@ -319,7 +367,9 @@ def _ensure_general_purpose_subagent(subs: list[dict]) -> None: ) -def _maybe_swap_async_subagents(subs: list, middleware: list | None = None) -> list: +def _maybe_swap_async_subagents( + subs: list, middleware: list | None = None, *, cfg=None +) -> list: """Replace ``_async``-flagged sub-agents with ``AsyncSubAgent`` specs when enabled. Reads the ``_async`` field carried through by ``utils.load_subagents._build_one`` @@ -341,7 +391,7 @@ def _maybe_swap_async_subagents(subs: list, middleware: list | None = None) -> l appends ``AsyncWatcherMiddleware`` so launches spawn an ``async_notifier`` watcher. """ - cfg = _ensure_config() + cfg = cfg if cfg is not None else _ensure_config() if not getattr(cfg, "enable_async_subagents", False): # Async fully disabled — strip the internal flag before handoff. for s in subs: @@ -418,11 +468,14 @@ def _maybe_swap_async_subagents(subs: list, middleware: list | None = None) -> l return out -def _build_base_kwargs(base_backend, base_middleware, *, workspace_dir=None): +def _build_base_kwargs( + base_backend, base_middleware, *, cfg=None, chat_model=None, workspace_dir=None +): """Build agent kwargs *without* MCP (fast, no subprocess spawning).""" from .tools import skill_manager, tavily_search, think_tool from .utils import load_subagents + cfg = cfg if cfg is not None else _ensure_config() tool_registry = {"think_tool": think_tool} if os.environ.get("TAVILY_API_KEY"): tool_registry["tavily_search"] = tavily_search @@ -433,16 +486,18 @@ def _build_base_kwargs(base_backend, base_middleware, *, workspace_dir=None): tool_registry=tool_registry, ) _ensure_general_purpose_subagent(subs) - _inject_subagent_middleware(subs, workspace_dir=workspace_dir) - subs = _maybe_swap_async_subagents(subs, base_middleware) + _inject_subagent_middleware( + subs, workspace_dir=workspace_dir, cfg=cfg, chat_model=chat_model + ) + subs = _maybe_swap_async_subagents(subs, base_middleware, cfg=cfg) return { "name": "EvoScientist", - "model": _ensure_chat_model(), + "model": chat_model if chat_model is not None else _ensure_chat_model(), "tools": list(base_tools), "backend": base_backend, "subagents": subs, "middleware": base_middleware, - "system_prompt": _configured_system_prompt(_ensure_config()), + "system_prompt": _configured_system_prompt(cfg), "skills": list(DEFAULT_SKILL_SOURCES), } @@ -452,6 +507,8 @@ def load_mcp_and_build_kwargs( base_middleware, *, on_mcp_progress=None, + cfg=None, + chat_model=None, workspace_dir=None, ): """Load MCP tools (cached by config) and build agent kwargs. @@ -462,15 +519,22 @@ def load_mcp_and_build_kwargs( Args: on_mcp_progress: Optional per-server progress callback. Forwarded to the MCP loader so UIs can render live status. + cfg: Explicit config to thread through instead of reading the cached + ``_config``. Used by the pure ``create_cli_agent`` path. + chat_model: Explicit chat model to bind instead of + ``_ensure_chat_model()`` (which would write module globals). """ from .tools import skill_manager, tavily_search, think_tool from .utils import load_subagents + cfg = cfg if cfg is not None else _ensure_config() mcp_by_agent = _load_mcp_tools_cached(on_progress=on_mcp_progress) if not mcp_by_agent: return _build_base_kwargs( base_backend, base_middleware, + cfg=cfg, + chat_model=chat_model, workspace_dir=workspace_dir, ) @@ -493,7 +557,9 @@ def load_mcp_and_build_kwargs( ) _ensure_general_purpose_subagent(subs) - _inject_subagent_middleware(subs, workspace_dir=workspace_dir) + _inject_subagent_middleware( + subs, workspace_dir=workspace_dir, cfg=cfg, chat_model=chat_model + ) # Inject MCP tools into subagents by name for sa in subs: @@ -502,16 +568,16 @@ def load_mcp_and_build_kwargs( # Swap selected sub-agents to AsyncSubAgent (must happen AFTER MCP injection # since async sub-agents are remote graphs that load their own tools). - subs = _maybe_swap_async_subagents(subs, base_middleware) + subs = _maybe_swap_async_subagents(subs, base_middleware, cfg=cfg) return { "name": "EvoScientist", - "model": _ensure_chat_model(), + "model": chat_model if chat_model is not None else _ensure_chat_model(), "tools": base_tools + mcp_main, "backend": base_backend, "subagents": subs, "middleware": base_middleware, - "system_prompt": _configured_system_prompt(_ensure_config()), + "system_prompt": _configured_system_prompt(cfg), "skills": list(DEFAULT_SKILL_SOURCES), } @@ -561,6 +627,8 @@ def _get_default_middleware( *, for_async_subagent: bool = False, workspace_dir: str | Path | None = None, + cfg=None, + chat_model=None, memory_source_agent: str = "EvoScientist", ): """Build the default middleware list. @@ -576,6 +644,9 @@ def _get_default_middleware( ``subagents/_factory.py`` deliberately skips ``interrupt_on=`` on the deepagents level. Defaults to False (full middleware list) for the CLI's in-process agent. + cfg: Explicit config to use instead of the cached ``_config``. + chat_model: Explicit model to bind instead of ``_ensure_chat_model()`` + (avoids writing module globals on the pure path). memory_source_agent: Attribution name for profile/observation writes. Async sub-agent factories pass their deployed agent name here. """ @@ -594,10 +665,10 @@ def _get_default_middleware( load_fallback_chain, ) - cfg = _ensure_config() + cfg = cfg if cfg is not None else _ensure_config() if cfg.model_fallbacks: load_fallback_chain(cfg.model_fallbacks) - model = _ensure_chat_model() + model = chat_model if chat_model is not None else _ensure_chat_model() memory_dir = str(_paths_mod.MEMORIES_DIR) source_type = ( MemorySourceType.SUBAGENT if for_async_subagent else MemorySourceType.TURN @@ -627,9 +698,19 @@ def _get_default_middleware( # keep the main model (they do real work, not a one-off helper call). # context_editing stays on the main model — its model only sizes the # context-window trigger for the main agent's own history. - tool_selector_model = ( - model if for_async_subagent else _ensure_auxiliary_chat_model() - ) + if for_async_subagent: + tool_selector_model = model + elif chat_model is None: + tool_selector_model = _ensure_auxiliary_chat_model() + else: + aux_model = cfg.auxiliary_model or cfg.model + aux_provider = cfg.auxiliary_provider or cfg.provider + if (aux_model, aux_provider) == (cfg.model, cfg.provider): + tool_selector_model = model + else: + from .llm import get_chat_model + + tool_selector_model = get_chat_model(model=aux_model, provider=aux_provider) mw = [ ConfigurableModelMiddleware(), create_context_editing_middleware(model), @@ -761,6 +842,7 @@ def create_cli_agent( workspace_dir: str | None = None, checkpointer=None, config=None, + chat_model=None, *, on_mcp_progress=None, ): @@ -770,6 +852,14 @@ def create_cli_agent( ``paths.WORKSPACE_ROOT`` (or the explicit *workspace_dir*), so runtime ``set_workspace_root()`` changes are always respected. + **Pure path:** when *both* ``config`` and ``chat_model`` are explicit, this + writes none of the cached config/model module globals (``_config``, + ``_chat_model``, ``_chat_model_key``, ``_EvoScientist_agent``) — the agent + is built purely from the passed-in locals. The caller commits the switch + on success via ``set_active_config`` / ``set_chat_model_instance`` (see + ``/model``). Otherwise the existing module-global path runs (langgraph + dev, notebooks, and CLI startup, which pass ``config=`` only). + Args: workspace_dir: Per-session workspace directory. If ``None``, defaults to the current ``paths.WORKSPACE_ROOT``. @@ -778,6 +868,9 @@ def create_cli_agent( config: Optional pre-loaded ``EvoScientistConfig``. If ``None``, loads from file/env/defaults. Passing this avoids double loading when the CLI has already loaded config. + chat_model: Optional pre-built chat model. Only triggers the pure + path when ``config`` is also explicit; otherwise it is ignored in + favor of the ``_ensure_chat_model()`` fallback. """ import os as _os @@ -787,7 +880,16 @@ def create_cli_agent( from . import paths as _paths from .backends import CustomSandboxBackend, MergedSkillsBackend - cfg = _ensure_config(config) + # Pure path only when BOTH config and chat_model are explicit: build from + # locals and write no module globals. Otherwise keep the legacy + # global-writing behavior — callers that pass config= only (CLI startup, + # langgraph dev) rely on it to seat the active config/model. + if config is not None and chat_model is not None: + cfg = config + _apply_env_from_config(cfg) + else: + cfg = _ensure_config(config) + chat_model = None if checkpointer is None: from langgraph.checkpoint.memory import InMemorySaver @@ -838,7 +940,9 @@ def create_cli_agent( # Delegate middleware construction to the single source of truth so the # CLI agent never drifts from the default chain. Anything CLI-specific # (e.g. ``HumanInTheLoopMiddleware``) is appended below. - mw: list[AgentMiddleware] = _get_default_middleware(workspace_dir=workspace_dir) + mw: list[AgentMiddleware] = _get_default_middleware( + workspace_dir=workspace_dir, cfg=cfg, chat_model=chat_model + ) # HITL on main agent only — passing `interrupt_on=` to create_deep_agent # would propagate it to every subagent, breaking parallel execute calls @@ -855,6 +959,8 @@ def create_cli_agent( be, mw, on_mcp_progress=on_mcp_progress, + cfg=cfg, + chat_model=chat_model, workspace_dir=workspace_dir, ) diff --git a/EvoScientist/cli/agent.py b/EvoScientist/cli/agent.py index 10e6ca2..2e8caa8 100644 --- a/EvoScientist/cli/agent.py +++ b/EvoScientist/cli/agent.py @@ -62,6 +62,7 @@ def _load_agent( workspace_dir: str | None = None, checkpointer=None, config=None, + chat_model=None, *, on_mcp_progress=None, ): @@ -73,6 +74,9 @@ def _load_agent( Falls back to ``InMemorySaver`` when ``None``. config: Optional pre-loaded ``EvoScientistConfig``. Forwarded to ``create_cli_agent`` to avoid double config loading. + chat_model: Optional pre-built chat model. Forwarded to + ``create_cli_agent``; combined with an explicit ``config`` it + selects the pure (no module-global write) build path. on_mcp_progress: Optional per-server MCP progress callback. Signature ``(event, server_name, detail) -> None``. """ @@ -82,5 +86,6 @@ def _load_agent( workspace_dir=workspace_dir, checkpointer=checkpointer, config=config, + chat_model=chat_model, on_mcp_progress=on_mcp_progress, ) diff --git a/EvoScientist/commands/implementation/model.py b/EvoScientist/commands/implementation/model.py index 4be8f15..1d11b2c 100644 --- a/EvoScientist/commands/implementation/model.py +++ b/EvoScientist/commands/implementation/model.py @@ -143,71 +143,50 @@ class ModelCommand(Command): ) -> None: import copy - from ... import EvoScientist as _mod from ...cli.agent import _load_agent - from ...EvoScientist import _ensure_config, set_chat_model + from ...EvoScientist import ( + _build_chat_model, + _ensure_config, + set_active_config, + set_chat_model_instance, + ) cfg = _ensure_config() - # Build a temporary config to verify the agent can be created - # before mutating any global state. + # Build a temporary config + its chat model and verify the agent can be + # built before committing anything. ``create_cli_agent(config=..., + # chat_model=...)`` is pure (issue #183) — it writes none of the cached + # config/model module globals — so a failure below leaves the session + # on the original model with no snapshot/restore needed. temp_cfg = copy.copy(cfg) temp_cfg.model = model_name temp_cfg.provider = provider - # create_cli_agent(config=temp_cfg) calls _ensure_config and - # _ensure_chat_model before finishing, so a failure further - # down (middleware build, MCP reconnect, deepagents wiring) - # would leave the session pointing at the new model. Snapshot - # the four globals those helpers write so we can restore on - # error. Best-effort: references already captured by concurrent - # readers (e.g. a channel thread mid-turn) are not retroactively - # patched, but /model is user-initiated from an idle prompt in - # practice. - snap = ( - _mod._config, - _mod._chat_model, - _mod._chat_model_key, - _mod._EvoScientist_agent, - ) - - def _restore_globals() -> None: - """Roll back the four module globals to their pre-call values. - - Keeps the two failure sites (``_load_agent`` and - ``set_chat_model``) in sync — adding a new snapshotted global - only requires updating ``snap`` and this helper. - """ - ( - _mod._config, - _mod._chat_model, - _mod._chat_model_key, - _mod._EvoScientist_agent, - ) = snap - try: + new_chat_model = _build_chat_model(temp_cfg) new_agent = _load_agent( workspace_dir=ctx.workspace_dir, checkpointer=ctx.checkpointer, config=temp_cfg, + chat_model=new_chat_model, ) except Exception as e: - _restore_globals() - ctx.ui.append_system(f"Failed to switch model: {e}", style="red") - return - - # Agent built successfully — now commit the change globally. - try: - set_chat_model(model_name, provider=provider) - except Exception as e: - # _load_agent already mutated the four globals; restore them so a - # failure here doesn't leave the session half-switched. - _restore_globals() ctx.ui.append_system(f"Failed to switch model: {e}", style="red") return + # Agent built with no global mutation — commit the switch atomically. + # These are pure assignments and cannot fail, so the session can never + # be left half-switched. Apply the switch to the LIVE ``cfg`` in place + # (the active config object) instead of rebinding ``_config`` to the + # fresh ``temp_cfg`` — callers that hold the active config by reference + # (e.g. serve's ``agent_holder["config"]`` and its workspace-changing + # ``/resume`` reload) must observe the new model/provider. The verify + # build above used the ``temp_cfg`` copy, so a failed build never reaches + # here and the live ``cfg`` stays untouched (failure still no-ops). cfg.model = model_name cfg.provider = provider + set_active_config(cfg) + set_chat_model_instance(new_chat_model, (model_name, provider)) ctx.agent = new_agent # Persist to config file if --save was given diff --git a/tests/test_auxiliary_model.py b/tests/test_auxiliary_model.py index 37de4fd..ccb3c5b 100644 --- a/tests/test_auxiliary_model.py +++ b/tests/test_auxiliary_model.py @@ -157,3 +157,61 @@ class TestAuxiliaryMiddlewareScope: assert cap["tool_selector"] is main_model assert cap["context_editing"] is main_model + + def test_pure_path_tool_selector_uses_threaded_main_when_aux_empty(self): + cap, fake_ts, fake_ce = self._capture() + cfg = _mock_cfg() + cfg.model = "new-main" + cfg.provider = "new-provider" + cfg.auxiliary_model = "" + cfg.auxiliary_provider = "" + main_model = object() + + with ( + patch.object(E, "_ensure_config", side_effect=AssertionError), + patch.object(E, "_ensure_chat_model", side_effect=AssertionError), + patch.object(E, "_ensure_auxiliary_chat_model", side_effect=AssertionError), + patch( + "EvoScientist.middleware.create_tool_selector_middleware", + side_effect=fake_ts, + ), + patch( + "EvoScientist.middleware.create_context_editing_middleware", + side_effect=fake_ce, + ), + ): + E._get_default_middleware(cfg=cfg, chat_model=main_model) + + assert cap["tool_selector"] is main_model + assert cap["context_editing"] is main_model + + def test_pure_path_tool_selector_builds_aux_from_threaded_config(self): + cap, fake_ts, fake_ce = self._capture() + cfg = _mock_cfg() + cfg.model = "new-main" + cfg.provider = "new-provider" + cfg.auxiliary_model = "new-aux" + cfg.auxiliary_provider = "aux-provider" + main_model, aux_model = object(), object() + + with ( + patch.object(E, "_ensure_config", side_effect=AssertionError), + patch.object(E, "_ensure_chat_model", side_effect=AssertionError), + patch.object(E, "_ensure_auxiliary_chat_model", side_effect=AssertionError), + patch( + "EvoScientist.llm.get_chat_model", return_value=aux_model + ) as get_model, + patch( + "EvoScientist.middleware.create_tool_selector_middleware", + side_effect=fake_ts, + ), + patch( + "EvoScientist.middleware.create_context_editing_middleware", + side_effect=fake_ce, + ), + ): + E._get_default_middleware(cfg=cfg, chat_model=main_model) + + get_model.assert_called_once_with(model="new-aux", provider="aux-provider") + assert cap["tool_selector"] is aux_model + assert cap["context_editing"] is main_model diff --git a/tests/test_model_command.py b/tests/test_model_command.py index 51977e3..b2ba42d 100644 --- a/tests/test_model_command.py +++ b/tests/test_model_command.py @@ -150,9 +150,9 @@ class TestModelCommandSwitch: "EvoScientist.EvoScientist._ensure_config", return_value=cfg, ), - patch( - "EvoScientist.EvoScientist.set_chat_model", - ), + patch("EvoScientist.EvoScientist._build_chat_model"), + patch("EvoScientist.EvoScientist.set_active_config") as set_cfg, + patch("EvoScientist.EvoScientist.set_chat_model_instance"), patch( "EvoScientist.cli.agent._load_agent", return_value=new_agent, @@ -160,9 +160,12 @@ class TestModelCommandSwitch: ): _run(cmd.execute(ctx, ["claude-opus-4-8"])) - # Config should be updated - assert cfg.model == "claude-opus-4-8" - assert cfg.provider == "anthropic" + # The switch is committed via set_active_config(temp_cfg), not by + # mutating the original cfg object in place. + set_cfg.assert_called_once() + committed = set_cfg.call_args[0][0] + assert committed.model == "claude-opus-4-8" + assert committed.provider == "anthropic" # Agent should be replaced on context assert ctx.agent == new_agent @@ -191,7 +194,9 @@ class TestModelCommandSwitch: "EvoScientist.EvoScientist._ensure_config", return_value=cfg, ), - patch("EvoScientist.EvoScientist.set_chat_model"), + patch("EvoScientist.EvoScientist._build_chat_model"), + patch("EvoScientist.EvoScientist.set_active_config"), + patch("EvoScientist.EvoScientist.set_chat_model_instance"), patch( "EvoScientist.cli.agent._load_agent", return_value=MagicMock(), @@ -226,7 +231,9 @@ class TestModelCommandSwitch: "EvoScientist.EvoScientist._ensure_config", return_value=cfg, ), - patch("EvoScientist.EvoScientist.set_chat_model"), + patch("EvoScientist.EvoScientist._build_chat_model"), + patch("EvoScientist.EvoScientist.set_active_config"), + patch("EvoScientist.EvoScientist.set_chat_model_instance"), patch( "EvoScientist.cli.agent._load_agent", return_value=MagicMock(), @@ -244,9 +251,9 @@ class TestModelCommandSwitch: class TestModelCommandFailure: - """Verify error handling when set_chat_model raises.""" + """Verify error handling when chat-model construction raises.""" - def test_set_chat_model_error(self): + def test_build_chat_model_error(self): from EvoScientist.commands.implementation.model import ModelCommand cmd = ModelCommand() @@ -265,17 +272,13 @@ class TestModelCommandFailure: return_value=cfg, ), patch( - "EvoScientist.cli.agent._load_agent", - return_value=MagicMock(), - ), - patch( - "EvoScientist.EvoScientist.set_chat_model", + "EvoScientist.EvoScientist._build_chat_model", side_effect=RuntimeError("API key missing"), - ) as mock_set, + ) as mock_build, ): _run(cmd.execute(ctx, ["claude-opus-4-8"])) - mock_set.assert_called_once() + mock_build.assert_called_once() ui.append_system.assert_called_once() call_args = ui.append_system.call_args assert "Failed to switch model" in call_args[0][0] @@ -429,18 +432,18 @@ class TestEnsureChatModelCacheInvalidation: class TestApplyModelIntegration: - """End-to-end regression for #179: `_apply_model` must produce an agent - bound to the NEW model, not a stale cached one. + """End-to-end regression for #179 + #183: `_apply_model` must produce an + agent bound to the NEW model, threaded in via the pure ``create_cli_agent`` + path. Exercises the real chain: - ``_apply_model → _load_agent → _ensure_config → _ensure_chat_model`` + ``_apply_model → _build_chat_model → _load_agent(chat_model=...) → + set_active_config / set_chat_model_instance`` - Only ``_load_agent`` is replaced by a minimal fake that mirrors the - exact two globals ``create_cli_agent`` mutates (``_ensure_config`` - + ``_ensure_chat_model``) — so the bug path is fully exercised - without having to spin up deepagents, MCP tools, middleware, and - subagent YAML. ``get_chat_model`` returns a distinct sentinel per - ``(model, provider)`` pair so we can assert on identity. + Only ``_load_agent`` is faked (to avoid spinning up deepagents, MCP tools, + middleware, and subagent YAML); it binds the ``chat_model`` it receives. + ``get_chat_model`` returns a distinct sentinel per ``(model, provider)`` + pair so we can assert on identity. """ def test_new_agent_is_bound_to_newly_selected_model(self, evo_module_state): @@ -460,14 +463,17 @@ class TestApplyModelIntegration: return sentinels[key] def _fake_load_agent( - workspace_dir=None, checkpointer=None, config=None, *, on_mcp_progress=None + workspace_dir=None, + checkpointer=None, + config=None, + chat_model=None, + *, + on_mcp_progress=None, ): - # Replicates the two create_cli_agent side effects that - # reveal the bug: ``_ensure_config`` writes the new cfg, - # then ``_ensure_chat_model`` must rebuild to match it. - mod._ensure_config(config) + # The pure path threads the freshly built chat model in; bind it + # directly instead of re-deriving via _ensure_chat_model. agent = MagicMock(name="fake-agent") - agent._bound_model = mod._ensure_chat_model() + agent._bound_model = chat_model return agent cfg = EvoScientistConfig(model="claude-sonnet-4-6", provider="anthropic") @@ -499,14 +505,18 @@ class TestApplyModelIntegration: _run(cmd._apply_model(ctx, "minimax-m2.7", "openrouter")) # The agent produced by _apply_model must be bound to the - # NEWLY requested model, not the previously cached one. + # NEWLY requested model, threaded in via chat_model=. assert ctx.agent._bound_model is not old_model assert ctx.agent._bound_model._bound_model == "minimax-m2.7" assert ctx.agent._bound_model._bound_provider == "openrouter" - # Global state reflects the switch end-to-end. + # Global state reflects the switch end-to-end, committed via the setters. assert mod._chat_model_key == ("minimax-m2.7", "openrouter") assert mod._chat_model is sentinels[("minimax-m2.7", "openrouter")] + # The switch is applied to the LIVE cfg in place and ``_config`` stays + # bound to that same object, so callers holding the active config by + # reference (e.g. serve's ``agent_holder["config"]``) observe the swap. + assert mod._config is cfg assert cfg.model == "minimax-m2.7" assert cfg.provider == "openrouter" @@ -516,6 +526,80 @@ class TestApplyModelIntegration: assert "openrouter" in msg +class TestApplyModelPreservesConfigByReference: + """Regression for the din0s review on #267: serve mode (and any long-lived + caller) holds the active config object by reference via + ``agent_holder["config"]``. The pure-path commit must apply the switch to + that LIVE object in place — not rebind ``_config`` to a fresh ``temp_cfg`` + copy — otherwise a later workspace-changing ``/resume`` reloads the agent + from the stale startup config and silently reverts the ``/model`` switch. + + Critically this must hold across *repeated* switches: the earlier + "also mutate cfg but still rebind to temp_cfg" remedy only survives one + switch (the held object stops being the active ``_config`` after the first). + """ + + def test_held_config_reference_tracks_repeated_switches(self, evo_module_state): + from EvoScientist.commands.implementation.model import ModelCommand + from EvoScientist.config.settings import EvoScientistConfig + + mod = evo_module_state + + def _fake_get_chat_model(model, provider=None): + m = MagicMock(name=f"chat_model[{model}|{provider}]") + m._bound_model = model + return m + + def _fake_load_agent( + workspace_dir=None, + checkpointer=None, + config=None, + chat_model=None, + *, + on_mcp_progress=None, + ): + return MagicMock(name="fake-agent") + + cfg = EvoScientistConfig(model="claude-sonnet-4-6", provider="anthropic") + mod._config = cfg + mod._chat_model = None + mod._chat_model_key = None + mod._EvoScientist_agent = None + + # Simulate serve capturing the startup config object once (commands.py: + # ``agent_holder = {... "config": config}``) and never re-reading it. + agent_holder = {"config": cfg} + + ctx = MagicMock() + ctx.ui = MagicMock() + ctx.ui.supports_interactive = True + ctx.workspace_dir = "/tmp/test_byref" + ctx.checkpointer = None + + cmd = ModelCommand() + with ( + patch( + "EvoScientist.llm.get_chat_model", + side_effect=_fake_get_chat_model, + ), + patch( + "EvoScientist.cli.agent._load_agent", + side_effect=_fake_load_agent, + ), + ): + for model, provider in [ + ("claude-opus-4-8", "anthropic"), + ("minimax-m2.7", "openrouter"), + ("claude-sonnet-4-6", "anthropic"), + ]: + _run(cmd._apply_model(ctx, model, provider)) + # The held reference must reflect the LATEST switch on every + # iteration — not just the first — and stay the active config. + assert agent_holder["config"].model == model + assert agent_holder["config"].provider == provider + assert mod._config is agent_holder["config"] + + class TestModelCommandLoadAgentFailure: """Verify the transactional ordering: when ``_load_agent`` raises, nothing downstream (``set_chat_model``, ``cfg`` mutation, @@ -544,13 +628,17 @@ class TestModelCommandLoadAgentFailure: "EvoScientist.EvoScientist._ensure_config", return_value=cfg, ), + patch("EvoScientist.EvoScientist._build_chat_model"), patch( "EvoScientist.cli.agent._load_agent", side_effect=RuntimeError("agent build failed"), ) as mock_load, patch( - "EvoScientist.EvoScientist.set_chat_model", - ) as mock_set, + "EvoScientist.EvoScientist.set_active_config", + ) as mock_set_cfg, + patch( + "EvoScientist.EvoScientist.set_chat_model_instance", + ) as mock_set_model, patch( "EvoScientist.config.settings.set_config_value", ) as mock_save, @@ -562,8 +650,9 @@ class TestModelCommandLoadAgentFailure: # _load_agent was attempted (transactional first step). mock_load.assert_called_once() - # Downstream side-effects must NOT have happened. - mock_set.assert_not_called() + # Commit setters and downstream side-effects must NOT have happened. + mock_set_cfg.assert_not_called() + mock_set_model.assert_not_called() mock_save.assert_not_called() # Config must be untouched. assert cfg.model == "claude-sonnet-4-6" @@ -576,46 +665,38 @@ class TestModelCommandLoadAgentFailure: class TestApplyModelLoadAgentFailureTransactional: - """Regression: if ``create_cli_agent`` raises AFTER partially mutating - module globals via ``_ensure_config`` + ``_ensure_chat_model``, - ``_apply_model`` must roll those back so the session stays on the - original model. + """Regression for #183: when agent construction fails, the session stays on + the original model with no snapshot/restore. - Complements :class:`TestModelCommandLoadAgentFailure`, which tests - the early-failure path where ``_load_agent`` never reaches - ``create_cli_agent`` and no globals get mutated. + Because ``create_cli_agent(config=..., chat_model=...)`` is now pure — it + writes none of the four config/model globals — and ``_apply_model`` commits + only after a successful build, a failing ``_load_agent`` leaves all four + globals untouched. This replaces the old snapshot/restore rollback test. + + Complements :class:`TestModelCommandLoadAgentFailure`, which asserts the + downstream setters never run on failure. """ - def test_globals_restored_after_create_cli_agent_partial_mutation( - self, evo_module_state - ): + def test_globals_unchanged_when_load_agent_raises(self, evo_module_state): from EvoScientist.commands.implementation.model import ModelCommand from EvoScientist.config.settings import EvoScientistConfig mod = evo_module_state - sentinels: dict[tuple[str, str | None], MagicMock] = {} - - def _fake_get_chat_model(model, provider=None): - key = (model, provider) - sentinels.setdefault(key, MagicMock(name=f"chat_model[{model}|{provider}]")) - return sentinels[key] def _fake_load_agent( workspace_dir=None, checkpointer=None, config=None, + chat_model=None, *, on_mcp_progress=None, ): - # Mimic ``create_cli_agent``: mutate globals via - # ``_ensure_config`` + ``_ensure_chat_model``, then raise - # (as if middleware construction or deepagents wiring failed). - mod._ensure_config(config) - mod._ensure_chat_model() + # The pure path writes no globals; mimic a failure partway through + # agent wiring (middleware build, deepagents, MCP reconnect, ...). raise RuntimeError("middleware build failed") cfg = EvoScientistConfig(model="claude-sonnet-4-6", provider="anthropic") - old_model = _fake_get_chat_model("claude-sonnet-4-6", "anthropic") + old_model = MagicMock(name="old-model") old_agent = MagicMock(name="old-default-agent") mod._config = cfg @@ -631,8 +712,8 @@ class TestApplyModelLoadAgentFailureTransactional: with ( patch( - "EvoScientist.llm.get_chat_model", - side_effect=_fake_get_chat_model, + "EvoScientist.EvoScientist._build_chat_model", + return_value=MagicMock(name="new-model"), ), patch( "EvoScientist.cli.agent._load_agent", @@ -642,7 +723,7 @@ class TestApplyModelLoadAgentFailureTransactional: cmd = ModelCommand() _run(cmd._apply_model(ctx, "minimax-m2.7", "openrouter")) - # All four globals restored to their pre-call state. + # All four globals are unchanged — nothing was committed. assert mod._config is cfg assert mod._chat_model is old_model assert mod._chat_model_key == ("claude-sonnet-4-6", "anthropic") @@ -655,89 +736,6 @@ class TestApplyModelLoadAgentFailureTransactional: assert "Failed to switch model" in msg -class TestApplyModelSetChatModelFailureTransactional: - """Regression (CodeRabbit review on PR #187): if ``set_chat_model`` - raises *after* ``_load_agent`` has already mutated module globals, - those globals must be restored. Without the rollback the session - ends up half-switched — new ``_config`` / ``_chat_model`` committed, - but no successful agent to back them. - - Complements :class:`TestApplyModelLoadAgentFailureTransactional` - which covers the earlier failure site. - """ - - def test_globals_restored_when_set_chat_model_raises(self, evo_module_state): - from EvoScientist.commands.implementation.model import ModelCommand - from EvoScientist.config.settings import EvoScientistConfig - - mod = evo_module_state - sentinels: dict[tuple[str, str | None], MagicMock] = {} - - def _fake_get_chat_model(model, provider=None): - key = (model, provider) - sentinels.setdefault(key, MagicMock(name=f"chat_model[{model}|{provider}]")) - return sentinels[key] - - def _fake_load_agent( - workspace_dir=None, - checkpointer=None, - config=None, - *, - on_mcp_progress=None, - ): - # Mimic the real ``create_cli_agent``: mutate globals via - # ``_ensure_config`` + ``_ensure_chat_model``, then succeed. - mod._ensure_config(config) - mod._ensure_chat_model() - return MagicMock(name="new-agent") - - cfg = EvoScientistConfig(model="claude-sonnet-4-6", provider="anthropic") - old_model = _fake_get_chat_model("claude-sonnet-4-6", "anthropic") - old_agent = MagicMock(name="old-default-agent") - - mod._config = cfg - mod._chat_model = old_model - mod._chat_model_key = ("claude-sonnet-4-6", "anthropic") - mod._EvoScientist_agent = old_agent - - ctx = MagicMock() - ctx.ui = MagicMock() - ctx.ui.supports_interactive = True - ctx.workspace_dir = "/tmp/test_rollback_set" - ctx.checkpointer = None - - with ( - patch( - "EvoScientist.llm.get_chat_model", - side_effect=_fake_get_chat_model, - ), - patch( - "EvoScientist.cli.agent._load_agent", - side_effect=_fake_load_agent, - ), - patch( - "EvoScientist.EvoScientist.set_chat_model", - side_effect=RuntimeError("API key missing at commit step"), - ), - ): - cmd = ModelCommand() - _run(cmd._apply_model(ctx, "minimax-m2.7", "openrouter")) - - # All four globals restored — the new agent was built, but the - # commit step (set_chat_model) failed, so the session must remain - # on the original model. - assert mod._config is cfg - assert mod._chat_model is old_model - assert mod._chat_model_key == ("claude-sonnet-4-6", "anthropic") - assert mod._EvoScientist_agent is old_agent - # cfg itself must not have been mutated (happens after the commit). - assert cfg.model == "claude-sonnet-4-6" - assert cfg.provider == "anthropic" - # User sees an error message. - msg = ctx.ui.append_system.call_args[0][0] - assert "Failed to switch model" in msg - - class TestModelCommandOllamaPicker: """Verify Ollama discovery augments the picker entries and the sentinel is always present when Ollama is configured.""" @@ -922,7 +920,9 @@ class TestModelCommandOllamaPicker: "EvoScientist.llm.ollama_discovery.discover_ollama_models", side_effect=fake_discover, ), - patch("EvoScientist.EvoScientist.set_chat_model"), + patch("EvoScientist.EvoScientist._build_chat_model"), + patch("EvoScientist.EvoScientist.set_active_config") as set_cfg, + patch("EvoScientist.EvoScientist.set_chat_model_instance"), patch( "EvoScientist.cli.agent._load_agent", return_value=MagicMock(), @@ -930,5 +930,8 @@ class TestModelCommandOllamaPicker: ): _run(ModelCommand().execute(ctx, [])) - assert cfg.model == "llama3.3" - assert cfg.provider == "ollama" + # Committed via set_active_config(temp_cfg); original cfg untouched. + set_cfg.assert_called_once() + committed = set_cfg.call_args[0][0] + assert committed.model == "llama3.3" + assert committed.provider == "ollama" diff --git a/uv.lock b/uv.lock index 7b4cd7f..e47ccd6 100644 --- a/uv.lock +++ b/uv.lock @@ -894,7 +894,7 @@ wheels = [ [[package]] name = "evoscientist" -version = "0.1.3" +version = "0.1.4" source = { editable = "." } dependencies = [ { name = "deepagents", extra = ["quickjs"] },