Skip to content

Commit 16aa876

Browse files
authored
fix(chat): Add more fsWrite validation and validate before diff view (aws#7054)
## Problem - Sometimes the fsWrite input is invalid causing the diff view to show 0 changes ## Solution - Add more fsWrite validation and validate before diff view --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent 0eea1d8 commit 16aa876

File tree

3 files changed

+32
-53
lines changed

3 files changed

+32
-53
lines changed

packages/core/src/codewhispererChat/controllers/chat/messenger/messenger.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,8 @@ export class Messenger {
330330
)
331331
explanation = input.explanation
332332
}
333+
// validate the fsWrite tool input before showing the diff view
334+
await tool.tool.validate()
333335
session.setShowDiffOnFileWrite(true)
334336
changeList = await tool.tool.getDiffChanges()
335337
}

packages/core/src/codewhispererChat/tools/fsWrite.ts

Lines changed: 30 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,7 @@ interface BaseParams {
1717

1818
export interface CreateParams extends BaseParams {
1919
command: 'create'
20-
fileText?: string
21-
newStr?: string
20+
fileText: string
2221
}
2322

2423
export interface StrReplaceParams extends BaseParams {
@@ -111,31 +110,50 @@ export class FsWrite {
111110
oldContent = await fs.readFileText(sanitizedPath)
112111
isNew = false
113112
} catch (err) {
113+
this.logger.warn('old content does not exist, set it to empty string')
114114
oldContent = ''
115115
isNew = true
116116
}
117117
return { filePath: sanitizedPath, content: oldContent, isNew }
118118
}
119119

120120
public async validate(): Promise<void> {
121+
if (!this.params.path) {
122+
throw new Error('Path must not be empty')
123+
}
124+
const sanitizedPath = sanitizePath(this.params.path)
121125
switch (this.params.command) {
122-
case 'create':
123-
if (!this.params.path) {
124-
throw new Error('Path must not be empty')
126+
case 'create': {
127+
if (this.params.fileText === undefined) {
128+
throw new Error('fileText must be provided for create command')
129+
}
130+
const fileExists = await fs.existsFile(sanitizedPath)
131+
if (fileExists) {
132+
const oldContent = await fs.readFileText(sanitizedPath)
133+
if (oldContent === this.params.fileText) {
134+
throw new Error('The file already exists with the same content')
135+
}
125136
}
126137
break
127-
case 'strReplace':
138+
}
139+
case 'strReplace': {
140+
if (this.params.oldStr === this.params.newStr) {
141+
throw new Error('The provided oldStr and newStr are the exact same, this is a no-op')
142+
}
143+
const fileExists = await fs.existsFile(sanitizedPath)
144+
if (!fileExists) {
145+
throw new Error('The provided path must exist in order to replace contents into it')
146+
}
147+
break
148+
}
128149
case 'insert': {
129-
const fileExists = await fs.existsFile(this.params.path)
150+
const fileExists = await fs.existsFile(sanitizedPath)
130151
if (!fileExists) {
131-
throw new Error('The provided path must exist in order to replace or insert contents into it')
152+
throw new Error('The provided path must exist in order to insert contents into it')
132153
}
133154
break
134155
}
135156
case 'append':
136-
if (!this.params.path) {
137-
throw new Error('Path must not be empty')
138-
}
139157
if (!this.params.newStr) {
140158
throw new Error('Content to append must not be empty')
141159
}
@@ -220,15 +238,7 @@ export class FsWrite {
220238
}
221239

222240
private getCreateCommandText(params: CreateParams): string {
223-
if (params.fileText) {
224-
return params.fileText
225-
}
226-
if (params.newStr) {
227-
this.logger.warn('Required field `fileText` is missing, use the provided `newStr` instead')
228-
return params.newStr
229-
}
230-
this.logger.warn('No content provided for the create command')
231-
return ''
241+
return params.fileText
232242
}
233243

234244
private escapeRegExp(string: string): string {

packages/core/src/test/codewhispererChat/tools/fsWrite.test.ts

Lines changed: 0 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -66,39 +66,6 @@ describe('FsWrite Tool', function () {
6666

6767
assert.deepStrictEqual(output, expectedOutput)
6868
})
69-
70-
it('uses newStr when fileText is not provided', async function () {
71-
const filePath = path.join(testFolder.path, 'file2.txt')
72-
73-
const params: CreateParams = {
74-
command: 'create',
75-
newStr: 'Hello World',
76-
path: filePath,
77-
}
78-
const fsWrite = new FsWrite(params)
79-
const output = await fsWrite.invoke(process.stdout)
80-
81-
const content = await fs.readFileText(filePath)
82-
assert.strictEqual(content, 'Hello World')
83-
84-
assert.deepStrictEqual(output, expectedOutput)
85-
})
86-
87-
it('creates an empty file when no content is provided', async function () {
88-
const filePath = path.join(testFolder.path, 'file3.txt')
89-
90-
const params: CreateParams = {
91-
command: 'create',
92-
path: filePath,
93-
}
94-
const fsWrite = new FsWrite(params)
95-
const output = await fsWrite.invoke(process.stdout)
96-
97-
const content = await fs.readFileText(filePath)
98-
assert.strictEqual(content, '')
99-
100-
assert.deepStrictEqual(output, expectedOutput)
101-
})
10269
})
10370

10471
describe('handleStrReplace', async function () {

0 commit comments

Comments
 (0)