-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: handle square bracket HTML entities in Gemini responses #7577
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
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 because apparently I trust no one, not even myself.
| .replace(/"/g, '"') | ||
| .replace(/'/g, "'") | ||
| .replace(/'/g, "'") | ||
| .replace(/[/g, "[") |
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.
Is the order of replacements intentional here? I notice that the ampersand replacement is placed last, which seems correct since replacing it first could interfere with other entity replacements. Would it be worth adding a comment explaining why it must be last to prevent future reordering mistakes?
| .replace(/[/g, "[") | ||
| .replace(/]/g, "]") | ||
| .replace(/[/g, "[") | ||
| .replace(/]/g, "]") |
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.
While this PR fixes the square bracket issue, are there other HTML entities that Gemini might escape that we're not handling? For example: parentheses (( and )), curly braces ({ and }), or other mathematical/programming symbols. Might be worth investigating what other entities Gemini commonly escapes in code contexts.
| expect(unescapeHtmlEntities(undefined as unknown as string)).toBe(undefined) | ||
| }) | ||
|
|
||
| it("unescapes square bracket entities (numeric)", () => { |
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.
Would it make sense to group all the square bracket tests together in a nested describe block for better organization? This would make the test structure clearer and easier to navigate.
| expect(unescapeHtmlEntities("matrix[i][j]")).toBe("matrix[i][j]") | ||
| }) | ||
|
|
||
| it("handles C# array syntax with escaped square brackets", () => { |
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.
Great test coverage! This test case directly addresses the reported issue with C# array syntax. The test is well-documented with a comment explaining its purpose.
daniel-lxs
left a comment
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.
LGTM
Description
This PR fixes an issue where square brackets and array indices were being stripped from C# code when using Google Gemini models (specifically gemini-2.5-pro).
Problem
When using Gemini API with Roo Code to generate C# code containing arrays, the square brackets were missing from the output. For example:
string[] myArray = new string[5];string myArray = new string5;Solution
The issue was that Gemini was escaping square brackets as HTML entities (
[,],[,]), but ourunescapeHtmlEntitiesfunction wasn't handling these specific entities.This PR adds support for unescaping both numeric and named HTML entities for square brackets in the
unescapeHtmlEntitiesfunction.Changes
[,],[, and]entities insrc/utils/text-normalization.tsTesting
Fixes #7576
Important
Fixes missing square brackets in C# code by updating
unescapeHtmlEntitiesto handle specific HTML entities.unescapeHtmlEntitiesintext-normalization.ts.[,],[, and]entities.text-normalization.spec.ts.This description was created by
for 5b2fd3f. You can customize this summary. It will automatically update as commits are pushed.