fix(*): carry a live model switch to every provider holder - #282
Conversation
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) <noreply@anthropic.com>
Review of #282 (panel)Reviewed by two independent agents on different model families, each reading the PR head ( The direction is right and the fan-out inside Severity labels: R1 - before-merge - correctnessWhere: 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: 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 Suggested fix: Either make the promise real -- snapshot at entry ( Verify: A test that starts a fake multi-iteration turn, calls R2 - before-merge - correctnessWhere: Problem: A model id and a credential are one pair. Failure scenario: 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 Verify: Instantiate R3 - before-merge - test-coverageWhere: Problem: The diff adds 11
So the suite catches "the dispatcher forgot a holder" and nothing else. Combined with the duck-typed fan-out at Failure scenario: Someone adds a provider-holding segment builder, or renames Suggested fix: Two additions, both cheap because the fixtures already exist. (1) Build a real engine through Verify: Re-run the three mutations above; each should now turn something red. R4 - describe - dead-branchWhere: Problem: Failure scenario: No runtime break introduced by this PR, but a reader of Suggested fix: Either delete Verify: R5 - describe - scopeWhere: PR title and description ("every provider holder", "the five subsystems"), plus the maintenance note at Problem: Two holders take the same 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 Verify: N/A (wording and follow-up tracking). R6 - describe - verification-claimWhere: PR description, last paragraph of Problem: The claim that 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: R7 - nit - commentsWhere: Problem: Two of these say something the code does not. Separately, and purely take-it-or-leave-it: Failure scenario: N/A (accuracy of prose that lands on main). Suggested fix: Fix the direction and the list at Verify: N/A. Checked and found fineSo they do not get re-litigated later:
|
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Second round on the delta (
|
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Adversarial review pass, scoped to the five commits on this branch
(53aeb0c...HEAD -- main...HEAD locally also pulls in the already-merged #279,
which I excluded).
The diagnosis is right and the fan-out is complete. One inline note, on the park
itself. Everything else below is what I checked and found sound.
Fan-out completeness. I enumerated every object handed provider in
AgentLoop.__init__. context_engine (the factory builds only
SkillsSegmentBuilder and CuratorSegmentBuilder as LLM holders, both of which
implement set_provider), subagents, memory_consolidator -- all covered, and
so are the nested holders (CuratorAssembler -> HistoryTrimmer,
SkillsSegmentBuilder -> rewriter/gate). ContextBuilder.llm_provider reaches
LocalSkillCatalog, which documents it as unused; no tool is registered with
self.provider; Personalizer is built per turn; self.router is unwired in the
TUI.
Counter discipline. No await between the entry adopt and
_turns_in_flight += 1, and none between the decrement and the exit adopt, so
cancellation cannot leak a slot, and finally covers both raise and cancel.
_set_model is fully synchronous on the event loop, so the is_turn_active check
-> set_provider sequence has no interleaving window and the counter is never
touched off-loop.
Signature changes. _run_subagent / _run_subagent_inner gained two trailing
params; the only callers are spawn and the three test stubs, all updated.
@trace.instrument's semconv.subagent extractor binds by name, so the added
params do not disturb it, and the decorator does not dump all bound args -- no
provider leaks into a span.
Pin semantics. LLMGateFilter.set_provider (del model, keeps _model) and
QueryRewriter match their docstrings against the call sites
(model=self._model or None; the rewriter passes no model) and match what a
restart would produce.
Tests. tests/test_agent_loop_model_switch.py tests/test_subagent_manager.py tests/test_tui_rpc_config.py -> 55 passed; the related
context-engine / skill-forge / consolidator suites -> 74 passed. The new tests
drive the real run_turn, the real _run_subagent through a closed concurrency
gate, and assert on the real holder instances rather than the dispatcher alone --
they would catch a dropped or renamed setter. The mutation table in the
description matches what I saw.
Two things noted but not filed:
AgentLoop._image_tool_result_okcaches an image-support verdict keyed by model
id but computed fromself.provider, so a swap that keeps the model id reuses
the old verdict. Pre-existing (loop.providerwas already being reassigned
before this PR), so out of scope here -- but it is now one of the things
_adopt_providerarguably ought to invalidate.- Detached
MemoryConsolidatortasks are not snapshotted. The description
acknowledges this and the reasoning holds: each consolidation is a self-contained
call, not one conversation relayed across two vendors.
| Detached subagents are not covered by that park -- they outlive the | ||
| turn that spawned them -- so ``SubagentManager`` snapshots instead. | ||
| """ | ||
| if self._turns_in_flight: |
There was a problem hiding this comment.
The park keys on a global in-flight count, so a turn started after the switch also runs on the abandoned provider.
_turns_in_flight cannot distinguish "a turn that predates this park" from "a turn
that started while it was parked" -- and run_turn only adopts on the way in when
the count is zero. Concretely, in the TUI (user and system pools of one slot each,
cron on its own cron:<job.id> lane):
- a cron turn is in flight
/modelarrives with nosession_idfor that lane, so
is_turn_active(session_id)does not reject; the RPC answersapplied: True
andconfig.jsonis already rewritten- the switch parks here
- the user sends a message -- the count is non-zero, so
run_turndoes not
adopt, and that whole turn (loop, curator, gate/rewriter, consolidator, and any
spawn's snapshot) runs on the old provider and old model
For the dead-credential case this is visible: the user's turn 401s as before, and
the description's "applied on disk while the loop reports the old model" covers it.
For a plain model change it is silent -- the turn is served by the previous model
while both the config file and the RPC response say otherwise, and the transcript
records no sign of it.
Before this diff the reassignment took effect immediately, so this particular
window is new. I do not think a counter can close it: separating the two
populations needs per-turn state, which is what #284's "model as a property of the
conversation" provides. If #284 is close behind, the pragmatic answer may just be
to note the bound here; if it is not, the parked switch needs to be visible
somewhere the user looks -- right now nothing surfaces the divergence, and it lasts
as long as any lane stays busy.
(Read from the code; I did not build the cron-in-flight reproduction.)
## Summary Closes out the provider path in one PR: the provider module redesign, every open issue and backlog item that lives on the identity / connection / routing / capability axes, and multi-endpoint failover as a new feature. 78 commits, one problem per commit, rebased onto current main. Redesign (base): four provider decisions were implemented outside `raven/providers/` and had drifted copies -- wire form, credential grammar, pin resolution, price/window ladder, cache dialect. Each now has one owning module the surfaces call. Fixes on that base, most visible first: - The context window walks one ladder (explicit config > the model's real window > documented fallback). Previously the config default 65536 fed trimming and budgets raw, so a 200k-window model lost two thirds of its context every turn; an unresolvable window now renders the gauge's empty state instead of a number that is nobody's. Construction and /model switches resolve without touching the network, and a switch parked behind a running turn re-resolves the window at adoption, not at the RPC call. - A provider without real streaming (azure, codex) no longer renders upstream errors as normal assistant text: the terminal stream delta carries the classification and joins the same recovery path the non-streaming call uses. Both classify their non-200 from the live status code (shared ProviderHTTPError) instead of regex-guessing the rendered text, which also removes the bare "404" substring match that misfiled a 400 whose body happened to embed one. - Codex SSE failures keep their structured code, so an overloaded backend is retried instead of classified unknown. - Orphan `</think>` recovery (backends launched without their reasoning parser) is gated to the backend shapes that produce it, so ordinary content mentioning the tag is never cut. - Fallback hops are vetoed when both identities are certain and disagree (previously a cross-vendor hop went out under the wrong key, or silently to the wrong backend on a shared model name); the knn path dispatches each hop to its own endpoint and inherits model_overrides through the rotor. - User model_overrides win over shipped extra_body defaults, and the merge no longer drops user keys behind a gateway. - The credential gate, the reader and the builder answer one way: a spec's shipped default address satisfies the gate exactly when the reader would serve it (custom runs on a bare key again, azure still demands its address), endpoint entries inherit the flat api_base/extra_headers per field, and every display face -- provider list, endpoint list, the TUI picker -- reports the same resolved view, secrets redacted (extra_headers values included, on the flat section field too). - The vision probe stops joining operator-chosen names against the vendor catalog: it reads through the TUI's lazy proxy to see the Azure transport, treats an explicit-selection gateway (custom) as caller-chosen, and the background catalog warm is no longer suppressed for the life of the process by a stale on-disk table. - Skill forge rewriter/gate follow the configured agent model (the only auxiliary LLM calls that did not). - The onboarding wizard refuses the six litellm vendors a bare API key cannot configure, with the actual requirement named, instead of writing a section that 401s forever. - The OAuth handoff replays a signal swallowed mid-login, so a SIGHUP no longer leaves a headless TUI. New feature -- multi-endpoint failover (several accounts on one vendor): - `providers.<name>.endpoints` (label / apiKey / apiBase / extraHeaders) with `endpointStrategy: sticky | round_robin`; the three credential spellings (explicit list, Gemini api_key_list, flat fields) resolve through one reader with strict precedence and no key merging. - `EndpointRotorProvider` rotates and fails over with per-endpoint cooldown (30s doubling to 300s, process-local state); auth failures rotate -- another account's key is exactly what a dead key needs -- while endpoint-agnostic failures return immediately. Streams rotate only before the first delta, so tokens are never replayed. - Managed from `raven provider endpoint add|remove|list` and the TUI model picker; the session footer names the active endpoint. Write faces refuse a keyless endpoint for key-credential providers (local deployments keep their legitimate keyless shape); invalid sections fail loudly instead of being read as empty and overwritten. - Verified live twice: the rotor directly, and the full stack from a config file through make_provider (a dead key 401s, cools, fails over, answers). The first live run caught the auth-rotation gap the mocks could not. Also in this PR: the onboarding wizard split (5069 -> 3000 lines, pure moves with every migrated monkeypatch target mutation-checked), CONTEXT.md terms (Provider Endpoint, window ladder), benchmark alignment (pinchbench now prices through the shared ladder instead of a private drifted copy), and the model picker reads the config twice per open instead of twice per provider row. Reviewed adversarially across three external rounds and four internal panel rounds on two model families: 24 before-merge findings raised in total, every one either fixed with a mutation-verified test or refuted with executed evidence (one reviewer finding was withdrawn after a 945-case main-parity sweep); final verdicts RATIFY. The rebase also adopted the review notes left on #282/#285 that landed on this code: image-capability verdicts are invalidated on a provider switch, and a parked switch logs its park and its adoption. Known follow-ups, named in the review thread (issues to follow): the fallback routing loop contradicts the registry's explicit-selection note for `custom` (pre-existing on main), the remaining bare status substrings in classify_error (429/5xx, pre-existing), per-hop identity rebuilding for fallback chains, and the pre-existing SessionInfo shape mismatch. ## Type - [x] Feature - [ ] Fix - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification - `uv run pytest tests/ -q` -- 6128 passed, 33 skipped, run after the rebase onto current main; the provider-path test files additionally re-run under an empty HOME with identical results. - `make lint-python` clean; commit messages pass commitlint and scripts/check_commit_messages.py across all 78 commits. - `cd ui-tui && npm run lint && npx tsc --noEmit && npx vitest run` -- 86 files, 982 tests passed; `npm run gen:rpc -- --check` in sync. - Live probes (OpenRouter, tiny max_tokens): rotor failover and full-stack assembly failover, both passing; logs kept locally. - Fixes are pinned by deletion mutations that turn a named test red; review rounds ran 22 such mutations and the two survivors were themselves fixed (one dead guard deleted, one vacuous test replaced). ## Risk - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes User-visible behavior changes: `contextWindowTokens` unset now resolves to the model's real window (explicit values are fully respected and no longer overridden); the context gauge shows an empty state when the window is unknown; azure/codex upstream errors surface as errors instead of assistant text; the wizard refuses six key-only-unconfigurable vendors with the real requirement named; `custom` with only an apiKey starts again (as on main) and the picker no longer demands an address the gate does not; key-credential providers refuse keyless endpoints at write time and `endpoint add --api-key` becomes optional for local deployments; endpoint keys and extra_headers values are redacted in `provider get`/`list` and over RPC; a model switch logs when it parks behind a running turn and when it adopts. Rollback is a straight revert of the squash commit; no data migration is involved (config additions are opt-in fields). ## Related Issues Fixes #124, fixes #234, fixes #155, fixes #152, fixes #254, fixes #151, fixes #143, fixes #144, fixes #197. References #281 (not reproducible on current or reported code; the api_key forwarding it suspected is now pinned by regression tests), #119 (already fixed by #116; remaining item is the installer redirect, not provider code). --------- Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
3 participants
Summary
A live
/modelswitch rebuilt the provider but only reassignedloop.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_providernow 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
selfat call time, so an unconditional swap relays one conversation across two vendors. How that surfaces depends on the path: thechat_with_retrysites turn a rejected request intofinish_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 onlyTimeoutErrorand 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
asynciomakes at task creation -- if the two land together, the park described here exists onlybetween the two merges.
run_turnentry. One boundary covers eightself.providerreads inloop/main.pyplus 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:OriginPoolsgates 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.spawncaptures 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.configalready 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 nosession_id, and proactive turns running in their own lanes. Note the RPC still answersapplied: Trueand 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_modelis 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 emptycontext.curator_modelfollows the agent model, and now follows it in both places.set_provideron the context-engine ABC is concrete so a future implementation with no LLM-backed segment is not forced to write an empty override.ContextAssembleris the only one today and does override it.Scope
AgentLoopand the subsystems it builds.HeartbeatServiceand the Sentinel stack take the same provider but are siblings on the gateway side, which registers no tui_rpc methods, soloop.set_providercannot and does not reach them. Not reachable today; worth an issue if the two sides ever converge.MemoryConsolidatoris 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
Verification
That failure is not this branch.
tests/test_cli_theme.py::test_bold_accent_renders_styled_not_barefails the same way on an unmodified
mainat53aeb0cwhen the whole file runs (verified in adetached worktree) and passes when the single test runs alone; it is a
COLORTERMartifact and CIis green on it.
tests/test_default_context_engine.py::TestTwoTrackConcurrency::test_skill_and_memory_run_concurrentlyalso 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: onestubbed
_run_subagentwholesale (provingspawnpasses a pair, not when the pair is read) and theother called
_run_subagent_innerdirectly, bypassingspawn, the concurrency gate and the sandboxboot. 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_retrysites but does on_llm_call_stream, which is the path a TUI turn takes; and thecontext-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_providerwithpassleft it green. They now build a realAgentLoopand 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 realrun_turnand the real_run_subagent_inner. Verified by mutation -- each of these turns something red:SubagentManager.set_provider->passCuratorSegmentBuilder.set_provider->passsubagentsattribute the fan-out reachesfinallythat releases the turn slotrun_turnentry unconditionallyset_providerdrops the re-derive_run_subagent_innerRisk
_run_subagent/_run_subagent_innertake 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