Add LLM file creation: CreateFile response attachments and inline files on posting tools - #932
Add LLM file creation: CreateFile response attachments and inline files on posting tools#932crspeller wants to merge 10 commits into
Conversation
…ge tools Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…te, created-file registry, FileInfo link helpers) Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…se posts Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…unt limits, doc/comment accuracy Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…hment merge Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
🤖 LLM Evaluation ResultsOpenAI
❌ Failed EvaluationsShow 7 failuresOPENAI1. TestReactEval/[openai]_react_cat_message
2. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[openai]_bot_dm_tool_introspection
Anthropic
❌ Failed EvaluationsShow 7 failuresANTHROPIC1. TestReactEval/[anthropic]_react_cat_message
2. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[anthropic]_bot_dm_tool_introspection
This comment was automatically generated by the eval CI pipeline. |
📝 WalkthroughWalkthroughThis change adds agent-created response attachments, inline files for posting tools, automatic execution for built-in file creation, stream-level file propagation, persisted-file recovery, documentation, and end-to-end coverage. Meeting summarization also removes its direct database dependency. ChangesResponse file attachments
Built-in auto-execution policy handling
Meeting file update cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
e2e/tests/agents/create-file-attachment.spec.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFile name uses kebab-case.
Coding guidelines require
snake_casefor.tsfile names, so this would becreate_file_attachment.spec.ts— though every existing spec and helper ine2e/uses kebab-case. If the e2e suite is intentionally exempt, ignore this; otherwise rename and updatee2e/scripts/ci-test-groups.mjs.As per coding guidelines: "Use
snake_casefor file names (snake_case.go/snake_case.ts(x))."🤖 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 `@e2e/tests/agents/create-file-attachment.spec.ts` at line 1, Rename the spec file from kebab-case to snake_case as required by the coding guidelines, and update the corresponding reference in e2e/scripts/ci-test-groups.mjs so the test grouping continues to resolve the renamed file.Source: Coding guidelines
mmtools/create_file.go (2)
65-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThread
ctxintoresolveCreateFileand wrap the upload in a span.The resolver receives a
context.Contextand discards it, so the upload (an external call) is untraced and uncancellable. Even thoughmmapi.Client.UploadFilecurrently takes no context, passingctxdown and adding a telemetry span around the upload keeps this on par with the rest of the LLM call path.As per coding guidelines: "Thread
ctx context.Contextas the first parameter through every entry point in the LLM call path... and add OpenTelemetry spans with the repo's telemetry helpers and attribute keys."♻️ Sketch
- Resolver: func(_ context.Context, llmCtx *llm.Context, argsGetter llm.ToolArgumentGetter) (string, error) { - return resolveCreateFile(client, llmCtx, argsGetter) + Resolver: func(ctx context.Context, llmCtx *llm.Context, argsGetter llm.ToolArgumentGetter) (string, error) { + return resolveCreateFile(ctx, client, llmCtx, argsGetter) },func resolveCreateFile(ctx context.Context, client mmapi.Client, llmCtx *llm.Context, argsGetter llm.ToolArgumentGetter) (string, error) { ctx, span := telemetry.Tracer().Start(ctx, "tool create file") defer span.End() // ... }🤖 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 `@mmtools/create_file.go` around lines 65 - 71, Update the resolver callback and resolveCreateFile to thread context.Context as the first parameter, then start and defer-end an OpenTelemetry span around the create-file upload using the repository’s telemetry helpers and attribute keys. Pass the threaded context through the upload path while preserving existing return and error behavior.Source: Coding guidelines
135-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ConsumeCreatedFilesdoes not consume — consider renaming.The function is a pure read (the test explicitly pins that "reading must not remove the files"), so
Consume*misleads callers into assuming single-use semantics. Something likeCreatedFileswould match behaviour. Cheap while the only caller is the new response-file decoration.🤖 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 `@mmtools/create_file.go` around lines 135 - 139, Rename ConsumeCreatedFiles to a read-oriented name such as CreatedFiles, updating its declaration and the only caller in the response-file decoration. Preserve the existing non-destructive behavior of returning ctx.CreatedFilesList() without removing or mutating the created files.mcpserver/tools/posts.go (1)
339-358: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTriplicated attachment/inline-file resolution across the three posting resolvers. The same sequence (legacy upload →
checkCombinedFileCap→uploadInlineFiles→mergePostFileIDs, plus identical comments) appears once per resolver; a shared helper would keep the cap-before-upload policy in one place.
mcpserver/tools/posts.go#L339-L358: replace the block with a call to a newresolvePostFileIDs(ctx, client, args.ChannelID, args.Attachments, args.Files, mcpContext.AccessMode)helper returning(fileIDs, attachmentMessage, inlineCount, err).mcpserver/tools/posts.go#L473-L492: call the same helper withdmChannel.Id.mcpserver/tools/posts.go#L563-L582: call the same helper withgmChannel.Id.🤖 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/posts.go` around lines 339 - 358, The attachment and inline-file resolution sequence is duplicated across three posting resolvers. In mcpserver/tools/posts.go at lines 339-358, 473-492, and 563-582, add and use a shared resolvePostFileIDs helper returning fileIDs, attachmentMessage, inlineCount, and err; pass the corresponding channel ID (args.ChannelID, dmChannel.Id, or gmChannel.Id), attachments, files, and access mode, while preserving the existing cap-before-inline-upload behavior and comments through the helper.conversations/response_files.go (2)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared "10 files per post" cap defined independently in two packages. Both
maxResponseAttachmentsandmaxPostAttachmentshardcode the same Mattermost per-post attachment limit (currently 10) with no shared source of truth, risking drift if the platform limit ever changes.
conversations/response_files.go#L16-L18: keep this as the canonical definition (or move to a small shared package) and havestreamingimport it.streaming/streaming.go#L39-L44: replace the independentmaxPostAttachments = 10constant with a reference toconversations's (or a shared) constant instead of redefining it.🤖 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 `@conversations/response_files.go` around lines 16 - 18, The attachment limit has duplicate definitions across packages. Keep conversations/response_files.go lines 16-18 as the canonical maxResponseAttachments definition, or move it to a shared package, and update streaming/streaming.go lines 39-44 to reference that symbol instead of defining maxPostAttachments = 10; adjust visibility or imports as needed.
55-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading truncation warning when
post.FileIdsalready exceeds the cap.When
roomis negative (post already has ≥maxResponseAttachmentsfiles) andcandidatesis empty,len(candidates) > roomstill evaluates true (e.g.,0 > -2), so the code logs"Truncating created files beyond the response attachment cap"with a computeddroppedcount even though nothing was actually truncated fromcandidates. This is a rare edge case (requires an existing post with more attachments than the cap) but produces a misleading log entry.♻️ Proposed guard
- if room := maxResponseAttachments - len(post.FileIds); len(candidates) > room { - if room < 0 { - room = 0 - } - if c.mmClient != nil { + if room := max(maxResponseAttachments-len(post.FileIds), 0); len(candidates) > room { + if c.mmClient != nil { c.mmClient.LogWarn("Truncating created files beyond the response attachment cap", "post_id", post.Id, "dropped", len(candidates)-room, "cap", maxResponseAttachments) } candidates = candidates[:room] }🤖 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 `@conversations/response_files.go` around lines 55 - 90, Update the truncation condition in collectAttachableFileIDs so the warning and slicing occur only when candidates are non-empty and actually exceed the available room. Preserve the room < 0 normalization and existing truncation behavior when candidate IDs must be dropped, while avoiding any warning when there is nothing to truncate.
🤖 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.
Nitpick comments:
In `@conversations/response_files.go`:
- Around line 16-18: The attachment limit has duplicate definitions across
packages. Keep conversations/response_files.go lines 16-18 as the canonical
maxResponseAttachments definition, or move it to a shared package, and update
streaming/streaming.go lines 39-44 to reference that symbol instead of defining
maxPostAttachments = 10; adjust visibility or imports as needed.
- Around line 55-90: Update the truncation condition in collectAttachableFileIDs
so the warning and slicing occur only when candidates are non-empty and actually
exceed the available room. Preserve the room < 0 normalization and existing
truncation behavior when candidate IDs must be dropped, while avoiding any
warning when there is nothing to truncate.
In `@e2e/tests/agents/create-file-attachment.spec.ts`:
- Line 1: Rename the spec file from kebab-case to snake_case as required by the
coding guidelines, and update the corresponding reference in
e2e/scripts/ci-test-groups.mjs so the test grouping continues to resolve the
renamed file.
In `@mcpserver/tools/posts.go`:
- Around line 339-358: The attachment and inline-file resolution sequence is
duplicated across three posting resolvers. In mcpserver/tools/posts.go at lines
339-358, 473-492, and 563-582, add and use a shared resolvePostFileIDs helper
returning fileIDs, attachmentMessage, inlineCount, and err; pass the
corresponding channel ID (args.ChannelID, dmChannel.Id, or gmChannel.Id),
attachments, files, and access mode, while preserving the existing
cap-before-inline-upload behavior and comments through the helper.
In `@mmtools/create_file.go`:
- Around line 65-71: Update the resolver callback and resolveCreateFile to
thread context.Context as the first parameter, then start and defer-end an
OpenTelemetry span around the create-file upload using the repository’s
telemetry helpers and attribute keys. Pass the threaded context through the
upload path while preserving existing return and error behavior.
- Around line 135-139: Rename ConsumeCreatedFiles to a read-oriented name such
as CreatedFiles, updating its declaration and the only caller in the
response-file decoration. Preserve the existing non-destructive behavior of
returning ctx.CreatedFilesList() without removing or mutating the created files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9e76a0cc-5c87-427d-97fc-9ea568a3bd70
📒 Files selected for processing (40)
conversations/bot_channel_tool_filter.goconversations/bot_channel_tool_filter_test.goconversations/conversations.goconversations/handle_messages.goconversations/regeneration.goconversations/response_files.goconversations/response_files_test.goconversations/test_helpers_test.goconversations/tool_approval.goconversations/tool_policy_test.godocs/admin_guide.mddocs/user_guide.mde2e/helpers/aimock-fixtures.tse2e/scripts/ci-test-groups.mjse2e/tests/agents/create-file-attachment.spec.tsllm/context.gollm/context_test.gollm/stream.gollm/tools.gollmcontext/llm_context.gollmcontext/llm_context_test.gomcpserver/tools/file_utils.gomcpserver/tools/file_utils_test.gomcpserver/tools/posts.gomcpserver/tools/posts_test.gomcpserver/tools/schema_test.gomeetings/meeting_summarization.gomeetings/service.gommapi/client.gommapi/mocks/client_mock.gommtools/create_file.gommtools/create_file_test.gommtools/provider.gommtools/provider_test.goprompts/standard_personality_without_locale.tmplserver/main.gostreaming/benchmark_client_test.gostreaming/files_event_test.gostreaming/streaming.gostreaming/test_helpers_test.go
💤 Files with no reviewable changes (2)
- server/main.go
- meetings/service.go
2 participants
Summary
Gives the LLM an "Artifacts"-style ability to create text files and attach them to posts, in two complementary places:
1. Built-in
CreateFiletool — respond directly with file attachmentsmmtools/create_file.go): the model providesfile_name+content, the file is uploaded to the conversation channel via the plugin API and attached to the bot's streamed reply post at end of turn.llm.Tool.AutoExecuteflag, honored only for built-ins with emptyServerOrigin— same trust position as the MCP dynamic-loading meta-tools). No approval card; the webapp shows the standard auto-approved tool card.activate_aiflows).conversations/response_files.go) emits a newllm.EventTypeFilesevent before stream end; the streaming layer merges the IDs intopost.FileIdsand the server'sUpdatePostperforms the attach (AttachToPostsemantics; matchesmin_server_version11.9.0). Cross-request resumes (channel share-decision follow-ups) recover file IDs by scanning persistedCreateFiletool results (structured JSON results,ServerOrigin-guarded).EnableFileAttachmentsand the requesting user's upload permission before uploading. Regeneration replaces the previous generation's attachments.meetings/updatePostWithFile), which relied on a DB pre-link that ≥11.9 servers strip duringUpdatePostfile-change processing.2. Inline
filesparam on MCP posting toolscreate_post,dm, andgroup_messageacceptfiles: [{name, content}]and attach the created files to the new post. Available in both access modes (including the embedded server used by agent conversations, where the local-onlyattachmentsparam is hidden). Uploads run on the authenticated user's session, so server-side permissions apply; upload failures fail the tool call before any post is created.Also included: prompt guidance (
CreateFile-conditional block in the personality template), user/admin docs, and a Playwright e2e spec (e2e/tests/agents/create-file-attachment.spec.ts, registered ine2e-shard-2) covering both the DM CreateFile flow (asserting no approval UI and a rendered attachment) and the channelcreate_post-with-files flow.QA evidence (run against
mattermost-enterprise-edition:release-11.9via testcontainers):multiplayer-tool-calling3/3 passed (approval/share/redaction flows unaffected by theshouldAutoExecuteToolrestructuring).-raceon touched packages,make check-style,check-shards,check-i18nall green. (check-locksdrift on the dev VM was npm-version normalization only; branch touches neitherpackage.json.)QA test steps:
.pyattachment with an auto-approved CreateFile tool card and no approval prompt.create_post, the created post has the attachment.Ticket Link
None
Release Note
Summary by CodeRabbit