diff --git a/CHANGELOG.md b/CHANGELOG.md index 084a574b..e9faff4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,46 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`/model` listed one provider instead of every configured one.** Step 1 of + the picker showed a single row — `anthropic · 22 models` — no matter how many + providers were set up. `model.options` was a stub: it called the + `get_settings` control, which describes only the provider the session is + *currently* on, and synthesized a hardcoded one-element array from it. The + 26-entry registry (`PROVIDER_INFO`, hand-written provider classes plus the + data-driven OpenAI-compatible specs) was never enumerated, and no backend + control exposed it, so the row on screen was the active provider echoed back + at itself. A new `list_model_providers` control returns the real catalog, + ordered active-first, then providers that can authenticate, then the rest. + + Three paths behind it had never been reachable, and two were broken: + + - Selecting a model from another provider was refused. `set_model` will not + point the live provider at a foreign model id — a cross-provider switch + needs the registry rebuild only `set_provider` performs — so its refusal + now carries a machine-readable `provider_mismatch` and the client re-drives + the switch through `set_provider` before re-applying the model. If that + second step fails it rolls back and reports which provider the session + actually ended on. + - `model.save_key` was never wired and fell through to the adapter's default + case, so inline API-key entry always answered "failed to save key". Keys + now persist where `clawcodex login` writes them. + - `model.disconnect` was likewise unwired, so `^d` silently did nothing. + + Disconnect deletes only environment-variable names a provider *owns*. The + candidate list is a resolution preference, not a claim of ownership — + `nvidia-nim` accepts `DEEPSEEK_API_KEY` — so treating it as a delete-set + would destroy a credential set for something else, including the provider the + session is running on. Ownership follows the primary candidate, contested + names are reported rather than removed, and a key the live session + authenticates through is never touched. A shell export sitting behind a + config key is detected before deletion, since removing the config copy makes + the provider look disconnected while the export restores it on next launch. + + Known limitation: when no provider is configured at all the session fails to + start, and the `init_error` guard short-circuits every control — including + this one — so the picker cannot yet be used to paste a first key. It now + reports that reason instead of inventing a provider row. + - **`--model`/`/model` selection is now resolved from one rule at every entrypoint.** The persisted `/model` choice was applied only in the interactive agent-server, and only *after* the provider was constructed diff --git a/src/providers/catalog.py b/src/providers/catalog.py new file mode 100644 index 00000000..09cc9a97 --- /dev/null +++ b/src/providers/catalog.py @@ -0,0 +1,175 @@ +"""Provider catalog for the ``/model`` picker's step 1. + +``get_settings`` reports only the provider the session is *currently* on, so a +picker built on it can never show more than one row. This module enumerates +:data:`src.providers.PROVIDER_INFO` — the merged registry of hand-written +provider classes plus the data-driven OpenAI-compatible specs — which is +exactly the set ``--provider`` accepts. + +Each row carries what the picker renders: the ○/●/* auth mark +(``authenticated`` / ``is_current``), the model list, and the fields that drive +the inline API-key stage (``auth_type`` == ``"api_key"`` plus ``key_env``). + +Credential probing is read-only and best-effort: a provider whose key cannot be +resolved is reported unauthenticated rather than raising, so one bad config +block cannot blank the whole picker. +""" + +from __future__ import annotations + +from typing import Any + + +def exclusive_env_vars(pid: str) -> tuple[list[str], list[str]]: + """Split a provider's key env-vars into ``(exclusively-own, shared)``. + + ``provider_env_vars`` is a *resolution preference* list — "any of these + will authenticate me" — not an ownership list, and several providers + legitimately name another vendor's variable: ``nvidia-nim`` accepts + ``DEEPSEEK_API_KEY``, ``siliconflow`` and ``siliconflow-cn`` share + ``SILICONFLOW_API_KEY``, ``huggingface`` reads ``HF_TOKEN``, ``gemini`` + reads ``GOOGLE_API_KEY``. + + The config ``env`` block is one global secret namespace, so treating that + preference list as a delete-set destroys credentials the user set for a + *different* provider — including, in the ``nvidia-nim`` → ``DEEPSEEK`` + case, the provider the session is actively running on, which would route + straight around the active-provider guard. + + Ownership follows the PRIMARY candidate, not mere membership. A plain + "does anyone else list this name" test over-corrects into uselessness: + ``DEEPSEEK_API_KEY`` is DeepSeek's own primary variable but also + ``nvidia-nim``'s last-resort fallback, so membership alone would make + DeepSeek — the provider this project is built around — permanently + undisconnectable. A name is this provider's when it heads its own list and + no other provider also leads with it, or when nobody else names it at all. + Genuinely contested names (``siliconflow`` and ``siliconflow-cn`` both lead + with ``SILICONFLOW_API_KEY``) stay shared and are reported, not deleted. + """ + from . import PROVIDER_INFO, provider_env_vars + + mine = list(provider_env_vars(pid)) + primary = mine[0] if mine else None + others_all: set[str] = set() + others_primary: set[str] = set() + for other in PROVIDER_INFO: + if other == pid: + continue + candidates = provider_env_vars(other) + others_all.update(candidates) + if candidates: + others_primary.add(candidates[0]) + + owned: list[str] = [] + shared: list[str] = [] + for name in mine: + if (name == primary and name not in others_primary) or name not in others_all: + owned.append(name) + else: + shared.append(name) + return owned, shared + + +def provider_catalog( + current: str | None = None, + current_models: list[str] | None = None, + current_ready: bool = False, +) -> list[dict[str, Any]]: + """Every known provider, ordered for the picker. + + ``current`` is the session's active provider id (its row gets + ``is_current``). ``current_models`` overrides that provider's static model + list with the session's live one, so endpoint-discovered catalogues + (Ollama / vLLM / SGLang) show their real models instead of the static stub. + ``current_ready`` marks the active provider authenticated because the + session demonstrably constructed it. + + Credential probing deliberately mirrors ``get_provider_config``'s literal + lookup rather than trying to be cleverer than it, because + ``_do_set_provider`` resolves the same way: a row reported here as + authenticated but which ``set_provider`` then refuses would be a picker + offering a provider you cannot actually select. + + The ACTIVE provider is the one exception. A session launched as + ``--provider glm`` has a working ``[providers.glm]`` block that a canonical + ``zai`` lookup cannot see — the packaged defaults seed an empty ``zai`` + block that wins the literal lookup — so probing it would render the user's + own running provider as needing a key. + + Ordering: the active provider first, then providers that can authenticate, + then the rest alphabetically — so the useful rows are above the fold and + the unconfigured long tail sorts predictably. + """ + from . import ( + PROVIDER_INFO, + canonical_provider_name, + provider_env_vars, + provider_has_credentials, + provider_requires_api_key, + resolve_api_key, + ) + + current_id = canonical_provider_name(current) if current else None + + rows: list[dict[str, Any]] = [] + for pid, info in PROVIDER_INFO.items(): + is_current = pid == current_id + try: + api_key = resolve_api_key(pid) + except Exception: # noqa: BLE001 — one bad block must not blank the picker + api_key = "" + try: + authenticated = provider_has_credentials(pid, api_key) + except Exception: # noqa: BLE001 + authenticated = bool(api_key) + if is_current and current_ready: + authenticated = True + + env_vars = provider_env_vars(pid) + key_env = env_vars[0] if env_vars else "" + + models = [str(m) for m in (info.get("available_models") or [])] + if is_current and current_models: + models = [str(m) for m in current_models] + + row: dict[str, Any] = { + "slug": pid, + "name": info.get("label") or pid, + "models": models, + "total_models": len(models), + "authenticated": authenticated, + # Only providers that actually take a key get the picker's inline + # key stage; local servers (Ollama / vLLM / SGLang) need none. + "auth_type": "api_key" if provider_requires_api_key(pid) else "none", + "is_current": is_current, + } + if key_env: + row["key_env"] = key_env + # What ^d would actually delete, so the confirm screen can name it + # rather than saying a vague "removes saved credentials". Several of + # these are conventional variables other tooling reads (HF_TOKEN, + # GOOGLE_API_KEY), so the user deserves to see the list before + # confirming — this only ever clears clawcodex's own config, never a + # shell profile, but an informed choice beats a narrower rule. + owned_env, _shared_env = exclusive_env_vars(pid) + if current_id: + # Subtract what the live session authenticates through: the + # handler declines to delete those, so listing them here would + # promise a removal the confirm screen then does not perform — + # in the field this whole disclosure exists to make honest. + in_use = set(provider_env_vars(current_id)) + owned_env = [e for e in owned_env if e not in in_use] + if owned_env: + row["removes_env"] = owned_env + if not authenticated: + row["warning"] = ( + f"paste {key_env} to activate" + if key_env + else "run `clawcodex login` to configure" + ) + rows.append(row) + + rows.sort( + key=lambda r: (not r["is_current"], not r["authenticated"], str(r["name"]).lower()) + ) + return rows diff --git a/src/server/agent_server.py b/src/server/agent_server.py index 98485e30..b7b7565c 100644 --- a/src/server/agent_server.py +++ b/src/server/agent_server.py @@ -512,6 +512,17 @@ async def _handle_control_request(self, msg: dict) -> None: if subtype == "set_provider": self._do_set_provider(request_id, inner.get("provider")) return + if subtype == "list_model_providers": + self._do_list_model_providers(request_id) + return + if subtype == "save_provider_key": + self._do_save_provider_key( + request_id, inner.get("slug"), inner.get("api_key") + ) + return + if subtype == "disconnect_provider": + self._do_disconnect_provider(request_id, inner.get("slug")) + return if subtype == "set_output_style": self._do_set_output_style(request_id, inner.get("style")) return @@ -1431,9 +1442,29 @@ def _do_set_model(self, request_id: object, model: object, provider: object = No # vision wrapper has to be installed around it. if self._do_set_fusion_model(request_id, model): return - if isinstance(provider, str) and provider and provider != self.provider_name: + # Canonicalized on BOTH sides: a session launched as ``--provider glm`` + # keeps that spelling in ``provider_name`` while the picker's rows + # carry the canonical ``zai``, so a raw string compare would refuse a + # same-provider switch and then send the client off to "switch" to a + # provider it is already on. + from src.providers import canonical_provider_name as _canonical + + if ( + isinstance(provider, str) + and provider + and _canonical(provider) != _canonical(self.provider_name or "") + ): + # ``provider_mismatch`` is the machine-readable half of this + # refusal: the /model picker legitimately selects a model from + # another provider (step 1 picks the provider, step 2 the model), + # and its client re-drives the switch through ``set_provider`` + # — which does the registry rebuild this handler deliberately + # will not — before re-applying the model. Without the flag the + # client would have to pattern-match the error prose. self._reply(request_id, { "ok": False, + "provider_mismatch": True, + "provider": self.provider_name, "error": f"model '{model}' expects provider '{provider}' but this " f"session is on '{self.provider_name}'", }) @@ -1604,6 +1635,303 @@ def _do_set_provider(self, request_id: object, name: object) -> None: logger.exception("[agent-server] set_provider failed") self._reply(request_id, {"ok": False, "error": str(exc)}) + def _do_list_model_providers(self, request_id: object) -> None: + """Every provider ClawCodex knows about — the /model picker's step 1. + + ``get_settings`` reports only the ACTIVE provider, so a picker built on + it can only ever render one row (the bug this fixes: the list showed + `anthropic · 22 models` and nothing else). The catalog enumerates the + real registry instead, and the active provider's row carries this + session's live model list so endpoint-discovered catalogues show their + real models. + """ + try: + from src.providers.catalog import provider_catalog + + providers = provider_catalog( + current=self.provider_name, + current_models=self._available_models(), + current_ready=self.provider is not None, + ) + except Exception as exc: # noqa: BLE001 + logger.exception("[agent-server] list_model_providers failed") + self._reply(request_id, {"ok": False, "error": str(exc), "providers": []}) + return + self._reply(request_id, { + "ok": True, + "model": getattr(self.provider, "model", None), + # Same channel get_settings uses: on a fused session ``model`` is + # the BASE id that serves the turn, so without this the picker + # would mark the base row current instead of the fusion entry the + # user actually selected. + "fusion": self._active_fusion_name(), + "provider": self.provider_name, + "providers": providers, + }) + + def _do_save_provider_key( + self, request_id: object, slug: object, api_key: object + ) -> None: + """Persist an API key typed into the picker's inline key stage. + + Writes where ``clawcodex login`` writes (the global config's + ``providers.`` block) so the two paths agree and a key set here + survives a restart. An existing ``base_url`` / ``default_model`` is + preserved — seeding the defaults unconditionally would silently + clobber a user's custom endpoint. + + The existing block is read from the GLOBAL tier, never the merged + view. ``get_provider_config`` merges the project/local tiers, and this + writes globally — so reading merged would launder a repo-committable + ``providers.*.base_url`` into the user's global config, permanently and + for every other project, paired with the key they just typed. That is + the exact redirect ``_UNTRUSTED_TIER_BLOCKED_KEYS`` exists to contain. + """ + if not isinstance(slug, str) or not slug.strip(): + self._reply(request_id, {"ok": False, "error": "missing provider"}) + return + if not isinstance(api_key, str) or not api_key.strip(): + self._reply(request_id, {"ok": False, "error": "missing api key"}) + return + try: + from src.config import _get_default_manager, set_api_key + from src.providers import PROVIDER_INFO, canonical_provider_name + from src.providers.catalog import provider_catalog + + pid = canonical_provider_name(slug.strip()) + info = PROVIDER_INFO.get(pid) + if info is None: + self._reply( + request_id, {"ok": False, "error": f"unknown provider '{slug}'"} + ) + return + global_blocks = _get_default_manager().load_global().get("providers") or {} + existing = global_blocks.get(pid) or {} + if not isinstance(existing, dict): + existing = {} + set_api_key( + pid, + api_key=api_key.strip(), + base_url=existing.get("base_url") or info.get("default_base_url"), + default_model=( + existing.get("default_model") or info.get("default_model") + ), + ) + row = next( + ( + r + for r in provider_catalog( + current=self.provider_name, + current_models=self._available_models(), + current_ready=self.provider is not None, + ) + if r["slug"] == pid + ), + None, + ) + except Exception as exc: # noqa: BLE001 + logger.exception("[agent-server] save_provider_key failed") + self._reply(request_id, {"ok": False, "error": str(exc)}) + return + self._reply(request_id, {"ok": True, "provider": row}) + + def _do_disconnect_provider(self, request_id: object, slug: object) -> None: + """Clear a provider's stored credentials (the picker's ^d). + + Refuses the ACTIVE provider: this session holds an already-constructed + provider instance, so pulling its key would leave the next turn firing + at an endpoint it can no longer authenticate against. Switch away + first. + + Clears all three places a key can live — the config ``providers.`` + block, the config ``env`` block, and a subscription OAuth login — then + re-probes. A key exported in the real shell environment cannot be + removed from here, so that case is reported honestly rather than + claiming a disconnect that did not happen. + """ + if not isinstance(slug, str) or not slug.strip(): + self._reply(request_id, {"ok": False, "error": "missing provider"}) + return + try: + from src.config import _get_default_manager + from src.providers import ( + PROVIDER_INFO, + canonical_provider_name, + provider_env_vars, + provider_has_credentials, + provider_requires_api_key, + resolve_api_key, + ) + from src.providers.catalog import exclusive_env_vars + from src.secret_store import delete_secret, get_secret + + pid = canonical_provider_name(slug.strip()) + if pid not in PROVIDER_INFO: + self._reply( + request_id, {"ok": False, "error": f"unknown provider '{slug}'"} + ) + return + if pid == canonical_provider_name(self.provider_name or ""): + self._reply(request_id, { + "ok": False, + "disconnected": False, + "error": f"'{pid}' is the active provider — switch to another " + "provider before disconnecting it", + }) + return + + removed = False + mgr = _get_default_manager() + cfg = mgr.load_global() + blocks = cfg.get("providers") + if isinstance(blocks, dict): + # Rebuild rather than pop in place: load_global returns a + # SHALLOW copy, so mutating a nested block reaches the + # manager's cache and can desync it from disk when nothing + # ends up being written. + rebuilt = dict(blocks) + touched = False + for key, block in blocks.items(): + if not isinstance(block, dict): + continue + if canonical_provider_name(key) != pid: + continue + if str(block.get("api_key") or "").strip(): + stripped = {k: v for k, v in block.items() if k != "api_key"} + rebuilt[key] = stripped + touched = True + if touched: + cfg["providers"] = rebuilt + mgr.save_global(cfg) + removed = True + # Sampled BEFORE the delete loop. ``delete_secret`` pops the name + # from ``os.environ`` as well as the config block, so reading this + # afterwards would find nothing precisely when the name exists in + # BOTH places — and that is the case that matters: the config copy + # goes, the shell export survives to resurrect it next launch, and + # the reply would have claimed a clean disconnect. + # + # Presence in os.environ alone does NOT mean the user exported it: + # ``set_secret`` mirrors every config-block write into the live + # process. The value is what separates them — a mirrored secret + # matches its config entry, a shell export does not (a name absent + # from the block compares against ""). + import os as _os + + from src.secret_store import CONFIG_ENV_KEY + + stored_env = cfg.get(CONFIG_ENV_KEY) + if not isinstance(stored_env, dict): + stored_env = {} + shell_env = [] + for _name in provider_env_vars(pid): + live = (_os.environ.get(_name) or "").strip() + if live and live != str(stored_env.get(_name) or "").strip(): + shell_env.append(_name) + + # Only names this provider EXCLUSIVELY owns. Shared ones belong to + # another provider too (nvidia-nim lists DEEPSEEK_API_KEY), and + # deleting them would destroy a credential the user set for + # something else. + owned, shared = exclusive_env_vars(pid) + # …and never a name the LIVE session authenticates through. The + # active-provider guard above only compares slugs, so it misses the + # borrowed-name case: a session running on nvidia-nim via + # DEEPSEEK_API_KEY would lose its credential when the user + # disconnects deepseek, which owns that name outright. Whatever the + # slug, disconnect must not de-authenticate the session you are in. + active = canonical_provider_name(self.provider_name or "") + in_use = set(provider_env_vars(active)) if active else set() + kept_in_use: list[str] = [] + for env_name in owned: + if env_name in in_use: + kept_in_use.append(env_name) + continue + if delete_secret(env_name): + removed = True + # Kept SEPARATE from kept_shared: the two have different remedies. + # A contested name is fixed by disconnecting the co-owner; a name + # the live session resolves through is fixed by switching + # providers first — and folding them together emits the co-owner + # advice for the in-use case, which names the active provider that + # this handler's own guard will refuse to disconnect. + kept_shared = [e for e in shared if (get_secret(e) or "").strip()] + if pid == "anthropic": + from src.auth.anthropic_subscription import remove_credentials + + removed = remove_credentials() or removed + elif pid == "openai": + from src.auth.openai_subscription import remove_credentials + + removed = remove_credentials() or removed + + # ``or bool(shell_env)``: a shell export that delete_secret popped + # out of this process is still in the user's environment and will + # be back next launch, so the provider is NOT disconnected even + # though re-probing now finds nothing. + still = provider_has_credentials(pid, resolve_api_key(pid)) or bool(shell_env) + keyless = not provider_requires_api_key(pid) + except Exception as exc: # noqa: BLE001 + logger.exception("[agent-server] disconnect_provider failed") + self._reply(request_id, {"ok": False, "error": str(exc)}) + return + response: dict = { + "ok": True, + "disconnected": removed and not still, + "still_authenticated": still, + } + if kept_shared: + response["kept_shared_env"] = kept_shared + if kept_in_use: + response["kept_in_use_env"] = kept_in_use + if still: + if keyless: + # A local server (Ollama / vLLM / SGLang) accepts any or no + # token, so it is never "disconnected" in the credential sense. + detail = f"'{pid}' is a local server and needs no credentials" + elif shell_env: + # Checked BEFORE the shared case: when a name is both shared + # and shell-exported, "unset it in your shell" is the half the + # user can act on. + detail = ( + f"'{pid}' still authenticates — {', '.join(shell_env)} is " + "exported in your shell, which only your shell can unset" + ) + elif kept_in_use: + # The remedy here is switching providers, NOT disconnecting + # the co-owner (that IS the active provider, which the guard + # above refuses) and NOT unsetting the name (the live value is + # the config copy this handler just declined to delete). + detail = ( + f"'{pid}' still authenticates via {', '.join(kept_in_use)} — " + f"this session's provider '{active}' resolves through it, so " + "it was left in place; switch to another provider first" + ) + elif kept_shared: + # Name the co-owner and the way out; "another provider also + # uses it" alone leaves the user with no next step. + owners = sorted( + o + for o in PROVIDER_INFO + if o != pid and set(provider_env_vars(o)) & set(kept_shared) + ) + detail = ( + f"'{pid}' still authenticates via {', '.join(kept_shared)}, " + f"also used by {', '.join(owners)} — left in place; disconnect " + f"that too, or unset {kept_shared[0]} yourself" + ) + else: + # Reached when the surviving key lives somewhere this handler + # does not write: a project/local `.clawcodex/config.json`, or + # a subscription login. Naming the environment here would send + # the user hunting through shell rc files for nothing. + detail = ( + f"'{pid}' still authenticates from outside the global " + "config (project config or a subscription login)" + ) + response["error"] = detail + self._reply(request_id, response) + def _install_provider( self, provider: Any, name: str, model: str | None, *, persist_model: str | None = None, diff --git a/tests/server/test_model_provider_picker.py b/tests/server/test_model_provider_picker.py new file mode 100644 index 00000000..e4edce0f --- /dev/null +++ b/tests/server/test_model_provider_picker.py @@ -0,0 +1,603 @@ +"""The /model picker's provider list, inline key entry, and disconnect. + +Regression cover for a picker that could only ever show ONE provider: the TUI +synthesized step 1's list from ``get_settings``, which reports just the +*active* provider, so the 26-entry registry was never surfaced and the list +read ``anthropic · 22 models`` and nothing else. + +These lock the server half — the catalog plus the three controls the picker +drives (``list_model_providers``, ``save_provider_key``, ``disconnect_provider``) +— and the ``provider_mismatch`` signal the client needs to re-drive a +cross-provider selection through ``set_provider``. +""" + +from __future__ import annotations + +import json +import time +from types import SimpleNamespace + +import pytest + +import src.config as config_mod +from src.providers import PROVIDER_INFO +from src.providers.catalog import provider_catalog +from src.server.agent_server import _AgentSession + + +class _StubSession: + """Minimal stand-in exposing only what the three handlers touch. + + The handlers are pure control-plane code — provider name, live model list, + and a reply sink — so driving them directly keeps this focused on the + picker contract instead of the WebSocket harness. + """ + + def __init__(self, provider_name: str = "anthropic", models=None, fusion=None): + self.provider_name = provider_name + self.provider = SimpleNamespace(model="claude-opus-5") + self._models = list(models or []) + self._fusion = fusion + self.replies: list[tuple[object, dict]] = [] + + def _available_models(self) -> list[str]: + return list(self._models) + + def _active_fusion_name(self) -> str | None: + """The picker marks the fusion entry current rather than its base + model, so the control reports this alongside ``model`` exactly as + ``get_settings`` does.""" + return self._fusion + + def _do_set_fusion_model(self, request_id: object, model: object) -> bool: + """Not a fusion model, so _do_set_model falls through to the ordinary + provider-compare path these tests exercise. Selecting a fusion model + is its own control flow with its own coverage.""" + return False + + def _reply(self, request_id: object, payload: dict) -> None: + self.replies.append((request_id, payload)) + + @property + def last(self) -> dict: + return self.replies[-1][1] + + +@pytest.fixture(autouse=True) +def sandbox_config(tmp_path, monkeypatch): + """Move EVERY credential home off the developer's real one. + + Two independent roots are in play and missing either one lets a test reach + into the real ``~/.clawcodex``: + + * ``GLOBAL_CONFIG_FILE`` binds ``Path.home()`` at import and ignores + ``$CLAWCODEX_CONFIG_DIR``, so it has to be patched at the module + constant; + * the subscription OAuth files (``anthropic-oauth.json`` / + ``openai-oauth.json``) resolve ``$CLAWCODEX_CONFIG_DIR`` at call time, so + they need the env var. + + Autouse, and deliberately belt-and-braces: an earlier version patched only + the config module, which left ``remove_credentials()`` pointed at the real + home. The disconnect tests were then safe only because the handler refuses + to disconnect the active provider — so the moment that guard was mutated + for a mutation-test run, the suite deleted a real Claude subscription + login. A test fixture must never depend on the code under test declining + to do the destructive thing. + """ + monkeypatch.setenv("CLAWCODEX_CONFIG_DIR", str(tmp_path)) + cfg = tmp_path / "config.json" + monkeypatch.setattr(config_mod, "GLOBAL_CONFIG_DIR", tmp_path) + monkeypatch.setattr(config_mod, "GLOBAL_CONFIG_FILE", cfg) + config_mod._get_default_manager().invalidate() + yield cfg + config_mod._get_default_manager().invalidate() + + +# ── the catalog ────────────────────────────────────────────────────────────── + + +class TestProviderCatalog: + def test_lists_every_registered_provider_not_just_the_current_one(self): + """The bug: one row. The registry has many, and all of them belong.""" + rows = provider_catalog(current="anthropic") + + assert len(rows) == len(PROVIDER_INFO) + assert len(rows) > 1, "a single-row catalog is the bug this fixes" + slugs = {r["slug"] for r in rows} + # A hand-written class, a spec-registry row, and a local server — + # the three shapes the registry merges. + assert {"anthropic", "openai", "deepseek", "together", "ollama"} <= slugs + + def test_active_provider_sorts_first_and_is_marked(self): + rows = provider_catalog(current="deepseek") + + assert rows[0]["slug"] == "deepseek" + assert rows[0]["is_current"] is True + assert [r["slug"] for r in rows].count("deepseek") == 1 + assert sum(1 for r in rows if r["is_current"]) == 1 + + def test_authenticated_providers_sort_above_unconfigured_ones(self, monkeypatch): + monkeypatch.setattr( + "src.providers.provider_has_credentials", + lambda name, key: name in {"anthropic", "together"}, + ) + rows = provider_catalog(current="anthropic") + + flags = [r["authenticated"] for r in rows] + assert flags == sorted(flags, reverse=True), "authenticated rows must lead" + assert rows[0]["slug"] == "anthropic" + + def test_unconfigured_api_key_provider_carries_key_env_and_warning( + self, monkeypatch + ): + """These two fields are what unlock the picker's inline key stage.""" + monkeypatch.setattr( + "src.providers.provider_has_credentials", lambda name, key: False + ) + row = next(r for r in provider_catalog() if r["slug"] == "together") + + assert row["authenticated"] is False + assert row["auth_type"] == "api_key" + assert row["key_env"] == "TOGETHER_API_KEY" + assert row["warning"] == "paste TOGETHER_API_KEY to activate" + + def test_local_servers_need_no_key(self): + """Ollama/vLLM/SGLang accept any token, so no key stage for them.""" + row = next(r for r in provider_catalog() if r["slug"] == "ollama") + + assert row["auth_type"] == "none" + assert row["authenticated"] is True + + def test_live_model_list_overrides_the_static_one_for_the_active_provider(self): + """Endpoint-discovered catalogues (Ollama et al) must win over the stub.""" + rows = provider_catalog(current="ollama", current_models=["llama4:70b"]) + + active = next(r for r in rows if r["slug"] == "ollama") + other = next(r for r in rows if r["slug"] == "anthropic") + assert active["models"] == ["llama4:70b"] + assert active["total_models"] == 1 + assert other["models"] == PROVIDER_INFO["anthropic"]["available_models"] + + def test_unreadable_config_still_yields_a_full_catalog(self, monkeypatch): + """One bad config block must not blank the picker.""" + monkeypatch.setattr( + "src.config.load_config", + lambda: (_ for _ in ()).throw(RuntimeError("corrupt config")), + ) + rows = provider_catalog(current="anthropic") + + assert len(rows) == len(PROVIDER_INFO) + + def test_active_provider_is_authenticated_even_under_an_alias_spelling( + self, sandbox_config + ): + """A session launched as ``--provider glm`` must not show its own + running provider as needing a key. + + Driven through a REAL config file, not a mocked ``load_config``: the + packaged defaults seed an empty ``providers.zai`` block that wins + ``get_provider_config``'s literal lookup, so a canonical probe cannot + see the user's ``[providers.glm]`` credential. Mocking the config away + is what let an earlier version of this test pass while the picker sent + the user to a key prompt for the provider they were already using. + """ + config_mod.save_config({"providers": {"glm": {"api_key": "sk-zai-real"}}}) + + # Probed canonically, zai looks unconfigured — that is the trap. + assert next(r for r in provider_catalog() if r["slug"] == "zai")[ + "authenticated" + ] is False + + row = next( + r + for r in provider_catalog(current="glm", current_ready=True) + if r["slug"] == "zai" + ) + assert row["is_current"] is True + assert row["authenticated"] is True + assert "warning" not in row + + def test_a_listed_provider_is_never_one_set_provider_would_refuse( + self, sandbox_config + ): + """The catalog must agree with ``_do_set_provider``'s own resolution, + or the picker offers rows that error the moment you pick them.""" + from src.providers import provider_has_credentials, resolve_api_key + + config_mod.save_config({"providers": {"together": {"api_key": "sk-tog"}}}) + for row in provider_catalog(): + if row["is_current"]: + continue + # Exactly what _do_set_provider gates on. + assert row["authenticated"] is provider_has_credentials( + row["slug"], resolve_api_key(row["slug"]) + ), f"{row['slug']} would be offered but refused" + + +# ── list_model_providers ───────────────────────────────────────────────────── + + +class TestListModelProvidersControl: + def test_returns_the_whole_registry_with_the_current_model(self): + sess = _StubSession("anthropic", models=["claude-opus-5"]) + _AgentSession._do_list_model_providers(sess, "r1") + + reply = sess.last + assert reply["ok"] is True + assert reply["provider"] == "anthropic" + assert reply["model"] == "claude-opus-5" + assert len(reply["providers"]) == len(PROVIDER_INFO) > 1 + + def test_reports_the_active_fusion_name_so_the_picker_marks_the_right_row(self): + """On a fused session ``model`` is the BASE id that serves the turn, so + without this channel the picker's `*` would land on the base model row + instead of the fusion entry the user selected.""" + sess = _StubSession("deepseek", models=["deepseek-v4-pro"], fusion="deepseek-v4-pro-V") + _AgentSession._do_list_model_providers(sess, "r1") + + assert sess.last["fusion"] == "deepseek-v4-pro-V" + assert sess.last["model"] == "claude-opus-5" + + def test_catalog_failure_reports_an_error_rather_than_crashing(self, monkeypatch): + monkeypatch.setattr( + "src.providers.catalog.provider_catalog", + lambda **kw: (_ for _ in ()).throw(RuntimeError("boom")), + ) + sess = _StubSession() + _AgentSession._do_list_model_providers(sess, "r1") + + assert sess.last["ok"] is False + assert sess.last["providers"] == [] + + +# ── save_provider_key ──────────────────────────────────────────────────────── + + +class TestSaveProviderKeyControl: + def test_persists_the_key_and_returns_the_refreshed_row(self, sandbox_config): + sess = _StubSession("anthropic") + _AgentSession._do_save_provider_key(sess, "r1", "together", "sk-tog-123") + + reply = sess.last + assert reply["ok"] is True + assert reply["provider"]["slug"] == "together" + assert reply["provider"]["authenticated"] is True + + block = config_mod.load_config()["providers"]["together"] + assert block["api_key"] == "sk-tog-123" + # Seeded so the block is complete, matching what `clawcodex login` writes. + assert block["base_url"] == PROVIDER_INFO["together"]["default_base_url"] + + def test_does_not_clobber_a_custom_base_url(self, sandbox_config): + config_mod.save_config( + {"providers": {"together": {"base_url": "https://my-proxy.internal/v1"}}} + ) + sess = _StubSession("anthropic") + _AgentSession._do_save_provider_key(sess, "r1", "together", "sk-tog-123") + + block = config_mod.load_config()["providers"]["together"] + assert block["base_url"] == "https://my-proxy.internal/v1" + assert block["api_key"] == "sk-tog-123" + + def test_does_not_promote_a_project_scoped_base_url_into_global_config( + self, sandbox_config, monkeypatch + ): + """Reading the MERGED view and writing GLOBAL would launder a + repo-committable ``providers.*.base_url`` into the user's global + config — permanently, for every other project, paired with the key + they just typed. That redirect is exactly what the untrusted-tier + strip exists to contain. + + ``get_provider_config`` is the merged reader; stub it to look like a + project config is contributing an endpoint, and assert none of it + reaches the global file. + """ + monkeypatch.setattr( + "src.config.get_provider_config", + lambda name: {"base_url": "https://gateway.project-scoped.example/v1"}, + ) + sess = _StubSession("anthropic") + _AgentSession._do_save_provider_key(sess, "r1", "together", "sk-tog-123") + + assert sess.last["ok"] is True + on_disk = json.loads(sandbox_config.read_text())["providers"]["together"] + assert on_disk["base_url"] == PROVIDER_INFO["together"]["default_base_url"] + assert "project-scoped" not in on_disk["base_url"] + + @pytest.mark.parametrize( + "slug,key,expected", + [ + ("", "sk-1", "missing provider"), + ("together", " ", "missing api key"), + ("nope-ai", "sk-1", "unknown provider"), + ], + ) + def test_rejects_bad_input(self, sandbox_config, slug, key, expected): + sess = _StubSession("anthropic") + _AgentSession._do_save_provider_key(sess, "r1", slug, key) + + assert sess.last["ok"] is False + assert expected in sess.last["error"] + + def test_alias_slug_writes_the_canonical_block(self, sandbox_config): + sess = _StubSession("anthropic") + _AgentSession._do_save_provider_key(sess, "r1", "glm", "sk-zai-1") + + assert sess.last["ok"] is True + assert config_mod.load_config()["providers"]["zai"]["api_key"] == "sk-zai-1" + + +# ── disconnect_provider ────────────────────────────────────────────────────── + + +class TestDisconnectProviderControl: + def test_refuses_to_disconnect_the_active_provider(self, sandbox_config): + """Pulling the live provider's key would break the session's next turn.""" + sess = _StubSession("anthropic") + _AgentSession._do_disconnect_provider(sess, "r1", "anthropic") + + assert sess.last["ok"] is False + assert sess.last["disconnected"] is False + assert "active provider" in sess.last["error"] + + def test_refuses_via_an_alias_spelling_too(self, sandbox_config): + """`glm` and `zai` are the same provider — the guard must see that.""" + sess = _StubSession("zai") + _AgentSession._do_disconnect_provider(sess, "r1", "glm") + + assert sess.last["ok"] is False + assert "active provider" in sess.last["error"] + + def test_clears_a_stored_key_for_an_inactive_provider( + self, sandbox_config, monkeypatch + ): + monkeypatch.delenv("TOGETHER_API_KEY", raising=False) + config_mod.save_config({"providers": {"together": {"api_key": "sk-tog-123"}}}) + sess = _StubSession("anthropic") + _AgentSession._do_disconnect_provider(sess, "r1", "together") + + reply = sess.last + assert reply["ok"] is True + assert reply["disconnected"] is True + assert reply["still_authenticated"] is False + # Assert on the raw file: load_config() deep-merges the packaged + # defaults, which supply an empty api_key for every provider, so the + # merged view can never show the key as absent. + on_disk = json.loads(sandbox_config.read_text()) + assert "api_key" not in on_disk["providers"]["together"] + from src.providers import resolve_api_key + + assert resolve_api_key("together") == "" + + def test_reports_honestly_when_a_shell_env_key_survives( + self, sandbox_config, monkeypatch + ): + """delete_secret cannot unset a key exported in the user's shell.""" + config_mod.save_config({"providers": {"together": {"api_key": "sk-tog-123"}}}) + monkeypatch.setenv("TOGETHER_API_KEY", "sk-from-shell") + sess = _StubSession("anthropic") + _AgentSession._do_disconnect_provider(sess, "r1", "together") + + reply = sess.last + assert reply["ok"] is True + assert reply["disconnected"] is False + assert reply["still_authenticated"] is True + assert "only your shell can unset" in reply["error"] + + def test_unknown_provider_is_rejected(self, sandbox_config): + sess = _StubSession("anthropic") + _AgentSession._do_disconnect_provider(sess, "r1", "nope-ai") + + assert sess.last["ok"] is False + assert "unknown provider" in sess.last["error"] + + def test_does_not_delete_a_key_another_provider_also_uses( + self, sandbox_config, monkeypatch + ): + """`nvidia-nim` lists DEEPSEEK_API_KEY among its candidates. Treating + that preference list as a delete-set would destroy the DeepSeek + credential — and DeepSeek may be the provider this very session is + running on, routing right around the active-provider guard.""" + from src.providers import provider_env_vars, resolve_api_key + from src.secret_store import set_secret + + assert "DEEPSEEK_API_KEY" in provider_env_vars("nvidia-nim"), ( + "precondition: nvidia-nim borrows the DeepSeek key name" + ) + monkeypatch.delenv("NVIDIA_API_KEY", raising=False) + set_secret("DEEPSEEK_API_KEY", "sk-deepseek-live") + + sess = _StubSession("deepseek") + _AgentSession._do_disconnect_provider(sess, "r1", "nvidia-nim") + + assert resolve_api_key("deepseek") == "sk-deepseek-live" + # And it must not claim a clean disconnect it did not achieve. + assert sess.last["disconnected"] is False + assert sess.last["kept_shared_env"] == ["DEEPSEEK_API_KEY"] + + def test_a_provider_can_still_disconnect_its_own_primary_key( + self, sandbox_config, monkeypatch + ): + """The mirror-image trap of the fix above. DEEPSEEK_API_KEY is + DeepSeek's own primary variable AND nvidia-nim's fallback, so a naive + "is anyone else listing this?" ownership test makes the project's + flagship provider permanently undisconnectable.""" + from src.providers import resolve_api_key + from src.secret_store import set_secret + + monkeypatch.delenv("DEEPSEEK_API_KEY", raising=False) + set_secret("DEEPSEEK_API_KEY", "sk-deepseek-1") + sess = _StubSession("anthropic") + _AgentSession._do_disconnect_provider(sess, "r1", "deepseek") + + assert sess.last["disconnected"] is True + assert resolve_api_key("deepseek") == "" + + def test_a_contested_primary_key_is_reported_not_deleted( + self, sandbox_config, monkeypatch + ): + """siliconflow and siliconflow-cn both lead with SILICONFLOW_API_KEY. + Neither can claim it, so it survives and is reported.""" + from src.providers import resolve_api_key + from src.secret_store import set_secret + + monkeypatch.delenv("SILICONFLOW_API_KEY", raising=False) + set_secret("SILICONFLOW_API_KEY", "sk-sf-1") + sess = _StubSession("anthropic") + _AgentSession._do_disconnect_provider(sess, "r1", "siliconflow") + + assert sess.last["kept_shared_env"] == ["SILICONFLOW_API_KEY"] + assert sess.last["disconnected"] is False + assert resolve_api_key("siliconflow-cn") == "sk-sf-1" + + def test_deletes_a_key_only_this_provider_owns(self, sandbox_config, monkeypatch): + from src.providers import resolve_api_key + from src.secret_store import set_secret + + monkeypatch.delenv("TOGETHER_API_KEY", raising=False) + set_secret("TOGETHER_API_KEY", "sk-tog-1") + sess = _StubSession("anthropic") + _AgentSession._do_disconnect_provider(sess, "r1", "together") + + assert sess.last["disconnected"] is True + assert resolve_api_key("together") == "" + + def test_removes_a_subscription_login(self, sandbox_config, monkeypatch): + """The OAuth-deleting branch — the most destructive lines in the + handler, and previously the only ones no test executed.""" + import src.auth.anthropic_subscription as anthropic_auth + + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + anthropic_auth.save_credentials( + anthropic_auth.SubscriptionCredentials( + access_token="a", refresh_token="r", expires_at=time.time() + 9999 + ) + ) + assert anthropic_auth.load_credentials() is not None + # Sandboxed away from the real home (see sandbox_config). + assert sandbox_config.parent in anthropic_auth.credentials_path().parents + + sess = _StubSession("deepseek") # NOT anthropic, so the guard allows it + _AgentSession._do_disconnect_provider(sess, "r1", "anthropic") + + assert sess.last["ok"] is True + assert sess.last["disconnected"] is True + assert anthropic_auth.load_credentials() is None + + def test_never_deauthenticates_the_session_through_a_borrowed_name( + self, sandbox_config, monkeypatch + ): + """The active-provider guard compares slugs, so it misses this: a + session running on nvidia-nim authenticates through the BORROWED + DEEPSEEK_API_KEY, which deepseek owns outright. Disconnecting deepseek + would pull the live session's own credential — a different slug, the + same breakage the guard exists to prevent.""" + from src.providers import provider_has_credentials, resolve_api_key + from src.secret_store import set_secret + + monkeypatch.delenv("DEEPSEEK_API_KEY", raising=False) + monkeypatch.delenv("NVIDIA_API_KEY", raising=False) + set_secret("DEEPSEEK_API_KEY", "sk-borrowed") + sess = _StubSession("nvidia-nim") + assert provider_has_credentials("nvidia-nim", resolve_api_key("nvidia-nim")) + + _AgentSession._do_disconnect_provider(sess, "r1", "deepseek") + + assert provider_has_credentials("nvidia-nim", resolve_api_key("nvidia-nim")), ( + "the live session lost its credential" + ) + assert sess.last["disconnected"] is False + # Reported in its OWN field, not folded into kept_shared_env: the two + # have different remedies, and the co-owner advice would name the + # active provider — which this handler's guard refuses to disconnect. + assert sess.last["kept_in_use_env"] == ["DEEPSEEK_API_KEY"] + assert "kept_shared_env" not in sess.last + assert "switch to another provider first" in sess.last["error"] + assert "disconnect that too" not in sess.last["error"] + + def test_does_not_advertise_removing_a_key_the_session_needs( + self, sandbox_config + ): + """The confirm screen's `removes_env` is the whole basis for calling + this an informed choice, so it must not promise a removal the handler + will then decline.""" + rows = provider_catalog(current="nvidia-nim", current_ready=True) + deepseek = next(r for r in rows if r["slug"] == "deepseek") + + assert "DEEPSEEK_API_KEY" not in (deepseek.get("removes_env") or []) + # Unrelated providers still disclose their own names. + together = next(r for r in rows if r["slug"] == "together") + assert together["removes_env"] == ["TOGETHER_API_KEY"] + + def test_a_shell_export_behind_a_config_key_is_not_a_clean_disconnect( + self, sandbox_config, monkeypatch + ): + """delete_secret removes the config entry AND pops os.environ, so a + name present in both places looks gone the instant after deleting it — + while the shell export resurrects it on the next launch. Sampling the + environment before the delete loop is what keeps this honest.""" + from src.secret_store import set_secret + + set_secret("TOGETHER_API_KEY", "sk-from-config") + monkeypatch.setenv("TOGETHER_API_KEY", "sk-from-shell") + sess = _StubSession("anthropic") + _AgentSession._do_disconnect_provider(sess, "r1", "together") + + assert sess.last["disconnected"] is False + assert sess.last["still_authenticated"] is True + assert "only your shell can unset" in sess.last["error"] + + def test_reports_a_project_config_survivor_without_blaming_the_shell( + self, sandbox_config, monkeypatch + ): + """`still_authenticated` was honest but its prose sent users hunting + through shell rc files for a key that lives in project config.""" + monkeypatch.delenv("TOGETHER_API_KEY", raising=False) + monkeypatch.setattr( + "src.providers.resolve_api_key", lambda name, cfg=None: "sk-from-project" + ) + sess = _StubSession("anthropic") + _AgentSession._do_disconnect_provider(sess, "r1", "together") + + assert sess.last["still_authenticated"] is True + assert "shell" not in sess.last["error"] + assert "project config" in sess.last["error"] + + +# ── the cross-provider signal ──────────────────────────────────────────────── + + +class TestCrossProviderSignal: + def test_mismatched_provider_refusal_is_machine_readable(self): + """The picker's client re-drives this through ``set_provider``; without + the flag it would have to pattern-match the error prose.""" + sess = _StubSession("anthropic") + _AgentSession._do_set_model(sess, "r1", "gpt-5.4", "openai") + + reply = sess.last + assert reply["ok"] is False + assert reply["provider_mismatch"] is True + assert reply["provider"] == "anthropic" + # The human-readable half stays intact. + assert "openai" in reply["error"] + + def test_an_alias_spelling_is_not_a_cross_provider_switch(self): + """A session launched as ``--provider glm`` keeps that spelling while + the picker's rows carry canonical ``zai``. A raw string compare would + refuse this same-provider switch and then send the client off to + "switch" to the provider it is already on — where set_provider fails + because the credential lives under the alias key. /model would be + dead for all 59 aliases.""" + sess = _StubSession("glm") + _AgentSession._do_set_model(sess, "r1", "GLM-5.2", "zai") + + assert sess.last["ok"] is True + assert sess.last["model"] == "GLM-5.2" + + def test_a_genuine_cross_provider_switch_under_an_alias_still_refuses(self): + sess = _StubSession("glm") + _AgentSession._do_set_model(sess, "r1", "gpt-5.4", "openai") + + assert sess.last["ok"] is False + assert sess.last["provider_mismatch"] is True diff --git a/ui-tui/src/__tests__/fusionModel.test.ts b/ui-tui/src/__tests__/fusionModel.test.ts index 6e415b1d..ef77ace7 100644 --- a/ui-tui/src/__tests__/fusionModel.test.ts +++ b/ui-tui/src/__tests__/fusionModel.test.ts @@ -142,14 +142,32 @@ describe('fusion models', () => { // ── /model interaction ──────────────────────────────────────────────────── + // model.options now asks `list_model_providers` (the picker lists every + // provider, not just the active one). The fusion contract is unchanged: the + // reply carries the base id in `model` and the fusion name separately, and + // the picker's "current" marker must follow the fusion name. + const FUSED_CATALOG = { + ok: true, + provider: 'deepseek', + providers: [ + { + authenticated: true, + is_current: true, + models: ['dsv', 'deepseek-v4-pro'], + name: 'DeepSeek', + slug: 'deepseek', + total_models: 2 + } + ] + } + it('marks the fusion model as current in the /model picker', async () => { const p = gw.request('model.options', {}) - await replyToControl('get_settings', { - available_models: ['dsv', 'deepseek-v4-pro'], + await replyToControl('list_model_providers', { + ...FUSED_CATALOG, fusion: 'dsv', - model: 'deepseek-v4-pro', - provider: 'deepseek' + model: 'deepseek-v4-pro' }) // Must point at the fusion entry the user selected, not the base row. await expect(p).resolves.toMatchObject({ model: 'dsv' }) @@ -158,15 +176,50 @@ describe('fusion models', () => { it('leaves the picker on the plain model when not fused', async () => { const p = gw.request('model.options', {}) - await replyToControl('get_settings', { - available_models: ['deepseek-v4-pro'], + await replyToControl('list_model_providers', { + ...FUSED_CATALOG, fusion: '', - model: 'deepseek-v4-pro', - provider: 'deepseek' + model: 'deepseek-v4-pro' }) await expect(p).resolves.toMatchObject({ model: 'deepseek-v4-pro' }) }) + it('keeps the fusion marker on the get_settings fallback path', async () => { + // An old backend that does not know list_model_providers answers null; + // the synthesized single-provider row must still resolve the fusion name. + vi.useFakeTimers() + + try { + const p = gw.request('model.options', {}) + await vi.waitFor(() => { + seen.push(...stdinFrames()) + expect(seen.find(f => f.request?.subtype === 'list_model_providers')).toBeTruthy() + }) + await vi.advanceTimersByTimeAsync(5_100) + await vi.waitFor(() => { + seen.push(...stdinFrames()) + expect(seen.find(f => f.request?.subtype === 'get_settings')).toBeTruthy() + }) + const req = seen.find(f => f.request?.subtype === 'get_settings')! + proc.line({ + response: { + request_id: req.request_id, + response: { + available_models: ['dsv', 'deepseek-v4-pro'], + fusion: 'dsv', + model: 'deepseek-v4-pro', + provider: 'deepseek' + } + }, + type: 'control_response' + }) + + await expect(p).resolves.toMatchObject({ model: 'dsv' }) + } finally { + vi.useRealTimers() + } + }) + it('reports a refused /model switch instead of claiming success', async () => { // set_model declines for several actionable reasons (a disabled fusion // model, missing credentials for a half, an active turn). Printing diff --git a/ui-tui/src/__tests__/gatewayClient.test.ts b/ui-tui/src/__tests__/gatewayClient.test.ts index 4afa055e..826c02cf 100644 --- a/ui-tui/src/__tests__/gatewayClient.test.ts +++ b/ui-tui/src/__tests__/gatewayClient.test.ts @@ -395,6 +395,232 @@ describe('GatewayClient NDJSON adapter', () => { await expect(p).rejects.toThrow("model 'm' expects provider 'other' but this session is on 'deepseek'") }) + // ── cross-provider selection (the picker's step 1 + step 2) ─────────────── + // `set_model` refuses to point the live provider at a foreign model id, so a + // picker selection from another provider has to go through `set_provider` + // first. A replier that can answer the SECOND set_model frame is required — + // replyToControl always matches the first, which is already resolved. + + const makeReplier = () => { + const answered = new Set() + + return async (subtype: string, response: unknown) => { + let req: any + await vi.waitFor(() => { + seen.push(...stdinFrames()) + req = seen.find( + f => f.type === 'control_request' && f.request?.subtype === subtype && !answered.has(f.request_id) + ) + expect(req).toBeTruthy() + }) + answered.add(req.request_id) + proc.line({ response: { request_id: req.request_id, response }, type: 'control_response' }) + } + } + + it('switches provider then re-applies the model on provider_mismatch', async () => { + const reply = makeReplier() + const p = gw.request('config.set', { key: 'model', value: 'gpt-5.4 --provider openai --tui-session' }) + + await reply('set_model', { + error: "model 'gpt-5.4' expects provider 'openai' but this session is on 'anthropic'", + ok: false, + provider: 'anthropic', + provider_mismatch: true + }) + await reply('set_provider', { model: 'gpt-5.4', ok: true, provider: 'openai' }) + await reply('set_model', { model: 'gpt-5.4', ok: true }) + + await expect(p).resolves.toEqual({ value: 'gpt-5.4' }) + + const switched = seen.find(f => f.request?.subtype === 'set_provider')!.request + expect(switched.provider).toBe('openai') + // Two set_model frames: the probe that got refused, then the retry. + expect(seen.filter(f => f.request?.subtype === 'set_model')).toHaveLength(2) + }) + + it('surfaces a failed provider switch instead of the model error', async () => { + const reply = makeReplier() + const p = gw.request('config.set', { key: 'model', value: 'gpt-5.4 --provider openai' }) + + await reply('set_model', { error: 'wrong provider', ok: false, provider_mismatch: true }) + await reply('set_provider', { error: "provider 'openai' is not configured (no API key)", ok: false }) + + await expect(p).rejects.toThrow("provider 'openai' is not configured (no API key)") + }) + + it('does not retry forever when the switch does not take', async () => { + const reply = makeReplier() + const p = gw.request('config.set', { key: 'model', value: 'gpt-5.4 --provider openai' }) + + await reply('set_model', { error: 'wrong provider', ok: false, provider_mismatch: true }) + await reply('set_provider', { ok: true, provider: 'openai' }) + await reply('set_model', { error: 'still wrong provider', ok: false, provider_mismatch: true }) + + await expect(p).rejects.toThrow('still wrong provider') + expect(seen.filter(f => f.request?.subtype === 'set_provider')).toHaveLength(1) + }) + + // ── model.options / save_key / disconnect ───────────────────────────────── + + it('lists every provider from the catalog control', async () => { + const p = gw.request('model.options', {}) + await replyToControl('list_model_providers', { + model: 'claude-opus-5', + ok: true, + provider: 'anthropic', + providers: [ + { authenticated: true, is_current: true, models: ['claude-opus-5'], name: 'Anthropic Claude', slug: 'anthropic' }, + { authenticated: false, key_env: 'TOGETHER_API_KEY', models: [], name: 'Together AI', slug: 'together' } + ] + }) + + const r: any = await p + expect(r.providers).toHaveLength(2) + expect(r.providers.map((x: any) => x.slug)).toEqual(['anthropic', 'together']) + expect(r.model).toBe('claude-opus-5') + }) + + it('surfaces a catalog refusal instead of inventing a fake provider row', async () => { + // The `init_error` short-circuit answers every control with {ok:false} — + // and it fires exactly when no provider is configured. Falling back to the + // get_settings synthesis there would render one row literally named + // `clawcodex` (the `?? 'clawcodex'` default), reproducing the original + // one-row symptom on top of a provider that does not exist. + const p = gw.request('model.options', {}) + await replyToControl('list_model_providers', { + error: "API key for provider 'anthropic' is not configured. Run `clawcodex login` to set it up.", + ok: false + }) + + await expect(p).rejects.toThrow('Run `clawcodex login`') + expect(seen.find(f => f.request?.subtype === 'get_settings')).toBeUndefined() + }) + + it('falls back to the active provider only when the backend never answers', async () => { + // A null reply means an RPC timeout or a backend too old to know the + // control — the one case where synthesizing from get_settings is right. + vi.useFakeTimers() + + try { + const p = gw.request('model.options', {}) + await vi.waitFor(() => { + seen.push(...stdinFrames()) + expect(seen.find(f => f.request?.subtype === 'list_model_providers')).toBeTruthy() + }) + await vi.advanceTimersByTimeAsync(5_100) // past RPC_TIMEOUT_MS + await vi.waitFor(() => { + seen.push(...stdinFrames()) + expect(seen.find(f => f.request?.subtype === 'get_settings')).toBeTruthy() + }) + const req = seen.find(f => f.request?.subtype === 'get_settings')! + proc.line({ + response: { + request_id: req.request_id, + response: { available_models: ['a', 'b'], model: 'a', provider: 'deepseek' } + }, + type: 'control_response' + }) + + const r: any = await p + expect(r.providers).toHaveLength(1) + expect(r.providers[0]).toMatchObject({ is_current: true, slug: 'deepseek', total_models: 2 }) + } finally { + vi.useRealTimers() + } + }) + + it('rolls back the provider when the model cannot be selected after switching', async () => { + // set_provider has already committed backend-side (registry rebuilt, + // model reset to the new provider's default, pairing persisted), so a + // bare "model switch failed" would read as "nothing happened". + const reply = makeReplier() + const p = gw.request('config.set', { key: 'model', value: 'gpt-5.4 --provider openai' }) + + await reply('set_model', { ok: false, provider: 'anthropic', provider_mismatch: true }) + await reply('set_provider', { ok: true, provider: 'openai' }) + await reply('set_model', { error: 'model switch failed: bad id', ok: false }) + await reply('set_provider', { ok: true, provider: 'anthropic' }) + + await expect(p).rejects.toThrow("rolled back to 'anthropic'") + + const switches = seen.filter(f => f.request?.subtype === 'set_provider') + expect(switches).toHaveLength(2) + expect(switches[1]!.request.provider).toBe('anthropic') + }) + + it('does not roll back when the retry only timed out', async () => { + // A silent backend may well have applied the model; rolling back there + // would discard a switch that actually worked. + vi.useFakeTimers() + try { + const reply = makeReplier() + const p = gw.request('config.set', { key: 'model', value: 'gpt-5.4 --provider openai' }) + // Attach the rejection handler BEFORE advancing timers: the rejection + // fires inside advanceTimersByTimeAsync, and an assertion added after + // that leaves a window Node reports as an unhandled rejection. + const rejects = expect(p).rejects.toThrow('may be on') + + await reply('set_model', { ok: false, provider: 'anthropic', provider_mismatch: true }) + await reply('set_provider', { ok: true, provider: 'openai' }) + await vi.advanceTimersByTimeAsync(5_100) // the retried set_model never answers + + await rejects + expect(seen.filter(f => f.request?.subtype === 'set_provider')).toHaveLength(1) + } finally { + vi.useRealTimers() + } + }) + + it('says the session moved when the rollback itself fails', async () => { + const reply = makeReplier() + const p = gw.request('config.set', { key: 'model', value: 'gpt-5.4 --provider openai' }) + + await reply('set_model', { ok: false, provider: 'anthropic', provider_mismatch: true }) + await reply('set_provider', { ok: true, provider: 'openai' }) + await reply('set_model', { error: 'model switch failed: bad id', ok: false }) + await reply('set_provider', { error: 'anthropic is not configured', ok: false }) + + await expect(p).rejects.toThrow("session is now on 'openai'") + }) + + it('saves an api key through the save_provider_key control', async () => { + const p = gw.request('model.save_key', { api_key: 'sk-tog-1', slug: 'together' }) + await replyToControl('save_provider_key', { + ok: true, + provider: { authenticated: true, models: ['m1'], name: 'Together AI', slug: 'together' } + }) + + const r: any = await p + expect(r.provider).toMatchObject({ authenticated: true, slug: 'together' }) + + const req = seen.find(f => f.request?.subtype === 'save_provider_key')!.request + expect(req).toMatchObject({ api_key: 'sk-tog-1', slug: 'together' }) + }) + + it('surfaces a refused disconnect rather than reporting success', async () => { + const p = gw.request('model.disconnect', { slug: 'anthropic' }) + await replyToControl('disconnect_provider', { + disconnected: false, + error: "'anthropic' is the active provider — switch to another provider before disconnecting it", + ok: false + }) + + await expect(p).rejects.toThrow('is the active provider') + }) + + it('surfaces a partial disconnect whose key survives in the shell', async () => { + const p = gw.request('model.disconnect', { slug: 'together' }) + await replyToControl('disconnect_provider', { + disconnected: false, + error: "'together' still authenticates — its key is set in the process environment, which only your shell can unset", + ok: true, + still_authenticated: true + }) + + await expect(p).rejects.toThrow('only your shell can unset') + }) + it('dispatches an unknown slash as a backend workflow command (send)', async () => { const p = gw.request('slash.exec', { command: 'deep-research what is love' }) await replyToControl('workflow_command', { diff --git a/ui-tui/src/components/modelPicker.tsx b/ui-tui/src/components/modelPicker.tsx index 64c6d6ba..aa6e9f45 100644 --- a/ui-tui/src/components/modelPicker.tsx +++ b/ui-tui/src/components/modelPicker.tsx @@ -224,6 +224,7 @@ export function ModelPicker({ allowPersistGlobal = true, gw, onCancel, onSelect, } setKeySaving(true) + setKeyError('') gw.request<{ disconnected?: boolean }>('model.disconnect', { slug: provider.slug, ...(sessionId ? { session_id: sessionId } : {}) @@ -241,7 +242,7 @@ export function ModelPicker({ allowPersistGlobal = true, gw, onCancel, onSelect, authenticated: false, models: [], total_models: 0, - warning: p.key_env ? `paste ${p.key_env} to activate` : 'run `clawcodex model` to configure' + warning: p.key_env ? `paste ${p.key_env} to activate` : 'run `clawcodex login` to configure' } : p ) @@ -251,9 +252,12 @@ export function ModelPicker({ allowPersistGlobal = true, gw, onCancel, onSelect, setKeySaving(false) setStage('provider') }) - .catch(() => { + .catch((e: unknown) => { + // Refusals are real information — disconnecting the active + // provider, or a key still exported in the shell. Stay put and + // show why instead of dropping back as if it had worked. + setKeyError(rpcErrorMessage(e)) setKeySaving(false) - setStage('provider') }) return @@ -410,7 +414,7 @@ export function ModelPicker({ allowPersistGlobal = true, gw, onCancel, onSelect, - Paste your API key below (saved to ~/.clawcodex/.env) + Paste your API key below — saved to providers.{provider.slug}.api_key @@ -418,7 +422,7 @@ export function ModelPicker({ allowPersistGlobal = true, gw, onCancel, onSelect, - {provider.key_env}: + {provider.name} API key ({provider.key_env}): @@ -463,9 +467,19 @@ export function ModelPicker({ allowPersistGlobal = true, gw, onCancel, onSelect, - This removes saved credentials for {provider.name}. + Clears providers.{provider.slug}.api_key from ~/.clawcodex/config.json + {provider.removes_env?.length ? ( + + …and {provider.removes_env.join(', ')} from its env block + + ) : ( + + {' '} + + )} + You can re-authenticate later by selecting it again. @@ -474,6 +488,16 @@ export function ModelPicker({ allowPersistGlobal = true, gw, onCancel, onSelect, {' '} + {keyError ? ( + + error: {keyError} + + ) : ( + + {' '} + + )} + {keySaving ? ( disconnecting… diff --git a/ui-tui/src/gatewayClient.ts b/ui-tui/src/gatewayClient.ts index 9b8ec9ce..af304941 100644 --- a/ui-tui/src/gatewayClient.ts +++ b/ui-tui/src/gatewayClient.ts @@ -804,29 +804,86 @@ export class GatewayClient extends EventEmitter { return Promise.resolve({ provider_configured: true } as T) // ── runtime ────────────────────────────────────────────────────────── + // The picker's step 1 needs EVERY provider clawcodex knows about, which + // only `list_model_providers` reports. `get_settings` describes just the + // active one, so synthesizing the list from it (as this used to) could + // never show more than a single row. case 'model.options': - return this.controlQuery('get_settings', {}).then((r: any) => { - const models: string[] = Array.isArray(r?.available_models) ? r.available_models : [] - const provider = String(r?.provider ?? 'clawcodex') + return this.controlQuery('list_model_providers', {}).then((r: any) => { + const providers = Array.isArray(r?.providers) ? r.providers : [] + + if (providers.length) { + return { + // On a fused session the backend reports `model` as the base id; + // the picker's "current" marker must point at the fusion entry + // the user actually selected, not at the base model row. + model: typeof r?.fusion === 'string' && r.fusion ? r.fusion : r?.model, + provider: r?.provider, + providers + } as T + } - return { - // On a fused session the backend reports `model` as the base id; - // the picker's "current" marker must point at the fusion entry - // the user actually selected (which `available_models` lists - // first), not at the base model row. - model: (typeof r?.fusion === 'string' && r.fusion) ? r.fusion : r?.model, - provider, - providers: [ - { - authenticated: true, - is_current: true, - models, - name: provider, - slug: provider, - total_models: models.length - } - ] - } as T + // An explicit refusal is real information — most importantly the + // `init_error` short-circuit, which fires exactly when no provider + // is configured. Papering over it with the get_settings synthesis + // would invent a single row named `clawcodex` (that being the + // `?? 'clawcodex'` default when the errored reply carries no + // provider) and reproduce the original one-row symptom. Surface it. + if (r != null) { + throw new Error( + typeof r.error === 'string' && r.error ? r.error : 'could not list providers' + ) + } + + // Only a null reply reaches here: a backend too old to know the + // control, or an RPC timeout. Fall back to the single active + // provider so /model still switches models. + return this.controlQuery('get_settings', {}).then((s: any) => { + const models: string[] = Array.isArray(s?.available_models) ? s.available_models : [] + const provider = String(s?.provider ?? 'clawcodex') + + return { + // Same fusion rule as the catalog path above. + model: typeof s?.fusion === 'string' && s.fusion ? s.fusion : s?.model, + provider, + providers: [ + { + authenticated: true, + is_current: true, + models, + name: provider, + slug: provider, + total_models: models.length + } + ] + } as T + }) + }) + + case 'model.save_key': + return this.controlQuery('save_provider_key', { + api_key: String(p.api_key ?? ''), + slug: String(p.slug ?? '') + }).then((r: any) => { + if (r == null) {throw new Error('save key: no response from backend')} + + if (r.ok === false) {throw new Error(typeof r.error === 'string' && r.error ? r.error : 'failed to save key')} + + return { provider: r.provider } as T + }) + + case 'model.disconnect': + return this.controlQuery('disconnect_provider', { slug: String(p.slug ?? '') }).then((r: any) => { + if (r == null) {throw new Error('disconnect: no response from backend')} + + // A refusal (the active provider) and a partial disconnect (a key + // still exported in the shell) both carry `error`; surface either + // rather than silently returning to the list as if it worked. + if (r.ok === false || (r.disconnected !== true && r.error)) { + throw new Error(typeof r.error === 'string' && r.error ? r.error : 'disconnect failed') + } + + return { disconnected: r.disconnected === true } as T }) case 'prompt.submit': { const text = String(p.text ?? '') @@ -1172,12 +1229,82 @@ export class GatewayClient extends EventEmitter { const model = modelParts.join(' ') + return this.applyModel(model, provider) + } + + // `set_model` deliberately refuses to point the live provider at another + // provider's model id — a cross-provider switch needs the registry rebuild + // only `set_provider` performs. The /model picker selects exactly that way + // (step 1 a provider, step 2 one of its models), so on the backend's + // `provider_mismatch` signal we do the switch first and re-apply the model. + // `allowSwitch` guards the retry against recursing. + private applyModel( + model: string, + provider: string | undefined, + allowSwitch = true + ): Promise<{ value: string; warning?: string }> { return this.controlQuery('set_model', { model, ...(provider ? { provider } : {}) }).then((r: any) => { if (r == null) { - throw new Error('model switch: no response from backend') + // Tagged: a silent backend may still have APPLIED the model, so the + // cross-provider retry below must not "roll back" over it. + throw Object.assign(new Error('model switch: no response from backend'), { + indeterminate: true + }) } if (r.ok === false) { + if (r.provider_mismatch === true && provider && allowSwitch) { + return this.controlQuery('set_provider', { provider }).then((sr: any) => { + if (sr == null) { + throw new Error('provider switch: no response from backend') + } + + if (sr.ok === false) { + throw new Error( + typeof sr.error === 'string' && sr.error ? sr.error : `could not switch to provider '${provider}'` + ) + } + + // The switch has already COMMITTED backend-side: set_provider + // rebuilt the registry, reset the model to the new provider's + // default and persisted the pairing. If selecting the requested + // model now fails, "model switch failed" would read as "nothing + // happened" while the session quietly sits on a different + // provider AND a different model. Roll back to where we came + // from (the mismatch reply names it) and say what actually stuck. + return this.applyModel(model, provider, false).catch((e: unknown) => { + const why = e instanceof Error ? e.message : String(e) + const previous = typeof r.provider === 'string' ? r.provider : '' + + // A silent backend is NOT a known failure — it may have applied + // the model. Rolling back would then throw away a switch that + // worked, so report the uncertainty instead of acting on it. + if ((e as { indeterminate?: boolean })?.indeterminate) { + throw new Error( + `switched to '${provider}' but the model selection got no response — ` + + `the session may be on '${provider}'` + ) + } + + if (!previous) { + throw new Error(`switched to '${provider}' but could not select '${model}': ${why}`) + } + + return this.controlQuery('set_provider', { provider: previous }).then((back: any) => { + throw new Error( + back != null && back.ok !== false + ? // set_provider resets the model to that provider's + // configured default and persists it, so the provider is + // restored but the previously-selected model is not. + `could not select '${model}' on '${provider}': ${why} — rolled back to ` + + `'${previous}' (its default model)` + : `could not select '${model}': ${why} — session is now on '${provider}'` + ) + }) + }) + }) + } + throw new Error(typeof r.error === 'string' && r.error ? r.error : 'model switch failed') } diff --git a/ui-tui/src/gatewayTypes.ts b/ui-tui/src/gatewayTypes.ts index 36dfef08..e8f6bf68 100644 --- a/ui-tui/src/gatewayTypes.ts +++ b/ui-tui/src/gatewayTypes.ts @@ -561,6 +561,8 @@ export interface ModelOptionProvider { key_env?: string models?: string[] name: string + /** Env vars ^d would clear from the config — shown on the confirm screen. */ + removes_env?: string[] slug: string total_models?: number warning?: string