Skip to content

MCP server: fix DM thread replies, DM/GM create_post validation, and silent attachment failures #950

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.

Sign up for GitHub

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

Open
chesterXalan wants to merge 2 commits into
base: master
Choose a base branch
from
Open

MCP server: fix DM thread replies, DM/GM create_post validation, and silent attachment failures #950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0cde2d6
mcpserver: fix DM threading, DM/GM create_post validation, and attach…
chesterXalan Aug 5, 2026
927f36b
mcpserver: enforce the combined file cap before any upload
chesterXalan Aug 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 30 additions & 31 deletions mcpserver/tools/file_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,11 +336,9 @@ func uploadInlineFiles(ctx context.Context, client *model.Client4, channelID str
return fileIDs, nil
}

// checkCombinedFileCap returns a model-facing error when resolved legacy attachment
// IDs plus requested inline files would exceed the per-post attachment cap. Resolvers
// call it after resolving legacy attachments but before uploading any inline file, so
// a cap violation never orphans inline uploads (best-effort legacy attachment uploads
// may still be orphaned when the cap trips).
// checkCombinedFileCap returns a model-facing error when legacy attachments plus
// requested inline files would exceed the per-post attachment cap. Resolvers call
// it before any upload, so a cap violation cannot orphan uploads.
func checkCombinedFileCap(attachmentIDCount, inlineFileCount int) error {
if total := attachmentIDCount + inlineFileCount; total > maxFilesPerPost {
return fmt.Errorf("too many attachments: %d files and attachments combined but a post can have at most %d - reduce the number of inline files or attachments", total, maxFilesPerPost)
Expand Down Expand Up @@ -370,14 +368,18 @@ func inlineFilesMessage(count int) string {
}

// resolvePostFiles resolves a posting tool's legacy attachments and inline
// files into the post's file IDs plus a success-message suffix. Legacy
// attachments resolve first (best-effort) so the combined cap is enforced
// before any inline upload; an inline-file failure aborts because a post
// missing files the model explicitly asked to create is broken output.
// files into the post's file IDs plus a success-message suffix. The combined
// cap is enforced up front — each attachment resolves to exactly one file ID,
// so the total is known before any upload — and a failure on either path
// aborts because a post missing files the model explicitly asked for is
// broken output.
func resolvePostFiles(ctx context.Context, client *model.Client4, channelID string, attachments []string, files []InlineFile, accessMode AccessMode) ([]string, string, error) {
attachmentFileIDs, attachmentMessage := uploadFilesAndUrlsForLocal(ctx, client, channelID, attachments, accessMode)
if capErr := checkCombinedFileCap(len(attachments), len(files)); capErr != nil {
return nil, "", capErr
}

if err := checkCombinedFileCap(len(attachmentFileIDs), len(files)); err != nil {
attachmentFileIDs, attachmentMessage, err := uploadFilesAndUrlsForLocal(ctx, client, channelID, attachments, accessMode)
if err != nil {
return nil, "", err
}

Expand All @@ -394,29 +396,26 @@ func resolvePostFiles(ctx context.Context, client *model.Client4, channelID stri
return fileIDs, attachmentMessage + inlineFilesMessage(len(inlineFileIDs)), nil
}

// uploadFilesAndUrlsForLocal uploads files from URLs or file paths (local access only) and returns file IDs and status message
func uploadFilesAndUrlsForLocal(ctx context.Context, client *model.Client4, channelID string, attachments []string, accessMode AccessMode) ([]string, string) {
var fileIDs []string
var attachmentMessage string
// uploadFilesAndUrlsForLocal uploads files from URLs or file paths (local access only)
// and returns file IDs and a status message for the success response. When any
// attachment fails to upload it returns an error so callers can abort instead of
// posting a message without its attachments.
func uploadFilesAndUrlsForLocal(ctx context.Context, client *model.Client4, channelID string, attachments []string, accessMode AccessMode) ([]string, string, error) {
if len(attachments) == 0 {
return nil, "", nil
}

if len(attachments) > 0 {
if accessMode != AccessModeLocal {
attachmentMessage = " (file attachments not supported in remote access mode)"
return nil, attachmentMessage
}
if accessMode != AccessModeLocal {
return nil, "", fmt.Errorf("file attachments are not supported in remote access mode; the post was NOT created - retry without attachments if the message alone is acceptable")
}

uploadedFileIDs, uploadErr := uploadFilesForLocal(ctx, client, channelID, attachments, accessMode)
if uploadErr != nil {
if errors.Is(uploadErr, errMCPFileUploadFailed) {
attachmentMessage = " (file upload failed)"
} else {
attachmentMessage = fmt.Sprintf(" (file upload failed: %v)", uploadErr)
}
} else {
fileIDs = uploadedFileIDs
attachmentMessage = fmt.Sprintf(" (uploaded %d files)", len(fileIDs))
fileIDs, uploadErr := uploadFilesForLocal(ctx, client, channelID, attachments, accessMode)
if uploadErr != nil {
if errors.Is(uploadErr, errMCPFileUploadFailed) {
return nil, "", fmt.Errorf("file upload failed; the post was NOT created - fix the attachment or retry without it")
}
return nil, "", fmt.Errorf("file upload failed (%v); the post was NOT created - fix the attachment or retry without it", uploadErr)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

return fileIDs, attachmentMessage
return fileIDs, fmt.Sprintf(" (uploaded %d files)", len(fileIDs)), nil
}
42 changes: 42 additions & 0 deletions mcpserver/tools/file_utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,3 +208,45 @@ func TestUploadInlineFiles(t *testing.T) {
})
}
}

func TestUploadFilesAndUrlsForLocal_FailureAbortsPost(t *testing.T) {
testCases := []struct {
name string
attachments []string
accessMode AccessMode
wantErr bool
}{
{
name: "no attachments is a no-op",
attachments: nil,
accessMode: AccessModeLocal,
wantErr: false,
},
{
name: "attachments in remote mode return an error instead of posting silently",
attachments: []string{"anything.txt"},
accessMode: AccessModeRemote,
wantErr: true,
},
{
name: "unreadable attachment returns an error instead of posting silently",
attachments: []string{"does-not-exist.txt"},
accessMode: AccessModeLocal,
wantErr: true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
fileIDs, message, err := uploadFilesAndUrlsForLocal(t.Context(), nil, "channelid", tc.attachments, tc.accessMode)
if tc.wantErr {
require.Error(t, err)
require.Empty(t, fileIDs)
} else {
require.NoError(t, err)
require.Empty(t, fileIDs)
require.Empty(t, message)
}
})
}
}
129 changes: 101 additions & 28 deletions mcpserver/tools/posts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ type CreatePostAsUserArgs struct {
type DMArgs struct {
Username string `json:"username,omitempty" jsonschema:"Target username. If omitted the message is sent to yourself."`
Message string `json:"message" jsonschema:"The message content to send,minLength=1"`
RootID string `json:"root_id,omitempty" jsonschema:"Optional post ID to reply in an existing thread of this DM. Any post ID from the thread works; replies always land on the thread root.,maxLength=26"`
Attachments []string `json:"attachments,omitempty" access:"local" jsonschema:"Optional list of file paths or URLs to attach"`
Files []InlineFile `json:"files,omitempty" jsonschema:"Optional text files to create from inline content and attach to the post (max 10). The file name extension determines the file type."`
}
Expand All @@ -52,6 +53,7 @@ type DMArgs struct {
type GroupMessageArgs struct {
Usernames []string `json:"usernames" jsonschema:"Target usernames (must be at least 2)."`
Message string `json:"message" jsonschema:"The message content to send,minLength=1"`
RootID string `json:"root_id,omitempty" jsonschema:"Optional post ID to reply in an existing thread of this group conversation. Any post ID from the thread works; replies always land on the thread root.,maxLength=26"`
Attachments []string `json:"attachments,omitempty" access:"local" jsonschema:"Optional list of file paths or URLs to attach"`
Files []InlineFile `json:"files,omitempty" jsonschema:"Optional text files to create from inline content and attach to the post (max 10). The file name extension determines the file type."`
}
Expand All @@ -62,11 +64,11 @@ type GroupMessageArgs struct {
const (
readPostDescription = "Read a specific post and its thread from Mattermost. Parameters: post_id (required), include_thread (boolean, default true). Returns post content, author info, and optionally all replies in the thread. Example: {\"post_id\": \"8xqzn3pfmtbyfkr9hqbw4hheoa\", \"include_thread\": true}"

createPostDescriptionFmt = "Create a new post in Mattermost. IMPORTANT WORKFLOW: You MUST first call get_channel_info to obtain the channel_id, channel_display_name, and team_display_name. Present this context to the user before posting. Then call this tool with all required parameters. This ensures full transparency about where the message will be posted. Parameters: channel_id (required), message (required), root_id (optional - for replies), files (optional: create and attach text files inline by providing name + content; the extension sets the file type; max 10)%s. Returns created post details including ID and timestamp. Example: {\"channel_id\": \"h5wqm8kxptbztfgzpaxbsqozah\", \"message\": \"Hello team!\"}"
createPostDescriptionFmt = "Create a new post in Mattermost. IMPORTANT WORKFLOW: You MUST first call get_channel_info to obtain the channel_id, channel_display_name, and team_display_name. Present this context to the user before posting. Then call this tool with all required parameters. This ensures full transparency about where the message will be posted. For DM and group conversations (which have no display name or team) pass channel_display_name 'Direct Message' or 'Group Message' and team_display_name 'N/A'. Parameters: channel_id (required), message (required), root_id (optional - for replies; any post ID from the thread works), files (optional: create and attach text files inline by providing name + content; the extension sets the file type; max 10)%s. Returns created post details including ID and timestamp. Example: {\"channel_id\": \"h5wqm8kxptbztfgzpaxbsqozah\", \"message\": \"Hello team!\"}"

dmDescriptionFmt = "Send a direct message to a user. Provide username to specify the recipient. If username is omitted, the message is sent to yourself. This is the DEFAULT way to message people — call it multiple times to message multiple people individually. Only use the group_message tool when the user explicitly asks for a group chat. Parameters: message (required), username (optional), files (optional: create and attach text files inline by providing name + content; the extension sets the file type; max 10)%s. Returns confirmation with message ID. Example: {\"message\": \"Hello!\", \"username\": \"john\"}"
dmDescriptionFmt = "Send a direct message to a user. Provide username to specify the recipient. If username is omitted, the message is sent to yourself. This is the DEFAULT way to message people — call it multiple times to message multiple people individually. Only use the group_message tool when the user explicitly asks for a group chat. Parameters: message (required), username (optional), root_id (optional - to reply in an existing thread of the DM), files (optional: create and attach text files inline by providing name + content; the extension sets the file type; max 10)%s. Returns confirmation with message ID. Example: {\"message\": \"Hello!\", \"username\": \"john\"}"

groupMessageDescriptionFmt = "Send a message to a shared group conversation with 2 or more other users. All participants can see each other's messages. ONLY use this when the user explicitly asks for a group message, group chat, or group conversation. If the user just asks to 'message' or 'send to' multiple people, use the dm tool once per person instead. Parameters: message (required), usernames (at least 2 required), files (optional: create and attach text files inline by providing name + content; the extension sets the file type; max 10)%s. Returns confirmation with message ID. Example: {\"message\": \"Hey team!\", \"usernames\": [\"alice\", \"bob\"]}"
groupMessageDescriptionFmt = "Send a message to a shared group conversation with 2 or more other users. All participants can see each other's messages. ONLY use this when the user explicitly asks for a group message, group chat, or group conversation. If the user just asks to 'message' or 'send to' multiple people, use the dm tool once per person instead. Parameters: message (required), usernames (at least 2 required), root_id (optional - to reply in an existing thread), files (optional: create and attach text files inline by providing name + content; the extension sets the file type; max 10)%s. Returns confirmation with message ID. Example: {\"message\": \"Hey team!\", \"usernames\": [\"alice\", \"bob\"]}"
)

// getPostTools returns all post-related tools
Expand Down Expand Up @@ -288,6 +290,32 @@ func (p *MattermostToolProvider) toolReadPost(mcpContext *MCPToolContext, args R
return result.String(), nil
}

// resolveThreadRoot validates a reply target and returns the ID of the actual
// thread root. Mattermost only supports one level of threading, so when rootID
// points at a reply the reply's own root is returned; models frequently pass
// the ID of the latest post in a thread rather than the root. An empty rootID
// resolves to "".
func resolveThreadRoot(mcpContext *MCPToolContext, channelID, rootID string) (string, error) {
if rootID == "" {
return "", nil
}
if err := requireID("root_id", rootID); err != nil {
return "", err
}

rootPost, _, err := mcpContext.Client.GetPost(mcpContext.Ctx, rootID, "")
if err != nil {
return "", fmt.Errorf("error fetching root post %s: %w", rootID, err)
}
if rootPost.ChannelId != channelID {
return "", fmt.Errorf("root_id %s belongs to channel %s, not the target conversation %s - use a post ID from the same conversation", rootID, rootPost.ChannelId, channelID)
}
if rootPost.RootId != "" {
return rootPost.RootId, nil
}
return rootPost.Id, nil
}

// toolCreatePost implements the create_post tool
func (p *MattermostToolProvider) toolCreatePost(mcpContext *MCPToolContext, args CreatePostArgs) (string, error) {
// Validate required fields
Expand Down Expand Up @@ -318,37 +346,61 @@ func (p *MattermostToolProvider) toolCreatePost(mcpContext *MCPToolContext, args
return "", fmt.Errorf("error fetching channel for validation: %w", err)
}

// Check if channel display name matches
if channel.DisplayName != args.ChannelDisplayName {
return "", fmt.Errorf("channel_display_name mismatch: provided '%s' but channel ID '%s' has display name '%s'",
args.ChannelDisplayName, args.ChannelID, channel.DisplayName)
}
// DM and group channels have no display name and no team, so the strict
// display-name context check only applies to regular team channels.
channelDisplayName := channel.DisplayName
var teamDisplayName string
switch channel.Type {
case model.ChannelTypeDirect, model.ChannelTypeGroup:
if channelDisplayName == "" {
channelDisplayName = "Direct Message"
if channel.Type == model.ChannelTypeGroup {
channelDisplayName = "Group Message"
}
}
teamDisplayName = "N/A"
default:
// Check if channel display name matches
if channel.DisplayName != args.ChannelDisplayName {
return "", fmt.Errorf("channel_display_name mismatch: provided '%s' but channel ID '%s' has display name '%s'",
args.ChannelDisplayName, args.ChannelID, channel.DisplayName)
}

// Get team info to validate team display name
team, _, err := client.GetTeam(ctx, channel.TeamId, "")
if err != nil {
return "", fmt.Errorf("error fetching team for validation: %w", err)
}
// Get team info to validate team display name
team, _, teamErr := client.GetTeam(ctx, channel.TeamId, "")
if teamErr != nil {
return "", fmt.Errorf("error fetching team for validation: %w", teamErr)
}

// Check if team display name matches
if team.DisplayName != args.TeamDisplayName {
return "", fmt.Errorf("team_display_name mismatch: provided '%s' but team ID '%s' has display name '%s'",
args.TeamDisplayName, channel.TeamId, team.DisplayName)
// Check if team display name matches
if team.DisplayName != args.TeamDisplayName {
return "", fmt.Errorf("team_display_name mismatch: provided '%s' but team ID '%s' has display name '%s'",
args.TeamDisplayName, channel.TeamId, team.DisplayName)
}
teamDisplayName = team.DisplayName
}

fileIDs, filesMessage, err := resolvePostFiles(ctx, client, args.ChannelID, args.Attachments, args.Files, mcpContext.AccessMode)
// Resolve the reply target to the actual thread root
rootID, err := resolveThreadRoot(mcpContext, args.ChannelID, args.RootID)
if err != nil {
return "", err
}

// Create the post
// Build and validate the post before uploading attachments so validation
// failures do not leave orphaned uploads on the server.
post := &model.Post{
ChannelId: args.ChannelID,
Message: args.Message,
RootId: args.RootID,
FileIds: fileIDs,
RootId: rootID,
}

// Upload files if specified
fileIDs, filesMessage, err := resolvePostFiles(ctx, client, args.ChannelID, args.Attachments, args.Files, mcpContext.AccessMode)
if err != nil {
return "", err
}
post.FileIds = fileIDs

p.stampAIGenerated(post, mcpContext, "")

createdPost, _, err := client.CreatePost(ctx, post)
Expand All @@ -357,7 +409,7 @@ func (p *MattermostToolProvider) toolCreatePost(mcpContext *MCPToolContext, args
}

return fmt.Sprintf("Successfully created post in channel '%s' (Team: %s) with ID: %s%s",
channel.DisplayName, team.DisplayName, createdPost.Id, filesMessage), nil
channelDisplayName, teamDisplayName, createdPost.Id, filesMessage), nil
}

// toolCreatePostAsUser implements the create_post_as_user tool with custom authentication
Expand Down Expand Up @@ -391,7 +443,10 @@ func (p *MattermostToolProvider) toolCreatePostAsUser(mcpContext *MCPToolContext
}

// Upload files if specified
fileIDs, attachmentMessage := uploadFilesAndUrlsForLocal(ctx, userClient, args.ChannelID, args.Attachments, mcpContext.AccessMode)
fileIDs, attachmentMessage, err := uploadFilesAndUrlsForLocal(ctx, userClient, args.ChannelID, args.Attachments, mcpContext.AccessMode)
if err != nil {
return "", err
}

// Create the post
post := &model.Post{
Expand Down Expand Up @@ -454,18 +509,27 @@ func (p *MattermostToolProvider) toolDM(mcpContext *MCPToolContext, args DMArgs)
return "", fmt.Errorf("error creating direct channel: %w", err)
}

fileIDs, filesMessage, err := resolvePostFiles(ctx, client, dmChannel.Id, args.Attachments, args.Files, mcpContext.AccessMode)
// Resolve the reply target to the actual thread root
rootID, err := resolveThreadRoot(mcpContext, dmChannel.Id, args.RootID)
if err != nil {
return "", err
}

// Create the post in the DM channel
// Build and validate the post before uploading attachments so validation
// failures do not leave orphaned uploads on the server.
post := &model.Post{
ChannelId: dmChannel.Id,
Message: args.Message,
FileIds: fileIDs,
RootId: rootID,
}

// Upload files if specified
fileIDs, filesMessage, err := resolvePostFiles(ctx, client, dmChannel.Id, args.Attachments, args.Files, mcpContext.AccessMode)
if err != nil {
return "", err
}
post.FileIds = fileIDs

// Set from_webhook only when DM'ing yourself (prevents AI auto-response loop)
if dmSelf {
post.AddProp("from_webhook", "true")
Expand Down Expand Up @@ -528,16 +592,25 @@ func (p *MattermostToolProvider) toolGroupMessage(mcpContext *MCPToolContext, ar
return "", fmt.Errorf("error creating group channel: %w", err)
}

fileIDs, filesMessage, err := resolvePostFiles(ctx, client, gmChannel.Id, args.Attachments, args.Files, mcpContext.AccessMode)
// Resolve the reply target to the actual thread root
rootID, err := resolveThreadRoot(mcpContext, gmChannel.Id, args.RootID)
if err != nil {
return "", err
}

// Build and validate the post before uploading attachments so validation
// failures do not leave orphaned uploads on the server.
post := &model.Post{
ChannelId: gmChannel.Id,
Message: args.Message,
FileIds: fileIDs,
RootId: rootID,
}

fileIDs, filesMessage, err := resolvePostFiles(ctx, client, gmChannel.Id, args.Attachments, args.Files, mcpContext.AccessMode)
if err != nil {
return "", err
}
post.FileIds = fileIDs

p.stampAIGenerated(post, mcpContext, currentUser.Id)

Expand Down
Loading