Add scoped schema 2.4 provider discovery catalog - #1041
PierreLeGuen wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Review · Status🟩 CompletedIronLoop completed the review and posted it to GitHub. ResultRun detailsAutomatic trigger · attempt 1 of 3 · completed in 22m 19s |
Claude Code Review — PR #1041Reviewed the full diff ( No blocking correctness bug found. The additive route, Notes in priority order — none are hard blockers, but (1) and (2) are worth fixing before launch. 1. Zero observability on the 503 path (
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0167951ff3
ℹ️ 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".
| "stop" => json!({"type": "array"}), | ||
| // Variable token-ID keys cannot be represented by a closed object. | ||
| "logit_bias" => json!({"type": "unknown"}), | ||
| _ => continue, |
There was a problem hiding this comment.
top_a capability
When the GLM catalog declares top_a—a value accepted by the admin vocabulary and forwarded by the completion path—the wildcard arm silently drops it from supported_parameters. Because this endpoint is OpenRouter's discovery feed, the generated catalog incorrectly reports that the deployment lacks this supported sampling control; handle top_a as a range alongside top_p and min_p.
Useful? React with 👍 / 👎.
|
|
||
| /// OpenRouter provider schema 2.4; only the explicitly approved launch model. | ||
| /// Readiness is independent of the general Cloud catalog and defaults to false. | ||
| #[utoipa::path( |
There was a problem hiding this comment.
The runtime route is deliberately unauthenticated, but ApiDoc installs global session_token/api_key requirements, so this operation inherits them without an explicit security(()) override. Generated clients and the API documentation will therefore require bearer credentials for the public discovery URL; add the same empty security override used by /v1/models.
Useful? React with 👍 / 👎.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review · Summary
Found one low-severity catalog capability omission.
Findings: 🟡 Low 1
Code-specific findings are attached to the diff.
Validation
- ✅ OpenRouter route tests — Five focused route and serialization tests passed.
- ✅ Provider schema 2.4 document — The generated catalog fixture validated against the provider schema.
Review details
- Run:
b0b24ac8-ea45-4246-925c-097f55652860 - Attempts: 1
| "stop" => json!({"type": "array"}), | ||
| // Variable token-ID keys cannot be represented by a closed object. | ||
| "logit_bias" => json!({"type": "unknown"}), | ||
| _ => continue, |
There was a problem hiding this comment.
🟡 Low · Expose top_a when the catalog supports it
top_a is an allowed catalog sampling parameter and is accepted by the completion path, but this conversion falls through to continue. If the launch model declares support for it, the 2.4 document omits the capability, so OpenRouter cannot expose that supported control. Map it to the same range descriptor used for the other continuous sampling parameters.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| .get_models_with_pricing() | ||
| .await | ||
| .map_err(|_| unavailable())?; |
There was a problem hiding this comment.
The map_err(|_| unavailable()) silently discards the original error from get_models_with_pricing(), replacing it with a generic "catalog_unavailable" 503. The established convention in the same codebase (completions.rs:1509-1513) logs the original error with tracing::warn! before falling back to a default. Without logging, operators cannot distinguish a DB outage from a transient fault during incident response. Consider logging the error before mapping it to the 503 response.
Suggestion:
| .get_models_with_pricing() | |
| .await | |
| .map_err(|_| unavailable())?; | |
| .get_models_with_pricing() | |
| .await | |
| .map_err(|e| { | |
| tracing::warn!(error = %e, "Failed to read model catalog for OpenRouter endpoint"); | |
| unavailable() | |
| })?; |
| return Err(unavailable()); | ||
| } | ||
| let model = super::completions::model_with_pricing_to_info(model); | ||
| data.push(document(model, state.ready, state.zdr).ok_or_else(unavailable)?); |
There was a problem hiding this comment.
When document() returns None for a model that passed the initial model_name == LAUNCH_MODEL filter, the handler returns 503 (via ok_or_else(unavailable)?). This conflates two distinct conditions: (1) the catalog service is genuinely unavailable, and (2) the catalog returned the launch model but its metadata failed validation. A 503 for a metadata validation failure is misleading — the service is up, but the model configuration is non-compliant for this launch contract. Consider logging a distinct warning when document() returns None so operators can diagnose configuration issues versus availability issues.
Suggestion:
| data.push(document(model, state.ready, state.zdr).ok_or_else(unavailable)?); | |
| match document(model, state.ready, state.zdr) { | |
| Some(doc) => data.push(doc), | |
| None => { | |
| tracing::warn!( | |
| model = %model.id, | |
| "Launch model present in catalog but failed OpenRouter metadata validation" | |
| ); | |
| return Err(unavailable()); | |
| } | |
| } |
| pub fn from_env(models_service: Arc<dyn ModelsServiceTrait>) -> Self { | ||
| Self { | ||
| models_service, | ||
| ready: std::env::var("OPENROUTER_GLM53_FLASH_READY").as_deref() == Ok("true"), |
There was a problem hiding this comment.
OpenRouterState::from_env reads OPENROUTER_GLM53_FLASH_READY and OPENROUTER_GLM53_FLASH_ZDR only at router construction time (process startup). Changing these env vars at runtime has no effect without a full process restart. This is acceptable for a launch gate, but consider adding a code comment noting this behavior so operators toggling readiness flags aren't surprised by stale values.
Suggestion:
| pub fn from_env(models_service: Arc<dyn ModelsServiceTrait>) -> Self { | |
| Self { | |
| models_service, | |
| ready: std::env::var("OPENROUTER_GLM53_FLASH_READY").as_deref() == Ok("true"), | |
| /// Reads readiness and ZDR flags from the environment at call time. | |
| /// Since this is invoked only during router construction (process startup), | |
| /// changes to these env vars require a full process restart to take effect. | |
| pub fn from_env(models_service: Arc<dyn ModelsServiceTrait>) -> Self { | |
| Self { | |
| models_service, | |
| ready: std::env::var("OPENROUTER_GLM53_FLASH_READY").as_deref() == Ok("true"), |
1 participant
The existing
/v1/modelsresponse uses the legacy flat provider document and includes the entire Cloud catalog. Add a separate public/v1/openrouter/modelsfeed using provider schema 2.4, initially scoped to the active canonicalz-ai/glm-5.3-flashmodel.The feed derives string USD prices, modality capabilities, identity and limits from the catalog. It stays hidden by default and requires explicit launch/ZDR declarations plus catalog readiness and datacenter metadata. It rejects unsupported pricing/deployment shapes and leaves the existing OpenAI catalog response intact. The ZDR environment variable declares a verified policy; it does not change data handling.
Validation: five focused route/serialization tests, independent validation of the emitted document against the official 2.4 schema,
cargo clippy -p api --lib -- -D warnings, formatting and diff checks. No deployment is included.Related: nearai/cvm-compose-files#253 disables request-body disk buffering in the GLM nginx configuration.