refactor(providers): every provider is a modular composition (source · adapter · knobs)#57
Conversation
…· adapter · knobs) The provider concern was scattered: a provider's catalog SOURCE lived in `sources/` (gated by ad-hoc ifs in build_registry), its wire ADAPTER in `provider_adapters/` (hand-wired in serve.py), and its operator KNOBS in a flat settings.SCHEMA — three places to touch to add one provider. Add `providers.py`: one `Provider` record per provider composing up to four aspects (the structural declaration stays in config.live.lua, core-facing): - source — the catalog/pricing/discovery builder (+ an `enabled` predicate) - adapter — the wire backend for an api_kind (None ⇒ default openai_compatible) - knobs — operator-tunable settings, declared next to the provider build_registry, the api_kind dispatcher handlers, and settings.SCHEMA all DERIVE from PROVIDERS. Adding a provider = one config.live.lua entry + one Provider row. Composition, not inheritance: a provider supplies only the aspects it has (source=None / adapter=None), so no fat base class and no re-coupling of the catalog/wire concerns #25 deliberately split. codex stays the one documented exception (its scarcity source OBSERVES its own backend, wired in serve.py) — generalising that single coupling would be mechanism for one case. A follow-up turns codex into a load-balancer provider over N subscriptions, dissolving the exception. Pure refactor, behaviour-preserving: same sources built, same dispatcher handlers, same SCHEMA (15 knobs). Suite 420/0.
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughIntroduces ChangesProvider Registry Centralization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
serve.py (1)
161-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerive streaming handlers from the provider registry. The current wiring hardcodes
stream_unsupported_api_kindfor every nativeapi_kind, soProvider.stream_adapteris never used. Build the streaming handler map fromPROVIDERS(or remove the field/docs) to keep the registry and dispatcher in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@serve.py` around lines 161 - 166, The streaming handler wiring in the dispatcher is hardcoded for native api_kind values, so Provider.stream_adapter is bypassed and the registry can drift out of sync. Update the handler map in the serve.py streaming setup to derive entries from PROVIDERS (using each provider’s stream_adapter where available) and keep the special openai_codex path intact, or remove the unused field/docs if streaming is not supported. Refer to the streaming dispatch code that builds handlers, the Provider.stream_adapter hook, and the PROVIDERS registry when making the change.providers.py (1)
50-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThread
stream_adapterinto streaming dispatchProvider.stream_adapteris currently dead configuration:serve.pystill assemblesstreaming_callfrom a hardcoded map (stream_unsupported_api_kindfor native kinds plus a Codex special case). Either consumestream_adapterhere or drop the field to keep the registry and dispatcher in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers.py` around lines 50 - 52, `Provider.stream_adapter` is currently unused, so the registry and streaming dispatcher have drifted apart. Update the streaming path in `serve.py` to read and invoke `stream_adapter` from the selected `Provider` instead of relying on the hardcoded `streaming_call` map, while preserving the existing `stream_unsupported_api_kind` fallback and any Codex-specific behavior. Use the `Provider` dataclass and the streaming dispatch assembly in `serve.py` as the main touchpoints; if the field is not meant to be supported, remove `stream_adapter` from `providers.py` instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers.py`:
- Around line 234-240: The provider knob export is still sharing nested mutable
values from PROVIDERS because provider_knob_schema() only shallow-copies each
spec. Update provider_knob_schema() to deep-copy each knob spec before stamping
in the provider id, so mutable defaults like lists are isolated from both
PROVIDERS and settings.SCHEMA. Use the provider_knob_schema() loop over
PROVIDERS and p.knobs as the place to apply the deep copy, preserving the
existing schema shape while preventing downstream in-place mutations from
leaking back.
---
Nitpick comments:
In `@providers.py`:
- Around line 50-52: `Provider.stream_adapter` is currently unused, so the
registry and streaming dispatcher have drifted apart. Update the streaming path
in `serve.py` to read and invoke `stream_adapter` from the selected `Provider`
instead of relying on the hardcoded `streaming_call` map, while preserving the
existing `stream_unsupported_api_kind` fallback and any Codex-specific behavior.
Use the `Provider` dataclass and the streaming dispatch assembly in `serve.py`
as the main touchpoints; if the field is not meant to be supported, remove
`stream_adapter` from `providers.py` instead.
In `@serve.py`:
- Around line 161-166: The streaming handler wiring in the dispatcher is
hardcoded for native api_kind values, so Provider.stream_adapter is bypassed and
the registry can drift out of sync. Update the handler map in the serve.py
streaming setup to derive entries from PROVIDERS (using each provider’s
stream_adapter where available) and keep the special openai_codex path intact,
or remove the unused field/docs if streaming is not supported. Refer to the
streaming dispatch code that builds handlers, the Provider.stream_adapter hook,
and the PROVIDERS registry when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fc9ed45-20d1-4143-9f09-9d8f4954142b
📒 Files selected for processing (5)
providers.pyserve.pysettings.pysources/__init__.pytests/test_providers.py
| def provider_knob_schema() -> "dict[str, dict]": | ||
| """The provider knobs, namespaced `<id>.<knob>` and stamped with the provider | ||
| group — the per-provider half of settings.SCHEMA, derived from PROVIDERS.""" | ||
| schema: dict[str, dict] = {} | ||
| for p in PROVIDERS: | ||
| for name, spec in p.knobs.items(): | ||
| schema[f"{p.id}.{name}"] = {**spec, "provider": p.id} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Deep-copy knob specs before exporting them.
provider_knob_schema() only clones the top-level dict, so mutable defaults like the lists in Line 144 and Line 149 stay shared with PROVIDERS and with the merged settings.SCHEMA. Any downstream in-place mutation of spec["default"] will leak back into the registry and future callers.
Proposed fix
+from copy import deepcopy
+
def provider_knob_schema() -> "dict[str, dict]":
"""The provider knobs, namespaced `<id>.<knob>` and stamped with the provider
group — the per-provider half of settings.SCHEMA, derived from PROVIDERS."""
schema: dict[str, dict] = {}
for p in PROVIDERS:
for name, spec in p.knobs.items():
- schema[f"{p.id}.{name}"] = {**spec, "provider": p.id}
+ exported = deepcopy(spec)
+ exported["provider"] = p.id
+ schema[f"{p.id}.{name}"] = exported
return schema📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def provider_knob_schema() -> "dict[str, dict]": | |
| """The provider knobs, namespaced `<id>.<knob>` and stamped with the provider | |
| group — the per-provider half of settings.SCHEMA, derived from PROVIDERS.""" | |
| schema: dict[str, dict] = {} | |
| for p in PROVIDERS: | |
| for name, spec in p.knobs.items(): | |
| schema[f"{p.id}.{name}"] = {**spec, "provider": p.id} | |
| from copy import deepcopy | |
| def provider_knob_schema() -> "dict[str, dict]": | |
| """The provider knobs, namespaced `<id>.<knob>` and stamped with the provider | |
| group — the per-provider half of settings.SCHEMA, derived from PROVIDERS.""" | |
| schema: dict[str, dict] = {} | |
| for p in PROVIDERS: | |
| for name, spec in p.knobs.items(): | |
| exported = deepcopy(spec) | |
| exported["provider"] = p.id | |
| schema[f"{p.id}.{name}"] = exported |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@providers.py` around lines 234 - 240, The provider knob export is still
sharing nested mutable values from PROVIDERS because provider_knob_schema() only
shallow-copies each spec. Update provider_knob_schema() to deep-copy each knob
spec before stamping in the provider id, so mutable defaults like lists are
isolated from both PROVIDERS and settings.SCHEMA. Use the provider_knob_schema()
loop over PROVIDERS and p.knobs as the place to apply the deep copy, preserving
the existing schema shape while preventing downstream in-place mutations from
leaking back.
…op dead stream_adapter
Addresses the review's two blocking points.
Axis 2 (dependency direction) — break the sources ↔ providers cycle. `providers`
lazy-imports `sources.*`, yet `sources/__init__.build_registry` imported back into
`providers` — a form-level cycle, benign only because the imports were lazy, and
inconsistent with how `settings ↔ providers` was already dodged. Make `providers`
the single composition root that depends DOWN only:
- `build_source_registry` now builds the COMPLETE source list, codex included,
via a `_codex_source` factory (it only needs the codex provider id from the
catalog). `special` no longer means "source skipped" — it marks the one thing
that is still imperative in serve.py: codex's adapter + the observe/bind
coupling (its source watching its own backend's quota traffic).
- `sources/__init__` loses `build_registry` (and its `import providers`) and is
now a pure leaf. serve.py and the tests call `providers.build_source_registry`.
The edge is one-directional now: serve/settings → providers → sources/* ·
provider_adapters/*.
Axis 3 (act over potency) — delete `Provider.stream_adapter`. It was declared and
documented but read by nothing (streaming handlers hardcode
`stream_unsupported_api_kind` per native api_kind). Reintroduce only when a
provider actually ships a streaming twin and a derivation reads it.
Still behaviour-preserving: same sources built, same handlers, same SCHEMA.
Suite 421 passed / 0 failed.
Non-blocking review nit: `_i`/`_f` (env-var → int/float with a default) were duplicated in settings.py and providers.py — providers.py couldn't import them from settings without recreating the settings ↔ providers cycle, so they were copied. Pull them into `env_coerce.py`, a leaf depending on nothing but `os`. Both modules import `env_int`/`env_float` from it: one implementation, and the dependency arrows stay one-directional (settings → env_coerce ← providers, no cycle). Behaviour-identical. Suite 421 passed / 0 failed.
|
Thanks — both blocking points were spot on; addressed in three follow-up commits (still behaviour-preserving, suite 421/0). Axis 2 — sources ↔ providers cycle (
Axis 3 — Axis 1 — The |
… pages (#59) * feat(pricing): price the direct providers from their official pricing pages PR B of the pricing roadmap. openai/anthropic/google had NO price source, so every (provider, family) sat at the field default price = +inf (core/llm_policy/fields.lua): the direct candidate never passed a cost ceiling and ranked last on cost, even though the first-party API is usually cheaper than the same model via openrouter. This gives each direct provider a price source. No provider exposes a pricing API — all three publish prices on documentation pages — so the source scrapes the OFFICIAL page each provider serves and parses it. The formats differ, so each has a tailored, tested parser: - Anthropic — the Markdown twin (pricing.md): a real pipe table. (in / cache read / out) - OpenAI — the .md is MDX; pricing lives in a <TextTokenPricingTables> JSX component as [name, input, cached, output] array rows. - Google — the devsite HTML, where each model is an <h2 id="gemini-…"> (the id is the catalog family verbatim) with <td>Input price</td><td>$…</td> rows; the cache-read and per-hour storage prices share one <br>-split cell. A model label maps to a catalog family by a dot/dash/space-insensitive slug, so no hand-maintained alias map drifts ("Claude Opus 4.8" ↔ claude-opus-4-8). Durable + fail-safe, strictly off the request path: - new host_store table `provider_prices(provider_id, model_family, price_in, price_out, price_cached_in, updated_at)` — the source of truth, in Postgres. - the source upserts the table and returns in/out to the core; on a failed scrape or a restyled page past the parser it COASTS on the table (which also warm-starts a fresh process), so coverage degrades, a price never snaps to +inf. poll ~hourly. - cache-READ price is captured into the table (verified against all three live pages) for the effective-cost work in PRs C/D; the core ranks on in/out only. Builds on the modular registry (#57): each direct provider just declares a `source`. Suite 428/0; parsers verified against the live pages (anthropic 15 / openai 45 / gemini 19 families, in+out+cache). * fix(pricing): guard the durable price write — drop implausible parses, never commit (Axis 7) A per-Mtok price is invariant-bearing: it governs both routing and billing, and because pricing() COASTS on the table, a bad parse that lands there becomes a sticky wrong price the whole fleet routes/bills on — persisting even after the page is fixed. The previous write only checked `not None`. Validate before the upsert (in _resolve) and symmetrically on read (in _prices, so a poisoned coasted row can't re-stamp either): - in/out must be a positive number <= $1000/Mtok. For these first-party PAID providers a $0 is a misparse, not a free offer (unlike the marketplace's legitimate $0); the ceiling matches config.live.lua's market_price_cap and clears real prices (o1-pro is $600/Mtok out) while a misread number (a context window ~272k, a 1e6 storage figure) sits far above it. A bad row DROPS → coverage degrades, coasting on the last good value. - cache-read must be plausible AND strictly cheaper than the base input (a cache read is always a fraction of input; cached >= input means the parser grabbed output/storage/the wrong cell). Implausible cache is nulled, not dropped — the in/out row is still good. Adversarial tests: a $0 row and an over-ceiling row drop and are not committed to the table; a poisoned stored row is not re-served on coast; a cache >= input is nulled. Verified against the live pages (0 cached>=input violations across all three). Suite 432/0.
1 participant
What
The provider concern was scattered across three places — to add one provider you touched
build_registry(the source),serve.py(the wire adapter), andsettings.SCHEMA(the knobs). This makes every provider a modular composition declared in ONE place.providers.py— oneProviderrecord per provider, composing up to four aspects (the structural declaration stays inconfig.live.lua, since the core reads the catalog):sourceenabledpredicate over the catalog)adapterapi_kind(None⇒ the defaultopenai_compatible)knobsbuild_registry, the api_kind dispatcher handlers, andsettings.SCHEMAall derive fromPROVIDERS. Adding a provider = oneconfig.live.luaentry + oneProviderrow with the aspects it has.Composition, not inheritance: a provider supplies only its aspects (
source=None/adapter=None), so there's no fat base class and no re-coupling of the catalog/wire concerns #25 deliberately split.codex — the one exception (for now)
codex's scarcity-price source OBSERVES its own wire backend, so its source/adapter are wired imperatively inserve.py; the registry lists it (for its knobs) but skips building it. Generalising that single coupling would be mechanism for one case (act over potency). Follow-up: turn codex into a load-balancer provider over N subscriptions — the backend records aggregate quota state and the source DERIVES the scarcity price from it (the route_* "store raw, derive" pattern), which dissolves the exception.This is the foundation for the pricing work
The 4 pricing PRs (coverage / effective-multiplier / measured-correction) build on this: a direct provider (anthropic/openai/gemini) becoming priced = giving it a
sourcewithpricing()here.Verification
Pure refactor, behaviour-preserving: same sources, same handlers, same SCHEMA (15 knobs: 14 provider + compaction). Full suite 420 passed / 3 skipped / 0 failed (incl.
test_settings,test_sources, the Config-tab dashboard test, all unchanged) + a newtest_providers.py.Summary by CodeRabbit
New Features
Bug Fixes