You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Wires dynamic MCP loading into conversation, approval, channel analysis, regeneration, and API flows, including safer prompt guidance for underspecified side-effecting actions.
Reason: The output does not provide any actual bug entries. It states it cannot access a bug tracker and includes a blank table template and instructions for the user to supply bugs, so it is not itself a list of bugs.
Reason: The output does not include descriptions of actual bugs; it only asks the user to provide bug entries and shows an empty table template. Therefore it does not include a description of each bug.
Reason: The output asks the user to paste bug entries and provides a table template with a "Reported by" column, but it does not actually attribute any specific bug to any user because no bugs or reporters are listed.
Rubric: attributes the bug about trying to save without a color and the save button not doing anything to @maria.nunez
Reason: The output does not mention the specific bug about saving without a color or the save button doing nothing, nor does it attribute that bug to @maria.nunez. It only asks for bug entries to be provided.
Rubric: the bug about the end user being able to change channel banner is attributed to @maria.nunez
Reason: The output does not mention any specific bug about an end user being able to change the channel banner, nor does it attribute such a bug to @maria.nunez. It only asks for bug entries to be provided.
Rubric: mentions Github and refers to the documentation
Reason: The output refers to documentation (docs.mattermost.com) but does not mention GitHub anywhere, so it does not satisfy the rubric requirement to mention GitHub and refer to the documentation.
Anthropic
⚠️Overall: 20/28 tests passed (71.4%)
Provider
Total
Passed
Failed
Pass Rate
⚠️ ANTHROPIC
28
20
8
71.4%
❌ Failed Evaluations
Show 8 failures
ANTHROPIC
1. TestReactEval/[anthropic]_react_cat_message
Score: 0.00
Rubric: The word/emoji is a cat emoji or a heart/love emoji
Reason: The output is the literal text "heart_eyes_cat" rather than an actual cat emoji (e.g., 😺/🐱) or a heart/love emoji (e.g., ❤️/😍).
Rubric: mentions @claudio.costa is working on adding code coverage tracking to the monorepo
Reason: The output says @claudio.costa is adding code coverage tracking to the webapp, not specifically to the monorepo. It does not mention the monorepo explicitly.
Reason: The output does not list any bugs or provide descriptions of each bug; it only states it cannot access a tracker and suggests ways to find bug details.
Reason: The output does not list any bugs at all, and therefore does not attribute each bug to a user. It instead explains it cannot access a tracker and suggests ways to find bugs.
Rubric: attributes the bug about trying to save without a color and the save button not doing anything to @maria.nunez
Reason: The output does not mention @maria.nunez or attribute any bug (including saving without a color or the save button doing nothing) to her. It only states inability to access trackers and suggests ways to find bugs.
Rubric: mentions Github and refers to the documentation
Reason: The output refers to documentation (docs.mattermost.com) but does not mention GitHub anywhere, so it does not satisfy the rubric requirement to mention GitHub and refer to the documentation.
This comment was automatically generated by the eval CI pipeline.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
This PR adds optional agent MCPDynamicToolLoading, preloads and validates embedded MCP tools (including bare-name aliasing and ServerOrigin checks), centralizes single-build LLM context construction with late conversation-ID binding, refactors tool-resolution and policy lookups, and expands tests and prompt text for dynamic MCP search/load workflows.
CreateAgentRequest and UpdateAgentRequest gain optional MCPDynamicToolLoading *bool fields that default to true on create and update only when provided. Tests verify defaulting, persistence, and JSON round-trips.
Tool Store Constructor Standardization api/api_llm_bridge.go, channels/analysis_conversation_test.go, conversations/bot_channel_tool_filter_test.go, conversations/tool_policy_test.go, prompts/standard_personality_without_locale_test.go
Replaces bare llm.NewToolStore() uses with explicit llm.NewToolStore(nil, false) across runtime and tests for consistent initialization.
Channel Analysis Tool Preloading and Validation api/api_channel.go, api/api_channel_analysis_test.go
Preloads required embedded MCP tools (read_channel, get_channel_info) into LLM context, computes available vs missing embedded tools (handles namespaced/bare aliases), logs diagnostics, and returns 500 on missing tools. Tests exercise preload and availability logic.
Tool Resolution with Metadata Validation channels/channels.go, channels/analysis_conversation_test.go
Introduces helpers to resolve MCP tools by exact or bare names, enforces embedded ServerOrigin, binds channel_id args, and rebuilds scoped tool stores containing only the bound embedded tools.
Tool Policy and Auto-Execution Resolution conversations/conversations.go, conversations/tool_policy_test.go
Resolves incoming tool-call names against the visible tool store (exact or unique bare-name), auto-allows MCP meta-tools, and looks up policy by ServerOrigin + bare name; updates auto-execution and all-tools-auto-run logic and adds tests for denormalization and disambiguation.
Conversation Context Building with Late Binding conversations/handle_messages.go, conversations/regeneration.go, conversations/single_build_test.go
Introduces buildConversationContextWithTools and AttachConversationID for single-pass MCP tool registry building, injects per-user MCP preference context options, and preserves visible-store semantics via post-build pruning. Tests verify single GetToolsForUser invocation and late-binding restoration.
Adds channelFollowUpMCPToolFilterContextOptions and shouldConstrainChannelFollowUpToAutoEverywhere to conditionally apply auto-run-everywhere filtering for channel follow-ups; filter preserves meta-tools and records disabled non-meta tools info when checker is nil. Tests cover fail-closed registry lookups, DM exclusion, and activation gating.
Approved Tool Block Resolution and Loaded State conversations/tool_approval.go, conversations/tool_approval_internal_test.go, conversations/loaded_state_flow_test.go
Adds resolveApprovedToolUseBlock to validate approved loaded-tool metadata (ServerOrigin, MCP bare name) before resolution, tightens Shared/DecidedAt semantics, and adds loaded-state tests for restore, revocation, and regeneration flows.
Adds dynamicWorkflowLLM stub and integration tests that simulate search→load→business tool sequences, asserting persistence of loaded tools, schema materialization in context, and policy-mode differences (auto-run vs ask).
Prompt Template and MCP Tests prompts/standard_personality_without_locale.tmpl, prompts/standard_personality_without_locale_test.go, api/api_mcp_test.go, api/api_config_test.go
Adds conditional dynamic MCP workflow instructions to the standard personality prompt when dynamic loading and meta-tools are present; updates tests for prompt rendering, bare-name exposure, retrieval override preservation, and admin-config round-trips.
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested labels
Setup Cloud Test Server
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 16.24% which is insufficient. The required threshold is 80.00%.
Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name
Status
Explanation
Title check
✅ Passed
The title 'dynamic mcp: wire conversation workflows' directly summarizes the main change—integrating dynamic MCP loading into conversation and related workflows.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches📝 Generate docstrings
Create stacked PR
Commit on current branch
🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch dynamic-mcp-05-conversations-api
Comment @coderabbitai help to get the list of available commands and usage tips.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Nitpick comments (2)
api/api_channel_analysis_test.go (1)
158-194: 💤 Low value
Consider closing the response body to prevent resource leaks.
At line 186, resp := recorder.Result() returns an *http.Response whose body should be closed. While this rarely causes issues in tests, it's good hygiene to close it.
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_channel_analysis_test.go` around lines 158 - 194, In
TestHandleChannelAnalysisPreloadsRequiredMCPTools, the http.Response returned by
recorder.Result() (resp) is never closed; after calling resp :=
recorder.Result() add a resp.Body.Close() (or immediately defer
resp.Body.Close()) to avoid leaking the response body; locate the resp variable
in TestHandleChannelAnalysisPreloadsRequiredMCPTools and insert the close right
after obtaining resp.
channels/channels.go (1)
117-144: 💤 Low value
Re-aliasing tool name alters the returned tool's identity.
At line 142, tool.Name = name mutates the copy to replace the namespaced name (e.g., mattermost__read_channel) with the bare name (read_channel). This is intentional per the docstring in api/api_channel.go (lines 190-195) explaining that preload re-aliases tools. However, this implicit mutation could surprise callers who expect the original namespaced name to be preserved.
Consider adding a brief inline comment to clarify this intentional re-aliasing, e.g., // Return tool with bare name for scoped binding.
🤖 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 `@channels/channels.go` around lines 117 - 144, In
requiredEmbeddedToolByExactOrBareName, callers may be surprised by the
intentional re-aliasing of the tool name; add a brief inline comment immediately
before the statement that sets tool.Name = name to explain this behavior (e.g.,
"// Return tool with bare name for scoped binding" or similar) so readers
understand the mutation is deliberate and matches the preload re-aliasing
behavior referenced in api/api_channel.go.
🤖 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 `@api/api_agents_test.go`:
- Around line 246-320: Combine the three separate tests
TestCreateAgentMCPDynamicToolLoadingDefaultsTrueWhenOmitted,
TestCreateAgentMCPDynamicToolLoadingPersistsExplicitFalse, and
TestCreateAgentMCPDynamicToolLoadingPersistsExplicitTrue into a single
table-driven test that iterates cases for "omitted", "explicit true", and
"explicit false" and runs each as a subtest; keep the shared setup calls
(setupAgentTestEnvironment, mockLicensed, mocks for
HasPermissionTo/CreateBot/LogError) and for each case supply the request body
(nil with deletion for omitted, or map with "mcpDynamicToolLoading":
true/false), perform doRequest, decode into llm.BotConfig and assert both the
response value and stored value in
e.agentStore.agents[agent.ID].MCPDynamicToolLoading accordingly; also refactor
the similar tests in the later block referenced (lines ~895-1020) the same way
so all MCP dynamic-tool-loading variants are covered by a single table-driven
subtest loop.
In `@api/api_mcp_test.go`:
- Around line 98-212: Combine the three separate tests
TestHandleGetUserMCPToolsReturnsBareNamesForNamespacedRuntimeTools,
TestHandleGetUserMCPToolsBareAllowlistUICompatibility, and
TestHandleGetUserMCPToolsReturnsUpstreamDescriptionNotOverride into a single
table-driven test (e.g. TestHandleGetUserMCPTools_TableDriven) that iterates
over cases with fields for server config, mock tool (use
mockMCPClientManager.tools), expected tool.Name, Enabled, Policy and
Description; inside the loop call SetupTestEnvironment once per subtest, set
e.config.mcpConfig and e.api.mcpClientManager, call getUserMCPToolsResponse and
assert the expected values using t.Run for each case, keeping existing unique
symbols (getUserMCPToolsResponse, mockMCPClientManager, mcp.ServerConfig,
mcp.ToolConfig) so behavior from the three original tests is preserved as table
entries.
In `@conversations/bot_channel_tool_filter_test.go`:
- Around line 247-452: Combine the repeated "ChannelFollowUpStrict" tests into a
single table-driven subtest that iterates cases (e.g., failures on GetPost,
GetUser, DM-no-filter, ActivateAI true) instead of duplicating logic; for each
case provide inputs/fixtures (origin, config, builder tools, mmClient
expectations, rootPost/user, toolPolicyChecker) and assertions, then run the
same flow that calls channelFollowUpMCPToolFilterContextOptions,
buildChannelFollowUpStrictContext, and uses channelFollowUpSearchToolNames to
assert presence/absence of "jira__safe_tool" and "jira__ask_tool"; retain and
reuse helper symbols like newChannelFollowUpTestBuilder,
channelFollowUpTestMCPTool, buildChannelFollowUpStrictContext, and
channelFollowUpMCPToolFilterContextOptions to construct each case and move
per-case mmClient setups into the table entries' setup functions to keep tests
concise and table-driven.
In `@conversations/dynamic_mcp_workflow_test.go`:
- Around line 78-250: Combine TestDynamicMCPStrictSearchLoadCallHappyPath and
TestDynamicMCPLoadedToolStillRequiresApprovalWhenPolicyAsks into a single
table-driven test that iterates over cases differing by toolPolicyChecker (e.g.,
origin -> "get_issue" {policy: mcp.ToolPolicyAutoRunInDM, enabled:true} vs
mcp.ToolPolicyAsk) and expected outcomes (resolverCalls >0 vs 0,
foundPendingBusinessTool true/false, number of lm.requests, turns length,
loadedStore rows assertions). Reuse the shared harness setup
(loadedStateConversationStore(), loadedStateStore{}, jiraTool,
newChannelFollowUpTestBuilder, builder.SetLoadedMCPToolStore(loadedStore), lm :=
&dynamicWorkflowLLM{}, bot := loadedStateBot(lm), llmContext :=
builder.BuildLLMContextUserRequest(...), and c := &Conversations{...}) inside
the loop, and assert per-case expectations for resolverCalls, tool approval
events, lm.requests indices (check
lm.requests[2].Context.Tools.GetTool("jira__get_issue")), and persisted
loadedStore rows; leave TestDynamicMCPMetaToolsBypassApproval as a separate test
unchanged.
In `@prompts/standard_personality_without_locale_test.go`:
- Around line 184-252: The four tests
TestStandardPersonalityIncludesDynamicToolWorkflowWhenEnabled,
TestStandardPersonalityIncludesDynamicToolWorkflowWithDisabledToolsInfo,
TestStandardPersonalityOmitsDynamicToolWorkflowWhenFlagOff, and
TestStandardPersonalityOmitsDynamicToolWorkflowWithoutMetaTools should be
combined into a single table-driven test that iterates subtests via t.Run;
create a slice of cases with a name, an llm.Context (varying Tools:
dynamicMetaToolStore() vs llm.NewNoTools(), MCPDynamicToolLoading true/false,
and DisabledToolsInfo when needed), and expected positive/negative string
assertions, then for each case call renderStandardPersonalityWithoutLocale and
run assert.Contains/assert.NotContains according to the case expectations; keep
references to renderStandardPersonalityWithoutLocale, dynamicMetaToolStore,
llm.NewNoTools, DisabledToolsInfo, and MCPDynamicToolLoading when constructing
the cases so the logic is identical to the original tests.
---
Nitpick comments:
In `@api/api_channel_analysis_test.go`:
- Around line 158-194: In TestHandleChannelAnalysisPreloadsRequiredMCPTools, the
http.Response returned by recorder.Result() (resp) is never closed; after
calling resp := recorder.Result() add a resp.Body.Close() (or immediately defer
resp.Body.Close()) to avoid leaking the response body; locate the resp variable
in TestHandleChannelAnalysisPreloadsRequiredMCPTools and insert the close right
after obtaining resp.
In `@channels/channels.go`:
- Around line 117-144: In requiredEmbeddedToolByExactOrBareName, callers may be
surprised by the intentional re-aliasing of the tool name; add a brief inline
comment immediately before the statement that sets tool.Name = name to explain
this behavior (e.g., "// Return tool with bare name for scoped binding" or
similar) so readers understand the mutation is deliberate and matches the
preload re-aliasing behavior referenced in api/api_channel.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
Re-prune disabled MCP servers after AttachConversationID.
buildConversationContextWithTools strips DisabledMCPServerOrigins before the conversation exists, but AttachConversationID restores loaded tools afterwards. In a DM or group DM, that can put a previously loaded tool from a user-disabled server back into llmContext.Tools, so existing conversations ignore the user's MCP preference.
Proposed fix
c.contextBuilder.AttachConversationID(llmContext, bot, convResult.ConversationID)
+ removePreFilteredMCPServersFromVisibleStore(llmContext)
// Anchor this run's trace to the user turn ID. Link to the previous user
🤖 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/handle_messages.go` around lines 438 - 458, The context is
rebuilt with buildConversationContextWithTools which prunes
DisabledMCPServerOrigins too early and AttachConversationID can re-add tools;
after calling c.contextBuilder.AttachConversationID(llmContext, bot,
convResult.ConversationID) re-apply the pruning of DisabledMCPServerOrigins (or
call the existing helper that removes user-disabled MCP servers from
llmContext.Tools) so DM/group DM flows do not reintroduce tools the user
disabled; update the sequence around CreateOrGetDMConversation /
AttachConversationID to prune llmContext.Tools immediately after
AttachConversationID (ensuring ensureDMWebSearchTracking still runs on the final
pruned context).
🧹 Nitpick comments (2)
api/api_channel_analysis_test.go (1)
196-212: ⚡ Quick win
Convert this two-scenario test to table-driven subtests.
This function exercises two distinct cases (non-embedded origin missing, then embedded origin present) but keeps them in one linear test body. Please convert to a tests := []struct{...} + t.Run(...) pattern for consistency with repo test rules.
As per coding guidelines: **/*_test.go: Go tests must be table-driven when there is more than one test case.
🤖 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_channel_analysis_test.go` around lines 196 - 212, Replace the linear
TestChannelAnalysisToolAvailabilityRequiresEmbeddedOrigin with a table-driven
test: create a tests := []struct{name string, initialTools []llm.Tool, addTools
[]llm.Tool, expectedMissing []string}{...} containing the two scenarios (first:
initialTools includes read_channel with ServerOrigin
"https://remote.example.com" and get_channel_info with mcp.EmbeddedClientKey
expecting missing ["read_channel"]; second: initialTools includes both tools
with EmbeddedClientKey expecting missing []), then iterate tests with
t.Run(tc.name, func(t *testing.T){ tools := llm.NewToolStore(nil, false);
tools.AddTools(tc.initialTools); if len(tc.addTools)>0 {
tools.AddTools(tc.addTools) }; _, missing :=
channelAnalysisToolAvailability(tools); use require.ElementsMatch/require.Empty
as appropriate }), making sure each subtest creates a fresh ToolStore to avoid
state leakage.
conversations/bot_channel_tool_filter_test.go (1)
206-208: Zero-arg llm.NewToolStore() is safe (equivalent to llm.NewToolStore(nil, false)).
llm.NewToolStore is func NewToolStore(options ...any) *ToolStore; when called with no arguments it defaults to log=nil and doTrace=false, so llm.NewToolStore() at lines 206-208 won’t cause a compile-time break. Standardizing to llm.NewToolStore(nil, false) is optional for consistency.
🤖 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 206 - 208, The
call llm.NewToolStore() is valid but for consistency make its defaults explicit:
in the llm.Context initialization replace llm.NewToolStore() with
llm.NewToolStore(nil, false) so the intent (log=nil, doTrace=false) is clear;
update the llm.Context creation where Tools is set to reference the explicit
call to llm.NewToolStore(nil, false).
🤖 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/loaded_state_flow_test.go`:
- Around line 213-216: The fake UpsertLoadedMCPTool currently appends every
tool, causing duplicates; change loadedStateStore.UpsertLoadedMCPTool to
implement upsert semantics by scanning s.rows for an existing
store.LoadedMCPTool with the same identity (match on the unique identifier
field(s) used by LoadedMCPTool, e.g., ID or Name), replace that entry in-place
if found, and return; only append if no match was found. Use the existing
function name UpsertLoadedMCPTool and the loadedStateStore.rows slice to locate
and update the matching element.
---
Outside diff comments:
In `@conversations/handle_messages.go`:
- Around line 438-458: The context is rebuilt with
buildConversationContextWithTools which prunes DisabledMCPServerOrigins too
early and AttachConversationID can re-add tools; after calling
c.contextBuilder.AttachConversationID(llmContext, bot,
convResult.ConversationID) re-apply the pruning of DisabledMCPServerOrigins (or
call the existing helper that removes user-disabled MCP servers from
llmContext.Tools) so DM/group DM flows do not reintroduce tools the user
disabled; update the sequence around CreateOrGetDMConversation /
AttachConversationID to prune llmContext.Tools immediately after
AttachConversationID (ensuring ensureDMWebSearchTracking still runs on the final
pruned context).
---
Nitpick comments:
In `@api/api_channel_analysis_test.go`:
- Around line 196-212: Replace the linear
TestChannelAnalysisToolAvailabilityRequiresEmbeddedOrigin with a table-driven
test: create a tests := []struct{name string, initialTools []llm.Tool, addTools
[]llm.Tool, expectedMissing []string}{...} containing the two scenarios (first:
initialTools includes read_channel with ServerOrigin
"https://remote.example.com" and get_channel_info with mcp.EmbeddedClientKey
expecting missing ["read_channel"]; second: initialTools includes both tools
with EmbeddedClientKey expecting missing []), then iterate tests with
t.Run(tc.name, func(t *testing.T){ tools := llm.NewToolStore(nil, false);
tools.AddTools(tc.initialTools); if len(tc.addTools)>0 {
tools.AddTools(tc.addTools) }; _, missing :=
channelAnalysisToolAvailability(tools); use require.ElementsMatch/require.Empty
as appropriate }), making sure each subtest creates a fresh ToolStore to avoid
state leakage.
In `@conversations/bot_channel_tool_filter_test.go`:
- Around line 206-208: The call llm.NewToolStore() is valid but for consistency
make its defaults explicit: in the llm.Context initialization replace
llm.NewToolStore() with llm.NewToolStore(nil, false) so the intent (log=nil,
doTrace=false) is clear; update the llm.Context creation where Tools is set to
reference the explicit call to llm.NewToolStore(nil, false).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review — No Findings
I performed a comprehensive security analysis of the changes in this PR which wire dynamic MCP tool loading into conversation, approval, channel analysis, regeneration, and API flows. I examined:
Tool resolution and approval flow (resolveApprovedToolUseBlock): Validates ServerOrigin and MCPBareName metadata from stored conversation blocks before executing, preventing tool substitution after registry changes. The HandleToolCall path verifies conv.UserID == userID (requester-only gating).
Bot-activated channel constraint enforcement: The pre-build WithLLMContextMCPToolFilter restricts the MCP tool registry to only auto_run_everywhere-policy tools before the strict store is built. This means search_tools/load_tool meta-tools can only discover/load tools with the appropriate policy — not bypass the constraint. The post-build applyBotChannelAutoEverywhereToolFilter provides defense-in-depth.
DM disabled-server re-pruning: removePreFilteredMCPServersFromVisibleStore is correctly called after AttachConversationID (which may restore loaded tools), so user-disabled MCP servers remain pruned even after late-binding the conversation.
Fail-closed patterns: When toolPolicyChecker is nil, shouldAutoExecuteTool returns false, botChannelAutoEverywhereKeepTool returns false, and allToolsAutoRunEverywhere returns false — all fail-closed.
Shared flag scoping: The HandleToolResult change narrows Shared=true to only tool blocks belonging to the clicked post (via clickedPostToolUseIDs), reducing the blast radius of the share decision — a security improvement.
Meta-tool identity check: isMCPMetaToolCall requires BOTH name-match AND empty ServerOrigin, preventing an MCP server from registering a tool named search_tools to gain auto-approval.
No medium, high, or critical vulnerabilities identified.
The reason will be displayed to describe this comment to others. Learn more.
Security Review — No Findings
I reviewed the production code changes in this PR with focus on the security-sensitive areas for a multi-user Mattermost plugin:
Areas Analyzed
MCP meta-tools (search_tools, load_tool) bypassing approval — These are intentionally auto-approved as they are read-only discovery/loading operations. search_tools only returns tool names/summaries from the user-scoped registry (built via GetToolsForUser). load_tool only materializes a tool's schema. Business tools loaded via load_tool still go through full policy enforcement before execution.
Channel tool filtering preserving meta-tools — applyBotChannelAutoEverywhereToolFilter now preserves meta-tools even in fail-closed mode. This is safe because loaded business tools still require auto_run_everywhere policy to auto-execute in channels, and tools with ask policy surface for user approval.
resolveApprovedToolUseBlock server origin validation — This is a security improvement. The new function validates ServerOrigin and MCPBareName against persisted approval metadata before executing, preventing tool identity spoofing during deferred approval execution.
resolveToolStoreLookupName ambiguity handling — Fails closed when a bare tool name maps to multiple registered tools without a disambiguating ServerOrigin. Prevents unintended tool execution.
Meta-tool spoofing resistance — isMCPMetaToolCall requires tc.ServerOrigin == "" and verifies the tool store doesn't associate the name with a non-empty origin, preventing an external MCP server from registering tools named search_tools/load_tool that bypass approval.
Prompt injection via dynamic tool workflow — The system prompt instructs the LLM to ask clarification before side-effecting actions. While this is a defense-in-depth measure (LLM instructions are not a security boundary), the server-side policy enforcement (shouldAutoExecuteTool, allToolsAutoRunEverywhere) provides the actual gate — tools that need approval still require it regardless of what the LLM is instructed to do.
Conclusion
No medium, high, or critical vulnerabilities identified. The PR maintains existing permission/authorization patterns while adding server-origin validation on tool approval execution (a security hardening).
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
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
nickmisasi
force-pushed
the
dynamic-mcp-04-context-toolrunner
branch
from
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Summary
Wires dynamic MCP loading into conversation, approval, channel analysis, regeneration, and API flows, including safer prompt guidance for underspecified side-effecting actions.
Stack: 5/6
Ticket Link
N/A
Screenshots
N/A
Release Note
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests