Skip to content

Conversation

code-asher
Copy link
Member

A follow up to the read tool added in #19562

@code-asher code-asher force-pushed the asher/mcp-file-write branch 2 times, most recently from 49ea415 to 15720af Compare August 27, 2025 22:37
@code-asher code-asher force-pushed the asher/mcp-file-read branch 3 times, most recently from 2972507 to a042936 Compare August 28, 2025 19:53
@code-asher code-asher force-pushed the asher/mcp-file-write branch from 15720af to 911efb7 Compare August 28, 2025 20:26
@github-actions github-actions bot added the stale This issue is like stale bread. label Sep 6, 2025
@Kira-Pilot Kira-Pilot requested a review from ThomasK33 September 8, 2025 18:23
@github-actions github-actions bot removed the stale This issue is like stale bread. label Sep 9, 2025
Base automatically changed from asher/mcp-file-read to main September 9, 2025 23:12
@code-asher code-asher force-pushed the asher/mcp-file-write branch 3 times, most recently from 8c658eb to 13e42f0 Compare September 9, 2025 23:25
Copy link
Member

@ThomasK33 ThomasK33 left a 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.

Comment on lines +1466 to +1468
"content": map[string]any{
"type": "string",
"description": "The base64-encoded bytes to write to the file.",
Copy link
Member

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.

Copy link
Member Author

@code-asher code-asher Sep 11, 2025

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.

Copy link
Member Author

@code-asher code-asher Sep 11, 2025

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...

Copy link
Member

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.

Comment on lines +1466 to +1468
"content": map[string]any{
"type": "string",
"description": "The base64-encoded bytes to write to the file.",
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants