refactor(ai): single provider registry as the backend source of truth - #1075
Conversation
There was a problem hiding this comment.
1 issue found across 11 files
Confidence score: 4/5
- In
api/credentials_service.py, deriving provider metadata dynamically from the registry can hide which providers are OpenAI-compatible and which discovery URLs are expected, so a registry mismatch could slip through and cause runtime credential/discovery failures; before merging, add explicit tests/assertions for provider compatibility and exact discovery URLs (or keep a clear in-file allowlist) to preserve review-time visibility.
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="api/credentials_service.py">
<violation number="1" location="api/credentials_service.py:388">
P2: Replacing the explicit `url_map` with a dynamic derivation from the registry removes in-file visibility of which providers are OpenAI-compatible and what their discovery URLs are. The existing tests verify structural constraints (e.g., exactly one required env var) and that every provider has a discovery function, but they do not assert the exact set of providers with `openai_compat_discovery_url` or pin their URL values. Without that guard, a future registry edit could silently omit, alter, or misassign a discovery URL—causing `discover_models` to return nothing or misparse a non-OpenAI response as `data[]`. Consider adding a test that asserts the exact expected mapping.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "openrouter": "https://openrouter.ai/api/v1/models", | ||
| "dashscope": "https://dashscope.aliyuncs.com/compatible-mode/v1/models", | ||
| "minimax": "https://api.minimax.io/v1/models", | ||
| name: spec.openai_compat_discovery_url |
There was a problem hiding this comment.
P2: Replacing the explicit url_map with a dynamic derivation from the registry removes in-file visibility of which providers are OpenAI-compatible and what their discovery URLs are. The existing tests verify structural constraints (e.g., exactly one required env var) and that every provider has a discovery function, but they do not assert the exact set of providers with openai_compat_discovery_url or pin their URL values. Without that guard, a future registry edit could silently omit, alter, or misassign a discovery URL—causing discover_models to return nothing or misparse a non-OpenAI response as data[]. Consider adding a test that asserts the exact expected mapping.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/credentials_service.py, line 388:
<comment>Replacing the explicit `url_map` with a dynamic derivation from the registry removes in-file visibility of which providers are OpenAI-compatible and what their discovery URLs are. The existing tests verify structural constraints (e.g., exactly one required env var) and that every provider has a discovery function, but they do not assert the exact set of providers with `openai_compat_discovery_url` or pin their URL values. Without that guard, a future registry edit could silently omit, alter, or misassign a discovery URL—causing `discover_models` to return nothing or misparse a non-OpenAI response as `data[]`. Consider adding a test that asserts the exact expected mapping.</comment>
<file context>
@@ -426,16 +383,11 @@ def models_endpoint(url: str) -> str:
- "openrouter": "https://openrouter.ai/api/v1/models",
- "dashscope": "https://dashscope.aliyuncs.com/compatible-mode/v1/models",
- "minimax": "https://api.minimax.io/v1/models",
+ name: spec.openai_compat_discovery_url
+ for name, spec in PROVIDERS.items()
+ if spec.openai_compat_discovery_url
</file context>
Provider metadata (env vars, modalities, connection-test models, OpenAI-compatible discovery URLs, display names, docs links) is now defined once in open_notebook/ai/provider_registry.py. The existing surfaces are derived from it, keeping every import and call-site shape unchanged: - api/credentials_service.py: PROVIDER_ENV_CONFIG, PROVIDER_MODALITIES and the discovery url_map are built from the registry - open_notebook/ai/connection_tester.py: TEST_MODELS derived - open_notebook/ai/model_discovery.py: OPENAI_COMPAT_PROVIDERS built from registry entries with a discovery URL (quirk hooks stay local) The SupportedProvider Literal (typing, can't be built at runtime) and the frontend provider tables remain manual copies; the cross-check tests now assert registry keys == Literal == frontend list, plus registry internal consistency and discovery-table coverage. New GET /api/providers endpoint exposes the registry (name, display name, modalities, docs_url, env-configured status) so clients can stop hardcoding provider lists (frontend adoption is a follow-up). Docs updated: open_notebook/AGENTS.md and docs/7-DEVELOPMENT/credentials.md now describe the registry instead of the four-place sync rule.
- Build PROVIDERS via _build_registry(), which raises on a duplicate provider name at import time instead of silently dropping the earlier spec (dict-comprehension behavior); regression test added - Pin the exact OpenAI-compatible provider -> discovery URL mapping in a test so a registry edit can't silently drop or misassign a URL - Give TEST_MODELS a real type annotation (Dict[str, Tuple[Optional[str], str]]) instead of bare dict
fd03371 to
1a66680
Compare
|
Addressed the review findings in 1a66680:
Rebased on latest main; 436 tests pass, ruff clean, mypy unchanged vs the main baseline. |
|
The re-review finding was already addressed in 1a66680: |
Register anthropic_compatible in the provider registry so its env config, modalities, test model, and /api/providers entry are derived from PROVIDERS (lfnovo#1075's single source of truth); the only manual copy is the SupportedProvider Literal. Maps to esperanto's anthropic provider with a custom base_url, and re-injects that base_url via ChatAnthropic since esperanto's to_langchain drops it. Connection-test and model discovery mirror the openai_compatible siblings, including DNS-rebinding pinning (prepare_pinned_http_target). A single shared validator enforces the base_url + api_key requirement on both the create and update paths.
Register anthropic_compatible in the provider registry so its env config, modalities, test model, and /api/providers entry are derived from PROVIDERS (lfnovo#1075's single source of truth); the only manual copy is the SupportedProvider Literal. Maps to esperanto's anthropic provider with a custom base_url, and re-injects that base_url via ChatAnthropic since esperanto's to_langchain drops it. Connection-test and model discovery mirror the openai_compatible siblings, including DNS-rebinding pinning (prepare_pinned_http_target). A single shared validator enforces the base_url + api_key requirement on both the create and update paths.
Register anthropic_compatible in the provider registry so its env config, modalities, test model, and /api/providers entry are derived from PROVIDERS (lfnovo#1075's single source of truth); the only manual copy is the SupportedProvider Literal. Maps to esperanto's anthropic provider with a custom base_url, and re-injects that base_url via ChatAnthropic since esperanto's to_langchain drops it. Connection-test and model discovery mirror the openai_compatible siblings, including DNS-rebinding pinning (prepare_pinned_http_target). A single shared validator enforces the base_url + api_key requirement on both the create and update paths.
Register anthropic_compatible in the provider registry so its env config, modalities, test model, and /api/providers entry are derived from PROVIDERS (#1075's single source of truth); the only manual copy is the SupportedProvider Literal. Maps to esperanto's anthropic provider with a custom base_url, and re-injects that base_url via ChatAnthropic since esperanto's to_langchain drops it. Connection-test and model discovery mirror the openai_compatible siblings, including DNS-rebinding pinning (prepare_pinned_http_target). A single shared validator enforces the base_url + api_key requirement on both the create and update paths. Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: Luis Novo <lfnovo@gmail.com>
Summary
Adding an AI provider today means hand-syncing ~6 independent dicts. This PR makes
open_notebook/ai/provider_registry.pythe backend's single source of truth and derives everything else from it, without changing the shape of any existing surface — every import and call site keeps working.Before: adding a provider touched 6 places
api/models.py—SupportedProviderLiteralapi/credentials_service.py—PROVIDER_ENV_CONFIGapi/credentials_service.py—PROVIDER_MODALITIES(+ the discoveryurl_map)open_notebook/ai/connection_tester.py—TEST_MODELSopen_notebook/ai/model_discovery.py—OPENAI_COMPAT_PROVIDERS/ discovery wiringfrontend/src/lib/providers.tsx—ALL_PROVIDERS,PROVIDER_MODALITIES,PROVIDER_DOCSAfter: registry + 2 manual copies (both test-enforced)
ProviderSpectoPROVIDERSinopen_notebook/ai/provider_registry.py— env vars, modalities, test model, discovery URL, display name, docs link, all in one place.PROVIDER_ENV_CONFIG,PROVIDER_MODALITIES,TEST_MODELS, the discoveryurl_mapandOPENAI_COMPAT_PROVIDERSare now thin derivations in their current homes.SupportedProviderLiteral (typing — can't be built at runtime).frontend/src/lib/providers.tsx.tests/test_credential_provider_validation.pyenforces registry keys == Literal == frontend list, checks each spec's internal consistency (env config present, compat-discovery providers have exactly one required env var), and asserts the discovery function table covers the registry.New endpoint
GET /api/providersexposes the registry:name,display_name,modalities,docs_url,env_configured. The frontend still uses its own tables — migrating it to consume this endpoint (and killing the regex-based.tsxcross-check test) is a deliberate follow-up, not part of this PR.Docs
open_notebook/AGENTS.mdanddocs/7-DEVELOPMENT/credentials.mdupdated from the "keep four locations in sync" rule to the registry + 2 copies rule.Verification
uv run pytest tests/— 434 passedruff check .— cleanmypyon the touched modules — 36 errors, identical set to the origin/main baseline (all pre-existing, none in the new/changed code)