MCP server: return images from read_file and read text without the plugin - #951
MCP server: return images from read_file and read text without the plugin#951chesterXalan wants to merge 2 commits into
Conversation
- add an optional RichResolver to MCPTool so tools can return non-text MCP content blocks; read_file now returns JPEG/PNG/GIF/WebP attachments as inline image content (5 MB cap, metadata-only reply above it) - when the plugin's extraction endpoint is unreachable (e.g. a standalone server pointed at a Mattermost without the plugin), read_file falls back to reading plain-text files directly through the user's client with the same rune-windowed paging - WritePost lists file attachments as metadata (name, type, size, File ID) so models can discover File IDs from read_post/read_channel and fetch content on demand via read_file
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR formats attached file metadata in posts and adds structured MCP file responses. The ChangesPost attachment formatting
MCP file reading
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant read_file
participant MattermostFileClient
participant FileContentService
MCPClient->>read_file: Request file content
read_file->>MattermostFileClient: Fetch file metadata
alt Supported image within 5 MB
read_file->>MattermostFileClient: Download image bytes
read_file-->>MCPClient: Return image content
else Text-like file
read_file->>FileContentService: Extract text
FileContentService-->>read_file: Return text or error
read_file-->>MCPClient: Return text or download fallback
else Unsupported or oversized file
read_file-->>MCPClient: Return error or file link
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
mcpserver/tools/files.go (3)
108-131: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider lowering the direct-read download bound.
maxStandaloneTextBytesallows a 10 MB download.files.Slicethen converts the body to a[]rune, which allocates up to 4 bytes per rune. A 10 MB text file therefore costs about 10 MB for the body plus up to 40 MB for the rune slice, per concurrent call, while the tool returns at mostMaxReadRunescharacters.The plugin extraction path has the same cost, so this is not a regression. Consider a smaller bound for the direct path, or slice by byte offset before the rune conversion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpserver/tools/files.go` around lines 108 - 131, Reduce the direct-read size limit used by readFileContentStandalone to avoid unnecessary memory allocation before files.Slice converts the entire body to runes; update maxStandaloneTextBytes to a smaller bound appropriate for the tool’s MaxReadRunes output while preserving the existing size validation and error behavior.
84-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared MIME normalization.
Both helpers repeat
strings.ToLower(strings.TrimSpace(strings.SplitN(mimeType, ";", 2)[0])). Extract a smallbaseMimeTypehelper. This keeps the two classifiers consistent if the normalization changes later.♻️ Proposed refactor
+// baseMimeType returns the lower-cased media type without parameters. +func baseMimeType(mimeType string) string { + return strings.ToLower(strings.TrimSpace(strings.SplitN(mimeType, ";", 2)[0])) +} + func isInlineImageMimeType(mimeType string) bool { - switch strings.ToLower(strings.TrimSpace(strings.SplitN(mimeType, ";", 2)[0])) { + switch baseMimeType(mimeType) { case "image/jpeg", "image/png", "image/gif", "image/webp": return true } return false } func isTextLikeMimeType(mimeType string) bool { - base := strings.ToLower(strings.TrimSpace(strings.SplitN(mimeType, ";", 2)[0])) + base := baseMimeType(mimeType) if strings.HasPrefix(base, "text/") {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpserver/tools/files.go` around lines 84 - 106, Extract the repeated MIME normalization into a shared baseMimeType helper, then update isInlineImageMimeType and isTextLikeMimeType to use it instead of performing their own strings.ToLower/TrimSpace/SplitN logic. Preserve both classifiers’ existing matching behavior.
176-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePreserve the sentinel error on the forbidden path.
Line 177 builds a new error with
fmt.Errorfand a constant string. This drops thefiles.ErrForbiddensentinel, so no caller can useerrors.Is. It also usesfmt.Errorfwithout a format verb, which some linters flag.Wrap the sentinel instead.
♻️ Proposed refactor
if errors.Is(svcErr, files.ErrForbidden) { - return nil, fmt.Errorf("you do not have permission to read this file") + return nil, fmt.Errorf("you do not have permission to read this file: %w", files.ErrForbidden) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpserver/tools/files.go` around lines 176 - 178, Update the forbidden-error branch in the surrounding file-read method to wrap and preserve files.ErrForbidden while retaining the user-facing permission message. Replace the constant-string fmt.Errorf usage with an error construction that supports errors.Is checks against the sentinel.mcpserver/tools/files_test.go (1)
100-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for media types that carry parameters.
Mattermost frequently stores media types with parameters, for example
text/plain; charset=utf-8. Every case in the table uses a bare media type. The parameter-stripping logic inisInlineImageMimeTypeandisTextLikeMimeTypeis therefore untested through the resolver.Add two cases: a text file with
text/plain; charset=utf-8, and an image withimage/PNGorimage/png; charset=binary. The image case also pins the media type that the resolver puts in theImageContentblock, which relates to the normalization issue raised onmcpserver/tools/files.goline 161.💚 Proposed test cases
{ name: "nil service falls back to direct read for text files", fileID: fileID, info: &model.FileInfo{Id: fileID, Name: "notes.txt", MimeType: "text/plain"}, fileData: []byte("plain text body"), service: nil, wantText: []string{"File: notes.txt (text/plain)", "plain text body"}, }, + { + name: "text media type with parameters falls back to direct read", + fileID: fileID, + info: &model.FileInfo{Id: fileID, Name: "notes.txt", MimeType: "text/plain; charset=utf-8"}, + fileData: []byte("plain text body"), + service: nil, + wantText: []string{"plain text body"}, + }, + { + name: "image media type with parameters is returned inline", + fileID: fileID, + info: &model.FileInfo{Id: fileID, Name: "photo.png", MimeType: "image/PNG", Size: 4}, + fileData: []byte{0x89, 0x50, 0x4E, 0x47}, + service: &fakeFileContentService{}, + wantImage: true, + },The image case will fail the
assert.Equal(t, tt.info.MimeType, image.MIMEType)assertion at line 207 until the resolver normalizes the media type. Adjust that assertion to the expected normalized value once the resolver change lands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpserver/tools/files_test.go` around lines 100 - 179, Extend the resolver table tests with a text file using a parameterized media type such as text/plain; charset=utf-8 and an image using image/PNG or image/png; charset=binary. Verify both are classified correctly, and update the image MIME type assertion to expect the resolver’s normalized media type in the ImageContent block rather than the original input.mcpserver/tools/provider.go (1)
314-333: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNormalize nil resolver contents before returning the MCP result.
CallToolResult.contentis required by MCP. A future rich resolver returningnil, nilwould omit the content array if the SDK treats nil slices as absent, so return[]for nilcontentsor a placeholder text block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpserver/tools/provider.go` around lines 314 - 333, Update the rich-resolver branch around mcpTool.RichResolver to normalize a nil contents slice after a successful resolve and before constructing mcp.CallToolResult. Return an empty content slice (or the established placeholder text block) when contents is nil, while preserving non-nil resolver results unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpserver/tools/files.go`:
- Around line 149-162: Use the normalized base media type when constructing the
mcp.ImageContent in the inline-image branch, reusing the existing baseMimeType
helper used by isInlineImageMimeType. Keep info.MimeType unchanged in the
descriptive TextContent if desired, and preserve the allowlist and download
behavior.
---
Nitpick comments:
In `@mcpserver/tools/files_test.go`:
- Around line 100-179: Extend the resolver table tests with a text file using a
parameterized media type such as text/plain; charset=utf-8 and an image using
image/PNG or image/png; charset=binary. Verify both are classified correctly,
and update the image MIME type assertion to expect the resolver’s normalized
media type in the ImageContent block rather than the original input.
In `@mcpserver/tools/files.go`:
- Around line 108-131: Reduce the direct-read size limit used by
readFileContentStandalone to avoid unnecessary memory allocation before
files.Slice converts the entire body to runes; update maxStandaloneTextBytes to
a smaller bound appropriate for the tool’s MaxReadRunes output while preserving
the existing size validation and error behavior.
- Around line 84-106: Extract the repeated MIME normalization into a shared
baseMimeType helper, then update isInlineImageMimeType and isTextLikeMimeType to
use it instead of performing their own strings.ToLower/TrimSpace/SplitN logic.
Preserve both classifiers’ existing matching behavior.
- Around line 176-178: Update the forbidden-error branch in the surrounding
file-read method to wrap and preserve files.ErrForbidden while retaining the
user-facing permission message. Replace the constant-string fmt.Errorf usage
with an error construction that supports errors.Is checks against the sentinel.
In `@mcpserver/tools/provider.go`:
- Around line 314-333: Update the rich-resolver branch around
mcpTool.RichResolver to normalize a nil contents slice after a successful
resolve and before constructing mcp.CallToolResult. Return an empty content
slice (or the established placeholder text block) when contents is nil, while
preserving non-nil resolver results unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: cf2be9e8-1cd9-49bb-8207-d02f84a45a78
📒 Files selected for processing (5)
format/format.goformat/format_test.gomcpserver/tools/files.gomcpserver/tools/files_test.gomcpserver/tools/provider.go
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
Still active and green — awaiting review. |
2 participants
Summary
Split from #888 to keep reviews small and focused. This part makes
read_fileuseful for images and for servers without the Agents plugin:RichResolvertoMCPToolso tools can return non-text MCP content blocks.read_filenow returns JPEG/PNG/GIF/WebP attachments as inline image content (5 MB cap; larger images get a metadata-only reply pointing atget_file_link).read_filefalls back to reading plain-text files directly through the user's client with the same rune-windowed paging.format.WritePostlists file attachments as metadata (name, type, size, File ID) so models can discover File IDs fromread_post/read_channeland fetch content on demand viaread_file.QA test steps:
read_post→ the attachment metadata includes its File ID. Callread_filewith that ID → an inline image content block plus a short text description is returned.read_fileon an image larger than 5 MB → a metadata-only text reply suggestingget_file_link.read_fileon a.txtattachment → raw text is returned with paging info.Ticket Link
None. Supersedes part of #888.
Release Note
Summary by CodeRabbit