fix(model): return clear error instead of silent openai default for unrecognized models#2492
Conversation
…nrecognized models
![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 improves model resolution by replacing the silent 'openai' default with a specific 'model_not_found' error when a provider cannot be inferred. The chat handler was updated to catch this error and return a descriptive bad request response. Feedback includes a requirement to add unit tests for these changes as per the repository style guide and a recommendation to replace 'any' type casting with a shared interface to improve maintainability.
| // Last resort: no provider could be inferred — return a clear error instead | ||
| // of silently defaulting to "openai", which would produce a misleading | ||
| // "No credentials for provider: openai" response when the model name | ||
| // is unrecognised (e.g. a missing combo, a typo, or a bare model id | ||
| // that doesn't exist in any provider's catalog). | ||
| return { | ||
| provider: "openai", | ||
| provider: null, | ||
| model: modelId, | ||
| extendedContext, | ||
| errorType: "model_not_found", | ||
| errorMessage: `Unable to determine provider for model '${modelId}'. Use a provider/model prefix (e.g. openai/${modelId}) or ensure the model is added as a combo entry.`, | ||
| }; |
There was a problem hiding this comment.
The repository style guide (Rule 9) requires that all changes to production code in open-sse/ include tests. Please add unit tests (e.g., in tests/unit/services/model.test.ts) to verify that resolveModelByProviderInference correctly returns the model_not_found error object when no provider can be inferred, ensuring this new logic is covered and preventing regressions.
References
- Always include tests when changing production code (src/, open-sse/, electron/, bin/). (link)
| // model_not_found: raised by resolveModelByProviderInference when no | ||
| // provider could be inferred — return a clear error instead of the | ||
| // misleading "openai" default that the old code silently fell back to. | ||
| if ((modelInfo as any).errorType === "model_not_found") { |
There was a problem hiding this comment.
The use of any casting for modelInfo is a maintainability issue as it bypasses TypeScript's type checking. While this pattern exists elsewhere in the file, it would be better to define a shared interface for the model resolution result (including errorType and errorMessage) in open-sse/services/model.ts and use it here to avoid unsafe casts.
Code Review SummaryStatus: 2 Issues Found (Already Reported) | Recommendation: Address existing comments before merge Overview
Overview of ChangesThis PR fixes a misleading error behavior where Changes:
Existing Inline Comments
Positive Observations
Files Reviewed (4 files)
Reviewed by qwen3.6-plus · 303,940 tokens |
- Updated existing 'made-up-model' test in services-branch-hardening to expect model_not_found instead of silent openai default - Added new resolveModelOrError test in chat-helpers for unrecognised bare model names returning model_not_found error
diegosouzapw
merged commit 8d43cbd
into
diegosouzapw:release/v3.8.2
|
Thank you for this contribution! 🎉 Your fix has been integrated into the upcoming v3.8.2 release. Great work! |
2 participants
Description
When a model name cannot be resolved to any known provider,
resolveModelByProviderInference()was silently defaulting to{ provider: "openai" }, producing a misleading "No credentials for provider: openai" error.Changes
open-sse/services/model.ts:
resolveModelByProviderInference()from{ provider: "openai" }to{ provider: null, errorType: "model_not_found", errorMessage: "..." }src/sse/handlers/chatHelpers.ts:
errorType === "model_not_found"inresolveModelOrError()Related Issue
Closes #2491