MCP server: DM thread replies, safer attachments, image reads, and message priority - #888
Open
chesterXalan wants to merge 4 commits into
Open
MCP server: DM thread replies, safer attachments, image reads, and message priority#888chesterXalan wants to merge 4 commits into
chesterXalan wants to merge 4 commits into
Conversation
…t handling - dm/group_message accept root_id to reply in existing threads; reply targets resolve to the actual thread root so any post ID in the thread works (create_post too) - create_post no longer dead-ends on DM/GM channels: the display-name context check only applies to regular team channels (DM channels have no display name and no team, so validation could never pass) - absolute attachment paths can be allowed via MM_MCP_ATTACHMENT_ROOTS (path-list of directories, opened through os.Root so symlinks cannot escape); relative paths still resolve inside the data directory - attachment upload failures now abort the post instead of silently posting the message without its files
- 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
…sage
Adds priority ('important'/'urgent') and request_ack arguments. Priority
metadata is only valid on thread roots, so replies with priority are
rejected before posting; request_ack requires a licensed server and
surfaces the server's license error without creating the post.
|
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 Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe changes constrain absolute-path attachment reads, abort post creation on upload failures, add rich ChangesAttachment handling
Rich file reading
Post messaging metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant MCPToolProvider
participant PostTool
participant FileUtils
participant MattermostAPI
Caller->>MCPToolProvider: invoke post tool
MCPToolProvider->>PostTool: decode arguments
PostTool->>MattermostAPI: resolve thread root
PostTool->>FileUtils: upload attachments
FileUtils-->>PostTool: file IDs or error
PostTool->>MattermostAPI: create post with metadata
MattermostAPI-->>PostTool: created post
PostTool-->>MCPToolProvider: formatted result
MCPToolProvider-->>Caller: tool response
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 12
🤖 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 `@format/format.go`:
- Around line 172-188: Route all new attachment-facing text through the existing
i18n extraction mechanism: update the attachment heading and metadata templates
in format/format.go, the absolute-path rejection guidance at
mcpserver/tools/file_utils.go lines 147-147, and the upload failure, retry
guidance, and success status at lines 335-347. Preserve the current formatting
and behavior while replacing hardcoded user-facing strings with localized
messages.
- Around line 175-187: Update the attached-files formatting logic to track
whether at least one non-nil metadata entry was rendered; if none were rendered,
fall back to entry.Post.FileIds instead of leaving an empty section. Preserve
the existing metadata output for usable files and the bare-ID output for valid
FileIds.
In `@mcpserver/tools/file_utils.go`:
- Around line 339-344: Update the uploadFilesForLocal/CreatePost flow to clean
up any file IDs uploaded before a later attachment failure. Track the
successfully uploaded IDs and delete them when uploadFilesForLocal returns an
error, before returning the existing post-not-created error; preserve the
current error messaging and successful upload behavior.
In `@mcpserver/tools/files.go`:
- Around line 166-168: Change the ErrForbidden branch in the file resolver to
return nil with svcErr (or a wrapped error) so permission denial is reported as
a tool error rather than successful content. Update
mcpserver/tools/files_test.go lines 119-124 to assert the resolver returns an
error instead of permission-denial text content.
- Line 34: Route every new tool-facing string through the repository’s i18n
extraction mechanism: extract the read_file description in
mcpserver/tools/files.go:34, argument-decoding and rich-result errors in
mcpserver/tools/provider.go:67-75 and 314-324, standalone-read errors in
mcpserver/tools/files.go:108-122, file-info/image-error/limit/metadata output in
mcpserver/tools/files.go:125-154, and permission/fallback errors in
mcpserver/tools/files.go:161-176; preserve the existing messages and behavior
while replacing hardcoded strings with localized lookups.
- Line 122: Update the file-content return path around files.Slice to pass the
uploaded data as raw text without applying strings.TrimSpace, preserving leading
and trailing characters so paging offsets remain aligned with the original file.
- Around line 169-176: Update the fallback flow around the file content service
error and readFileContentStandalone: only invoke direct reading when the file’s
MIME type is text-like, and return the original extraction service error for
non-text documents instead of allowing them to be reported as having no
extractable text. Preserve the existing forbidden-service handling and
successful text fallback behavior.
- Around line 117-122: Update the standalone text download fallback around
GetFile to enforce a size limit before materializing the full attachment, or use
ranged/streamed reads so only the requested offset and limit are loaded.
Preserve the existing files.Slice result semantics while preventing unbounded
memory usage and avoid unnecessary full-buffer/string copies.
In `@mcpserver/tools/posts.go`:
- Around line 414-430: Reorder the post creation flow in the affected handlers,
including the paths using toolDM and toolGroupMessage: construct the model.Post
and call applyPostPriority before uploadFilesAndUrlsForLocal, then assign the
returned fileIDs to post.FileIds only after a successful upload. Preserve
existing error returns and add regression coverage confirming invalid
priority/acknowledgement combinations perform no upload.
- Around line 439-440: Update the success response in the post-creation flow to
format the created channel, team, and post using the existing format/ package
helpers before interpolation, rather than directly using channelDisplayName,
teamDisplayName, and createdPost.Id in fmt.Sprintf. Preserve the current
response structure and attachmentMessage handling.
- Around line 28-29: Replace the newly hardcoded user-facing strings in the post
tool schema fields, tool descriptions, validation errors, and DM/group labels
with the repository’s established i18n extraction mechanism. Update the affected
symbols around the schema definitions and the logic near the referenced
validation and label sections, preserving the existing messages and behavior
while ensuring every tool-facing string is sourced through i18n.
In `@mcpserver/tools/provider.go`:
- Around line 314-331: Instrument the RichResolver execution in the mcpTool
handler by starting a span from ctx with the repository’s standard telemetry
helper and attaching the standard tool attribute keys. Ensure the span is ended
on every return path, and record resolver errors on the span before returning
the existing error result; retain the current debug logging and response
behavior.
🪄 Autofix (Beta)
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
Run ID: 8b4f6526-c24e-4fd0-b064-46892e1a2daf
📒 Files selected for processing (9)
format/format.goformat/format_test.gomcpserver/tools/file_utils.gomcpserver/tools/file_utils_test.gomcpserver/tools/files.gomcpserver/tools/files_test.gomcpserver/tools/posts.gomcpserver/tools/posts_test.gomcpserver/tools/provider.go
- read_file: permission denials and extraction failures for non-text documents are tool errors instead of successful text; the standalone fallback no longer trims raw text (keeps paging offsets aligned) and rejects text files above 10 MB (mirrors files.Service) - post tools validate priority metadata before uploading attachments so validation failures cannot leave orphaned uploads - WritePost falls back to bare FileIds when metadata contains only nil entries
Collaborator
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A set of usability fixes for the standalone MCP server, found while running it as a stdio server against a Mattermost instance without the plugin installed. Happy to split this into smaller PRs if preferred.
Thread replies in DMs and group messages
dmandgroup_messagenow accept an optionalroot_idto reply in an existing thread. Previously there was no way to reply to a DM thread at all (see below).resolveThreadRoot, also applied tocreate_post): models frequently pass the ID of the latest post in a thread instead of the root, which the server would reject.create_postno longer dead-ends on DM/GM channelschannel_display_namerequiresminLength=1while the channel's display name is"", andGetTeam("")errors). The check now only applies to regular team channels; transparency validation for team channels is unchanged.Attachment handling
MM_MCP_ATTACHMENT_ROOTSenvironment variable (path-list of directories): absolute attachment paths are allowed when they fall under one of the configured roots, each opened viaos.Rootso symlinks cannot escape. Default behavior (data directory only) is unchanged when the variable is unset.read_fileimprovementsRichResolveronMCPTool; larger images return metadata plus a hint to useget_file_link.read_filefalls back to reading plain-text files directly through the user's client, with the same rune-windowed paging. Document extraction (PDF/Office) still requires the plugin.format.WritePostnow lists file attachments as metadata (name, type, size, File ID), so File IDs are discoverable fromread_post/read_channel/search output without an extraget_post_filescall.Message priority
create_post,dm, andgroup_messageacceptpriority(important/urgent) andrequest_ack. Both are rejected with a clear error on thread replies (Mattermost only supports priority on roots) before any post is created.request_ackon an unlicensed server surfaces the server's license error without creating the post.QA test steps
Run the standalone server over stdio against any Mattermost instance (
mattermost-mcp-server -s <url> -t <token>), then via an MCP client:dmyourself a message;dmagain withroot_idset to the returned post ID → reply lands in the thread (also works passing a non-root post ID from the thread).create_postinto a DM channel withchannel_display_name: "Direct Message",team_display_name: "N/A"→ posts successfully (previously always failed validation).MM_MCP_ATTACHMENT_ROOTS=/some/dirand attach/some/dir/file.png→ uploads; attach a path outside the roots → tool errors and no post is created.read_post→ attachment metadata with File ID;read_filethat ID → inline image content.read_filea.md/.txtattachment on a server without the plugin → text content.dmwithpriority: "important"→ post carries the priority label; retry withroot_idset → rejected without posting.Unit tests added for the new helpers and behaviors (
mcpserver/tools,format), table-driven per the repo conventions.Ticket Link
None — found while using the standalone MCP server. Happy to file issues for each item if that helps triage.
Release Note
Summary by CodeRabbit
New Features
priorityandrequest_acksupport for new messages, including threaded DM and group messages.read_fileto return richer output with inline image previews and ranged text, plus sensible fallbacks.Bug Fixes