MCP server: fix DM thread replies, DM/GM create_post validation, and silent attachment failures - #950
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughThe posting tools now support threaded replies for direct, group, and regular channels. They validate reply targets and channel context. Local attachment failures now return errors before post creation. ChangesPost creation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PostTool
participant ThreadResolver
participant AttachmentUploader
participant Mattermost
PostTool->>ThreadResolver: Validate reply target and resolve thread root
ThreadResolver->>Mattermost: Fetch referenced post
Mattermost-->>ThreadResolver: Return post and root ID
ThreadResolver-->>PostTool: Return normalized root ID
PostTool->>AttachmentUploader: Upload local attachments
AttachmentUploader-->>PostTool: Return file IDs or error
PostTool->>Mattermost: Create post with root ID and file IDs
Mattermost-->>PostTool: Return created post
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: 1
🤖 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/file_utils.go`:
- Around line 294-300: Update the uploadFilesForLocal error path to delete all
fileIDs uploaded before the failure, using the existing file-deletion mechanism,
before returning either upload error. Add a two-attachment test that makes the
first upload succeed and the second fail, then verifies the first file ID is
deleted and the post creation is aborted.
🪄 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: 1372cad7-9b3c-41c5-872d-1e2606ad61e6
📒 Files selected for processing (4)
mcpserver/tools/file_utils.gomcpserver/tools/file_utils_test.gomcpserver/tools/posts.gomcpserver/tools/posts_test.go
…ment upload failures - 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) - attachment upload failures now abort the post instead of silently posting the message without its files; posts are validated before uploading so failures cannot leave orphaned uploads
12f7522 to
0cde2d6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/file_utils.go`:
- Around line 377-384: The resolvePostFiles function currently uploads
attachments before validating the combined file cap. Move the
checkCombinedFileCap call ahead of uploadFilesAndUrlsForLocal, using the
attachment specification count and len(files), while preserving the existing
error returns; add or update a test to verify over-cap requests send no upload
request.
🪄 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: 50218037-8d1e-49fc-bc06-b9766079a232
📒 Files selected for processing (4)
mcpserver/tools/file_utils.gomcpserver/tools/file_utils_test.gomcpserver/tools/posts.gomcpserver/tools/posts_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- mcpserver/tools/posts_test.go
- mcpserver/tools/posts.go
- mcpserver/tools/file_utils_test.go
|
@crspeller When you have a moment, could you take a look at this one? It rebases on top of your #932 and extends its file-resolution path with abort-on-failure semantics (details in the description), so your eyes on that interaction would be especially valuable. CodeRabbit has approved and CI is green. |
|
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 contains only the bug fixes for the MCP posting tools:
dmandgroup_messageacceptroot_idto reply in existing threads. Reply targets are resolved to the actual thread root, so any post ID in the thread works (create_posttoo). Models frequently pass the ID of the latest post in a thread, which the server rejects since Mattermost only supports one level of threading.create_postno longer dead-ends on DM/GM channels: the display-name context check now only applies to regular team channels. DM/GM channels have no display name and no team, so the check could never pass before.Note: rebased on top of #932 after it landed. The abort-on-failure semantics now also cover the unified file-resolution path introduced there:
access:"local"schema validation rejects them before the resolver runs);Two cases in
TestPostToolsCombinedAttachmentCapwere updated to pin the stricter behavior.QA test steps:
dmwith aroot_idpointing at the latest reply of an existing DM thread → the message lands in that thread instead of erroring.create_postwith a DM channel ID (channel_display_name: "Direct Message",team_display_name: "N/A") → the post is created instead of failing context validation.dmwith a nonexistent attachment path → the tool call errors and no post is created.Ticket Link
None. Supersedes part of #888.
Release Note
Summary by CodeRabbit