-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: add coder_workspace_write_file MCP tool #19591
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
49ea415
to
15720af
Compare
2972507
to
a042936
Compare
15720af
to
911efb7
Compare
8c658eb
to
13e42f0
Compare
13e42f0
to
d231994
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.
Some minor nits and suggestions, nothing that should block a merge though.
"content": map[string]any{ | ||
"type": "string", | ||
"description": "The base64-encoded bytes to write to the file.", |
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.
Do we maybe want to limit the amount of file writing? Reads are 1MB, I think, so it might make sense to introduce a restriction here, too.
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 had that thought too but I am not sure how to make that restriction. The max is useful to make sure we are not going to run out of memory, but by the time we are here, anything in memory is already in memory and we might as well write it out since we have it.
I think instead of the tool handler, the restriction would need to be in the MCP library at the point where it reads in the data (and maybe it already has limits, I have not tested). Not sure if this is configurable though, or if we can hook in somewhere to enforce the restriction.
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.
Hmm maybe WithHooks
will let me do it. Ohhh wait there is a MaxLength
that I could pass through, which is probably what you were already thinking of 😅 Gonna implement this. I will need an offset as well, or an append boolean.
edit: er wait does not seem the server is enforcing the max length property...
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.
Ah, gotcha. In that case, I think we're good here.
53f1f3b
to
2503ffd
Compare
"content": map[string]any{ | ||
"type": "string", | ||
"description": "The base64-encoded bytes to write to the file.", |
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.
Ah, gotcha. In that case, I think we're good here.
A follow up to the read tool added in #19562