Add ask_agent: agent-to-agent delegation via the embedded MCP server - #893
Add ask_agent: agent-to-agent delegation via the embedded MCP server#893crspeller wants to merge 33 commits into
Conversation
…k_agent at ask policy Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…turn runner Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…us reconcile endpoint Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…ty keepalive Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…claim persistence Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…t a stream Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…dentity Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…rd assignment Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…y otherwise Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
🤖 LLM Evaluation ResultsOpenAI
❌ Failed EvaluationsShow 7 failuresOPENAI1. TestReactEval/[openai]_react_cat_message
2. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[openai]_bot_dm_tool_introspection
Anthropic
❌ Failed EvaluationsShow 7 failuresANTHROPIC1. TestReactEval/[anthropic]_react_cat_message
2. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[anthropic]_bot_dm_tool_introspection
This comment was automatically generated by the eval CI pipeline. |
|
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:
📝 WalkthroughWalkthroughAdds embedded agent-to-agent delegation with ChangesDelegation contracts and MCP entry
Delegation runtime and persistence
Platform integration and web validation
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
delegation/delegation_eval_test.go (1)
188-193: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the delegated task preserves the user’s request.
Checking only
NotEmptyallows regressions such as delegating"handle this"while the canned resolver and response rubric still pass. Add expected task terms per case and assert thatargs.Taskincludes them.🤖 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 `@delegation/delegation_eval_test.go` around lines 188 - 193, Update the wantDelegation assertions in the delegation evaluation test to define expected user-request terms for each test case and verify args.Task contains those terms, rather than only checking that it is non-empty. Keep the existing agent-target assertion and ensure the canned resolver/rubric cannot pass when the delegated task omits the request’s key terms.
🤖 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/tool_approval.go`:
- Around line 132-146: Make delegation approval claiming atomic in the flow
around persistAcceptedToolDecisions and executeResolvedToolBatch: replace the
unconditional decision persistence with a transactional or compare-and-set
update that succeeds only when the targeted blocks are still pending. Abort the
current request when the claim fails as stale, and launch the detached execution
goroutine only after a successful claim so concurrent clicks cannot execute
ask_agent twice.
In `@delegation/records.go`:
- Around line 48-62: Update saveRecord to prevent partial persistence when the
parent-tool-call KVSetWithExpiry fails: after a failed secondary write, remove
the conversation-key entry using the available KV delete operation, or use an
atomic persistence mechanism if supported. Ensure saveRecord still returns the
persistence error and both indexes remain consistent.
In `@delegation/service.go`:
- Around line 687-691: The latestSubTurnState function must propagate GetTurns
errors instead of returning subTurnStateNone; update its callers in
delegation/service.go to explicitly handle the returned error in await, status,
and orphan-completion flows. In delegation/service_test.go lines 259-262,
replace the subTurnStateNone expectation with an assertion that the turn-store
error is propagated.
- Around line 421-423: The delegation flow around saveRecord must not continue
after persistence fails. Replace the log-only handling with explicit retry logic
or fail the delegation and return/propagate the error, ensuring downstream
status lookup and orphaned-cluster completion cannot proceed without the durable
record.
- Around line 448-453: Update the asynchronous title-generation path around
GenerateTitle to accept and pass a detached, bounded context as the first
parameter, preserving cancellation independence while enforcing a timeout.
Thread this context through every entry point in the GenerateTitle call path,
and instrument the relevant methods with OpenTelemetry spans using the
repository’s telemetry helpers and attribute keys.
In `@delegation/waiters_test.go`:
- Around line 12-68: Convert TestWaiterRegistry into a table-driven test using a
case slice with per-case setup and assertion callbacks, then iterate over the
cases with t.Run. Preserve all existing scenarios, names, registry operations,
and assertions while removing the separate handwritten subtest blocks.
In `@mcpserver/tools/agents.go`:
- Around line 71-99: Update createMCPToolContext so it always extracts BotUserID
from trusted embedded metadata, regardless of TrackAIGenerated; keep that
setting limited to post-attribution behavior. Preserve ask_agent availability
and add a regression test constructing the context with tracking disabled,
verifying toolAskAgent receives the bot identity instead of failing
authentication.
- Around line 109-110: Update the task-length validation in the relevant
delegation function to count Unicode characters with
utf8.RuneCountInString(task) instead of len(task), while preserving the existing
maximum and error message behavior.
In `@webapp/src/components/delegation/delegation_card.tsx`:
- Around line 175-182: Update formatElapsed and its usages in the delegation
card to route the elapsed-duration output through FormattedMessage or the
project’s existing localized duration formatter, including the additional
occurrences at the referenced lines. Remove hardcoded “m” and “s” user-facing
text while preserving the current minutes/seconds formatting behavior.
- Around line 203-210: Update the expandable Task rendering in the delegation
card, including the corresponding instance near the alternate task layout, so
its clickable variant uses a styled button or equivalent focusable keyboard
semantics. Preserve the existing taskNeedsClamp click behavior and
non-expandable rendering, while ensuring keyboard users can focus and activate
task expansion.
- Around line 103-105: Update the visual-state derivation around terminalPhase,
isPendingApproval, and isRejected in the delegation card so rejected calls and
local decisions take precedence over active phases and stale approval prompts.
Ensure rendering produces exactly one mutually exclusive state, then add
regression coverage for persisted rejection and local decision states.
- Around line 131-144: Update the elapsed timer effect using startedAtRef so it
adopts reconciled?.created_at whenever that timestamp becomes available, even if
the ref was initialized with Date.now(). Preserve the current fallback for
missing creation times and ensure the elapsed calculation remains based on the
reconciled start time.
---
Nitpick comments:
In `@delegation/delegation_eval_test.go`:
- Around line 188-193: Update the wantDelegation assertions in the delegation
evaluation test to define expected user-request terms for each test case and
verify args.Task contains those terms, rather than only checking that it is
non-empty. Keep the existing agent-target assertion and ensure the canned
resolver/rubric cannot pass when the delegated task omits the request’s key
terms.
🪄 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: e035102d-72fe-4c4d-8a87-6136939dc059
📒 Files selected for processing (55)
api/api.goapi/api_delegation.goconversations/conversations.goconversations/delegation_subturn.goconversations/delegation_subturn_internal_test.goconversations/handle_messages.goconversations/regeneration.goconversations/tool_approval.godelegation/delegation_eval_test.godelegation/progress.godelegation/records.godelegation/service.godelegation/service_test.godelegation/types.godelegation/waiters.godelegation/waiters_test.godocs/admin_guide.mde2e/scripts/ci-test-groups.mjse2e/tests/agent-delegation/delegation.spec.tsformat/format.goi18n/en.jsonllm/token_usage_fields.gollm/tool_call_context.gomcp/client_manager.gomcp/client_manager_test.gomcp/testhelpers_test.gomcp/user_clients.gomcp/user_clients_test.gomcp/vetted_tools.gomcp/vetted_tools_test.gomcpserver/http_server.gomcpserver/inmemory_server.gomcpserver/plugin_handlers.gomcpserver/server.gomcpserver/stdio_server.gomcpserver/tools/agents.gomcpserver/tools/agents_test.gomcpserver/tools/provider.gometrics/metrics.gometrics/noop.goprompts/delegated_task_system.tmplprompts/prompts_vars.goserver/cluster_events.goserver/embedded_mcp_server.goserver/inmemory_basic_test.goserver/main.gotoolrunner/toolrunner.gowebapp/src/client.tsxwebapp/src/components/delegation/delegation_card.test.tsxwebapp/src/components/delegation/delegation_card.tsxwebapp/src/components/tool_approval_set.tsxwebapp/src/hooks/use_delegation_updates.tswebapp/src/i18n/en.jsonwebapp/src/index.tsxwebapp/src/types/delegation.ts
… and state-error propagation Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
… eval Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…alized elapsed time Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: christopher <christopher@mattermost.com>
Co-authored-by: christopher <christopher@mattermost.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_no_tools_test.go`:
- Around line 145-148: Update mockConvServiceStore.UpdateTurnContentIfMatches in
api/api_no_tools_test.go (lines 145-148) to compare stored content with
expected, return false, nil on mismatch, and only update matching content. Apply
the same compare-before-update behavior in
channels/analysis_conversation_test.go (lines 108-114) and add losing-claim
coverage; in channels/channels_eval_test.go (lines 187-190), return false for
missing turns; and in search/search_conversation_test.go (lines 103-109),
preserve the expected-content check and add stale-claim coverage.
In `@conversations/dm_conversation_test.go`:
- Around line 153-176: Keep the fake CAS operation atomic in
UpdateTurnContentIfMatches in conversations/dm_conversation_test.go:153-176 and
conversations/loaded_state_flow_test.go:142-165 by applying updated directly
while s.mu remains held, including updating the mirrored turns slice, then
return true without calling the separately locking UpdateTurnContent helper.
🪄 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: 20892484-c7ae-463a-a90c-494f66d1b0ca
📒 Files selected for processing (23)
.github/workflows/ci.ymlapi/api_no_tools_test.gochannels/analysis_conversation_test.gochannels/channels_eval_test.goconversation/service.goconversations/delegation_subturn_internal_test.goconversations/dm_conversation_test.goconversations/loaded_state_flow_test.goconversations/tool_approval.godelegation/delegation_eval_test.godelegation/service.godelegation/service_test.godelegation/waiters_test.godocs/admin_guide.mde2e/helpers/mmcontainer.tse2e/tests/agent-delegation/delegation.spec.tsplugin.jsonsearch/search_conversation_test.gostore/turns.gostore/turns_test.gowebapp/src/components/delegation/delegation_card.test.tsxwebapp/src/components/delegation/delegation_card.tsxwebapp/src/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (9)
- delegation/waiters_test.go
- delegation/delegation_eval_test.go
- docs/admin_guide.md
- webapp/src/i18n/en.json
- delegation/service_test.go
- e2e/tests/agent-delegation/delegation.spec.ts
- webapp/src/components/delegation/delegation_card.tsx
- conversations/tool_approval.go
- delegation/service.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2dd7abfcb
ℹ️ 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".
Co-authored-by: christopher <christopher@mattermost.com>
Co-authored-by: christopher <christopher@mattermost.com>
Co-authored-by: christopher <christopher@mattermost.com>
Co-authored-by: christopher <christopher@mattermost.com>
Co-authored-by: christopher <christopher@mattermost.com>
Co-authored-by: christopher <christopher@mattermost.com>
Co-authored-by: christopher <christopher@mattermost.com>
Co-authored-by: christopher <christopher@mattermost.com>
Co-authored-by: christopher <christopher@mattermost.com>
Co-authored-by: christopher <christopher@mattermost.com>
|
This pull request introduces a high-severity risk of indirect prompt injection by returning unvalidated LLM output directly to the delegating agent. The
🟠 Indirect Prompt Injection via Unvalidated LLM Output in
|
| Vulnerability | Indirect Prompt Injection via Unvalidated LLM Output |
|---|---|
| Description | The DelegationResult function formats the output of a delegated agent by prepending attribution text and then appending the raw answer string from the target agent. This formatted string is then returned to the delegating agent as a tool result. If the target agent's answer contains malicious instructions (prompt injection), these instructions are directly injected into the delegating agent's context as part of the tool result, potentially allowing the target agent to manipulate the delegating agent's behavior. |
mattermost-plugin-agents/format/format.go
Lines 53 to 56 in 45367d7
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
Summary
Adds an
ask_agenttool to the embedded MCP server that lets an agent delegate a self-contained task to any agent the user can access, including itself ("one pane of glass"). The delegating agent calls it like any other embedded tool; admin per-tool policy (seeded atask), per-agent MCP enablement, dynamic tool loading, and user-level disablement all apply automatically.How it works
delegation/package: validate (target exists,CheckUsageRestrictionsForUser) → surface (labeled task post +Operation="delegation"conversation in the initiator's DM with the target agent) → run (normal DM conversation machinery: target agent's persona + a delegated-task preamble, tools built for the initiator,ask_agentexcluded from sub-turns on every path incl. approval resume and regen) → await → return (attributed answer + permalink as the tool result).silent_notificationdelivery. The thread remains visible and streams normally without incrementing the initiator's DM unread or mention counts or producing desktop, push, or email notifications.prepareToolCallMetadata), keyed per call via a context-stamped tool-call ID.delegation_completeplugin cluster event for cross-node approvals, re-reading conversation state from the DB on wake.HandleToolCallpersists the full decision batch first (which closes duplicate-click double execution), returns immediately, and runs the batch on a detached goroutine. Delegated sub-turn approvals also claim their decision batch before synchronous execution because they can be displayed on two surfaces. ATouchUserActivitykeepalive prevents the 30-minute MCP idle sweep from severing long-running delegations.ask_agent— target agent, task, live phase (starting→running("Using search_posts…") →waiting_on_you→completed/failed/timed_out), inline approval cards, elapsed time, "View conversation" permalink, and a collapsed answer preview. Driven by a newdelegation_updatewebsocket event (initiator-scoped) and reconciled on reload fromGET /plugins/mattermost-ai/delegations/{parent_tool_call_id}(initiator-only, 404 otherwise).EnableChannelMentionToolCallinggate; the delegation thread still lives in the initiator's DM; the sub-agent's answer stays an initiator-private tool result until explicitly shared (existing share/redaction machinery).llm.OperationDelegation(sub-turn tokens meter under the target agent's service), adelegate to agentspan, delegation counters + duration histogram, and a per-delegation log line.Testing
silent_notification, inline delegated approval rendering/submission, and delegation card state.GOEVALS=1 LLM_PROVIDER=anthropic): the agent delegates when another agent is better suited and answers directly otherwise — 4/4 passing.tests/agent-delegation/delegation.spec.ts(aimock, assigned toe2e-shard-1): DM happy path, nested approval directly from the parent thread, channel share gate, and an assertion that delegation leaves the target-agent DM's unread and mention counts unchanged.waiting_on_youphase.Demo videos (real Anthropic agents)
DM happy path — pending card → Accept → live phases → completed with answer preview → synthesis → delegation thread in the DM:
delegation-dm-happy-path.webm
Earlier in-thread approval flow (still supported) — sub-agent needs an
ask-policy tool (create_post), parent card flips to "Waiting on you", survives a page reload, in-thread approval resumes the sub-turn and the parent completes; the sub-agent's post lands in Town Square as the initiator:delegation-nested-approval-reload.webm
Channel flow — delegation from a channel mention; an onlooker sees no approval buttons and no unshared answer content; after Share the synthesis streams channel-visible:
delegation-channel-share-initiator.webm · delegation-channel-share-onlooker.webm
Screenshots
Release Note
Summary by CodeRabbit
ask_agenttool, including capped delegated tool rounds and execution in a visible DM thread.