-
Notifications
You must be signed in to change notification settings - Fork 14.2k
fix(core-tools): resolve Jupyter Notebook and JSON corruption in write_file #28000
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
Changes from 7 commits
fe7d77d
12a68e4
08559eb
ff8ea46
09462ef
1a7cb75
aef101b
043016a
eb66932
58ff7f2
d8f2878
7e9a1b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -356,6 +356,7 @@ describe('WriteFileTool', () => { | |
| abortSignal, | ||
| true, | ||
| true, // aggressiveUnescape | ||
| false, // isJsonLike | ||
| ); | ||
| expect(result.correctedContent).toBe(correctedContent); | ||
| expect(result.originalContent).toBe(''); | ||
|
|
@@ -389,9 +390,46 @@ describe('WriteFileTool', () => { | |
| abortSignal, | ||
| true, | ||
| false, // aggressiveUnescape | ||
| false, // isJsonLike | ||
| ); | ||
| }); | ||
|
|
||
| it('should set isJsonLike to true and aggressiveUnescape to false for .ipynb, .json, map, yaml, toml, and common JSON-based dotfiles', async () => { | ||
| const abortSignal = new AbortController().signal; | ||
| const filesToTest = [ | ||
| 'notebook.ipynb', | ||
| 'data.json', | ||
| 'bundle.js.map', | ||
| 'workflow.yaml', | ||
| 'config.yml', | ||
| 'Cargo.toml', | ||
| '.eslintrc', | ||
| '.prettierrc', | ||
| ]; | ||
|
amelidev marked this conversation as resolved.
Comment on lines
+397
to
+422
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should update the test suite to verify that the newly added structured formats (XML, SVG, CSV, TSV, Terraform/HCL, Terraform state, and Web App Manifests) are also correctly identified as it('should set isJsonLike to true and aggressiveUnescape to false for .ipynb, .json, map, yaml, toml, and common JSON-based dotfiles', async () => {
const abortSignal = new AbortController().signal;
const filesToTest = [
'notebook.ipynb',
'data.json',
'bundle.js.map',
'workflow.yaml',
'config.yml',
'Cargo.toml',
'.eslintrc',
'.prettierrc',
'.env',
'.env.local',
'config.ini',
'.gitconfig',
'data.xml',
'image.svg',
'data.csv',
'data.tsv',
'main.tf',
'terraform.tfvars',
'config.hcl',
'terraform.tfstate',
'manifest.webmanifest',
]; |
||
|
|
||
| for (const file of filesToTest) { | ||
| mockEnsureCorrectFileContent.mockClear(); | ||
| mockEnsureCorrectFileContent.mockResolvedValue('content'); | ||
|
|
||
| const filePath = path.join(rootDir, file); | ||
| await getCorrectedFileContent( | ||
| mockConfig, | ||
| filePath, | ||
| 'content', | ||
| abortSignal, | ||
| ); | ||
|
|
||
| expect(mockEnsureCorrectFileContent).toHaveBeenCalledWith( | ||
| 'content', | ||
| mockBaseLlmClientInstance, | ||
| abortSignal, | ||
| true, | ||
| false, // aggressiveUnescape | ||
| true, // isJsonLike | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| it('should call ensureCorrectFileContent for an existing file', async () => { | ||
| const filePath = path.join(rootDir, 'existing_corrected_file.txt'); | ||
| const originalContent = 'Original existing content.'; | ||
|
|
@@ -416,6 +454,7 @@ describe('WriteFileTool', () => { | |
| abortSignal, | ||
| true, | ||
| true, // aggressiveUnescape | ||
| false, // isJsonLike | ||
| ); | ||
| expect(result.correctedContent).toBe(correctedProposedContent); | ||
| expect(result.originalContent).toBe(originalContent); | ||
|
|
@@ -493,6 +532,7 @@ describe('WriteFileTool', () => { | |
| abortSignal, | ||
| true, | ||
| true, // aggressiveUnescape | ||
| false, // isJsonLike | ||
| ); | ||
| expect(confirmation).toEqual( | ||
| expect.objectContaining({ | ||
|
|
@@ -531,6 +571,7 @@ describe('WriteFileTool', () => { | |
| abortSignal, | ||
| true, | ||
| true, // aggressiveUnescape | ||
| false, // isJsonLike | ||
| ); | ||
| expect(confirmation).toEqual( | ||
| expect.objectContaining({ | ||
|
|
@@ -737,6 +778,7 @@ describe('WriteFileTool', () => { | |
| abortSignal, | ||
| true, | ||
| true, // aggressiveUnescape | ||
| false, // isJsonLike | ||
| ); | ||
| expect(result.llmContent).toMatch( | ||
| /Successfully created and wrote to new file/, | ||
|
|
@@ -782,6 +824,7 @@ describe('WriteFileTool', () => { | |
| abortSignal, | ||
| true, | ||
| true, // aggressiveUnescape | ||
| false, // isJsonLike | ||
| ); | ||
| expect(result.llmContent).toMatch(/Successfully overwrote file/); | ||
| const writtenContent = await fsService.readTextFile(filePath); | ||
|
|
@@ -1052,6 +1095,7 @@ describe('WriteFileTool', () => { | |
| abortSignal, | ||
| true, | ||
| true, // aggressiveUnescape | ||
| false, // isJsonLike | ||
| ); | ||
| expect(result.correctedContent).toBe(proposedContent); | ||
| expect(result.fileExists).toBe(false); | ||
|
|
@@ -1080,6 +1124,7 @@ describe('WriteFileTool', () => { | |
| abortSignal, | ||
| true, | ||
| true, // aggressiveUnescape | ||
| false, // isJsonLike | ||
| ); | ||
| expect(result.correctedContent).toBe(proposedContent); | ||
| expect(result.originalContent).toBe(originalContent); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,14 +131,37 @@ export async function getCorrectedFileContent( | |
| } | ||
| } | ||
|
|
||
| const aggressiveUnescape = !isGemini3Model(config.getActiveModel()); | ||
| const fileExtension = path.extname(filePath).toLowerCase(); | ||
| const fileName = path.basename(filePath).toLowerCase(); | ||
|
|
||
| const isJsonLike = | ||
| fileExtension.includes('json') || | ||
| fileExtension === '.ipynb' || | ||
| fileExtension === '.map' || | ||
| fileExtension === '.yaml' || | ||
| fileExtension === '.yml' || | ||
| fileExtension === '.toml' || | ||
| fileExtension === '.lock' || | ||
| [ | ||
| '.eslintrc', | ||
| '.babelrc', | ||
| '.prettierrc', | ||
| '.stylelintrc', | ||
| '.watchmanconfig', | ||
| '.nycrc', | ||
| '.releaserc', | ||
| ].includes(fileName); | ||
|
amelidev marked this conversation as resolved.
amelidev marked this conversation as resolved.
Comment on lines
+137
to
+175
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To prevent silent corruption of other highly structured data and configuration formats, we should expand the const isJsonLike =
fileExtension.includes('json') ||
fileExtension === '.ipynb' ||
fileExtension === '.map' ||
fileExtension === '.yaml' ||
fileExtension === '.yml' ||
fileExtension === '.toml' ||
fileExtension === '.lock' ||
fileExtension === '.ini' ||
fileExtension === '.properties' ||
fileExtension === '.cfg' ||
fileExtension === '.conf' ||
fileExtension === '.config' ||
fileExtension === '.xml' ||
fileExtension === '.svg' ||
fileExtension === '.csv' ||
fileExtension === '.tsv' ||
fileExtension === '.tf' ||
fileExtension === '.tfvars' ||
fileExtension === '.hcl' ||
fileExtension === '.tfstate' ||
fileExtension === '.webmanifest' ||
fileName.startsWith('.env') ||
[
'.eslintrc',
'.babelrc',
'.prettierrc',
'.stylelintrc',
'.watchmanconfig',
'.nycrc',
'.releaserc',
'.gitconfig',
'.editorconfig',
'.npmrc',
'.yarnrc',
'.jshintrc',
'.markdownlintrc',
].includes(fileName); |
||
|
|
||
| const aggressiveUnescape = | ||
| !isJsonLike && !isGemini3Model(config.getActiveModel()); | ||
|
|
||
| correctedContent = await ensureCorrectFileContent( | ||
| proposedContent, | ||
| config.getBaseLlmClient(), | ||
| abortSignal, | ||
| config.getDisableLLMCorrection(), | ||
| aggressiveUnescape, | ||
| isJsonLike, | ||
| ); | ||
|
|
||
| return { originalContent, correctedContent, fileExists }; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.