-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: improve GLM-4.5 model handling to prevent hallucination and enhance tool understanding #6943
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
…nce tool understanding - Add GLM-specific system prompt enhancements to prevent file hallucination - Include clear instructions for tool usage protocol and content management - Implement message preprocessing for better GLM model understanding - Add token limit adjustments and model-specific parameters for GLM-4.5 - Enhance completePrompt method with instruction prefix for GLM models - Add comprehensive tests for GLM-specific functionality Fixes #6942
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| type MainlandZAiModelId, | ||
| ZAI_DEFAULT_TEMPERATURE, | ||
| } from "@roo-code/types" | ||
| import { Anthropic } from "@anthropic-ai/sdk" |
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 Anthropic import is included but not directly used in the implementation. Is this intentional? The ApiStream return type annotation also seems to be missing from the override declaration. Consider cleaning up unused imports or adding the proper type annotation:
| import { Anthropic } from "@anthropic-ai/sdk" | |
| override async *createMessage( | |
| systemPrompt: string, | |
| messages: Anthropic.Messages.MessageParam[], | |
| metadata?: ApiHandlerCreateMessageMetadata, | |
| ): AsyncGenerator<ApiStream> |
|
|
||
| // Check if the model is GLM-4.5 or GLM-4.5-Air | ||
| const modelId = options.apiModelId || defaultModelId | ||
| this.isGLM45 = modelId.includes("glm-4.5") |
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.
Could this cause a runtime error if both options.apiModelId and defaultModelId are undefined? Consider adding a null check:
| this.isGLM45 = modelId.includes("glm-4.5") | |
| this.isGLM45 = modelId?.includes("glm-4.5") ?? false |
|
|
||
| // For GLM models, we may need to adjust the max_tokens to leave room for proper responses | ||
| // GLM models sometimes struggle with very high token limits | ||
| const adjustedMaxTokens = this.isGLM45 && max_tokens ? Math.min(max_tokens, 32768) : max_tokens |
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 32768 token limit is hard-coded here and on line 100. Would it make sense to extract this as a constant like GLM_MAX_TOKENS = 32768 for better maintainability?
| const processedContent = msg.content.map((block: any) => { | ||
| if (block.type === "text") { | ||
| // Add clear markers for tool results to help GLM understand context | ||
| if (block.text.includes("[ERROR]") || block.text.includes("Error:")) { |
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.
This string matching logic for detecting errors/success might miss edge cases. What happens if a message contains both "Error:" and "successfully"? Consider using more robust detection or documenting the precedence rules.
| ) | ||
| }) | ||
|
|
||
| it("should enhance system prompt for GLM-4.5 models", async () => { |
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.
Good test coverage for the GLM-specific enhancements! Consider adding an edge case test for when modelId is undefined to ensure the code handles it gracefully.
|
Closing, problem with the model |
Summary
This PR addresses issue #6942 by improving GLM-4.5 model handling to prevent hallucination and enhance tool understanding.
Problem
GLM-4.5 models were experiencing:
Solution
Enhanced the ZAiHandler with GLM-specific improvements:
1. System Prompt Enhancements
2. Message Preprocessing
3. Model-Specific Parameters
4. Comprehensive Testing
Testing
Related Issue
Fixes #6942
Important
Enhances
ZAiHandlerfor GLM-4.5 models to prevent hallucinations and improve tool understanding with specific prompt and parameter adjustments.ZAiHandlerto prevent hallucinations and improve tool understanding for GLM-4.5 models.createMessage().max_tokensto 32768 and addedtop_p,frequency_penalty, andpresence_penaltyfor GLM models.completePrompt()with GLM-specific instruction prefix.preprocessMessages()for better GLM understanding.zai.spec.ts.This description was created by
for 667a79b. You can customize this summary. It will automatically update as commits are pushed.