Edit tool confirmation uses vscode to show diff when cagent is run within vscode#1441
Edit tool confirmation uses vscode to show diff when cagent is run within vscode#1441simonferquel wants to merge 2 commits intodocker:mainfrom
Conversation
|
i think we should not render the diff at all in the terminal when showing in vscode, the background of that dialog seems broken |
|
I checked and have the same rendering from outside vscode. |
|
Even dumber: attached a debugger and found that the model is generating edits that... do not modify anything: |
|
Needs a rebase then i'll approve, just gave it a test
What i meant is also that BOTH the TUI and vscode are rendering the diff right now, which is harmless i guess :) 
Another nit: when using vscode insiders, cagent will detect normal vscode and open that in a separate window. Also not a blocker for this, just something to think about 
|
|
yes, vscode insider and cursor detection are not easy. I'll try to find a better way to handle that. |
|
also, I'd like to have a followup so that you can "always review file changes" in vscode even if you are not running within the editor |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
Found 3 issues in the new VSCode diff integration code:
🔴 High Severity (1)
- Sequential edit application will fail for multiple edits affecting nearby text
🟡 Medium Severity (2)
- Goroutine leak with no cancellation mechanism
- External command execution without context for cancellation
Please address the high severity issue before merging. The medium severity issues should also be fixed to enable graceful shutdown.
| } | ||
| modifiedContent := string(originalContent) | ||
| for _, edit := range args.Edits { | ||
| modifiedContent = strings.Replace(modifiedContent, edit.OldText, edit.NewText, 1) |
There was a problem hiding this comment.
🔴 HIGH: Incorrect edit application logic
Applying edits sequentially using strings.Replace(modifiedContent, edit.OldText, edit.NewText, 1) in a loop is incorrect. Each edit is applied to the result of the previous edit, which means:
- If edit Remove release/signing workflow #2's
OldTextis near edit Telemetry #1's changes, it may not be found in the modified content - Edits that overlap or affect nearby regions will produce incorrect results
- The order of edits will matter when it shouldn't
This will cause the VSCode diff viewer to show incorrect changes to the user.
Recommended fix: Apply edits based on their original positions in the file, not sequentially. Consider using a proper diff/patch algorithm or applying edits from end-to-beginning to avoid position shifts.
| // if already loaded, do nothing else, start the diff process | ||
| if !loaded { | ||
| go todo.(func())() | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM: Goroutine leak - no cancellation mechanism
The goroutine go todo.(func())() launches an external editor process with no way to cancel it. If the application exits while the editor is still open:
- The goroutine cannot be stopped
- Temp files may not be cleaned up if the process is killed
- The application cannot gracefully shut down while editors are open
Recommended fix: Pass a context that can be cancelled on application shutdown. Store running goroutines and cancel them during cleanup.
| return | ||
| } | ||
| // wd is for diff + wait for editor to close. This makes sure we delete the temp dir only after editing is done. | ||
| if err := exec.Command(externalTool, "-wd", path, tempFilePath).Run(); err != nil { |
There was a problem hiding this comment.
🟡 MEDIUM: No context for command execution
exec.Command(externalTool, "-wd", path, tempFilePath).Run() runs without a context for cancellation. If the editor hangs or the user wants to exit the application, there's no way to terminate this command.
Recommended fix: Use exec.CommandContext(ctx, externalTool, "-wd", path, tempFilePath).Run() with a cancellable context.
|
@simonferquel @krissetto what's the status on this one? |
|
not sure i think @simonferquel wanted to implement further support, but we should probably just get vscode in for now then add more support later |
4 participants
When
cagentis run from within a vs code terminal, use the vscode diff to review edit_tool changes.Diff.in.VS.Code.mp4