fix: spec regeneration doesn't usercustom models #495
Conversation
- Introduced `getProviderForModelWithExplicit` method in ProviderFactory to allow explicit provider specification, enhancing flexibility in provider selection. - Updated `simpleQuery` and `streamingQuery` functions to utilize the new method for improved provider handling. - Modified `PhaseModelSelector` to pass explicit provider information during model selection. - Enhanced `PhaseModelEntry` and related interfaces to support optional provider fields for dynamic model routing.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 |
Summary of ChangesHello @Shironex, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where custom models, particularly those not easily identifiable by their model ID patterns, were not being correctly utilized during spec regeneration. The core solution involves introducing an explicit Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Shironex
added
Bug
![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 a useful feature to allow specifying an explicit model provider, which is particularly helpful for custom models. The changes are well-implemented across the server, UI, and shared type libraries.
I have one main piece of feedback regarding the new getProviderForModelWithExplicit method in provider-factory.ts. The current implementation silently falls back to the 'claude' provider if an explicit provider is not found, which could mask configuration issues. I've suggested a change to throw an error in this case for more robust error handling. I also noted some code duplication that could be addressed to improve maintainability.
| if (!provider) { | ||
| // Fallback to claude if provider not found | ||
| const claudeReg = providerRegistry.get('claude'); | ||
| if (claudeReg) { | ||
| return claudeReg.factory(); | ||
| } | ||
| throw new Error(`No provider found for model: ${modelId}`); | ||
| } |
There was a problem hiding this comment.
When an explicitProvider is given but not found, the code silently falls back to the 'claude' provider. This can hide configuration errors and lead to unexpected behavior. If a user explicitly specifies a provider, it's better to throw an error if that provider is not found.
Additionally, the logic in this if (!provider) block is identical to the one in the getProviderForModel method (lines 120-127). The disconnection check logic is also duplicated. Consider refactoring the common parts into a private helper method to reduce code duplication and improve maintainability.
if (!provider) {
if (explicitProvider) {
throw new Error("Explicitly specified provider '" + explicitProvider + "' not found.");
}
// Fallback to claude if provider not found
const claudeReg = providerRegistry.get('claude');
if (claudeReg) {
return claudeReg.factory();
}
throw new Error("No provider found for model: " + modelId);
}
Changes from branch feature/v0.11.0rc-1768420878341-xzhv