on main help sign in
handle errors correctly in the edit tool
commit921f5ee5bd74837ff4566fc2d1e45051c87d9c38 authorKujtim Hoxha <kujtimii.h@gmail.com> dateMon 14 Apr 2025, 11:08 (2 years ago) parent9ae05fe
4 files changed+78−82
internal/llm/tools/diagnostics.go+2−2 internal/llm/tools/edit.go+74−78 internal/llm/tools/view.go+1−1 internal/llm/tools/write.go+1−1
internal/llm/tools/diagnostics.go+2−2
@@ -82,7 +82,7 @@ func (b *diagnosticsTool) Run(ctx context.Context, call ToolCall) (ToolResponse,
8282 waitForLspDiagnostics(ctx, params.FilePath, lsps)
8383 }
8484
85− output := appendDiagnostics(params.FilePath, lsps)
85+ output := getDiagnostics(params.FilePath, lsps)
8686
8787 return NewTextResponse(output), nil
8888}
@@ -154,7 +154,7 @@ func hasDiagnosticsChanged(current, original map[protocol.DocumentUri][]protocol
154154 return false
155155}
156156
157−func appendDiagnostics(filePath string, lsps map[string]*lsp.Client) string {
157+func getDiagnostics(filePath string, lsps map[string]*lsp.Client) string {
158158 fileDiagnostics := []string{}
159159 projectDiagnostics := []string{}
160160
internal/llm/tools/edit.go+74−78
@@ -131,68 +131,54 @@ func (e *editTool) Run(ctx context.Context, call ToolCall) (ToolResponse, error)
131131 params.FilePath = filepath.Join(wd, params.FilePath)
132132 }
133133
134+ var response ToolResponse
135+ var err error
136+
134137 if params.OldString == "" {
135− result, err := e.createNewFile(ctx, params.FilePath, params.NewString)
138+ response, err = e.createNewFile(ctx, params.FilePath, params.NewString)
136139 if err != nil {
137− return NewTextErrorResponse(fmt.Sprintf("error creating file: %s", err)), nil
140+ return response, nil
138141 }
139− return WithResponseMetadata(NewTextResponse(result.text), EditResponseMetadata{
140− Additions: result.additions,
141− Removals: result.removals,
142− }), nil
143142 }
144143
145144 if params.NewString == "" {
146− result, err := e.deleteContent(ctx, params.FilePath, params.OldString)
145+ response, err = e.deleteContent(ctx, params.FilePath, params.OldString)
147146 if err != nil {
148− return NewTextErrorResponse(fmt.Sprintf("error deleting content: %s", err)), nil
147+ return response, nil
149148 }
150− return WithResponseMetadata(NewTextResponse(result.text), EditResponseMetadata{
151− Additions: result.additions,
152− Removals: result.removals,
153− }), nil
154149 }
155150
156− result, err := e.replaceContent(ctx, params.FilePath, params.OldString, params.NewString)
151+ response, err = e.replaceContent(ctx, params.FilePath, params.OldString, params.NewString)
157152 if err != nil {
158− return NewTextErrorResponse(fmt.Sprintf("error replacing content: %s", err)), nil
153+ return response, nil
159154 }
160155
161156 waitForLspDiagnostics(ctx, params.FilePath, e.lspClients)
162− text := fmt.Sprintf("<result>\n%s\n</result>\n", result.text)
163− text += appendDiagnostics(params.FilePath, e.lspClients)
164− return WithResponseMetadata(NewTextResponse(text), EditResponseMetadata{
165− Additions: result.additions,
166− Removals: result.removals,
167− }), nil
168−}
169−
170−type editResponse struct {
171− text string
172− additions int
173− removals int
157+ text := fmt.Sprintf("<result>\n%s\n</result>\n", response.Content)
158+ text += getDiagnostics(params.FilePath, e.lspClients)
159+ response.Content = text
160+ return response, nil
174161}
175162
176−func (e *editTool) createNewFile(ctx context.Context, filePath, content string) (editResponse, error) {
177− er := editResponse{}
163+func (e *editTool) createNewFile(ctx context.Context, filePath, content string) (ToolResponse, error) {
178164 fileInfo, err := os.Stat(filePath)
179165 if err == nil {
180166 if fileInfo.IsDir() {
181− return er, fmt.Errorf("path is a directory, not a file: %s", filePath)
167+ return NewTextErrorResponse(fmt.Sprintf("path is a directory, not a file: %s", filePath)), nil
182168 }
183− return er, fmt.Errorf("file already exists: %s. Use the Replace tool to overwrite an existing file", filePath)
169+ return NewTextErrorResponse(fmt.Sprintf("file already exists: %s", filePath)), nil
184170 } else if !os.IsNotExist(err) {
185− return er, fmt.Errorf("failed to access file: %w", err)
171+ return ToolResponse{}, fmt.Errorf("failed to access file: %w", err)
186172 }
187173
188174 dir := filepath.Dir(filePath)
189175 if err = os.MkdirAll(dir, 0o755); err != nil {
190− return er, fmt.Errorf("failed to create parent directories: %w", err)
176+ return ToolResponse{}, fmt.Errorf("failed to create parent directories: %w", err)
191177 }
192178
193179 sessionID, messageID := GetContextValues(ctx)
194180 if sessionID == "" || messageID == "" {
195− return er, fmt.Errorf("session ID and message ID are required for creating a new file")
181+ return ToolResponse{}, fmt.Errorf("session ID and message ID are required for creating a new file")
196182 }
197183
198184 diff, stats, err := git.GenerateGitDiffWithStats(
@@ -201,7 +187,7 @@ func (e *editTool) createNewFile(ctx context.Context, filePath, content string)
201187 content,
202188 )
203189 if err != nil {
204− return er, fmt.Errorf("failed to get file diff: %w", err)
190+ return ToolResponse{}, fmt.Errorf("failed to get file diff: %w", err)
205191 }
206192 p := e.permissions.Request(
207193 permission.CreatePermissionRequest{
@@ -216,63 +202,67 @@ func (e *editTool) createNewFile(ctx context.Context, filePath, content string)
216202 },
217203 )
218204 if !p {
219− return er, fmt.Errorf("permission denied")
205+ return ToolResponse{}, permission.ErrorPermissionDenied
220206 }
221207
222208 err = os.WriteFile(filePath, []byte(content), 0o644)
223209 if err != nil {
224− return er, fmt.Errorf("failed to write file: %w", err)
210+ return ToolResponse{}, fmt.Errorf("failed to write file: %w", err)
225211 }
226212
227213 recordFileWrite(filePath)
228214 recordFileRead(filePath)
229215
230− er.text = "File created: " + filePath
231− er.additions = stats.Additions
232− er.removals = stats.Removals
233− return er, nil
216+ return WithResponseMetadata(
217+ NewTextResponse("File created: "+filePath),
218+ EditResponseMetadata{
219+ Additions: stats.Additions,
220+ Removals: stats.Removals,
221+ },
222+ ), nil
234223}
235224
236−func (e *editTool) deleteContent(ctx context.Context, filePath, oldString string) (editResponse, error) {
237− er := editResponse{}
225+func (e *editTool) deleteContent(ctx context.Context, filePath, oldString string) (ToolResponse, error) {
238226 fileInfo, err := os.Stat(filePath)
239227 if err != nil {
240228 if os.IsNotExist(err) {
241− return er, fmt.Errorf("file not found: %s", filePath)
229+ return NewTextErrorResponse(fmt.Sprintf("file not found: %s", filePath)), nil
242230 }
243− return er, fmt.Errorf("failed to access file: %w", err)
231+ return ToolResponse{}, fmt.Errorf("failed to access file: %w", err)
244232 }
245233
246234 if fileInfo.IsDir() {
247− return er, fmt.Errorf("path is a directory, not a file: %s", filePath)
235+ return NewTextErrorResponse(fmt.Sprintf("path is a directory, not a file: %s", filePath)), nil
248236 }
249237
250238 if getLastReadTime(filePath).IsZero() {
251− return er, fmt.Errorf("you must read the file before editing it. Use the View tool first")
239+ return NewTextErrorResponse("you must read the file before editing it. Use the View tool first"), nil
252240 }
253241
254242 modTime := fileInfo.ModTime()
255243 lastRead := getLastReadTime(filePath)
256244 if modTime.After(lastRead) {
257− return er, fmt.Errorf("file %s has been modified since it was last read (mod time: %s, last read: %s)",
258− filePath, modTime.Format(time.RFC3339), lastRead.Format(time.RFC3339))
245+ return NewTextErrorResponse(
246+ fmt.Sprintf("file %s has been modified since it was last read (mod time: %s, last read: %s)",
247+ filePath, modTime.Format(time.RFC3339), lastRead.Format(time.RFC3339),
248+ )), nil
259249 }
260250
261251 content, err := os.ReadFile(filePath)
262252 if err != nil {
263− return er, fmt.Errorf("failed to read file: %w", err)
253+ return ToolResponse{}, fmt.Errorf("failed to read file: %w", err)
264254 }
265255
266256 oldContent := string(content)
267257
268258 index := strings.Index(oldContent, oldString)
269259 if index == -1 {
270− return er, fmt.Errorf("old_string not found in file. Make sure it matches exactly, including whitespace and line breaks")
260+ return NewTextErrorResponse("old_string not found in file. Make sure it matches exactly, including whitespace and line breaks"), nil
271261 }
272262
273263 lastIndex := strings.LastIndex(oldContent, oldString)
274264 if index != lastIndex {
275− return er, fmt.Errorf("old_string appears multiple times in the file. Please provide more context to ensure a unique match")
265+ return NewTextErrorResponse("old_string appears multiple times in the file. Please provide more context to ensure a unique match"), nil
276266 }
277267
278268 newContent := oldContent[:index] + oldContent[index+len(oldString):]
@@ -280,7 +270,7 @@ func (e *editTool) deleteContent(ctx context.Context, filePath, oldString string
280270 sessionID, messageID := GetContextValues(ctx)
281271
282272 if sessionID == "" || messageID == "" {
283− return er, fmt.Errorf("session ID and message ID are required for creating a new file")
273+ return ToolResponse{}, fmt.Errorf("session ID and message ID are required for creating a new file")
284274 }
285275
286276 diff, stats, err := git.GenerateGitDiffWithStats(
@@ -289,7 +279,7 @@ func (e *editTool) deleteContent(ctx context.Context, filePath, oldString string
289279 newContent,
290280 )
291281 if err != nil {
292− return er, fmt.Errorf("failed to get file diff: %w", err)
282+ return ToolResponse{}, fmt.Errorf("failed to get file diff: %w", err)
293283 }
294284
295285 p := e.permissions.Request(
@@ -305,62 +295,66 @@ func (e *editTool) deleteContent(ctx context.Context, filePath, oldString string
305295 },
306296 )
307297 if !p {
308− return er, fmt.Errorf("permission denied")
298+ return ToolResponse{}, permission.ErrorPermissionDenied
309299 }
310300
311301 err = os.WriteFile(filePath, []byte(newContent), 0o644)
312302 if err != nil {
313− return er, fmt.Errorf("failed to write file: %w", err)
303+ return ToolResponse{}, fmt.Errorf("failed to write file: %w", err)
314304 }
315305 recordFileWrite(filePath)
316306 recordFileRead(filePath)
317307
318− er.text = "Content deleted from file: " + filePath
319− er.additions = stats.Additions
320− er.removals = stats.Removals
321− return er, nil
308+ return WithResponseMetadata(
309+ NewTextResponse("Content deleted from file: "+filePath),
310+ EditResponseMetadata{
311+ Additions: stats.Additions,
312+ Removals: stats.Removals,
313+ },
314+ ), nil
322315}
323316
324−func (e *editTool) replaceContent(ctx context.Context, filePath, oldString, newString string) (editResponse, error) {
325− er := editResponse{}
317+func (e *editTool) replaceContent(ctx context.Context, filePath, oldString, newString string) (ToolResponse, error) {
326318 fileInfo, err := os.Stat(filePath)
327319 if err != nil {
328320 if os.IsNotExist(err) {
329− return er, fmt.Errorf("file not found: %s", filePath)
321+ return NewTextErrorResponse(fmt.Sprintf("file not found: %s", filePath)), nil
330322 }
331− return er, fmt.Errorf("failed to access file: %w", err)
323+ return ToolResponse{}, fmt.Errorf("failed to access file: %w", err)
332324 }
333325
334326 if fileInfo.IsDir() {
335− return er, fmt.Errorf("path is a directory, not a file: %s", filePath)
327+ return NewTextErrorResponse(fmt.Sprintf("path is a directory, not a file: %s", filePath)), nil
336328 }
337329
338330 if getLastReadTime(filePath).IsZero() {
339− return er, fmt.Errorf("you must read the file before editing it. Use the View tool first")
331+ return NewTextErrorResponse("you must read the file before editing it. Use the View tool first"), nil
340332 }
341333
342334 modTime := fileInfo.ModTime()
343335 lastRead := getLastReadTime(filePath)
344336 if modTime.After(lastRead) {
345− return er, fmt.Errorf("file %s has been modified since it was last read (mod time: %s, last read: %s)",
346− filePath, modTime.Format(time.RFC3339), lastRead.Format(time.RFC3339))
337+ return NewTextErrorResponse(
338+ fmt.Sprintf("file %s has been modified since it was last read (mod time: %s, last read: %s)",
339+ filePath, modTime.Format(time.RFC3339), lastRead.Format(time.RFC3339),
340+ )), nil
347341 }
348342
349343 content, err := os.ReadFile(filePath)
350344 if err != nil {
351− return er, fmt.Errorf("failed to read file: %w", err)
345+ return ToolResponse{}, fmt.Errorf("failed to read file: %w", err)
352346 }
353347
354348 oldContent := string(content)
355349
356350 index := strings.Index(oldContent, oldString)
357351 if index == -1 {
358− return er, fmt.Errorf("old_string not found in file. Make sure it matches exactly, including whitespace and line breaks")
352+ return NewTextErrorResponse("old_string not found in file. Make sure it matches exactly, including whitespace and line breaks"), nil
359353 }
360354
361355 lastIndex := strings.LastIndex(oldContent, oldString)
362356 if index != lastIndex {
363− return er, fmt.Errorf("old_string appears multiple times in the file. Please provide more context to ensure a unique match")
357+ return NewTextErrorResponse("old_string appears multiple times in the file. Please provide more context to ensure a unique match"), nil
364358 }
365359
366360 newContent := oldContent[:index] + newString + oldContent[index+len(oldString):]
@@ -368,7 +362,7 @@ func (e *editTool) replaceContent(ctx context.Context, filePath, oldString, newS
368362 sessionID, messageID := GetContextValues(ctx)
369363
370364 if sessionID == "" || messageID == "" {
371− return er, fmt.Errorf("session ID and message ID are required for creating a new file")
365+ return ToolResponse{}, fmt.Errorf("session ID and message ID are required for creating a new file")
372366 }
373367 diff, stats, err := git.GenerateGitDiffWithStats(
374368 removeWorkingDirectoryPrefix(filePath),
@@ -376,7 +370,7 @@ func (e *editTool) replaceContent(ctx context.Context, filePath, oldString, newS
376370 newContent,
377371 )
378372 if err != nil {
379− return er, fmt.Errorf("failed to get file diff: %w", err)
373+ return ToolResponse{}, fmt.Errorf("failed to get file diff: %w", err)
380374 }
381375
382376 p := e.permissions.Request(
@@ -393,19 +387,21 @@ func (e *editTool) replaceContent(ctx context.Context, filePath, oldString, newS
393387 },
394388 )
395389 if !p {
396− return er, fmt.Errorf("permission denied")
390+ return ToolResponse{}, permission.ErrorPermissionDenied
397391 }
398392
399393 err = os.WriteFile(filePath, []byte(newContent), 0o644)
400394 if err != nil {
401− return er, fmt.Errorf("failed to write file: %w", err)
395+ return ToolResponse{}, fmt.Errorf("failed to write file: %w", err)
402396 }
403397
404398 recordFileWrite(filePath)
405399 recordFileRead(filePath)
406− er.text = "Content replaced in file: " + filePath
407− er.additions = stats.Additions
408− er.removals = stats.Removals
409400
410− return er, nil
401+ return WithResponseMetadata(
402+ NewTextResponse("Content replaced in file: "+filePath),
403+ EditResponseMetadata{
404+ Additions: stats.Additions,
405+ Removals: stats.Removals,
406+ }), nil
411407}
internal/llm/tools/view.go+1−1
@@ -177,7 +177,7 @@ func (v *viewTool) Run(ctx context.Context, call ToolCall) (ToolResponse, error)
177177 params.Offset+len(strings.Split(content, "\n")))
178178 }
179179 output += "\n</file>\n"
180− output += appendDiagnostics(filePath, v.lspClients)
180+ output += getDiagnostics(filePath, v.lspClients)
181181 recordFileRead(filePath)
182182 return NewTextResponse(output), nil
183183}
internal/llm/tools/write.go+1−1
@@ -183,7 +183,7 @@ func (w *writeTool) Run(ctx context.Context, call ToolCall) (ToolResponse, error
183183
184184 result := fmt.Sprintf("File successfully written: %s", filePath)
185185 result = fmt.Sprintf("<result>\n%s\n</result>", result)
186− result += appendDiagnostics(filePath, w.lspClients)
186+ result += getDiagnostics(filePath, w.lspClients)
187187 return WithResponseMetadata(NewTextResponse(result),
188188 WriteResponseMetadata{
189189 Additions: stats.Additions,