fix(*): carry a live model switch to every provider holder

[arelchan]

Summary

A live /model switch rebuilt the provider but only reassigned loop.provider / loop.model. AgentLoop.__init__ had already handed that provider to the subagent manager, the context engine's LLM-backed segments and the memory consolidator, and each kept its own reference. Switching away from a dead credential fixed the main loop while subagents and the skill rewriter/gate went on authenticating against the endpoint the user had just abandoned. Cron was a fourth victim: its runs failed with the same 401 and its history rendered the failures as blank rows.

AgentLoop.set_provider now fans the pair out to every holder it built, and the context engine walks its builders duck-typed so a text-only segment is skipped rather than raising.

In-flight work

Every LLM call site reads the provider off self at call time, so an unconditional swap relays 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 -- the path a TUI turn takes -- catches only TimeoutError and lets the rejection propagate. Neither is a diagnosis the user can act on.

Two mechanisms, because the two lifetimes differ. Both are superseded by #284, which makes the
model a property of the conversation and gets the same guarantee from the context copy that
asyncio makes at task creation -- if the two land together, the park described here exists only
between the two merges.

• The loop parks. A switch arriving while any turn runs is held and adopted at the next run_turn entry. One boundary covers eight self.provider reads in loop/main.py plus the context engine and consolidator underneath them; a snapshot would have to be threaded through each. The park is a depth counter, not a flag: OriginPools gates USER and system origins on independent semaphores with no global cap, and the TUI defaults to one slot each, so a user turn and a cron turn overlap on one loop. Both ends gate on zero, and the last turn out adopts so a park cannot outlive the turns it waited on.
• Subagents snapshot. A spawn is a detached task that outlives the turn, so the park cannot reach it. spawn captures the pair it was asked for and passes it down; capturing later would miss the window where a spawn waits on the concurrency gate and a sandbox boot.

This is the second line of defence, not the first. tui_rpc.methods.config already rejects a switch outright when the caller's own session has a turn in flight; the park covers what that guard cannot see -- a caller that passes no session_id, and proactive turns running 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.

Also here

• curator_model is re-derived on a switch with the constructor's own expression, so the same config cannot mean one thing at build time and another after. The default is non-empty, so in practice it is a pin; an explicitly empty context.curator_model follows the agent model, and now follows it in both places.
• The concrete no-op set_provider on the context-engine ABC is concrete 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.

Scope

AgentLoop and the subsystems it builds. HeartbeatService and the Sentinel stack take the same provider but are siblings on the gateway side, which registers no tui_rpc methods, so loop.set_provider cannot and does not reach them. Not reachable today; worth an issue if the two sides ever converge. MemoryConsolidator is re-pointed but its detached consolidation tasks are not snapshotted -- a single call rather than a multi-turn conversation, so the split-conversation argument does not apply, but it is the same shape.

Type

• Fix

Verification

uv run pytest tests/ -q
5302 passed, 1 failed

uv run ruff check raven/ tests/     # All checks passed
uv run ruff format raven/ tests/    # unchanged

That failure is not this branch. tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare
fails the same way on an unmodified main at 53aeb0c when the whole file runs (verified in a
detached worktree) and passes when the single test runs alone; it is a COLORTERM artifact and CI
is green on it. tests/test_default_context_engine.py::TestTwoTrackConcurrency::test_skill_and_memory_run_concurrently
also failed in some runs of this branch and of unrelated ones -- a timing assertion that flakes
under full-suite load, passing alone and with its own file. A clean re-run at this head has only the
theme failure.

A later review round found one of these mutations still surviving -- moving the spawn snapshot into
_run_subagent_inner -- because neither existing test could see the window it exists for: one
stubbed _run_subagent wholesale (proving spawn passes a pair, not when the pair is read) and the
other called _run_subagent_inner directly, bypassing spawn, the concurrency gate and the sandbox
boot. There is now a test that holds the gate shut, switches the provider while the task sits in
that window, releases it, and asserts which provider actually served the call. The same round found
two docstrings scoped wider than the code: the mid-turn split does not raise on the
chat_with_retry sites but does on _llm_call_stream, which is the path a TUI turn takes; and the
context-engine ABC's concrete no-op was justified by a reason an abstract method would satisfy
equally. Both corrected.

The tests here were rebuilt after a review found the previous set only exercised the dispatcher -- replacing any receiver's set_provider with pass left it green. They now build a real AgentLoop and assert the gate, rewriter, curator, curator assembler, history trimmer, subagent manager and consolidator all moved; guard the attribute names the fan-out walks against a rename; and drive the real run_turn and the real _run_subagent_inner. Verified by mutation -- each of these turns something red:

Mutation	Result
SubagentManager.set_provider -> pass	3 failed
CuratorSegmentBuilder.set_provider -> pass	1 failed
rename the subagents attribute the fan-out reaches	2 failed
delete the finally that releases the turn slot	3 failed
adopt on run_turn entry unconditionally	1 failed
never park	2 failed
spawn without the snapshot	1 failed
curator set_provider drops the re-derive	1 failed
move the spawn snapshot into _run_subagent_inner	1 failed

• Relevant tests pass locally
• Relevant lint / type checks pass locally
• User-facing docs or screenshots are updated when needed

Risk

• Security impact considered
• Backward compatibility considered
• Rollback path is clear for risky changes

_run_subagent / _run_subagent_inner take the provider and model as parameters now; the two suites that stub them are updated. No public API changes. Rollback is a revert -- the previous behaviour is the 401.

Related Issues

N/A

[0xKT]

Review of #282 (panel)

Reviewed by two independent agents on different model families, each reading the PR head (2238051) from scratch; every finding below was then re-verified line by line against the code before being written here. Both reviewers landed on "do not merge as-is", but this is a comment, not a request for changes -- the severity labels are our reading, and what to act on is your call.

The direction is right and the fan-out inside AgentLoop is complete: all four provider exits in __init__ are accounted for, and LocalSkillCatalog really is a dead parameter (skill_forge/catalog.py:41, accepted for caller compat, never stored). What follows is scoped to what we could not confirm.

Severity labels: before-merge (we think it should be fixed first) - describe (code is fine, the prose that lands on main is not) - nit (take it or leave it).

---

R1 - before-merge - correctness

Where: raven/agent/loop/main.py:626-628, raven/agent/subagent/manager.py:82-85 (plus the same sentence in the commit body and the PR description)

Problem: Both docstrings state that in-flight turns and subagents keep the provider they started with, so no single conversation spans two endpoints. No mechanism provides that. Every LLM call site reads the instance attribute at call time, not a per-turn snapshot: main.py:1411, main.py:1596, main.py:1721, and manager.py:206 inside while iteration < max_iterations (manager.py:203). The only guard, is_turn_active in tui_rpc/methods/config.py, checks the one session_id the caller passed (optional -- absent means no check at all), is keyed by session_key in tui_rpc/methods/turn.py whose own comment says "One turn per session at a time" (so concurrent in-flight turns in other sessions are normal), and never registers subagents at all.

The direction is also inverted: before this PR the subagent manager's reference never changed, so a running subagent genuinely could not span two endpoints. After it, it can. The sentence describes the property this change removes, and it is presented as a deliberate non-change.

Failure scenario: Session A triggers spawn_subagent (up to 15 iterations, no turn registration). While it runs, /model switches from anthropic to openai from session B, or with no session_id at all. is_turn_active does not cover either case, so iteration k+1 of that subagent calls openai carrying k iterations of anthropic-shaped history. Per litellm_provider.py the vendor filter drops thinking_blocks rather than hard-failing, so the likely outcome is not a 400 but one subagent conversation silently relayed across two vendors, with billing and behaviour that cannot be reconstructed afterwards.

Suggested fix: Either make the promise real -- snapshot at entry (provider, model = self.provider, self.model at the top of _run_subagent_inner and of the loop's run), so set_provider only affects the next start -- or drop the promise and state the actual behaviour ("an iteration already in flight picks up the new provider on its next call"). If you take the second option, all four copies need the same edit: the two docstrings, the commit body, and the PR description.

Verify: A test that starts a fake multi-iteration turn, calls set_provider between iterations, and asserts which provider iteration 2 used. Today that test would show the new one.

---

R2 - before-merge - correctness

Where: raven/memory_engine/skill_forge/gate.py:64-71 (del model, only _provider is replaced), raven/context_engine/segments/curator.py:85-95

Problem: A model id and a credential are one pair. _resolve_model in raven/providers/litellm_provider.py picks the vendor prefix from the model string, while litellm_provider.py:348-350 puts the instance's api_key into that same request. Keeping a pinned model while swapping the provider therefore produces a combination that was never valid: vendor A's model id with vendor B's key. The PR describes keeping the pin as a deliberate non-change, but the pin used to be paired with the provider that served it.

Failure scenario: providers.openai.api_key is set and skill_forge.llm_gate_model is pinned to an openai model. /model switches to anthropic/.... The gate now sends the anthropic key with the openai model id and gets a 401 on every call. It was working before the switch. gate.py:99-101 swallows that into candidates[: self._legacy_top_k] and one warning line -- the same way the bug this PR fixes stayed invisible. Note this needs an explicit pin: llm_gate_model defaults to None, and an unpinned gate correctly follows the new provider's default model (gate.py:90 passes self._model or None).

Suggested fix: Make the pin and the provider move together or not at all. Smallest version: when a pin is present, leave that holder's provider alone. More thorough: resolve the pinned model with find_by_model and build a provider from that vendor's own credentials. Either way, say in the docstring that a pin is only safe while it names the same vendor -- "a pin is the user's choice" is not the whole story.

Verify: Instantiate LLMGateFilter(provider_a, model="<vendor-b>/<model>"), call set_provider(provider_b, ...), and assert whatever invariant you choose to keep (provider unchanged, or model re-resolved).

---

R3 - before-merge - test-coverage

Where: tests/test_agent_loop_model_switch.py (test_set_provider_reaches_every_holder and test_assembler_forwards_to_llm_backed_builders_only)

Problem: The diff adds 11 set_provider implementations in raven/ (13 in the diff, two of which are test doubles). Only two of them, LLMGateFilter and QueryRewriter, are exercised on the real class. AgentLoop.set_provider is tested via object.__new__(AgentLoop) plus _Recorder stubs, which verifies the dispatcher but never the classes it dispatches to. Both reviewers ran mutations in isolated worktrees and got the same result:

• SubagentManager.set_provider replaced with pass -> related tests all green
• MemoryConsolidator.set_provider replaced with pass -> all green
• CuratorSegmentBuilder.set_provider replaced with pass (including the curator_model logic and the forward to self.assembler) -> all green
• renaming the subagents attribute the fan-out reaches -> all green, though it would raise AttributeError in a real run
• control: deleting the self.subagents.set_provider(...) line at main.py:632 -> test_set_provider_reaches_every_holder fails as expected

So the suite catches "the dispatcher forgot a holder" and nothing else. Combined with the duck-typed fan-out at context_engine/assembler.py (getattr(builder, "set_provider", None), no log, no assertion), a holder whose method is renamed, deleted, or quietly wrong is skipped in silence -- which is exactly the symptom this PR set out to remove.

Failure scenario: Someone adds a provider-holding segment builder, or renames loop.subagents. CI is green, the change merges, and the 401 warning line comes back in production.

Suggested fix: Two additions, both cheap because the fixtures already exist. (1) Build a real engine through build_context_engine -- tests/test_context_engine_factory.py already shows the MagicMock-provider plus tmp-workspace pattern -- then assert after set_provider that the real CuratorSegmentBuilder.provider, assembler.trimmer.provider and SkillsSegmentBuilder._gate._provider all moved. (2) Guard the holder list against silent drift: assert the attribute names set_provider touches exist on a real AgentLoop, so a rename fails the suite instead of production.

Verify: Re-run the three mutations above; each should now turn something red.

---

R4 - describe - dead-branch

Where: raven/context_engine/segments/curator.py:93-94 and its docstring at :88-89

Problem: ContextConfig.curator_model is declared as str = "gemini-2.5-flash" (raven/config/raven.py:77) -- non-empty and not Optional. if not self.config.curator_model is therefore false in every default deployment, so self.curator_model = model never executes, and the docstring's distinction ("an explicit config.curator_model is a pin") cannot be expressed by the schema: a default value and a user-set value are indistinguishable, and the effective behaviour is "always pinned". This also predates the PR at construction time (:72 has the same or model), so the new code is consistent with the old -- what is new is that the unreachable branch is now documented as an intentional semantic.

Failure scenario: No runtime break introduced by this PR, but a reader of :88-94 will believe curator follows the agent model when unpinned, and it never does. Combined with R2, a default deployment that switches away from gemini has the curator slow path calling gemini-2.5-flash with the new vendor's key until the deterministic fallback catches it.

Suggested fix: Either delete :93-94 and say plainly that curator_model is always a pin, or change the schema to str | None = None so "unpinned" becomes expressible. The schema change is a behaviour change and probably belongs in its own PR rather than this one.

Verify: grep -n 'curator_model' raven/config/raven.py and a test asserting whichever semantic you settle on.

---

R5 - describe - scope

Where: PR title and description ("every provider holder", "the five subsystems"), plus the maintenance note at raven/agent/loop/main.py:556-558

Problem: Two holders take the same provider object without being reachable from AgentLoop: HeartbeatService (raven/cli/gateway_commands.py:424, stored at proactive_engine/schedulers/heartbeat/service.py:78) and the Sentinel stack via build_sentinel_stack (gateway_commands.py:212). They are siblings of the loop, not children, so loop.set_provider() cannot reach them. Reachability today is zero, and we checked before raising it: register_config_methods runs only in the TUI process (tui_rpc/methods/__init__.py:125 via cli/tui_commands.py:654), the gateway process registers no tui_rpc methods, and Sentinel/heartbeat live only on the gateway side. It is still worth naming because HeartbeatService is on by default, calls an LLM on its own timer, and holds a construction-time provider -- it is where this bug class reappears first if the two sides ever converge.

Failure scenario: Not reachable today. Future: any hot switch added on the gateway side leaves heartbeat and Sentinel on the abandoned credential, with the same swallowed-warning signature.

Suggested fix: Narrow "every provider holder" to "AgentLoop and the subsystems it builds" in the title/description, and make the note at main.py:556-558 state that its scope stops at AgentLoop. Consider an issue for heartbeat/Sentinel/PerModelProvider so the residue is recorded rather than remembered.

Verify: N/A (wording and follow-up tracking).

---

R6 - describe - verification-claim

Where: PR description, last paragraph of ## Verification

Problem: The claim that tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare "fails the same way on an unmodified main" did not reproduce for either reviewer: on the PR head, with no --deselect, the full suite and the file on its own were both green, and the CI unit job is green without the deselect too. We cannot disprove what happened on your machine, only report that it does not reproduce on a clean worktree.

Failure scenario: N/A for the code. The risk is procedural: the next person to hit it starts from "known pre-existing failure on main" as an established fact.

Suggested fix: Restate it as what was observed ("fails locally in a full-suite run, not reproducible on a clean worktree or in CI"), or drop the deselect and the paragraph.

Verify: uv run pytest tests/test_cli_theme.py -q on a clean checkout of main.

---

R7 - nit - comments

Where: raven/agent/loop/main.py:556-558, raven/context_engine/base.py:146-151, and the one-line docstrings on the five small setters (subagent/manager.py:79, context_engine/curator.py:356, context_engine/segments/curator.py:86, memory_engine/consolidate/consolidator.py:1727, plus history_trimmer.py)

Problem: Two of these say something the code does not. main.py:556-558 reads "Every subsystem below was handed provider above", but all four receivers are above that comment (:418, :460, :508, :543), and the block below it lists self.context.skills, which is precisely the one not in the fan-out; both readings make the sentence false. base.py:151 justifies the concrete no-op with "an engine with no LLM-backed segment needs no override", but no such engine exists -- ContextAssembler is the only implementation, as base.py's own class docstring and factory.py both say. The real reason is the type contract on AgentLoop.context_engine, which is worth writing instead.

Separately, and purely take-it-or-leave-it: """Adopt the provider a live /model switch just built.""" is repeated verbatim on five two-line setters where the method name already says it, and the incident narrative appears in three source docstrings plus two test docstrings on top of the commit body and PR description.

Failure scenario: N/A (accuracy of prose that lands on main).

Suggested fix: Fix the direction and the list at main.py:556-558, and change the base.py rationale to the type contract. The gate and rewriter "pin stays put" docstrings earn their space; the repeated one-liners are yours to keep or drop.

Verify: N/A.

---

Checked and found fine

So they do not get re-litigated later:

• Fan-out coverage inside the context engine is complete: of the six builders in build_context_engine, only Skills and Curator hold a provider and both implement the method; the router, its sources and the hub client hold none.
• del model for an intentionally unused parameter has precedent in this repo (skill_forge/everos_source.py, local_source.py).
• object.__new__(AgentLoop) in tests has precedent too (tests/test_read_file_image.py); the concern in R3 is coverage, not the technique.
• An unpinned gate and the rewriter do follow the new provider's default model (gate.py:90, and the rewriter passes no model at all), so del model there is correct.
• MemoryConsolidator passes self.provider/self.model down per call rather than caching them further, so there is no second-level holder behind it.
• gateway_commands.py:197 wraps the provider in PerModelProvider for the knn routing backend while _set_model builds a bare one, so a switch drops that wrapper. This predates the PR (the old two-attribute assignment dropped it too), so we are not counting it here.
• Metadata is compliant: title length leaves room for the squash suffix, no non-ASCII in the commit message or the description, one Type box matching the commit type, trailer in the right place, and the new test file name follows AGENTS.md 5.1.

[0xKT]

Second round on the delta (2238051..3eaf3a0)

Same setup as before, plus a third reviewer from a different model family this time. Two of the items below are us taking something back. As before, this is a comment, not a request for changes.

Correcting R6 -- you were right, we were wrong

Our claim that the test_bold_accent_renders_styled_not_bare failure "did not reproduce" was an artifact of our own environment. The test's outcome depends on COLORTERM:

COLORTERM=""          -> 1 failed, 44 passed
COLORTERM=truecolor   -> 45 passed

Both reviewers last round happened to run with truecolor, so neither could see it. You had reported it accurately, and we asked you to soften a true statement. Sorry about that. We see you have since found the same cause and put it in the description.

Correcting R2's suggested fix -- do not do what we suggested

We proposed "when a pin is present, leave that holder's provider alone". That is wrong, and it is worth saying plainly: the usual reason to switch providers is that the current credential stopped working. Leaving a pinned gate on the old provider would keep it on the dead credential permanently -- which is the exact failure this PR exists to remove. Please disregard that suggestion.

R2 restated, with the piece our argument was missing

The fact we should have supplied last round: on main, LLMGateFilter has no set_provider at all, and _set_model only assigns loop.provider. So on main a live switch leaves the gate on the provider it booted with, and a config where the pin and the boot provider are the same vendor keeps working until restart. After this PR the gate follows the switch while the pin does not, so that config starts returning 401. That is a behaviour change introduced here, and it is checkable rather than a matter of opinion.

We also withdraw the "never valid" framing. Your "same as a restart" point holds: a restart on the new model rebuilds the gate with the new provider and the same pin, reaching the identical state. The mismatch is inherent in the configuration semantics -- a pin is a bare model id while the credential always follows the agent's provider -- and that predates this PR on the restart path. This change makes the switch path agree with the restart path, which is defensible.

So we are not asking for a behaviour change. The one thing that seems worth adding: when a switch lands and a pinned model names a vendor the new provider does not serve, log a warning. Today the resulting 401 is swallowed into candidates[: self._legacy_top_k] with a single line, which is the same shape of silence this PR set out to fix. The curator's curator_model is the same case with no warning at all.

R5 is still open in the title

The description now carries a ### Scope section that states the boundary accurately, including that HeartbeatService and the Sentinel stack are siblings the fan-out cannot reach. The title still says "every provider holder", so the two disagree with each other. The title becomes the squash header on main, which is the copy that outlives the PR.

R8 (new) -- a parked switch is invisible to the user

set_provider parks silently when _turns_in_flight is non-zero, and neither the park nor the later adopt logs anything. By then _set_model has already written the config and returned applied: True, so the TUI shows the new model while replies still come from the old provider. A user in that window cannot tell which model answered, and afterwards there is nothing to look at. Your own criterion in this PR's description -- that a failure the user cannot act on is the problem worth fixing -- applies here. Two logger.info lines, one at park and one at adopt, would close it.

On the mechanism itself we have nothing: three reviewers went through the new logic independently and the concurrency semantics hold up. The in-flight count is paired by try/finally with no await between the decrement and the adopt, overlapping turns gate on zero at both ends, queued turns do not hold the count, nested spawns do not re-enter, and the spawn snapshot is taken at the right point.

Confirmed fixed

R1, R3, R4 and R7 are done, and R3 is worth calling out: every mutation we ran this round turned something red, including the three that survived last round (SubagentManager, MemoryConsolidator and CuratorSegmentBuilder set_provider replaced with pass) and renaming the attribute the fan-out walks. The tests now build a real AgentLoop and drive the real run_turn and _run_subagent_inner. Adopting mutation testing as the standard of evidence here, and publishing the table in the description, is a better outcome than the fix we asked for.