Exempt embedded Mattermost MCP tools from the MCP license gate - #924
Conversation
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…onsole Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5de1bfccc6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func (c *Conversations) isRemoteMCPLicensed() bool { | ||
| return c.licenseChecker != nil && c.licenseChecker.IsBasicsLicensed() |
There was a problem hiding this comment.
When an unlicensed server already has a remote MCP tool configured with an auto_run_in_dm or auto_run_everywhere policy, this predicate is never consulted: the initial DM path passes shouldAutoExecuteTool to the runner at conversations/conversations.go:215-216, and toolrunner/toolrunner.go:285-315 executes an all-approved batch directly without calling either approval handler. Consequently, remote tools can still run on an unlicensed server despite the new UI notice and approval checks; incorporate the license into tool availability or the auto-execution callback before the runner resolves the tool.
Useful? React with 👍 / 👎.
| if (slices.Contains(acceptedToolIDs, b.ID) || b.WouldAutoExecute) && !c.isRemoteMCPLicensed() { | ||
| return ErrRemoteMCPNotLicensed |
There was a problem hiding this comment.
When a mixed batch persisted a remote call with WouldAutoExecute=true and its policy is changed to ask or disabled before the user resolves the batch, this preflight returns 403 solely from the stale marker even if the remote ID is omitted from acceptedToolIDs. The execution branch at conversations/tool_approval.go:230 deliberately performs a fresh autoExec policy check and would reject rather than run that call, so the early return instead prevents the user from rejecting it or processing other calls in the batch; base this license decision on the fresh execution decision rather than WouldAutoExecute alone.
Useful? React with 👍 / 👎.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemote MCP licensing is enforced when building tool contexts, approving tools, sharing results, serving API decisions, and configuring remote servers. Embedded MCP and built-in tools remain available without the remote MCP license, with updated tests and documentation. ChangesRemote MCP licensing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant MCPServers
participant LicenseHook
participant Conversations
participant LLMContextBuilder
Admin->>MCPServers: Open MCP configuration
MCPServers->>LicenseHook: Read Basics license state
LicenseHook-->>MCPServers: Licensed or unlicensed
MCPServers-->>Admin: Show controls or qualifying-plan notice
Conversations->>LicenseHook: Check remote MCP decision
LicenseHook-->>Conversations: Allow or ErrRemoteMCPNotLicensed
LLMContextBuilder->>LicenseHook: Check remote MCP availability
LicenseHook-->>LLMContextBuilder: Include or filter remote tools
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}
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…cceptance 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. |
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 `@conversations/conversations.go`:
- Around line 251-258: Apply the remote MCP license check to the
allToolsAutoRunEverywhere batch predicate as well as the existing
individual-tool path. Ensure an unlicensed remote MCP initial batch cannot
qualify for automatic execution, and add coverage for this unlicensed
initial-batch scenario.
🪄 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: b9b21787-19b7-42c9-ae07-2940e7528478
📒 Files selected for processing (9)
conversations/ask_user_question_flow_test.goconversations/conversations.goconversations/dynamic_mcp_workflow_test.goconversations/tool_approval.goconversations/tool_approval_license_test.goconversations/tool_policy_test.godocs/admin_guide.mddocs/upgrading_to_2.0.mdwebapp/src/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (3)
- webapp/src/i18n/en.json
- docs/upgrading_to_2.0.md
- docs/admin_guide.md
…er config UI when unlicensed Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
conversations/bot_channel_tool_filter_test.go (1)
91-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBackward-compatible parameterization; duplicated license-mock logic noted for consolidation.
Delegating the default (
licensed=true) preserves existing callers' behaviour. The license-conditional mock block (Lines 103-107) duplicates the same logic intool_approval_license_test.go; see the consolidated comment.🤖 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/bot_channel_tool_filter_test.go` around lines 91 - 107, Consolidate the duplicated license-mock setup used by newChannelFollowUpTestBuilderWithLicense with the existing helper or shared logic from tool_approval_license_test.go. Preserve newChannelFollowUpTestBuilder’s default licensed=true behavior and ensure licensed and unlicensed cases continue returning the corresponding mock License values.api/api_agents_test.go (1)
85-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
model.LicenseShortSkuEnterpriseconstant instead of the literal"enterprise".Every other license mock in this PR (tool_approval_license_test.go, bot_channel_tool_filter_test.go, single_build_test.go, api_test.go) uses
model.LicenseShortSkuEnterprise. The raw string here is a minor inconsistency that could silently drift if the SKU value ever changes upstream.♻️ Proposed fix
func mockLicensed(mockAPI *plugintest.API) { overrideLicenseMocks(mockAPI, &model.License{ Features: &model.Features{ LDAP: model.NewPointer(true), }, - SkuShortName: "enterprise", + SkuShortName: model.LicenseShortSkuEnterprise, }) }Please confirm
model.LicenseShortSkuEnterprisestill resolves to"enterprise"in the pinnedmattermost/server/publicversion used by this repo.🤖 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 `@api/api_agents_test.go` around lines 85 - 93, Update mockLicensed to use model.LicenseShortSkuEnterprise for License.SkuShortName instead of the literal "enterprise", preserving the existing enterprise license mock behavior and aligning it with the pinned Mattermost model constant.conversations/tool_approval_license_test.go (2)
71-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate license-mock boilerplate vs.
bot_channel_tool_filter_test.go.The
if licensed {...} else {...}block mockingGetLicense/GetConfig(Lines 78-83) is identical tonewChannelFollowUpTestBuilderWithLicenseinconversations/bot_channel_tool_filter_test.go. See the consolidated comment below for a suggested shared helper.🤖 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/tool_approval_license_test.go` around lines 71 - 108, Consolidate the duplicated license/config mock setup in toolLicenseTestBuilder with the existing newChannelFollowUpTestBuilderWithLicense helper from bot_channel_tool_filter_test.go. Reuse or extract a shared helper for GetConfig and GetLicense behavior, preserving the licensed and unlicensed responses while removing the local if/else boilerplate.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated license-mock block into a shared test helper. Both files mock
GetLicense/GetConfigwith the identicalif licensed {...Enterprise...} else {...nil...}block; the shared root cause is the lack of a common helper for license-state mocking in theconversationstest package.
conversations/tool_approval_license_test.go#L71-108: extract theGetConfig/GetLicenseconditional (Lines 78-83) into a shared helper (e.g.,mockLicenseState(mockAPI *plugintest.API, licensed bool)) and call it here.conversations/bot_channel_tool_filter_test.go#L91-107: replace the identical conditional (Lines 103-107) with a call to the same shared helper.🤖 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/tool_approval_license_test.go` at line 1, Extract the duplicated GetConfig/GetLicense licensed-state conditional from the tests into a shared conversations test helper such as mockLicenseState, accepting *plugintest.API and a licensed boolean. Replace the inline blocks in the license test and bot channel tool filter test with calls to this helper, preserving both Enterprise license and nil-license behaviors.
🤖 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 `@webapp/src/components/system_console/mcp_servers.tsx`:
- Around line 595-598: Update the EnterpriseChip usage in the remote MCP server
section to pass a localized plan-neutral title such as “Licensed feature”
through its title prop, replacing the default “Enterprise feature” wording while
preserving the existing text and subtext.
---
Nitpick comments:
In `@api/api_agents_test.go`:
- Around line 85-93: Update mockLicensed to use model.LicenseShortSkuEnterprise
for License.SkuShortName instead of the literal "enterprise", preserving the
existing enterprise license mock behavior and aligning it with the pinned
Mattermost model constant.
In `@conversations/bot_channel_tool_filter_test.go`:
- Around line 91-107: Consolidate the duplicated license-mock setup used by
newChannelFollowUpTestBuilderWithLicense with the existing helper or shared
logic from tool_approval_license_test.go. Preserve
newChannelFollowUpTestBuilder’s default licensed=true behavior and ensure
licensed and unlicensed cases continue returning the corresponding mock License
values.
In `@conversations/tool_approval_license_test.go`:
- Around line 71-108: Consolidate the duplicated license/config mock setup in
toolLicenseTestBuilder with the existing
newChannelFollowUpTestBuilderWithLicense helper from
bot_channel_tool_filter_test.go. Reuse or extract a shared helper for GetConfig
and GetLicense behavior, preserving the licensed and unlicensed responses while
removing the local if/else boilerplate.
- Line 1: Extract the duplicated GetConfig/GetLicense licensed-state conditional
from the tests into a shared conversations test helper such as mockLicenseState,
accepting *plugintest.API and a licensed boolean. Replace the inline blocks in
the license test and bot channel tool filter test with calls to this helper,
preserving both Enterprise license and nil-license behaviors.
🪄 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: 343e1a4e-1f14-4ccb-bc28-c71dae63fcc1
📒 Files selected for processing (15)
api/api_agents_test.goapi/api_test.goconversations/ask_user_question_flow_test.goconversations/bot_channel_tool_filter_test.goconversations/single_build_test.goconversations/tool_approval.goconversations/tool_approval_license_test.godocs/admin_guide.mdllmcontext/llm_context.gollmcontext/llm_context_license_test.gollmcontext/llm_context_test.gomcp/vetted_tools.gowebapp/src/components/system_console/mcp_servers.test.tsxwebapp/src/components/system_console/mcp_servers.tsxwebapp/src/i18n/en.json
💤 Files with no reviewable changes (1)
- webapp/src/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- conversations/tool_approval.go
- conversations/ask_user_question_flow_test.go
… helpers Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
3 participants
Summary
On a Professional (or unlicensed) server, submitting tool decisions for embedded Mattermost MCP tools (e.g. read_channel, search_posts, create_post) failed with "Failed to submit tool decisions" in the approval UI and {"level":"error","msg":"feature not licensed","plugin_id":"mattermost-ai"} in the server log.
This PR fixes the issue by allowing the embedded MCP server on all plans.
Ticket Link
None
Screenshots
Captured from the live System Console of real Mattermost v11.9.1 servers running the actual builds of this branch (post-merge with master) and of current master. "Unlicensed" shots are from Team Edition (no license); the "licensed" shot is from Enterprise Edition with its auto-applied Entry license.
With a qualifying license (Entry, auto-applied on Enterprise Edition) the panel is unchanged:
After: licensed server shows the full remote MCP server configuration as before
Release Note
To show artifacts inline, enable in settings.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests