mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(session): prevent LLM bypass of per-role plan/task model overrides
The LLM was passing ``task_agent(model="default")`` (and the same for plan_agent) and routing to whichever backend the auto-created ``default`` alias was attached to at boot — flatspark in the verified case (ws_id 7dde674) — silently bypassing the operator-configured ``model.task_alias`` / ``model.plan_alias`` (gh200). Root fix: - ``load_model_registry`` only synthesises the back-compat ``default`` alias when neither DB nor ``[models.*]`` populate the registry. The shim was only ever meant for single-CLI-model setups; with a multi- model DB it became a phantom routing target aliasing ``LLM_BASE_URL``. - ``_render_agent_tool_descriptions`` filters ``default`` out of the LLM-visible alias list. The English reading of "default" trips the model into picking it explicitly even when the description tells it to omit ``model=`` for the per-role default. Defense-in-depth at the validator chokepoint (``_validate_agent_model_override``): explicit rejection of ``alias == "default"`` (post-strip) with corrective guidance; ``default`` filtered out of the unknown-alias retry list so an LLM probing with a bogus alias can't enumerate it back; the no-alternatives wording is distinguished from the no-registry-configured wording. The render path also always rewrites tool descriptions instead of returning early on filter-empty, so a reload that drops the registry to only ``default`` clears stale alias names left over from a prior render.
This commit is contained in:
@@ -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
|
||||
|
||||
+133
-3
@@ -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."""
|
||||
|
||||
@@ -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,
|
||||
|
||||
+55
-10
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user