fix: surface Cohere model fetch errors - #285
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCohere model discovery now throws an error for unsuccessful HTTP responses. Successful responses without usable model data still use the default model list. ChangesCohere model discovery
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/providers/cohere.ts">
<violation number="1" location="src/providers/cohere.ts:79">
P3: The new throw-on-non-OK path in CohereProvider.fetchModels has no test coverage, even though it is the exact behavior this fix introduces (previous tests in tests/providers/provider-adapters.test.mjs only cover `complete`). Add a test that mocks a non-OK `/models` response (e.g. status 401) and asserts `fetchModels` rejects, mirroring how `complete` surfaces API errors at lines 234-249, so the stale-model regression this PR fixes stays guarded.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| if (!response.ok) { | ||
| return ['command-r-plus', 'command-r', 'command-xlarge', 'command-large']; | ||
| throw new Error(`Failed to fetch models (${response.status}): ${response.statusText}`); |
There was a problem hiding this comment.
P3: The new throw-on-non-OK path in CohereProvider.fetchModels has no test coverage, even though it is the exact behavior this fix introduces (previous tests in tests/providers/provider-adapters.test.mjs only cover complete). Add a test that mocks a non-OK /models response (e.g. status 401) and asserts fetchModels rejects, mirroring how complete surfaces API errors at lines 234-249, so the stale-model regression this PR fixes stays guarded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/providers/cohere.ts, line 79:
<comment>The new throw-on-non-OK path in CohereProvider.fetchModels has no test coverage, even though it is the exact behavior this fix introduces (previous tests in tests/providers/provider-adapters.test.mjs only cover `complete`). Add a test that mocks a non-OK `/models` response (e.g. status 401) and asserts `fetchModels` rejects, mirroring how `complete` surfaces API errors at lines 234-249, so the stale-model regression this PR fixes stays guarded.</comment>
<file context>
@@ -76,7 +76,7 @@ export class CohereProvider implements Provider {
if (!response.ok) {
- return ['command-r-plus', 'command-r', 'command-xlarge', 'command-large'];
+ throw new Error(`Failed to fetch models (${response.status}): ${response.statusText}`);
}
</file context>
|
1 participant
Summary
Closes #284
Testing
npm test— 356 passed, 0 failednpm run format:check— passedSummary by CodeRabbit