-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat(api): add cognima provider support #8868
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add support for the Cognima AI provider, including API key handling, model fetching, and handler integration across the codebase.
Review SummaryI've reviewed the Cognima provider implementation. The overall structure follows the established patterns for adding new providers, but there are a few issues that need to be addressed: Issues Found
|
| name: "cognima", | ||
| baseURL: "https://cog2.cognima.com.br/openai/v1", | ||
| apiKey: options.cognimaApiKey, | ||
| modelId: options.cognimaModelId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cognimaModelId property is referenced here but is not defined in the cognimaSchema in packages/types/src/provider-settings.ts. The schema only includes cognimaApiKey. You need to add cognimaModelId: z.string().optional() to the cognimaSchema (around line 358) to match the pattern used by other providers like Groq.
|
|
||
| import type { ModelInfo } from "@roo-code/types" | ||
|
|
||
| import { parseApiPrice } from "../../../shared/cost" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import parseApiPrice is not used; please remove it to keep the code clean.
| import { parseApiPrice } from "../../../shared/cost" |
|
|
||
| import type { ModelInfo } from "@roo-code/types" | ||
|
|
||
| import { parseApiPrice } from "../../../shared/cost" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parseApiPrice import is unused in this file and should be removed to keep the code clean.
|
|
||
| export type CognimaModelId = string | ||
|
|
||
| export const cognimaDefaultModelId: CognimaModelId = "gpt-4o" No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing newline at end of file. Add a newline after the last line to follow standard conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review complete. Found 3 issues that need to be addressed before approval. Please see the inline comments for details.
Add cognimaModelId to provider settings schema and implement model selection logic in the UI hook for the cognima provider. Remove unused import from cognima fetcher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two of the three issues have been resolved. One issue remains: the missing newline at the end of packages/types/src/providers/cognima.ts.
Add cognima as a new provider in the webview message handler initialization, extending support for additional model sources.
| ollama: {}, | ||
| lmstudio: {}, | ||
| roo: {}, | ||
| cognima: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cognima: {} entry initializes an empty model record, but there's no corresponding entry in the modelFetchPromises array (around lines 789-820) to actually fetch Cognima models from the API. Since Cognima is defined as a dynamic provider and has a getCognimaModels function in src/api/providers/fetchers/cognima.ts, you should add a fetch entry similar to other dynamic providers like glama or unbound. For example:
{
key: "cognima",
options: {
provider: "cognima",
apiKey: apiConfiguration.cognimaApiKey,
},
}Without this, users won't be able to see or select any Cognima models in the UI.
| expect(mockGetModels).toHaveBeenCalledWith({ provider: "requesty", apiKey: "requesty-key" }) | ||
| expect(mockGetModels).toHaveBeenCalledWith({ provider: "glama" }) | ||
| expect(mockGetModels).toHaveBeenCalledWith({ provider: "unbound", apiKey: "unbound-key" }) | ||
| expect(mockGetModels).toHaveBeenCalledWith({ provider: "unbound",apiKey: "unbound-key" }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after comma. Should be { provider: "unbound", apiKey: "unbound-key" } for consistency with the rest of the codebase.
| expect(mockGetModels).toHaveBeenCalledWith({ provider: "unbound",apiKey: "unbound-key" }) | |
| expect(mockGetModels).toHaveBeenCalledWith({ provider: "unbound", apiKey: "unbound-key" }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review complete. There are still 2 minor issues to address before approval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest commit adds proper test coverage for the Cognima provider. No new issues found in this update. There are still 2 minor issues remaining from the previous review that need to be addressed before approval.
|
Thanks for the contribution!. We’re not planning to add new third-party providers at this time, as we’re focusing on maintaining the existing integrations. Closing this for now, but thank you again for the PR and for your interest in improving Roo Code. |
Add the AI provider Cognima, it"s a Brazilian provider, I used it in openai compatible but I think it would be interesting to have it natively.
Important
Adds support for Cognima AI provider, including handler, model fetching, and provider settings integration.
CognimaHandlerto handle Cognima API requests inindex.tsandproviders/cognima.ts.createMessageinCognimaHandlerto convert messages and handle streaming responses.getCognimaModelsinfetchers/cognima.tsto fetch models from Cognima API.cognimatodynamicProvidersandproviderNamesinprovider-settings.ts.cognimaSchemainprovider-settings.tsfor provider configuration.providerSettingsSchemaDiscriminatedandproviderSettingsSchemato includecognimaSchema.cognimaApiKeytoSECRET_STATE_KEYSinglobal-settings.ts.CognimaHandlerinproviders/index.tsandapi/index.ts.MODELS_BY_PROVIDERinprovider-settings.tsto include Cognima.This description was created by
for 5e5c5d5. You can customize this summary. It will automatically update as commits are pushed.