fix(codex): expose native model ids in catalog#2012
fix(codex): expose native model ids in catalog#2012diegosouzapw merged 2 commits intodiegosouzapw:release/v3.8.0from
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces native routing for Codex models, specifically adding support for 'codex-auto-review' and routing 'gpt-5.5' requests from Codex-native clients to the 'codex' provider. It also updates the models catalog to include bare and prefixed IDs for these models. Feedback includes a critical fix for a ReferenceError caused by an undefined contextLength variable and a suggestion to consolidate redundant logic in the model registration loop.
| ...(contextLength ? { context_length: contextLength } : {}), | ||
| ...(visionFields || {}), |
There was a problem hiding this comment.
The variable contextLength is not defined in this scope, which will cause a ReferenceError at runtime when this block is executed. Since the preceding model registration blocks in this loop do not include context length metadata, this line should be removed.
| ...(contextLength ? { context_length: contextLength } : {}), | |
| ...(visionFields || {}), | |
| ...(visionFields || {}), |
| if ( | ||
| canonicalProviderId === "codex" && | ||
| CODEX_NATIVE_UNPREFIXED_MODELS.has(model.id) && | ||
| !models.some((existingModel) => existingModel.id === model.id) | ||
| ) { | ||
| models.push({ | ||
| id: model.id, | ||
| object: "model", | ||
| created: timestamp, | ||
| owned_by: canonicalProviderId, | ||
| permission: [], | ||
| root: model.id, | ||
| parent: `${canonicalProviderId}/${model.id}`, | ||
| ...(contextLength ? { context_length: contextLength } : {}), | ||
| ...(visionFields || {}), | ||
| }); | ||
| } |
There was a problem hiding this comment.
This block is redundant with the loop starting at line 399, which also handles adding unprefixed versions of models in CODEX_NATIVE_UNPREFIXED_MODELS. The loop at line 399 is more comprehensive as it ensures all three ID variants (alias-prefixed, provider-prefixed, and bare) are registered correctly. Consider consolidating this logic to avoid duplication and potential inconsistencies.
|
Addressed both review comments in a separate commit: I removed the redundant Codex bare-model insertion block from |
|
Thank you @Tr0sT for your contribution! This has been reviewed and is now merged into |
diegosouzapw
merged commit d96f0c2
into
diegosouzapw:release/v3.8.0
2 participants
Summary
auto-review can resolve them
gpt-5.5to thecodexprovider instead of OpenAI
gpt-5.5resolution unchangedValidation
node --import tsx/esm --test tests/unit/chat-helpers.test.tsnode --import tsx/esm --test tests/unit/plan3-p0.test.tsnode --import tsx/esm --test tests/unit/models-catalog- route.test.tsgit diff --checknpm run test:unit: 4145 passed, 0 failed/api/v1/responses/compactsmoke no longer fails withNo credentials for provider: openai; logs showgpt-5.5 -> codex/gpt- 5.5