-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: add coder_workspace_edit_file MCP tool #19629
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
base: main
Are you sure you want to change the base?
Conversation
f2ba812
to
5b5b443
Compare
91c875b
to
9435706
Compare
8c658eb
to
13e42f0
Compare
13e42f0
to
d231994
Compare
9435706
to
d82a0ff
Compare
d82a0ff
to
1f7ae1f
Compare
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.
I prefer this PR to be implemented as a multi-file edit (internally), and then have two MCP tools: one for a single file edit, and the other for bulk/multi-file edit operations.
type FileEdit struct { | ||
Search string `json:"search"` | ||
Replace string `json:"replace"` | ||
} |
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 include a file path here as well?
This way, one could bundle multiple file writes or perform bulk operations into a single API call.
We can still have a tool that edits a single file, but internally, it could bundle the path and call it the multi-file edit one.
if len(edits) == 0 { | ||
return http.StatusBadRequest, xerrors.New("must specify at least one edit") | ||
} |
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.
Shouldn't this check be moved to the top?
if err != nil { | ||
return http.StatusInternalServerError, xerrors.Errorf("edit %s: %w", path, err) |
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.
Shouldn't we clean up the tmpfile if the copy failed?
A continuation from #19591