feat(providers): endpoint failover and provider path close-out - #287
Conversation
Adversarial review: 11 findings, 4 of them merge blockersThe module redesign holds up well -- the seven owning modules plus the Every item below was verified by executing code against the PR head, not by Merge blockers1. 2. # config: {"providers": {"openrouter": {"apiKey": "sk-or-WORKING"}}}
add_provider_endpoint("openrouter", label="eu", api_base="https://eu.example/v1")
credential_status("openrouter", section).ok # True -> "openrouter is configured"
provider_endpoints(section) # [ResolvedEndpoint(label='eu', api_key='')]
3.
Each one silently burns a fallback model and puts a healthy rotor endpoint into 4. find_by_name("bedrock") # None
find_by_model("bedrock/anthropic.claude-sonnet-4-20250514") # None
# -> addressed is None -> return False before the family axis is consulted
# old expression matched the anthropic spec -> TrueBedrock is a live path ( Should fix5. 6. 7. 8. 9. One spec name is carrying two incompatible meanings; the gate needs an axis 10. 11. Not includedThree lower-severity items (pinchbench pinning |
|
Thank you -- this is an exceptional review. Every blocker reproduced exactly as Fixed, with the finding's own repro turned into a regression test:
Finding 1 (written-out 65536 on upgraded installs) -- acknowledged, and The four candidates you did not finish verifying -- rotor retry depth on |
Follow-up: fixes re-verified, plus the four candidates finishedI re-ran the original repros against The On finding 1, one option that fits your stance without rewriting anyone's The four candidates, finishedRotor retry depth on auth -- REFUTED, no action needed. My concern was that One endpoint, one attempt, then rotate. The only residue is that the caller Write path does not enforce the refusal the factory does -- CONFIRMED, minor.
The mismatched spelling makes the snapshot lookup miss, so the row degrades to One more, outside that listThe
The registry says aihubmix does not support prompt caching; the old I have not verified what aihubmix does with the field on the wire -- it may |
|
Second pass, deliberately from a different angle than the first: instead of All eleven earlier findings reproduce as closed -- I re-ran each finding's own What did not close is the shape of the problem: the gate, the reader and the Must fix1.
2. 3. The same line does it for headers: 4. Should fix5. The write path also does not enforce the refusal 6. 7. 8. Lower confidence
Checked and cleanThe rotor's rotation / cooldown / stream-boundary discipline and its delegation On the four candidates left unverified last round: the per-hop cache strip is |
|
Thanks for finishing the four candidates and for re-running the originals -- Everything you confirmed, plus the new finding, is addressed in one commit:
The rotor refutation needed no code, but the residue you noted rode along |
|
Correction on my previous comment: I reviewed against Retracted -- finding 1 ( Corrected -- Also already fixed -- the write-path refusal half of finding 5. One Still open at
|
| # | Where | Note |
|---|---|---|
| 2 | providers/auth.py:238 |
auth.py is untouched by all three commits; providers.custom with only an apiKey still refuses to start despite its shipped default_api_base |
| 3 | providers/auth.py:148 and cli/_helpers.py:166 |
_helpers.py changed only to adopt the shared refusal predicate; ep.api_base or config.get_api_base(model) is unchanged, so the gate and the builder still disagree, headers included |
| 4 | providers/base.py:474 |
base.py untouched; _STATUS_404 is still a widened bucket sitting ahead of the 400 one |
| 5 (first half) | ui-tui/src/components/modelPicker.tsx:553 |
no ui-tui/ changes; a blank key still persists a keyless endpoint into the rotation |
| 6 | config/update_providers.py:598 |
only add_provider_endpoint changed; list_providers can still report ****set**** with configured: False, and the (N endpoints) branch stays unreachable |
| 7 | providers/per_model_provider.py:64 |
the two new guards do not touch this line; getattr(fallback, "model_overrides", None) still reads an attribute neither EndpointRotorProvider nor LazyProvider defines |
| 8 | agent/loop/main.py:667 |
still reads "the only write here is one int attribute", while the body now also writes the assembler, its trimmer and the consolidator |
One note worth more than its severity, on #7: the reason it survived a green
suite is that test_sub_providers_inherit_configured_model_overrides builds its
fallback as a LiteLLMProvider, which is the one class that does define
model_overrides. The bug only appears when the fallback is a rotor or a
LazyProvider, so the test passes and the behavior is still wrong. Worth a
parametrized fallback rather than a second test.
On the litellm-import probe change in f3df92e6: I checked it for a weakened
assertion and it is not one. The claim ("litellm" not in sys.modules
mid-process) still runs and is now reported through both stdout and the exit
code, so a regression turns it red either way; what is skipped is interpreter
teardown, which was never part of the claim. Reasonable isolation of a native
atexit segfault.
Four decisions about providers were implemented outside raven/providers/: the credential-gate exit, the price and context-window ladder, pin resolution, and the predicate for which wire accepts a cache dialect. Each had grown a second copy at the surface that needed it, and the copies had drifted, so the same provider answered the same question differently depending on which entry point asked. Each decision now has one module that owns it, and the surfaces call it: wire.py outbound route prefix and inbound storage form auth.py credential requirements as an AND-of-OR grammar pin.py which provider a stored model routes to rates.py price and context window, with an explicit ladder prompt_cache.py who accepts cache_control, and who refused it catalog.py model labels, from a snapshot pinned to a commit Behaviour this changes for a user: every surface that writes the default model now also decides its pin, so the model is served by the provider just configured rather than by whichever one a keyword matched; missing credentials are reported as the sentence naming what is missing instead of an exit; a provider that refuses cache_control is learned once and not asked again. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
The error and response.failed SSE events were collapsed into a bare "Codex response failed" RuntimeError, so classify_error landed in the unknown bucket (retryable=False) and an overloaded backend was never retried. Carry the structured code and message into the exception text; the existing "overloaded" substring needle then classifies server_is_overloaded as a retryable server error without touching the classifier. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The rewriter had no model parameter and the gate only its dedicated llm_gate_model, so both fell through to provider.default_model on every call -- the only auxiliary LLM calls in the repo not chained back to the configured agent model. Thread build_context_engine's model into _build_rewriter_and_gate: the rewriter now sends it explicitly and the gate falls back to it when llm_gate_model is unset. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The configured default 65536 flowed raw into trimming, token budgets, memory consolidation, and the TUI gauge, so a model with a 200k window lost two thirds of it every turn and the gauge rendered against a number that was nobody's. The ladder now lives in providers/rates.py effective_context_window: explicit config wins, then the model's real window, then a documented fallback. AgentLoop resolves once at construction and pins explicit values (a pin now also survives the per-call live lookup), /model switches re-walk the ladder, and an unknown window reports context_max=0 so the UI shows its empty state instead of a guess. Known gap, unchanged scope: trimmer and consolidator keep their construction-time snapshot after a model switch. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
A backend launched without its reasoning parser (e.g. sglang without --reasoning-parser) swallows the opening <think> into the prompt template, so the completion arrives as bare reasoning prose plus an orphaned </think>. LiteLLM's only fallback anchors on an opening tag and runs non-streaming only, so the broken protocol was recorded as-is into trajectories. split_orphan_think in providers/reasoning.py splits the leading reasoning off once the full text is in hand, wired into the non-streaming parse and the stream assembly; live deltas stay untouched because a delta cannot be classified before the closing tag arrives. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The onboarding wizard's "another supported vendor" branch treated every LiteLLM vendor as a paste-a-key shape, so chatgpt (device flow that ignores the key), bedrock/sagemaker (AWS credential chain), vertex_ai (project+location+ADC), azure (base+version+key/Entra) and cloudflare (key+base/account_id) were written down as configured and then failed on the first call. auth.py now carries the refusal table with what each vendor actually needs, and _collect_credentials refuses before collecting instead of persisting a section that can never authenticate. chatgpt points at raven's own openai_codex flow; gigachat stays configurable and only gains a hint that its key is base64(client_id:client_secret). Deliberately no registry specs or multi-credential schema for these six until someone actually needs them served. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Investigating a reported spawn-subagent 401 established that chat() and chat_stream() both pass the provider's api_key explicitly to acompletion and that a subagent reuses the main provider instance verbatim -- the reported asymmetry is not reproducible from the code, on HEAD or on the release it was reported against. What the investigation did find is that no test ever asserted the api_key kwarg arrives, so the explicit-forwarding line could be dropped without anything turning red. These three tests pin the mechanism on both call paths and through the subagent manager's shared instance. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
…erwriting model_overrides is already the channel for arbitrary sampling/serving params -- LiteLLM auto-forwards unknown top-level keys into extra_body for OpenAI-compatible backends, and a nested structure can be written as extra_body directly. But both chat() and chat_stream() assigned the provider's wire-routing extra_body (e.g. the OpenRouter provider pin) over whatever a model_overrides entry had placed there, so a user's extra_body keys were silently dropped whenever the request travelled through a gateway. Merge the two instead, wire keys winning on collision because routing pins must reach the wire intact. The schema comment now names model_overrides as the passthrough channel, so no sampling_extra/request_extra fields get invented for the same decision. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The picker already marks MiniMax as an open-source partner; this adds the one sentence telling other vendors that the door is open. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Two guards the wizard was missing. The curated picker catalogue is hand-written and the existing tests only checked one direction, so a provider added to the registry but never to the shortlist stayed unreachable with everything green; the new set-equality test turns either drift direction red. And both prefill paths -- the language screen defaulting to the active language, and the model prompt defaulting to the already-configured model -- had no assertions at all (every _pick_model test passed current_model=None), so deleting either default left the suite green. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
suspendForHandoff defers signal exits while a device-flow login owns the terminal, but the handler dropped the signal outright: a SIGHUP landing in that window (terminal gone, delivered to the whole process group) killed the login subprocess yet left the TUI process alive with no controlling terminal -- a headless process only kill could reach. The handler now records the first deferred signal and the outermost release replays it through the normal exit path, so the intent the user's terminal expressed during the handoff is honored right after it. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
chat_with_retry walked the fallback chain by swapping the model string while the instance's key, base and gateway stayed frozen, so a direct provider sent another vendor's fallback model out under its own credentials -- usually a 400, and silently the wrong backend when two vendors share a model name. can_serve now skips such hops with a warning (gateways still answer for everything; unresolvable ids still fail loudly at the wire), comparing vendors through canonical_provider_name so config aliases do not read as cross-vendor. PerModelProvider stops funneling the whole chain into the primary model's endpoint and dispatches each hop to its own routed sub-provider, matching the base loop's continuation semantics. Per-hop identity rebuilding and per-hop wire overrides stay with the multi-endpoint work. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
… table Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
…mode A provider without real streaming (azure, codex) replays its chat() result through the default chat_stream fallback as one terminal StreamDelta -- which had nowhere to put finish_reason or error_classification, and the stream collation then hardcoded the finish_reason to stop/tool_calls. Net effect on the default streaming path: an upstream error was rendered to the user as a normal assistant reply, with no retry, no fallback, and the turn recorded as success. The terminal delta now carries both fields, and _llm_call_stream diverts an error delta away from token rendering into the same error LLMResponse shape the non-streaming path already returns, so the existing recovery and terminal-error handling apply unchanged. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The 429 and 5xx buckets each carry their literal status as a substring marker for the degraded string path, but the 404 bucket only had the wordier phrasings -- so a provider that renders its non-200 body into a plain string (azure's path) with a route-level "Resource not found" message fell through to unknown and lost the fallback. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
…bout can_serve treated "my provider_name resolves to nothing" as a mismatch, so an instance constructed without a provider_name -- the proactive planner and evolver both build one that way -- skipped every resolvable fallback hop, and an OAuth identity like github_copilot, whose one grant serves several upstream vendors, refused models it actually serves. The veto now requires both sides to be known, non-OAuth and different; every unresolved identity falls back to failing loudly at the wire, which is the base class's stated default. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The merge gave the provider's built-in extra_body priority on collisions, justified by "routing pins that must reach the wire" -- but the whole _WIRE_OVERRIDES table is one shipped default workaround (disabling OpenRouter's qwen reasoning mode), not a routing pin, and the same change set documented model_overrides as the channel that overrides shipped defaults. A collision is the user deliberately reversing one, so the user's value wins now; the conflict test mirrors the one real collision instead of an invented provider pin. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
… place First stage of multi-endpoint support: ProviderConfig gains an endpoints list (label + key + base + headers per entry), and providers/endpoints.py resolves the three spellings a section can carry -- explicit endpoints, Gemini's api_key_list, the flat fields -- into one uniform list with strict precedence and no merging, so a stale flat key cannot outlive the endpoint meant to replace it. Reading the shapes independently is how the Gemini list came to be declared and never used. Rotation and failover read this list next; make_provider will refuse the field on OAuth/azure/codex providers when it is wired up. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The window ladder put a synchronous OpenRouter catalogue fetch (10s httpx timeout on a cold cache) inside AgentLoop construction and the /model switch handler -- the former stalls startup, the latter runs on the asyncio event loop and freezes every turn with it. Both callers now resolve with allow_fetch=False: an in-process cache of any age answers, then the on-disk cache of any age, then the documented fallback, and the network is never touched. The per-call usage path keeps refreshing normally; it already lives inside an await and is where a stale window catches up. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
onboard_commands.py had grown past 5000 lines; the EverOS/Step4 cluster (~1500 lines, 36 symbols) now lives in onboard_everos.py. Pure move, zero behavior change: shared wizard UI state stays in onboard_commands and both modules reach across through module references with call-time attribute access, so import order cannot bite and monkeypatches keep hitting the code that actually runs. Every migrated patch target in the test file was mutation-checked one by one against pointing at the old module -- all 23 turn red, none patch a shell. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
split_orphan_think ran on every provider's every response, so any ordinary answer that mentioned a bare closing think tag -- a tutorial, a code sample -- had its leading half silently folded into reasoning. The parser-less sglang/vLLM shape only comes from a self-hosted backend, so emits_unparsed_reasoning gates both call sites: custom, local and unspecced identities normalize, gateways and known direct vendors leave content alone. The pairing check now looks for the shared opening-tag prefix, so a block that opens <think> and closes </thinking> no longer leaks its opener into the reasoning text. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
extra_headers can carry a secret (an auth header some gateways need alongside the key), and both endpoint display faces handed the values back verbatim. Each value is now masked on its own with the key names left visible -- masking the whole dict as one string would also hide which headers are configured -- in list_provider_endpoints and in the nested-model redaction get_provider_config applies per endpoint. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Every picker row re-parsed the config from disk twice -- once for the list_providers mapping rebuilt inside _build_provider_entry, once for _configured_overlays -- so opening the picker cost two parses per registry provider (22 observed for 21 rows). _entries_off_loop now loads the config once and hands each row its mapping and section; the single-row path keeps loading on its own. A counting test pins the bound at two parses per open. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
…tion Dropping the 404 text needle left codex's non-200 path -- a plain RuntimeError whose message starts "HTTP 404:" -- classifying as unknown, so a real 404 stopped triggering fallback. The status-carrying exception azure grew for the same reason is promoted to a shared ProviderHTTPError (subclassing RuntimeError, so any broad handler keeps matching) and codex raises it too; both providers now classify from the live status code instead of the rendered text. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
… field list_provider_endpoints (feeding `provider endpoint list` and the TUI picker's endpoint rows) read each entry's own fields, so an endpoint written with only --label/--api-key -- the shape the flat-inheritance change exists for -- displayed an empty address while requests were correctly using the section's flat one, reading as "the config did not take". The display face now consumes provider_endpoints, the same resolved view every request uses. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The credential gate's spec-default fallback checked only that default_api_base was non-empty, while Config.get_api_base serves a default only for a gateway or local deployment -- a second derivation of the same rule, which happened to agree for the two requires_api_base specs that exist today and would silently diverge for a spec carrying a default the reader never hands out. Both now read one registry property (usable_default_api_base), and an invariant test walks every spec asserting the gate never passes a bare-key config whose default the reader would then refuse to serve. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
needs_api_base was derived from the credential shape alone, so the TUI picker kept blocking a bare-key custom submission client-side -- the exact refusal the save_key alignment was supposed to end -- while rendering the address field as "(required)". It now says "an address must be supplied": true for local deployments (the address is the credential) and for endpoint-credential specs shipping no usable default (azure), false for custom, whose shipped localhost default the gate accepts. The picker's existing check and label read the flag unchanged. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The per-endpoint extra_headers got per-value redaction while the flat section field -- where an AiHubMix APP-Code actually lives, per its own comment -- still printed plaintext through `provider get` without --show-secrets: one table, two rules. The field now carries the schema-level secret marker (the direction _is_secret_field's docstring already names) and _redact learned to mask a dict per value, keys left visible. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Reporting only the first gap made a bare azure submission a two-round trip: "requires an API key", then "requires an address" after the key was supplied. All missing labels are joined into one message, and the field picked for error.data no longer assumes the first requirement carries one. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
For a section whose endpoints all lack keys, the gate's hint said `provider set --api-key` -- which writes the flat field the gate then ignores exactly because endpoints exist, so following the hint changed nothing and the gate repeated itself. The hint now names `provider endpoint add` whenever the section carries endpoints. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The picker-rows hoist left a third disk read in place: every row's _provider_models fetched its curated model list through get_provider_config, an uncached raw read and parse per registry row. The all-rows path now reads the models off the section it already loaded; the single-row path keeps fetching on its own. The counting test now also counts raw reads, which the load_config-only bound was blind to. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The is_local branch ran before the endpoints one, so a local provider with several keyless endpoints -- the multi-instance shape the write path explicitly allows -- listed as just "(not needed for local)" with no trace of them, while the same shape on a keyed provider showed the count. The endpoints branch now runs first and keeps the local wording alongside the count. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The early return for names outside api_key/api_base changed no answer: ResolvedEndpoint carries no other requirement field, so the getattr already answers False for api_key_list and anything else. Let it. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Three copies of the endpoint-inheritance comment said an entry inherits when it names "neither" api_base nor extra_headers, while the code falls back per field independently -- an entry naming only its address still inherits the flat headers. All three now say so. The refresh_context_window docstring stops enumerating its cascaded writes (the enumeration was already one object short); the invariant that matters is that every transitive write is a GIL-atomic int assignment. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The classification-level test constructed ProviderHTTPError directly, so reverting the raise site to a plain RuntimeError kept the suite green -- the exact null-verification shape a mutation run exposed. A faked non-200 stream now walks chat() end to end and asserts the classification arrives as model_unavailable; the same mutation turns it red. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
…ecrets set_provider_fields' OAuth branch matched everything marked is_secret, so the new secret marker on extra_headers turned `provider set --extra-headers` into a refusal for OAuth providers -- and the refusal sent the user to `provider login`, which never writes a header. The guard now names the credential fields its own docstring promises (api_key, api_key_list); extra_headers is secret to display, not a credential. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
…c spec The every-spec invariant is vacuous while no registry spec carries a default_api_base that differs from its usable one -- reverting the gate to read the raw default kept it green. A synthetic requires_api_base spec whose default the reader never serves pins the distinction. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
_localize keyed the endpoint-add hint off any truthy endpoints attribute while _present only treats a real list as the endpoints shape; a duck-typed section could be told to run endpoint add while the gate was judging its flat fields. Same isinstance guard on both. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Deleting the isinstance guard in _localize turned nothing red: the duck-typed shape it protects only ever reaches the gate from test doubles, so no test exercised it. One does now. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The background warm skipped whenever the in-process table was non-empty, and _cached_catalog_only adopts a disk table of any age into exactly that table -- so a long-lived session booting on a days-old cache file never warmed again for the life of the process. The guard now asks for freshness (a table with a live timestamp inside the fetch TTL), which a disk-adopted table deliberately does not have. Also stated honestly in the re-check comment that the unlocked read narrows the warm race rather than eliminating it. Both raised in review of the upstream change this branch carried the catalog reader over from. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
…custom Two holes in the vision probe's exclusion list, both raised in review of the upstream change and both about a served name wrongly joined against the vendor catalog -- the silent lose-the-picture direction: - The TUI's loop holds a LazyProvider, so the isinstance probe for the Azure transport answered about the proxy and a bare deployment name was looked up as OpenAI's model. The proxy now exposes `unwrapped` (the materialized inner, never a building accessor) and the probe reads through it. - A `custom` gateway serves whatever its operator named the model, and find_by_model resolves that name to the vendor's spec -- which also made the documented vision_override rollback unreachable for it. The probe now asks the configured provider which section built it (provider_name, delegated through the rotor) and treats an explicit-selection gateway (is_gateway with no keywords) as caller-chosen. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Two residues review left on the switch fan-out. The image-tool-result verdict is cached per model id but computed from the provider, so a swap that kept the model id went on serving the old transport's answer; adoption now clears it. And a parked switch was invisible end to end -- the RPC had already answered applied=True with the config written, so the window where replies still come from the old provider left no trace; the park and the adoption each log one line. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
…call The RPC called refresh_context_window right after set_provider, but a switch that parks returns immediately -- the refresh recomputed the old model's window, and when the parked switch was adopted at turn drain nothing refreshed again, so the new model ran with the old window for the rest of the session. The refresh now lives in _adopt_provider, the one point both the immediate and the parked path go through, and the RPC call site drops its copy. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The fetch docstring contradicted its neighbor (the table is double-keyed by full id and bare alias, and two consumers rely on the alias); the real park-to-adopt test now proves the window re-resolves at adoption via a sentinel instead of asserting nothing; the unbuilt lazy proxy's "reads as not Azure" branch is pinned; and two comments are trimmed -- one restated the docstring eight lines above it, one anchored itself to a line position that moves. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
f3df92e to
acd6b8c
Compare
|
Thanks for the correction, and sorry about the race: your second pass One logistics note first: the branch was rebased onto the current main All seven still-open items are addressed, one commit per item, and an Your seven:
Smaller things that rode along: endpoint display faces redact Your #285 notes came along for the ride. The rebase carried the One deliberate non-fix, stating our reasoning rather than silently Two known follow-ups we are deferring, not hiding: the remaining If nothing else surfaces, a formal review verdict would help us close |
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:
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.
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.
is retried instead of classified unknown.
</think>recovery (backends launched without their reasoningparser) is gated to the backend shapes that produce it, so ordinary
content mentioning the tag is never cut.
(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.
no longer drops user keys behind a gateway.
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).
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.
auxiliary LLM calls that did not).
cannot configure, with the actual requirement named, instead of writing
a section that 401s forever.
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 credentialspellings (explicit list, Gemini api_key_list, flat fields) resolve
through one reader with strict precedence and no key merging.
EndpointRotorProviderrotates 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.
raven provider endpoint add|remove|listand the TUI modelpicker; 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.
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 substringsin classify_error (429/5xx, pre-existing), per-hop identity rebuilding for
fallback chains, and the pre-existing SessionInfo shape mismatch.
Type
Verification
uv run pytest tests/ -q-- 6128 passed, 33 skipped, run after therebase onto current main; the provider-path test files additionally
re-run under an empty HOME with identical results.
make lint-pythonclean; commit messages pass commitlint andscripts/check_commit_messages.py across all 78 commits.
cd ui-tui && npm run lint && npx tsc --noEmit && npx vitest run-- 86files, 982 tests passed;
npm run gen:rpc -- --checkin sync.assembly failover, both passing; logs kept locally.
review rounds ran 22 such mutations and the two survivors were
themselves fixed (one dead guard deleted, one vacuous test replaced).
Risk
User-visible behavior changes:
contextWindowTokensunset now resolves tothe 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;
customwith 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-keybecomes optional for local deployments; endpointkeys and extra_headers values are redacted in
provider get/listandover 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).