fix: improve error handling for LMStudio model compatibility #8576
Conversation
- Add specific error detection for connection failures - Add model not found error handling - Add context length exceeded error handling - Provide clearer error messages for debugging - Update tests to cover new error scenarios Fixes #8575
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| errorMessage.includes("context") || | ||
| errorMessage.includes("token") || | ||
| errorMessage.includes("length") | ||
| ) { |
There was a problem hiding this comment.
P1: Overly broad error detection pattern
The context length error detection will incorrectly match ANY error containing "context", "token", or "length". This creates false positives for unrelated errors like:
- "Invalid token format"
- "Response length too short"
- "Authentication token expired"
Consider using more specific patterns that match actual LM Studio context length errors, such as checking for phrases like "context length exceeded", "maximum context", or "token limit".
| ) { | |
| // Check for context length errors | |
| if ( | |
| errorMessage.toLowerCase().includes("context length exceeded") || | |
| errorMessage.toLowerCase().includes("maximum context") || | |
| errorMessage.toLowerCase().includes("token limit exceeded") | |
| ) { |
| } catch (error) { | ||
| // Handle specific error cases | ||
| const errorMessage = error instanceof Error ? error.message : String(error) | ||
|
|
||
| // Check for connection errors | ||
| if (errorMessage.includes("ECONNREFUSED") || errorMessage.includes("ENOTFOUND")) { | ||
| throw new Error( | ||
| `Cannot connect to LM Studio at ${this.options.lmStudioBaseUrl || "http://localhost:1234"}. Please ensure LM Studio is running and the server is started.`, | ||
| ) | ||
| } | ||
|
|
||
| // Check for model not found errors | ||
| if ( | ||
| errorMessage.includes("model") && | ||
| (errorMessage.includes("not found") || errorMessage.includes("does not exist")) | ||
| ) { | ||
| throw new Error( | ||
| `Model "${this.getModel().id}" not found in LM Studio. Please ensure the model is loaded in LM Studio.`, | ||
| ) | ||
| } | ||
|
|
||
| // Check for context length errors | ||
| if ( | ||
| errorMessage.includes("context") || | ||
| errorMessage.includes("token") || | ||
| errorMessage.includes("length") | ||
| ) { | ||
| throw new Error( | ||
| `Context length exceeded for model "${this.getModel().id}". Please load the model with a larger context window in LM Studio, or use a different model that supports longer contexts.`, | ||
| ) | ||
| } | ||
|
|
||
| // Use the enhanced error handler for other OpenAI-like errors | ||
| throw handleOpenAIError(error, this.providerName) | ||
| } |
There was a problem hiding this comment.
P2: Code duplication - extract error handling logic
The error handling logic (lines 100-132) is duplicated in completePrompt() (lines 219-251). This violates DRY principles and makes maintenance harder. Consider extracting this into a private method like handleLmStudioError(error: unknown): never that both methods can use.
| } catch (error) { | ||
| // If error was already processed and re-thrown above, just re-throw it | ||
| if (error instanceof Error && error.message.includes("LM Studio")) { | ||
| throw error | ||
| } | ||
|
|
||
| // Generic fallback error | ||
| throw new Error( | ||
| "Please check the LM Studio developer logs to debug what went wrong. You may need to load the model with a larger context length to work with Roo Code's prompts.", | ||
| `LM Studio error: ${error instanceof Error ? error.message : String(error)}. Please check the LM Studio developer logs for more details.`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
P2: Inconsistent error handling flow
This catch block re-checks if the error message contains "LM Studio", but the specific error handlers above already throw errors with "LM Studio" in the message. This means:
- Connection errors, model not found, and context errors will be re-thrown here (lines 174-176)
- Errors from
handleOpenAIError()will hit the generic fallback (lines 179-181)
However, handleOpenAIError() returns errors with format "LM Studio completion error: ..." which DOES contain "LM Studio", so they'll also be re-thrown at line 175, never reaching the generic fallback.
The generic fallback at lines 179-181 appears unreachable. Consider simplifying this logic or removing the unreachable code.
| for await (const _chunk of stream) { | ||
| // Should not reach here | ||
| } | ||
| }).rejects.toThrow("LM Studio completion error") |
There was a problem hiding this comment.
P3: Test expects wrong error message
The test expects "LM Studio completion error" but based on the implementation:
- The error "Unknown API Error" doesn't match connection/model/context patterns
- It gets passed to
handleOpenAIError()which returns"LM Studio completion error: Unknown API Error" - This error contains "LM Studio" so it's re-thrown at line 174-176
- The actual error message should be
"LM Studio completion error: Unknown API Error"
The test should verify the complete error message or use .toMatch() for partial matching.
| it("should handle generic API errors", async () => { | ||
| mockCreate.mockRejectedValueOnce(new Error("Unknown API Error")) | ||
|
|
||
| const stream = handler.createMessage(systemPrompt, messages) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // Should not reach here | ||
| } | ||
| }).rejects.toThrow("LM Studio completion error") | ||
| }) |
There was a problem hiding this comment.
P3: Missing test coverage for false positives
The tests don't verify that the error detection correctly distinguishes between different error types. Add tests for errors that contain "token", "context", or "length" but aren't actually context length errors, such as:
- "Invalid token format"
- "Authentication token expired"
- "Response length mismatch"
These should NOT trigger the context length error message.
hannesrudolph
added
the
Issue/PR - Triage
Code Review SummaryI've completed the review and identified the following issues: Issues Found
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| try { | ||
| results = await this.client.chat.completions.create(params) | ||
| } catch (error) { | ||
| // Handle specific error cases |
There was a problem hiding this comment.
Consider refactoring duplicated error handling logic into a helper function. The same error checks (connection errors, model not found, and context length) appear in both createMessage and completePrompt, which can lead to maintenance challenges.
| // Check for context length errors | ||
| if ( | ||
| errorMessage.includes("context") || | ||
| errorMessage.includes("token") || | ||
| errorMessage.includes("length") | ||
| ) { |
There was a problem hiding this comment.
The context length error detection will incorrectly match ANY error containing "context", "token", or "length". This creates false positives for unrelated errors like "Invalid token format", "Response length too short", or "Authentication token expired". Consider using more specific patterns that match actual LM Studio context length errors, such as checking for phrases like "context length exceeded", "maximum context", or "token limit".
| // Check for context length errors | |
| if ( | |
| errorMessage.includes("context") || | |
| errorMessage.includes("token") || | |
| errorMessage.includes("length") | |
| ) { | |
| // Check for context length errors | |
| if ( | |
| errorMessage.toLowerCase().includes("context length exceeded") || | |
| errorMessage.toLowerCase().includes("maximum context") || | |
| errorMessage.toLowerCase().includes("token limit exceeded") | |
| ) { |
| // Handle specific error cases | ||
| const errorMessage = error instanceof Error ? error.message : String(error) | ||
|
|
||
| // Check for connection errors | ||
| if (errorMessage.includes("ECONNREFUSED") || errorMessage.includes("ENOTFOUND")) { | ||
| throw new Error( | ||
| `Cannot connect to LM Studio at ${this.options.lmStudioBaseUrl || "http://localhost:1234"}. Please ensure LM Studio is running and the server is started.`, | ||
| ) | ||
| } | ||
|
|
||
| // Check for model not found errors | ||
| if ( | ||
| errorMessage.includes("model") && | ||
| (errorMessage.includes("not found") || errorMessage.includes("does not exist")) | ||
| ) { | ||
| throw new Error( | ||
| `Model "${this.getModel().id}" not found in LM Studio. Please ensure the model is loaded in LM Studio.`, | ||
| ) | ||
| } | ||
|
|
||
| // Check for context length errors | ||
| if ( | ||
| errorMessage.includes("context") || | ||
| errorMessage.includes("token") || | ||
| errorMessage.includes("length") | ||
| ) { | ||
| throw new Error( | ||
| `Context length exceeded for model "${this.getModel().id}". Please load the model with a larger context window in LM Studio, or use a different model that supports longer contexts.`, | ||
| ) | ||
| } | ||
|
|
||
| // Use the enhanced error handler for other OpenAI-like errors | ||
| throw handleOpenAIError(error, this.providerName) |
There was a problem hiding this comment.
The error handling logic is duplicated in completePrompt() (lines 219-251). This violates DRY principles and makes maintenance harder. Consider extracting this into a private method like handleLmStudioError(error: unknown): never that both methods can use.
| } catch (error) { | ||
| // If error was already processed and re-thrown above, just re-throw it | ||
| if (error instanceof Error && error.message.includes("LM Studio")) { | ||
| throw error | ||
| } | ||
|
|
||
| // Generic fallback error | ||
| throw new Error( | ||
| "Please check the LM Studio developer logs to debug what went wrong. You may need to load the model with a larger context length to work with Roo Code's prompts.", | ||
| `LM Studio error: ${error instanceof Error ? error.message : String(error)}. Please check the LM Studio developer logs for more details.`, | ||
| ) |
There was a problem hiding this comment.
This catch block re-checks if the error message contains "LM Studio", but the specific error handlers above already throw errors with "LM Studio" in the message (connection, model not found, context errors), and handleOpenAIError() returns errors with format "LM Studio completion error: ...". All these errors will be re-thrown at lines 174-176, so the generic fallback at lines 179-181 appears unreachable. Consider simplifying this logic or removing the unreachable code.
| for await (const _chunk of stream) { | ||
| // Should not reach here | ||
| } | ||
| }).rejects.toThrow("LM Studio completion error") |
There was a problem hiding this comment.
The test expects "LM Studio completion error" but based on the implementation, the error "Unknown API Error" doesn't match connection/model/context patterns, so it gets passed to handleOpenAIError() which returns "LM Studio completion error: Unknown API Error". This error contains "LM Studio" so it's re-thrown at lines 174-176. The actual error message should be "LM Studio completion error: Unknown API Error". The test should verify the complete error message or use .toMatch() for partial matching.
| it("should handle connection errors", async () => { | ||
| const connectionError = new Error("connect ECONNREFUSED 127.0.0.1:1234") | ||
| mockCreate.mockRejectedValueOnce(connectionError) | ||
|
|
||
| const stream = handler.createMessage(systemPrompt, messages) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // Should not reach here | ||
| } | ||
| }).rejects.toThrow("Please check the LM Studio developer logs to debug what went wrong") | ||
| }).rejects.toThrow("Cannot connect to LM Studio at http://localhost:1234") | ||
| }) | ||
|
|
||
| it("should handle model not found errors", async () => { | ||
| const modelError = new Error("model 'local-model' not found") | ||
| mockCreate.mockRejectedValueOnce(modelError) | ||
|
|
||
| const stream = handler.createMessage(systemPrompt, messages) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // Should not reach here | ||
| } | ||
| }).rejects.toThrow('Model "local-model" not found in LM Studio') | ||
| }) | ||
|
|
||
| it("should handle context length errors", async () => { | ||
| const contextError = new Error("context length exceeded") | ||
| mockCreate.mockRejectedValueOnce(contextError) | ||
|
|
||
| const stream = handler.createMessage(systemPrompt, messages) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // Should not reach here | ||
| } | ||
| }).rejects.toThrow("Context length exceeded") | ||
| }) | ||
|
|
||
| it("should handle generic API errors", async () => { | ||
| mockCreate.mockRejectedValueOnce(new Error("Unknown API Error")) | ||
|
|
||
| const stream = handler.createMessage(systemPrompt, messages) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // Should not reach here | ||
| } | ||
| }).rejects.toThrow("LM Studio completion error") | ||
| }) |
There was a problem hiding this comment.
The tests don't verify that the error detection correctly distinguishes between different error types. Add tests for errors that contain "token", "context", or "length" but aren't actually context length errors, such as "Invalid token format", "Authentication token expired", or "Response length mismatch". These should NOT trigger the context length error message.
Description
This PR attempts to address Issue #8575 where Roo Code errors out when using the Kwaipilot/KAT-Dev Q8 model with LM Studio or Jan.ai.
Changes
Enhanced Error Handling
Test Coverage
Technical Details
The changes enhance the error handling in the LmStudioHandler class to provide more specific and helpful error messages when issues occur. This should help users:
Limitations
While this PR improves error handling significantly, the original issue report lacks specific error details. The implementation addresses common error scenarios that could cause Roo Code to "error out" with local models, but without the exact error messages from the Kwaipilot model, we cannot guarantee this fully resolves the specific compatibility issue.
Testing
Next Steps
If this doesn't fully resolve the issue, we may need:
Fixes #8575
Feedback Welcome
This PR represents an attempt to improve the user experience when errors occur with LM Studio models. Feedback and testing with the actual Kwaipilot/KAT-Dev model would be greatly appreciated.
Important
Improves error handling in
LmStudioHandlerfor connection, model not found, and context length errors, with comprehensive test coverage.LmStudioHandlerinlm-studio.tsnow detects connection errors (ECONNREFUSED,ENOTFOUND) and throws specific messages.lmstudio.spec.tsadds tests for connection errors, model not found, context length, and generic API errors.This description was created by
for e407b1e. You can customize this summary. It will automatically update as commits are pushed.