fix: Use this.name instead of hardcoded provider name in OpenAIResponsesProvider.getModels() #1237
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…sesProvider.getModels() Previously, OpenAIResponsesProvider.getModels() hardcoded 'openai-responses' in 4 places (lines 209, 245, 256, 268) instead of using this.name. This caused provider aliases (like 'codex') to incorrectly show as 'openai-responses' in ModelsDialog, even when the provider's name was overridden via Object.defineProperty(). Changed all 4 occurrences to use this.name, matching the pattern already used in OpenAIProvider and OpenAIVercelProvider. Added test to verify that provider field uses this.name so aliases work correctly. Fixes #1209
github-actions
bot
added
the
maintainer:e2e:ok
Contributor
WalkthroughOpenAIResponsesProvider's model listing methods are refactored to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
LLxprt PR Review – PR #1237Issue AlignmentThe fix directly resolves issue #1209. The bug caused provider aliases (Synthetic, codex) to display the wrong provider name in ModelsDialog because Side Effects
Code Quality
Tests and Coverage
VerdictReady – The fix is minimal, targeted, and properly tested. It resolves the root cause of issue #1209 and maintains consistency with how other OpenAI providers handle aliases. |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
|
@CodeRabbit I see how this addresses something important, but I don't see how it addresses #1209. Am I wrong? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 268024 🏁 Script executed: Length of output: 7360
PR
What PR
The disconnect: Since OpenAIProvider already uses Verdict: You're not wrong—PR
Would you like me to investigate further what's actually happening with the Synthetic alias and ModelsDialog? |
Collaborator
Author
|
@CodeRabbit I think the issues is provider aliases and the models.dev/model dialog integration. Its giving API rather than the alias. But since we may have defaults in the alias, we actually want it to use synthetic provider alias rather than the base provider openai. Does that make sense? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
Let me investigate the models.dev integration and how ModelsDialog fetches models to understand where the disconnect is happening: 🏁 Script executed: Length of output: 39049 Perfect! Now I can see the issue. Let me examine the hydration code more closely to see where the provider field gets overwritten: 🏁 Script executed: Length of output: 4078 Excellent! I found it. Let me check the IModel interface and see which field is being used: 🏁 Script executed: Length of output: 18450 Now I need to check what happens in the registry-only fallback and the transformer: 🏁 Script executed: Length of output: 7247 Perfect! Now I can see the issue clearly. Let me check one more thing to confirm: 🏁 Script executed: Length of output: 908
The Root Cause: When you select the Synthetic alias in ModelsDialog:
The Fix: The Synthetic: ['openai'], // Not ['synthetic']This way:
The Confusion: PR Would you like me to create a fix for this? The change would be in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TLDR
OpenAIResponsesProvider.getModels()hardcodedprovider: 'openai-responses'in 4 places instead of usingthis.name, causing ModelsDialog to show the wrong provider name for aliases like Synthetic and codex that usebaseProvider: openai-responses.closes #1209
Dive Deeper
Root Cause
When a provider alias is created (e.g., Synthetic with
baseProvider: openai, or codex withbaseProvider: openai-responses), the system callsbindProviderAliasIdentity()which overridesthis.nameviaObject.defineProperty(). BothOpenAIProvider.getModels()andOpenAIVercelProvider.getModels()already usethis.namefor the provider field, so aliases work correctly for those base providers. However,OpenAIResponsesProvider.getModels()hardcodedprovider: 'openai-responses'in all 4 return paths (no-auth fallback, API fetch, API empty-response fallback, and catch fallback), ignoring the overridden name.Fix
Replaced all 4 occurrences of
provider: 'openai-responses'ingetModels()withprovider: this.name. This is the same pattern already used by OpenAIProvider and OpenAIVercelProvider. The default behavior is unchanged sincethis.namedefaults to'openai-responses'for the non-aliased provider.Test
Added a test that simulates alias identity binding (Object.defineProperty on name) and verifies that
getModels()returns models with the overridden provider name.Reviewer Test Plan
npm run testOpenAIResponsesProvider.models.test.tspasses/modeland verify the provider column shows the alias name, notopenai-responsesTesting Matrix
Linked issues / bugs
closes #1209