From b39293ab03bfb494d6d2cabe4d2a0c2431cd38de Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Thu, 11 Jun 2026 18:45:21 -0700 Subject: [PATCH] state: persist model/verbose/expanded-view; restore them at launch (#280) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three side-effect handlers were TODO stubs — /model, /verbose, and the expanded-view toggle reset on every restart. - _persist_user_settings: single writer chokepoint (global config "settings" section + cache invalidate, fail-soft); the three advisor handlers deduped onto it (~70 lines of triplicated boilerplate) - /model now actually persists: ModelCommand._apply and the TUI picker route through persist_model_choice (reactive store when wired, else direct write) — previously nothing wrote main_loop_model in production, so the handler never fired - multi-provider guard: the model is persisted PAIRED with its provider key (new AppState.main_loop_provider + settings.model_provider, the advisor_model/advisor_provider precedent); get_persisted_model restores it only when the launch provider matches, so a model chosen on provider A can never feed provider B an invalid id - entrypoints (TUI/headless/REPL) resolve startup model as cli option > persisted choice > provider default_model; get_persisted_model reads the config layers directly because the merged schema bakes in DEFAULT_SETTINGS.model - verbose/expanded_view seed back into get_default_app_state - test_model_command gets the theme/vim-style config isolation fixture (without it the new persist call would write the developer's real ~/.clawcodex/config.json during tests) Closes #280 Co-Authored-By: Claude Opus 4.7 --- src/command_system/model_command.py | 60 +++++-- src/entrypoints/headless.py | 8 +- src/entrypoints/tui.py | 8 +- src/repl/core.py | 7 +- src/settings/settings.py | 42 +++++ src/settings/types.py | 13 ++ src/state/app_state.py | 213 ++++++++++++++---------- src/tui/app.py | 11 ++ tests/test_app_state_persistence.py | 243 ++++++++++++++++++++++++++++ tests/test_model_command.py | 42 +++++ 10 files changed, 544 insertions(+), 103 deletions(-) create mode 100644 tests/test_app_state_persistence.py diff --git a/src/command_system/model_command.py b/src/command_system/model_command.py index 9adaa53d..7adeaab4 100644 --- a/src/command_system/model_command.py +++ b/src/command_system/model_command.py @@ -12,10 +12,10 @@ ``kwargs.get("model", self.model)`` and neither the main loop nor the fast path passes a ``model=`` override, so the held provider's ``.model`` decides the next query's model. So this command sets **``ctx.provider.model``** (the channel inference reads), reachable on the -REPL because Phase 7 also wires ``provider`` into the REPL command context. It is the **only** -write: the reactive ``AppState.main_loop_model`` has no production reader (its -``set_main_loop_model_override`` mirror is dead), and TS ``/model`` never writes disk — so -this is session-only, no settings write. +REPL because Phase 7 also wires ``provider`` into the REPL command context. Since #280 the +choice is ALSO persisted: ``_apply`` routes through ``persist_model_choice`` (reactive store +when wired, else a direct user-settings write paired with the provider key), and entrypoints +restore it at the next launch via ``get_persisted_model``. **Headless keystone:** the arg paths (``/model ``, ``current``/``status``/…, ``help``) need no UI; only the no-args picker needs a surface (``NullUIHost.select`` raises there). @@ -31,8 +31,8 @@ the **exact listed id**; the picker is the ergonomic path there. * **Static description** ("Set the AI model"); TS's is dynamic ``…(currently {model})`` — a frozen ``CommandBase.description: str`` can't be a getter. ``current`` shows the live model. - * **``provider.model`` is the sole write** (Python reads it, not AppState — an architectural - divergence from TS, which writes ``AppState.mainLoopModel``). + * **``provider.model`` is the live-inference write**; ``persist_model_choice`` additionally + writes the reactive store / user settings so the choice survives restarts (#280). * **Effort suffix in ``current``** reads ``settings.effort`` (the Phase 6 channel), not AppState ``effortValue``. * **Label = ``display_name``** (drops TS ``renderModelLabel``'s ``(default)``/alias decoration). @@ -116,13 +116,51 @@ def _show_current(context: CommandContext) -> str: return f"Current model: {_label(cur)}{_effort_suffix()}" -def _apply(provider, model: str) -> None: - """Set the live model. ``provider.model`` is the channel inference reads; guarded - exactly like the TUI's ``_open_model_picker`` (app.py).""" +def _provider_key(provider) -> str | None: + """Reverse-map a provider instance to its config key (exact class + match — each key resolves a distinct class). None for unknown/custom + providers, which skips the persistence pairing (#280).""" + try: + from src.providers import PROVIDER_INFO, get_provider_class + + for name in PROVIDER_INFO: + try: + if type(provider) is get_provider_class(name): + return name + except Exception: + continue + except Exception: + pass + return None + + +def _apply(provider, model: str, context) -> None: + """Set the live model + persist the choice (#280). + + ``provider.model`` is the channel inference reads; guarded exactly + like the TUI's ``_open_model_picker`` (app.py). Persistence goes + through ``persist_model_choice``: via the reactive store when wired + (fires the side-effect router), else straight to user settings. + """ try: provider.model = model except Exception: pass + try: + from src.state.app_state import persist_model_choice + + persist_model_choice( + getattr(context, "app_state_store", None), + _provider_key(provider), + model, + ) + except Exception: + # The live switch already took effect; only restarts lose it. + import logging + + logging.getLogger(__name__).debug( + "model persistence failed", exc_info=True + ) @dataclass(frozen=True) @@ -161,7 +199,7 @@ async def _pick(self, context: CommandContext) -> InteractiveOutcome: return InteractiveOutcome( message=f"Kept model as {_label(current)}", display="system" ) - _apply(prov, picked) + _apply(prov, picked, context) return InteractiveOutcome(message=f"Set model to {_label(picked)}", display="user") def _set(self, context: CommandContext, arg: str) -> InteractiveOutcome: @@ -174,7 +212,7 @@ def _set(self, context: CommandContext, arg: str) -> InteractiveOutcome: # provider lists nothing (unknown provider) so a valid id still goes through. if models and canon not in models: return InteractiveOutcome(message=f"Model '{arg}' not found", display="system") - _apply(prov, canon) + _apply(prov, canon, context) return InteractiveOutcome(message=f"Set model to {_label(canon)}", display="user") diff --git a/src/entrypoints/headless.py b/src/entrypoints/headless.py index 3c05d611..cbd2ad36 100644 --- a/src/entrypoints/headless.py +++ b/src/entrypoints/headless.py @@ -135,7 +135,13 @@ def run_headless(options: HeadlessOptions) -> int: ) provider_cls = get_provider_class(provider_name) - model = options.model or provider_cfg.get("default_model") + from src.settings.settings import get_persisted_model + + model = ( + options.model + or get_persisted_model(provider_name) + or provider_cfg.get("default_model") + ) provider = provider_cls( api_key=provider_cfg["api_key"], base_url=provider_cfg.get("base_url"), diff --git a/src/entrypoints/tui.py b/src/entrypoints/tui.py index 64cbdb50..7f421a84 100644 --- a/src/entrypoints/tui.py +++ b/src/entrypoints/tui.py @@ -90,7 +90,13 @@ def run_tui(options: TUIOptions) -> int: 2, ) provider_cls = get_provider_class(provider_name) - model = options.model or provider_cfg.get("default_model") + from src.settings.settings import get_persisted_model + + model = ( + options.model + or get_persisted_model(provider_name) + or provider_cfg.get("default_model") + ) provider = provider_cls( api_key=provider_cfg["api_key"], base_url=provider_cfg.get("base_url"), diff --git a/src/repl/core.py b/src/repl/core.py index 61174a94..10c7ba13 100644 --- a/src/repl/core.py +++ b/src/repl/core.py @@ -416,12 +416,15 @@ def __init__( self.console.print("Run [bold]clawcodex login[/bold] to configure.") sys.exit(1) - # Initialize provider + # Initialize provider. The persisted /model choice (#280) wins + # over the provider's configured default. + from src.settings.settings import get_persisted_model + provider_class = get_provider_class(provider_name) self.provider = provider_class( api_key=config["api_key"], base_url=config.get("base_url"), - model=config.get("default_model") + model=get_persisted_model(provider_name) or config.get("default_model") ) # Create session diff --git a/src/settings/settings.py b/src/settings/settings.py index 6f6747d0..1e56018f 100644 --- a/src/settings/settings.py +++ b/src/settings/settings.py @@ -65,3 +65,45 @@ def get_settings( if _settings_cache is None: _settings_cache = load_settings(config_manager=config_manager, cwd=cwd) return _settings_cache + + +def get_persisted_model( + provider_name: str | None, + *, + cwd: str | Path | None = None, +) -> str | None: + """The user's persisted /model choice for ``provider_name``, or None. + + Restore channel for #280: entrypoints resolve the startup model as + ``cli option > persisted model > provider default_model``. The model + is restored only when the persisted ``model_provider`` pairing + matches the launch provider — model names are provider-scoped in a + multi-provider config, and feeding provider B a model persisted on + provider A would fail the first API call. + + Reads the config layers directly (most specific non-empty model + wins: local > project > global; an empty ``model`` does NOT mask a + less-specific layer) rather than the merged ``SettingsSchema`` — the + schema bakes in a DEFAULT_SETTINGS.model, which must NOT override a + provider's configured default for users who never ran /model. + Fail-soft — a broken settings file must not block startup. + """ + try: + manager = ConfigManager(cwd=cwd) + for cfg in ( + manager.load_local(), + manager.load_project(), + manager.load_global(), + ): + section = cfg.get("settings") + if isinstance(section, dict) and section.get("model"): + persisted_provider = section.get("model_provider") or "" + if provider_name and persisted_provider == provider_name: + return str(section["model"]) + # Unpaired or mismatched: the persisted model belongs to + # another provider (or predates the pairing) — fall back + # to the provider's own default. + return None + return None + except Exception: + return None diff --git a/src/settings/types.py b/src/settings/types.py index b2836f6f..7d4b005b 100644 --- a/src/settings/types.py +++ b/src/settings/types.py @@ -144,6 +144,19 @@ class SettingsSchema: # Fast mode (use small model) fast_mode: bool = False + # Display preferences persisted by the AppState side-effect router + # (#280; TS onChangeAppState.ts:123-136). ``expanded_view`` stores the + # Python store's own 'none' | 'tasks' | 'teammates' shape directly + # rather than the TS legacy showExpandedTodos/showSpinnerTree boolean + # pair — this port's config file is not shared with TS Claude Code. + verbose: bool = False + expanded_view: str = "" # "" = unset (AppState default applies) + # Provider key paired with the persisted ``model`` (#280): a /model + # choice is only restored when the next launch uses the same provider + # (the advisor_model/advisor_provider precedent — model names are + # provider-scoped in a multi-provider config). + model_provider: str = "" + # Disable dynamic workflows (also honored via CLAUDE_CODE_DISABLE_WORKFLOWS # and the camelCase ``disableWorkflows`` JSON key). See src/workflow/gating.py. disable_workflows: bool = False diff --git a/src/state/app_state.py b/src/state/app_state.py index 9c7e3812..c0c68dc7 100644 --- a/src/state/app_state.py +++ b/src/state/app_state.py @@ -75,6 +75,14 @@ class AppState: # Model selection (TS: state/AppStateStore.ts:93) main_loop_model: str | None = None + # Provider key (matches ``~/.clawcodex/config.json``'s providers map) + # that ``main_loop_model`` belongs to. Persisted alongside the model + # (#280): clawcodex is multi-provider and a persisted model is only + # restorable when the next launch uses the same provider (the + # advisor_model/advisor_provider precedent). Written together with + # main_loop_model via ``persist_model_choice``. + main_loop_provider: str | None = None + # Verbose mode (TS: AppStateStore.ts:92) verbose: bool = False @@ -125,8 +133,27 @@ def replace_state(state: AppState, **changes: Any) -> AppState: def get_default_app_state() -> AppState: - """Mirror of TS ``getDefaultAppState`` (``AppStateStore.ts:458``).""" - return AppState() + """Mirror of TS ``getDefaultAppState`` (``AppStateStore.ts:458``). + + Display preferences persisted by the side-effect router (#280) + are seeded back here so /verbose and the expanded-view toggle + survive restarts. ``main_loop_model`` is deliberately NOT seeded: + entrypoints own model resolution (cli option > settings.model > + provider default_model) and pass it via ``provider.model``. + """ + state = AppState() + try: + from src.settings.settings import get_settings + + settings = get_settings() + if settings.verbose: + state = replace_state(state, verbose=True) + if settings.expanded_view in ("none", "tasks", "teammates"): + state = replace_state(state, expanded_view=settings.expanded_view) + except Exception: + # Settings load failures must not block startup; defaults apply. + logger.debug("could not seed AppState from settings", exc_info=True) + return state # --------------------------------------------------------------------------- @@ -134,18 +161,56 @@ def get_default_app_state() -> AppState: # --------------------------------------------------------------------------- +def _persist_user_settings(**values: Any) -> None: + """Write ``values`` into the user-scoped global config's ``settings`` + section and invalidate the read cache — the single writer chokepoint + (#280; TS ``updateSettingsForSource('userSettings', ...)``). + + Failures are logged, never propagated: the in-memory store update + already took effect for this process; only re-launches would lose + the setting. + """ + # Local imports avoid making this module's import time pay for the + # settings stack on cold start. Use the SHARED default ConfigManager + # so callers reading via ``load_config()`` / the global cache see + # the new value, not a stale in-memory snapshot from before the + # write. + from src import config as cfg_mod + from src.settings.settings import invalidate_settings_cache + + try: + mgr = cfg_mod._get_default_manager() + cfg = mgr.load_global() + settings_section = cfg.get("settings") + # Copy before mutating: load_global returns a shallow copy, so an + # in-place update would leak unsaved values into the manager's + # cache if save_global fails. + if isinstance(settings_section, dict): + settings_section = dict(settings_section) + else: + settings_section = {} + settings_section.update(values) + cfg["settings"] = settings_section + mgr.save_global(cfg) + invalidate_settings_cache() + logger.debug("persisted user settings %s + cache invalidated", values) + except Exception: + logger.exception( + "Failed to persist %s to settings; in-memory value still active", + sorted(values.keys()), + ) + + def _on_main_loop_model_change(old: AppState, new: AppState) -> None: - """Mirror model choice into bootstrap singleton. + """Mirror model choice into bootstrap singleton + persist (#280). Matches TS at ``onChangeAppState.ts:97-120`` — when the user changes the model (via /model slash command or the model picker), the bootstrap-state override must update so the next API call reads the new value, and settings persistence happens as a side effect. - - Settings persistence is currently no-op — the settings.json layering - lives in ``src.settings`` and the writer is not yet wired through - a single chokepoint. Plan §P2.1 left this stub here as the wiring - target. + ``settings.model`` is the restore channel: entrypoints resolve the + startup model as ``cli option > settings.model > provider + default_model``. """ if old.main_loop_model == new.main_loop_model: return @@ -155,27 +220,66 @@ def _on_main_loop_model_change(old: AppState, new: AppState) -> None: old.main_loop_model, new.main_loop_model, ) - # TODO: persist to user settings via ``src.settings`` once the - # writer-side has a single chokepoint. + _persist_user_settings(model=new.main_loop_model or "") + + +def _on_main_loop_provider_change(old: AppState, new: AppState) -> None: + """Persist the provider key paired with the model choice (#280). + + ``get_persisted_model`` only restores a model whose persisted + provider matches the launch provider — without the pairing, a model + persisted on provider A would feed provider B an invalid model id + on the next launch. + """ + if old.main_loop_provider == new.main_loop_provider: + return + _persist_user_settings(model_provider=new.main_loop_provider or "") + + +def persist_model_choice( + store: Any, provider_name: str | None, model: str | None +) -> None: + """Single write path for a user model choice (#280). + + When a reactive store is wired, write through it so the side-effect + router persists (and mirrors bootstrap); otherwise persist directly + via the same chokepoint the handlers use — the ``/advisor`` command + pattern (``builtins._persist_advisor_model``). + """ + if store is not None: + store.set_state( + lambda s: replace_state( + s, + main_loop_model=model or None, + main_loop_provider=provider_name or None, + ) + ) + return + set_main_loop_model_override(model or None) + _persist_user_settings( + model=model or "", model_provider=provider_name or "" + ) def _on_verbose_change(old: AppState, new: AppState) -> None: if old.verbose == new.verbose: return logger.debug("AppState.verbose %s -> %s", old.verbose, new.verbose) - # TODO: persist to global config. + _persist_user_settings(verbose=bool(new.verbose)) def _on_expanded_view_change(old: AppState, new: AppState) -> None: if old.expanded_view == new.expanded_view: return logger.debug( - "AppState.expanded_view %s -> %s — persist as showExpandedTodos/showSpinnerTree", + "AppState.expanded_view %s -> %s — persisted", old.expanded_view, new.expanded_view, ) - # TODO: persist to global config as showExpandedTodos + - # showSpinnerTree (TS: onChangeAppState.ts:123-136). + # Persisted as the store's own 'none' | 'tasks' | 'teammates' string + # rather than the TS legacy showExpandedTodos/showSpinnerTree boolean + # pair (this port's config file is not shared with TS Claude Code). + _persist_user_settings(expanded_view=new.expanded_view or "none") # Permission-mode notification hooks. Real listeners (CCR bridge, SDK @@ -252,38 +356,9 @@ def _on_advisor_model_change(old: AppState, new: AppState) -> None: """ if old.advisor_model == new.advisor_model: return - # Local imports avoid making this module's import time pay for the - # settings stack on cold start. Use the SHARED default ConfigManager - # so callers reading via ``load_config()`` / the global cache see - # the new value, not a stale in-memory snapshot from before the - # write. - from src import config as cfg_mod - from src.settings.settings import invalidate_settings_cache - try: - mgr = cfg_mod._get_default_manager() - cfg = mgr.load_global() - settings_section = cfg.get("settings") - if not isinstance(settings_section, dict): - settings_section = {} - # None / "" both map to "unset" — write empty string for - # round-trip fidelity with the SettingsSchema default. - settings_section["advisor_model"] = new.advisor_model or "" - cfg["settings"] = settings_section - mgr.save_global(cfg) - invalidate_settings_cache() - logger.debug( - "AppState.advisor_model %s -> %s — persisted + cache invalidated", - old.advisor_model, - new.advisor_model, - ) - except Exception: - # The slash command already reported success to the user based on - # the in-memory store update; surface persistence failures via the - # log but don't propagate (the in-memory change still works for - # the current process; only re-launches would lose the setting). - logger.exception( - "Failed to persist advisor_model to settings; in-memory value still active" - ) + # None / "" both map to "unset" — write empty string for round-trip + # fidelity with the SettingsSchema default. + _persist_user_settings(advisor_model=new.advisor_model or "") def _on_advisor_provider_change(old: AppState, new: AppState) -> None: @@ -297,27 +372,7 @@ def _on_advisor_provider_change(old: AppState, new: AppState) -> None: """ if old.advisor_provider == new.advisor_provider: return - from src import config as cfg_mod - from src.settings.settings import invalidate_settings_cache - try: - mgr = cfg_mod._get_default_manager() - cfg = mgr.load_global() - settings_section = cfg.get("settings") - if not isinstance(settings_section, dict): - settings_section = {} - settings_section["advisor_provider"] = new.advisor_provider or "" - cfg["settings"] = settings_section - mgr.save_global(cfg) - invalidate_settings_cache() - logger.debug( - "AppState.advisor_provider %s -> %s — persisted + cache invalidated", - old.advisor_provider, - new.advisor_provider, - ) - except Exception: - logger.exception( - "Failed to persist advisor_provider to settings; in-memory value still active" - ) + _persist_user_settings(advisor_provider=new.advisor_provider or "") def _on_advisor_client_mode_change(old: AppState, new: AppState) -> None: @@ -330,27 +385,7 @@ def _on_advisor_client_mode_change(old: AppState, new: AppState) -> None: """ if old.advisor_client_mode == new.advisor_client_mode: return - from src import config as cfg_mod - from src.settings.settings import invalidate_settings_cache - try: - mgr = cfg_mod._get_default_manager() - cfg = mgr.load_global() - settings_section = cfg.get("settings") - if not isinstance(settings_section, dict): - settings_section = {} - settings_section["advisor_client_mode"] = bool(new.advisor_client_mode) - cfg["settings"] = settings_section - mgr.save_global(cfg) - invalidate_settings_cache() - logger.debug( - "AppState.advisor_client_mode %s -> %s — persisted + cache invalidated", - old.advisor_client_mode, - new.advisor_client_mode, - ) - except Exception: - logger.exception( - "Failed to persist advisor_client_mode to settings; in-memory value still active" - ) + _persist_user_settings(advisor_client_mode=bool(new.advisor_client_mode)) # Registry. EVERY field in AppState must appear here as a handler @@ -359,6 +394,7 @@ def _on_advisor_client_mode_change(old: AppState, new: AppState) -> None: # ``test_every_field_appears_in_handler_registry``. _FIELD_HANDLERS: dict[str, Callable[[AppState, AppState], None]] = { "main_loop_model": _on_main_loop_model_change, + "main_loop_provider": _on_main_loop_provider_change, "verbose": _on_verbose_change, "expanded_view": _on_expanded_view_change, "permission_mode": _on_permission_mode_change, @@ -414,6 +450,7 @@ def create_app_state_store( "create_app_state_store", "get_default_app_state", "on_change_app_state", + "persist_model_choice", "replace_state", "set_permission_mode_listener", "set_session_metadata_listener", diff --git a/src/tui/app.py b/src/tui/app.py index 5b1fe3d4..1082625b 100644 --- a/src/tui/app.py +++ b/src/tui/app.py @@ -1087,6 +1087,17 @@ def _on_selected(model_id: str | None) -> None: except Exception: pass self.app_state.model = model_id + # Persist the choice so it survives restarts (#280) — + # through the reactive store when one exists (fires the + # side-effect router), else directly to user settings. + try: + from src.state.app_state import persist_model_choice + + persist_model_choice( + self._app_state_store, self.provider_name, model_id + ) + except Exception: + pass transcript.append_system(f"Model switched to {model_id}.", style="muted") if self._repl_screen is not None: self._repl_screen.status_bar.refresh_identity(model=model_id) diff --git a/tests/test_app_state_persistence.py b/tests/test_app_state_persistence.py new file mode 100644 index 00000000..fa4b2da8 --- /dev/null +++ b/tests/test_app_state_persistence.py @@ -0,0 +1,243 @@ +"""#280 — AppState model/verbose/expanded-view persistence round-trips. + +The side-effect router must persist /model, /verbose, and the +expanded-view toggle into the user-scoped global config's ``settings`` +section, and the read side must restore them: ``get_default_app_state`` +seeds verbose/expanded_view, and ``get_persisted_model`` feeds the +entrypoints' model-resolution chain (cli option > settings.model > +provider default_model). +""" +from __future__ import annotations + +import tempfile +import unittest +from pathlib import Path +from unittest.mock import patch + +from src.state.app_state import ( + AppState, + get_default_app_state, + on_change_app_state, + persist_model_choice, + replace_state, +) + + +class _IsolatedEnv: + """Isolate ALL config persistence to a tmp dir (the + test_advisor_command.py pattern: the module-level path constants are + evaluated at import time, so they're swapped directly). ``tmp`` is + exposed so tests can pass it as the ConfigManager cwd, keeping the + project/local layers hermetic too (the repo itself could grow a + .claude/config.json one day).""" + + @property + def tmp(self) -> Path: + return self._tmp + + def __enter__(self): + import src.config as cfg_mod + + self._tmp = Path(tempfile.mkdtemp(prefix="appstate_persist_")) + self._saved_global_path = cfg_mod.GLOBAL_CONFIG_FILE + self._saved_history_path = cfg_mod.HISTORY_FILE + cfg_mod.GLOBAL_CONFIG_FILE = self._tmp / ".clawcodex" / "config.json" + cfg_mod.HISTORY_FILE = self._tmp / ".clawcodex" / "history.jsonl" + cfg_mod.GLOBAL_CONFIG_DIR = self._tmp / ".clawcodex" + cfg_mod._default_manager = None + from src.settings.settings import invalidate_settings_cache + + invalidate_settings_cache() + return self + + def __exit__(self, *a): + import src.config as cfg_mod + + cfg_mod.GLOBAL_CONFIG_FILE = self._saved_global_path + cfg_mod.HISTORY_FILE = self._saved_history_path + cfg_mod.GLOBAL_CONFIG_DIR = self._saved_global_path.parent + cfg_mod._default_manager = None + from src.settings.settings import invalidate_settings_cache + + invalidate_settings_cache() + + +def _fire(field: str, value) -> None: + """Run the side-effect router for a single-field change.""" + old = AppState() + new = replace_state(old, **{field: value}) + on_change_app_state(old, new) + + +class TestModelPersistence(unittest.TestCase): + def test_model_choice_round_trips_for_matching_provider(self) -> None: + with _IsolatedEnv() as env: + persist_model_choice(None, "anthropic", "claude-opus-4-7") + from src.settings.settings import ( + get_persisted_model, + get_settings, + invalidate_settings_cache, + ) + + invalidate_settings_cache() # simulate restart + self.assertEqual(get_settings().model, "claude-opus-4-7") + self.assertEqual(get_settings().model_provider, "anthropic") + self.assertEqual( + get_persisted_model("anthropic", cwd=env.tmp), + "claude-opus-4-7", + ) + + def test_model_not_restored_for_other_provider(self) -> None: + # A model persisted on provider A must not feed provider B an + # invalid model id at the next launch. + with _IsolatedEnv() as env: + persist_model_choice(None, "anthropic", "claude-opus-4-7") + from src.settings.settings import ( + get_persisted_model, + invalidate_settings_cache, + ) + + invalidate_settings_cache() + self.assertIsNone(get_persisted_model("glm", cwd=env.tmp)) + + def test_store_path_fires_router_and_persists_pair(self) -> None: + with _IsolatedEnv() as env: + from src.state.app_state import create_app_state_store + + store = create_app_state_store() + persist_model_choice(store, "glm", "zai/glm-5") + from src.settings.settings import ( + get_persisted_model, + invalidate_settings_cache, + ) + + invalidate_settings_cache() + self.assertEqual( + get_persisted_model("glm", cwd=env.tmp), "zai/glm-5" + ) + + def test_clearing_model_persists_unset(self) -> None: + with _IsolatedEnv() as env: + persist_model_choice(None, "anthropic", "claude-opus-4-7") + persist_model_choice(None, "anthropic", None) + from src.settings.settings import ( + get_persisted_model, + invalidate_settings_cache, + ) + + invalidate_settings_cache() + self.assertIsNone(get_persisted_model("anthropic", cwd=env.tmp)) + + def test_get_persisted_model_none_when_unset(self) -> None: + with _IsolatedEnv() as env: + from src.settings.settings import get_persisted_model + + self.assertIsNone(get_persisted_model("anthropic", cwd=env.tmp)) + + def test_unpaired_legacy_model_is_not_restored(self) -> None: + # A hand-edited settings.model with no model_provider pairing + # cannot be trusted in a multi-provider config. + with _IsolatedEnv() as env: + import src.config as cfg_mod + + mgr = cfg_mod._get_default_manager() + cfg = mgr.load_global() + cfg["settings"] = {"model": "claude-opus-4-7"} + mgr.save_global(cfg) + from src.settings.settings import ( + get_persisted_model, + invalidate_settings_cache, + ) + + invalidate_settings_cache() + self.assertIsNone(get_persisted_model("anthropic", cwd=env.tmp)) + + +class TestVerbosePersistence(unittest.TestCase): + def test_verbose_round_trips_through_default_state(self) -> None: + with _IsolatedEnv(): + _fire("verbose", True) + from src.settings.settings import invalidate_settings_cache + + invalidate_settings_cache() # simulate restart + self.assertTrue(get_default_app_state().verbose) + + def test_verbose_off_round_trips(self) -> None: + with _IsolatedEnv(): + _fire("verbose", True) + old = replace_state(AppState(), verbose=True) + on_change_app_state(old, replace_state(old, verbose=False)) + from src.settings.settings import invalidate_settings_cache + + invalidate_settings_cache() + self.assertFalse(get_default_app_state().verbose) + + +class TestExpandedViewPersistence(unittest.TestCase): + def test_expanded_view_round_trips_through_default_state(self) -> None: + with _IsolatedEnv(): + _fire("expanded_view", "tasks") + from src.settings.settings import invalidate_settings_cache + + invalidate_settings_cache() # simulate restart + self.assertEqual(get_default_app_state().expanded_view, "tasks") + + def test_unknown_persisted_value_falls_back_to_default(self) -> None: + with _IsolatedEnv(): + import src.config as cfg_mod + + mgr = cfg_mod._get_default_manager() + cfg = mgr.load_global() + cfg["settings"] = {"expanded_view": "bogus-value"} + mgr.save_global(cfg) + from src.settings.settings import invalidate_settings_cache + + invalidate_settings_cache() + self.assertEqual(get_default_app_state().expanded_view, "none") + + +class TestPersistenceIsFailSoft(unittest.TestCase): + def test_write_failure_does_not_propagate(self) -> None: + with _IsolatedEnv(): + import src.config as cfg_mod + + mgr = cfg_mod._get_default_manager() + with patch.object( + type(mgr), "save_global", side_effect=OSError("disk full") + ): + # Must not raise — the in-memory change still works. + _fire("verbose", True) + + def test_settings_load_failure_does_not_block_default_state(self) -> None: + with patch( + "src.settings.settings.get_settings", + side_effect=RuntimeError("corrupt settings"), + ): + state = get_default_app_state() + self.assertFalse(state.verbose) + self.assertEqual(state.expanded_view, "none") + + +class TestAdvisorHandlersStillPersist(unittest.TestCase): + """The advisor handlers were deduped onto the chokepoint — pin that + their persistence semantics are unchanged.""" + + def test_advisor_fields_round_trip(self) -> None: + with _IsolatedEnv(): + _fire("advisor_model", "claude-opus-4-6") + _fire("advisor_provider", "anthropic") + _fire("advisor_client_mode", True) + from src.settings.settings import ( + get_settings, + invalidate_settings_cache, + ) + + invalidate_settings_cache() + settings = get_settings() + self.assertEqual(settings.advisor_model, "claude-opus-4-6") + self.assertEqual(settings.advisor_provider, "anthropic") + self.assertTrue(settings.advisor_client_mode) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_model_command.py b/tests/test_model_command.py index 634c3b4a..d58918df 100644 --- a/tests/test_model_command.py +++ b/tests/test_model_command.py @@ -105,6 +105,17 @@ def _no_effort(monkeypatch): ) +@pytest.fixture(autouse=True) +def _isolated_config(tmp_path, monkeypatch): + """#280: ``_apply`` now persists the model choice to the global + config — point the writer at a per-test file (the theme/vim-command + pattern) so tests never touch the developer's real config.""" + import src.config as cfg + + monkeypatch.setattr(cfg, "GLOBAL_CONFIG_FILE", tmp_path / "config.json") + monkeypatch.setattr(cfg, "_default_manager", cfg.ConfigManager(cwd=tmp_path)) + + # --------------------------------------------------------------------------- # # A. Metadata + registration # --------------------------------------------------------------------------- # @@ -299,3 +310,34 @@ def test_repl_wires_provider_into_command_context(): src_text = inspect.getsource(ClawcodexREPL._init_command_system) assert "provider=self.provider" in src_text + + +# --------------------------------------------------------------------------- # +# H. #280: the choice persists for the next launch +# --------------------------------------------------------------------------- # +async def test_set_persists_choice_for_restart(tmp_path): + """/model writes settings.model paired with the provider key; + get_persisted_model restores it for the same provider only.""" + from src.providers.anthropic_provider import AnthropicProvider + + prov = AnthropicProvider(api_key="test", model="claude-sonnet-4-6") + out = await MODEL_COMMAND.run("claude-opus-4-6", _ctx(tmp_path, provider=prov)) + assert "Set model to" in out.message + + from src.settings.settings import get_persisted_model, invalidate_settings_cache + + invalidate_settings_cache() # simulate restart + assert get_persisted_model("anthropic", cwd=tmp_path) == "claude-opus-4-6" + assert get_persisted_model("glm", cwd=tmp_path) is None + + +async def test_set_with_unknown_provider_class_does_not_pair(tmp_path): + """FakeProvider isn't a registered provider class — the model is + written unpaired and therefore not restored (fail-safe).""" + prov = FakeProvider(model=_SONNET) + await MODEL_COMMAND.run("opus", _ctx(tmp_path, provider=prov)) + + from src.settings.settings import get_persisted_model, invalidate_settings_cache + + invalidate_settings_cache() + assert get_persisted_model("anthropic", cwd=tmp_path) is None