diff --git a/tests/test_model_registry.py b/tests/test_model_registry.py index fba575ad..73c83ddf 100644 --- a/tests/test_model_registry.py +++ b/tests/test_model_registry.py @@ -331,7 +331,11 @@ class TestLoadModelRegistry: api_key="dummy", model="local-model", ) - assert reg.count == 2 # "openai" + "default" + # The CLI ``"default"`` shim is suppressed once ``[models.*]`` + # populates configs — only the explicit alias survives. + assert reg.count == 1 + assert reg.has_alias("openai") + assert not reg.has_alias("default") assert reg.default == "openai" _, model, _ = reg.resolve() assert model == "gpt-4o" @@ -562,7 +566,12 @@ class TestLoadModelRegistryWithDB: assert cfg.source == "config" def test_db_only_models_coexist(self) -> None: - """DB models coexist alongside config.toml models.""" + """DB models coexist alongside config.toml models. + + The CLI ``"default"`` shim is suppressed when DB / config models + already populate the registry — see + ``test_cli_default_shim_skipped_when_db_models_present``. + """ storage = _MockStorage( [ { @@ -586,7 +595,7 @@ class TestLoadModelRegistryWithDB: reg = load_model_registry("http://x/v1", "x", "x", storage=storage) assert reg.has_alias("db-only") assert reg.has_alias("config-only") - assert reg.has_alias("default") + assert not reg.has_alias("default") assert reg.get_config("db-only").source == "db" assert reg.get_config("config-only").source == "config" @@ -606,10 +615,12 @@ class TestLoadModelRegistryWithDB: } ] ) + # The CLI default shim is suppressed when the DB row populates + # configs, so only the DB-sourced alias exists here. with patch("turnstone.core.model_registry.load_config", return_value={}): reg = load_model_registry("http://x/v1", "x", "x", storage=storage) assert reg.get_config("from-db").source == "db" - assert reg.get_config("default").source == "" + assert not reg.has_alias("default") def test_disabled_db_models_excluded(self) -> None: """Disabled DB models are not loaded.""" @@ -1675,6 +1686,58 @@ class TestLoadModelRegistryDBOnly: reg = load_model_registry(model="", storage=storage) assert not reg.has_alias("default") + def test_cli_default_shim_skipped_when_db_models_present(self) -> None: + """An auto-detected ``--model`` does NOT synthesise a ``default`` + alias when the DB already contributes models. + + Regression for the silent bypass of ``model.task_alias`` / + ``model.plan_alias``: a synthesised ``default`` aliased to whatever + ``--base-url`` was at boot leaks into the LLM-visible alias list, + and the LLM picks it for ``task_agent(model="default")`` — which + then routes around the operator-configured per-role default. + """ + storage = _MockStorage( + [ + { + "alias": "gh200", + "model": "deepseek-ai/DeepSeek-V4-Flash", + "provider": "openai", + "base_url": "http://gh200:8000/v1", + "api_key": "sk-gh200", + "context_window": 1048576, + "capabilities": "{}", + "enabled": True, + } + ] + ) + with patch("turnstone.core.model_registry.load_config", return_value={}): + reg = load_model_registry( + base_url="http://flatspark:8000/v1", + api_key="sk-flatspark", + model="qwen3.6-35B-A3B", # populated by ``detect_model`` + storage=storage, + ) + assert reg.has_alias("gh200") + assert not reg.has_alias("default") + + def test_cli_default_shim_skipped_when_config_models_present(self) -> None: + """Same shim suppression when only ``[models.*]`` populates configs.""" + fake_cfg: dict[str, Any] = { + "models": {"local": {"model": "qwen3-32b"}}, + } + with patch("turnstone.core.model_registry.load_config", return_value=fake_cfg): + reg = load_model_registry("http://x/v1", "x", "fallback-model") + assert reg.has_alias("local") + assert not reg.has_alias("default") + + def test_cli_default_shim_still_fires_when_registry_empty(self) -> None: + """Single-model CLI mode (no DB, no config.toml [models.*]) keeps + the back-compat ``default`` alias.""" + with patch("turnstone.core.model_registry.load_config", return_value={}): + reg = load_model_registry("http://x/v1", "x", "lone-model") + assert reg.has_alias("default") + assert reg.get_config("default").model == "lone-model" + # --------------------------------------------------------------------------- # server._effective_routing / _apply_routing_overrides diff --git a/tests/test_session.py b/tests/test_session.py index 9819ecc2..0afa8789 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -504,9 +504,51 @@ class TestAgentModelOverride: assert item.get("needs_approval") is False assert "error" in item assert "unknown model alias 'bogus'" in item["error"] - # The error guidance must list the available aliases so the LLM can retry. - for alias in ("default", "smart", "fast"): + # Error guidance lists the aliases the LLM may retry, intentionally + # excluding ``default`` — that alias is operator-only (see + # ``test_prepare_plan_default_model_rejected``). Surfacing it here + # would re-enable the per-role-override bypass even though the + # tool description hides it. + for alias in ("smart", "fast"): assert alias in item["error"] + assert "default" not in item["error"] + + def test_prepare_plan_default_model_rejected(self, tmp_db) -> None: + """``model="default"`` is rejected even when the alias exists in + the registry — bypasses the operator-configured ``plan_alias``.""" + session = _make_session(registry=self._registry(), model_alias="default") + item = session._prepare_plan("c1", {"goal": "do x", "model": "default"}) + assert item.get("needs_approval") is False + assert "error" in item + assert "'default' is not a selectable model alias" in item["error"] + assert "Omit `model=`" in item["error"] + + def test_prepare_plan_default_model_rejected_with_whitespace(self, tmp_db) -> None: + """The ``default`` rejection runs after ``strip()`` so leading/ + trailing whitespace can't sneak the alias past the carve-out.""" + session = _make_session(registry=self._registry(), model_alias="default") + item = session._prepare_plan("c1", {"goal": "do x", "model": " default "}) + assert item.get("needs_approval") is False + assert "'default' is not a selectable model alias" in item["error"] + + def test_prepare_plan_unknown_model_with_only_default_in_registry(self, tmp_db) -> None: + """When the registry holds only the reserved ``default`` alias + (single-CLI-model back-compat), the unknown-alias error must say + '(no alternative aliases configured — omit `model=`)' — not the + misleading '(no registry configured)' that suggests routing isn't + wired up at all.""" + from turnstone.core.model_registry import ModelConfig, ModelRegistry + + reg = ModelRegistry( + models={"default": ModelConfig("default", "x", "x", "m")}, + default="default", + ) + session = _make_session(registry=reg, model_alias="default") + item = session._prepare_plan("c1", {"goal": "do x", "model": "bogus"}) + assert item.get("needs_approval") is False + assert "unknown model alias 'bogus'" in item["error"] + assert "no alternative aliases configured" in item["error"] + assert "no registry configured" not in item["error"] # ---- _prepare_task ---- @@ -526,6 +568,15 @@ class TestAgentModelOverride: assert item.get("needs_approval") is False assert "error" in item assert "unknown model alias 'bogus'" in item["error"] + assert "default" not in item["error"] + + def test_prepare_task_default_model_rejected(self, tmp_db) -> None: + """Symmetric carve-out for task_agent — see + ``test_prepare_plan_default_model_rejected``.""" + session = _make_session(registry=self._registry(), model_alias="default") + item = session._prepare_task("c1", {"prompt": "do x", "model": "default"}) + assert item.get("needs_approval") is False + assert "'default' is not a selectable model alias" in item["error"] # ---- tool description rendering ---- @@ -544,8 +595,11 @@ class TestAgentModelOverride: tool = self._agent_tool(session, name) assert tool is not None, f"{name} missing from session tools" desc = tool["function"]["parameters"]["properties"]["model"]["description"] - for alias in ("default", "smart", "fast"): + for alias in ("smart", "fast"): assert f"`{alias}`" in desc, f"alias {alias} missing from {desc!r}" + # ``default`` is intentionally hidden — see + # ``test_render_omits_default_alias_from_description``. + assert "`default`" not in desc def test_render_no_op_without_registry(self, tmp_db) -> None: """No registry → leave the placeholder description untouched.""" @@ -576,6 +630,82 @@ class TestAgentModelOverride: desc = plan_tool["function"]["parameters"]["properties"]["model"]["description"] assert "`bigboi`" in desc + def test_render_omits_default_alias_from_description(self, tmp_db) -> None: + """The ``default`` alias is filtered from the LLM-facing alias list. + + Reading "default" as English ("use the default") and passing it + explicitly bypasses the operator-configured per-role plan_alias / + task_alias. The LLM should reach the per-role default by omitting + ``model=`` instead. + """ + from turnstone.core.model_registry import ModelConfig, ModelRegistry + + reg = ModelRegistry( + models={ + "default": ModelConfig("default", "x", "x", "m"), + "gh200": ModelConfig("gh200", "x", "x", "m"), + "opus-4.7": ModelConfig("opus-4.7", "x", "x", "m"), + }, + default="default", + ) + session = _make_session(registry=reg, model_alias="default") + for name in ("plan_agent", "task_agent"): + tool = self._agent_tool(session, name) + assert tool is not None + desc = tool["function"]["parameters"]["properties"]["model"]["description"] + assert "`gh200`" in desc + assert "`opus-4.7`" in desc + assert "`default`" not in desc + + def test_render_falls_back_to_base_when_only_default_alias(self, tmp_db) -> None: + """Single-CLI-model registries (only ``default`` in registry) leave + the base description untouched — the LLM sees ``"No alternative + aliases configured"`` rather than an empty alias list.""" + from turnstone.core.model_registry import ModelConfig, ModelRegistry + + reg = ModelRegistry( + models={"default": ModelConfig("default", "x", "x", "m")}, + default="default", + ) + session = _make_session(registry=reg, model_alias="default") + plan_tool = self._agent_tool(session, "plan_agent") + assert plan_tool is not None + desc = plan_tool["function"]["parameters"]["properties"]["model"]["description"] + assert "No alternative aliases configured" in desc + + def test_refresh_into_only_default_resets_to_base(self, tmp_db) -> None: + """A reload that drops the registry to only ``default`` must clear + stale alias names from the previously-rendered tool descriptions — + not return early and leave them in place.""" + from turnstone.core.model_registry import ModelConfig, ModelRegistry + + reg = ModelRegistry( + models={ + "default": ModelConfig("default", "x", "x", "m"), + "smart": ModelConfig("smart", "x", "x", "m"), + "fast": ModelConfig("fast", "x", "x", "m"), + }, + default="default", + ) + session = _make_session(registry=reg, model_alias="default") + # Sanity: initial render carries the non-default aliases. + plan_tool = self._agent_tool(session, "plan_agent") + assert plan_tool is not None + desc = plan_tool["function"]["parameters"]["properties"]["model"]["description"] + assert "`smart`" in desc and "`fast`" in desc + + # Reload the registry down to only ``default`` (admin removed + # every other model definition). + reg.reload({"default": ModelConfig("default", "x", "x", "m")}, "default") + session.refresh_agent_tool_schemas() + + plan_tool = self._agent_tool(session, "plan_agent") + assert plan_tool is not None + desc = plan_tool["function"]["parameters"]["properties"]["model"]["description"] + assert "`smart`" not in desc, f"stale alias survived reload: {desc!r}" + assert "`fast`" not in desc, f"stale alias survived reload: {desc!r}" + assert "No alternative aliases configured" in desc + def test_module_level_constants_not_mutated(self, tmp_db) -> None: """Rendering must not pollute the module-level TOOLS list shared across all sessions.""" diff --git a/turnstone/core/model_registry.py b/turnstone/core/model_registry.py index bf6f9308..cac9096e 100644 --- a/turnstone/core/model_registry.py +++ b/turnstone/core/model_registry.py @@ -500,10 +500,14 @@ def load_model_registry( server_compat=entry_server_compat, ) - # 3. Ensure a "default" entry from CLI args (only if not already defined - # by config.toml or DB — those take precedence, and only when a CLI - # model was actually provided) - if "default" not in configs and model: + # 3. Back-compat shim: synthesize a "default" alias from CLI/auto-detected + # ``--base-url`` + ``--model`` only when no DB or config.toml models exist. + # Auto-creating "default" alongside DB models leaks a non-routing alias + # into the public list — the LLM picks it in plan_agent / task_agent + # ``model=`` and silently bypasses the operator's per-role + # plan_alias / task_alias overrides (the "default" alias points at + # whatever LLM_BASE_URL was at boot, not at the configured default). + if not configs and model: configs["default"] = ModelConfig( alias="default", base_url=base_url, diff --git a/turnstone/core/session.py b/turnstone/core/session.py index d06b70fe..bc4e2574 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -1516,9 +1516,13 @@ class ChatSession: """ if self._registry is None: return - aliases = sorted(self._registry.list_aliases()) - if not aliases: - return + # Hide ``default`` from the alias list — the LLM reads the English + # word and picks it explicitly, which routes to whichever model + # carries that alias rather than the operator-configured per-role + # default (plan_alias / task_alias). Omitting ``model=`` already + # selects the per-role default; offering the literal name as an + # alternative invites the bypass. + aliases = sorted(a for a in self._registry.list_aliases() if a != "default") aliases_str = ", ".join(f"`{a}`" for a in aliases) new_tools: list[dict[str, Any]] = [] @@ -1532,11 +1536,22 @@ class ChatSession: new_tool = copy.deepcopy(tool) props = new_tool.get("function", {}).get("parameters", {}).get("properties", {}) if "model" in props: - props["model"]["description"] = ( - f"Optional model alias to run this {name} on. " - f"Omit to use the operator-configured {kind}. " - f"Available aliases: {aliases_str}." - ) + # Always rewrite — a reload that filters down to no + # alternatives (only ``default`` remains in the registry) + # must clear any stale alias names left over from a prior + # render, not return early and leave them in place. + if aliases: + props["model"]["description"] = ( + f"Optional model alias to run this {name} on. " + f"Omit to use the operator-configured {kind}. " + f"Available aliases: {aliases_str}." + ) + else: + props["model"]["description"] = ( + f"Optional model alias to run this {name} on. " + "Omit to use the current session model. " + "(No alternative aliases configured in this session.)" + ) new_tools.append(new_tool) self._tools = new_tools @@ -6151,9 +6166,39 @@ class ChatSession: alias = str(raw).strip() if not alias: return None, None + # ``default`` is operator-only — the alias either back-compat-shims + # a single-CLI-model registry or aliases a hand-named DB row, and + # in both cases an LLM that explicitly routes here bypasses the + # operator-configured ``plan_alias`` / ``task_alias`` per-role + # default. Symmetric with the description filter at + # ``_render_agent_tool_descriptions`` — closes the loophole where + # an LLM that learned the alias name out-of-band (training data, + # prior turn, prompt injection) can re-issue it directly. + if alias == "default": + return None, { + "call_id": call_id, + "func_name": func_name, + "header": f"\u2717 {func_name}: 'default' is not selectable", + "preview": "", + "needs_approval": False, + "error": ( + "Error: 'default' is not a selectable model alias for " + f"{func_name}. Omit `model=` to use the operator-configured " + "per-role default." + ), + } if self._registry is None or not self._registry.has_alias(alias): - available = sorted(self._registry.list_aliases()) if self._registry is not None else [] - available_str = ", ".join(available) if available else "(no registry configured)" + # ``default`` excluded from the retry list so an LLM probing + # with a bogus alias can't enumerate it back from the error. + if self._registry is None: + available_str = "(no registry configured)" + else: + available = sorted(a for a in self._registry.list_aliases() if a != "default") + available_str = ( + ", ".join(available) + if available + else "(no alternative aliases configured — omit `model=`)" + ) return None, { "call_id": call_id, "func_name": func_name,