From 2238051132f34e0b4c2953ba51966ea29a51bae0 Mon Sep 17 00:00:00 2001 From: arelchan <204152633+arelchan@users.noreply.github.com> Date: Fri, 7 Aug 2026 22:24:41 +0800 Subject: [PATCH 1/8] fix(*): carry a live model switch to every provider holder config.set key="model" built a fresh provider and then assigned loop.provider and loop.model. The loop is not the only holder: AgentLoop hands the provider it was built with to the subagent manager, to the context engine's LLM-backed segments (skill rewriter, skill gate, curator and its history trimmer) and to the memory consolidator, and each keeps its own reference. A switch that stopped at the loop left all of them calling the provider built at process start for the rest of the run. What that looks like in practice: switching away from an unusable credential fixes the main loop, while subagent spawns and the skill rewriter/gate keep failing to authenticate against the abandoned endpoint. The auth error is classified non-retryable, so each one fails on the first attempt and is swallowed by its caller's fallback, which is why this stayed invisible apart from a warning line. AgentLoop.set_provider now fans the new provider out to every holder, and the RPC handler calls it instead of assigning the two attributes. The context engine walks its builders and forwards to the ones implementing set_provider, so a purely textual segment needs no override. A pinned gate model and an explicit config.curator_model survive the switch; both follow the agent's model only when they were already following it. In-flight turns and subagents keep the provider they started with, so no single conversation spans two endpoints. Co-authored-by: Claude (claude-opus-5) --- raven/agent/loop/main.py | 26 +++++ raven/agent/subagent/manager.py | 12 +++ raven/context_engine/assembler.py | 10 ++ raven/context_engine/base.py | 10 ++ raven/context_engine/curator.py | 6 ++ raven/context_engine/history_trimmer.py | 6 ++ raven/context_engine/segments/curator.py | 12 +++ raven/context_engine/segments/skills.py | 9 ++ .../memory_engine/consolidate/consolidator.py | 5 + raven/memory_engine/skill_forge/gate.py | 10 ++ raven/memory_engine/skill_forge/rewriter.py | 11 +++ raven/tui_rpc/methods/config.py | 6 +- tests/test_agent_loop_model_switch.py | 97 +++++++++++++++++++ tests/test_tui_rpc_config.py | 28 +++++- 14 files changed, 244 insertions(+), 4 deletions(-) create mode 100644 tests/test_agent_loop_model_switch.py diff --git a/raven/agent/loop/main.py b/raven/agent/loop/main.py index 1ddadc64..4eb92543 100644 --- a/raven/agent/loop/main.py +++ b/raven/agent/loop/main.py @@ -553,6 +553,10 @@ def __init__( self._consolidation_tasks: set[asyncio.Task] = set() + # Every subsystem below was handed ``provider`` above and holds its + # own reference; ``set_provider`` is what keeps them from outliving + # a live model switch. Add the call there when adding another. + # Phase B-3: the L4 facade (``DefaultMemoryEngine`` / # ``MemoryEngine`` ABC) has been retired. AgentLoop now holds # the underlying subsystems directly: @@ -607,6 +611,28 @@ def _apply_disabled_tools(self) -> None: if self.tools.has(name): self.tools.unregister(name) + def set_provider(self, provider: LLMProvider, model: str) -> None: + """Point the loop and everything it built at a new provider/model. + + ``config.set model`` builds a provider from the prospective config + and hands it here. Assigning ``self.provider`` alone is not enough: + the subagent manager, the context engine's LLM-backed segments and + the consolidator each captured the provider handed to them in + ``__init__``. Left behind, they keep calling the old endpoint for + the rest of the process -- which is how switching away from a dead + credential fixed the main loop while subagents and the skill + rewriter/gate went on failing to authenticate. + + In-flight work is not migrated: a turn or subagent already running + finishes on the provider it started with, so one conversation never + spans two endpoints. + """ + self.provider = provider + self.model = model + self.subagents.set_provider(provider, model) + self.context_engine.set_provider(provider, model) + self.memory_consolidator.set_provider(provider, model) + def configure_personalization(self, enable: bool) -> None: """Global switch for the 4-step personalization flow (PAHF-inspired). diff --git a/raven/agent/subagent/manager.py b/raven/agent/subagent/manager.py index 1d17e90e..1e32a739 100644 --- a/raven/agent/subagent/manager.py +++ b/raven/agent/subagent/manager.py @@ -75,6 +75,18 @@ def __init__( # pruned to the rolling window on access, so it self-bounds. self._session_spawn_times: dict[str, deque[float]] = {} + def set_provider(self, provider: LLMProvider, model: str) -> None: + """Adopt the provider a live ``/model`` switch just built. + + Subagents run on the parent's provider, so a switch that is not + propagated here leaves every spawn calling the credential the loop + has already abandoned. Tasks already in flight keep the provider + they started with -- swapping mid-turn would split one subagent's + conversation across two endpoints. + """ + self.provider = provider + self.model = model + async def spawn( self, task: str, diff --git a/raven/context_engine/assembler.py b/raven/context_engine/assembler.py index a2c990dd..debf165d 100644 --- a/raven/context_engine/assembler.py +++ b/raven/context_engine/assembler.py @@ -37,6 +37,7 @@ if TYPE_CHECKING: from raven.context_engine.curator import TurnContext + from raven.providers.base import LLMProvider class ContextAssembler(ContextEngine): @@ -64,6 +65,15 @@ def owns_compaction(self) -> bool: # the full append-only log and skips the host MemoryConsolidator. return True + def set_provider(self, provider: "LLMProvider", model: str) -> None: + # Duck-typed on purpose: only the builders that actually call an LLM + # implement it, and putting it on the SegmentBuilder protocol would + # force an empty override onto every purely textual builder. + for builder in self._builders: + setter = getattr(builder, "set_provider", None) + if callable(setter): + setter(provider, model) + async def assemble( self, session_key: str, diff --git a/raven/context_engine/base.py b/raven/context_engine/base.py index 398387ff..77b15356 100644 --- a/raven/context_engine/base.py +++ b/raven/context_engine/base.py @@ -35,6 +35,7 @@ # for ``ContextEngine``, so referencing ``TurnContext`` only in type # hints keeps the loop unbroken. from raven.context_engine.curator import TurnContext + from raven.providers.base import LLMProvider # --------------------------------------------------------------------------- @@ -142,6 +143,15 @@ def owns_compaction(self) -> bool: lets the engine manage history compaction itself (Curator archives messages out-of-band).""" + def set_provider(self, provider: "LLMProvider", model: str) -> None: + """Adopt the provider a live ``/model`` switch just built. + + Segments that call an LLM hold the provider handed to them at + construction; without this they keep calling the old one for the + rest of the process. Concrete rather than abstract so an engine + with no LLM-backed segment needs no override. + """ + @abstractmethod async def assemble( self, diff --git a/raven/context_engine/curator.py b/raven/context_engine/curator.py index 9864b213..732ee683 100644 --- a/raven/context_engine/curator.py +++ b/raven/context_engine/curator.py @@ -352,6 +352,12 @@ def __init__( # CuratorSegmentBuilder before any build/validate call. self.prefix: "AssembledPrefix | None" = None + def set_provider(self, provider: LLMProvider, model: str) -> None: + """Adopt the provider a live ``/model`` switch just built.""" + self.provider = provider + self.model = model + self.trimmer.set_provider(provider, model) + @staticmethod def working_state_segment(working_state: str | None) -> str: """Render segment 6 text (``# Curator Working State``) or ``""``.""" diff --git a/raven/context_engine/history_trimmer.py b/raven/context_engine/history_trimmer.py index d40d08ba..b4a62cac 100644 --- a/raven/context_engine/history_trimmer.py +++ b/raven/context_engine/history_trimmer.py @@ -80,6 +80,12 @@ def __init__( self.get_tool_definitions = get_tool_definitions self.context_window_tokens = context_window_tokens + def set_provider(self, provider: LLMProvider, model: str) -> None: + """Adopt the provider a live ``/model`` switch just built, so token + estimates keep matching the model actually being called.""" + self.provider = provider + self.model = model + # ------------------------------------------------------------------ # Pure history-shaping helpers (no token estimation / no I/O) # ------------------------------------------------------------------ diff --git a/raven/context_engine/segments/curator.py b/raven/context_engine/segments/curator.py index c443021b..28779a2b 100644 --- a/raven/context_engine/segments/curator.py +++ b/raven/context_engine/segments/curator.py @@ -82,6 +82,18 @@ def __init__( ) self._turn_ids: dict[str, str] = {} + def set_provider(self, provider: LLMProvider, model: str) -> None: + """Adopt the provider a live ``/model`` switch just built. + + ``curator_model`` only follows when it was following the agent's + model already -- an explicit ``config.curator_model`` is a pin. + """ + self.provider = provider + self.model = model + if not self.config.curator_model: + self.curator_model = model + self.assembler.set_provider(provider, model) + async def build(self, ctx: AssemblyContext) -> Segment | None: if ctx.prefix is None: raise RuntimeError("CuratorSegmentBuilder requires ctx.prefix (phase B)") diff --git a/raven/context_engine/segments/skills.py b/raven/context_engine/segments/skills.py index 948a8465..dfd53b7a 100644 --- a/raven/context_engine/segments/skills.py +++ b/raven/context_engine/segments/skills.py @@ -41,6 +41,7 @@ from raven.memory_engine.skill_forge.gate import LLMGateFilter from raven.memory_engine.skill_forge.rewriter import QueryRewriter from raven.memory_engine.skill_forge.types import RouterHit + from raven.providers.base import LLMProvider from raven.skill_hub import SkillHubClient log = logging.getLogger(__name__) @@ -73,6 +74,14 @@ def __init__( self._hub_client = hub_client self._get_tool_definitions = get_tool_definitions + def set_provider(self, provider: "LLMProvider", model: str) -> None: + """Hand a live ``/model`` switch down to the two LLM users in this + segment. The router itself holds no provider.""" + if self._rewriter is not None: + self._rewriter.set_provider(provider, model) + if self._gate is not None: + self._gate.set_provider(provider, model) + @trace.instrument("skill.inject", kind="skill", detached=True, extract=semconv.skill_inject_skills) async def build(self, ctx: AssemblyContext) -> Segment | None: if self._router is None: diff --git a/raven/memory_engine/consolidate/consolidator.py b/raven/memory_engine/consolidate/consolidator.py index 853ce093..1a9daa32 100644 --- a/raven/memory_engine/consolidate/consolidator.py +++ b/raven/memory_engine/consolidate/consolidator.py @@ -1723,6 +1723,11 @@ def __init__( self.enable_foresight = enable_foresight self._locks: weakref.WeakValueDictionary[str, asyncio.Lock] = weakref.WeakValueDictionary() + def set_provider(self, provider: "LLMProvider", model: str) -> None: + """Adopt the provider a live ``/model`` switch just built.""" + self.provider = provider + self.model = model + def get_lock(self, session_key: str) -> asyncio.Lock: """Return the shared consolidation lock for one session.""" return self._locks.setdefault(session_key, asyncio.Lock()) diff --git a/raven/memory_engine/skill_forge/gate.py b/raven/memory_engine/skill_forge/gate.py index d9fbe33d..7dcde286 100644 --- a/raven/memory_engine/skill_forge/gate.py +++ b/raven/memory_engine/skill_forge/gate.py @@ -61,6 +61,16 @@ def __init__( self._temperature = temperature self._max_tokens = max_tokens + def set_provider(self, provider: "LLMProvider", model: str) -> None: + """Adopt the provider a live ``/model`` switch just built. + + ``_model`` is left alone either way: unset means the gate already + follows whatever the new provider defaults to, and set means a + deliberate pin that a switch elsewhere must not undo. + """ + del model + self._provider = provider + @trace.instrument("skill.gate", kind="skill", extract=semconv.skill_gate) async def filter( self, diff --git a/raven/memory_engine/skill_forge/rewriter.py b/raven/memory_engine/skill_forge/rewriter.py index 7009e03b..5c3dd3b6 100644 --- a/raven/memory_engine/skill_forge/rewriter.py +++ b/raven/memory_engine/skill_forge/rewriter.py @@ -74,6 +74,17 @@ def __init__( self._max_tokens = max_tokens self._temperature = temperature + def set_provider(self, provider: "LLMProvider", model: str) -> None: + """Adopt the provider a live ``/model`` switch just built. + + Held rather than looked up per call, so without this the rewriter + keeps calling the provider captured at construction after the loop + has moved on -- a switch away from an unusable credential fixes the + main path and leaves this one failing. + """ + del model # the rewriter runs on the provider's default model + self._provider = provider + @trace.instrument("skill.rewrite", kind="skill", extract=semconv.skill_rewrite) async def analyze(self, query: str) -> RewriteResult: truncated = (query or "").strip()[:_QUERY_MAX_LENGTH] diff --git a/raven/tui_rpc/methods/config.py b/raven/tui_rpc/methods/config.py index 91239a26..15567c17 100644 --- a/raven/tui_rpc/methods/config.py +++ b/raven/tui_rpc/methods/config.py @@ -389,8 +389,10 @@ def _set_model( _save_config(payload) if loop is not None: - loop.provider = built_provider - loop.model = raw_value + # Not a two-attribute assignment: the loop hands its provider to the + # subagent manager, the context engine and the consolidator at build + # time, and each keeps it. set_provider is what reaches them. + loop.set_provider(built_provider, raw_value) return {"applied": True, "previous": previous, "value": raw_value} diff --git a/tests/test_agent_loop_model_switch.py b/tests/test_agent_loop_model_switch.py new file mode 100644 index 00000000..74a420fc --- /dev/null +++ b/tests/test_agent_loop_model_switch.py @@ -0,0 +1,97 @@ +"""A live model switch must reach everything holding the old provider. + +``config.set key="model"`` builds a fresh provider and hands it to +``AgentLoop.set_provider``. The loop is not the only holder: the subagent +manager, the context engine's LLM-backed segments and the consolidator +each captured the provider they were built with. When the switch stopped +at ``loop.provider``, those three kept calling the abandoned endpoint -- +subagent spawns and the skill rewriter/gate failed to authenticate while +the main loop worked fine. +""" + +from types import SimpleNamespace + +from raven.agent.loop.main import AgentLoop +from raven.context_engine.assembler import ContextAssembler +from raven.memory_engine.skill_forge.gate import LLMGateFilter +from raven.memory_engine.skill_forge.rewriter import QueryRewriter + + +class _Recorder: + """Holder that remembers the provider/model it was last pointed at.""" + + def __init__(self) -> None: + self.provider: object = "old-provider" + self.model = "old-model" + + def set_provider(self, provider: object, model: str) -> None: + self.provider = provider + self.model = model + + +class _TextOnlyBuilder: + """A segment that never calls an LLM, so it has no set_provider.""" + + name = "identity" + order = 1 + needs_prefix = False + + async def build(self, ctx): # pragma: no cover - never invoked here + return None + + +def test_set_provider_reaches_every_holder() -> None: + loop = object.__new__(AgentLoop) + loop.provider = "old-provider" + loop.model = "old-model" + loop.subagents = _Recorder() + loop.context_engine = _Recorder() + loop.memory_consolidator = _Recorder() + + new_provider = SimpleNamespace(name="new-provider") + loop.set_provider(new_provider, "anthropic/claude-opus-4-8") + + assert loop.provider is new_provider + assert loop.model == "anthropic/claude-opus-4-8" + for holder in (loop.subagents, loop.context_engine, loop.memory_consolidator): + assert holder.provider is new_provider + assert holder.model == "anthropic/claude-opus-4-8" + + +def test_assembler_forwards_to_llm_backed_builders_only() -> None: + llm_builder = _Recorder() + llm_builder.name = "skills" + llm_builder.order = 5 + llm_builder.needs_prefix = False + text_builder = _TextOnlyBuilder() + + assembler = ContextAssembler([llm_builder, text_builder], lambda: []) + new_provider = SimpleNamespace(name="new-provider") + + # The text-only builder has no set_provider; walking must skip it rather + # than blow up, which is why the fan-out is duck-typed. + assembler.set_provider(new_provider, "anthropic/claude-opus-4-8") + + assert llm_builder.provider is new_provider + assert llm_builder.model == "anthropic/claude-opus-4-8" + + +def test_rewriter_and_gate_adopt_the_new_provider() -> None: + new_provider = SimpleNamespace(name="new-provider") + + rewriter = QueryRewriter("old-provider") + rewriter.set_provider(new_provider, "anthropic/claude-opus-4-8") + assert rewriter._provider is new_provider + + gate = LLMGateFilter("old-provider") + gate.set_provider(new_provider, "anthropic/claude-opus-4-8") + assert gate._provider is new_provider + # Unset means the gate follows the provider's default model, so the + # switch must not pin it to the agent's model behind the user's back. + assert gate._model is None + + +def test_pinned_gate_model_survives_the_switch() -> None: + gate = LLMGateFilter("old-provider", model="openai/gpt-5-mini") + gate.set_provider(SimpleNamespace(name="new-provider"), "anthropic/claude-opus-4-8") + assert gate._model == "openai/gpt-5-mini" diff --git a/tests/test_tui_rpc_config.py b/tests/test_tui_rpc_config.py index caa18cd3..66e031a7 100644 --- a/tests/test_tui_rpc_config.py +++ b/tests/test_tui_rpc_config.py @@ -137,10 +137,30 @@ async def test_config_set_missing_key_param_raises_validation(fake_home: Path) - # ---------------------------------------------------------------------------- +class _FakeLoop: + """Stand-in for AgentLoop's half of the switch contract. + + Records the ``set_provider`` calls: assigning ``provider``/``model`` + directly would leave the subagent manager, the context engine and the + consolidator on the old provider, so the handler must go through the + method, not the attributes. + """ + + def __init__(self, provider: object, model: str) -> None: + self.provider = provider + self.model = model + self.switches: list[tuple[object, str]] = [] + + def set_provider(self, provider: object, model: str) -> None: + self.provider = provider + self.model = model + self.switches.append((provider, model)) + + async def test_config_set_model_reassigns_loop_and_persists(fake_home: Path, monkeypatch) -> None: import raven.tui_rpc.methods.config as config_mod - loop = SimpleNamespace(provider="old-prov", model="old-model") + loop = _FakeLoop("old-prov", "old-model") new_provider = SimpleNamespace(name="new-prov") monkeypatch.setattr(config_mod, "is_turn_active", lambda _key: False) @@ -165,6 +185,9 @@ async def test_config_set_model_reassigns_loop_and_persists(fake_home: Path, mon assert result["value"] == "anthropic/claude-opus-4-8" assert loop.model == "anthropic/claude-opus-4-8" assert loop.provider is new_provider + # Routed through set_provider, so everything holding the old provider + # (subagents, context-engine segments, consolidator) gets told too. + assert loop.switches == [(new_provider, "anthropic/claude-opus-4-8")] cfg = json.loads((fake_home / ".raven" / "config.json").read_text()) assert cfg["agents"]["defaults"]["model"] == "anthropic/claude-opus-4-8" @@ -219,7 +242,7 @@ def _boom(_cfg): lambda *a, **k: SimpleNamespace(agents=SimpleNamespace(defaults=SimpleNamespace(model="", provider="auto"))), ) - loop = SimpleNamespace(provider="keep-prov", model="anthropic/claude-sonnet-4-5") + loop = _FakeLoop("keep-prov", "anthropic/claude-sonnet-4-5") with pytest.raises(ModelNotAvailableError): await config_set( { @@ -233,6 +256,7 @@ def _boom(_cfg): # Loop untouched and on-disk model preserved. assert loop.model == "anthropic/claude-sonnet-4-5" assert loop.provider == "keep-prov" + assert loop.switches == [] cfg = json.loads((fake_home / ".raven" / "config.json").read_text()) assert cfg["agents"]["defaults"]["model"] == "anthropic/claude-sonnet-4-5" From 1b44ad51c82f55c77f3bd81d241991578bdf916e Mon Sep 17 00:00:00 2001 From: arelchan <204152633+arelchan@users.noreply.github.com> Date: Sat, 8 Aug 2026 17:53:44 +0800 Subject: [PATCH 2/8] fix(agent): hold a model switch until the turn in flight ends Review of #282 found the fan-out landed but its promise did not. Both docstrings claimed a running turn or subagent keeps the provider it started with; nothing provided that. Every LLM call site reads the provider off self at call time, so before this the subagent manager's reference simply never changed -- the fan-out is what made a running subagent able to span two vendors, and that was documented as a deliberate non-change. Two mechanisms, because the two lifetimes differ. AgentLoop parks a switch that arrives mid-turn and adopts it at the next run_turn entry: one boundary covers the dozen self.provider reads plus the context engine and consolidator underneath them, where a snapshot would have to be threaded through each. A subagent is a detached task that outlives its turn, so the park cannot reach it; _run_subagent_inner reads the provider and model once before its iteration loop instead. Also from the review: - curator: drop the branch on config.curator_model. It is declared str with a non-empty default, so it is never falsy and the branch never ran; curator_model is always a pin, at construction too. - gate: stop describing a kept pin as safe. A pin is only a model id while the credential comes from the provider, so a pin naming a vendor the provider does not serve was already broken at boot. Fixing that pairing is a separate change. - context_engine.base: the concrete no-op exists because AgentLoop calls through the ABC unconditionally, not because an engine without LLM-backed segments exists. There is only one implementation. - main.py: the fan-out comment pointed the wrong way. All four receivers are above it, and the list below it names the one attribute not in the fan-out. Tests: the previous file only exercised the dispatcher, so replacing any receiver with pass left it green. It now builds a real AgentLoop and asserts the gate, rewriter, curator, curator assembler, trimmer, subagent manager and consolidator all moved; guards the attribute names the fan-out walks against a rename; and drives the real _run_subagent_inner across a switch. Each of those five mutations now fails something. Co-authored-by: Claude (claude-opus-5) --- raven/agent/loop/main.py | 80 +++++++- raven/agent/subagent/manager.py | 19 +- raven/context_engine/base.py | 5 +- raven/context_engine/segments/curator.py | 7 +- raven/memory_engine/skill_forge/gate.py | 11 +- tests/test_agent_loop_model_switch.py | 231 ++++++++++++++++++++++- 6 files changed, 323 insertions(+), 30 deletions(-) diff --git a/raven/agent/loop/main.py b/raven/agent/loop/main.py index 4eb92543..10f548dc 100644 --- a/raven/agent/loop/main.py +++ b/raven/agent/loop/main.py @@ -553,9 +553,17 @@ def __init__( self._consolidation_tasks: set[asyncio.Task] = set() - # Every subsystem below was handed ``provider`` above and holds its - # own reference; ``set_provider`` is what keeps them from outliving - # a live model switch. Add the call there when adding another. + # A switch that arrives mid-turn is parked here and adopted at the + # next ``run_turn`` entry, so the turn in flight finishes on the + # provider it started with. + self._pending_provider: tuple[LLMProvider, str] | None = None + self._turn_in_flight = False + + # ``self.subagents``, ``self.context_engine`` and + # ``self.memory_consolidator`` were each handed ``provider`` earlier in + # this constructor and hold their own reference; ``_adopt_provider`` is + # what keeps them from outliving a live model switch. Add the call + # there when adding another holder. # Phase B-3: the L4 facade (``DefaultMemoryEngine`` / # ``MemoryEngine`` ABC) has been retired. AgentLoop now holds @@ -623,16 +631,37 @@ def set_provider(self, provider: LLMProvider, model: str) -> None: credential fixed the main loop while subagents and the skill rewriter/gate went on failing to authenticate. - In-flight work is not migrated: a turn or subagent already running - finishes on the provider it started with, so one conversation never - spans two endpoints. + A switch that lands while a turn is running is parked rather than + applied: every call site reads ``self.provider`` at call time, so + adopting mid-turn would relay one conversation across two vendors + (LiteLLM drops the shapes the new vendor rejects instead of + failing, so the split is silent). ``run_turn`` adopts it on the way + in, which makes the switch effective from the next turn. + + Detached subagents are not covered by that park -- they outlive the + turn that spawned them -- so ``SubagentManager`` snapshots instead. """ + if self._turn_in_flight: + self._pending_provider = (provider, model) + return + self._adopt_provider(provider, model) + + def _adopt_provider(self, provider: LLMProvider, model: str) -> None: + """Hand a provider to the loop and every subsystem holding the old one.""" self.provider = provider self.model = model self.subagents.set_provider(provider, model) self.context_engine.set_provider(provider, model) self.memory_consolidator.set_provider(provider, model) + def _adopt_pending_provider(self) -> None: + """Apply a switch parked by ``set_provider`` during the previous turn.""" + pending = self._pending_provider + if pending is None: + return + self._pending_provider = None + self._adopt_provider(*pending) + def configure_personalization(self, enable: bool) -> None: """Global switch for the 4-step personalization flow (PAHF-inspired). @@ -2584,14 +2613,49 @@ async def run_turn( inline_tool_stream: bool = False, usage_sink: dict[str, Any] | None = None, text_sink: dict[str, Any] | None = None, + ) -> TurnOutcome: + """Turn boundary for a live ``/model`` switch; see ``_run_turn``. + + A switch parked by ``set_provider`` is adopted here, before the turn + reads ``self.provider`` for the first time, and the flag set for the + duration is what parks the next one. Wrapping rather than snapshotting + because the provider is read from ``self`` at a dozen call sites and by + the context engine and consolidator underneath them -- one boundary + covers all of them, a snapshot would have to be threaded through each. + """ + self._adopt_pending_provider() + self._turn_in_flight = True + try: + return await self._run_turn( + req, + emit, + drain, + stream=stream, + inline_tool_stream=inline_tool_stream, + usage_sink=usage_sink, + text_sink=text_sink, + ) + finally: + self._turn_in_flight = False + + async def _run_turn( + self, + req: TurnRequest, + emit: Emit, + drain: Drain, + *, + stream: bool = True, + inline_tool_stream: bool = False, + usage_sink: dict[str, Any] | None = None, + text_sink: dict[str, Any] | None = None, ) -> TurnOutcome: """Spine-native turn entry: consume a TurnRequest, fan the agent's output onto the single ``emit``, return a TurnOutcome. Collapses the legacy output paths (a str return + the five callbacks) onto one boundary. Named ``run_turn`` rather than ``run``: ``run`` is the runtime keep-alive - (executor / debug server / MCP up, then idle). A spine runner wraps this - method to satisfy the TurnRunner protocol. + (executor / debug server / MCP up, then idle). A spine runner calls the + public ``run_turn`` to satisfy the TurnRunner protocol. ``stream`` is the canon Q2-D assembly switch: a streaming outlet (TUI) wires it True so the reply goes out as StreamDelta and dissolves (b2 — no diff --git a/raven/agent/subagent/manager.py b/raven/agent/subagent/manager.py index 1e32a739..1d109708 100644 --- a/raven/agent/subagent/manager.py +++ b/raven/agent/subagent/manager.py @@ -80,9 +80,10 @@ def set_provider(self, provider: LLMProvider, model: str) -> None: Subagents run on the parent's provider, so a switch that is not propagated here leaves every spawn calling the credential the loop - has already abandoned. Tasks already in flight keep the provider - they started with -- swapping mid-turn would split one subagent's - conversation across two endpoints. + has already abandoned. Only spawns started after this call are + affected: a subagent is a detached task that outlives the turn that + spawned it, so the loop's park cannot cover it and + ``_run_subagent_inner`` snapshots what it starts with. """ self.provider = provider self.model = model @@ -200,13 +201,21 @@ async def _run_subagent_inner( final_result: str | None = None final_status = "ok" + # Read once, not per iteration: a ``/model`` switch can land + # between two iterations of a task that runs for minutes, and + # picking it up here would send the second half of one + # conversation to a different vendor carrying the first half's + # message shapes. + provider = self.provider + model = self.model + while iteration < max_iterations: iteration += 1 - response = await self.provider.chat_with_retry( + response = await provider.chat_with_retry( messages=messages, tools=tools.get_definitions(), - model=self.model, + model=model, ) if response.has_tool_calls: diff --git a/raven/context_engine/base.py b/raven/context_engine/base.py index 77b15356..910776a1 100644 --- a/raven/context_engine/base.py +++ b/raven/context_engine/base.py @@ -148,8 +148,9 @@ def set_provider(self, provider: "LLMProvider", model: str) -> None: Segments that call an LLM hold the provider handed to them at construction; without this they keep calling the old one for the - rest of the process. Concrete rather than abstract so an engine - with no LLM-backed segment needs no override. + rest of the process. Concrete rather than abstract because + ``AgentLoop.context_engine`` is typed to this ABC and the loop calls + this unconditionally -- a no-op default keeps that call total. """ @abstractmethod diff --git a/raven/context_engine/segments/curator.py b/raven/context_engine/segments/curator.py index 28779a2b..a6e71df9 100644 --- a/raven/context_engine/segments/curator.py +++ b/raven/context_engine/segments/curator.py @@ -85,13 +85,12 @@ def __init__( def set_provider(self, provider: LLMProvider, model: str) -> None: """Adopt the provider a live ``/model`` switch just built. - ``curator_model`` only follows when it was following the agent's - model already -- an explicit ``config.curator_model`` is a pin. + ``curator_model`` is left alone: it is declared ``str`` with a + non-empty default (``ContextConfig.curator_model``), so it is always + a pin and never follows the agent's model -- at construction either. """ self.provider = provider self.model = model - if not self.config.curator_model: - self.curator_model = model self.assembler.set_provider(provider, model) async def build(self, ctx: AssemblyContext) -> Segment | None: diff --git a/raven/memory_engine/skill_forge/gate.py b/raven/memory_engine/skill_forge/gate.py index 7dcde286..10184dc9 100644 --- a/raven/memory_engine/skill_forge/gate.py +++ b/raven/memory_engine/skill_forge/gate.py @@ -64,9 +64,14 @@ def __init__( def set_provider(self, provider: "LLMProvider", model: str) -> None: """Adopt the provider a live ``/model`` switch just built. - ``_model`` is left alone either way: unset means the gate already - follows whatever the new provider defaults to, and set means a - deliberate pin that a switch elsewhere must not undo. + Unset, ``_model`` follows whatever the new provider defaults to. + + Set, it is a pin, and this leaves it pinned -- which is what a + restart on the new model would produce, since the gate is built + with the agent's provider and the pin regardless of which vendor + the pin names. Note that a pin is only a model id: the credential + comes from the provider, so a pin naming a vendor the provider does + not serve was already broken at boot, and stays broken here. """ del model self._provider = provider diff --git a/tests/test_agent_loop_model_switch.py b/tests/test_agent_loop_model_switch.py index 74a420fc..6444899a 100644 --- a/tests/test_agent_loop_model_switch.py +++ b/tests/test_agent_loop_model_switch.py @@ -7,14 +7,45 @@ at ``loop.provider``, those three kept calling the abandoned endpoint -- subagent spawns and the skill rewriter/gate failed to authenticate while the main loop worked fine. + +Work already in flight is the other half: every call site reads the +provider from ``self`` at call time, so an unconditional swap would relay +one conversation across two vendors. The loop parks a mid-turn switch +until the next ``run_turn``; a detached subagent outlives that window and +snapshots instead. """ +from __future__ import annotations + +import asyncio from types import SimpleNamespace +import pytest + from raven.agent.loop.main import AgentLoop +from raven.agent.subagent.manager import SubagentManager +from raven.config.raven import ContextConfig, SkillForgeConfig from raven.context_engine.assembler import ContextAssembler +from raven.context_engine.segments.curator import CuratorSegmentBuilder +from raven.context_engine.segments.skills import SkillsSegmentBuilder from raven.memory_engine.skill_forge.gate import LLMGateFilter from raven.memory_engine.skill_forge.rewriter import QueryRewriter +from raven.providers.base import LLMResponse, ToolCallRequest + +NEW_MODEL = "anthropic/claude-opus-4-8" + + +class _Provider: + """Minimal provider stand-in; identity is what the assertions track.""" + + def __init__(self, name: str = "old") -> None: + self.name = name + + def get_default_model(self) -> str: + return "fake/model" + + async def chat_with_retry(self, **kwargs) -> LLMResponse: + return LLMResponse(content="ok", finish_reason="stop") class _Recorder: @@ -29,6 +60,22 @@ def set_provider(self, provider: object, model: str) -> None: self.model = model +class _StubExecutor: + """``_run_subagent_inner`` only passes this to ExecTool; no command runs.""" + + @property + def is_sandboxed(self) -> bool: + return False + + async def exec(self, command: str, **kwargs): # pragma: no cover - unused + raise NotImplementedError + + +def _noop_submit(*args, **kwargs) -> None: + """``_announce_result`` calls the spine submit without awaiting it.""" + return None + + class _TextOnlyBuilder: """A segment that never calls an LLM, so it has no set_provider.""" @@ -40,6 +87,21 @@ async def build(self, ctx): # pragma: no cover - never invoked here return None +def _loop(tmp_path) -> AgentLoop: + return AgentLoop( + provider=_Provider(), + workspace=tmp_path, + model="fake/model", + context_config=ContextConfig(), + skill_forge_config=SkillForgeConfig(), + ) + + +# --------------------------------------------------------------------------- +# Fan-out +# --------------------------------------------------------------------------- + + def test_set_provider_reaches_every_holder() -> None: loop = object.__new__(AgentLoop) loop.provider = "old-provider" @@ -47,15 +109,59 @@ def test_set_provider_reaches_every_holder() -> None: loop.subagents = _Recorder() loop.context_engine = _Recorder() loop.memory_consolidator = _Recorder() + loop._turn_in_flight = False + loop._pending_provider = None new_provider = SimpleNamespace(name="new-provider") - loop.set_provider(new_provider, "anthropic/claude-opus-4-8") + loop.set_provider(new_provider, NEW_MODEL) assert loop.provider is new_provider - assert loop.model == "anthropic/claude-opus-4-8" + assert loop.model == NEW_MODEL for holder in (loop.subagents, loop.context_engine, loop.memory_consolidator): assert holder.provider is new_provider - assert holder.model == "anthropic/claude-opus-4-8" + assert holder.model == NEW_MODEL + + +def test_switch_reaches_the_real_holders_a_loop_builds(tmp_path) -> None: + """The stubbed fan-out above proves the dispatcher; this proves the + receivers. Every holder here is the class a real run uses, reached by + walking the engine the factory actually assembled -- so a setter that is + renamed, dropped, or quietly wrong fails here instead of in production. + """ + loop = _loop(tmp_path) + engine = loop.context_engine + assert isinstance(engine, ContextAssembler) + + skills = next(b for b in engine._builders if isinstance(b, SkillsSegmentBuilder)) + curator = next(b for b in engine._builders if isinstance(b, CuratorSegmentBuilder)) + assert skills._gate is not None, "llm_gate_enabled defaults True; the gate is a holder" + assert skills._rewriter is not None + + new_provider = _Provider("new") + loop.set_provider(new_provider, NEW_MODEL) + + assert loop.provider is new_provider + assert loop.subagents.provider is new_provider + assert loop.subagents.model == NEW_MODEL + assert loop.memory_consolidator.provider is new_provider + assert skills._gate._provider is new_provider + assert skills._rewriter._provider is new_provider + assert curator.provider is new_provider + assert curator.assembler.provider is new_provider + assert curator.assembler.trimmer.provider is new_provider + + +def test_fan_out_targets_still_exist_on_a_real_loop(tmp_path) -> None: + """Guard against silent drift. ``ContextAssembler.set_provider`` walks its + builders duck-typed and skips anything without the method, so a renamed + holder or setter would leave the fan-out green while it quietly stops + covering that subsystem. + """ + loop = _loop(tmp_path) + for attr in ("subagents", "context_engine", "memory_consolidator"): + holder = getattr(loop, attr, None) + assert holder is not None, f"AgentLoop.{attr} is gone; _adopt_provider still calls it" + assert callable(getattr(holder, "set_provider", None)), f"AgentLoop.{attr} lost set_provider" def test_assembler_forwards_to_llm_backed_builders_only() -> None: @@ -70,21 +176,26 @@ def test_assembler_forwards_to_llm_backed_builders_only() -> None: # The text-only builder has no set_provider; walking must skip it rather # than blow up, which is why the fan-out is duck-typed. - assembler.set_provider(new_provider, "anthropic/claude-opus-4-8") + assembler.set_provider(new_provider, NEW_MODEL) assert llm_builder.provider is new_provider - assert llm_builder.model == "anthropic/claude-opus-4-8" + assert llm_builder.model == NEW_MODEL + + +# --------------------------------------------------------------------------- +# Pins +# --------------------------------------------------------------------------- def test_rewriter_and_gate_adopt_the_new_provider() -> None: new_provider = SimpleNamespace(name="new-provider") rewriter = QueryRewriter("old-provider") - rewriter.set_provider(new_provider, "anthropic/claude-opus-4-8") + rewriter.set_provider(new_provider, NEW_MODEL) assert rewriter._provider is new_provider gate = LLMGateFilter("old-provider") - gate.set_provider(new_provider, "anthropic/claude-opus-4-8") + gate.set_provider(new_provider, NEW_MODEL) assert gate._provider is new_provider # Unset means the gate follows the provider's default model, so the # switch must not pin it to the agent's model behind the user's back. @@ -93,5 +204,109 @@ def test_rewriter_and_gate_adopt_the_new_provider() -> None: def test_pinned_gate_model_survives_the_switch() -> None: gate = LLMGateFilter("old-provider", model="openai/gpt-5-mini") - gate.set_provider(SimpleNamespace(name="new-provider"), "anthropic/claude-opus-4-8") + gate.set_provider(SimpleNamespace(name="new-provider"), NEW_MODEL) assert gate._model == "openai/gpt-5-mini" + + +def test_curator_model_is_always_a_pin(tmp_path) -> None: + """``ContextConfig.curator_model`` is ``str`` with a non-empty default, so + it never follows the agent model -- at construction or across a switch. + """ + loop = _loop(tmp_path) + curator = next(b for b in loop.context_engine._builders if isinstance(b, CuratorSegmentBuilder)) + pinned = curator.curator_model + assert pinned == ContextConfig().curator_model + + loop.set_provider(_Provider("new"), NEW_MODEL) + assert curator.curator_model == pinned + + +# --------------------------------------------------------------------------- +# In-flight work +# --------------------------------------------------------------------------- + + +def test_a_switch_during_a_turn_is_parked_until_the_next_one(tmp_path) -> None: + """Every call site reads ``self.provider`` at call time, so adopting + mid-turn would send the rest of one turn to a different vendor. + """ + loop = _loop(tmp_path) + started = _Provider("started-with") + loop.set_provider(started, "started/model") + + loop._turn_in_flight = True + switched = _Provider("switched-to") + loop.set_provider(switched, NEW_MODEL) + + assert loop.provider is started, "the turn in flight must keep what it started with" + assert loop.subagents.provider is started + assert loop._pending_provider == (switched, NEW_MODEL) + + loop._turn_in_flight = False + loop._adopt_pending_provider() + + assert loop.provider is switched + assert loop.subagents.provider is switched + assert loop._pending_provider is None + + +def test_a_switch_between_turns_applies_immediately(tmp_path) -> None: + loop = _loop(tmp_path) + switched = _Provider("switched-to") + loop.set_provider(switched, NEW_MODEL) + + assert loop.provider is switched + assert loop._pending_provider is None + + +@pytest.mark.asyncio +async def test_a_running_subagent_keeps_the_provider_it_started_with(tmp_path) -> None: + """A subagent is a detached task that outlives the turn that spawned it, + so the loop's park cannot cover it -- it snapshots at entry instead. Without + that, iteration k+1 calls the new vendor carrying k iterations of the old + vendor's message shapes, and LiteLLM drops what the new one rejects rather + than failing, so the split conversation is silent. + """ + seen: list[tuple[str, str]] = [] + release = asyncio.Event() + + class _TwoStepProvider(_Provider): + async def chat_with_retry(self, **kwargs) -> LLMResponse: + seen.append((self.name, kwargs.get("model"))) + if len(seen) == 1: + # Hold the task open across the switch, then ask for one more + # iteration so a re-read of self.provider would show up. + await release.wait() + return LLMResponse( + content="", + tool_calls=[ToolCallRequest(id="c1", name="list_dir", arguments={"path": "."})], + finish_reason="tool_calls", + ) + return LLMResponse(content="done", finish_reason="stop") + + manager = SubagentManager( + provider=_TwoStepProvider("started-with"), + workspace=tmp_path, + model="started/model", + ) + manager._submit = _noop_submit + + task = asyncio.create_task( + manager._run_subagent_inner( + "t1", + "do the thing", + "thing", + {"channel": "cli", "chat_id": "direct", "session_key": "s"}, + _StubExecutor(), + ) + ) + await asyncio.sleep(0) + manager.set_provider(_TwoStepProvider("switched-to"), NEW_MODEL) + release.set() + await task + + assert [name for name, _ in seen] == ["started-with", "started-with"] + assert [model for _, model in seen] == ["started/model", "started/model"] + # The next spawn does get the new one -- the snapshot is per task, not a freeze. + assert manager.provider.name == "switched-to" + assert manager.model == NEW_MODEL From 8b1e65b51581abe45974fbb16ad773ce7bc037fc Mon Sep 17 00:00:00 2001 From: arelchan <204152633+arelchan@users.noreply.github.com> Date: Sat, 8 Aug 2026 18:29:57 +0800 Subject: [PATCH 3/8] fix(agent): park a model switch on a turn count, not a flag Panel review of the two commits above found the park did nothing in the one configuration it exists for. OriginPools gates USER and system origins on independent semaphores with no global cap (spine/scheduler.py), and the TUI defaults to one slot each, so a user turn and a cron turn run concurrently on one AgentLoop. With a bool: the shorter turn's finally cleared the flag under the longer one, and a correctly parked switch was adopted by an unrelated turn entering run_turn. Both land the switch mid-flight, which is what the park exists to prevent. Now a depth counter, with both ends gated on zero, and the last turn out adopts so a park cannot outlive the turns it waited on. The subagent snapshot moved from _run_subagent_inner to spawn. A spawn queues behind the concurrency gate and a sandbox boot before the inner method runs, and a switch landing in that window handed the task an endpoint the user chose after asking for it -- so "only spawns started after this call are affected" was not true of the window that matters. Three prose corrections, all cases of describing a property the code does not have: - "LiteLLM drops the shapes the new vendor rejects instead of failing" named the wrong mechanism. drop_params filters request kwargs, not message content. The silence comes from the provider turning a rejected request into finish_reason="error" content. - "curator_model is always a pin, at construction either" was false for an explicitly empty context.curator_model, which the constructor's own `or model` still follows. set_provider now re-derives with the constructor's expression instead of asserting. - "a dozen call sites" was eight. The park's relationship to the RPC guard is now stated: is_turn_active rejects a same-session switch first, the park covers what that cannot see, and a parked switch is on disk while the loop still reports the old model. Tests: the run_turn wrapper had no coverage at all -- deleting its finally left the suite green -- because the park test hand-set the flag and hand-called the adopt. It now drives the real run_turn: adopt on entry, slot released on return and on exception, and a second concurrent turn that must not unpark a switch held for the first. Plus a spawn-time snapshot test. Signature change to _run_subagent/_run_subagent_inner updated in the two suites that stub them. Co-authored-by: Claude (claude-opus-5) --- raven/agent/loop/main.py | 66 ++++++--- raven/agent/subagent/manager.py | 28 ++-- raven/context_engine/segments/curator.py | 9 +- tests/test_agent_loop_model_switch.py | 171 +++++++++++++++++++++-- tests/test_sandbox_unit.py | 9 +- tests/test_subagent_manager.py | 6 +- 6 files changed, 235 insertions(+), 54 deletions(-) diff --git a/raven/agent/loop/main.py b/raven/agent/loop/main.py index 10f548dc..dc905053 100644 --- a/raven/agent/loop/main.py +++ b/raven/agent/loop/main.py @@ -553,11 +553,14 @@ def __init__( self._consolidation_tasks: set[asyncio.Task] = set() - # A switch that arrives mid-turn is parked here and adopted at the - # next ``run_turn`` entry, so the turn in flight finishes on the - # provider it started with. + # A switch that arrives mid-turn is parked here until no turn is + # running, so a turn in flight finishes on the provider it started + # with. A depth counter, not a flag: OriginPools gates USER and + # system origins on independent semaphores with no global cap + # (spine/scheduler.py), so a user turn and a cron turn overlap on + # this loop under the TUI defaults. self._pending_provider: tuple[LLMProvider, str] | None = None - self._turn_in_flight = False + self._turns_in_flight = 0 # ``self.subagents``, ``self.context_engine`` and # ``self.memory_consolidator`` were each handed ``provider`` earlier in @@ -631,17 +634,27 @@ def set_provider(self, provider: LLMProvider, model: str) -> None: credential fixed the main loop while subagents and the skill rewriter/gate went on failing to authenticate. - A switch that lands while a turn is running is parked rather than - applied: every call site reads ``self.provider`` at call time, so - adopting mid-turn would relay one conversation across two vendors - (LiteLLM drops the shapes the new vendor rejects instead of - failing, so the split is silent). ``run_turn`` adopts it on the way - in, which makes the switch effective from the next turn. + A switch that lands while any turn is running is parked rather than + applied: the loop reads ``self.provider`` at call time (eight sites + in this module, plus the context engine and consolidator + underneath), so adopting mid-turn would relay one conversation + across two vendors. That split does not raise -- the provider turns + a rejected request into ``finish_reason="error"`` content, so the + turn reports a failure with no indication that its endpoint moved. + + The park is the second line of defence, not the first: the RPC + rejects a switch outright when the caller's own session has a turn + in flight (``is_turn_active`` in ``tui_rpc.methods.config``). This + covers what that guard cannot see -- a caller that passes no + ``session_id``, and the proactive turns that run in their own lanes. + Note the RPC still answers ``applied: True`` and the config file is + already written, so a parked switch is applied on disk while the + loop reports the old model until the last turn drains. Detached subagents are not covered by that park -- they outlive the turn that spawned them -- so ``SubagentManager`` snapshots instead. """ - if self._turn_in_flight: + if self._turns_in_flight: self._pending_provider = (provider, model) return self._adopt_provider(provider, model) @@ -655,7 +668,7 @@ def _adopt_provider(self, provider: LLMProvider, model: str) -> None: self.memory_consolidator.set_provider(provider, model) def _adopt_pending_provider(self) -> None: - """Apply a switch parked by ``set_provider`` during the previous turn.""" + """Apply a parked switch. Callers must check that no turn is running.""" pending = self._pending_provider if pending is None: return @@ -2616,15 +2629,24 @@ async def run_turn( ) -> TurnOutcome: """Turn boundary for a live ``/model`` switch; see ``_run_turn``. - A switch parked by ``set_provider`` is adopted here, before the turn - reads ``self.provider`` for the first time, and the flag set for the - duration is what parks the next one. Wrapping rather than snapshotting - because the provider is read from ``self`` at a dozen call sites and by - the context engine and consolidator underneath them -- one boundary - covers all of them, a snapshot would have to be threaded through each. + A parked switch is adopted here, before the turn reads + ``self.provider`` for the first time, and the count kept for the + duration is what parks the next one. Wrapping rather than + snapshotting because the provider is read from ``self`` at eight + sites in this module and by the context engine and consolidator + underneath them -- one boundary covers all of them, a snapshot + would have to be threaded through each. + + Both ends gate on zero, because turns overlap: a user turn and a + proactive turn hold slots in separate pools. Adopting on the way in + would otherwise land a switch parked for a turn that is still + running, and clearing a flag on the way out would unpark it just as + wrongly. Adopting again on the last exit is what keeps a park from + outliving the turns it was waiting on. """ - self._adopt_pending_provider() - self._turn_in_flight = True + if self._turns_in_flight == 0: + self._adopt_pending_provider() + self._turns_in_flight += 1 try: return await self._run_turn( req, @@ -2636,7 +2658,9 @@ async def run_turn( text_sink=text_sink, ) finally: - self._turn_in_flight = False + self._turns_in_flight -= 1 + if self._turns_in_flight == 0: + self._adopt_pending_provider() async def _run_turn( self, diff --git a/raven/agent/subagent/manager.py b/raven/agent/subagent/manager.py index 1d109708..3e6ef382 100644 --- a/raven/agent/subagent/manager.py +++ b/raven/agent/subagent/manager.py @@ -80,10 +80,10 @@ def set_provider(self, provider: LLMProvider, model: str) -> None: Subagents run on the parent's provider, so a switch that is not propagated here leaves every spawn calling the credential the loop - has already abandoned. Only spawns started after this call are + has already abandoned. Only spawns requested after this call are affected: a subagent is a detached task that outlives the turn that - spawned it, so the loop's park cannot cover it and - ``_run_subagent_inner`` snapshots what it starts with. + spawned it, so the loop's park cannot cover it and ``spawn`` + snapshots the pair it was asked for. """ self.provider = provider self.model = model @@ -120,7 +120,13 @@ async def spawn( display_label = label or task[:30] + ("..." if len(task) > 30 else "") origin = {"channel": origin_channel, "chat_id": origin_chat_id, "session_key": quota_key} - bg_task = asyncio.create_task(self._run_subagent(task_id, task, display_label, origin)) + # Snapshot here rather than where the task starts running: it queues + # behind the concurrency gate and a sandbox boot first, and a switch + # landing in that window would hand this task an endpoint the user + # chose after asking for it. + bg_task = asyncio.create_task( + self._run_subagent(task_id, task, display_label, origin, self.provider, self.model) + ) self._running_tasks[task_id] = bg_task if session_key: self._session_tasks.setdefault(session_key, set()).add(task_id) @@ -144,6 +150,8 @@ async def _run_subagent( task: str, label: str, origin: dict[str, str], + provider: LLMProvider, + model: str, ) -> None: """Execute the subagent task and announce the result.""" logger.info("Subagent [{}] starting task: {}", task_id, label) @@ -154,7 +162,7 @@ async def _run_subagent( async with self._gate: executor = build_executor(self._sandbox_config, self.workspace, self._owned_ids) async with executor: - await self._run_subagent_inner(task_id, task, label, origin, executor) + await self._run_subagent_inner(task_id, task, label, origin, executor, provider, model) except Exception as e: error_msg = f"Error: {str(e)}" logger.error("Subagent [{}] failed: {}", task_id, e) @@ -167,6 +175,8 @@ async def _run_subagent_inner( label: str, origin: dict[str, str], executor: Any, + provider: LLMProvider, + model: str, ) -> None: try: # Build subagent tools (no message tool, no spawn tool) @@ -201,14 +211,6 @@ async def _run_subagent_inner( final_result: str | None = None final_status = "ok" - # Read once, not per iteration: a ``/model`` switch can land - # between two iterations of a task that runs for minutes, and - # picking it up here would send the second half of one - # conversation to a different vendor carrying the first half's - # message shapes. - provider = self.provider - model = self.model - while iteration < max_iterations: iteration += 1 diff --git a/raven/context_engine/segments/curator.py b/raven/context_engine/segments/curator.py index a6e71df9..1c2055d7 100644 --- a/raven/context_engine/segments/curator.py +++ b/raven/context_engine/segments/curator.py @@ -85,12 +85,15 @@ def __init__( def set_provider(self, provider: LLMProvider, model: str) -> None: """Adopt the provider a live ``/model`` switch just built. - ``curator_model`` is left alone: it is declared ``str`` with a - non-empty default (``ContextConfig.curator_model``), so it is always - a pin and never follows the agent's model -- at construction either. + ``curator_model`` is re-derived with the constructor's own + expression, so a switch cannot make it mean something it did not + mean at build time. The default is non-empty, so in practice it is + a pin; an explicitly empty ``context.curator_model`` is the one + config that follows the agent model, and it follows it here too. """ self.provider = provider self.model = model + self.curator_model = self.config.curator_model or model self.assembler.set_provider(provider, model) async def build(self, ctx: AssemblyContext) -> Segment | None: diff --git a/tests/test_agent_loop_model_switch.py b/tests/test_agent_loop_model_switch.py index 6444899a..c4550e68 100644 --- a/tests/test_agent_loop_model_switch.py +++ b/tests/test_agent_loop_model_switch.py @@ -10,9 +10,11 @@ Work already in flight is the other half: every call site reads the provider from ``self`` at call time, so an unconditional swap would relay -one conversation across two vendors. The loop parks a mid-turn switch -until the next ``run_turn``; a detached subagent outlives that window and -snapshots instead. +one conversation across two vendors. The loop parks a switch until no +turn is running -- a count, not a flag, because a user turn and a +proactive turn hold slots in separate pools and overlap. A detached +subagent outlives that window entirely, so ``spawn`` snapshots the pair +it was asked for. """ from __future__ import annotations @@ -109,7 +111,7 @@ def test_set_provider_reaches_every_holder() -> None: loop.subagents = _Recorder() loop.context_engine = _Recorder() loop.memory_consolidator = _Recorder() - loop._turn_in_flight = False + loop._turns_in_flight = 0 loop._pending_provider = None new_provider = SimpleNamespace(name="new-provider") @@ -208,9 +210,9 @@ def test_pinned_gate_model_survives_the_switch() -> None: assert gate._model == "openai/gpt-5-mini" -def test_curator_model_is_always_a_pin(tmp_path) -> None: - """``ContextConfig.curator_model`` is ``str`` with a non-empty default, so - it never follows the agent model -- at construction or across a switch. +def test_the_default_curator_model_is_a_pin(tmp_path) -> None: + """``ContextConfig.curator_model`` has a non-empty default, so in practice + it does not follow the agent model -- at construction or across a switch. """ loop = _loop(tmp_path) curator = next(b for b in loop.context_engine._builders if isinstance(b, CuratorSegmentBuilder)) @@ -221,12 +223,32 @@ def test_curator_model_is_always_a_pin(tmp_path) -> None: assert curator.curator_model == pinned +def test_an_empty_curator_model_follows_the_agent_model_both_times(tmp_path) -> None: + """The field has no ``min_length``, so ``curator_model: ""`` validates and + the constructor's ``or model`` follows the agent model. A switch has to + follow it too, or the same config means one thing at build time and + another after. + """ + loop = AgentLoop( + provider=_Provider(), + workspace=tmp_path, + model="fake/model", + context_config=ContextConfig(curator_model=""), + skill_forge_config=SkillForgeConfig(), + ) + curator = next(b for b in loop.context_engine._builders if isinstance(b, CuratorSegmentBuilder)) + assert curator.curator_model == "fake/model" + + loop.set_provider(_Provider("new"), NEW_MODEL) + assert curator.curator_model == NEW_MODEL + + # --------------------------------------------------------------------------- # In-flight work # --------------------------------------------------------------------------- -def test_a_switch_during_a_turn_is_parked_until_the_next_one(tmp_path) -> None: +def test_a_switch_during_a_turn_is_parked(tmp_path) -> None: """Every call site reads ``self.provider`` at call time, so adopting mid-turn would send the rest of one turn to a different vendor. """ @@ -234,7 +256,7 @@ def test_a_switch_during_a_turn_is_parked_until_the_next_one(tmp_path) -> None: started = _Provider("started-with") loop.set_provider(started, "started/model") - loop._turn_in_flight = True + loop._turns_in_flight = 1 switched = _Provider("switched-to") loop.set_provider(switched, NEW_MODEL) @@ -242,7 +264,7 @@ def test_a_switch_during_a_turn_is_parked_until_the_next_one(tmp_path) -> None: assert loop.subagents.provider is started assert loop._pending_provider == (switched, NEW_MODEL) - loop._turn_in_flight = False + loop._turns_in_flight = 0 loop._adopt_pending_provider() assert loop.provider is switched @@ -262,10 +284,9 @@ def test_a_switch_between_turns_applies_immediately(tmp_path) -> None: @pytest.mark.asyncio async def test_a_running_subagent_keeps_the_provider_it_started_with(tmp_path) -> None: """A subagent is a detached task that outlives the turn that spawned it, - so the loop's park cannot cover it -- it snapshots at entry instead. Without + so the loop's park cannot cover it -- ``spawn`` snapshots instead. Without that, iteration k+1 calls the new vendor carrying k iterations of the old - vendor's message shapes, and LiteLLM drops what the new one rejects rather - than failing, so the split conversation is silent. + vendor's message shapes. """ seen: list[tuple[str, str]] = [] release = asyncio.Event() @@ -298,6 +319,8 @@ async def chat_with_retry(self, **kwargs) -> LLMResponse: "thing", {"channel": "cli", "chat_id": "direct", "session_key": "s"}, _StubExecutor(), + manager.provider, + manager.model, ) ) await asyncio.sleep(0) @@ -310,3 +333,125 @@ async def chat_with_retry(self, **kwargs) -> LLMResponse: # The next spawn does get the new one -- the snapshot is per task, not a freeze. assert manager.provider.name == "switched-to" assert manager.model == NEW_MODEL + + +@pytest.mark.asyncio +async def test_run_turn_adopts_on_entry_and_releases_on_exit(tmp_path) -> None: + """The wrapper is the whole mechanism, so drive the real one. Asserting on + ``_turns_in_flight`` alone would pass with the wrapper deleted. + """ + loop = _loop(tmp_path) + started = _Provider("started-with") + loop.set_provider(started, "started/model") + + switched = _Provider("switched-to") + loop._pending_provider = (switched, NEW_MODEL) + + seen: dict[str, object] = {} + + async def _fake_run_turn(*args, **kwargs): + seen["provider"] = loop.provider + seen["depth"] = loop._turns_in_flight + return "outcome" + + loop._run_turn = _fake_run_turn + assert await loop.run_turn(None, None, None) == "outcome" + + assert seen["provider"] is switched, "a parked switch must land before the turn reads it" + assert seen["depth"] == 1 + assert loop._turns_in_flight == 0 + + +@pytest.mark.asyncio +async def test_run_turn_releases_its_slot_when_the_turn_raises(tmp_path) -> None: + """A turn that fails must not leave the loop looking busy forever -- every + later switch would be parked and never adopted. + """ + loop = _loop(tmp_path) + + async def _boom(*args, **kwargs): + raise RuntimeError("turn failed") + + loop._run_turn = _boom + with pytest.raises(RuntimeError): + await loop.run_turn(None, None, None) + + assert loop._turns_in_flight == 0 + + switched = _Provider("switched-to") + loop.set_provider(switched, NEW_MODEL) + assert loop.provider is switched + + +@pytest.mark.asyncio +async def test_a_second_turn_does_not_unpark_a_switch_under_the_first(tmp_path) -> None: + """OriginPools gates USER and system origins on independent semaphores with + no global cap, so a user turn and a cron turn overlap on one loop. A count + is what makes the park survive the shorter of the two. + """ + loop = _loop(tmp_path) + started = _Provider("started-with") + loop.set_provider(started, "started/model") + + long_turn_running = asyncio.Event() + release_long = asyncio.Event() + switched = _Provider("switched-to") + during_short: dict[str, object] = {} + + async def _long(*args, **kwargs): + long_turn_running.set() + await release_long.wait() + during_short["provider_at_end_of_long"] = loop.provider + return "long" + + async def _short(*args, **kwargs): + during_short["provider_during_short"] = loop.provider + return "short" + + loop._run_turn = _long + long_task = asyncio.create_task(loop.run_turn(None, None, None)) + await long_turn_running.wait() + + # The switch lands while only the long turn is running. + loop.set_provider(switched, NEW_MODEL) + assert loop.provider is started + assert loop._pending_provider is not None + + # A second, shorter turn starts and finishes underneath it. + loop._run_turn = _short + await loop.run_turn(None, None, None) + + assert during_short["provider_during_short"] is started, "the short turn must not adopt the park" + assert loop.provider is started, "the long turn is still running" + assert loop._turns_in_flight == 1 + + release_long.set() + await long_task + + assert loop.provider is switched, "the last turn out adopts it" + assert loop._pending_provider is None + + +@pytest.mark.asyncio +async def test_spawn_snapshots_before_the_task_queues(tmp_path) -> None: + """The snapshot is taken in ``spawn``, not where the task starts running: + a spawn waits on the concurrency gate and a sandbox boot first, and a + switch landing in that window would move an endpoint the user asked for + before switching. + """ + manager = SubagentManager(provider=_Provider("started-with"), workspace=tmp_path, model="started/model") + manager._gate = asyncio.Semaphore(0) # nothing gets past this + + captured: dict[str, object] = {} + + async def _capture(task_id, task, label, origin, provider, model): + captured["provider"] = provider + captured["model"] = model + + manager._run_subagent = _capture + await manager.spawn("do the thing", label="thing", session_key="s") + manager.set_provider(_Provider("switched-to"), NEW_MODEL) + await asyncio.sleep(0) + + assert captured["provider"].name == "started-with" + assert captured["model"] == "started/model" diff --git a/tests/test_sandbox_unit.py b/tests/test_sandbox_unit.py index a66561b0..93945e63 100644 --- a/tests/test_sandbox_unit.py +++ b/tests/test_sandbox_unit.py @@ -1168,12 +1168,17 @@ def _patched_build(cfg, workspace, owned_ids=None): subagent_mod.build_executor = _patched_build try: # Patch the inner method so the agent loop completes quickly - async def _fast_inner(task_id, task, label, origin, executor): + async def _fast_inner(task_id, task, label, origin, executor, provider, model): await manager._announce_result(task_id, label, task, "done", origin, "ok") manager._run_subagent_inner = _fast_inner await manager._run_subagent( - "t1", "test task", "test", {"channel": "cli", "chat_id": "direct", "session_key": "cli:direct"} + "t1", + "test task", + "test", + {"channel": "cli", "chat_id": "direct", "session_key": "cli:direct"}, + manager.provider, + manager.model, ) finally: subagent_mod.build_executor = original diff --git a/tests/test_subagent_manager.py b/tests/test_subagent_manager.py index 1f65a772..15298c3f 100644 --- a/tests/test_subagent_manager.py +++ b/tests/test_subagent_manager.py @@ -94,7 +94,7 @@ async def _drive(monkeypatch, *, max_concurrent: int, spawn_n: int) -> int: state = {"current": 0, "peak": 0} release = asyncio.Event() - async def _stub_inner(task_id, task, label, origin, executor) -> None: + async def _stub_inner(task_id, task, label, origin, executor, provider, model) -> None: state["current"] += 1 state["peak"] = max(state["peak"], state["current"]) await release.wait() @@ -146,6 +146,8 @@ async def _capture_announcement(task_id, label, task, result, origin, status) -> "delete", {"channel": "tui", "chat_id": "default", "session_key": "tui:session-a"}, executor, + manager.provider, + manager.model, ) assert executor.commands == [] @@ -247,7 +249,7 @@ async def test_cancel_by_session_cancels_live_task(monkeypatch): entered = asyncio.Event() release = asyncio.Event() - async def _blocking_inner(task_id, task, label, origin, executor) -> None: + async def _blocking_inner(task_id, task, label, origin, executor, provider, model) -> None: entered.set() await release.wait() # never set — keeps the task live until cancelled From 8a63371d056d5d89a52bc16f2e94ada5cc9faaf2 Mon Sep 17 00:00:00 2001 From: arelchan <204152633+arelchan@users.noreply.github.com> Date: Sat, 8 Aug 2026 22:55:17 +0800 Subject: [PATCH 4/8] docs(agent): scope two rationales to what the code actually does Review found two docstrings of the kind this PR was already rejected for once. The park docstring said a mid-turn split "does not raise"; that holds for the chat_with_retry sites, but _llm_call_stream -- the path a TUI turn takes -- catches only TimeoutError, so there the rejection propagates. And the context-engine ABC justified its concrete no-op by the loop calling it unconditionally, which an abstract method would satisfy equally; what concrete buys is not forcing a future implementation to write an empty override. Co-authored-by: Claude (claude-opus-5) --- raven/agent/loop/main.py | 9 ++++++--- raven/context_engine/base.py | 7 ++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/raven/agent/loop/main.py b/raven/agent/loop/main.py index dc905053..ccaf0b66 100644 --- a/raven/agent/loop/main.py +++ b/raven/agent/loop/main.py @@ -638,9 +638,12 @@ def set_provider(self, provider: LLMProvider, model: str) -> None: applied: the loop reads ``self.provider`` at call time (eight sites in this module, plus the context engine and consolidator underneath), so adopting mid-turn would relay one conversation - across two vendors. That split does not raise -- the provider turns - a rejected request into ``finish_reason="error"`` content, so the - turn reports a failure with no indication that its endpoint moved. + across two vendors. How that surfaces depends on the path: the + ``chat_with_retry`` sites turn a rejected request into + ``finish_reason="error"`` content, so the turn reports a failure + with no sign that its endpoint moved, while ``_llm_call_stream`` + (which a TUI turn takes) catches only ``TimeoutError`` and lets the + rejection propagate. Neither is a diagnosis the user can act on. The park is the second line of defence, not the first: the RPC rejects a switch outright when the caller's own session has a turn diff --git a/raven/context_engine/base.py b/raven/context_engine/base.py index 910776a1..3b8e8722 100644 --- a/raven/context_engine/base.py +++ b/raven/context_engine/base.py @@ -148,9 +148,10 @@ def set_provider(self, provider: "LLMProvider", model: str) -> None: Segments that call an LLM hold the provider handed to them at construction; without this they keep calling the old one for the - rest of the process. Concrete rather than abstract because - ``AgentLoop.context_engine`` is typed to this ABC and the loop calls - this unconditionally -- a no-op default keeps that call total. + rest of the process. Concrete rather than abstract so a future + implementation with no LLM-backed segment is not forced to write an + empty override; ``ContextAssembler`` is the only one today and does + override it. """ @abstractmethod From 3eaf3a0933b62fe57f6fb9208806964561cba750 Mon Sep 17 00:00:00 2001 From: arelchan <204152633+arelchan@users.noreply.github.com> Date: Sun, 9 Aug 2026 12:13:34 +0800 Subject: [PATCH 5/8] test(subagent): cover the window the spawn snapshot exists for The only mutation the review could not kill: moving the snapshot from spawn into _run_subagent_inner left the suite green, which is exactly the state the commit before it was written to fix. Neither existing test could see it -- one stubbed _run_subagent wholesale, so it proved spawn passes a pair but not when the pair is read; the other called _run_subagent_inner directly, bypassing spawn, the concurrency gate and the sandbox boot, so it proved the iteration loop does not re-read but not where the read happens. This drives the real _run_subagent with the gate held shut, switches the provider while the task sits in that window, then releases it and asserts which provider actually served the call. Co-authored-by: Claude (claude-opus-5) --- tests/test_agent_loop_model_switch.py | 60 +++++++++++++++++++-------- 1 file changed, 43 insertions(+), 17 deletions(-) diff --git a/tests/test_agent_loop_model_switch.py b/tests/test_agent_loop_model_switch.py index c4550e68..365ddcd5 100644 --- a/tests/test_agent_loop_model_switch.py +++ b/tests/test_agent_loop_model_switch.py @@ -72,6 +72,12 @@ def is_sandboxed(self) -> bool: async def exec(self, command: str, **kwargs): # pragma: no cover - unused raise NotImplementedError + async def __aenter__(self): + return self + + async def __aexit__(self, *exc): + return False + def _noop_submit(*args, **kwargs) -> None: """``_announce_result`` calls the spine submit without awaiting it.""" @@ -434,24 +440,44 @@ async def _short(*args, **kwargs): @pytest.mark.asyncio async def test_spawn_snapshots_before_the_task_queues(tmp_path) -> None: - """The snapshot is taken in ``spawn``, not where the task starts running: - a spawn waits on the concurrency gate and a sandbox boot first, and a - switch landing in that window would move an endpoint the user asked for - before switching. + """The snapshot must be taken in ``spawn``, not where the task starts + running: a spawn waits on the concurrency gate and a sandbox boot first, + and a switch landing in that window would hand the task an endpoint the + user chose after asking for it. + + Driven through the real ``_run_subagent`` so the window is genuinely + open -- stubbing it would prove only that ``spawn`` passes *a* pair, which + a snapshot taken later would also satisfy. """ - manager = SubagentManager(provider=_Provider("started-with"), workspace=tmp_path, model="started/model") - manager._gate = asyncio.Semaphore(0) # nothing gets past this + import raven.agent.subagent.manager as manager_mod - captured: dict[str, object] = {} + served: list[str] = [] - async def _capture(task_id, task, label, origin, provider, model): - captured["provider"] = provider - captured["model"] = model - - manager._run_subagent = _capture - await manager.spawn("do the thing", label="thing", session_key="s") - manager.set_provider(_Provider("switched-to"), NEW_MODEL) - await asyncio.sleep(0) + class _RecordingProvider(_Provider): + async def chat_with_retry(self, **kwargs) -> LLMResponse: + served.append(self.name) + return LLMResponse(content="done", finish_reason="stop") - assert captured["provider"].name == "started-with" - assert captured["model"] == "started/model" + manager = SubagentManager( + provider=_RecordingProvider("started-with"), + workspace=tmp_path, + model="started/model", + ) + manager._submit = _noop_submit + # Hold every spawn in exactly the window the snapshot exists for. + manager._gate = asyncio.Semaphore(0) + + original_build = manager_mod.build_executor + manager_mod.build_executor = lambda cfg, workspace, owned_ids=None: _StubExecutor() + try: + await manager.spawn("do the thing", label="thing", session_key="s") + manager.set_provider(_RecordingProvider("switched-to"), NEW_MODEL) + manager._gate.release() + for _ in range(50): + await asyncio.sleep(0) + if served: + break + finally: + manager_mod.build_executor = original_build + + assert served == ["started-with"], "the spawn ran on the provider it was asked for" From 68829166d9d58d017cb1d1b96feb3271ea7aac34 Mon Sep 17 00:00:00 2001 From: arelchan <204152633+arelchan@users.noreply.github.com> Date: Sun, 9 Aug 2026 12:19:11 +0800 Subject: [PATCH 6/8] feat(agent): bind the model to the conversation, not to the process The model was two attributes on a process-wide loop, so there was one answer for everyone: two sessions could not run different models, a switch in one moved all of them, and the "default" was whatever the last switch happened to write. This makes the model a property of the conversation. A ModelBinding is a model id and the credential that serves it, as one value. ProviderPool is the single place a model id becomes such a pair, caching per (vendor, model) because building one imports LiteLLM and writes its vendor's key into the environment. run_turn resolves the binding for the turn's session and holds it in a context var for the whole turn tree; the loop's provider/model, the context engine's LLM-backed segments, the skill gate and rewriter and the consolidator all read that instead of a reference of their own. What that buys, rule by rule: - Different sessions on different models, and a switch that moves only the session that asked: a dict of overrides, read at turn entry. - A new session on the configured default: a session-scoped switch does not write agents.defaults, so nothing accumulates. config.set model takes a scope, session (what the picker sends) or default, and /model --default is the counterpart in the TUI. The session's choice is stored on its own record and restored on resume, so it outlives the process without becoming everyone's default. - A subsystem with a model and credentials of its own uses them, otherwise it follows the conversation: the factory resolves each pin through the pool, so a holder has either a complete pair or nothing. A gateway binds a pin through itself, since it serves any id under its own key. - A switch mid-turn landing on the next turn: free. The turn holds the binding it entered on, so a later switch is not visible to it. The client-side refusal that used to pre-empt this is gone, and so is ModelSwitchInTurnError across the Python errors module, the TypeScript client, the OpenRPC schema and the code-table test. Detached work inherits the context copy asyncio makes at task creation, so a spawned subagent finishes on the model it was spawned under; spawn also passes the pair explicitly, because a subagent outlives its turn and that is worth being able to read in the code. The picker and session.info now report the session's own model rather than agents.defaults, which otherwise showed two models for one conversation. No subsystem ships a vendor default any more. context.curator_model, token_wise.tool_result_lifecycle.summary_model and skill_forge.detect_model all hardcoded the same Gemini id and token_wise.smart_routing.tiers shipped six models across three vendors, for users who may hold no key for any of them. All four are unset now, which is what "not configured" has to mean for the rule above to be expressible. Only curator_model has readers today; the other three are dead config, emptied for consistency. The media tools keep their defaults, in the tool code rather than the schema: they are capability-bound, and no conversation model generates images or speech. Note for the release: with context.curator_model unset, the Curator's slow path runs on the conversation's model instead of failing on a Gemini id nobody had a key for and dropping to the deterministic plan. That is the rule working as asked, but on a long conversation it is up to 12 tool-calling requests per turn of context housekeeping that previously cost nothing. Set context.curator_model to a small model, and configure that vendor's key, to keep it cheap. Review of this branch by four agents in isolated worktrees found the persistence half-built (the write to the session record had no reader, so a switch died with the process while the code said otherwise), the provider pool handed a config snapshot at all three construction sites so the freshness it documents never fired, and the picker overriding its selection for every session because session_model falls back to the default and so never answers None. All are fixed here, along with the gateway pin escaping the factory's guard, a deleted session leaving its override behind, and a fork dropping its parent's model. On the TUI side two existing tests asserted the config.set params by exact equality and would have gone red in CI on the new scope key; a default-scoped switch painted the new default into the status bar while the session kept its own model; and --default is now stripped in any position and however many times, with /model --default alone opening the picker instead of sending an empty model id. The mutation pass left seven holes green, all the same shape: a helper tested directly while the handler or registration calling it was not. Each now fails a test when broken, including the window the spawn snapshot exists for -- the concurrency gate and sandbox boot a spawn waits through before its task starts running. Co-authored-by: Claude (claude-opus-5) --- CONTEXT.md | 35 ++ docs/Raven-vs-OpenClaw-Hermes.md | 2 +- raven/agent/loop/main.py | 226 ++++--- raven/agent/subagent/manager.py | 36 +- raven/cli/agent_commands.py | 3 + raven/cli/gateway_commands.py | 3 + raven/cli/tui_commands.py | 3 + raven/config/raven.py | 31 +- raven/context_engine/curator.py | 17 +- raven/context_engine/factory.py | 12 + raven/context_engine/history_trimmer.py | 17 +- raven/context_engine/segments/curator.py | 64 +- .../memory_engine/consolidate/consolidator.py | 17 +- raven/memory_engine/skill_forge/gate.py | 58 +- raven/memory_engine/skill_forge/rewriter.py | 26 +- raven/providers/binding.py | 84 +++ raven/providers/pool.py | 171 ++++++ raven/tui_rpc/errors.py | 8 - raven/tui_rpc/methods/__init__.py | 2 +- raven/tui_rpc/methods/config.py | 105 +++- raven/tui_rpc/methods/model.py | 39 +- raven/tui_rpc/methods/session.py | 50 +- raven/tui_rpc/models.py | 9 + tests/test_agent_loop_model_switch.py | 331 +---------- tests/test_agent_loop_session_model.py | 559 ++++++++++++++++++ tests/test_provider_pool.py | 360 +++++++++++ tests/test_read_file_image.py | 7 +- tests/test_rpc_schema_match.py | 1 - tests/test_tui_rpc_config.py | 192 +++++- tests/test_tui_rpc_model.py | 39 ++ tests/test_tui_rpc_session.py | 80 +++ ui-tui/CONTEXT.md | 8 + ui-tui/rpc-schema/openrpc.json | 27 +- .../src/__tests__/createSlashHandler.test.ts | 47 ++ ui-tui/src/app/slash/commands/session.ts | 36 +- ui-tui/src/components/modelPicker.tsx | 2 +- ui-tui/src/rpc/errors.ts | 7 - ui-tui/src/rpc/generated.ts | 6 + ui-tui/src/rpc/index.ts | 1 - 39 files changed, 2156 insertions(+), 565 deletions(-) create mode 100644 raven/providers/binding.py create mode 100644 raven/providers/pool.py create mode 100644 tests/test_agent_loop_session_model.py create mode 100644 tests/test_provider_pool.py diff --git a/CONTEXT.md b/CONTEXT.md index eb16c695..90fdddc1 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -24,6 +24,41 @@ The bare `chat_id` value shown to and accepted from users (the channel prefix is stripped for display, re-prepended to form the session key). Presentation term; in code the value lives in the `chat_id` field and the composite is the `session_key`. +**Model binding**: +A model id together with the provider whose credential serves it, as one value +(`raven/providers/binding.py`). The pairing is the point: a model id alone does +not say which key reaches it, and updating one half is how one vendor's key ends +up on another vendor's endpoint. A turn resolves its binding once at `run_turn` +entry and holds it in a context var for the whole turn tree, so everything under +that turn -- the loop, the context engine's LLM-backed segments, the skill gate +and rewriter, the consolidator, and any task the turn detaches -- reads the same +pair. _Avoid_: "the current model" / "the active provider" for this; both name +one half. + +**Session binding**: +The model binding one conversation runs on. Sessions that never switched have no +entry and resolve to the **default binding**; a switch writes only that +session's entry, so it moves no other conversation and does not change what a +new one starts on. Stored on the session record so it survives a restart. + +**Default binding**: +What a session with no binding of its own runs on: `agents.defaults` from config, +verbatim. Changed by a `scope="default"` switch, which leaves sessions that +already chose their own model where they are. + +**Provider pool**: +The one place a model id becomes a model binding (`raven/providers/pool.py`), +caching a provider per (vendor, model) and dropping the cache when the +credentials behind it change. Also what turns a **subsystem pin** into a pair. + +**Subsystem pin**: +A model configured for one subsystem rather than for the conversation +(`context.curator_model`, `skill_forge.llm_gate_model`). A pin is only honoured +when the pool can pair it with credentials of its own, or when a gateway is +serving it; otherwise the subsystem follows the conversation's model, because a +bare pinned id sent on the conversation's key is exactly the mis-pairing above. +Unset out of the box -- no subsystem ships a vendor default. + **Turn**: One complete agent reaction: from an inbound message entering the agent loop to the agent's final response, including every LLM call and tool execution in between. diff --git a/docs/Raven-vs-OpenClaw-Hermes.md b/docs/Raven-vs-OpenClaw-Hermes.md index 8d54d53e..2132a6e0 100644 --- a/docs/Raven-vs-OpenClaw-Hermes.md +++ b/docs/Raven-vs-OpenClaw-Hermes.md @@ -34,7 +34,7 @@ **Hermes 的做法**：`ContextCompressor` — 一套固定的 4 阶段（裁剪工具输出 → 保护边界 → 中段摘要 → 增量更新）。触发式、被动、不可恢复。 -**我们的做法**：Curator 是一个**独立的小模型 Agent**（默认 gemini-2.5-flash），有自己的 11 个内部工具： +**我们的做法**：Curator 是一个**独立的小模型 Agent**（默认跟随对话所用的模型；可通过 context.curator_model 单独指定一个更小更快的模型），有自己的 11 个内部工具： ``` curator_check_budget — 理解当前 token 压力 diff --git a/raven/agent/loop/main.py b/raven/agent/loop/main.py index ccaf0b66..b1d9591b 100644 --- a/raven/agent/loop/main.py +++ b/raven/agent/loop/main.py @@ -44,6 +44,7 @@ from raven.memory_engine.base import TokenBudget from raven.memory_engine.consolidate.consolidator import MemoryConsolidator, MemoryStore from raven.providers.base import LLMProvider, LLMResponse, ToolCallRequest +from raven.providers.binding import ModelBinding, active_binding, use_binding from raven.providers.capabilities import image_placeholder_text, supports_image_tool_result from raven.sandbox import SandboxConfig, SandboxExecutor, SandboxInitError, build_executor from raven.session.manager import Session, SessionManager @@ -79,6 +80,7 @@ from raven.context_engine import ContextEngine from raven.memory_engine.backend import MemoryBackend from raven.proactive_engine.schedulers.cron.service import CronService + from raven.providers.pool import ProviderPool from raven.routing.router import ModelRouter from raven.sandbox.debug_server import SandboxDebugServer from raven.skill_hub import SkillHubClient @@ -310,6 +312,7 @@ def __init__( now_fn: Callable | None = None, context_config: "ContextConfig | None" = None, runtime_config: "RuntimeConfig | None" = None, + provider_pool: "ProviderPool | None" = None, interactive: bool = True, jina_api_key: str | None = None, max_concurrent_subagents: int = 4, @@ -365,9 +368,14 @@ def __init__( # Returning None means "fall through to normal flow". self.decision_consumer = decision_consumer self.channels_config = channels_config - self.provider = provider self.workspace = workspace - self.model = model or provider.get_default_model() + # The model a turn runs on is per session, so it cannot live in two + # attributes on a process-wide loop. ``_default_binding`` is what a + # session starts on; ``provider``/``model`` below read whichever + # binding the running turn entered. + self._provider_pool = provider_pool + self._default_binding = ModelBinding(provider, model or provider.get_default_model()) + self._session_bindings: dict[str, ModelBinding] = {} # Resolved lazily on the first tool result that carries an image. Keyed # by model, not a single flag: the loop is a long-lived singleton and # takes a per-call model (strategies rewrite it, and the model chain @@ -462,7 +470,7 @@ def __init__( config=context_config, builder=self.context, provider=provider, - model=self.model, + model=self._default_binding.model, context_window_tokens=context_window_tokens, get_tool_definitions=self.tools.get_definitions, now_fn=now_fn, @@ -473,6 +481,7 @@ def __init__( skill_forge_router_config=skill_forge_router_config, skill_forge_config=skill_forge_config, skill_hub_client=self._skill_hub_client, + provider_pool=provider_pool, ) # Runtime discipline (5th pillar). Bug2 uses ``runtime.checkpoint``; @@ -508,7 +517,7 @@ def __init__( self.subagents = SubagentManager( provider=provider, workspace=workspace, - model=self.model, + model=self._default_binding.model, brave_api_key=brave_api_key, jina_api_key=jina_api_key, web_proxy=web_proxy, @@ -543,7 +552,7 @@ def __init__( self.memory_consolidator = MemoryConsolidator( workspace=workspace, provider=provider, - model=self.model, + model=self._default_binding.model, sessions=self.sessions, context_window_tokens=context_window_tokens, build_messages=self.context.build_messages, @@ -553,20 +562,12 @@ def __init__( self._consolidation_tasks: set[asyncio.Task] = set() - # A switch that arrives mid-turn is parked here until no turn is - # running, so a turn in flight finishes on the provider it started - # with. A depth counter, not a flag: OriginPools gates USER and - # system origins on independent semaphores with no global cap - # (spine/scheduler.py), so a user turn and a cron turn overlap on - # this loop under the TUI defaults. - self._pending_provider: tuple[LLMProvider, str] | None = None - self._turns_in_flight = 0 - # ``self.subagents``, ``self.context_engine`` and # ``self.memory_consolidator`` were each handed ``provider`` earlier in - # this constructor and hold their own reference; ``_adopt_provider`` is - # what keeps them from outliving a live model switch. Add the call - # there when adding another holder. + # this constructor. Inside a turn they read the turn's binding; the + # reference they hold is only the fallback for work that runs outside + # one, and ``set_default_binding`` is what keeps that fallback current. + # Add the call there when adding another holder. # Phase B-3: the L4 facade (``DefaultMemoryEngine`` / # ``MemoryEngine`` ABC) has been retired. AgentLoop now holds @@ -622,61 +623,106 @@ def _apply_disabled_tools(self) -> None: if self.tools.has(name): self.tools.unregister(name) - def set_provider(self, provider: LLMProvider, model: str) -> None: - """Point the loop and everything it built at a new provider/model. - - ``config.set model`` builds a provider from the prospective config - and hands it here. Assigning ``self.provider`` alone is not enough: - the subagent manager, the context engine's LLM-backed segments and - the consolidator each captured the provider handed to them in - ``__init__``. Left behind, they keep calling the old endpoint for - the rest of the process -- which is how switching away from a dead - credential fixed the main loop while subagents and the skill - rewriter/gate went on failing to authenticate. - - A switch that lands while any turn is running is parked rather than - applied: the loop reads ``self.provider`` at call time (eight sites - in this module, plus the context engine and consolidator - underneath), so adopting mid-turn would relay one conversation - across two vendors. How that surfaces depends on the path: the - ``chat_with_retry`` sites turn a rejected request into - ``finish_reason="error"`` content, so the turn reports a failure - with no sign that its endpoint moved, while ``_llm_call_stream`` - (which a TUI turn takes) catches only ``TimeoutError`` and lets the - rejection propagate. Neither is a diagnosis the user can act on. - - The park is the second line of defence, not the first: the RPC - rejects a switch outright when the caller's own session has a turn - in flight (``is_turn_active`` in ``tui_rpc.methods.config``). This - covers what that guard cannot see -- a caller that passes no - ``session_id``, and the proactive turns that run in their own lanes. - Note the RPC still answers ``applied: True`` and the config file is - already written, so a parked switch is applied on disk while the - loop reports the old model until the last turn drains. - - Detached subagents are not covered by that park -- they outlive the - turn that spawned them -- so ``SubagentManager`` snapshots instead. + @property + def provider(self) -> LLMProvider: + """The provider of the binding the running turn entered. + + A property, not an attribute: the model is per session now, so there + is no single answer to cache on the loop. Outside a turn (startup, a + one-shot CLI call) this is the configured default. """ - if self._turns_in_flight: - self._pending_provider = (provider, model) - return - self._adopt_provider(provider, model) - - def _adopt_provider(self, provider: LLMProvider, model: str) -> None: - """Hand a provider to the loop and every subsystem holding the old one.""" - self.provider = provider - self.model = model - self.subagents.set_provider(provider, model) - self.context_engine.set_provider(provider, model) - self.memory_consolidator.set_provider(provider, model) - - def _adopt_pending_provider(self) -> None: - """Apply a parked switch. Callers must check that no turn is running.""" - pending = self._pending_provider - if pending is None: + binding = active_binding() + return binding.provider if binding is not None else self._default_binding.provider + + @property + def model(self) -> str: + """The model id of the binding the running turn entered.""" + binding = active_binding() + return binding.model if binding is not None else self._default_binding.model + + @property + def provider_pool(self) -> "ProviderPool | None": + """Where a model id becomes a model id plus the credential for it.""" + return self._provider_pool + + @property + def default_binding(self) -> ModelBinding: + """What a session with no switch of its own runs on.""" + return self._default_binding + + def binding_for_session(self, session_key: str) -> ModelBinding: + """The binding this session runs on: its own switch, else the default. + + A new session has no entry, so it starts on the configured default + rather than on whatever the last session switched to. + """ + return self._session_bindings.get(session_key, self._default_binding) + + def session_model(self, session_key: str) -> str: + """What to show this session's user, which is not the global default.""" + return self.binding_for_session(session_key).model + + def has_session_binding(self, session_key: str) -> bool: + """Did this session switch, or is it just following the default? + + ``session_model`` cannot answer that -- it falls back to the default, + so it never returns None. Callers that must distinguish "chose this" + from "inherited this" ask here. + """ + return session_key in self._session_bindings + + def restore_session_model(self, session_key: str, model: str, provider_name: str | None = None) -> None: + """Put a resumed session back on the model it was last switched to. + + Session overrides live in memory, so without this a restart moves every + switched session back to the default and the user's choice lasts + exactly as long as the process. The model comes from the session + record. A model that can no longer be built (a credential since + removed) leaves the session on the default rather than failing the + resume. + """ + pool = self._provider_pool + if pool is None or not model: return - self._pending_provider = None - self._adopt_provider(*pending) + try: + self.set_session_binding(session_key, pool.bind(model, provider_name)) + except (SystemExit, RuntimeError, ValueError) as exc: + logger.warning("session {!r} cannot resume on {!r} ({}); using the default", session_key, model, exc) + + def set_session_binding(self, session_key: str, binding: ModelBinding) -> None: + """Switch one session, leaving every other session where it was. + + Applied immediately and still safe mid-turn: a turn resolves its + binding once at ``run_turn`` entry and holds it in a context var for + its whole tree, including anything it detaches. So a switch during a + turn cannot move that turn -- it lands on the next one -- and no + parking is needed to arrange that. + """ + self._session_bindings[session_key] = binding + + def clear_session_binding(self, session_key: str) -> None: + """Drop a session's override so it follows the default again.""" + self._session_bindings.pop(session_key, None) + + def set_default_binding(self, binding: ModelBinding) -> None: + """Change what new sessions start on. + + Sessions that already switched keep their own binding; sessions that + never did pick this up on their next turn. Subsystem fallbacks are + re-pointed too, for the paths that run outside a turn and therefore + have no binding to read. + """ + self._default_binding = binding + self.subagents.set_provider(binding.provider, binding.model) + self.context_engine.set_provider(binding.provider, binding.model) + self.memory_consolidator.set_provider(binding.provider, binding.model) + + def set_provider(self, provider: LLMProvider, model: str) -> None: + """Change the default binding. Kept for callers that are not + session-aware (the gateway, a CLI one-shot); the session-scoped path + is ``set_session_binding``. + """ + self.set_default_binding(ModelBinding(provider, model)) def configure_personalization(self, enable: bool) -> None: """Global switch for the 4-step personalization flow (PAHF-inspired). @@ -2630,27 +2676,23 @@ async def run_turn( usage_sink: dict[str, Any] | None = None, text_sink: dict[str, Any] | None = None, ) -> TurnOutcome: - """Turn boundary for a live ``/model`` switch; see ``_run_turn``. - - A parked switch is adopted here, before the turn reads - ``self.provider`` for the first time, and the count kept for the - duration is what parks the next one. Wrapping rather than - snapshotting because the provider is read from ``self`` at eight - sites in this module and by the context engine and consolidator - underneath them -- one boundary covers all of them, a snapshot - would have to be threaded through each. - - Both ends gate on zero, because turns overlap: a user turn and a - proactive turn hold slots in separate pools. Adopting on the way in - would otherwise land a switch parked for a turn that is still - running, and clearing a flag on the way out would unpark it just as - wrongly. Adopting again on the last exit is what keeps a park from - outliving the turns it was waiting on. + """Bind the turn to its session's model; see ``_run_turn`` for the turn. + + This is where a session's model becomes the one thing everything under + the turn reads: the loop's own ``provider``/``model``, the context + engine's LLM-backed segments, the skill gate and rewriter, the + consolidator, and anything the turn detaches (a subagent inherits the + context it was created in). + + Resolving once here is also what makes a mid-turn switch harmless + without any parking. The binding is captured before the first read and + held for the tree, so a switch that lands while this turn runs is + simply not visible to it -- it takes effect on the session's next + turn. Turns from other sessions run under their own binding + concurrently, which is the point. """ - if self._turns_in_flight == 0: - self._adopt_pending_provider() - self._turns_in_flight += 1 - try: + session_key = req.conversation or f"{req.source.channel}:{req.source.chat_id}" + with use_binding(self.binding_for_session(session_key)): return await self._run_turn( req, emit, @@ -2660,10 +2702,6 @@ async def run_turn( usage_sink=usage_sink, text_sink=text_sink, ) - finally: - self._turns_in_flight -= 1 - if self._turns_in_flight == 0: - self._adopt_pending_provider() async def _run_turn( self, diff --git a/raven/agent/subagent/manager.py b/raven/agent/subagent/manager.py index 3e6ef382..bdfbf4ec 100644 --- a/raven/agent/subagent/manager.py +++ b/raven/agent/subagent/manager.py @@ -16,6 +16,7 @@ from raven.agent.tools.web import WebFetchTool, WebSearchTool from raven.config.schema import ExecToolConfig from raven.providers.base import LLMProvider +from raven.providers.binding import ModelBinding, resolve from raven.sandbox import SandboxConfig, build_executor from raven.security.trust import wrap_untrusted from raven.tracing import semconv, trace @@ -50,7 +51,6 @@ def __init__( ): from raven.config.schema import ExecToolConfig - self.provider = provider self.workspace = workspace # Spine submit, late-bound (the scheduler pins its home loop at # construction and is built inside each entry point's run loop; this @@ -58,7 +58,7 @@ def __init__( # set_submit before any announce; the result re-injection submits a # SUBAGENT-origin turn. self._submit = None - self.model = model or provider.get_default_model() + self._fallback = ModelBinding(provider, model or provider.get_default_model()) self.brave_api_key = brave_api_key self.jina_api_key = jina_api_key self.web_proxy = web_proxy @@ -78,15 +78,19 @@ def __init__( def set_provider(self, provider: LLMProvider, model: str) -> None: """Adopt the provider a live ``/model`` switch just built. - Subagents run on the parent's provider, so a switch that is not - propagated here leaves every spawn calling the credential the loop - has already abandoned. Only spawns requested after this call are - affected: a subagent is a detached task that outlives the turn that - spawned it, so the loop's park cannot cover it and ``spawn`` - snapshots the pair it was asked for. + Only the out-of-turn fallback moves. A spawn requested during a turn + takes that turn's binding, so a subagent follows the conversation + that asked for it rather than whatever this manager was built with. """ - self.provider = provider - self.model = model + self._fallback = ModelBinding(provider, model) + + @property + def provider(self) -> LLMProvider: + return resolve(None, self._fallback).provider + + @property + def model(self) -> str: + return resolve(None, self._fallback).model async def spawn( self, @@ -120,12 +124,14 @@ async def spawn( display_label = label or task[:30] + ("..." if len(task) > 30 else "") origin = {"channel": origin_channel, "chat_id": origin_chat_id, "session_key": quota_key} - # Snapshot here rather than where the task starts running: it queues - # behind the concurrency gate and a sandbox boot first, and a switch - # landing in that window would hand this task an endpoint the user - # chose after asking for it. + # The binding of the turn that asked for this spawn, snapshotted here + # rather than where the task starts running: it queues behind the + # concurrency gate and a sandbox boot first, and a switch landing in + # that window would hand it an endpoint chosen after it was asked for. + # A subagent has no model of its own, so it follows its conversation. + binding = resolve(None, self._fallback) bg_task = asyncio.create_task( - self._run_subagent(task_id, task, display_label, origin, self.provider, self.model) + self._run_subagent(task_id, task, display_label, origin, binding.provider, binding.model) ) self._running_tasks[task_id] = bg_task if session_key: diff --git a/raven/cli/agent_commands.py b/raven/cli/agent_commands.py index 715c64f4..c4127607 100644 --- a/raven/cli/agent_commands.py +++ b/raven/cli/agent_commands.py @@ -320,7 +320,10 @@ def agent( registry=plugin_registry, ) + from raven.providers.pool import ProviderPool + agent_loop = AgentLoop( + provider_pool=ProviderPool(lambda: load_runtime_config(None, None)), provider=provider, now_fn=parse_fake_now(fake_now), workspace=config.workspace_path, diff --git a/raven/cli/gateway_commands.py b/raven/cli/gateway_commands.py index 9edfccca..2d77372a 100644 --- a/raven/cli/gateway_commands.py +++ b/raven/cli/gateway_commands.py @@ -222,7 +222,10 @@ def gateway( backend = maybe_build_memory_backend(config.workspace_path, ec_config) # Create agent with cron service + from raven.providers.pool import ProviderPool + agent = AgentLoop( + provider_pool=ProviderPool(lambda: load_runtime_config(None, None)), provider=provider, now_fn=parse_fake_now(fake_now), workspace=config.workspace_path, diff --git a/raven/cli/tui_commands.py b/raven/cli/tui_commands.py index caf78ee0..79aea73b 100644 --- a/raven/cli/tui_commands.py +++ b/raven/cli/tui_commands.py @@ -438,7 +438,10 @@ def _build_tui_agent_loop(): registry=plugin_registry, ) + from raven.providers.pool import ProviderPool + agent_loop = AgentLoop( + provider_pool=ProviderPool(lambda: load_runtime_config(None, None)), provider=provider, workspace=config.workspace_path, model=config.agents.defaults.model, diff --git a/raven/config/raven.py b/raven/config/raven.py index 07f160da..cab115ec 100644 --- a/raven/config/raven.py +++ b/raven/config/raven.py @@ -74,8 +74,17 @@ class ContextConfig(_Base): fast_path_threshold: float = 0.60 """Curator Fast Path cutoff. Below this % of budget → zero-LLM pass-through.""" - curator_model: str = "gemini-2.5-flash" - """Model used by the Curator agent loop (Slow Path). Kept small & fast.""" + curator_model: str | None = None + """Model for the Curator agent loop (Slow Path). Unset means the Curator + runs on the model of the conversation it is curating. + + Worth setting, and worth setting to something small: one slow-path pass is + a bounded agent loop of up to 12 tool-calling requests, so it is per-turn + housekeeping rather than an answer, and unset means a long conversation + pays conversation-model prices for it. A vendor id here needs that + vendor's credentials in ``providers``: a model id without a key of its own + is not a configured subsystem, and the Curator falls back to the + conversation rather than send that id on the conversation's key.""" curator_timeout_seconds: float = 30.0 """Max wall time for one Curator slow-path invocation before fallback.""" @@ -668,13 +677,12 @@ class SmartRoutingConfig(_Base): """SmartRouter configuration.""" enabled: bool = False - tiers: dict[str, list[str]] = Field( - default_factory=lambda: { - "light": ["gemini-2.5-flash", "claude-haiku-4-5"], - "medium": ["claude-sonnet-4-6", "gpt-4.1-mini"], - "heavy": ["claude-opus-4-6", "gpt-4.1"], - } - ) + tiers: dict[str, list[str]] = Field(default_factory=dict) + """Which models each tier may route to. Empty out of the box: the table + this replaced named six models across three vendors, for users who may + hold no key for any of them, and routing has no meaning without models to + choose between -- so enabling this means listing your own.""" + default_tier: Literal["light", "medium", "heavy"] = "heavy" """Fallback tier when routing is uncertain — conservative default.""" @@ -686,7 +694,8 @@ class ToolResultLifecycleConfig(_Base): full_retention_turns: int = 3 summary_retention_turns: int = 10 placeholder_text: str = "[Tool result archived — retrievable via Curator]" - summary_model: str = "gemini-2.5-flash" + summary_model: str | None = None + """Unset means the conversation's own model. See ``curator_model``.""" class TokenWiseConfig(_Base): @@ -1003,7 +1012,7 @@ class SkillForgeConfig(_Base): rewrites — e.g. ``"claude-opus-4-6"``.""" # --- Detect / extraction gating (wired into everos) --- - detect_model: str = "gemini-2.5-flash" + detect_model: str | None = None """LLM used for the cheap per-turn classification work — today that's the everos boundary detector (multi-turn task split). A smaller / faster model than ``evolve_model`` is intentional: boundary diff --git a/raven/context_engine/curator.py b/raven/context_engine/curator.py index 732ee683..26cda66f 100644 --- a/raven/context_engine/curator.py +++ b/raven/context_engine/curator.py @@ -24,6 +24,7 @@ from raven.memory_engine.base import AssembledContext, TokenBudget from raven.memory_engine.consolidate.consolidator import MemoryStore from raven.providers.base import LLMProvider +from raven.providers.binding import ModelBinding, resolve from raven.utils.helpers import ( ensure_dir, estimate_message_tokens, @@ -338,8 +339,7 @@ def __init__( get_tool_definitions: Callable[[], list[dict[str, Any]]], context_window_tokens: int, ): - self.provider = provider - self.model = model + self._fallback = ModelBinding(provider, model) self.get_tool_definitions = get_tool_definitions self.context_window_tokens = context_window_tokens self.trimmer = HistoryTrimmer( @@ -352,10 +352,19 @@ def __init__( # CuratorSegmentBuilder before any build/validate call. self.prefix: "AssembledPrefix | None" = None + @property + def provider(self) -> "LLMProvider": + """The provider of the turn's binding; the build-time one outside a turn.""" + return resolve(None, self._fallback).provider + + @property + def model(self) -> str: + """The model of the turn's binding; the build-time one outside a turn.""" + return resolve(None, self._fallback).model + def set_provider(self, provider: LLMProvider, model: str) -> None: """Adopt the provider a live ``/model`` switch just built.""" - self.provider = provider - self.model = model + self._fallback = ModelBinding(provider, model) self.trimmer.set_provider(provider, model) @staticmethod diff --git a/raven/context_engine/factory.py b/raven/context_engine/factory.py index 11f522d3..87198960 100644 --- a/raven/context_engine/factory.py +++ b/raven/context_engine/factory.py @@ -59,6 +59,7 @@ QueryRewriter, SkillForgeRouter, ) + from raven.providers.pool import ProviderPool from raven.skill_hub import SkillHubClient @@ -77,9 +78,14 @@ def build_context_engine( skill_forge_router_config: "SkillForgeRouterConfig | None" = None, skill_forge_config: "SkillForgeConfig | None" = None, skill_hub_client: "SkillHubClient | None" = None, + provider_pool: "ProviderPool | None" = None, ) -> ContextEngine: """Build the one :class:`ContextAssembler` from a flat SegmentBuilder list. + ``provider_pool``, when supplied, is what turns a subsystem's pinned model + into a pinned model *and its own credential*. Without it a pin has no + credential of its own and the subsystem follows the conversation's model. + ``config.engine`` is no longer a dispatch key — there is a single engine. The field is retained in :class:`ContextConfig` for config back-compat but is ignored here. ``builder`` is used only as the @@ -108,6 +114,7 @@ def build_context_engine( rewriter, gate = _build_rewriter_and_gate( provider=provider, + provider_pool=provider_pool, skill_forge_config=skill_forge_config, skill_forge_router_config=skill_forge_router_config, ) @@ -134,6 +141,7 @@ def build_context_engine( get_tool_definitions=get_tool_definitions, ), CuratorSegmentBuilder( + pin=provider_pool.bind_pin(config.curator_model) if provider_pool else None, workspace=workspace, config=config, provider=provider, @@ -218,6 +226,7 @@ def _build_rewriter_and_gate( provider: LLMProvider, skill_forge_config: "SkillForgeConfig | None", skill_forge_router_config: "SkillForgeRouterConfig", + provider_pool: "ProviderPool | None" = None, ) -> "tuple[QueryRewriter | None, LLMGateFilter | None]": """Construct the optional rewriter + gate from the parent SkillForge config. Both fall to ``None`` when their respective flag is off or @@ -249,6 +258,9 @@ def _build_rewriter_and_gate( max_select=int(getattr(skill_forge_config, "llm_gate_max_select", 2) or 2), legacy_top_k=int(skill_forge_router_config.top_k or 5), model=getattr(skill_forge_config, "llm_gate_model", None) or None, + pin=( + provider_pool.bind_pin(getattr(skill_forge_config, "llm_gate_model", None)) if provider_pool else None + ), temperature=float(getattr(skill_forge_config, "llm_gate_temperature", 0.0)), max_tokens=int(getattr(skill_forge_config, "llm_gate_max_tokens", 8192) or 8192), ) diff --git a/raven/context_engine/history_trimmer.py b/raven/context_engine/history_trimmer.py index b4a62cac..b9d592d2 100644 --- a/raven/context_engine/history_trimmer.py +++ b/raven/context_engine/history_trimmer.py @@ -27,6 +27,7 @@ from typing import Any, Callable from raven.providers.base import LLMProvider +from raven.providers.binding import ModelBinding, resolve from raven.utils.helpers import estimate_prompt_tokens_chain # Provider-safe message keys. Anything else on a session message @@ -75,16 +76,24 @@ def __init__( get_tool_definitions: Callable[[], list[dict[str, Any]]], context_window_tokens: int, ) -> None: - self.provider = provider - self.model = model + self._fallback = ModelBinding(provider, model) self.get_tool_definitions = get_tool_definitions self.context_window_tokens = context_window_tokens + @property + def provider(self) -> "LLMProvider": + """The provider of the turn's binding; the build-time one outside a turn.""" + return resolve(None, self._fallback).provider + + @property + def model(self) -> str: + """The model of the turn's binding; the build-time one outside a turn.""" + return resolve(None, self._fallback).model + def set_provider(self, provider: LLMProvider, model: str) -> None: """Adopt the provider a live ``/model`` switch just built, so token estimates keep matching the model actually being called.""" - self.provider = provider - self.model = model + self._fallback = ModelBinding(provider, model) # ------------------------------------------------------------------ # Pure history-shaping helpers (no token estimation / no I/O) diff --git a/raven/context_engine/segments/curator.py b/raven/context_engine/segments/curator.py index 1c2055d7..764660aa 100644 --- a/raven/context_engine/segments/curator.py +++ b/raven/context_engine/segments/curator.py @@ -44,6 +44,7 @@ ) from raven.memory_engine.consolidate.consolidator import MemoryStore from raven.providers.base import LLMProvider +from raven.providers.binding import ModelBinding, resolve from raven.tracing import semconv, trace @@ -64,12 +65,17 @@ def __init__( get_tool_definitions: Callable[[], list[dict[str, Any]]], now_fn: Callable[[], datetime] | None = None, max_steps: int = 12, + pin: "ModelBinding | None" = None, ) -> None: self.workspace = workspace self.config = config - self.provider = provider - self.model = model - self.curator_model = config.curator_model or model + self._fallback = ModelBinding(provider, model) + # ``context.curator_model`` paired with its own credential, or None + # when it names a vendor Raven has no credentials for. None means the + # curator runs on the conversation's model, which is the configured + # rule for an unconfigured subsystem. + self._pin = pin + self._pin_warned = False self.context_window_tokens = context_window_tokens self.get_tool_definitions = get_tool_definitions self.max_steps = max_steps @@ -85,17 +91,48 @@ def __init__( def set_provider(self, provider: LLMProvider, model: str) -> None: """Adopt the provider a live ``/model`` switch just built. - ``curator_model`` is re-derived with the constructor's own - expression, so a switch cannot make it mean something it did not - mean at build time. The default is non-empty, so in practice it is - a pin; an explicitly empty ``context.curator_model`` is the one - config that follows the agent model, and it follows it here too. + Only the out-of-turn fallback moves. Which model the curator runs on + is decided per call by ``_curator_binding``, so a session switching + models is already covered without touching anything here. """ - self.provider = provider - self.model = model - self.curator_model = self.config.curator_model or model + self._fallback = ModelBinding(provider, model) self.assembler.set_provider(provider, model) + @property + def provider(self) -> LLMProvider: + return resolve(None, self._fallback).provider + + @property + def model(self) -> str: + return resolve(None, self._fallback).model + + @property + def curator_model(self) -> str: + """What the slow path is actually called with.""" + return self._curator_binding().model + + def _curator_binding(self) -> ModelBinding: + """Its own pinned pair if it has one, else the conversation's model. + + Unset out of the box, so the default is to follow the conversation. + + A pin becomes a pair only when the factory was given a + :class:`~raven.providers.pool.ProviderPool` and that pool could build + the pin's vendor from configured credentials. Either half missing + leaves ``_pin`` None and the curator follows the conversation, which is + reported once. Worth configuring properly: the slow path is a bounded + loop of up to ``max_steps`` tool-calling requests, which is per-turn + housekeeping, not an answer. + """ + if self.config.curator_model and self._pin is None and not self._pin_warned: + self._pin_warned = True + logger.warning( + "context.curator_model={!r} has no usable credentials of its own; " + "the curator follows the conversation's model instead", + self.config.curator_model, + ) + return resolve(self._pin, self._fallback) + async def build(self, ctx: AssemblyContext) -> Segment | None: if ctx.prefix is None: raise RuntimeError("CuratorSegmentBuilder requires ctx.prefix (phase B)") @@ -207,10 +244,11 @@ async def _slow_path(self, state: CuratorState, turn_id: str) -> Segment | None: "tools": registry.tool_names, }, ) - response = await self.provider.chat_with_retry( + binding = self._curator_binding() + response = await binding.provider.chat_with_retry( messages=messages, tools=registry.get_definitions(), - model=self.curator_model, + model=binding.model, max_tokens=2048, temperature=0.1, ) diff --git a/raven/memory_engine/consolidate/consolidator.py b/raven/memory_engine/consolidate/consolidator.py index 1a9daa32..305e7098 100644 --- a/raven/memory_engine/consolidate/consolidator.py +++ b/raven/memory_engine/consolidate/consolidator.py @@ -13,6 +13,7 @@ from loguru import logger +from raven.providers.binding import ModelBinding, resolve from raven.tracing import semconv, trace from raven.utils.helpers import ensure_dir, estimate_message_tokens, estimate_prompt_tokens_chain @@ -1711,8 +1712,7 @@ def __init__( enable_foresight: bool = False, ): self.store = MemoryStore(workspace, now_fn=now_fn) - self.provider = provider - self.model = model + self._fallback = ModelBinding(provider, model) self.sessions = sessions self.context_window_tokens = context_window_tokens self._build_messages = build_messages @@ -1723,10 +1723,19 @@ def __init__( self.enable_foresight = enable_foresight self._locks: weakref.WeakValueDictionary[str, asyncio.Lock] = weakref.WeakValueDictionary() + @property + def provider(self) -> "LLMProvider": + """The provider of the turn's binding; the build-time one outside a turn.""" + return resolve(None, self._fallback).provider + + @property + def model(self) -> str: + """The model of the turn's binding; the build-time one outside a turn.""" + return resolve(None, self._fallback).model + def set_provider(self, provider: "LLMProvider", model: str) -> None: """Adopt the provider a live ``/model`` switch just built.""" - self.provider = provider - self.model = model + self._fallback = ModelBinding(provider, model) def get_lock(self, session_key: str) -> asyncio.Lock: """Return the shared consolidation lock for one session.""" diff --git a/raven/memory_engine/skill_forge/gate.py b/raven/memory_engine/skill_forge/gate.py index 10184dc9..1670f064 100644 --- a/raven/memory_engine/skill_forge/gate.py +++ b/raven/memory_engine/skill_forge/gate.py @@ -24,6 +24,7 @@ from typing import TYPE_CHECKING from raven.memory_engine.skill_forge.types import RouterHit +from raven.providers.binding import ModelBinding, active_binding from raven.tracing import semconv, trace if TYPE_CHECKING: @@ -53,28 +54,58 @@ def __init__( model: str | None = None, temperature: float = 0.0, max_tokens: int = 8192, + pin: "ModelBinding | None" = None, ) -> None: - self._provider = provider + self._fallback_provider = provider self._max_select = max_select self._legacy_top_k = legacy_top_k self._model = model self._temperature = temperature self._max_tokens = max_tokens + # A pinned model paired with its own credential. Built by the caller + # from ``skill_forge.llm_gate_model``; None when unset or when that + # vendor has no credentials, in which case the gate follows the turn. + self._pin = pin + self._pin_warned = False def set_provider(self, provider: "LLMProvider", model: str) -> None: - """Adopt the provider a live ``/model`` switch just built. + """Move the out-of-turn fallback. - Unset, ``_model`` follows whatever the new provider defaults to. + Which model the gate runs on inside a turn is decided per call by + ``_binding``, so a session switching models needs nothing here. This + is only for the paths that filter skills outside a turn. + """ + self._fallback_provider = provider + + def _binding(self) -> tuple["LLMProvider", str | None]: + """Its own pinned pair if it has one, else the turn's model. - Set, it is a pin, and this leaves it pinned -- which is what a - restart on the new model would produce, since the gate is built - with the agent's provider and the pin regardless of which vendor - the pin names. Note that a pin is only a model id: the credential - comes from the provider, so a pin naming a vendor the provider does - not serve was already broken at boot, and stays broken here. + Unpinned is the common case and the configured intent: the gate reads + the same model the conversation is on, whichever session that is. A + pin that named a vendor with no credentials never became a pair, so + it is reported once and then ignored rather than sent on the turn's + key -- that combination 401s every call and is swallowed by the top-N + fallback below, which is how it stayed invisible. """ - del model - self._provider = provider + if self._model and self._pin is None and not self._pin_warned: + self._pin_warned = True + log.warning( + "skill_forge.llm_gate_model=%r has no usable credentials of its own; " + "the gate follows the conversation's model instead", + self._model, + ) + if self._pin is not None: + return self._pin.provider, self._pin.model + turn = active_binding() + if turn is not None: + return turn.provider, turn.model + # Outside a turn: the provider it was built with, and no model at all, + # which is what tells that provider to use its own default. + # Never ``self._model``: an unpaired pin sent on this provider's key + # is the mis-pairing the pool exists to prevent. No model at all tells + # the provider to use its own default, which is what an unpinned gate + # gets anyway. + return self._fallback_provider, None @trace.instrument("skill.gate", kind="skill", extract=semconv.skill_gate) async def filter( @@ -87,12 +118,13 @@ async def filter( return [] catalog, by_id = self._build_catalog(candidates) prompt = self._build_prompt(task, catalog, available_tools) + gate_provider, gate_model = self._binding() try: resp = await asyncio.wait_for( - self._provider.chat_with_retry( + gate_provider.chat_with_retry( messages=[{"role": "user", "content": prompt}], - model=self._model or None, + model=gate_model, max_tokens=self._max_tokens, temperature=self._temperature, ), diff --git a/raven/memory_engine/skill_forge/rewriter.py b/raven/memory_engine/skill_forge/rewriter.py index 5c3dd3b6..22cf2710 100644 --- a/raven/memory_engine/skill_forge/rewriter.py +++ b/raven/memory_engine/skill_forge/rewriter.py @@ -21,6 +21,7 @@ from dataclasses import dataclass from typing import TYPE_CHECKING +from raven.providers.binding import active_binding from raven.tracing import semconv, trace if TYPE_CHECKING: @@ -70,20 +71,29 @@ def __init__( max_tokens: int = 8192, temperature: float = 0.3, ) -> None: - self._provider = provider + self._fallback_provider = provider self._max_tokens = max_tokens self._temperature = temperature def set_provider(self, provider: "LLMProvider", model: str) -> None: """Adopt the provider a live ``/model`` switch just built. - Held rather than looked up per call, so without this the rewriter - keeps calling the provider captured at construction after the loop - has moved on -- a switch away from an unusable credential fixes the - main path and leaves this one failing. + Only the out-of-turn fallback moves. Inside a turn the rewriter reads + that turn's binding, so a session switching models is already covered. + The rewriter has no model of its own, so it follows the conversation. """ - del model # the rewriter runs on the provider's default model - self._provider = provider + self._fallback_provider = provider + + def _call_provider(self) -> "LLMProvider": + """The turn's provider inside a turn; the built-in one outside one. + + The rewriter has no model of its own, so it follows the conversation + by taking its provider and passing no model at all -- which lands on + that provider's default. For a pooled binding those coincide, because + the pool builds each provider with the bound model as its default. + """ + turn = active_binding() + return turn.provider if turn is not None else self._fallback_provider @trace.instrument("skill.rewrite", kind="skill", extract=semconv.skill_rewrite) async def analyze(self, query: str) -> RewriteResult: @@ -94,7 +104,7 @@ async def analyze(self, query: str) -> RewriteResult: prompt = _REWRITE_PROMPT.format(query=truncated) try: resp = await asyncio.wait_for( - self._provider.chat_with_retry( + self._call_provider().chat_with_retry( messages=[{"role": "user", "content": prompt}], max_tokens=self._max_tokens, temperature=self._temperature, diff --git a/raven/providers/binding.py b/raven/providers/binding.py new file mode 100644 index 00000000..a9b60c8e --- /dev/null +++ b/raven/providers/binding.py @@ -0,0 +1,84 @@ +"""The model a piece of work is running on, scoped to that work. + +A model id and the credential that serves it are one pair, so they travel +together as a :class:`ModelBinding` rather than as two attributes someone +can update by halves. + +The binding is held in a :class:`~contextvars.ContextVar` rather than +passed down, for two reasons. The turn path reads it at sixteen places +across three packages -- ``raven.agent`` (the loop and the subagent +manager), ``raven.context_engine`` (the curator and the history trimmer) +and ``raven.memory_engine`` (the skill gate, the rewriter and the +consolidator) -- and threading a parameter through all of them would touch +far more code than it explains. +More importantly, ``asyncio.create_task`` copies the current context, so +work detached during a turn (a subagent, a consolidation task) keeps the +binding it was started under for its whole life, which is exactly the +semantics those tasks need: a subagent spawned before a model switch must +not finish on the model chosen after it. + +Nothing here builds providers. :mod:`raven.providers.pool` does that, and +:class:`~raven.agent.loop.main.AgentLoop` decides which binding a turn +runs under. +""" + +from __future__ import annotations + +from contextlib import contextmanager +from contextvars import ContextVar +from dataclasses import dataclass +from typing import TYPE_CHECKING, Iterator + +if TYPE_CHECKING: + from raven.providers.base import LLMProvider + + +@dataclass(frozen=True) +class ModelBinding: + """A model id and the provider whose credential serves it.""" + + provider: "LLMProvider" + model: str + + def __post_init__(self) -> None: + if not self.model: + raise ValueError("a ModelBinding needs a model id") + + +_ACTIVE: ContextVar["ModelBinding | None"] = ContextVar("raven_active_model_binding", default=None) + + +def active_binding() -> "ModelBinding | None": + """The binding the current turn runs under, or None outside a turn. + + Callers outside a turn (startup, a CLI one-shot, a test) get None and + should fall back to whatever default they were built with. + """ + return _ACTIVE.get() + + +@contextmanager +def use_binding(binding: "ModelBinding") -> Iterator["ModelBinding"]: + """Run a block -- and everything it awaits or spawns -- on one binding.""" + token = _ACTIVE.set(binding) + try: + yield binding + finally: + _ACTIVE.reset(token) + + +def resolve(pin: "ModelBinding | None", fallback: "ModelBinding") -> "ModelBinding": + """Which binding should a subsystem use for this call? + + Precedence is the configured rule: a subsystem with a model of its own, + paired with credentials of its own, uses that; otherwise it follows the + model of the turn it is running under; outside a turn it uses the fallback + it was built with. + + ``pin`` is already a pair -- a bare pinned model id never reaches here, + because a model id without a credential is what mis-pairs one vendor's key + with another's endpoint. + """ + if pin is not None: + return pin + return _ACTIVE.get() or fallback diff --git a/raven/providers/pool.py b/raven/providers/pool.py new file mode 100644 index 00000000..5f2ee646 --- /dev/null +++ b/raven/providers/pool.py @@ -0,0 +1,171 @@ +"""Builds a :class:`ModelBinding` for a model id, and caches it. + +One place answers "which credential serves this model", so a per-session +model, a subsystem pin and the configured default all resolve the same way +instead of each guessing. Caching matters because a session switching back +and forth must not rebuild a provider per turn -- building one imports +LiteLLM and writes vendor env vars. + +Provider resolution has to go through a config copy with the model +substituted: :meth:`Config._match_provider` short-circuits on a forced +``agents.defaults.provider``, so asking the live config about another +vendor's model would answer with the forced one and pair the wrong key. +""" + +from __future__ import annotations + +from collections.abc import Callable +from typing import TYPE_CHECKING + +from loguru import logger + +from raven.providers.binding import ModelBinding + +if TYPE_CHECKING: + from raven.config.schema import Config + + +class ProviderPool: + """Resolve and cache one provider per (provider name, model) pair.""" + + def __init__(self, config: "Config | Callable[[], Config]") -> None: + # A supplier, not a snapshot: a credential fixed after start (an OAuth + # re-login, an edited config file) has to be visible without a + # restart, which the old per-switch config reload gave for free. + # Cached bindings are dropped when the config that produced them is no + # longer the current one. + self._supplier = config if callable(config) else (lambda: config) + self._cache: dict[tuple[str, str], ModelBinding] = {} + self._cache_key: str | None = None + + @property + def config(self) -> "Config": + return self._supplier() + + def _live_cache(self) -> dict[tuple[str, str], ModelBinding]: + """Drop cached providers when the credentials behind them changed. + + Not identity on the config object: a supplier that re-reads the file + returns a new object every call, which would clear the cache every + time and defeat the pool. A fingerprint of what a provider is actually + built from is the thing that has to match. + """ + fingerprint = self._credentials_fingerprint() + if self._cache_key != fingerprint: + self._cache_key = fingerprint + self._cache = {} + return self._cache + + def _credentials_fingerprint(self) -> str: + """What a built provider depends on: the keys, bases and headers.""" + import hashlib + import json + + try: + providers = self.config.providers.model_dump(exclude_none=True) + except Exception: + return "" + return hashlib.sha256(json.dumps(providers, sort_keys=True, default=str).encode()).hexdigest() + + def default(self) -> ModelBinding: + """The binding a session starts on: ``agents.defaults``, verbatim. + + Deliberately not "the last model anyone switched to" -- a per-session + switch is scoped to that session, so a new session starts here. + """ + defaults = self.config.agents.defaults + return self.bind(defaults.model, defaults.provider) + + def bind(self, model: str, provider_name: str | None = None) -> ModelBinding: + """Build (or reuse) the provider that serves ``model``. + + ``provider_name`` is what the caller already knows -- the picker sends + one, and ``agents.defaults.provider`` is one. Absent or ``"auto"``, the + vendor is derived from the model id, the same derivation + ``config.set model`` uses for a hand-typed id. + """ + resolved = self._resolve_provider_name(model, provider_name) + cache = self._live_cache() + key = (resolved, model) + cached = cache.get(key) + if cached is not None: + return cached + + from raven.cli._helpers import make_provider + + cfg = self.config.model_copy(deep=True) + cfg.agents.defaults.model = model + cfg.agents.defaults.provider = resolved + binding = ModelBinding(make_provider(cfg), model) + cache[key] = binding + return binding + + def bind_pin(self, model: str | None) -> ModelBinding | None: + """A subsystem's own model, on its own credential -- or None. + + This is what lets a pinned subsystem run off the session's model: a + pin naming a vendor Raven has credentials for gets that vendor's key, + not the agent provider's. None means the pin is unusable (no + credentials, or nothing built), and the caller should fall back to the + session's binding rather than send one vendor's key to another. + """ + if not model: + return None + # A gateway (or a local deployment) serves whatever id it is handed + # under its own credential, so the pin is already paired -- asking + # whether the upstream vendor has a key of its own would drop a pin + # that works. Bind it through the gateway instead. + gateway = self._configured_gateway() + resolved = gateway if gateway is not None else self._resolve_provider_name(model, None) + if gateway is None and not self._has_credentials(resolved, model): + return None + try: + return self.bind(model, resolved) + except (SystemExit, RuntimeError, ValueError) as exc: + # Called from the context-engine factory at construction, so this + # must leave the subsystem following the conversation rather than + # stop the agent from starting. + logger.warning("cannot build a provider for pinned model {!r}: {}", model, exc) + return None + + def _configured_gateway(self) -> str | None: + """The agent's provider, when it is a gateway or a local deployment.""" + from raven.providers.registry import find_by_name + + forced = self.config.agents.defaults.provider + if not forced or forced == "auto": + return None + spec = find_by_name(forced) + if spec is None: + return None + return forced if (spec.is_gateway or spec.is_local) else None + + def _resolve_provider_name(self, model: str, provider_name: str | None) -> str: + if provider_name and provider_name != "auto": + return provider_name + from raven.providers.registry import find_by_model + + spec = find_by_model(model) + return spec.name if spec is not None else "auto" + + def _has_credentials(self, provider_name: str, model: str) -> bool: + """Is there a usable section for this vendor, or only a placeholder? + + Every declared provider exists as an empty section, so presence proves + nothing -- reuse the same check the credential preflight uses. + """ + from raven.config.schema import _has_credentials as section_is_usable + from raven.providers.registry import find_by_name + + if provider_name == "auto": + # No vendor could be derived, so there is no section to check and + # no way to tell a working pin from a mis-paired one. Treat it as + # unusable: the caller's fallback is the session's own binding, + # which is at least a pair. + return False + # ``providers.get`` is the only spelling-insensitive lookup; reading the + # attribute sees one spelling of a name that has several. + section = self.config.providers.get(provider_name) + if section is None: + return False + return bool(section_is_usable(section, find_by_name(provider_name))) diff --git a/raven/tui_rpc/errors.py b/raven/tui_rpc/errors.py index 85be4b8d..3357a2f3 100644 --- a/raven/tui_rpc/errors.py +++ b/raven/tui_rpc/errors.py @@ -12,7 +12,6 @@ | -32006 | skill_not_found | skill_name not indexed | | -32007 | skill_pin_conflict | pin/unpin already in that state | | -32008 | model_not_available | model_id not routable | -| -32009 | model_switch_in_turn | switch attempt while turn live | | -32010 | config_field_readonly | not on hot-changeable whitelist | | -32011 | config_validation_error | Pydantic / semver validation | | -32012 | not_supported_in_v01 | hermes-only stub methods | @@ -101,11 +100,6 @@ class ModelNotAvailableError(RpcError): MESSAGE = "model_not_available" -class ModelSwitchInTurnError(RpcError): - CODE = -32009 - MESSAGE = "model_switch_in_turn" - - class ConfigFieldReadonlyError(RpcError): CODE = -32010 MESSAGE = "config_field_readonly" @@ -174,7 +168,6 @@ class InternalError(RpcError): SkillNotFoundError, SkillPinConflictError, ModelNotAvailableError, - ModelSwitchInTurnError, ConfigFieldReadonlyError, ConfigValidationError, NotSupportedInV01Error, @@ -197,7 +190,6 @@ class InternalError(RpcError): "SkillNotFoundError", "SkillPinConflictError", "ModelNotAvailableError", - "ModelSwitchInTurnError", "ConfigFieldReadonlyError", "ConfigValidationError", "NotSupportedInV01Error", diff --git a/raven/tui_rpc/methods/__init__.py b/raven/tui_rpc/methods/__init__.py index 3b503523..9155e875 100644 --- a/raven/tui_rpc/methods/__init__.py +++ b/raven/tui_rpc/methods/__init__.py @@ -129,7 +129,7 @@ def register_aligned_methods_except_system( # model.{options,save_key,disconnect,add_model,remove_model}: real handlers # must come AFTER register_stub_methods (Dispatcher.register raises on # duplicate; the stub group no longer owns these names). - register_model_methods(dispatcher) + register_model_methods(dispatcher, agent_loop_factory=agent_loop_factory) # harness-command-catalog-dynamic: real ``commands.catalog`` handler; # MUST come after ``register_stub_methods`` because the stub list dropped # its ``commands.catalog`` entry, and ``Dispatcher.register`` raises on diff --git a/raven/tui_rpc/methods/config.py b/raven/tui_rpc/methods/config.py index 15567c17..1f5767c0 100644 --- a/raven/tui_rpc/methods/config.py +++ b/raven/tui_rpc/methods/config.py @@ -26,8 +26,11 @@ import json import re +from collections.abc import Callable from pathlib import Path -from typing import TYPE_CHECKING, Any, Callable +from typing import TYPE_CHECKING, Any + +from loguru import logger from raven.cli._helpers import load_runtime_config, make_provider from raven.providers.registry import find_by_model, find_by_name @@ -35,9 +38,7 @@ ConfigFieldReadonlyError, ConfigValidationError, ModelNotAvailableError, - ModelSwitchInTurnError, ) -from raven.tui_rpc.methods.turn import is_turn_active if TYPE_CHECKING: from raven.tui_rpc.dispatcher import Dispatcher @@ -322,10 +323,21 @@ def _set_model( raw_value: Any, agent_loop_factory: "AgentLoopFactory | None", ) -> dict: - """Switch the global model (and provider) and reassign the live loop. + """Switch the model this session runs on, or the default new ones start on. + + Two scopes, because they answer different questions. With a + ``session_id`` (what the picker sends) the switch is scoped to that + session: no other session moves, and ``agents.defaults`` is left alone so + a new session still starts on the configured default. Pass + ``scope="default"``, or omit ``session_id``, to change that default + instead; sessions that already switched keep their own model. + + Either way the provider is built before anything is persisted or applied, + so a rebuild failure aborts with the on-disk model untouched. - Build the provider from the prospective config BEFORE persisting, so a - rebuild failure aborts cleanly with the on-disk model untouched. + A switch during a turn is not refused. The running turn holds the binding + it started on for its whole tree, so the new model takes effect on the + session's next turn -- which is what a user asking mid-answer means. """ if not isinstance(raw_value, str) or not raw_value: raise ConfigValidationError( @@ -359,42 +371,95 @@ def _set_model( ) session_id = params.get("session_id") - if isinstance(session_id, str) and session_id and is_turn_active(session_id): - raise ModelSwitchInTurnError( - f"cannot switch model while session {session_id!r} has an active turn", - data={"session_id": session_id}, + scope = params.get("scope") + if scope not in (None, "session", "default"): + raise ConfigValidationError( + "config.set model scope must be 'session' or 'default'", + data={"field": "scope", "got": repr(scope)}, ) - - payload = _load_config() - previous = _get_nested(payload, "agents.defaults.model") + session_scoped = scope != "default" and isinstance(session_id, str) and bool(session_id) loop = agent_loop_factory() if agent_loop_factory is not None else None - built_provider = None + binding = None if loop is not None: runtime = load_runtime_config(None, None) runtime.agents.defaults.model = raw_value if new_provider is not None: runtime.agents.defaults.provider = new_provider try: - built_provider = make_provider(runtime) + binding = _build_binding(loop, runtime, raw_value, new_provider) except (SystemExit, RuntimeError, ValueError) as exc: raise ModelNotAvailableError( f"cannot build provider for model {raw_value!r}", data={"model": raw_value, "error": str(exc)}, ) from exc + if session_scoped: + if loop is None: + # Nothing was built, so nothing was validated -- do not report a + # switch that did not happen. + return {"applied": False, "previous": None, "value": raw_value, "scope": "session"} + previous = loop.session_model(session_id) + loop.set_session_binding(session_id, binding) + _remember_session_model(loop, session_id, raw_value, new_provider) + return { + "applied": True, + "previous": previous, + "value": raw_value, + "scope": "session", + "session_id": session_id, + } + + payload = _load_config() + previous = _get_nested(payload, "agents.defaults.model") _set_nested(payload, "agents.defaults.model", raw_value) if new_provider is not None: _set_nested(payload, "agents.defaults.provider", new_provider) _save_config(payload) if loop is not None: - # Not a two-attribute assignment: the loop hands its provider to the - # subagent manager, the context engine and the consolidator at build - # time, and each keeps it. set_provider is what reaches them. - loop.set_provider(built_provider, raw_value) + # Not a two-attribute assignment: the subagent manager, the context + # engine and the consolidator each hold a fallback for work that runs + # outside a turn, and this is what re-points them. + loop.set_default_binding(binding) + + return {"applied": True, "previous": previous, "value": raw_value, "scope": "default"} + + +def _remember_session_model(loop: Any, session_key: str, model: str, provider_name: str | None) -> None: + """Persist the choice on the session, so a restart does not undo it. + + Stored on the session record rather than in ``agents.defaults``: it is + this conversation's model, and a new conversation must still start on the + configured default. + """ + sessions = getattr(loop, "sessions", None) + if sessions is None: + return + try: + session = sessions.get_or_create(session_key) + session.metadata["model"] = model + if provider_name: + session.metadata["provider"] = provider_name + sessions.save(session) + except Exception: + logger.warning("could not persist the model on session {!r}", session_key) + + +def _build_binding(loop: Any, runtime: Any, model: str, provider_name: str | None) -> Any: + """One provider per (vendor, model), reused across sessions and switches. + + Building one imports LiteLLM and writes vendor env vars, so a session + flipping between two models must not pay for it twice. The pool is the + loop's; without one (an older wiring, a test) fall back to building + directly. + """ + from raven.providers.binding import ModelBinding - return {"applied": True, "previous": previous, "value": raw_value} + pool = getattr(loop, "provider_pool", None) + if pool is not None: + return pool.bind(model, provider_name) + return ModelBinding(make_provider(runtime), model) def register_config_methods( diff --git a/raven/tui_rpc/methods/model.py b/raven/tui_rpc/methods/model.py index 044eb436..20a692d5 100644 --- a/raven/tui_rpc/methods/model.py +++ b/raven/tui_rpc/methods/model.py @@ -18,6 +18,7 @@ from __future__ import annotations import asyncio +from functools import partial from typing import TYPE_CHECKING, Any from pydantic import ValidationError @@ -56,6 +57,7 @@ if TYPE_CHECKING: from raven.tui_rpc.dispatcher import Dispatcher + from raven.tui_rpc.methods.session import AgentLoopFactory def _parse(model_cls: type, params: dict) -> Any: @@ -187,9 +189,20 @@ def _current_selection() -> tuple[str, str | None]: # --------------------------------------------------------------------------- -async def model_options(params: dict) -> dict: - _parse(ModelOptionsParams, params) +async def model_options(params: dict, *, agent_loop_factory: "AgentLoopFactory | None" = None) -> dict: + """Which models exist, and which one *this conversation* is on. + + The session matters: the model is per conversation now, so answering from + ``agents.defaults`` would star the wrong row for every session that has + switched -- the picker would disagree with the status bar it sits under. + """ + parsed = _parse(ModelOptionsParams, params) current_model, current_provider = _current_selection() + session_model = _session_model(agent_loop_factory, getattr(parsed, "session_id", None)) + if session_model: + current_model = session_model + spec = find_by_model(session_model) + current_provider = spec.name if spec else current_provider entries = await _entries_off_loop(current_provider) return { "model": current_model, @@ -302,9 +315,27 @@ async def model_remove_model(params: dict) -> dict: } -def register_model_methods(dispatcher: "Dispatcher") -> None: +def _session_model(agent_loop_factory: "AgentLoopFactory | None", session_id: str | None) -> str | None: + """This session's own model, or None when it never switched.""" + if not agent_loop_factory or not session_id: + return None + try: + loop = agent_loop_factory() + except Exception: + return None + # ``session_model`` falls back to the default, so it never answers None -- + # asking it alone would override a forced ``agents.defaults.provider`` for + # every session, including the ones that never switched. + has_own = getattr(loop, "has_session_binding", None) + if not callable(has_own) or not has_own(session_id): + return None + reader = getattr(loop, "session_model", None) + return reader(session_id) if callable(reader) else None + + +def register_model_methods(dispatcher: "Dispatcher", *, agent_loop_factory: "AgentLoopFactory | None" = None) -> None: """Register the five ``model.*`` handlers on a dispatcher instance.""" - dispatcher.register("model.options", model_options) + dispatcher.register("model.options", partial(model_options, agent_loop_factory=agent_loop_factory)) dispatcher.register("model.save_key", model_save_key) dispatcher.register("model.disconnect", model_disconnect) dispatcher.register("model.add_model", model_add_model) diff --git a/raven/tui_rpc/methods/session.py b/raven/tui_rpc/methods/session.py index f1f10f7a..11353d45 100644 --- a/raven/tui_rpc/methods/session.py +++ b/raven/tui_rpc/methods/session.py @@ -132,16 +132,48 @@ def _baseline_usage( } +def _restore_session_model(agent_loop: "AgentLoop", session_key: str) -> None: + """Re-apply the model this session was last switched to. + + Overrides live in the loop's memory, so a restart would otherwise move + every switched session back to the default. The session record is the only + place that choice survives, and this is the only reader of it. + """ + restore = getattr(agent_loop, "restore_session_model", None) + sessions = getattr(agent_loop, "sessions", None) + if not callable(restore) or sessions is None: + return + try: + record = sessions.peek(session_key) + except Exception: + return + metadata = getattr(record, "metadata", None) or {} + model = metadata.get("model") + if model: + restore(session_key, model, metadata.get("provider")) + + def _default_session_info( agent_loop: "AgentLoop | None", config: "Config", + session_key: str | None = None, ) -> dict[str, Any]: """Build the init bundle returned by ``session.create`` / ``session.resume``. ``agent_loop=None`` triggers graceful fallback (``tools={}``, ``skills={}``, zero usage, ``lazy=True``); version is always real (cached at module load). + + The model reported is the one this session runs on, not the configured + default: a session that switched has its own, and reporting the default + would show every other session's user the wrong model. With no + ``session_key`` (a session being created) the default is the right answer, + because that is what a new session starts on. """ model_id = config.agents.defaults.model + if session_key and agent_loop is not None: + session_model = getattr(agent_loop, "session_model", None) + if callable(session_model): + model_id = session_model(session_key) info: dict[str, Any] = { "model": model_id, "model_id": model_id, @@ -284,8 +316,10 @@ async def session_resume( """ agent_loop = _safe_invoke_factory(agent_loop_factory) config = load_config() - info = _default_session_info(agent_loop, config) session_key = params.get("session_id") + if isinstance(session_key, str) and session_key and agent_loop is not None: + _restore_session_model(agent_loop, session_key) + info = _default_session_info(agent_loop, config, session_key if isinstance(session_key, str) else None) if session_key: try: @@ -379,6 +413,12 @@ async def session_delete( config = load_config() mgr = _manager_for(agent_loop, config) removed = mgr.delete(session_key) + if removed and agent_loop is not None: + # Overrides are per session and live for the process; a deleted + # session must not leave one behind. + clear = getattr(agent_loop, "clear_session_binding", None) + if callable(clear): + clear(session_key) return {"deleted": session_key if removed else None} @@ -535,6 +575,14 @@ async def session_branch( config = load_config() mgr = _manager_for(agent_loop, config) child = mgr.fork(session_key, title=(name or None)) + if child is not None and agent_loop is not None: + # A fork continues its parent's conversation, so it continues on the + # parent's model; without this it would silently drop to the default. + binding_for = getattr(agent_loop, "binding_for_session", None) + setter = getattr(agent_loop, "set_session_binding", None) + has_own = getattr(agent_loop, "has_session_binding", None) + if callable(binding_for) and callable(setter) and callable(has_own) and has_own(session_key): + setter(child.key, binding_for(session_key)) if child is None: return {"session_id": None, "title": None} return { diff --git a/raven/tui_rpc/models.py b/raven/tui_rpc/models.py index 6355bf09..2e4bd53d 100644 --- a/raven/tui_rpc/models.py +++ b/raven/tui_rpc/models.py @@ -606,6 +606,11 @@ class ConfigGetResult(_Strict): class ConfigSetParams(_Strict): key: str value: JsonValue + # Model-switch extras. ``scope`` decides the reach of a ``key="model"`` + # switch: this conversation, or the default a new one starts on. + session_id: str | None = None + provider: str | None = None + scope: Literal["session", "default"] | None = None class ConfigSetResult(_Strict): @@ -615,6 +620,10 @@ class ConfigSetResult(_Strict): # already includes ``null``; the schema's redundant ``oneOf: [JsonValue, # null]`` collapses to the same canonical "any" form. previous: JsonValue = Field(...) + # Present on a model switch: what was applied, and where it reached. + value: str | None = None + scope: Literal["session", "default"] | None = None + session_id: str | None = None # --------------------------------------------------------------------------- diff --git a/tests/test_agent_loop_model_switch.py b/tests/test_agent_loop_model_switch.py index 365ddcd5..59c3d336 100644 --- a/tests/test_agent_loop_model_switch.py +++ b/tests/test_agent_loop_model_switch.py @@ -8,31 +8,22 @@ subagent spawns and the skill rewriter/gate failed to authenticate while the main loop worked fine. -Work already in flight is the other half: every call site reads the -provider from ``self`` at call time, so an unconditional swap would relay -one conversation across two vendors. The loop parks a switch until no -turn is running -- a count, not a flag, because a user turn and a -proactive turn hold slots in separate pools and overlap. A detached -subagent outlives that window entirely, so ``spawn`` snapshots the pair -it was asked for. +This file covers the out-of-turn fallback only -- the reference each holder +keeps for work that runs with no turn bound. What a turn actually runs on is +per session and lives in ``test_agent_loop_session_model.py``. """ from __future__ import annotations -import asyncio from types import SimpleNamespace -import pytest - from raven.agent.loop.main import AgentLoop -from raven.agent.subagent.manager import SubagentManager from raven.config.raven import ContextConfig, SkillForgeConfig from raven.context_engine.assembler import ContextAssembler from raven.context_engine.segments.curator import CuratorSegmentBuilder from raven.context_engine.segments.skills import SkillsSegmentBuilder -from raven.memory_engine.skill_forge.gate import LLMGateFilter -from raven.memory_engine.skill_forge.rewriter import QueryRewriter -from raven.providers.base import LLMResponse, ToolCallRequest +from raven.providers.base import LLMResponse +from raven.providers.binding import ModelBinding NEW_MODEL = "anthropic/claude-opus-4-8" @@ -72,12 +63,6 @@ def is_sandboxed(self) -> bool: async def exec(self, command: str, **kwargs): # pragma: no cover - unused raise NotImplementedError - async def __aenter__(self): - return self - - async def __aexit__(self, *exc): - return False - def _noop_submit(*args, **kwargs) -> None: """``_announce_result`` calls the spine submit without awaiting it.""" @@ -112,15 +97,14 @@ def _loop(tmp_path) -> AgentLoop: def test_set_provider_reaches_every_holder() -> None: loop = object.__new__(AgentLoop) - loop.provider = "old-provider" - loop.model = "old-model" loop.subagents = _Recorder() loop.context_engine = _Recorder() loop.memory_consolidator = _Recorder() - loop._turns_in_flight = 0 - loop._pending_provider = None + loop._provider_pool = None + loop._default_binding = ModelBinding(_Provider("old"), "old-model") + loop._session_bindings = {} - new_provider = SimpleNamespace(name="new-provider") + new_provider = _Provider("new-provider") loop.set_provider(new_provider, NEW_MODEL) assert loop.provider is new_provider @@ -152,8 +136,8 @@ def test_switch_reaches_the_real_holders_a_loop_builds(tmp_path) -> None: assert loop.subagents.provider is new_provider assert loop.subagents.model == NEW_MODEL assert loop.memory_consolidator.provider is new_provider - assert skills._gate._provider is new_provider - assert skills._rewriter._provider is new_provider + assert skills._gate._fallback_provider is new_provider + assert skills._rewriter._fallback_provider is new_provider assert curator.provider is new_provider assert curator.assembler.provider is new_provider assert curator.assembler.trimmer.provider is new_provider @@ -188,296 +172,3 @@ def test_assembler_forwards_to_llm_backed_builders_only() -> None: assert llm_builder.provider is new_provider assert llm_builder.model == NEW_MODEL - - -# --------------------------------------------------------------------------- -# Pins -# --------------------------------------------------------------------------- - - -def test_rewriter_and_gate_adopt_the_new_provider() -> None: - new_provider = SimpleNamespace(name="new-provider") - - rewriter = QueryRewriter("old-provider") - rewriter.set_provider(new_provider, NEW_MODEL) - assert rewriter._provider is new_provider - - gate = LLMGateFilter("old-provider") - gate.set_provider(new_provider, NEW_MODEL) - assert gate._provider is new_provider - # Unset means the gate follows the provider's default model, so the - # switch must not pin it to the agent's model behind the user's back. - assert gate._model is None - - -def test_pinned_gate_model_survives_the_switch() -> None: - gate = LLMGateFilter("old-provider", model="openai/gpt-5-mini") - gate.set_provider(SimpleNamespace(name="new-provider"), NEW_MODEL) - assert gate._model == "openai/gpt-5-mini" - - -def test_the_default_curator_model_is_a_pin(tmp_path) -> None: - """``ContextConfig.curator_model`` has a non-empty default, so in practice - it does not follow the agent model -- at construction or across a switch. - """ - loop = _loop(tmp_path) - curator = next(b for b in loop.context_engine._builders if isinstance(b, CuratorSegmentBuilder)) - pinned = curator.curator_model - assert pinned == ContextConfig().curator_model - - loop.set_provider(_Provider("new"), NEW_MODEL) - assert curator.curator_model == pinned - - -def test_an_empty_curator_model_follows_the_agent_model_both_times(tmp_path) -> None: - """The field has no ``min_length``, so ``curator_model: ""`` validates and - the constructor's ``or model`` follows the agent model. A switch has to - follow it too, or the same config means one thing at build time and - another after. - """ - loop = AgentLoop( - provider=_Provider(), - workspace=tmp_path, - model="fake/model", - context_config=ContextConfig(curator_model=""), - skill_forge_config=SkillForgeConfig(), - ) - curator = next(b for b in loop.context_engine._builders if isinstance(b, CuratorSegmentBuilder)) - assert curator.curator_model == "fake/model" - - loop.set_provider(_Provider("new"), NEW_MODEL) - assert curator.curator_model == NEW_MODEL - - -# --------------------------------------------------------------------------- -# In-flight work -# --------------------------------------------------------------------------- - - -def test_a_switch_during_a_turn_is_parked(tmp_path) -> None: - """Every call site reads ``self.provider`` at call time, so adopting - mid-turn would send the rest of one turn to a different vendor. - """ - loop = _loop(tmp_path) - started = _Provider("started-with") - loop.set_provider(started, "started/model") - - loop._turns_in_flight = 1 - switched = _Provider("switched-to") - loop.set_provider(switched, NEW_MODEL) - - assert loop.provider is started, "the turn in flight must keep what it started with" - assert loop.subagents.provider is started - assert loop._pending_provider == (switched, NEW_MODEL) - - loop._turns_in_flight = 0 - loop._adopt_pending_provider() - - assert loop.provider is switched - assert loop.subagents.provider is switched - assert loop._pending_provider is None - - -def test_a_switch_between_turns_applies_immediately(tmp_path) -> None: - loop = _loop(tmp_path) - switched = _Provider("switched-to") - loop.set_provider(switched, NEW_MODEL) - - assert loop.provider is switched - assert loop._pending_provider is None - - -@pytest.mark.asyncio -async def test_a_running_subagent_keeps_the_provider_it_started_with(tmp_path) -> None: - """A subagent is a detached task that outlives the turn that spawned it, - so the loop's park cannot cover it -- ``spawn`` snapshots instead. Without - that, iteration k+1 calls the new vendor carrying k iterations of the old - vendor's message shapes. - """ - seen: list[tuple[str, str]] = [] - release = asyncio.Event() - - class _TwoStepProvider(_Provider): - async def chat_with_retry(self, **kwargs) -> LLMResponse: - seen.append((self.name, kwargs.get("model"))) - if len(seen) == 1: - # Hold the task open across the switch, then ask for one more - # iteration so a re-read of self.provider would show up. - await release.wait() - return LLMResponse( - content="", - tool_calls=[ToolCallRequest(id="c1", name="list_dir", arguments={"path": "."})], - finish_reason="tool_calls", - ) - return LLMResponse(content="done", finish_reason="stop") - - manager = SubagentManager( - provider=_TwoStepProvider("started-with"), - workspace=tmp_path, - model="started/model", - ) - manager._submit = _noop_submit - - task = asyncio.create_task( - manager._run_subagent_inner( - "t1", - "do the thing", - "thing", - {"channel": "cli", "chat_id": "direct", "session_key": "s"}, - _StubExecutor(), - manager.provider, - manager.model, - ) - ) - await asyncio.sleep(0) - manager.set_provider(_TwoStepProvider("switched-to"), NEW_MODEL) - release.set() - await task - - assert [name for name, _ in seen] == ["started-with", "started-with"] - assert [model for _, model in seen] == ["started/model", "started/model"] - # The next spawn does get the new one -- the snapshot is per task, not a freeze. - assert manager.provider.name == "switched-to" - assert manager.model == NEW_MODEL - - -@pytest.mark.asyncio -async def test_run_turn_adopts_on_entry_and_releases_on_exit(tmp_path) -> None: - """The wrapper is the whole mechanism, so drive the real one. Asserting on - ``_turns_in_flight`` alone would pass with the wrapper deleted. - """ - loop = _loop(tmp_path) - started = _Provider("started-with") - loop.set_provider(started, "started/model") - - switched = _Provider("switched-to") - loop._pending_provider = (switched, NEW_MODEL) - - seen: dict[str, object] = {} - - async def _fake_run_turn(*args, **kwargs): - seen["provider"] = loop.provider - seen["depth"] = loop._turns_in_flight - return "outcome" - - loop._run_turn = _fake_run_turn - assert await loop.run_turn(None, None, None) == "outcome" - - assert seen["provider"] is switched, "a parked switch must land before the turn reads it" - assert seen["depth"] == 1 - assert loop._turns_in_flight == 0 - - -@pytest.mark.asyncio -async def test_run_turn_releases_its_slot_when_the_turn_raises(tmp_path) -> None: - """A turn that fails must not leave the loop looking busy forever -- every - later switch would be parked and never adopted. - """ - loop = _loop(tmp_path) - - async def _boom(*args, **kwargs): - raise RuntimeError("turn failed") - - loop._run_turn = _boom - with pytest.raises(RuntimeError): - await loop.run_turn(None, None, None) - - assert loop._turns_in_flight == 0 - - switched = _Provider("switched-to") - loop.set_provider(switched, NEW_MODEL) - assert loop.provider is switched - - -@pytest.mark.asyncio -async def test_a_second_turn_does_not_unpark_a_switch_under_the_first(tmp_path) -> None: - """OriginPools gates USER and system origins on independent semaphores with - no global cap, so a user turn and a cron turn overlap on one loop. A count - is what makes the park survive the shorter of the two. - """ - loop = _loop(tmp_path) - started = _Provider("started-with") - loop.set_provider(started, "started/model") - - long_turn_running = asyncio.Event() - release_long = asyncio.Event() - switched = _Provider("switched-to") - during_short: dict[str, object] = {} - - async def _long(*args, **kwargs): - long_turn_running.set() - await release_long.wait() - during_short["provider_at_end_of_long"] = loop.provider - return "long" - - async def _short(*args, **kwargs): - during_short["provider_during_short"] = loop.provider - return "short" - - loop._run_turn = _long - long_task = asyncio.create_task(loop.run_turn(None, None, None)) - await long_turn_running.wait() - - # The switch lands while only the long turn is running. - loop.set_provider(switched, NEW_MODEL) - assert loop.provider is started - assert loop._pending_provider is not None - - # A second, shorter turn starts and finishes underneath it. - loop._run_turn = _short - await loop.run_turn(None, None, None) - - assert during_short["provider_during_short"] is started, "the short turn must not adopt the park" - assert loop.provider is started, "the long turn is still running" - assert loop._turns_in_flight == 1 - - release_long.set() - await long_task - - assert loop.provider is switched, "the last turn out adopts it" - assert loop._pending_provider is None - - -@pytest.mark.asyncio -async def test_spawn_snapshots_before_the_task_queues(tmp_path) -> None: - """The snapshot must be taken in ``spawn``, not where the task starts - running: a spawn waits on the concurrency gate and a sandbox boot first, - and a switch landing in that window would hand the task an endpoint the - user chose after asking for it. - - Driven through the real ``_run_subagent`` so the window is genuinely - open -- stubbing it would prove only that ``spawn`` passes *a* pair, which - a snapshot taken later would also satisfy. - """ - import raven.agent.subagent.manager as manager_mod - - served: list[str] = [] - - class _RecordingProvider(_Provider): - async def chat_with_retry(self, **kwargs) -> LLMResponse: - served.append(self.name) - return LLMResponse(content="done", finish_reason="stop") - - manager = SubagentManager( - provider=_RecordingProvider("started-with"), - workspace=tmp_path, - model="started/model", - ) - manager._submit = _noop_submit - # Hold every spawn in exactly the window the snapshot exists for. - manager._gate = asyncio.Semaphore(0) - - original_build = manager_mod.build_executor - manager_mod.build_executor = lambda cfg, workspace, owned_ids=None: _StubExecutor() - try: - await manager.spawn("do the thing", label="thing", session_key="s") - manager.set_provider(_RecordingProvider("switched-to"), NEW_MODEL) - manager._gate.release() - for _ in range(50): - await asyncio.sleep(0) - if served: - break - finally: - manager_mod.build_executor = original_build - - assert served == ["started-with"], "the spawn ran on the provider it was asked for" diff --git a/tests/test_agent_loop_session_model.py b/tests/test_agent_loop_session_model.py new file mode 100644 index 00000000..9e66e925 --- /dev/null +++ b/tests/test_agent_loop_session_model.py @@ -0,0 +1,559 @@ +"""The model is per conversation, and one conversation's switch is its own. + +Five rules, in the order a user would state them: + +1. different sessions can be on different models; +2. switching one session does not move another; +3. a new session starts on the configured default, not on whatever the last + session switched to; +4. a subsystem with a model *and credentials* of its own uses them; without + both it follows the model of the conversation it is running under; +5. a switch that arrives while a turn is running takes effect on the next + turn, not in the middle of this one. + +Rule 5 is not a mechanism here, it is a consequence: ``run_turn`` resolves the +session's binding once and holds it in a context var for the whole turn tree, +so a switch landing mid-turn is simply not visible to that turn. The same +context copy is what makes a detached subagent finish on the model it was +spawned under. +""" + +from __future__ import annotations + +import asyncio + +import pytest + +from raven.agent.loop.main import AgentLoop +from raven.config.raven import ContextConfig, SkillForgeConfig +from raven.context_engine.segments.curator import CuratorSegmentBuilder +from raven.context_engine.segments.skills import SkillsSegmentBuilder +from raven.providers.base import LLMResponse +from raven.providers.binding import ModelBinding, active_binding +from raven.spine.message import ChatType, Source +from raven.spine.turn import Origin, TurnRequest + + +class _Provider: + def __init__(self, name: str) -> None: + self.name = name + + def get_default_model(self) -> str: + return f"{self.name}/default" + + async def chat_with_retry(self, **kwargs) -> LLMResponse: + return LLMResponse(content="ok", finish_reason="stop") + + +def _loop(tmp_path) -> AgentLoop: + return AgentLoop( + provider=_Provider("boot"), + workspace=tmp_path, + model="boot/model", + context_config=ContextConfig(), + skill_forge_config=SkillForgeConfig(), + ) + + +def _req(session_key: str) -> TurnRequest: + return TurnRequest( + origin=Origin.USER, + source=Source(channel="tui", chat_id="default", sender_id="user", chat_type=ChatType.DM), + text="hi", + conversation=session_key, + ) + + +def _binding(name: str, model: str) -> ModelBinding: + return ModelBinding(_Provider(name), model) + + +async def _run(loop: AgentLoop, session_key: str, body) -> object: + loop._run_turn = body + return await loop.run_turn(_req(session_key), None, None) + + +# --------------------------------------------------------------------------- +# 1 + 2 + 3: scope +# --------------------------------------------------------------------------- + + +def test_a_session_without_a_switch_is_on_the_default(tmp_path) -> None: + loop = _loop(tmp_path) + assert loop.session_model("tui:a") == "boot/model" + assert loop.binding_for_session("tui:a") is loop.default_binding + + +def test_two_sessions_can_be_on_two_models(tmp_path) -> None: + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + loop.set_session_binding("tui:b", _binding("prov-b", "vendor-b/model")) + + assert loop.session_model("tui:a") == "vendor-a/model" + assert loop.session_model("tui:b") == "vendor-b/model" + + +def test_switching_one_session_leaves_the_others_alone(tmp_path) -> None: + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + + assert loop.session_model("tui:b") == "boot/model", "an untouched session stays on the default" + assert loop.default_binding.model == "boot/model", "a session switch is not a default change" + + +def test_a_new_session_starts_on_the_default_not_the_last_switch(tmp_path) -> None: + """Rule 3. A session-scoped switch is deliberately not sticky: the next + session created reads the configured default again. + """ + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + + assert loop.session_model("tui:fresh") == "boot/model" + + +def test_changing_the_default_moves_only_sessions_that_never_switched(tmp_path) -> None: + loop = _loop(tmp_path) + loop.set_session_binding("tui:pinned", _binding("prov-a", "vendor-a/model")) + + loop.set_default_binding(_binding("prov-new", "vendor-new/model")) + + assert loop.session_model("tui:pinned") == "vendor-a/model" + assert loop.session_model("tui:drifting") == "vendor-new/model" + + +def test_dropping_a_session_override_returns_it_to_the_default(tmp_path) -> None: + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + loop.clear_session_binding("tui:a") + + assert loop.session_model("tui:a") == "boot/model" + + +# --------------------------------------------------------------------------- +# The turn boundary +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_a_turn_runs_on_its_own_session_model(tmp_path) -> None: + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + + seen: dict[str, object] = {} + + async def _body(*args, **kwargs): + # Everything under the turn reads the same pair, including the holders + # that used to keep a reference of their own. + seen["loop"] = (loop.provider.name, loop.model) + seen["subagents"] = (loop.subagents.provider.name, loop.subagents.model) + seen["consolidator"] = loop.memory_consolidator.provider.name + return "done" + + assert await _run(loop, "tui:a", _body) == "done" + assert seen["loop"] == ("prov-a", "vendor-a/model") + assert seen["subagents"] == ("prov-a", "vendor-a/model") + assert seen["consolidator"] == "prov-a" + + +@pytest.mark.asyncio +async def test_outside_a_turn_the_loop_reports_the_default(tmp_path) -> None: + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + + assert active_binding() is None + assert loop.model == "boot/model" + + +@pytest.mark.asyncio +async def test_two_concurrent_turns_each_keep_their_own_model(tmp_path) -> None: + """Rules 1 and 2 have to hold while both turns are in flight, which is the + case a shared provider could never express: a user turn and a cron turn run + at the same time on this loop. + """ + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + loop.set_session_binding("cron:job", _binding("prov-cron", "vendor-cron/model")) + + a_started = asyncio.Event() + release_a = asyncio.Event() + seen: dict[str, str] = {} + + async def _body(req, *args, **kwargs): + key = req.conversation + if key == "tui:a": + a_started.set() + await release_a.wait() + seen[key] = loop.model + return key + + loop._run_turn = _body + a = asyncio.create_task(loop.run_turn(_req("tui:a"), None, None)) + await a_started.wait() + await loop.run_turn(_req("cron:job"), None, None) + release_a.set() + await a + + assert seen["cron:job"] == "vendor-cron/model" + assert seen["tui:a"] == "vendor-a/model", "the cron turn must not have moved the user turn" + + +@pytest.mark.asyncio +async def test_a_switch_mid_turn_lands_on_the_next_turn(tmp_path) -> None: + """Rule 5, with no parking involved: the running turn holds the binding it + entered on, so the switch is invisible to it and current for the next one. + """ + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", _binding("prov-old", "vendor-old/model")) + + started = asyncio.Event() + release = asyncio.Event() + during: list[str] = [] + + async def _body(req, *args, **kwargs): + started.set() + await release.wait() + during.append(loop.model) + return "done" + + loop._run_turn = _body + running = asyncio.create_task(loop.run_turn(_req("tui:a"), None, None)) + await started.wait() + + loop.set_session_binding("tui:a", _binding("prov-new", "vendor-new/model")) + release.set() + await running + + assert during == ["vendor-old/model"], "the turn in flight must not move" + + after: list[str] = [] + + async def _next(req, *args, **kwargs): + after.append(loop.model) + return "done" + + await _run(loop, "tui:a", _next) + assert after == ["vendor-new/model"] + + +@pytest.mark.asyncio +async def test_the_turn_binding_is_released_when_the_turn_raises(tmp_path) -> None: + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + + async def _boom(*args, **kwargs): + raise RuntimeError("turn failed") + + loop._run_turn = _boom + with pytest.raises(RuntimeError): + await loop.run_turn(_req("tui:a"), None, None) + + assert active_binding() is None + assert loop.model == "boot/model" + + +# --------------------------------------------------------------------------- +# Rule 4: subsystems +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_a_spawned_subagent_keeps_its_conversations_model(tmp_path) -> None: + """A subagent has no model of its own, so it follows the conversation that + spawned it -- and keeps doing so after that conversation switches, because + it is a detached task that outlives the turn. + """ + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + + captured: dict[str, object] = {} + + async def _capture(task_id, task, label, origin, provider, model): + captured["pair"] = (provider.name, model) + + loop.subagents._run_subagent = _capture + loop.subagents._gate = asyncio.Semaphore(0) + + async def _body(*args, **kwargs): + await loop.subagents.spawn("do it", label="it", session_key="tui:a") + return "done" + + await _run(loop, "tui:a", _body) + loop.set_session_binding("tui:a", _binding("prov-new", "vendor-new/model")) + await asyncio.sleep(0) + + assert captured["pair"] == ("prov-a", "vendor-a/model") + + +@pytest.mark.asyncio +async def test_an_unconfigured_subsystem_follows_the_conversation(tmp_path) -> None: + """The gate and the curator are unpinned here (no credentials for the + default ``curator_model``), so both read the turn's model. + """ + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + skills = next(b for b in loop.context_engine._builders if isinstance(b, SkillsSegmentBuilder)) + curator = next(b for b in loop.context_engine._builders if isinstance(b, CuratorSegmentBuilder)) + + seen: dict[str, object] = {} + + async def _body(*args, **kwargs): + seen["gate"] = skills._gate._binding()[1] + seen["curator"] = curator.curator_model + seen["rewriter"] = skills._rewriter._call_provider().name + return "done" + + await _run(loop, "tui:a", _body) + assert seen["gate"] == "vendor-a/model" + assert seen["curator"] == "vendor-a/model" + assert seen["rewriter"] == "prov-a" + + +@pytest.mark.asyncio +async def test_a_configured_subsystem_uses_its_own_pair(tmp_path) -> None: + """With a model *and* a credential of its own, a subsystem stops following + the conversation -- that is the whole point of configuring one. + """ + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + skills = next(b for b in loop.context_engine._builders if isinstance(b, SkillsSegmentBuilder)) + curator = next(b for b in loop.context_engine._builders if isinstance(b, CuratorSegmentBuilder)) + + pin = _binding("prov-pin", "vendor-pin/small") + skills._gate._pin = pin + curator._pin = pin + + seen: dict[str, object] = {} + + async def _body(*args, **kwargs): + seen["gate"] = skills._gate._binding()[1] + seen["curator"] = curator.curator_model + return "done" + + await _run(loop, "tui:a", _body) + assert seen["gate"] == "vendor-pin/small" + assert seen["curator"] == "vendor-pin/small" + + +def test_the_gate_sends_no_model_outside_a_turn(tmp_path) -> None: + """An unpaired pin must never go out on the fallback provider's key. With + no turn bound there is nothing to follow, so the gate asks the provider for + its own default rather than posting a model id it has no credential for. + """ + loop = _loop(tmp_path) + skills = next(b for b in loop.context_engine._builders if isinstance(b, SkillsSegmentBuilder)) + skills._gate._model = "openai/gpt-5-mini" + skills._gate._pin = None + + assert active_binding() is None + assert skills._gate._binding()[1] is None + + +@pytest.mark.asyncio +async def test_a_request_without_a_conversation_falls_back_to_its_channel_key(tmp_path) -> None: + """Non-TUI channels and cron arrive with no ``conversation``; the + ``channel:chat_id`` key is the only one they get, so a switch stored under + it has to be the one their turn runs on. + """ + loop = _loop(tmp_path) + loop.set_session_binding("whatsapp:12345", _binding("prov-wa", "vendor-wa/model")) + + seen: list[str] = [] + + async def _body(*args, **kwargs): + seen.append(loop.model) + return "done" + + loop._run_turn = _body + req = TurnRequest( + origin=Origin.USER, + source=Source(channel="whatsapp", chat_id="12345", sender_id="u", chat_type=ChatType.DM), + text="hi", + ) + await loop.run_turn(req, None, None) + + assert seen == ["vendor-wa/model"] + + +def test_a_configured_pin_survives_a_real_factory_build(tmp_path) -> None: + """The pin only becomes a pair when a pool is wired, and every entry point + must wire one -- without it a correctly credentialed ``curator_model`` is + silently ignored and the user is told it has no credentials. + """ + from raven.config.schema import Config + from raven.providers.pool import ProviderPool + + cfg = Config() + cfg.agents.defaults.model = "claude-opus-4-5" + cfg.agents.defaults.provider = "auto" + cfg.providers.anthropic.api_key = "sk-ant" + cfg.providers.gemini.api_key = "AIza" + + context_config = ContextConfig(curator_model="gemini-2.5-flash") + loop = AgentLoop( + provider=_Provider("boot"), + workspace=tmp_path, + model="boot/model", + context_config=context_config, + skill_forge_config=SkillForgeConfig(), + provider_pool=ProviderPool(cfg), + ) + curator = next(b for b in loop.context_engine._builders if isinstance(b, CuratorSegmentBuilder)) + + assert curator._pin is not None, "a credentialed curator_model must become a pair" + assert curator.curator_model == "gemini-2.5-flash" + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + assert curator.curator_model == "gemini-2.5-flash", "a configured subsystem does not follow the turn" + + +def test_a_stored_model_is_restored_onto_a_resumed_session(tmp_path) -> None: + """The write half is useless without this read half. A switch has to + survive a restart, or the user's choice lasts exactly as long as the + process -- and the persistence looks like it works while doing nothing. + """ + from raven.config.schema import Config + from raven.providers.pool import ProviderPool + + cfg = Config() + cfg.agents.defaults.model = "claude-opus-4-5" + cfg.agents.defaults.provider = "auto" + cfg.providers.anthropic.api_key = "sk-ant" + loop = AgentLoop( + provider=_Provider("boot"), + workspace=tmp_path, + model="boot/model", + context_config=ContextConfig(), + skill_forge_config=SkillForgeConfig(), + provider_pool=ProviderPool(cfg), + ) + + assert loop.session_model("tui:a") == "boot/model", "a fresh process has no override" + + loop.restore_session_model("tui:a", "claude-sonnet-4-5") + + assert loop.session_model("tui:a") == "claude-sonnet-4-5" + assert loop.has_session_binding("tui:a") + + +def test_a_stored_model_that_cannot_be_built_leaves_the_default(tmp_path) -> None: + """A credential removed since the switch must not fail the resume.""" + from raven.config.schema import Config + from raven.providers.pool import ProviderPool + + cfg = Config() + cfg.agents.defaults.model = "claude-opus-4-5" + loop = AgentLoop( + provider=_Provider("boot"), + workspace=tmp_path, + model="boot/model", + context_config=ContextConfig(), + skill_forge_config=SkillForgeConfig(), + provider_pool=ProviderPool(cfg), + ) + + loop.restore_session_model("tui:a", "gemini-2.5-flash") + + assert loop.session_model("tui:a") == "boot/model" + assert not loop.has_session_binding("tui:a") + + +def test_has_session_binding_distinguishes_chosen_from_inherited(tmp_path) -> None: + """``session_model`` falls back to the default, so it cannot answer this -- + and callers that override a forced provider need the difference. + """ + loop = _loop(tmp_path) + assert not loop.has_session_binding("tui:a") + + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + + assert loop.has_session_binding("tui:a") + assert not loop.has_session_binding("tui:b") + + +def test_a_configured_gate_pin_survives_a_real_factory_build(tmp_path) -> None: + """The gate's pin is built in the same factory as the curator's but was + uncovered -- and a gate failure is swallowed by its top-N fallback, so + losing the pin degrades silently by design. + """ + from raven.config.schema import Config + from raven.providers.pool import ProviderPool + + cfg = Config() + cfg.agents.defaults.model = "claude-opus-4-5" + cfg.agents.defaults.provider = "auto" + cfg.providers.anthropic.api_key = "sk-ant" + cfg.providers.openai.api_key = "sk-openai" + + loop = AgentLoop( + provider=_Provider("boot"), + workspace=tmp_path, + model="boot/model", + context_config=ContextConfig(), + skill_forge_config=SkillForgeConfig(llm_gate_model="openai/gpt-5-mini"), + provider_pool=ProviderPool(cfg), + ) + skills = next(b for b in loop.context_engine._builders if isinstance(b, SkillsSegmentBuilder)) + + assert skills._gate is not None + assert skills._gate._pin is not None, "a credentialed llm_gate_model must become a pair" + assert skills._gate._binding()[1] == "openai/gpt-5-mini" + + loop.set_session_binding("tui:a", _binding("prov-a", "vendor-a/model")) + assert skills._gate._binding()[1] == "openai/gpt-5-mini", "a configured subsystem does not follow the turn" + + +@pytest.mark.asyncio +async def test_a_spawn_holds_its_binding_through_the_gate_and_the_sandbox_boot(tmp_path) -> None: + """The binding is taken in ``spawn``, not where the task starts running: a + spawn waits on the concurrency gate and a sandbox boot first, and a switch + landing in that window would hand it an endpoint chosen after it was asked + for. Driven through the real ``_run_subagent`` so the window is genuinely + open -- stubbing it would prove only that ``spawn`` passes a pair. + """ + import raven.agent.subagent.manager as manager_mod + from raven.providers.base import LLMResponse as _Resp + + served: list[str] = [] + + class _Recording(_Provider): + async def chat_with_retry(self, **kwargs) -> _Resp: + served.append(self.name) + return _Resp(content="done", finish_reason="stop") + + class _StubExecutor: + @property + def is_sandboxed(self) -> bool: + return False + + async def exec(self, command: str, **kwargs): # pragma: no cover - unused + raise NotImplementedError + + async def __aenter__(self): + return self + + async def __aexit__(self, *exc): + return False + + loop = _loop(tmp_path) + loop.set_session_binding("tui:a", ModelBinding(_Recording("started-with"), "started/model")) + loop.subagents._submit = lambda *a, **k: None + loop.subagents._gate = asyncio.Semaphore(0) + + original_build = manager_mod.build_executor + manager_mod.build_executor = lambda cfg, workspace, owned_ids=None: _StubExecutor() + try: + + async def _body(*args, **kwargs): + await loop.subagents.spawn("do it", label="it", session_key="tui:a") + return "done" + + await _run(loop, "tui:a", _body) + loop.set_session_binding("tui:a", ModelBinding(_Recording("switched-to"), "switched/model")) + loop.subagents._gate.release() + for _ in range(50): + await asyncio.sleep(0) + if served: + break + finally: + manager_mod.build_executor = original_build + + assert served == ["started-with"], "the spawn ran on the model its conversation had when it asked" diff --git a/tests/test_provider_pool.py b/tests/test_provider_pool.py new file mode 100644 index 00000000..3837e7a0 --- /dev/null +++ b/tests/test_provider_pool.py @@ -0,0 +1,360 @@ +"""Where a model id becomes a model id plus the credential that serves it. + +A pin like ``context.curator_model`` is a model id and nothing else, so +resolving it has to answer a second question: which configured provider +serves that vendor. Answering it in one place is what stops a subsystem +from sending one vendor's key to another's endpoint -- either the pool +returns a complete pair, or it returns nothing and the caller follows the +conversation instead. +""" + +from __future__ import annotations + +import os + +import pytest + +from raven.config.schema import Config +from raven.providers.binding import ModelBinding, active_binding, resolve, use_binding + + +@pytest.fixture(autouse=True) +def _restore_env(): + """Building a keyed provider writes the vendor's env var; keep that here.""" + before = dict(os.environ) + yield + os.environ.clear() + os.environ.update(before) + + +def _config(model: str = "claude-opus-4-5", **keys: str) -> Config: + cfg = Config() + cfg.agents.defaults.model = model + cfg.agents.defaults.provider = "auto" + for name, key in keys.items(): + section = cfg.providers.get(name) + assert section is not None, name + section.api_key = key + return cfg + + +def _pool(model: str = "claude-opus-4-5", **kwargs): + from raven.providers.pool import ProviderPool + + return ProviderPool(_config(model, **kwargs)) + + +# --------------------------------------------------------------------------- +# The default +# --------------------------------------------------------------------------- + + +def test_the_default_is_agents_defaults_verbatim() -> None: + pool = _pool(anthropic="sk-ant") + binding = pool.default() + + assert binding.model == "claude-opus-4-5" + assert binding.provider.api_key == "sk-ant" + + +def test_the_default_is_not_the_last_thing_anyone_switched_to() -> None: + """A per-session switch must not leak into what a new session starts on, + so the pool reads the config every time rather than caching a "current". + """ + pool = _pool(anthropic="sk-ant", gemini="AIza") + pool.bind("gemini-2.5-flash") + + assert pool.default().model == "claude-opus-4-5" + + +# --------------------------------------------------------------------------- +# Binding a model +# --------------------------------------------------------------------------- + + +def test_a_model_gets_its_own_vendors_key() -> None: + """The point of the pool: the credential follows the model id, not whatever + provider the caller happened to be holding. + """ + pool = _pool(anthropic="sk-ant", gemini="AIza") + + assert pool.bind("gemini-2.5-flash").provider.api_key == "AIza" + assert pool.bind("claude-sonnet-4-5").provider.api_key == "sk-ant" + + +def test_the_same_pair_is_built_once() -> None: + """Building a provider imports LiteLLM and writes vendor env vars, so a + session flipping between two models must not pay for it per turn. + """ + pool = _pool(anthropic="sk-ant") + first = pool.bind("claude-opus-4-5") + + assert pool.bind("claude-opus-4-5") is first + + +def test_two_models_of_one_vendor_are_two_bindings() -> None: + pool = _pool(anthropic="sk-ant") + + assert pool.bind("claude-opus-4-5") is not pool.bind("claude-sonnet-4-5") + + +def test_an_explicit_provider_name_wins_over_derivation() -> None: + """The picker sends the provider alongside the model; a gateway serving + another vendor's id would be mis-derived without it. + """ + pool = _pool("anthropic/claude-opus-4-5", openrouter="sk-or") + binding = pool.bind("anthropic/claude-opus-4-5", "openrouter") + + assert binding.provider.api_key == "sk-or" + + +# --------------------------------------------------------------------------- +# Pins +# --------------------------------------------------------------------------- + + +def test_a_pin_with_credentials_becomes_a_pair() -> None: + pool = _pool(anthropic="sk-ant", gemini="AIza") + pin = pool.bind_pin("gemini-2.5-flash") + + assert pin is not None + assert pin.model == "gemini-2.5-flash" + assert pin.provider.api_key == "AIza" + + +def test_a_pin_without_credentials_is_not_a_pair() -> None: + """The case that used to 401 every call: a model id whose vendor has no key. + None tells the caller to follow the conversation instead of sending this id + on the conversation's credential. + """ + pool = _pool(anthropic="sk-ant") + + assert pool.bind_pin("gemini-2.5-flash") is None + assert pool.bind_pin("openai/gpt-5-mini") is None + + +def test_an_unset_pin_is_not_a_pair() -> None: + pool = _pool(anthropic="sk-ant") + + assert pool.bind_pin(None) is None + assert pool.bind_pin("") is None + + +def test_a_pin_whose_vendor_cannot_be_derived_is_not_a_pair() -> None: + """With no vendor there is no section to check, so there is no way to tell a + working pin from a mis-paired one. Following the conversation is at least a + pair. + """ + pool = _pool(anthropic="sk-ant") + + assert pool.bind_pin("some-unknown-local-model") is None + + +def test_a_declared_but_empty_section_is_not_credentials() -> None: + """Every vendor exists as an empty section whether or not it was configured, + so presence proves nothing. + """ + pool = _pool(anthropic="sk-ant") + assert pool.config.providers.get("gemini") is not None + + assert pool.bind_pin("gemini-2.5-flash") is None + + +# --------------------------------------------------------------------------- +# The binding context +# --------------------------------------------------------------------------- + + +class _Stub: + def get_default_model(self) -> str: + return "stub/default" + + +def test_a_binding_needs_a_model() -> None: + with pytest.raises(ValueError): + ModelBinding(_Stub(), "") + + +def test_nothing_is_bound_outside_a_turn() -> None: + assert active_binding() is None + + +def test_a_binding_is_visible_for_its_block_and_no_longer() -> None: + binding = ModelBinding(_Stub(), "vendor/model") + with use_binding(binding): + assert active_binding() is binding + assert active_binding() is None + + +def test_a_binding_is_restored_when_the_block_raises() -> None: + with pytest.raises(RuntimeError): + with use_binding(ModelBinding(_Stub(), "vendor/model")): + raise RuntimeError("boom") + assert active_binding() is None + + +def test_bindings_nest() -> None: + outer = ModelBinding(_Stub(), "outer/model") + inner = ModelBinding(_Stub(), "inner/model") + with use_binding(outer): + with use_binding(inner): + assert active_binding() is inner + assert active_binding() is outer + + +def test_resolve_prefers_a_pin_then_the_turn_then_the_fallback() -> None: + pin = ModelBinding(_Stub(), "pin/model") + turn = ModelBinding(_Stub(), "turn/model") + fallback = ModelBinding(_Stub(), "fallback/model") + + assert resolve(None, fallback) is fallback + with use_binding(turn): + assert resolve(None, fallback) is turn + assert resolve(pin, fallback) is pin, "a configured subsystem does not follow the turn" + + +@pytest.mark.asyncio +async def test_a_detached_task_keeps_the_binding_it_was_created_under() -> None: + """This is what makes a spawned subagent finish on the model it started on: + ``create_task`` copies the context, so a later switch cannot reach it. + """ + import asyncio + + started = ModelBinding(_Stub(), "started/model") + seen: list[str | None] = [] + release = asyncio.Event() + + async def _detached() -> None: + await release.wait() + binding = active_binding() + seen.append(binding.model if binding else None) + + with use_binding(started): + task = asyncio.create_task(_detached()) + + # Outside the block now, and a different binding is current. + with use_binding(ModelBinding(_Stub(), "switched/model")): + release.set() + await task + + assert seen == ["started/model"] + + +def test_the_default_is_re_read_every_time() -> None: + """Not cached: the configured default is what a new session starts on, and + a user editing it must not need a restart. + """ + pool = _pool(anthropic="sk-ant") + assert pool.default().model == "claude-opus-4-5" + + pool.config.agents.defaults.model = "claude-sonnet-4-5" + assert pool.default().model == "claude-sonnet-4-5" + + +def test_a_config_supplier_is_re_read_and_drops_stale_bindings() -> None: + """A credential fixed after start (an OAuth re-login, an edited file) has + to be visible without a restart, which a snapshot would prevent. + """ + from raven.providers.pool import ProviderPool + + broken = _config(anthropic="") + fixed = _config(anthropic="sk-ant") + current = [broken] + pool = ProviderPool(lambda: current[0]) + + assert pool.bind_pin("claude-opus-4-5") is None, "no key yet" + + current[0] = fixed + pin = pool.bind_pin("claude-opus-4-5") + assert pin is not None + assert pin.provider.api_key == "sk-ant" + + +def test_a_gateway_serves_a_pin_its_upstream_vendor_has_no_key_for() -> None: + """A gateway bills its own key for whatever id it is handed, so asking + whether the upstream vendor has a key of its own would drop a pin that + works -- which is how a working setup got broken once already. + """ + cfg = _config("anthropic/claude-opus-4-5", openrouter="sk-or") + cfg.agents.defaults.provider = "openrouter" + + from raven.providers.pool import ProviderPool + + pin = ProviderPool(cfg).bind_pin("gemini-2.5-flash") + + assert pin is not None + assert pin.model == "gemini-2.5-flash" + assert pin.provider.api_key == "sk-or" + + +def test_an_unbuildable_pin_is_reported_not_raised(monkeypatch) -> None: + """``bind_pin`` is called from the factory at construction; a vendor whose + provider cannot be built must leave the subsystem following the + conversation, not stop the agent from starting. + """ + import raven.cli._helpers as helpers + + pool = _pool(anthropic="sk-ant", gemini="AIza") + + def _boom(_cfg): + raise RuntimeError("cannot build") + + monkeypatch.setattr(helpers, "make_provider", _boom) + assert pool.bind_pin("gemini-2.5-flash") is None + + +def test_a_credential_added_after_boot_is_visible_without_a_restart() -> None: + """The production wiring hands the pool a loader, not the boot config: a + user who connects a provider in the picker and then picks its model must + not be told the model is unavailable. + """ + from raven.providers.pool import ProviderPool + + cfg = _config() + pool = ProviderPool(lambda: cfg) + + assert pool.bind_pin("gemini-2.5-flash") is None, "no key yet" + + cfg.providers.gemini.api_key = "AIza-added-after-boot" + pin = pool.bind_pin("gemini-2.5-flash") + + assert pin is not None + assert pin.provider.api_key == "AIza-added-after-boot" + + +def test_a_re_reading_supplier_still_reuses_bindings() -> None: + """Invalidating on config identity would clear the cache on every call for + a supplier that re-reads, which defeats the pool. Only a credential change + may drop it. + """ + from raven.providers.pool import ProviderPool + + def _fresh(): + # A new object every call, same contents -- what a file loader does. + return _config(anthropic="sk-ant") + + pool = ProviderPool(_fresh) + first = pool.bind("claude-opus-4-5") + + assert pool.bind("claude-opus-4-5") is first + + +def test_a_gateway_pin_that_cannot_be_built_is_reported_not_raised(monkeypatch) -> None: + """The gateway branch is reached from the factory at construction too, so + it needs the same guard as the direct-vendor branch -- otherwise a missing + gateway key stops the agent from starting. + """ + import raven.cli._helpers as helpers + + cfg = _config("anthropic/claude-opus-4-5", openrouter="sk-or") + cfg.agents.defaults.provider = "openrouter" + + from raven.providers.pool import ProviderPool + + pool = ProviderPool(cfg) + + def _boom(_cfg): + raise RuntimeError("no gateway key") + + monkeypatch.setattr(helpers, "make_provider", _boom) + assert pool.bind_pin("gemini-2.5-flash") is None diff --git a/tests/test_read_file_image.py b/tests/test_read_file_image.py index d282211c..44b1550d 100644 --- a/tests/test_read_file_image.py +++ b/tests/test_read_file_image.py @@ -721,11 +721,14 @@ def test_capability_cache_is_keyed_by_model() -> None: """The loop is a long-lived singleton taking a per-call model, so a verdict learned for one model must not answer for another.""" from raven.agent.loop.main import AgentLoop + from raven.providers.binding import ModelBinding from raven.providers.litellm_provider import LiteLLMProvider loop = object.__new__(AgentLoop) - loop.provider = object.__new__(LiteLLMProvider) - loop.model = "claude-opus-4-5" + # The pair, not one half of it: ``provider``/``model`` are read-only views + # onto whichever binding is current. + loop._default_binding = ModelBinding(object.__new__(LiteLLMProvider), "claude-opus-4-5") + loop._session_bindings = {} loop._image_tool_result_ok = {} assert loop._supports_image_tool_result("claude-opus-4-5") is True diff --git a/tests/test_rpc_schema_match.py b/tests/test_rpc_schema_match.py index 2c205dc4..b9930491 100644 --- a/tests/test_rpc_schema_match.py +++ b/tests/test_rpc_schema_match.py @@ -420,7 +420,6 @@ def test_method_result_matches_schema( -32006: "skill_not_found", -32007: "skill_pin_conflict", -32008: "model_not_available", - -32009: "model_switch_in_turn", -32010: "config_field_readonly", -32011: "config_validation_error", -32012: "not_supported_in_v01", diff --git a/tests/test_tui_rpc_config.py b/tests/test_tui_rpc_config.py index 66e031a7..10061117 100644 --- a/tests/test_tui_rpc_config.py +++ b/tests/test_tui_rpc_config.py @@ -22,7 +22,6 @@ ConfigFieldReadonlyError, ConfigValidationError, ModelNotAvailableError, - ModelSwitchInTurnError, ) from raven.tui_rpc.methods.config import ( CONFIG_WRITABLE_KEYS, @@ -150,6 +149,20 @@ def __init__(self, provider: object, model: str) -> None: self.provider = provider self.model = model self.switches: list[tuple[object, str]] = [] + self.session_bindings: dict[str, object] = {} + self.provider_pool = None + + def session_model(self, session_key: str) -> str: + binding = self.session_bindings.get(session_key) + return binding.model if binding is not None else self.model + + def set_session_binding(self, session_key: str, binding: object) -> None: + self.session_bindings[session_key] = binding + + def set_default_binding(self, binding: object) -> None: + self.provider = binding.provider + self.model = binding.model + self.switches.append((binding.provider, binding.model)) def set_provider(self, provider: object, model: str) -> None: self.provider = provider @@ -163,7 +176,6 @@ async def test_config_set_model_reassigns_loop_and_persists(fake_home: Path, mon loop = _FakeLoop("old-prov", "old-model") new_provider = SimpleNamespace(name="new-prov") - monkeypatch.setattr(config_mod, "is_turn_active", lambda _key: False) monkeypatch.setattr(config_mod, "make_provider", lambda _cfg: new_provider) monkeypatch.setattr( config_mod, @@ -176,13 +188,14 @@ async def test_config_set_model_reassigns_loop_and_persists(fake_home: Path, mon "key": "model", "value": "anthropic/claude-opus-4-8", "provider": "anthropic", - "session_id": "tui:default", + "scope": "default", }, agent_loop_factory=lambda: loop, ) assert result["applied"] is True assert result["value"] == "anthropic/claude-opus-4-8" + assert result["scope"] == "default" assert loop.model == "anthropic/claude-opus-4-8" assert loop.provider is new_provider # Routed through set_provider, so everything holding the old provider @@ -207,20 +220,69 @@ async def test_config_set_model_bare_derives_provider(fake_home: Path) -> None: assert cfg["agents"]["defaults"]["provider"] == "anthropic" -async def test_config_set_model_rejected_during_active_turn(fake_home: Path, monkeypatch) -> None: +async def test_config_set_model_is_scoped_to_the_session_that_asked(fake_home: Path, monkeypatch) -> None: + """A session switching its own model must not move anyone else's, and must + not rewrite the default a new session starts on. + """ import raven.tui_rpc.methods.config as config_mod - monkeypatch.setattr(config_mod, "is_turn_active", lambda _key: True) + (fake_home / ".raven").mkdir() + (fake_home / ".raven" / "config.json").write_text( + json.dumps({"agents": {"defaults": {"model": "anthropic/claude-sonnet-4-5"}}}) + ) - with pytest.raises(ModelSwitchInTurnError): - await config_set( - { - "key": "model", - "value": "anthropic/claude-opus-4-8", - "session_id": "tui:default", - }, - agent_loop_factory=lambda: SimpleNamespace(provider=None, model="x"), - ) + new_provider = SimpleNamespace(name="new-prov") + loop = _FakeLoop("old-prov", "anthropic/claude-sonnet-4-5") + + monkeypatch.setattr(config_mod, "make_provider", lambda _cfg: new_provider) + monkeypatch.setattr( + config_mod, + "load_runtime_config", + lambda *a, **k: SimpleNamespace(agents=SimpleNamespace(defaults=SimpleNamespace(model="", provider="auto"))), + ) + + result = await config_set( + { + "key": "model", + "value": "anthropic/claude-opus-4-8", + "provider": "anthropic", + "session_id": "tui:a", + }, + agent_loop_factory=lambda: loop, + ) + + assert result["scope"] == "session" + assert result["session_id"] == "tui:a" + assert loop.session_bindings["tui:a"].model == "anthropic/claude-opus-4-8" + assert "tui:b" not in loop.session_bindings, "another session must not move" + assert loop.switches == [], "a session switch is not a default change" + + on_disk = json.loads((fake_home / ".raven" / "config.json").read_text()) + assert on_disk["agents"]["defaults"]["model"] == "anthropic/claude-sonnet-4-5", ( + "a new session must still start on the configured default" + ) + + +async def test_config_set_model_is_not_refused_mid_turn(fake_home: Path, monkeypatch) -> None: + """The running turn holds the binding it started on, so the switch lands on + the session's next turn rather than being rejected. + """ + import raven.tui_rpc.methods.config as config_mod + + new_provider = SimpleNamespace(name="new-prov") + loop = _FakeLoop("old-prov", "old-model") + monkeypatch.setattr(config_mod, "make_provider", lambda _cfg: new_provider) + monkeypatch.setattr( + config_mod, + "load_runtime_config", + lambda *a, **k: SimpleNamespace(agents=SimpleNamespace(defaults=SimpleNamespace(model="", provider="auto"))), + ) + + result = await config_set( + {"key": "model", "value": "anthropic/claude-opus-4-8", "provider": "anthropic", "session_id": "tui:busy"}, + agent_loop_factory=lambda: loop, + ) + assert result["applied"] is True async def test_config_set_model_unconstructable_preserves_previous(fake_home: Path, monkeypatch) -> None: @@ -234,7 +296,6 @@ async def test_config_set_model_unconstructable_preserves_previous(fake_home: Pa def _boom(_cfg): raise RuntimeError("no api key") - monkeypatch.setattr(config_mod, "is_turn_active", lambda _key: False) monkeypatch.setattr(config_mod, "make_provider", _boom) monkeypatch.setattr( config_mod, @@ -439,3 +500,104 @@ async def test_a_bare_id_the_pinned_provider_lists_itself_keeps_the_pin(fake_hom assert result["applied"] is True cfg = json.loads((fake_home / ".raven" / "config.json").read_text()) assert cfg["agents"]["defaults"]["provider"] == "mistral" + + +async def test_a_session_switch_is_written_to_the_session_record(fake_home: Path, monkeypatch, tmp_path) -> None: + """The in-memory override dies with the process, so the record is the only + place the choice survives -- and a write nobody reads is worse than none. + """ + import raven.tui_rpc.methods.config as config_mod + from raven.session.manager import SessionManager + + new_provider = SimpleNamespace(name="new-prov") + loop = _FakeLoop("old-prov", "old-model") + loop.sessions = SessionManager(tmp_path) + + monkeypatch.setattr(config_mod, "make_provider", lambda _cfg: new_provider) + monkeypatch.setattr( + config_mod, + "load_runtime_config", + lambda *a, **k: SimpleNamespace(agents=SimpleNamespace(defaults=SimpleNamespace(model="", provider="auto"))), + ) + + await config_set( + { + "key": "model", + "value": "anthropic/claude-opus-4-8", + "provider": "anthropic", + "session_id": "tui:a", + }, + agent_loop_factory=lambda: loop, + ) + + stored = loop.sessions.peek("tui:a") + assert stored is not None + assert stored.metadata["model"] == "anthropic/claude-opus-4-8" + assert stored.metadata["provider"] == "anthropic" + + +async def test_a_session_switch_reports_the_model_it_replaced(fake_home: Path, monkeypatch) -> None: + """``previous`` is the session's own model, not the global default.""" + import raven.tui_rpc.methods.config as config_mod + + loop = _FakeLoop("old-prov", "boot-model") + loop.session_bindings["tui:a"] = SimpleNamespace(provider=object(), model="was-on-this") + + monkeypatch.setattr(config_mod, "make_provider", lambda _cfg: SimpleNamespace(name="new-prov")) + monkeypatch.setattr( + config_mod, + "load_runtime_config", + lambda *a, **k: SimpleNamespace(agents=SimpleNamespace(defaults=SimpleNamespace(model="", provider="auto"))), + ) + + result = await config_set( + {"key": "model", "value": "anthropic/claude-opus-4-8", "provider": "anthropic", "session_id": "tui:a"}, + agent_loop_factory=lambda: loop, + ) + + assert result["previous"] == "was-on-this" + + +async def test_an_unknown_scope_is_rejected(fake_home: Path) -> None: + """A client typo must not silently degrade to a session switch.""" + with pytest.raises(ConfigValidationError): + await config_set( + {"key": "model", "value": "anthropic/claude-opus-4-8", "session_id": "tui:a", "scope": "globl"}, + agent_loop_factory=None, + ) + + +async def test_a_switch_goes_through_the_pool_when_the_loop_has_one(fake_home: Path, monkeypatch) -> None: + """The pool is what makes a switch reuse a provider instead of rebuilding + one per switch; without this the production path is never exercised. + """ + import raven.tui_rpc.methods.config as config_mod + + asked: list[tuple[str, str | None]] = [] + pooled = SimpleNamespace(provider=SimpleNamespace(name="pooled"), model="anthropic/claude-opus-4-8") + + class _Pool: + def bind(self, model: str, provider_name: str | None = None): + asked.append((model, provider_name)) + return pooled + + loop = _FakeLoop("old-prov", "old-model") + loop.provider_pool = _Pool() + + def _must_not_build(_cfg): + raise AssertionError("a loop with a pool must not build its own provider") + + monkeypatch.setattr(config_mod, "make_provider", _must_not_build) + monkeypatch.setattr( + config_mod, + "load_runtime_config", + lambda *a, **k: SimpleNamespace(agents=SimpleNamespace(defaults=SimpleNamespace(model="", provider="auto"))), + ) + + await config_set( + {"key": "model", "value": "anthropic/claude-opus-4-8", "provider": "anthropic", "session_id": "tui:a"}, + agent_loop_factory=lambda: loop, + ) + + assert asked == [("anthropic/claude-opus-4-8", "anthropic")] + assert loop.session_bindings["tui:a"] is pooled diff --git a/tests/test_tui_rpc_model.py b/tests/test_tui_rpc_model.py index e3f5c825..d2ac7761 100644 --- a/tests/test_tui_rpc_model.py +++ b/tests/test_tui_rpc_model.py @@ -622,3 +622,42 @@ def test_every_provider_stores_a_model_id_that_finds_it_again(spec) -> None: resolved = find_by_model(stored) assert resolved is not None and resolved.name == spec.name, f"{stored} resolves to {resolved and resolved.name}" + + +async def test_options_reports_the_session_model_when_that_session_switched() -> None: + """The picker sits under the status bar; reading the global default here is + how they end up showing two models for one conversation. + """ + from types import SimpleNamespace + + from raven.tui_rpc.methods.model import model_options + + loop = SimpleNamespace( + has_session_binding=lambda key: key == "tui:a", + session_model=lambda key: "anthropic/claude-opus-4-8", + ) + + result = await model_options({"session_id": "tui:a"}, agent_loop_factory=lambda: loop) + + assert result["model"] == "anthropic/claude-opus-4-8" + assert result["provider"] == "anthropic" + + +async def test_options_leaves_an_unswitched_session_on_the_configured_answer() -> None: + """``session_model`` falls back to the default, so asking it alone would + override a forced ``agents.defaults.provider`` for every session. + """ + from types import SimpleNamespace + + from raven.tui_rpc.methods.model import _current_selection, model_options + + configured_model, configured_provider = _current_selection() + loop = SimpleNamespace( + has_session_binding=lambda key: False, + session_model=lambda key: "anthropic/claude-opus-4-8", + ) + + result = await model_options({"session_id": "tui:b"}, agent_loop_factory=lambda: loop) + + assert result["model"] == configured_model + assert result["provider"] == (configured_provider or "") diff --git a/tests/test_tui_rpc_session.py b/tests/test_tui_rpc_session.py index 53949e0e..5cd20188 100644 --- a/tests/test_tui_rpc_session.py +++ b/tests/test_tui_rpc_session.py @@ -1138,3 +1138,83 @@ async def test_session_export_is_read_only_during_active_turn(tmp_path: Path, mo result = await session_export({"session_id": session_key}) assert result["exported"] is True + + +async def test_session_info_reports_this_sessions_model_not_the_default(monkeypatch) -> None: + """The picker and the status bar sit next to each other; reporting the + global default here would show two models for one conversation. + """ + from unittest.mock import MagicMock + + import raven.tui_rpc.methods.session as session_mod + + # MagicMock so the unrelated skills/tools enumeration in the bundle works; + # only ``session_model`` is under test. + loop = MagicMock() + loop.session_model = lambda key: "vendor-a/model" if key == "tui:a" else "boot/model" + info = session_mod._default_session_info(loop, session_mod.load_config(), "tui:a") + + assert info["model"] == "vendor-a/model" + assert info["model_id"] == "vendor-a/model" + + +async def test_session_info_without_a_session_reports_the_default() -> None: + """A session being created has no model of its own yet; the default is the + right answer, because that is what it will start on. + """ + from unittest.mock import MagicMock + + import raven.tui_rpc.methods.session as session_mod + + config = session_mod.load_config() + loop = MagicMock() + loop.session_model = lambda key: "vendor-a/model" + info = session_mod._default_session_info(loop, config, None) + + assert info["model"] == config.agents.defaults.model + + +async def test_session_resume_puts_the_session_back_on_its_stored_model(tmp_path) -> None: + """The handler, not the helper: a resume that stops passing the session key + would leave every restored session on the default with the suite green. + """ + from unittest.mock import MagicMock + + from raven.session.manager import SessionManager + from raven.tui_rpc.methods.session import session_resume + + sessions = SessionManager(tmp_path) + record = sessions.get_or_create("tui:a") + record.metadata["model"] = "vendor-a/model" + record.metadata["provider"] = "anthropic" + sessions.save(record) + + restored: list[tuple[str, str, str | None]] = [] + loop = MagicMock() + loop.sessions = sessions + loop.restore_session_model = lambda key, model, provider=None: restored.append((key, model, provider)) + loop.session_model = lambda key: "vendor-a/model" + + await session_resume({"session_id": "tui:a"}, agent_loop_factory=lambda: loop) + + assert restored == [("tui:a", "vendor-a/model", "anthropic")] + + +async def test_session_resume_without_a_stored_model_restores_nothing(tmp_path) -> None: + from unittest.mock import MagicMock + + from raven.session.manager import SessionManager + from raven.tui_rpc.methods.session import session_resume + + sessions = SessionManager(tmp_path) + sessions.save(sessions.get_or_create("tui:a")) + + restored: list[object] = [] + loop = MagicMock() + loop.sessions = sessions + loop.restore_session_model = lambda *a, **k: restored.append(a) + loop.session_model = lambda key: "boot/model" + + await session_resume({"session_id": "tui:a"}, agent_loop_factory=lambda: loop) + + assert restored == [] diff --git a/ui-tui/CONTEXT.md b/ui-tui/CONTEXT.md index 184d7874..e0aa8f2d 100644 --- a/ui-tui/CONTEXT.md +++ b/ui-tui/CONTEXT.md @@ -9,6 +9,14 @@ talks to the Runtime only via TUI-RPC. Single-session per client in v0.1. ## Language +**Model scope** (TUI): +Which conversations a `/model` switch reaches. Plain `/model ` is +session-scoped: it moves this conversation only and does not touch the +configured default, so a new session still starts where it always did. +`/model --default` changes that default instead, leaving conversations +that already chose their own model alone. The picker shows the scope it will +use. _Avoid_: "global model switch" -- that was the pre-session behaviour. + **Overlay**: A modal layer over the chat view, tracked in `overlayStore` and driven by keyboard. Kinds split into RPC-driven (Confirm, Approval, Clarify, Sudo, Secret) and user-toggled (Agents, diff --git a/ui-tui/rpc-schema/openrpc.json b/ui-tui/rpc-schema/openrpc.json index 6baa90c9..b38f05df 100644 --- a/ui-tui/rpc-schema/openrpc.json +++ b/ui-tui/rpc-schema/openrpc.json @@ -876,6 +876,24 @@ "name": "value", "required": true, "schema": { "$ref": "#/components/schemas/JsonValue" } + }, + { + "name": "session_id", + "summary": "Scopes a model switch to one conversation.", + "required": false, + "schema": { "type": "string" } + }, + { + "name": "provider", + "summary": "Vendor for a model switch; derived from the id when absent.", + "required": false, + "schema": { "type": "string" } + }, + { + "name": "scope", + "summary": "How far a model switch reaches: this conversation, or what new ones start on.", + "required": false, + "schema": { "type": "string", "enum": ["session", "default"] } } ], "result": { @@ -891,7 +909,10 @@ { "$ref": "#/components/schemas/JsonValue" }, { "type": "null" } ] - } + }, + "value": { "type": "string" }, + "scope": { "type": "string", "enum": ["session", "default"] }, + "session_id": { "type": "string" } } } }, @@ -1753,10 +1774,6 @@ "code": -32008, "message": "model_not_available" }, - "ModelSwitchInTurn": { - "code": -32009, - "message": "model_switch_in_turn" - }, "ConfigFieldReadonly": { "code": -32010, "message": "config_field_readonly" diff --git a/ui-tui/src/__tests__/createSlashHandler.test.ts b/ui-tui/src/__tests__/createSlashHandler.test.ts index d9143938..bdfebb58 100644 --- a/ui-tui/src/__tests__/createSlashHandler.test.ts +++ b/ui-tui/src/__tests__/createSlashHandler.test.ts @@ -65,10 +65,56 @@ describe('createSlashHandler', () => { expect(ctx.gateway.rpc).toHaveBeenCalledWith('config.set', { key: 'model', session_id: 'sid-abc', + scope: 'session', value: 'x-model' }) }) + it('sends scope default and leaves the status bar alone for /model --default', async () => { + patchUiState({ sid: 'sid-abc', info: { model: 'session-model', skills: {}, tools: {} } }) + + const ctx = buildCtx({ + gateway: { + ...buildGateway(), + rpc: vi.fn(() => Promise.resolve({ applied: true, previous: null, value: 'new-default' })) + } + }) + + expect(createSlashHandler(ctx)('/model new-default --default')).toBe(true) + expect(ctx.gateway.rpc).toHaveBeenCalledWith('config.set', { + key: 'model', + session_id: 'sid-abc', + scope: 'default', + value: 'new-default' + }) + await Promise.resolve() + await Promise.resolve() + // The session keeps its own model, so painting the new default into the + // status bar would show a model this conversation is not on. + expect(getUiState().info?.model).toBe('session-model') + }) + + it('strips --default from any position and opens the picker when nothing is left', async () => { + patchUiState({ sid: 'sid-abc' }) + + const ctx = buildCtx({ + gateway: { + ...buildGateway(), + rpc: vi.fn(() => Promise.resolve({ applied: true, previous: null, value: 'leading' })) + } + }) + + expect(createSlashHandler(ctx)('/model --default leading')).toBe(true) + expect(ctx.gateway.rpc).toHaveBeenCalledWith( + 'config.set', + expect.objectContaining({ value: 'leading', scope: 'default' }) + ) + + const picker = buildCtx({ gateway: { ...buildGateway(), rpc: vi.fn() } }) + expect(createSlashHandler(picker)('/model --default')).toBe(true) + expect(picker.gateway.rpc).not.toHaveBeenCalled() + }) + it('parses a --provider suffix into a structured provider param', async () => { patchUiState({ sid: 'sid-abc' }) @@ -84,6 +130,7 @@ describe('createSlashHandler', () => { key: 'model', provider: 'openrouter', session_id: 'sid-abc', + scope: 'session', value: 'claude-sonnet-4.6' }) }) diff --git a/ui-tui/src/app/slash/commands/session.ts b/ui-tui/src/app/slash/commands/session.ts index 6f5f956e..709f5d08 100644 --- a/ui-tui/src/app/slash/commands/session.ts +++ b/ui-tui/src/app/slash/commands/session.ts @@ -24,7 +24,7 @@ import { DEFAULT_INDICATOR_STYLE, INDICATOR_STYLES, type IndicatorStyle } from ' import { patchOverlayState } from '../../overlayStore.js' import { patchUiState } from '../../uiStore.js' -// v1 model switch is global-scope only. The picker passes ` --provider +// A model switch is per conversation; `--default` also changes what new ones start on. The picker passes ` --provider // `; a bare `/model ` carries no provider. Parse both into the // structured config.set params {key:'model', value, provider?}. const parseModelArg = (arg: string): { provider?: string; value: string } => { @@ -69,20 +69,27 @@ export const sessionCommands: SlashCommand[] = [ help: 'change or show model', name: 'model', run: (arg, ctx) => { - if (ctx.session.guardBusySessionSwitch('change models')) { - return - } - - if (!arg.trim()) { + // No busy guard: the server captures the binding at turn entry, so a + // switch asked for mid-answer lands on the next turn instead of being + // refused. + // `--default` changes what new sessions start on; without it the switch + // is this conversation's alone. Stripped before parsing, in any + // position and however many times, so it never lands in the model id. + const raw = arg.trim() + const asDefault = /(^|\s)--default(\s|$)/.test(raw) + const rest = raw.replace(/(^|\s)--default(?=\s|$)/g, '').trim() + if (!rest) { + // `/model` and `/model --default` both mean "show me the choices". return patchOverlayState({ modelPicker: true }) } - const { provider, value } = parseModelArg(arg) + const { provider, value } = parseModelArg(rest) ctx.gateway .rpc('config.set', { key: 'model', session_id: ctx.sid, + scope: asDefault ? 'default' : 'session', value, ...(provider ? { provider } : {}) }) @@ -92,13 +99,18 @@ export const sessionCommands: SlashCommand[] = [ return ctx.transcript.sys('error: invalid response: model switch') } - ctx.transcript.sys(`model → ${r.value}`) + ctx.transcript.sys(asDefault ? `default model → ${r.value}` : `model → ${r.value}`) ctx.local.maybeWarn(r) - patchUiState(state => ({ - ...state, - info: state.info ? { ...state.info, model: r.value! } : { model: r.value!, skills: {}, tools: {} } - })) + // A default-scoped switch does not move a session that already has + // its own model, so painting it into the status bar would show a + // model this conversation is not on. + if (!asDefault) { + patchUiState(state => ({ + ...state, + info: state.info ? { ...state.info, model: r.value! } : { model: r.value!, skills: {}, tools: {} } + })) + } }) ) } diff --git a/ui-tui/src/components/modelPicker.tsx b/ui-tui/src/components/modelPicker.tsx index 6610d7f9..6c8c5056 100644 --- a/ui-tui/src/components/modelPicker.tsx +++ b/ui-tui/src/components/modelPicker.tsx @@ -1032,7 +1032,7 @@ export function ModelPicker({ gw, launcher, onCancel, onSelect, sessionId, suspe - scope: global + {'scope: this conversation · /model --default sets the new-session default'} {models.length diff --git a/ui-tui/src/rpc/errors.ts b/ui-tui/src/rpc/errors.ts index a2d93b2d..1103f911 100644 --- a/ui-tui/src/rpc/errors.ts +++ b/ui-tui/src/rpc/errors.ts @@ -70,12 +70,6 @@ export class ModelNotAvailableError extends RpcError { this.name = 'ModelNotAvailableError' } } -export class ModelSwitchInTurnError extends RpcError { - constructor(f: JsonRpcErrorObject) { - super(f) - this.name = 'ModelSwitchInTurnError' - } -} export class ConfigFieldReadonlyError extends RpcError { constructor(f: JsonRpcErrorObject) { super(f) @@ -124,7 +118,6 @@ const CODE_TO_CTOR: Record RpcError> = { [-32006]: SkillNotFoundError, [-32007]: SkillPinConflictError, [-32008]: ModelNotAvailableError, - [-32009]: ModelSwitchInTurnError, [-32010]: ConfigFieldReadonlyError, [-32011]: ConfigValidationError, [-32012]: NotSupportedInV01Error, diff --git a/ui-tui/src/rpc/generated.ts b/ui-tui/src/rpc/generated.ts index 6ee8f91b..a6f8bac7 100644 --- a/ui-tui/src/rpc/generated.ts +++ b/ui-tui/src/rpc/generated.ts @@ -872,6 +872,9 @@ export interface ConfigGetResult { export interface ConfigSetParams { key: string; value: JsonValue; + session_id?: string; + provider?: string; + scope?: 'session' | 'default'; } /** * This interface was referenced by `RavenRpcRoot`'s JSON-Schema @@ -880,6 +883,9 @@ export interface ConfigSetParams { export interface ConfigSetResult { applied: boolean; previous: JsonValue | null; + value?: string; + scope?: 'session' | 'default'; + session_id?: string; } /** * This interface was referenced by `RavenRpcRoot`'s JSON-Schema diff --git a/ui-tui/src/rpc/index.ts b/ui-tui/src/rpc/index.ts index 888c95ca..0dc5e5f3 100644 --- a/ui-tui/src/rpc/index.ts +++ b/ui-tui/src/rpc/index.ts @@ -16,7 +16,6 @@ export { SkillNotFoundError, SkillPinConflictError, ModelNotAvailableError, - ModelSwitchInTurnError, ConfigFieldReadonlyError, ConfigValidationError, NotSupportedInV01Error, From 673050aaf7376e1b37c2d9ffefe66adb9e74822c Mon Sep 17 00:00:00 2001 From: arelchan <204152633+arelchan@users.noreply.github.com> Date: Mon, 10 Aug 2026 00:14:56 +0800 Subject: [PATCH 7/8] fix(*): keep a model switch inside the scope the caller asked for An explicit scope="session" carrying no session id fell through to the default branch, so it wrote agents.defaults.model and moved every session that never switched -- reachable from the TUI, whose session_id is null until the first session.create resolves and after a failed one. It is refused now rather than widened. A fork re-pointed its parent's live binding but never copied the record, so a branched session kept its model until the first restart and then dropped to the default. SessionManager.fork carries model and provider, which covers every caller rather than only the RPC handler. /model --default refused to repaint the status bar, on the theory that a default-scoped switch cannot move the asking session. That holds only for sessions which already chose their own model; a fresh conversation reads the default and does move, and it is the common case for that command. Whether it moved is now the server's answer (applies_to_session) rather than something the client infers from the scope, and an unapplied switch is reported as an error instead of being drawn as a success. Also closes the test gaps a review round found: the conjunct that routes --default, the fork inheritance, the binding released on session.delete and the production registration that makes the picker session-aware were each removable without turning anything red. One stale assertion message named a helper that no longer exists. Co-authored-by: Claude (claude-opus-5) --- raven/session/manager.py | 8 + raven/tui_rpc/methods/config.py | 31 +++- raven/tui_rpc/models.py | 4 + tests/test_agent_loop_model_switch.py | 18 +-- tests/test_session_manager.py | 33 +++++ tests/test_tui_rpc_config.py | 138 ++++++++++++++++++ tests/test_tui_rpc_model.py | 47 ++++++ tests/test_tui_rpc_session.py | 73 +++++++++ ui-tui/CONTEXT.md | 8 +- ui-tui/rpc-schema/openrpc.json | 3 +- .../src/__tests__/createSlashHandler.test.ts | 61 +++++++- ui-tui/src/app/slash/commands/session.ts | 14 +- ui-tui/src/gatewayTypes.ts | 6 + ui-tui/src/rpc/generated.ts | 1 + 14 files changed, 415 insertions(+), 30 deletions(-) diff --git a/raven/session/manager.py b/raven/session/manager.py index 6dda0cb3..132da6bd 100644 --- a/raven/session/manager.py +++ b/raven/session/manager.py @@ -440,6 +440,14 @@ def fork(self, source_key: str, *, title: str | None = None) -> "Session | None" if parent_title: child.metadata["title"] = f"{parent_title} (fork)" child.metadata["parent_session_id"] = source_key + # A fork continues its parent's conversation, so it continues on its + # parent's model. The caller re-points the live binding, but that lives + # in memory only -- without carrying the record too, the fork drops to + # the default the first time it is resumed in a new process. + for slot in ("model", "provider"): + inherited = (source.metadata or {}).get(slot) + if inherited: + child.metadata[slot] = inherited self.save(child) return child diff --git a/raven/tui_rpc/methods/config.py b/raven/tui_rpc/methods/config.py index 1f5767c0..c5c13e1a 100644 --- a/raven/tui_rpc/methods/config.py +++ b/raven/tui_rpc/methods/config.py @@ -377,7 +377,17 @@ def _set_model( "config.set model scope must be 'session' or 'default'", data={"field": "scope", "got": repr(scope)}, ) - session_scoped = scope != "default" and isinstance(session_id, str) and bool(session_id) + has_session = isinstance(session_id, str) and bool(session_id) + if scope == "session" and not has_session: + # Never widen a scope the caller narrowed: falling through to the + # default branch here would write agents.defaults and move every + # session that never switched. The TUI sends a session_id that is null + # until the first session.create resolves, so this is reachable. + raise ConfigValidationError( + "config.set model scope 'session' needs a session_id", + data={"field": "session_id", "got": repr(session_id)}, + ) + session_scoped = scope != "default" and has_session loop = agent_loop_factory() if agent_loop_factory is not None else None binding = None @@ -408,8 +418,19 @@ def _set_model( "value": raw_value, "scope": "session", "session_id": session_id, + "applies_to_session": True, } + # A default-scoped switch still moves the asking conversation when that + # conversation never chose a model of its own, because it reads the + # default. Answered here rather than inferred from the scope: the client + # cannot see which sessions have their own binding. + follows_default = None + if loop is not None and has_session: + has_own = getattr(loop, "has_session_binding", None) + if callable(has_own): + follows_default = not has_own(session_id) + payload = _load_config() previous = _get_nested(payload, "agents.defaults.model") _set_nested(payload, "agents.defaults.model", raw_value) @@ -423,7 +444,13 @@ def _set_model( # outside a turn, and this is what re-points them. loop.set_default_binding(binding) - return {"applied": True, "previous": previous, "value": raw_value, "scope": "default"} + return { + "applied": True, + "previous": previous, + "value": raw_value, + "scope": "default", + "applies_to_session": follows_default, + } def _remember_session_model(loop: Any, session_key: str, model: str, provider_name: str | None) -> None: diff --git a/raven/tui_rpc/models.py b/raven/tui_rpc/models.py index 2e4bd53d..0c944399 100644 --- a/raven/tui_rpc/models.py +++ b/raven/tui_rpc/models.py @@ -624,6 +624,10 @@ class ConfigSetResult(_Strict): value: str | None = None scope: Literal["session", "default"] | None = None session_id: str | None = None + # Does the asking conversation now run this model? A default-scoped switch + # moves the sessions that never chose one, so scope alone cannot answer it + # and a client that guesses shows a model the conversation is not on. + applies_to_session: bool | None = None # --------------------------------------------------------------------------- diff --git a/tests/test_agent_loop_model_switch.py b/tests/test_agent_loop_model_switch.py index 59c3d336..8ee8fb68 100644 --- a/tests/test_agent_loop_model_switch.py +++ b/tests/test_agent_loop_model_switch.py @@ -53,22 +53,6 @@ def set_provider(self, provider: object, model: str) -> None: self.model = model -class _StubExecutor: - """``_run_subagent_inner`` only passes this to ExecTool; no command runs.""" - - @property - def is_sandboxed(self) -> bool: - return False - - async def exec(self, command: str, **kwargs): # pragma: no cover - unused - raise NotImplementedError - - -def _noop_submit(*args, **kwargs) -> None: - """``_announce_result`` calls the spine submit without awaiting it.""" - return None - - class _TextOnlyBuilder: """A segment that never calls an LLM, so it has no set_provider.""" @@ -152,7 +136,7 @@ def test_fan_out_targets_still_exist_on_a_real_loop(tmp_path) -> None: loop = _loop(tmp_path) for attr in ("subagents", "context_engine", "memory_consolidator"): holder = getattr(loop, attr, None) - assert holder is not None, f"AgentLoop.{attr} is gone; _adopt_provider still calls it" + assert holder is not None, f"AgentLoop.{attr} is gone; set_provider still fans out to it" assert callable(getattr(holder, "set_provider", None)), f"AgentLoop.{attr} lost set_provider" diff --git a/tests/test_session_manager.py b/tests/test_session_manager.py index 6db033e8..0baf26d5 100644 --- a/tests/test_session_manager.py +++ b/tests/test_session_manager.py @@ -715,6 +715,39 @@ def test_fork_inherits_last_consolidated(tmp_path: Path): assert child.last_consolidated == 1 +def test_fork_inherits_the_model_the_parent_chose(tmp_path: Path): + """A fork continues its parent's conversation, so it continues on its model. + + The branch handler re-points the live binding, but that is in memory only. + Without the record the fork reads the default the first time it is resumed + in a new process, which is a silent downgrade rather than a visible one. + """ + mgr = SessionManager(tmp_path) + src = _seed(mgr, "cli:src08", ("user", "a")) + src.metadata["model"] = "anthropic/claude-opus-4-8" + src.metadata["provider"] = "anthropic" + mgr.save(src) + + child = mgr.fork("cli:src08") + + reloaded = SessionManager(tmp_path).get_or_create(child.key) + assert reloaded.metadata["model"] == "anthropic/claude-opus-4-8" + assert reloaded.metadata["provider"] == "anthropic" + + +def test_fork_of_an_unswitched_parent_carries_no_model(tmp_path: Path): + """The parent never chose one, so the fork must start on the default too -- + not on a model copied out of nowhere. + """ + mgr = SessionManager(tmp_path) + _seed(mgr, "cli:src09", ("user", "a")) + + child = mgr.fork("cli:src09") + + assert "model" not in child.metadata + assert "provider" not in child.metadata + + def test_fork_resets_pending_clarification(tmp_path: Path): """The child does not carry the source's clarification wait-state.""" mgr = SessionManager(tmp_path) diff --git a/tests/test_tui_rpc_config.py b/tests/test_tui_rpc_config.py index 10061117..018d8b38 100644 --- a/tests/test_tui_rpc_config.py +++ b/tests/test_tui_rpc_config.py @@ -156,6 +156,9 @@ def session_model(self, session_key: str) -> str: binding = self.session_bindings.get(session_key) return binding.model if binding is not None else self.model + def has_session_binding(self, session_key: str) -> bool: + return session_key in self.session_bindings + def set_session_binding(self, session_key: str, binding: object) -> None: self.session_bindings[session_key] = binding @@ -601,3 +604,138 @@ def _must_not_build(_cfg): assert asked == [("anthropic/claude-opus-4-8", "anthropic")] assert loop.session_bindings["tui:a"] is pooled + + +# ---------------------------------------------------------------------------- +# Scope is never widened +# ---------------------------------------------------------------------------- + + +async def test_a_session_scope_without_a_session_id_is_refused_not_widened(fake_home: Path, monkeypatch) -> None: + """An explicit ``scope="session"`` with no session must not fall through to + the default branch. + + The TUI sends ``session_id: ctx.sid``, which is null until the first + ``session.create`` resolves and after a failed one. Widening the scope + there rewrites ``agents.defaults.model`` on disk and moves every session + that never chose its own model -- from a request that asked for the + opposite. + """ + import raven.tui_rpc.methods.config as config_mod + + (fake_home / ".raven").mkdir() + (fake_home / ".raven" / "config.json").write_text( + json.dumps({"agents": {"defaults": {"model": "anthropic/claude-sonnet-4-5"}}}) + ) + + loop = _FakeLoop("old-prov", "anthropic/claude-sonnet-4-5") + monkeypatch.setattr(config_mod, "make_provider", lambda _cfg: SimpleNamespace(name="new-prov")) + monkeypatch.setattr( + config_mod, + "load_runtime_config", + lambda *a, **k: SimpleNamespace(agents=SimpleNamespace(defaults=SimpleNamespace(model="", provider="auto"))), + ) + + for absent in (None, ""): + with pytest.raises(ConfigValidationError): + await config_set( + { + "key": "model", + "value": "anthropic/claude-opus-4-8", + "scope": "session", + "session_id": absent, + }, + agent_loop_factory=lambda: loop, + ) + + assert loop.switches == [], "a refused switch must not move the default binding" + assert loop.session_bindings == {} + on_disk = json.loads((fake_home / ".raven" / "config.json").read_text()) + assert on_disk["agents"]["defaults"]["model"] == "anthropic/claude-sonnet-4-5" + + +async def test_a_default_scope_with_a_session_id_still_writes_the_default(fake_home: Path, monkeypatch) -> None: + """``/model X --default`` sends both ``scope="default"`` and the caller's + session id, and the scope has to win. + + Without the scope conjunct the session id alone decides, and the switch + silently becomes an override on the asking session -- the file is never + written, so nothing a new session starts on ever changes. + """ + import raven.tui_rpc.methods.config as config_mod + + (fake_home / ".raven").mkdir() + (fake_home / ".raven" / "config.json").write_text( + json.dumps({"agents": {"defaults": {"model": "anthropic/claude-sonnet-4-5"}}}) + ) + + new_provider = SimpleNamespace(name="new-prov") + loop = _FakeLoop("old-prov", "anthropic/claude-sonnet-4-5") + monkeypatch.setattr(config_mod, "make_provider", lambda _cfg: new_provider) + monkeypatch.setattr( + config_mod, + "load_runtime_config", + lambda *a, **k: SimpleNamespace(agents=SimpleNamespace(defaults=SimpleNamespace(model="", provider="auto"))), + ) + + result = await config_set( + { + "key": "model", + "value": "anthropic/claude-opus-4-8", + "provider": "anthropic", + "scope": "default", + "session_id": "tui:a", + }, + agent_loop_factory=lambda: loop, + ) + + assert result["scope"] == "default" + assert loop.switches == [(new_provider, "anthropic/claude-opus-4-8")] + assert "tui:a" not in loop.session_bindings, "a default switch is not a session override" + on_disk = json.loads((fake_home / ".raven" / "config.json").read_text()) + assert on_disk["agents"]["defaults"]["model"] == "anthropic/claude-opus-4-8" + + +async def test_a_default_switch_reports_whether_it_moved_the_asking_session(fake_home: Path, monkeypatch) -> None: + """The scope alone cannot tell a client whether to repaint the status bar. + + A session that never chose a model reads the default, so a default-scoped + switch moves it; a session with its own binding stays where it is. The + client cannot see the difference, so the server answers it. + """ + import raven.tui_rpc.methods.config as config_mod + + monkeypatch.setattr(config_mod, "make_provider", lambda _cfg: SimpleNamespace(name="new-prov")) + monkeypatch.setattr( + config_mod, + "load_runtime_config", + lambda *a, **k: SimpleNamespace(agents=SimpleNamespace(defaults=SimpleNamespace(model="", provider="auto"))), + ) + + loop = _FakeLoop("old-prov", "old-model") + loop.session_bindings["tui:chose"] = SimpleNamespace(provider="own-prov", model="own/model") + + params = {"key": "model", "value": "anthropic/claude-opus-4-8", "provider": "anthropic", "scope": "default"} + + followed = await config_set({**params, "session_id": "tui:followed"}, agent_loop_factory=lambda: loop) + assert followed["applies_to_session"] is True + + chose = await config_set({**params, "session_id": "tui:chose"}, agent_loop_factory=lambda: loop) + assert chose["applies_to_session"] is False + + +async def test_a_session_switch_always_applies_to_its_own_session(fake_home: Path, monkeypatch) -> None: + import raven.tui_rpc.methods.config as config_mod + + monkeypatch.setattr(config_mod, "make_provider", lambda _cfg: SimpleNamespace(name="new-prov")) + monkeypatch.setattr( + config_mod, + "load_runtime_config", + lambda *a, **k: SimpleNamespace(agents=SimpleNamespace(defaults=SimpleNamespace(model="", provider="auto"))), + ) + + result = await config_set( + {"key": "model", "value": "anthropic/claude-opus-4-8", "session_id": "tui:a"}, + agent_loop_factory=lambda: _FakeLoop("old-prov", "old-model"), + ) + assert result["applies_to_session"] is True diff --git a/tests/test_tui_rpc_model.py b/tests/test_tui_rpc_model.py index d2ac7761..b0e64dd4 100644 --- a/tests/test_tui_rpc_model.py +++ b/tests/test_tui_rpc_model.py @@ -661,3 +661,50 @@ async def test_options_leaves_an_unswitched_session_on_the_configured_answer() - assert result["model"] == configured_model assert result["provider"] == (configured_provider or "") + + +async def test_the_production_registration_makes_model_options_session_aware(fake_home: Path) -> None: + """The picker asks ``model.options`` which model to show as current, and the + answer is per session. + + Registered through the umbrella the production path uses, not by calling + the handler with a factory by hand: without the factory threaded here the + picker reports the configured default to every session, and calling + ``model_options`` directly would never notice. + """ + from raven.tui_rpc.dispatcher import Dispatcher + from raven.tui_rpc.methods import register_aligned_methods_except_system + + _write_config(fake_home, {"agents": {"defaults": {"model": "anthropic/claude-sonnet-4-5"}}}) + + class _Loop: + def has_session_binding(self, session_id: str) -> bool: + return session_id == "tui:switched" + + def session_model(self, session_id: str) -> str: + return "anthropic/claude-opus-4-8" + + d = Dispatcher() + register_aligned_methods_except_system(d, agent_loop_factory=lambda: _Loop()) + + switched = await d.dispatch( + { + "jsonrpc": "2.0", + "id": 1, + "method": "model.options", + "params": {"session_id": "tui:switched"}, + } + ) + assert switched["result"]["model"] == "anthropic/claude-opus-4-8" + + untouched = await d.dispatch( + { + "jsonrpc": "2.0", + "id": 2, + "method": "model.options", + "params": {"session_id": "tui:never-switched"}, + } + ) + assert untouched["result"]["model"] == "anthropic/claude-sonnet-4-5", ( + "a session that never switched still reports the configured default" + ) diff --git a/tests/test_tui_rpc_session.py b/tests/test_tui_rpc_session.py index 5cd20188..946da721 100644 --- a/tests/test_tui_rpc_session.py +++ b/tests/test_tui_rpc_session.py @@ -17,6 +17,7 @@ import json import re from pathlib import Path +from types import SimpleNamespace import pytest @@ -1218,3 +1219,75 @@ async def test_session_resume_without_a_stored_model_restores_nothing(tmp_path) await session_resume({"session_id": "tui:a"}, agent_loop_factory=lambda: loop) assert restored == [] + + +async def test_session_branch_carries_the_parents_model_to_the_child( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A fork continues its parent's conversation on its parent's model, and + keeps it across a restart. + + Both halves matter and fail independently: dropping the binding hand-off + leaves the child running on the default in this process, and dropping the + record leaves it running on the default in the next one. + """ + cfg = load_config() + cfg.agents.defaults.workspace = str(tmp_path) + monkeypatch.setattr(session_module, "load_config", lambda: cfg) + src_key = "tui:20260610_143052_bb0001" + _write_session(tmp_path, src_key, [{"role": "user", "content": "hi"}]) + + sessions = SessionManager(tmp_path) + parent = sessions.get_or_create(src_key) + parent.metadata["model"] = "vendor-a/model" + parent.metadata["provider"] = "anthropic" + sessions.save(parent) + + parent_binding = SimpleNamespace(provider="prov-a", model="vendor-a/model") + + class _Loop: + def __init__(self) -> None: + self.sessions = sessions + self.bindings: dict[str, object] = {src_key: parent_binding} + + def has_session_binding(self, key: str) -> bool: + return key in self.bindings + + def binding_for_session(self, key: str) -> object: + return self.bindings.get(key, SimpleNamespace(provider="boot", model="boot/model")) + + def set_session_binding(self, key: str, binding: object) -> None: + self.bindings[key] = binding + + loop = _Loop() + result = await session_branch({"session_id": src_key}, agent_loop_factory=lambda: loop) + + child_key = result["session_id"] + assert loop.bindings[child_key] is parent_binding, "the fork must run on its parent's model now" + + reloaded = SessionManager(tmp_path).get_or_create(child_key) + assert reloaded.metadata["model"] == "vendor-a/model", "and after a restart" + assert reloaded.metadata["provider"] == "anthropic" + + +async def test_session_delete_releases_the_sessions_binding(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """A deleted session must not leave its override -- and the live provider + behind it -- held for the life of the process. + """ + cfg = load_config() + cfg.agents.defaults.workspace = str(tmp_path) + monkeypatch.setattr(session_module, "load_config", lambda: cfg) + key = "tui:20260610_100000_bb0002" + _write_session(tmp_path, key, [{"role": "user", "content": "hi"}]) + + cleared: list[str] = [] + + class _Loop: + sessions = None + + def clear_session_binding(self, session_key: str) -> None: + cleared.append(session_key) + + await session_delete({"session_id": key}, agent_loop_factory=lambda: _Loop()) + + assert cleared == [key] diff --git a/ui-tui/CONTEXT.md b/ui-tui/CONTEXT.md index e0aa8f2d..7d6d1a85 100644 --- a/ui-tui/CONTEXT.md +++ b/ui-tui/CONTEXT.md @@ -14,8 +14,12 @@ Which conversations a `/model` switch reaches. Plain `/model ` is session-scoped: it moves this conversation only and does not touch the configured default, so a new session still starts where it always did. `/model --default` changes that default instead, leaving conversations -that already chose their own model alone. The picker shows the scope it will -use. _Avoid_: "global model switch" -- that was the pre-session behaviour. +that already chose their own model alone -- but it does move the ones that +never chose, including, usually, the conversation that asked. Which of the two +happened is the server's answer (`applies_to_session`), not something the scope +implies, and it is what decides whether the status bar repaints. The picker +shows the scope it will use. +_Avoid_: "global model switch" -- that was the pre-session behaviour. **Overlay**: A modal layer over the chat view, tracked in `overlayStore` and driven by keyboard. Kinds diff --git a/ui-tui/rpc-schema/openrpc.json b/ui-tui/rpc-schema/openrpc.json index b38f05df..2f688cfa 100644 --- a/ui-tui/rpc-schema/openrpc.json +++ b/ui-tui/rpc-schema/openrpc.json @@ -912,7 +912,8 @@ }, "value": { "type": "string" }, "scope": { "type": "string", "enum": ["session", "default"] }, - "session_id": { "type": "string" } + "session_id": { "type": "string" }, + "applies_to_session": { "type": "boolean" } } } }, diff --git a/ui-tui/src/__tests__/createSlashHandler.test.ts b/ui-tui/src/__tests__/createSlashHandler.test.ts index bdfebb58..1d5da738 100644 --- a/ui-tui/src/__tests__/createSlashHandler.test.ts +++ b/ui-tui/src/__tests__/createSlashHandler.test.ts @@ -70,13 +70,21 @@ describe('createSlashHandler', () => { }) }) - it('sends scope default and leaves the status bar alone for /model --default', async () => { + it('sends scope default and leaves the status bar alone when the session kept its own model', async () => { patchUiState({ sid: 'sid-abc', info: { model: 'session-model', skills: {}, tools: {} } }) const ctx = buildCtx({ gateway: { ...buildGateway(), - rpc: vi.fn(() => Promise.resolve({ applied: true, previous: null, value: 'new-default' })) + rpc: vi.fn(() => + Promise.resolve({ + applied: true, + previous: null, + value: 'new-default', + scope: 'default', + applies_to_session: false + }) + ) } }) @@ -89,8 +97,53 @@ describe('createSlashHandler', () => { }) await Promise.resolve() await Promise.resolve() - // The session keeps its own model, so painting the new default into the - // status bar would show a model this conversation is not on. + // This conversation chose its own model, so painting the new default into + // the status bar would show a model it is not on. + expect(getUiState().info?.model).toBe('session-model') + }) + + it('updates the status bar for /model --default when the session was following the default', async () => { + // The common case: a fresh conversation that never switched reads the + // default, so a default-scoped switch moves it immediately. Leaving the bar + // alone here showed the old model for the life of the session, because the + // bar is only refreshed on session.create / session.resume. + patchUiState({ sid: 'sid-abc', info: { model: 'old-default', skills: {}, tools: {} } }) + + const ctx = buildCtx({ + gateway: { + ...buildGateway(), + rpc: vi.fn(() => + Promise.resolve({ + applied: true, + previous: null, + value: 'new-default', + scope: 'default', + applies_to_session: true + }) + ) + } + }) + + expect(createSlashHandler(ctx)('/model new-default --default')).toBe(true) + await Promise.resolve() + await Promise.resolve() + expect(getUiState().info?.model).toBe('new-default') + }) + + it('reports an unapplied switch as an error and leaves the status bar alone', async () => { + patchUiState({ sid: 'sid-abc', info: { model: 'session-model', skills: {}, tools: {} } }) + + const ctx = buildCtx({ + gateway: { + ...buildGateway(), + rpc: vi.fn(() => Promise.resolve({ applied: false, previous: null, value: 'x-model' })) + } + }) + + expect(createSlashHandler(ctx)('/model x-model')).toBe(true) + await Promise.resolve() + await Promise.resolve() + expect(ctx.transcript.sys).toHaveBeenCalledWith('error: model switch was not applied: x-model') expect(getUiState().info?.model).toBe('session-model') }) diff --git a/ui-tui/src/app/slash/commands/session.ts b/ui-tui/src/app/slash/commands/session.ts index 709f5d08..344c5846 100644 --- a/ui-tui/src/app/slash/commands/session.ts +++ b/ui-tui/src/app/slash/commands/session.ts @@ -98,14 +98,20 @@ export const sessionCommands: SlashCommand[] = [ if (!r.value) { return ctx.transcript.sys('error: invalid response: model switch') } + if (!r.applied) { + // Nothing was built, so nothing was validated -- reporting a + // switch here would leave the bar on a model no turn will use. + return ctx.transcript.sys(`error: model switch was not applied: ${r.value}`) + } ctx.transcript.sys(asDefault ? `default model → ${r.value}` : `model → ${r.value}`) ctx.local.maybeWarn(r) - // A default-scoped switch does not move a session that already has - // its own model, so painting it into the status bar would show a - // model this conversation is not on. - if (!asDefault) { + // Whether this conversation now runs the model is the server's + // answer, not something the scope implies: a default-scoped switch + // does move a session that never chose its own model, and that is + // the common case for `--default`. + if (r.applies_to_session !== false) { patchUiState(state => ({ ...state, info: state.info ? { ...state.info, model: r.value! } : { model: r.value!, skills: {}, tools: {} } diff --git a/ui-tui/src/gatewayTypes.ts b/ui-tui/src/gatewayTypes.ts index 612d003b..c5b275cd 100644 --- a/ui-tui/src/gatewayTypes.ts +++ b/ui-tui/src/gatewayTypes.ts @@ -102,10 +102,16 @@ export interface ConfigGetValueResponse { export interface ConfigSetResponse { applied?: boolean + // Does the asking conversation now run this model? A default-scoped switch + // moves the sessions that never chose one, so the scope alone cannot answer + // it and a client that guesses paints a model the conversation is not on. + applies_to_session?: boolean credential_warning?: string history_reset?: boolean info?: SessionInfo previous?: null | string + scope?: 'default' | 'session' + session_id?: string value?: string warning?: string } diff --git a/ui-tui/src/rpc/generated.ts b/ui-tui/src/rpc/generated.ts index a6f8bac7..0b572d32 100644 --- a/ui-tui/src/rpc/generated.ts +++ b/ui-tui/src/rpc/generated.ts @@ -886,6 +886,7 @@ export interface ConfigSetResult { value?: string; scope?: 'session' | 'default'; session_id?: string; + applies_to_session?: boolean; } /** * This interface was referenced by `RavenRpcRoot`'s JSON-Schema From 03982ae8c9ae7d39b8bb351af25d57f0417c84a9 Mon Sep 17 00:00:00 2001 From: arelchan <204152633+arelchan@users.noreply.github.com> Date: Mon, 10 Aug 2026 00:15:16 +0800 Subject: [PATCH 8/8] feat(providers): configure a subsystem pin as a provider and model pair context.curator_model and skill_forge.llm_gate_model took a model id and nothing else, so the pool had to guess which credential served it -- the same shape as the mis-pairing this line of work is about, one layer up. The guess is unanswerable once a gateway is configured: openrouter serving anthropic/claude-haiku-4-5 and anthropic serving claude-haiku-4-5 are both valid, name different credentials and different bills, and the id does not distinguish them. Guessing "the configured gateway serves everything" then handed the gateway an id it has no route for, and the 404 was swallowed by the subsystem's own fallback -- a pin that looked configured and never ran. Each pin now takes a provider alongside the model, curator_provider and llm_gate_provider. Set, nothing is derived. Unset, the vendor is still derived from the id, which is what every existing config gets and what keeps them working. Either way a pin that cannot be paired is logged and dropped rather than silently borrowing the conversation's key. bind_pin's guard is broadened from the credential exceptions to anything: it runs in the context-engine factory at construction and building a provider imports a vendor module, so a misconfigured pin could stop the agent from starting -- the opposite of what its docstring promised. ProviderPool.default is deleted along with the two tests that covered it. What a new session starts on is AgentLoop._default_binding, built in the constructor, so the method had no production caller and the tests protected dead code. Co-authored-by: Claude (claude-opus-5) --- CONTEXT.md | 26 ++++-- raven/config/raven.py | 30 +++++-- raven/context_engine/factory.py | 13 ++- raven/providers/pool.py | 60 +++++++------ tests/test_agent_loop_session_model.py | 63 ++++++++++++++ tests/test_provider_pool.py | 114 +++++++++++++++++-------- 6 files changed, 232 insertions(+), 74 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 90fdddc1..ab13b44a 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -47,17 +47,25 @@ verbatim. Changed by a `scope="default"` switch, which leaves sessions that already chose their own model where they are. **Provider pool**: -The one place a model id becomes a model binding (`raven/providers/pool.py`), -caching a provider per (vendor, model) and dropping the cache when the -credentials behind it change. Also what turns a **subsystem pin** into a pair. +The one place a model id is resolved to the credential that serves it +(`raven/providers/pool.py`), caching a provider per (vendor, model) and dropping +the cache when the credentials behind it change. Also what turns a **subsystem +pin** into a pair. Constructing a `ModelBinding` from an already-resolved pair +happens in several places; deciding *which* provider a model id pairs with +happens only here. **Subsystem pin**: -A model configured for one subsystem rather than for the conversation -(`context.curator_model`, `skill_forge.llm_gate_model`). A pin is only honoured -when the pool can pair it with credentials of its own, or when a gateway is -serving it; otherwise the subsystem follows the conversation's model, because a -bare pinned id sent on the conversation's key is exactly the mis-pairing above. -Unset out of the box -- no subsystem ships a vendor default. +A model configured for one subsystem rather than for the conversation, as a +model and the provider serving it (`context.curator_model` + +`curator_provider`, `skill_forge.llm_gate_model` + `llm_gate_provider`). Both +halves because an id alone is ambiguous the moment a gateway is configured: +`openrouter` + `anthropic/claude-haiku-4-5` and `anthropic` + +`claude-haiku-4-5` are both valid and name different credentials. With the +provider set nothing is derived; with it unset the vendor is guessed from the +id, which is what configs predating the field get. A pin that cannot be paired +is reported and dropped, and the subsystem follows the conversation's model -- +a bare pinned id sent on the conversation's key is exactly the mis-pairing +above. Unset out of the box -- no subsystem ships a vendor default. **Turn**: One complete agent reaction: from an inbound message entering the agent loop to the diff --git a/raven/config/raven.py b/raven/config/raven.py index cab115ec..8eee895a 100644 --- a/raven/config/raven.py +++ b/raven/config/raven.py @@ -81,10 +81,20 @@ class ContextConfig(_Base): Worth setting, and worth setting to something small: one slow-path pass is a bounded agent loop of up to 12 tool-calling requests, so it is per-turn housekeeping rather than an answer, and unset means a long conversation - pays conversation-model prices for it. A vendor id here needs that - vendor's credentials in ``providers``: a model id without a key of its own - is not a configured subsystem, and the Curator falls back to the - conversation rather than send that id on the conversation's key.""" + pays conversation-model prices for it. Pair it with ``curator_provider``: + a model id alone does not say which credential serves it, and a model id + without a key of its own is not a configured subsystem -- the Curator + falls back to the conversation rather than send that id on the + conversation's key.""" + + curator_provider: str | None = None + """Which configured provider serves ``curator_model``. + + Set this whenever the id alone is ambiguous, which is most of the time + once a gateway is involved: ``openrouter`` with ``anthropic/claude-haiku-4-5`` + and ``anthropic`` with ``claude-haiku-4-5`` are both valid, name different + credentials and different bills, and only you know which was meant. Unset + falls back to deriving the vendor from the id.""" curator_timeout_seconds: float = 30.0 """Max wall time for one Curator slow-path invocation before fallback.""" @@ -974,7 +984,17 @@ class SkillForgeConfig(_Base): llm_gate_model: str | None = None """Optional model override for gate calls. ``None`` → use the - provider's default chat model (typically the agent's main model).""" + provider's default chat model (typically the agent's main model). + + Pair it with ``llm_gate_provider``: an id alone does not say which + credential serves it.""" + + llm_gate_provider: str | None = None + """Which configured provider serves ``llm_gate_model``. + + Same rule as ``context.curator_provider``: set it when the id alone is + ambiguous (a gateway serving another vendor's model), leave it unset to + derive the vendor from the id.""" llm_gate_temperature: float = 0.0 """Sampling temperature for gate calls. 0.0 for deterministic diff --git a/raven/context_engine/factory.py b/raven/context_engine/factory.py index 87198960..56a75a54 100644 --- a/raven/context_engine/factory.py +++ b/raven/context_engine/factory.py @@ -141,7 +141,11 @@ def build_context_engine( get_tool_definitions=get_tool_definitions, ), CuratorSegmentBuilder( - pin=provider_pool.bind_pin(config.curator_model) if provider_pool else None, + pin=( + provider_pool.bind_pin(config.curator_model, getattr(config, "curator_provider", None)) + if provider_pool + else None + ), workspace=workspace, config=config, provider=provider, @@ -259,7 +263,12 @@ def _build_rewriter_and_gate( legacy_top_k=int(skill_forge_router_config.top_k or 5), model=getattr(skill_forge_config, "llm_gate_model", None) or None, pin=( - provider_pool.bind_pin(getattr(skill_forge_config, "llm_gate_model", None)) if provider_pool else None + provider_pool.bind_pin( + getattr(skill_forge_config, "llm_gate_model", None), + getattr(skill_forge_config, "llm_gate_provider", None), + ) + if provider_pool + else None ), temperature=float(getattr(skill_forge_config, "llm_gate_temperature", 0.0)), max_tokens=int(getattr(skill_forge_config, "llm_gate_max_tokens", 8192) or 8192), diff --git a/raven/providers/pool.py b/raven/providers/pool.py index 5f2ee646..545cf63e 100644 --- a/raven/providers/pool.py +++ b/raven/providers/pool.py @@ -67,15 +67,6 @@ def _credentials_fingerprint(self) -> str: return "" return hashlib.sha256(json.dumps(providers, sort_keys=True, default=str).encode()).hexdigest() - def default(self) -> ModelBinding: - """The binding a session starts on: ``agents.defaults``, verbatim. - - Deliberately not "the last model anyone switched to" -- a per-session - switch is scoped to that session, so a new session starts here. - """ - defaults = self.config.agents.defaults - return self.bind(defaults.model, defaults.provider) - def bind(self, model: str, provider_name: str | None = None) -> ModelBinding: """Build (or reuse) the provider that serves ``model``. @@ -100,31 +91,52 @@ def bind(self, model: str, provider_name: str | None = None) -> ModelBinding: cache[key] = binding return binding - def bind_pin(self, model: str | None) -> ModelBinding | None: + def bind_pin(self, model: str | None, provider_name: str | None = None) -> ModelBinding | None: """A subsystem's own model, on its own credential -- or None. - This is what lets a pinned subsystem run off the session's model: a - pin naming a vendor Raven has credentials for gets that vendor's key, - not the agent provider's. None means the pin is unusable (no - credentials, or nothing built), and the caller should fall back to the + This is what lets a pinned subsystem run off the session's model. None + means the pin is unusable, and the caller should fall back to the session's binding rather than send one vendor's key to another. + + ``provider_name`` is the configured half of the pair, and when present + nothing is derived: an id alone cannot say whether ``anthropic`` or a + gateway reselling it is meant, and those are different credentials and + different bills. Absent, the vendor is guessed -- which is what a + config written before the provider field existed gets. """ if not model: return None - # A gateway (or a local deployment) serves whatever id it is handed - # under its own credential, so the pin is already paired -- asking - # whether the upstream vendor has a key of its own would drop a pin - # that works. Bind it through the gateway instead. - gateway = self._configured_gateway() - resolved = gateway if gateway is not None else self._resolve_provider_name(model, None) - if gateway is None and not self._has_credentials(resolved, model): - return None + if provider_name and provider_name != "auto": + configured = provider_name + if not self._has_credentials(configured, model): + # Explicitly configured and still unusable: a config error the + # user can fix, and silence here is what let a pinned + # subsystem look configured while never running. + logger.warning( + "pinned model {!r} names provider {!r}, which has no usable credentials; " + "the subsystem follows the conversation's model instead", + model, + configured, + ) + return None + resolved = configured + else: + # A gateway (or a local deployment) serves whatever id it is handed + # under its own credential, so the pin is already paired -- asking + # whether the upstream vendor has a key of its own would drop a pin + # that works. Bind it through the gateway instead. + gateway = self._configured_gateway() + resolved = gateway if gateway is not None else self._resolve_provider_name(model, None) + if gateway is None and not self._has_credentials(resolved, model): + return None try: return self.bind(model, resolved) - except (SystemExit, RuntimeError, ValueError) as exc: + except Exception as exc: # Called from the context-engine factory at construction, so this # must leave the subsystem following the conversation rather than - # stop the agent from starting. + # stop the agent from starting. Deliberately broad: building a + # provider imports a vendor module, so the failure modes are not + # only the credential ones. logger.warning("cannot build a provider for pinned model {!r}: {}", model, exc) return None diff --git a/tests/test_agent_loop_session_model.py b/tests/test_agent_loop_session_model.py index 9e66e925..a28286dd 100644 --- a/tests/test_agent_loop_session_model.py +++ b/tests/test_agent_loop_session_model.py @@ -405,6 +405,69 @@ def test_a_configured_pin_survives_a_real_factory_build(tmp_path) -> None: assert curator.curator_model == "gemini-2.5-flash", "a configured subsystem does not follow the turn" +def test_the_factory_hands_the_pool_the_pin_the_user_configured(tmp_path) -> None: + """Both halves of the configured pair have to reach the pool. + + A gateway serving another vendor's model is exactly the case the id cannot + express: derived from ``claude-haiku-4-5`` the vendor is Anthropic, and the + curator would run on the Anthropic key while the user asked for the + gateway. Dropping the provider argument at the factory leaves the pin + looking configured and pointed at the wrong bill. + """ + from raven.config.schema import Config + from raven.providers.pool import ProviderPool + + cfg = Config() + cfg.agents.defaults.model = "claude-opus-4-5" + cfg.agents.defaults.provider = "auto" + cfg.providers.anthropic.api_key = "sk-ant" + cfg.providers.openrouter.api_key = "sk-or" + + context_config = ContextConfig(curator_model="claude-haiku-4-5", curator_provider="openrouter") + loop = AgentLoop( + provider=_Provider("boot"), + workspace=tmp_path, + model="boot/model", + context_config=context_config, + skill_forge_config=SkillForgeConfig(), + provider_pool=ProviderPool(cfg), + ) + curator = next(b for b in loop.context_engine._builders if isinstance(b, CuratorSegmentBuilder)) + + assert curator._pin is not None + assert curator._pin.provider.api_key == "sk-or", "the configured provider serves the pin" + + +def test_the_factory_hands_the_pool_the_gate_pin_the_user_configured(tmp_path) -> None: + """Same wiring, the other pin.""" + from raven.config.schema import Config + from raven.context_engine.segments.skills import SkillsSegmentBuilder + from raven.providers.pool import ProviderPool + + cfg = Config() + cfg.agents.defaults.model = "claude-opus-4-5" + cfg.agents.defaults.provider = "auto" + cfg.providers.anthropic.api_key = "sk-ant" + cfg.providers.openrouter.api_key = "sk-or" + + loop = AgentLoop( + provider=_Provider("boot"), + workspace=tmp_path, + model="boot/model", + context_config=ContextConfig(), + skill_forge_config=SkillForgeConfig( + llm_gate_model="claude-haiku-4-5", + llm_gate_provider="openrouter", + ), + provider_pool=ProviderPool(cfg), + ) + skills = next(b for b in loop.context_engine._builders if isinstance(b, SkillsSegmentBuilder)) + + assert skills._gate is not None + assert skills._gate._pin is not None + assert skills._gate._pin.provider.api_key == "sk-or" + + def test_a_stored_model_is_restored_onto_a_resumed_session(tmp_path) -> None: """The write half is useless without this read half. A switch has to survive a restart, or the user's choice lasts exactly as long as the diff --git a/tests/test_provider_pool.py b/tests/test_provider_pool.py index 3837e7a0..4566f9ab 100644 --- a/tests/test_provider_pool.py +++ b/tests/test_provider_pool.py @@ -44,29 +44,6 @@ def _pool(model: str = "claude-opus-4-5", **kwargs): return ProviderPool(_config(model, **kwargs)) -# --------------------------------------------------------------------------- -# The default -# --------------------------------------------------------------------------- - - -def test_the_default_is_agents_defaults_verbatim() -> None: - pool = _pool(anthropic="sk-ant") - binding = pool.default() - - assert binding.model == "claude-opus-4-5" - assert binding.provider.api_key == "sk-ant" - - -def test_the_default_is_not_the_last_thing_anyone_switched_to() -> None: - """A per-session switch must not leak into what a new session starts on, - so the pool reads the config every time rather than caching a "current". - """ - pool = _pool(anthropic="sk-ant", gemini="AIza") - pool.bind("gemini-2.5-flash") - - assert pool.default().model == "claude-opus-4-5" - - # --------------------------------------------------------------------------- # Binding a model # --------------------------------------------------------------------------- @@ -240,17 +217,6 @@ async def _detached() -> None: assert seen == ["started/model"] -def test_the_default_is_re_read_every_time() -> None: - """Not cached: the configured default is what a new session starts on, and - a user editing it must not need a restart. - """ - pool = _pool(anthropic="sk-ant") - assert pool.default().model == "claude-opus-4-5" - - pool.config.agents.defaults.model = "claude-sonnet-4-5" - assert pool.default().model == "claude-sonnet-4-5" - - def test_a_config_supplier_is_re_read_and_drops_stale_bindings() -> None: """A credential fixed after start (an OAuth re-login, an edited file) has to be visible without a restart, which a snapshot would prevent. @@ -358,3 +324,83 @@ def _boom(_cfg): monkeypatch.setattr(helpers, "make_provider", _boom) assert pool.bind_pin("gemini-2.5-flash") is None + + +# --------------------------------------------------------------------------- +# A pin configured as a pair +# --------------------------------------------------------------------------- + + +def test_a_configured_provider_beats_the_gateway_guess() -> None: + """The id alone cannot say which credential was meant. + + On a gateway, ``anthropic/claude-...`` served by the gateway and + ``claude-...`` served by Anthropic direct are both valid, name different + credentials and different bills. Guessing picks one; the configured pair + says which. + """ + cfg = _config("anthropic/claude-opus-4-5", openrouter="sk-or", anthropic="sk-ant") + cfg.agents.defaults.provider = "openrouter" + + from raven.providers.pool import ProviderPool + + pin = ProviderPool(cfg).bind_pin("claude-haiku-4-5", "anthropic") + + assert pin is not None + assert pin.model == "claude-haiku-4-5" + assert pin.provider.api_key == "sk-ant", "the configured provider serves it, not the gateway" + + +def test_a_configured_provider_beats_the_vendor_the_id_names() -> None: + """The other direction: a gateway reselling a vendor's model. Deriving from + the id would send it to Anthropic direct on a key the user may not hold. + """ + cfg = _config("claude-opus-4-5", openrouter="sk-or", anthropic="sk-ant") + + from raven.providers.pool import ProviderPool + + pin = ProviderPool(cfg).bind_pin("anthropic/claude-haiku-4-5", "openrouter") + + assert pin is not None + assert pin.provider.api_key == "sk-or", "the gateway serves it, not the vendor in the id" + + +def test_a_configured_provider_without_credentials_is_not_a_pair() -> None: + """Explicitly configured and still unusable is a config error, and the + subsystem follows the conversation rather than borrowing another key. + """ + cfg = _config("claude-opus-4-5", anthropic="sk-ant") + + from raven.providers.pool import ProviderPool + + assert ProviderPool(cfg).bind_pin("gemini-2.5-flash", "gemini") is None + + +def test_an_unpaired_pin_still_derives_its_vendor() -> None: + """Configs written before the provider field existed keep working.""" + cfg = _config("claude-opus-4-5", anthropic="sk-ant", gemini="AIza") + + from raven.providers.pool import ProviderPool + + pin = ProviderPool(cfg).bind_pin("gemini-2.5-flash") + + assert pin is not None + assert pin.provider.api_key == "AIza" + + +def test_a_pin_that_cannot_be_built_at_all_does_not_stop_the_agent(monkeypatch) -> None: + """``bind_pin`` runs in the context-engine factory at construction, and + building a provider imports a vendor module -- so the failures are not only + the credential ones the narrower guard covered. + """ + import raven.cli._helpers as helpers + + cfg = _config("claude-opus-4-5", anthropic="sk-ant") + + from raven.providers.pool import ProviderPool + + def _boom(_cfg): + raise ModuleNotFoundError("no module named 'litellm'") + + monkeypatch.setattr(helpers, "make_provider", _boom) + assert ProviderPool(cfg).bind_pin("claude-haiku-4-5", "anthropic") is None