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
Adds a new per-tool Auto Run (Everywhere) policy while renaming the existing admin console option to Auto Run (DM). The legacy auto_run value keeps its current behavior: DMs auto-run to completion, and channel/@mention flows auto-execute but still require result sharing. The new auto_run_everywhere value auto-runs tools to completion in both DMs and multiplayer/channel contexts without requiring Share.
Also updates backend policy evaluation, streaming/callback flow, and targeted Go/E2E coverage for the DM-vs-channel split.
Follow-up fixes on top of the initial implementation:
stop channel auto-share continuation when UpdatePost fails, so websocket/callback processing cannot proceed against stale post state
scope mocked channel thread opening to the triggering post and stabilize the mocked tool-policy E2E flows so CI opens the intended RHS thread and matches the intended follow-up request
enable channel mention tool calling in the mocked tool-policy container so the new channel @mention coverage exercises the real channel approval/share path in CI
disambiguate the mocked channel DM-only request path by using the same split title/main mock pattern as the everywhere branch, keyed off the tool-only read_channel marker instead of raw mention text
simplify the mocked channel tool-policy request sequencing so the DM-only channel branch deterministically yields a tool call and Share-stage UI in CI without title-generation collisions
extract a local type alias for vetted tool config policy typing in webapp/src/client.tsx
refresh the MCP auto-approval comment to match the widened mcp.IsToolPolicyAutoRun behavior
rename the channel result-review helper in streaming.go for clearer semantics
match the mocked everywhere follow-up on the tool request marker locally, which resolves the final locally reproduced provider-666 miss in the mocked policy spec
QA notes:
Auto Run (DM) keeps the existing two-stage channel flow (Auto-approved badge + Share/Keep private).
Auto Run (Everywhere) skips both approval stages and continues directly after tool execution in channel/@mention flows.
System console tool policy labels now show Auto Run (DM), Auto Run (Everywhere), and Ask Every Time.
Focused Go tests, go vet, webapp type-checks, webapp lint, and webapp production build passed locally.
Docker/testcontainers were brought up locally on the agent, the plugin bundle was rebuilt, and tests/tool-config/mock-api/tool-call-policies.spec.ts now passes locally against the current branch head.
Local shard reruns still surface unrelated failures in tests/system-console/debug-panel.spec.ts and tests/action-item-extraction/ui-verification.spec.ts, so the latest commit is intentionally limited to the mocked tool-policy failure that was reproducible and related to this PR.
Ticket Link
NONE
Screenshots
Release Note
Added a new per-tool MCP approval policy, Auto Run (Everywhere), and renamed the existing Auto Run policy to Auto Run (DM). Auto Run (DM) keeps the existing behavior where channel/@mention tool calls still require result sharing, while Auto Run (Everywhere) now allows configured tools to run to completion without additional approval in both DMs and multiplayer/channel contexts.
Summary by CodeRabbit
New Features
Added "Auto Run (Everywhere)" policy — when selected, channel tool calls are auto-approved and can be auto-shared without requiring manual result review.
"Auto Run (DM)" remains DM-scoped and still requires result review in channels.
UI
Tool config dropdown and system console show separate "Auto Run (DM)" and "Auto Run (Everywhere)" options.
Tests
Added/updated unit and e2e tests for DM vs. everywhere auto-run and sharing behavior.
Rubric: The word/emoji is a cat emoji or a heart/love emoji
Reason: The output is the text "smiley_cat", not an actual cat emoji (e.g., 🐱/😺) or a heart/love emoji (e.g., ❤️/😍). Therefore it does not meet the rubric.
Anthropic
⚠️Overall: 18/19 tests passed (94.7%)
Provider
Total
Passed
Failed
Pass Rate
⚠️ ANTHROPIC
19
18
1
94.7%
❌ Failed Evaluations
Show 1 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 text "heart_eyes_cat", which is not itself a cat emoji (e.g., 😺/🐱) or a heart/love emoji (e.g., ❤️/😍).
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.
It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
📝 Walkthrough
Walkthrough
Adds an auto_run_everywhere MCP tool policy, helpers to recognize auto-run variants, updates server/client policy normalization and auto-approval checks, and changes conversation/streaming/tool-handling plus UI and e2e tests so channel tool results can be auto-shared under the new policy while preserving DM-scoped review behavior.
Add MCPToolPolicyAutoRunEverywhere / ToolPolicyAutoRunEverywhere; add IsToolPolicyAutoRun and IsToolPolicyAutoRunEverywhere; normalize/validate policies to treat both auto-run variants as auto-run.
Conversation control & auto-approval conversations/conversations.go, conversations/mcp_auto_approval.go
Introduce isToolAutoRunnable; switch auto-approval checks to use auto-run helpers; restrict MCP auto-approval stream wrapping to channels (non-DM).
Tool handling & result sharing conversations/tool_handling.go, conversations/tool_handling_test.go
Add/clear streaming.AutoShareToolResultProp across flows; gate auto-share on new prop; adjust DM vs channel auto-approval behavior; add tests for auto_run_everywhere.
Add AutoShareToolResultProp; refactor policy checks into shouldAutoApproveToolCall and helpers; compute requiresResultReview and set/delete pending vs auto-share props accordingly.
Widen tool policy UI helper labels and types to include “Auto Run (Everywhere)”; add Playwright helpers and tests verifying channel auto-share under auto_run_everywhere and review behavior under auto_run.
MCP tool config tests mcp/tool_config_test.go
Add test cases asserting GetToolPolicy and IsToolAutoRun for ToolPolicyAutoRunEverywhere.
Webapp UI types and console webapp/src/client.tsx, webapp/src/components/system_console/mcp_servers.tsx, webapp/src/components/system_console/mcp_tool_config_row.tsx
Extend vetted tool types and MCP tool-config union to include auto_run_everywhere; update dropdown labels to “Auto Run (DM)” and “Auto Run (Everywhere)” and adjust select width.
E2E system console helper e2e/helpers/system-console-container.ts
Add optional enableChannelMentionToolCalling flag and include it in plugin config with default false.
Sequence Diagram(s)
sequenceDiagram
participant User as User
participant Conv as Conversations
participant Policy as MCPPolicyChecker
participant Stream as Streaming
participant KV as KV
User->>Conv: submit request (triggers tool call)
Conv->>Policy: GetToolPolicy(tool)
Policy-->>Conv: policy + enabled
alt policy == "auto_run_everywhere" && enabled
Conv->>Stream: mark auto-approved & set AutoShareToolResultProp
Stream->>KV: persist shared tool results
Stream->>Conv: notify completed (auto-shared)
Conv->>User: stream auto-shared post
else policy == "auto_run" && enabled
Conv->>Stream: mark auto-approved & set PendingToolResultProp
Stream->>KV: persist pending tool results
Conv-->>User: stream post requiring review (Share/Keep)
else
Conv->>Stream: normal tool-call flow (manual review)
end
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
streaming/streaming.go (1)
408-440: ⚠️ Potential issue | 🟠 Major
Don't continue auto-execution after a failed UpdatePost.
The new auto-share path depends on AutoShareToolResultProp being persisted first. conversations/tool_handling.go:535-548 reads that prop from the stored post before deciding whether to auto-share, so if either UpdatePost fails but autoExecuteCallback still runs, the callback can observe stale state and take the wrong path. Please return before the websocket/callback path when the post update fails.
🛠️ Suggested guard
if err := p.mmClient.UpdatePost(post); err != nil {
p.mmClient.LogError("Failed to update post with auto-approved tool call", "error", err)
+ return
}
...
if updErr := p.mmClient.UpdatePost(post); updErr != nil {
p.mmClient.LogError("Failed to update post with tool call", "error", updErr)
+ return
}
As per coding guidelines, **/*.{go,ts,tsx}: Check all errors explicitly in production code.
Also applies to: 639-677
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@streaming/streaming.go` around lines 408 - 440, The UpdatePost error is
ignored and execution continues, which can cause the subsequent
PublishWebSocketEvent and the goroutine calling p.autoExecuteCallback(post.Id,
requesterID, approvedIDs) to run against stale post state; modify the block
after calling p.mmClient.UpdatePost to return (or otherwise stop processing)
when err != nil so that you do not publish the websocket event or invoke
autoExecuteCallback on UpdatePost failure. Specifically, after calling
p.mmClient.UpdatePost (the call in the current function), check the returned err
and if non-nil log the error (via p.mmClient.LogError) and immediately return
from the surrounding function (or skip the websocket/callback path), preventing
PublishWebSocketEvent and the go p.autoExecuteCallback(...) invocation from
running when post persistence fails; ensure the same guard is applied to the
analogous block around lines 639-677.
🧹 Nitpick comments (2)
conversations/mcp_auto_approval.go (1)
60-60: Update the function doc to match the widened auto-run check.
The implementation now accepts any auto-run policy via helper; the top comment still describes only ToolPolicyAutoRun.
Doc tweak
-// MCP tool calls whose per-tool policy is ToolPolicyAutoRun + enabled.+// MCP tool calls whose per-tool policy is an auto-run policy+// (ToolPolicyAutoRun or ToolPolicyAutoRunEverywhere) + enabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@conversations/mcp_auto_approval.go` at line 60, Update the top-of-function
comment to reflect that the code now accepts any auto-run policy via the helper
instead of only ToolPolicyAutoRun; specifically, revise the doc for the function
in conversations/mcp_auto_approval.go (the comment above the function that
contains the check using mcp.IsToolPolicyAutoRun(policy) || !enabled) to state
that it handles all auto-run policies delegated to mcp.IsToolPolicyAutoRun
rather than only ToolPolicyAutoRun, and ensure the wording mentions the helper
function name (mcp.IsToolPolicyAutoRun) so the doc matches the widened auto-run
check behavior.
webapp/src/client.tsx (1)
529-529: Consider extracting the policy/tool-config type alias here to avoid drift.
The same inline union/object shape appears twice in this function. A local type alias would keep future policy updates one-touch.
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/client.tsx` at line 529, The function getVettedToolSeed currently
repeats the inline return type shape ({name: string; policy: 'auto_run' |
'auto_run_everywhere' | 'ask'; enabled: boolean}) in its signature and later in
the body; extract a local type alias (e.g., ToolPolicy or ToolConfig) near the
top of the file and replace both the function return type and any variable/array
typings inside getVettedToolSeed with that alias to ensure a single source of
truth for the policy union and object shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/tool-config/mock-api/tool-call-policies.spec.ts`:
- Around line 63-69: The helper openLatestThread queries reply counts globally
(replyIndicator) which can match stale threads; change openLatestThread to
accept or compute a Locator scoped to the triggering post (e.g., pass the post's
Locator or message text and use post.locator(/\\d+ repl/i) or post.getByText to
find the reply indicator relative to that post), then waitFor/ click that scoped
locator and then wait for '#rhsContainer' as before; update the two other
occurrences (around lines 403 and 466) to call the new signature with the
specific post Locator or identifier so the RHS opens for the intended post only.
---
Outside diff comments:
In `@streaming/streaming.go`:
- Around line 408-440: The UpdatePost error is ignored and execution continues,
which can cause the subsequent PublishWebSocketEvent and the goroutine calling
p.autoExecuteCallback(post.Id, requesterID, approvedIDs) to run against stale
post state; modify the block after calling p.mmClient.UpdatePost to return (or
otherwise stop processing) when err != nil so that you do not publish the
websocket event or invoke autoExecuteCallback on UpdatePost failure.
Specifically, after calling p.mmClient.UpdatePost (the call in the current
function), check the returned err and if non-nil log the error (via
p.mmClient.LogError) and immediately return from the surrounding function (or
skip the websocket/callback path), preventing PublishWebSocketEvent and the go
p.autoExecuteCallback(...) invocation from running when post persistence fails;
ensure the same guard is applied to the analogous block around lines 639-677.
---
Nitpick comments:
In `@conversations/mcp_auto_approval.go`:
- Line 60: Update the top-of-function comment to reflect that the code now
accepts any auto-run policy via the helper instead of only ToolPolicyAutoRun;
specifically, revise the doc for the function in
conversations/mcp_auto_approval.go (the comment above the function that contains
the check using mcp.IsToolPolicyAutoRun(policy) || !enabled) to state that it
handles all auto-run policies delegated to mcp.IsToolPolicyAutoRun rather than
only ToolPolicyAutoRun, and ensure the wording mentions the helper function name
(mcp.IsToolPolicyAutoRun) so the doc matches the widened auto-run check
behavior.
In `@webapp/src/client.tsx`:
- Line 529: The function getVettedToolSeed currently repeats the inline return
type shape ({name: string; policy: 'auto_run' | 'auto_run_everywhere' | 'ask';
enabled: boolean}) in its signature and later in the body; extract a local type
alias (e.g., ToolPolicy or ToolConfig) near the top of the file and replace both
the function return type and any variable/array typings inside getVettedToolSeed
with that alias to ensure a single source of truth for the policy union and
object shape.
🪄 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: Add Auto Run (Everywhere) tool approval policy
No high-confidence vulnerabilities found.
What was reviewed
This PR introduces a new admin-configured tool policy auto_run_everywhere that, unlike the existing auto_run (DM-only), bypasses both the call-approval and result-sharing approval steps in channels.
Analysis of key security paths
Post prop trust (AutoShareToolResultProp): The auto_share_tool_result prop that controls the auto-share flow is set exclusively by server-side code in the streaming layer (handleAutoApprovedToolCalls and StreamToPost). Clients cannot set arbitrary post props through the Mattermost API. Webhook/plugin-originated posts are already blocked from triggering tool calling via computeAllowToolsInChannel / isAutomatedInvoker.
Identity checks preserved: HandleToolResult verifies requesterID == userID and the auto-share path in AutoExecuteApprovedToolCalls passes the same requesterID extracted from the server-set LLMRequesterUserID post prop. The authorization chain is intact.
Mixed batch safety: allToolResultsRequireReview returns true (requires review) if ANY tool in the batch is not auto_run_everywhere. This is the safe direction — a single non-everywhere tool forces the entire batch through review.
Defense-in-depth gates intact: The EnableChannelMentionToolCalling config check, the allowToolsInChannel post prop check, and the requester identity check in both HandleToolCall and HandleToolResult are all preserved in the auto-share flow.
Admin-only configuration: The auto_run_everywhere policy can only be set through the system admin console. The UI labels clearly distinguish "Auto Run (DM)" from "Auto Run (Everywhere)".
Channel data isolation: The auto-share flow shares tool results (which may contain data from the requesting user's accessible channels) to the channel without user review. This is the intended behavior of the admin-opt-in policy. The existing auto_run policy already auto-executed the tools — the delta is only the removal of the result-sharing approval step, which the admin explicitly opts into per-tool.
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick comments (3)
streaming/streaming.go (1)
198-211: Function name is slightly misleading but behavior is correct.
The function allToolResultsRequireReview returns true if any tool requires review (not if all do). The logic is correct for the use case—if any single tool requires review, the entire batch should require review. The naming could be clearer (e.g., anyToolResultRequiresReview or requiresResultReview), but this is a minor suggestion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@streaming/streaming.go` around lines 198 - 211, Rename the misleading
function allToolResultsRequireReview to a clearer name like
anyToolResultRequiresReview (or requiresResultReview) and update all
references/call sites accordingly; inside the function keep the same logic and
references to p.toolPolicyChecker, toolCalls, p.toolPolicyChecker.GetToolPolicy,
toolPolicyAllowsAutoRun, and toolPolicyRequiresResultReview so behavior is
unchanged, and run/update any tests or usages that call the old function name to
use the new name.
26-27: Consider importing types directly for clarity.
The inline type X = import(...) syntax works but is non-idiomatic. Consider importing the types directly with the existing imports:
✨ Suggested refactor
-import { test, expect } from '@playwright/test';+import { test, expect, Page, Locator } from '@playwright/test';
import MattermostContainer from 'helpers/mmcontainer';
// ... other imports ...
-type Page = import('@playwright/test').Page;-type Locator = import('@playwright/test').Locator;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/tool-config/mock-api/tool-call-policies.spec.ts` around lines 26 -
27, Replace the non-idiomatic inline type aliases "type Page =
import('@playwright/test').Page;" and "type Locator =
import('@playwright/test').Locator;" with a direct type import from Playwright
(e.g., import type { Page, Locator } from '@playwright/test';) so the file uses
canonical TypeScript type imports; update any existing references to
Page/Locator as needed after replacing the aliases.
74-80: Hardcoded timeout may cause flakiness.
The waitForTimeout(1000) at line 79 is a hardcoded sleep that may cause test flakiness. Consider waiting for a specific condition instead, such as waiting for the RHS content to stabilize or for a specific element within the thread to appear.
✨ Suggested improvement
async function openThreadForPost(post: Locator, timeout: number = 30000): Promise<void> {
const replyIndicator = post.getByText(/\d+ repl/i);
await expect(replyIndicator).toBeVisible({timeout});
await replyIndicator.click();
await post.page().locator('#rhsContainer').waitFor({state: 'visible', timeout: 10000});
- await post.page().waitForTimeout(1000);+ // Wait for RHS content to be ready (e.g., first post in thread)+ await post.page().locator('#rhsContainer .post').first().waitFor({state: 'visible', timeout: 5000});
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/tool-config/mock-api/tool-call-policies.spec.ts` around lines 74 -
80, The hardcoded sleep in openThreadForPost causes flakiness; replace the
post.page().waitForTimeout(1000) with a deterministic wait for RHS content or a
specific thread element to appear/stabilize (e.g., waitFor a message item
selector inside '#rhsContainer' or for an element indicating thread load
completion). Update openThreadForPost to await
post.page().locator('#rhsContainer <thread-message-selector>').waitFor({ state:
'visible', timeout: <sane timeout> }) (or use waitForElementState/stable)
instead of the fixed 1000ms delay so the test proceeds only when the thread
content is ready.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests/tool-config/mock-api/tool-call-policies.spec.ts`:
- Around line 26-27: Replace the non-idiomatic inline type aliases "type Page =
import('@playwright/test').Page;" and "type Locator =
import('@playwright/test').Locator;" with a direct type import from Playwright
(e.g., import type { Page, Locator } from '@playwright/test';) so the file uses
canonical TypeScript type imports; update any existing references to
Page/Locator as needed after replacing the aliases.
- Around line 74-80: The hardcoded sleep in openThreadForPost causes flakiness;
replace the post.page().waitForTimeout(1000) with a deterministic wait for RHS
content or a specific thread element to appear/stabilize (e.g., waitFor a
message item selector inside '#rhsContainer' or for an element indicating thread
load completion). Update openThreadForPost to await
post.page().locator('#rhsContainer <thread-message-selector>').waitFor({ state:
'visible', timeout: <sane timeout> }) (or use waitForElementState/stable)
instead of the fixed 1000ms delay so the test proceeds only when the thread
content is ready.
In `@streaming/streaming.go`:
- Around line 198-211: Rename the misleading function
allToolResultsRequireReview to a clearer name like anyToolResultRequiresReview
(or requiresResultReview) and update all references/call sites accordingly;
inside the function keep the same logic and references to p.toolPolicyChecker,
toolCalls, p.toolPolicyChecker.GetToolPolicy, toolPolicyAllowsAutoRun, and
toolPolicyRequiresResultReview so behavior is unchanged, and run/update any
tests or usages that call the old function name to use the new name.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review: Add Auto Run (Everywhere) tool approval policy
No high-confidence vulnerabilities found.
What was reviewed
This PR introduces a new admin-configured tool policy auto_run_everywhere that, unlike the existing auto_run (DM-only auto-approval), bypasses both the call-approval and result-sharing approval steps in channels.
Analysis of key security paths
Post prop trust (AutoShareToolResultProp): The auto_share_tool_result prop controlling the auto-share flow is set exclusively by server-side code in the streaming layer (handleAutoApprovedToolCalls and StreamToPost), guarded by allToolResultsRequireReview which performs server-side policy checks. Clients cannot set arbitrary post props through the Mattermost API. Webhook/bot-originated posts are already blocked from triggering tool calling via computeAllowToolsInChannel / isAutomatedInvoker.
Identity checks preserved: HandleToolResult validates requesterID == userID (line 328-329). In the auto-share path, AutoExecuteApprovedToolCalls passes the same requesterID (extracted from the server-set LLMRequesterUserID prop) to both HandleToolCall and HandleToolResult. The authorization chain is intact.
No tool double-execution: Tools pre-executed by wrapStreamWithMCPAutoApproval have status Success. When HandleToolCall processes them (line 196-199), it skips tools with non-pending/non-accepted status, preventing re-execution. The auto-share flow only handles the UI/sharing lifecycle stages.
Mixed batch safety: allToolResultsRequireReview returns true (requires review) if ANY tool in the batch is not auto_run_everywhere. This is the safe direction — a single non-everywhere tool forces the entire batch through result-sharing review.
Defense-in-depth gates intact: EnableChannelMentionToolCalling config check, allowToolsInChannel post prop check, and requester identity checks in both HandleToolCall and HandleToolResult are all preserved in the auto-share flow.
Admin-only configuration: The auto_run_everywhere policy can only be set through the system admin console. The UI labels clearly distinguish "Auto Run (DM)" from "Auto Run (Everywhere)".
Execution ordering: handleAutoApprovedToolCalls calls UpdatePost (persisting AutoShareToolResultProp) before launching the async autoExecuteCallback goroutine. The goroutine's GetPost call will observe the updated props, avoiding a TOCTOU race.
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/tool-config/mock-api/tool-call-policies.spec.ts` around lines 386 -
388, Replace the flaky fixed 2s sleep after page.goto by waiting for a concrete
condition: remove the page.waitForTimeout(2000) and instead wait for a reliable
element or network state (e.g., wait for the channel header or a
channel-specific element to be visible using
page.locator('#channelHeaderTitle').getByText('Off-Topic').waitFor({state:
'visible'}) or use Playwright's network idle/navigation wait options) so the
test proceeds only after the page is fully ready.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests/tool-config/mock-api/tool-call-policies.spec.ts`:
- Around line 386-388: Replace the flaky fixed 2s sleep after page.goto by
waiting for a concrete condition: remove the page.waitForTimeout(2000) and
instead wait for a reliable element or network state (e.g., wait for the channel
header or a channel-specific element to be visible using
page.locator('#channelHeaderTitle').getByText('Off-Topic').waitFor({state:
'visible'}) or use Playwright's network idle/navigation wait options) so the
test proceeds only after the page is fully ready.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review: Add Auto Run (Everywhere) tool approval policy
No high-confidence vulnerabilities found.
What was reviewed
This PR introduces a new admin-configured tool policy auto_run_everywhere that, unlike the existing auto_run (DM-only), bypasses both the call-approval and result-sharing approval steps in channels. The review traced all data flows through the new auto-share path.
Analysis of key security paths
Post prop trust (AutoShareToolResultProp): The auto_share_tool_result prop is set exclusively by server-side code (handleAutoApprovedToolCalls and StreamToPost), gated by anyToolResultRequiresReview which performs server-side policy lookups. Clients cannot inject this prop through the Mattermost API. Webhook/bot-originated posts are already blocked from triggering the tool-calling flow.
Identity checks preserved: HandleToolResult validates requesterID == userID (line 328). In the auto-share path, AutoExecuteApprovedToolCalls passes the same requesterID (from the server-set LLMRequesterUserID prop) to both HandleToolCall and HandleToolResult. Since HandleToolCall receives the post pointer and sets PendingToolResultProp on it, the subsequent HandleToolResult call observes the guard prop and proceeds correctly.
No tool double-execution: Pre-executed tools have Success status in KV. HandleToolCall's loop (line 196) skips non-pending/non-accepted statuses, preventing re-execution in the auto-share path.
Mixed batch safety: anyToolResultRequiresReview returns true if ANY tool in the batch is not auto_run_everywhere. A single non-everywhere tool forces the entire batch through manual result-sharing review (safe direction).
Defense-in-depth gates intact: EnableChannelMentionToolCalling config check, allowToolsInChannel post prop check, and requester identity verification are all preserved across the auto-share flow in both HandleToolCall and HandleToolResult.
TOCTOU safety: handleAutoApprovedToolCalls persists AutoShareToolResultProp via UpdatePost and returns on failure (line 421-423) before launching the async autoExecuteCallback goroutine. The goroutine's GetPost will observe the committed state. The autoShareResults flag in AutoExecuteApprovedToolCalls is read before HandleToolCall clears the prop, ensuring correct flow control.
Admin-only configuration: The auto_run_everywhere policy can only be set through the system admin console, with clear UI labeling ("Auto Run (DM)" vs "Auto Run (Everywhere)").
63-66: Loosen exact post-text matching to reduce mention-rendering flake risk.
waitForSentPost currently requires an exact full-string match (@bot message). Mention rendering can split/format text differently, which can intermittently miss the post. Prefer matching the unique message body (or a regex) and keep .last() for recency.
Suggested adjustment
async function waitForSentPost(page: Page, message: string, timeout: number = 30000): Promise<Locator> {
const post = page.locator('.post').filter({
- has: page.locator('.post-message__text').getByText(message, {exact: true}),+ has: page.locator('.post-message__text').getByText(message, {exact: false}),
}).last();
await expect(post).toBeVisible({timeout});
return post;
}
@@
- const post = await waitForSentPost(page, `@${botName} ${message}`, timeout);+ const post = await waitForSentPost(page, message, timeout);
Also applies to: 82-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/tool-config/mock-api/tool-call-policies.spec.ts` around lines 63 -
66, The locator in waitForSentPost is using exact full-string matching which
flakes when mentions render differently; change the `.getByText(message, {
exact: true })` usage in waitForSentPost to a looser match (either remove the
exact flag to allow substring matching or use a regex built from the message)
while keeping `.last()` for recency; apply the same relaxation to the other
occurrence around the referenced lines (the second waitForSentPost/getByText
usage) so both checks tolerate mention formatting differences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests/tool-config/mock-api/tool-call-policies.spec.ts`:
- Around line 63-66: The locator in waitForSentPost is using exact full-string
matching which flakes when mentions render differently; change the
`.getByText(message, { exact: true })` usage in waitForSentPost to a looser
match (either remove the exact flag to allow substring matching or use a regex
built from the message) while keeping `.last()` for recency; apply the same
relaxation to the other occurrence around the referenced lines (the second
waitForSentPost/getByText usage) so both checks tolerate mention formatting
differences.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review: Add Auto Run (Everywhere) tool approval policy
No high-confidence vulnerabilities found.
What was reviewed
This PR introduces a new admin-configurable auto_run_everywhere tool approval policy that extends the existing auto_run (DM-only) policy. When configured, it bypasses both tool-call approval and result-sharing approval in channels, allowing MCP tools to run to completion without user interaction.
Security analysis
Post prop integrity (AutoShareToolResultProp): The new AutoShareToolResultProp is set exclusively by server-side streaming code (streaming.go:handleAutoApprovedToolCalls, StreamToPost), never from user input. It is defensively cleared (DelProp) in all HandleToolCall and HandleToolResult code paths, preventing stale or injected values from persisting.
Requester identity enforcement preserved: The auto-share flow in AutoExecuteApprovedToolCalls passes the requesterID extracted from the post's LLMRequesterUserID prop (server-set) to both HandleToolCall and HandleToolResult. Both functions validate requesterID == userID before proceeding. The HTTP API endpoint (handleToolResult) continues to authenticate via Mattermost-User-Id header.
Tool execution scoping: Tools still execute within the requesting user's permission context via BuildLLMContextUserRequest. The auto_run_everywhere policy only changes the approval flow, not the permission scope of tool execution.
Channel defense-in-depth checks intact: EnableChannelMentionToolCalling() config check, allowToolsInChannel post prop check, and license checks remain enforced in all channel paths.
Admin-only configuration: The auto_run_everywhere policy is set via system console (MCPToolConfig), accessible only to system admins. The UI clearly labels it "Auto Run (Everywhere)" distinguishing it from the DM-scoped "Auto Run (DM)". Per review guidelines, admin opt-in to less restrictive modes with clear labeling is not reported as a vulnerability.
DM double-execution prevention: The refactoring to apply wrapStreamWithMCPAutoApproval only to channels (guarded by !isDM checks in both ProcessUserRequestWithContext and completeAndStreamToolResponse) correctly prevents DMs from being wrapped twice (DMs already use WithAutoRunTools).
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/tool-config/mock-api/tool-call-policies.spec.ts`:
- Around line 402-409: The test currently keys the dm-only mock off raw mention
text which makes it ambiguous; change the single
openAIMock.addCompletionMockWithRequestBody call using
buildToolCallResponse('call_channel_dm_only', ...) into the split title/main
mock pattern used by the auto_run_everywhere branch: add a title mock that
matches the assistant title-generation SSE (without the tool-only payload) and a
separate main/tool mock that matches the tool-only marker (match request body
containing "read_channel") so the main turn is unambiguously consumed by the
tool call; locate usages of openAIMock.addCompletionMockWithRequestBody and
buildToolCallResponse and replace the single ambiguous mock labeled 'tool policy
channel dm-only' with the two mocks (title + tool) that explicitly match on
"read_channel".
🪄 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: Add Auto Run (Everywhere) tool approval policy
No high-confidence vulnerabilities found.
What was reviewed
This PR introduces a new admin-configurable auto_run_everywhere tool approval policy that extends the existing auto_run (DM-only) policy to also bypass result-sharing approval in channels, allowing MCP tools to run to completion without user interaction.
Security analysis
Post prop integrity (AutoShareToolResultProp): The new auto_share_tool_result prop is set exclusively by server-side streaming code (handleAutoApprovedToolCalls at line 418, StreamToPost at line 647), gated by anyToolResultRequiresReview which performs server-side policy lookups. Clients cannot inject post props through the Mattermost REST API. The prop is defensively cleared (DelProp) in all HandleToolCall and HandleToolResult code paths (6 call sites), preventing stale or orphaned values.
Requester identity enforcement preserved: HandleToolResult validates requesterID == userID (line 328). In the auto-share path, AutoExecuteApprovedToolCalls passes the requesterID from the server-set LLMRequesterUserID prop to both HandleToolCall and HandleToolResult. The HTTP endpoint continues to authenticate via the Mattermost-User-Id header.
No tool double-execution: Pre-executed tools have Success/AutoApproved status. HandleToolCall's loop (line 196) skips tools with non-pending/non-accepted statuses. The auto-share path only handles the approval lifecycle, not re-execution.
Mixed batch safety: anyToolResultRequiresReview returns true if ANY tool in the batch is not auto_run_everywhere — a single non-everywhere tool forces the entire batch through manual result-sharing review. This is the safe direction.
Defense-in-depth gates intact: EnableChannelMentionToolCalling() config check, allowToolsInChannel post prop check, and license checks remain enforced in all channel paths for both HandleToolCall and HandleToolResult.
TOCTOU safety: handleAutoApprovedToolCalls persists AutoShareToolResultProp via UpdatePost and returns on failure (line 421-423) before launching the async autoExecuteCallback goroutine. The autoShareResults flag in AutoExecuteApprovedToolCalls is captured before HandleToolCall clears the prop, ensuring correct flow control.
DM double-execution prevention: The !isDM guards in both ProcessUserRequestWithContext and completeAndStreamToolResponse correctly prevent DMs from being wrapped with wrapStreamWithMCPAutoApproval twice (DMs already use WithAutoRunTools).
Admin-only configuration: The auto_run_everywhere policy is settable only through the system admin console, with clear UI labeling ("Auto Run (DM)" vs "Auto Run (Everywhere)").
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review: Add Auto Run (Everywhere) tool approval policy
No high-confidence vulnerabilities found.
What was reviewed
This PR introduces a new admin-configurable auto_run_everywhere tool approval policy that extends the existing auto_run (DM-only) policy to also bypass result-sharing approval in channels. I traced all data flows through the new auto-share path, focusing on authorization bypass, identity spoofing, data isolation, and TOCTOU risks.
Security analysis
Post prop integrity (AutoShareToolResultProp): The new auto_share_tool_result prop is set exclusively by server-side streaming code (handleAutoApprovedToolCalls at line 418, StreamToPost at line 647), gated by anyToolResultRequiresReview which performs server-side policy lookups against the admin-configured tool policy. Clients cannot inject post props through the Mattermost REST API. The prop is defensively cleared (DelProp) across all 6 HandleToolCall/HandleToolResult exit paths, preventing stale or orphaned values from persisting.
Requester identity enforcement preserved: HandleToolResult validates requesterID == userID (line 328). In the auto-share path, AutoExecuteApprovedToolCalls passes the requesterID extracted from the server-set LLMRequesterUserID prop to both HandleToolCall and HandleToolResult. The HTTP API endpoints continue to authenticate via the server-injected Mattermost-User-Id header.
Tool execution scoping unchanged: Tools still execute within the requesting user's permission context via BuildLLMContextUserRequest. The auto_run_everywhere policy only changes the approval flow, not the permission scope of tool execution. Tool results in the auto-share path pass through the same ResolveTool / HandleToolCall → HandleToolResult pipeline as manually approved results.
No tool double-execution: Pre-executed tools carry Success/AutoApproved status. HandleToolCall's loop (line 196) skips tools with non-pending/non-accepted statuses, preventing re-execution. The auto-share path handles only the approval lifecycle.
Mixed batch safety: anyToolResultRequiresReview returns true if ANY tool in the batch is not auto_run_everywhere — a single non-everywhere tool forces the entire batch through manual result-sharing review. This is the safe default direction.
Defense-in-depth gates intact: EnableChannelMentionToolCalling() config check, allowToolsInChannel post prop check, license checks, and requester identity verification are all enforced in both HandleToolCall and HandleToolResult channel paths.
TOCTOU safety: handleAutoApprovedToolCalls persists AutoShareToolResultProp via UpdatePost and returns on failure (lines 421–423) before launching the async autoExecuteCallback goroutine. The autoShareResults flag in AutoExecuteApprovedToolCalls (line 537) is captured from the persisted post before HandleToolCall clears the prop, ensuring correct flow control.
DM double-execution prevention: The !isDM guards added to both ProcessUserRequestWithContext and completeAndStreamToolResponse correctly prevent DMs from being wrapped with wrapStreamWithMCPAutoApproval (DMs already use WithAutoRunTools).
Admin-only configuration: The auto_run_everywhere policy is settable only through the system admin console, with clear UI labels ("Auto Run (DM)" vs "Auto Run (Everywhere)").
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review: Add Auto Run (Everywhere) tool approval policy
No high-confidence vulnerabilities found.
What was reviewed
This PR introduces a new admin-configurable auto_run_everywhere tool approval policy that extends the existing auto_run (DM-only) policy to also bypass result-sharing approval in channels. I traced all data flows through the new auto-share path, focusing on authorization bypass, identity spoofing, data isolation, and TOCTOU risks.
Security analysis
Post prop integrity (AutoShareToolResultProp): The new auto_share_tool_result prop is set exclusively by server-side streaming code (handleAutoApprovedToolCalls at line 418, StreamToPost at line 647), gated by anyToolResultRequiresReview which performs server-side policy lookups against the admin-configured tool policy. Clients cannot inject post props through the Mattermost REST API. The prop is defensively cleared (DelProp) across all 6 HandleToolCall/HandleToolResult exit paths, preventing stale or orphaned values from persisting.
Requester identity enforcement preserved: HandleToolResult validates requesterID == userID (line 328). In the auto-share path, AutoExecuteApprovedToolCalls passes the requesterID extracted from the server-set LLMRequesterUserID prop to both HandleToolCall and HandleToolResult. The HTTP API endpoints continue to authenticate via the server-injected Mattermost-User-Id header.
Tool execution scoping unchanged: Tools still execute within the requesting user's permission context via BuildLLMContextUserRequest. The auto_run_everywhere policy only changes the approval flow, not the permission scope of tool execution. Tool results in the auto-share path pass through the same ResolveTool / HandleToolCall → HandleToolResult pipeline as manually approved results.
No tool double-execution: Pre-executed tools carry Success/AutoApproved status. HandleToolCall's loop (line 196) skips tools with non-pending/non-accepted statuses, preventing re-execution. The auto-share path handles only the approval lifecycle.
Mixed batch safety: anyToolResultRequiresReview returns true if ANY tool in the batch is not auto_run_everywhere — a single non-everywhere tool forces the entire batch through manual result-sharing review. This is the safe default direction.
Defense-in-depth gates intact: EnableChannelMentionToolCalling() config check, allowToolsInChannel post prop check, license checks, and requester identity verification are all enforced in both HandleToolCall and HandleToolResult channel paths.
TOCTOU safety: handleAutoApprovedToolCalls persists AutoShareToolResultProp via UpdatePost and returns on failure (lines 421–423) before launching the async autoExecuteCallback goroutine. The autoShareResults flag in AutoExecuteApprovedToolCalls (line 537) is captured from the persisted post before HandleToolCall clears the prop, ensuring correct flow control.
DM double-execution prevention: The !isDM guards added to both ProcessUserRequestWithContext and completeAndStreamToolResponse correctly prevent DMs from being wrapped with wrapStreamWithMCPAutoApproval (DMs already use WithAutoRunTools).
Admin-only configuration: The auto_run_everywhere policy is settable only through the system admin console, with clear UI labels ("Auto Run (DM)" vs "Auto Run (Everywhere)").
The reason will be displayed to describe this comment to others. Learn more.
Security Review: Add Auto Run (Everywhere) tool approval policy
No high-confidence vulnerabilities found.
What was reviewed
This PR introduces a new auto_run_everywhere tool approval policy that extends the existing auto_run (DM-only) policy to also bypass the result-sharing approval step in channels. The following attack surfaces were traced:
Analysis
1. Channel data isolation (no issue)
When auto_run_everywhere is active, tool results are auto-shared into the channel without user review. This is by design — an admin must explicitly configure a tool with this policy via the system console. The tools still execute within the requesting user's permission scope (requesterID is validated against LLMRequesterUserID on the post in HandleToolCall), and all existing permission gates remain in place:
EnableChannelMentionToolCalling config check
allowToolsInChannel post prop check
requesterID == userID identity check
2. Post property tampering (AutoShareToolResultProp) (no issue)
The AutoShareToolResultProp is set and consumed entirely server-side (streaming layer sets it, AutoExecuteApprovedToolCalls reads it, HandleToolCall deletes it). Users cannot set properties on bot-authored posts. The prop is also always cleared in every branch of HandleToolCall and HandleToolResult, preventing stale state.
3. Mixed policy batches (no issue) anyToolResultRequiresReview() returns true if ANY tool in the batch is not auto_run_everywhere, ensuring the more restrictive policy wins. A single auto_run (DM-only) tool in the batch forces the entire batch through result review.
4. DM flow separation (no issue)
DMs now exclusively use the model-level WithAutoRunTools wrapper and skip wrapStreamWithMCPAutoApproval. The auto_run_everywhere policy has no behavioral change in DMs (was already fully automatic). The !isDM guard is correctly applied in both ProcessUserRequestWithContext and completeAndStreamToolResponse.
5. Admin-only configuration (no issue)
The new policy value is only settable through the system console admin UI. No user-facing API or LLM-controllable path can escalate a tool from auto_run to auto_run_everywhere.
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}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Summary
Adds a new per-tool
Auto Run (Everywhere)policy while renaming the existing admin console option toAuto Run (DM). The legacyauto_runvalue keeps its current behavior: DMs auto-run to completion, and channel/@mention flows auto-execute but still require result sharing. The newauto_run_everywherevalue auto-runs tools to completion in both DMs and multiplayer/channel contexts without requiring Share.Also updates backend policy evaluation, streaming/callback flow, and targeted Go/E2E coverage for the DM-vs-channel split.
Follow-up fixes on top of the initial implementation:
UpdatePostfails, so websocket/callback processing cannot proceed against stale post state@mentioncoverage exercises the real channel approval/share path in CIread_channelmarker instead of raw mention textwebapp/src/client.tsxmcp.IsToolPolicyAutoRunbehaviorstreaming.gofor clearer semanticsQA notes:
Auto Run (DM)keeps the existing two-stage channel flow (Auto-approvedbadge +Share/Keep private).Auto Run (Everywhere)skips both approval stages and continues directly after tool execution in channel/@mention flows.Auto Run (DM),Auto Run (Everywhere), andAsk Every Time.go vet, webapp type-checks, webapp lint, and webapp production build passed locally.tests/tool-config/mock-api/tool-call-policies.spec.tsnow passes locally against the current branch head.tests/system-console/debug-panel.spec.tsandtests/action-item-extraction/ui-verification.spec.ts, so the latest commit is intentionally limited to the mocked tool-policy failure that was reproducible and related to this PR.Ticket Link
NONE
Screenshots
Release Note
Summary by CodeRabbit
New Features
UI
Tests