feat(providers): add bounded model discovery contract - #652
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces registry-driven live model discovery with trusted URL resolution, bounded parsing, filtering, deduplication, transport-aware routing, safer authentication validation, catalog metadata normalization, tests, and localized configuration documentation. ChangesProvider discovery and transport handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderConfig
participant ProviderRegistry
participant ModelDiscovery
participant ProviderCatalog
ProviderConfig->>ProviderRegistry: resolve transport and discovery policy
ProviderRegistry->>ModelDiscovery: provide trusted URL, query, filter, and limits
ModelDiscovery->>ProviderCatalog: return bounded normalized model items
ProviderCatalog->>ProviderConfig: publish live catalog or fallback result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b373dddb30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs-site/src/content/docs/ru/reference/configuration.md (1)
166-174: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSync the
baseUrlrow in the locale docs
docs-site/src/content/docs/ja/reference/configuration.md:139,docs-site/src/content/docs/ko/reference/configuration.md:146, anddocs-site/src/content/docs/ru/reference/configuration.md:166still say only “upstream API base URL.” They should matchdocs-site/src/content/docs/reference/configuration.md:255anddocs-site/src/content/docs/zh-cn/reference/configuration.md:138, which now explain the collision-safe preserve rule and link to#fixed-provider-endpoints. Without that text, these locales miss the new fixed-endpoint routing behavior.🤖 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 `@docs-site/src/content/docs/ru/reference/configuration.md` around lines 166 - 174, Update the baseUrl documentation row in the Japanese, Korean, and Russian configuration references to match the current canonical wording. Include the collision-safe preserve rule for fixed-provider endpoint routing and link to the fixed-provider-endpoints section, while retaining the existing base URL description.Source: Path instructions
🤖 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 `@docs-site/src/content/docs/reference/configuration.md`:
- Around line 474-482: Separate the live-discovery limits paragraph from the
“Static model allowlists” introduction in both configuration documentation
sites: docs-site/src/content/docs/reference/configuration.md lines 474-482 and
docs-site/src/content/docs/ru/reference/configuration.md lines 281-288. Move
each limits paragraph below its corresponding “liveModels is false and models is
empty” sentence, or replace the preceding colon with a period, while keeping the
English and Russian content synchronized.
In `@src/codex/catalog/provider-fetch.ts`:
- Around line 192-232: Update normalizedStringList and the capability-key
normalization inside modelCapabilities to remove MODEL_DISCOVERY_CONTROL_CHARS
before trimming, lowercasing, whitespace normalization, and truncation. Reuse
the existing sanitizer used by boundedOwnedBy so capability, input-modality,
reasoning-effort, and boolean capability names cannot retain control characters
or ANSI escapes.
In `@src/providers/registry.ts`:
- Around line 48-65: Update ProviderModelDiscoverySpec so url and path form a
mutually exclusive union requiring exactly one of them, while preserving the
shared discovery fields across both variants. Revise the path documentation to
explicitly state that query strings and fragments are disallowed, matching the
validation already implemented in model discovery.
In `@src/server/management/provider-routes.ts`:
- Around line 349-351: Update the non-ok response branch in the provider test
handler to cancel res.body before returning the error response. Preserve the
existing status and latency error payload, and handle a missing body safely.
- Around line 340-347: The management probe around resolveProviderModelDiscovery
must validate the resolved modelsUrl with providerDestinationResolvedError
before calling fetch. Reuse the existing catalog destination-policy check and
return or handle the error consistently, ensuring upstream headers are never
sent to a rebound destination.
---
Outside diff comments:
In `@docs-site/src/content/docs/ru/reference/configuration.md`:
- Around line 166-174: Update the baseUrl documentation row in the Japanese,
Korean, and Russian configuration references to match the current canonical
wording. Include the collision-safe preserve rule for fixed-provider endpoint
routing and link to the fixed-provider-endpoints section, while retaining the
existing base URL description.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ffd24ed5-17f4-4222-8f08-e921b92113a6
📒 Files selected for processing (22)
docs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/configuration.mdsrc/codex/catalog/parsing.tssrc/codex/catalog/provider-fetch.tssrc/oauth/index.tssrc/oauth/key-providers.tssrc/oauth/login-cli.tssrc/providers/derive.tssrc/providers/model-discovery.tssrc/providers/registry.tssrc/router.tssrc/server/auth-cors.tssrc/server/management/provider-routes.tssrc/server/management/shared.tsstructure/01_runtime.mdtests/codex-catalog.test.tstests/fixtures/provider-model-discovery.jsontests/provider-model-discovery-contract.test.tstests/umans-provider.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/provider-connection-test.test.ts (1)
82-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the Google-style
{models:[...]}fallback branch.The tests here exercise the OpenAI-style
{data:[...]}shape (Lines 97-112, 153-182, 215-225) and the malformed-shape case, but I don't see a test that drivesprobe()through theextractModelEnvelopeRows(bounded.value, discovery.maxModels, ["models"])fallback added insrc/server/management/provider-routes.ts:382-387(the branch that keeps Google'smodels[].nameshape connectivity-only). If it's not covered elsewhere in this file's unshown ranges, add one — it's the exact code path where an equivalent branch is missing insrc/codex/catalog/provider-fetch.ts(see the critical finding there), so a regression test here would also help pin the contract going forward.A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem. As per path instructions.
🤖 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 `@tests/provider-connection-test.test.ts` around lines 82 - 226, Add a focused regression test in this provider probe suite that mocks a successful Google-style response containing a models array with name fields, invokes probe() for a Google provider, and asserts the probe succeeds with the expected model count. Ensure the test exercises the extractModelEnvelopeRows fallback using the ["models"] envelope while preserving the connectivity-only handling of Google model names.src/providers/registry.ts (1)
1211-1225: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
preserveCustomDestination: truetosrc/providers/registry.ts:596.providerMatchesRegistryTransport()returnstrueforauthKind: "key"entries unless that flag is set, so the new guard atsrc/codex/catalog/provider-fetch.ts:694still merges the registry’s OpenAI rows into a customopenai-apikeyprovider with a different base URL or adapter.🤖 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 `@src/providers/registry.ts` around lines 1211 - 1225, Add preserveCustomDestination: true to the registry entry for the provider at the referenced location, so providerMatchesRegistryTransport applies its fixed-destination and adapter matching checks and prevents mismatched custom openai-apikey providers from merging registry rows.
🤖 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.
Outside diff comments:
In `@src/providers/registry.ts`:
- Around line 1211-1225: Add preserveCustomDestination: true to the registry
entry for the provider at the referenced location, so
providerMatchesRegistryTransport applies its fixed-destination and adapter
matching checks and prevents mismatched custom openai-apikey providers from
merging registry rows.
In `@tests/provider-connection-test.test.ts`:
- Around line 82-226: Add a focused regression test in this provider probe suite
that mocks a successful Google-style response containing a models array with
name fields, invokes probe() for a Google provider, and asserts the probe
succeeds with the expected model count. Ensure the test exercises the
extractModelEnvelopeRows fallback using the ["models"] envelope while preserving
the connectivity-only handling of Google model names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: de4b650e-ebd3-4184-b382-180d77805a57
📒 Files selected for processing (11)
docs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/configuration.mdsrc/codex/catalog/provider-fetch.tssrc/oauth/index.tssrc/providers/registry.tssrc/server/management/provider-routes.tsstructure/01_runtime.mdtests/provider-connection-test.test.tstests/provider-model-discovery-contract.test.ts
92ab57d to
98d12a9
Compare
Resolve conflicts with lidge-jun#617 array support by teaching extractProviderModelItems to accept top-level /models arrays while retaining bounded reads, redirect:error, destination policy, and eligibility filtering on the management probe.
98d12a9 to
169e637
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/oauth/index.ts (1)
763-777: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake namespace validation and configuration writes atomic across both login flows.
Both flows perform a check-then-write sequence while
saveConfigonly atomically replaces the file; it does not make the preceding validation atomic. Use a shared cross-process lock or compare-and-swap/re-read strategy around collision validation and provider persistence.
src/oauth/index.ts#L763-L777: protect the late OAuth collision check, provider upsert, and save as one transaction.src/oauth/login-cli.ts#L143-L151: protect the key-login collision check, provider assignment, and save as one transaction.🤖 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 `@src/oauth/index.ts` around lines 763 - 777, Make namespace validation and provider persistence atomic in both flows by applying the same shared cross-process lock or compare-and-swap/re-read transaction strategy. In src/oauth/index.ts lines 763-777, protect the late collision check, upsertOAuthProvider, and saveLatestConfig together; in src/oauth/login-cli.ts lines 143-151, protect the key-login collision check, provider assignment, and save together, preserving the existing collision behavior.
🤖 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 `@docs-site/src/content/docs/reference/configuration.md`:
- Around line 499-503: Update the live discovery documentation to state that any
malformed row or invalid model ID rejects the entire discovery result before
caching and triggers the normal stale/static fallback, without partial caching.
Apply this synchronized clarification in
docs-site/src/content/docs/reference/configuration.md:499-503,
docs-site/src/content/docs/ja/reference/configuration.md:273-276,
docs-site/src/content/docs/ko/reference/configuration.md:300-303,
docs-site/src/content/docs/ru/reference/configuration.md:330-334, and
docs-site/src/content/docs/zh-cn/reference/configuration.md:287-290, translating
it appropriately for each locale.
In `@src/providers/model-discovery.ts`:
- Around line 238-281: Update comparableScalar and predicateMatches to preserve
a truncation/indeterminate signal when oversized strings or arrays exceeding
MODEL_DISCOVERY_MAX_FILTER_VALUES are skipped. Propagate that result through
providerModelMatchesDiscoveryFilter and reject rows when a noneOf predicate is
indeterminate, while retaining existing matching behavior for fully inspected
values and other predicates.
In `@src/server/management/shared.ts`:
- Line 204: Add a focused regression test covering
stripRegistryOnlyStaticHeaders() through the provider-management save path: save
a name-colliding provider with explicit headers, assert registry-only static
headers are stripped when providerMatchesRegistryTransport() matches, and
preserved for a non-matching transport. Place it alongside the existing provider
discovery/connection tests and exercise the relevant provider-routes save
behavior.
In `@tests/fixtures/provider-model-discovery.json`:
- Around line 3-20: Extend the provider model discovery fixture with a fourth
model row containing nested metadata.capabilities and metadata.limits alongside
conflicting flat fields. Set metadata.limits.max_context_length to 131072 and
the flat context_size to 8192, and include nested capabilities so
catalogHintsFromModelsApiItem exercises metadata-first precedence and verifies
the nested values win.
In `@tests/umans-provider.test.ts`:
- Line 217: Extend the validation tests around validateApiKey in
tests/umans-provider.test.ts with mocked-fetch cases for the registry-resolved
models URL and query parameters, and verify requests use redirect: "error". Add
a redirect-failure case asserting validateApiKey returns "unknown", while
preserving the existing valid-key coverage.
---
Outside diff comments:
In `@src/oauth/index.ts`:
- Around line 763-777: Make namespace validation and provider persistence atomic
in both flows by applying the same shared cross-process lock or
compare-and-swap/re-read transaction strategy. In src/oauth/index.ts lines
763-777, protect the late collision check, upsertOAuthProvider, and
saveLatestConfig together; in src/oauth/login-cli.ts lines 143-151, protect the
key-login collision check, provider assignment, and save together, preserving
the existing collision behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d494a6ca-831d-4ffc-a57a-22f576d635f6
📒 Files selected for processing (23)
docs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/configuration.mdsrc/codex/catalog/parsing.tssrc/codex/catalog/provider-fetch.tssrc/oauth/index.tssrc/oauth/key-providers.tssrc/oauth/login-cli.tssrc/providers/derive.tssrc/providers/model-discovery.tssrc/providers/registry.tssrc/router.tssrc/server/auth-cors.tssrc/server/management/provider-routes.tssrc/server/management/shared.tsstructure/01_runtime.mdtests/codex-catalog.test.tstests/fixtures/provider-model-discovery.jsontests/provider-connection-test.test.tstests/provider-model-discovery-contract.test.tstests/umans-provider.test.ts
| Live discovery rejects a response before caching when it exceeds 4 MiB or 2,000 raw model rows. | ||
| Built-in presets may lower either limit and filter mixed catalogs to chat-eligible rows. An | ||
| oversized or malformed response follows the normal stale/configured fallback path, while | ||
| ineligible rows are excluded. A valid result with zero eligible rows remains an authoritative | ||
| empty catalog; OpenCodex never silently truncates an over-limit response. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document whole-catalog rejection for malformed rows and invalid model IDs across all configuration locales.
The runtime SOT already states that malformed OpenAI envelopes and invalid model IDs are rejected before caching. The configuration pages currently say only that malformed responses use fallback, which can imply that malformed individual rows are silently filtered. Add one synchronized sentence explaining that any malformed row or invalid model ID rejects the complete discovery result and triggers stale/static fallback without partial caching.
docs-site/src/content/docs/reference/configuration.md#L499-L503: add the explicit row-level validation and whole-catalog rejection semantics to the English source.docs-site/src/content/docs/ja/reference/configuration.md#L273-L276: mirror the same whole-catalog failure behavior in Japanese.docs-site/src/content/docs/ko/reference/configuration.md#L300-L303: mirror the same whole-catalog failure behavior in Korean.docs-site/src/content/docs/ru/reference/configuration.md#L330-L334: mirror the same whole-catalog failure behavior in Russian.docs-site/src/content/docs/zh-cn/reference/configuration.md#L287-L290: mirror the same whole-catalog failure behavior in Simplified Chinese.
As per path instructions: “Check that user-facing docs stay in sync with actual CLI/API behavior and that translated locale pages (ja, ko, ru, zh-cn) are not left contradicting the English source.”
📍 Affects 5 files
docs-site/src/content/docs/reference/configuration.md#L499-L503(this comment)docs-site/src/content/docs/ja/reference/configuration.md#L273-L276docs-site/src/content/docs/ko/reference/configuration.md#L300-L303docs-site/src/content/docs/ru/reference/configuration.md#L330-L334docs-site/src/content/docs/zh-cn/reference/configuration.md#L287-L290
🤖 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 `@docs-site/src/content/docs/reference/configuration.md` around lines 499 -
503, Update the live discovery documentation to state that any malformed row or
invalid model ID rejects the entire discovery result before caching and triggers
the normal stale/static fallback, without partial caching. Apply this
synchronized clarification in
docs-site/src/content/docs/reference/configuration.md:499-503,
docs-site/src/content/docs/ja/reference/configuration.md:273-276,
docs-site/src/content/docs/ko/reference/configuration.md:300-303,
docs-site/src/content/docs/ru/reference/configuration.md:330-334, and
docs-site/src/content/docs/zh-cn/reference/configuration.md:287-290, translating
it appropriately for each locale.
Source: Path instructions
| { | ||
| "id": "acme/chat-pro", | ||
| "type": "chat", | ||
| "context_size": 262144, | ||
| "max_input_tokens": 250000, | ||
| "input_modalities": ["text", "image"], | ||
| "supported_features": ["tools", "reasoning"] | ||
| }, | ||
| { | ||
| "id": "acme/embed-small", | ||
| "type": "embedding", | ||
| "context_length": 8192 | ||
| }, | ||
| { | ||
| "id": "acme/video-gen", | ||
| "type": "video", | ||
| "capabilities": ["video-generation"] | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a row exercising the metadata.capabilities / metadata.limits precedence branch.
catalogHintsFromModelsApiItem (src/codex/catalog/provider-fetch.ts Lines 265-267) prefers the nested envelope — plainRecord(metadata?.capabilities) ?? plainRecord(item.capabilities) and plainRecord(metadata?.limits) for max_context_length/max_input_tokens — over the flat fields. Every row in this fixture uses only the flat shape (context_size, context_length, capabilities, max_input_tokens), so the metadata-first precedence introduced alongside the new CatalogModel.capabilities field is never exercised by fixture-based coverage.
🧪 Suggested fourth row
{
"id": "acme/video-gen",
"type": "video",
"capabilities": ["video-generation"]
+ },
+ {
+ "id": "acme/chat-nested",
+ "type": "chat",
+ "context_size": 8192,
+ "metadata": {
+ "capabilities": { "reasoning_effort": ["low", "high"] },
+ "limits": { "max_context_length": 131072, "max_input_tokens": 120000 }
+ }
}The assertion should show metadata.limits.max_context_length (131072) winning over the flat context_size (8192).
As per path instructions, "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
📝 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.
| { | |
| "id": "acme/chat-pro", | |
| "type": "chat", | |
| "context_size": 262144, | |
| "max_input_tokens": 250000, | |
| "input_modalities": ["text", "image"], | |
| "supported_features": ["tools", "reasoning"] | |
| }, | |
| { | |
| "id": "acme/embed-small", | |
| "type": "embedding", | |
| "context_length": 8192 | |
| }, | |
| { | |
| "id": "acme/video-gen", | |
| "type": "video", | |
| "capabilities": ["video-generation"] | |
| } | |
| { | |
| "id": "acme/chat-pro", | |
| "type": "chat", | |
| "context_size": 262144, | |
| "max_input_tokens": 250000, | |
| "input_modalities": ["text", "image"], | |
| "supported_features": ["tools", "reasoning"] | |
| }, | |
| { | |
| "id": "acme/embed-small", | |
| "type": "embedding", | |
| "context_length": 8192 | |
| }, | |
| { | |
| "id": "acme/video-gen", | |
| "type": "video", | |
| "capabilities": ["video-generation"] | |
| }, | |
| { | |
| "id": "acme/chat-nested", | |
| "type": "chat", | |
| "context_size": 8192, | |
| "metadata": { | |
| "capabilities": { "reasoning_effort": ["low", "high"] }, | |
| "limits": { "max_context_length": 131072, "max_input_tokens": 120000 } | |
| } | |
| } |
🤖 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 `@tests/fixtures/provider-model-discovery.json` around lines 3 - 20, Extend the
provider model discovery fixture with a fourth model row containing nested
metadata.capabilities and metadata.limits alongside conflicting flat fields. Set
metadata.limits.max_context_length to 131072 and the flat context_size to 8192,
and include nested capabilities so catalogHintsFromModelsApiItem exercises
metadata-first precedence and verifies the nested values win.
Source: Path instructions
| }) as typeof fetch; | ||
|
|
||
| const valid = await validateApiKey(KEY_LOGIN_PROVIDERS.umans, "sk-umans-valid"); | ||
| const valid = await validateApiKey("umans", KEY_LOGIN_PROVIDERS.umans, "sk-umans-valid"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add regression coverage for the new validation behavior.
These edits update the argument order correctly, but they do not verify the new URL-resolution and redirect: "error" behavior in src/oauth/key-providers.ts. Add mocked-fetch cases asserting the registry-resolved models URL/query and that redirect failures return "unknown".
As per path instructions, behavior changes in src/** should have focused regression tests near the relevant subsystem.
Also applies to: 238-238
🤖 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 `@tests/umans-provider.test.ts` at line 217, Extend the validation tests around
validateApiKey in tests/umans-provider.test.ts with mocked-fetch cases for the
registry-resolved models URL and query parameters, and verify requests use
redirect: "error". Add a redirect-failure case asserting validateApiKey returns
"unknown", while preserving the existing valid-key coverage.
Source: Path instructions
Wibias
left a comment
There was a problem hiding this comment.
Verdict: Approve (useful feature; land after CI green on tip)
What this is
Phase-1 contract for registry-owned live model discovery (umbrella #572): bounded fetches (4 MiB / 2k rows), declarative eligibility filters, redirect: \"error\", destination-policy checks, OAuth host pinning for fixed presets, and same-named custom-provider collision preservation. Not a bugfix — foundational hardening before provider batches (#653+). Worth landing.
Bugbot
No bugs found on the discovery contract tip.
Security review (maintainer)
Reviewed auth/discovery surface on tip 169e6374:
- Credentials do not follow redirects (
redirect: \"error\"on catalog + probe). - Probe and catalog both run
providerDestinationResolvedErrorbefore fetch. - Registry
modelDiscovery/preserveCustomDestinationstay out of persisted provider config (covered by tests). - Fixed OAuth discovery pins to registry adapter/baseUrl before relative-path resolution.
- Untrusted metadata strips control chars; live token limits require positive safe integers.
No remaining medium+ security blockers in the changed runtime.
Codex / CodeRabbit threads
All prior threads were marked resolved; verified fixed on tip:
- Codex P1 OAuth pin, P2 integer limits, P2 eligible probe counts — addressed in
4e8e7d59+ tests. - Rabbit body-cancel on non-OK probe responses, destination policy on probe, control-char metadata sanitization — present and covered.
Maintainer fix pushed (in this PR)
Branch was DIRTY vs current dev (#617 Together top-level /models arrays). Merged dev and taught extractProviderModelItems to accept top-level arrays so catalog + probe keep #617 behavior without dropping bounds/filter/redirect/destination policy. Added regression coverage. Tip: 169e6374.
Not requesting changes
No open must-fix left that belongs in a follow-up. Optional: wait for windows-latest + CodeRabbit on the new tip before merge.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/auth-cors.ts (1)
321-353: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore
provider.noteinsafeConfigDTO
src/server/auth-cors.ts:321-353only copies a fixed allowlist into the DTO, so persisted notes written throughPATCH /api/providersdisappear unless a registry entry also supplies one. That breaks the provider settings UI, which readsitem.notefrom this DTO. Add"note"to the copied fields or fall back toprovider.notewhen no registry note applies.🤖 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 `@src/server/auth-cors.ts` around lines 321 - 353, Update safeConfigDTO’s provider field allowlist to preserve persisted notes by copying the provider’s "note" field, while retaining the existing registryNote override behavior for applicable registry providers.
🤖 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 `@docs-site/src/content/docs/reference/configuration.md`:
- Line 282: Update the liveModels documentation to describe ${baseUrl}/models as
the generic OpenAI-style default and defer routing to adapter-specific behavior.
Apply the equivalent qualification in
docs-site/src/content/docs/reference/configuration.md:282,
docs-site/src/content/docs/ja/reference/configuration.md:162,
docs-site/src/content/docs/ko/reference/configuration.md:169,
docs-site/src/content/docs/ru/reference/configuration.md:192, and
docs-site/src/content/docs/zh-cn/reference/configuration.md:160, keeping all
locale descriptions synchronized.
In `@src/providers/model-discovery.ts`:
- Around line 149-161: Update the URL resolution block in the model-discovery
function to catch malformed effectiveBaseUrl, spec.url, spec.path, or defaultUrl
values and degrade by returning defaultUrl when parsing fails. Preserve the
existing path and query resolution behavior for valid URLs, while ensuring the
fallback itself is returned safely without allowing URL construction errors to
escape.
---
Outside diff comments:
In `@src/server/auth-cors.ts`:
- Around line 321-353: Update safeConfigDTO’s provider field allowlist to
preserve persisted notes by copying the provider’s "note" field, while retaining
the existing registryNote override behavior for applicable registry providers.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8a281926-c1ce-4d36-9ba6-e2cff61f9205
📒 Files selected for processing (23)
docs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/configuration.mdsrc/codex/catalog/parsing.tssrc/codex/catalog/provider-fetch.tssrc/oauth/index.tssrc/oauth/key-providers.tssrc/oauth/login-cli.tssrc/providers/derive.tssrc/providers/model-discovery.tssrc/providers/registry.tssrc/router.tssrc/server/auth-cors.tssrc/server/management/provider-routes.tssrc/server/management/shared.tsstructure/01_runtime.mdtests/codex-catalog.test.tstests/fixtures/provider-model-discovery.jsontests/provider-connection-test.test.tstests/provider-model-discovery-contract.test.tstests/umans-provider.test.ts
| | `defaultModel?` | `string` | Model used when this provider is selected without an explicit model. | | ||
| | `models?` | `string[]` | Seed/fallback model list. When `liveModels` is `false`, these are the only discovered models. | | ||
| | `liveModels?` | `boolean` | Fetch the provider's live `/models` catalog on start/sync (default `true`). Set `false` to use only configured `models`. | | ||
| | `liveModels?` | `boolean` | Fetch the provider's live model catalog on start/sync (default `true`). Built-in presets may use a trusted registry URL/query/filter; custom providers default to `${baseUrl}/models`. Set `false` to use only configured `models`. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify the ${baseUrl}/models default as adapter-specific.
The runtime does not use that path for every custom provider: Anthropic uses /v1/models, Google AI Studio uses /v1beta/models?pageSize=1000, and some adapters use bespoke discovery behavior. The documentation should describe ${baseUrl}/models as the generic OpenAI-style default and explicitly defer to adapter-specific routing.
docs-site/src/content/docs/reference/configuration.md#L282-L282: qualify the EnglishliveModelsdescription.docs-site/src/content/docs/ja/reference/configuration.md#L162-L162: mirror the adapter-specific qualification in Japanese.docs-site/src/content/docs/ko/reference/configuration.md#L169-L169: mirror the adapter-specific qualification in Korean.docs-site/src/content/docs/ru/reference/configuration.md#L192-L192: mirror the adapter-specific qualification in Russian.docs-site/src/content/docs/zh-cn/reference/configuration.md#L160-L160: mirror the adapter-specific qualification in Simplified Chinese.
As per path instructions, user-facing docs under docs-site/** must stay synchronized with actual CLI/API behavior and translated locale pages must not contradict the English source.
📍 Affects 5 files
docs-site/src/content/docs/reference/configuration.md#L282-L282(this comment)docs-site/src/content/docs/ja/reference/configuration.md#L162-L162docs-site/src/content/docs/ko/reference/configuration.md#L169-L169docs-site/src/content/docs/ru/reference/configuration.md#L192-L192docs-site/src/content/docs/zh-cn/reference/configuration.md#L160-L160
🤖 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 `@docs-site/src/content/docs/reference/configuration.md` at line 282, Update
the liveModels documentation to describe ${baseUrl}/models as the generic
OpenAI-style default and defer routing to adapter-specific behavior. Apply the
equivalent qualification in
docs-site/src/content/docs/reference/configuration.md:282,
docs-site/src/content/docs/ja/reference/configuration.md:162,
docs-site/src/content/docs/ko/reference/configuration.md:169,
docs-site/src/content/docs/ru/reference/configuration.md:192, and
docs-site/src/content/docs/zh-cn/reference/configuration.md:160, keeping all
locale descriptions synchronized.
Source: Path instructions
| let resolved: URL; | ||
| if (spec.url) { | ||
| resolved = new URL(spec.url); | ||
| } else if (spec.path) { | ||
| const base = new URL(effectiveBaseUrl.endsWith("/") ? effectiveBaseUrl : `${effectiveBaseUrl}/`); | ||
| resolved = spec.path.startsWith("/") | ||
| ? new URL(spec.path, base.origin) | ||
| : new URL(spec.path, base); | ||
| } else { | ||
| resolved = new URL(defaultUrl); | ||
| } | ||
| return appendDiscoveryQuery(resolved, spec.query).toString(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Do the callers guard against a synchronous throw here?
rg -nP -C8 'resolveProviderModelDiscoveryUrl\s*\(' --type=ts -g '!tests/**'Repository: lidge-jun/opencodex
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== model-discovery.ts context =="
nl -ba src/providers/model-discovery.ts | sed -n '1,260p'
echo
echo "== search for related symbols and call sites =="
rg -n --hidden --glob '!tests/**' 'resolveProviderModelDiscoveryUrl|appendDiscoveryQuery|model-discovery|discoveryUrl|defaultUrl|effectiveBaseUrl|spec\.url|spec\.path' srcRepository: lidge-jun/opencodex
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/providers/model-discovery.ts =="
cat -n src/providers/model-discovery.ts | sed -n '1,260p'
echo
echo "== search for related symbols and call sites =="
rg -n --hidden --glob '!tests/**' 'resolveProviderModelDiscoveryUrl|appendDiscoveryQuery|model-discovery|discoveryUrl|defaultUrl|effectiveBaseUrl|spec\.url|spec\.path' srcRepository: lidge-jun/opencodex
Length of output: 15535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/oauth/index.ts excerpt =="
cat -n src/oauth/index.ts | sed -n '480,545p'
echo
echo "== src/oauth/key-providers.ts excerpt =="
cat -n src/oauth/key-providers.ts | sed -n '1,150p'
echo
echo "== src/server/management/provider-routes.ts excerpt =="
cat -n src/server/management/provider-routes.ts | sed -n '180,230p'Repository: lidge-jun/opencodex
Length of output: 12369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/oauth/index.ts around discovery helpers =="
cat -n src/oauth/index.ts | sed -n '500,540p'
echo
echo "== src/oauth/key-providers.ts around discovery usage =="
cat -n src/oauth/key-providers.ts | sed -n '70,110p'
echo
echo "== all direct call sites =="
rg -n --hidden --glob '!tests/**' 'resolveProviderModelDiscoveryUrl\s*\(' srcRepository: lidge-jun/opencodex
Length of output: 4521
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== baseUrl validation / normalization =="
rg -n --hidden --glob '!tests/**' 'baseUrl.*non-empty|string|new URL\(.*baseUrl|provider.*baseUrl|normalize.*baseUrl|validate.*baseUrl|isSameEndpoint' src/server src/router.ts src/oauth src/providersRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/router.ts around effectiveBaseUrl =="
cat -n src/router.ts | sed -n '145,185p'
echo
echo "== src/server/management/provider-routes.ts around baseUrl handling =="
cat -n src/server/management/provider-routes.ts | sed -n '190,215p'Repository: lidge-jun/opencodex
Length of output: 4669
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== provider baseUrl validation helpers =="
cat -n src/server/auth-cors.ts | sed -n '218,260p'
echo
echo "== model discovery registry validation =="
cat -n src/providers/model-discovery.ts | sed -n '75,95p'
echo
echo "== buildModelsRequest callers =="
rg -n --hidden --glob '!tests/**' 'buildModelsRequest\s*\(' srcRepository: lidge-jun/opencodex
Length of output: 5040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/server/management/provider-routes.ts around buildModelsRequest =="
cat -n src/server/management/provider-routes.ts | sed -n '330,370p'
echo
echo "== src/codex/catalog/provider-fetch.ts around buildModelsRequest =="
cat -n src/codex/catalog/provider-fetch.ts | sed -n '360,410p'
echo
echo "== providerBaseUrlConfigError definition =="
rg -n --hidden --glob '!tests/**' 'function providerBaseUrlConfigError|export function providerBaseUrlConfigError' src/server/auth-cors.ts srcRepository: lidge-jun/opencodex
Length of output: 5427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/config.ts providerBaseUrlConfigError =="
cat -n src/config.ts | sed -n '450,510p'
echo
echo "== config validation entrypoints using providerManagementConfigError =="
rg -n --hidden --glob '!tests/**' 'providerManagementConfigError\(' srcRepository: lidge-jun/opencodex
Length of output: 3786
Guard discovery URL construction against stale invalid baseUrl values
src/providers/model-discovery.ts:151-158 still throws if effectiveBaseUrl or defaultUrl is malformed. src/oauth/key-providers.ts:91-106 already swallows that, but src/oauth/index.ts:507-534 and src/codex/catalog/provider-fetch.ts:386-387 call this before their own try/catch, so a legacy or migrated provider config can abort discovery instead of degrading. Wrap the new URL(...) block and return defaultUrl on parse failure.
🤖 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 `@src/providers/model-discovery.ts` around lines 149 - 161, Update the URL
resolution block in the model-discovery function to catch malformed
effectiveBaseUrl, spec.url, spec.path, or defaultUrl values and degrade by
returning defaultUrl when parsing fails. Preserve the existing path and query
resolution behavior for valid URLs, while ensuring the fallback itself is
returned safely without allowing URL construction errors to escape.
Summary
This is phase 1 of umbrella issue #572. It intentionally lands the shared contract before provider batches; the Baseten pilot follows in #653. The umbrella issue must remain open for the follow-on provider PRs.
Behavior Notes
redirect: "error"so credentials cannot follow a redirect to another destination.modelDiscoverydeclaration retain their adapter-derived discovery endpoint; this PR does not mechanically migrate old presets.baseUrlduring discovery, matching normal routing, before xAI/Copilot-style transport overrides run.Security
This touches discovery authentication and destination handling. The implementation keeps trust metadata registry-only, blocks credential-bearing redirects, and preserves custom-provider destinations. Explicit maintainer security review is requested before merge.
Verification
bun run typecheckbun run privacy:scanbun run lint:guiChecklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
baseUrl/liveModelssemantics, fixed-provider endpoint priority rules, and static allowlist + discovery limits/empty-catalog behavior.Tests