Skip to content

MM-65671: Agents CRUD, legacy bot migration, and bot account sync#589

Open
nickmisasi wants to merge 56 commits intomasterfrom
MM-65671
Open

MM-65671: Agents CRUD, legacy bot migration, and bot account sync#589
nickmisasi wants to merge 56 commits intomasterfrom
MM-65671

Conversation

@nickmisasi
Copy link
Copy Markdown
Collaborator

@nickmisasi nickmisasi commented Apr 2, 2026

Summary

Implements user agent CRUD improvements, migration of legacy config-defined bots into user agents, and synchronization of Mattermost bot accounts when agents are updated or deleted (display name patch, bot deactivation). Updates the agents UI, system console wiring, i18n strings, and adds e2e coverage plus a small permissions helper.

image image image image image image image image image

Tools list on RHS is based on allowed tools for selected agent
image
Vs an agent with no tools enabled
image

Ticket Link

Jira https://mattermost.atlassian.net/browse/MM-65671

Release Note

NONE

Made with Cursor

Summary by CodeRabbit

  • New Features

    • Full Agents product: REST APIs and UI to create/edit/delete agents (avatar upload), access controls, service & model selection, per-agent MCP tool controls, delegated admins, license gating, Agents product page and nav integration, and RHS/tool-picker respect per-agent allowlists.
  • Migrations

    • One-time migration of legacy bots into Agents and new Agents DB table + persistence APIs.
  • Tests

    • Expanded unit, integration, and Playwright E2E coverage for agents, access control, MCP tools, provider/config flows, and migrations.

nickmisasi and others added 29 commits March 31, 2026 13:02
Detailed implementation plan for Agents_UserAgents table, Morph migration,
CRUD store methods, and integration tests. Includes exact file paths,
line numbers, code snippets, and verification commands.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 1 of self-service agent creation: Agents_UserAgents table with
Morph migration 000004, full CRUD store methods with JSON slice field
marshaling, AgentStore interface in api package, and comprehensive
integration tests (12 test functions covering round-trips, soft delete,
edge cases, and concurrency).

Also fixes setupTestStore to set search_path via connection string so
concurrent test goroutines use the correct schema, and updates migration
count assertion from 3 to 4.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implements Phase 2 of self-service agent creation:
- Agent CRUD handlers (create, list, get, update, delete) with license gating
- Bot account lifecycle (create on agent create, deactivate on delete)
- Avatar upload endpoint for agent profile images
- Services listing endpoint (secrets stripped)
- Permission-based access control (create_agent permission, creator/admin checks)
- Partial update support via pointer fields in UpdateAgentRequest
- Comprehensive test coverage (9 test functions)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implements Phase 3 of self-service agent creation:
- AgentStore interface in bots package for loading user agents from DB
- userAgentToBotConfig converter maps UserAgent to llm.BotConfig
- EnsureBots merges DB-backed agents into the live bot registry
- forceRefresh flag bypasses optimistic config-equality cache for agent CRUD
- clusterEventAgentUpdate event propagates agent changes to HA nodes
- ClusterAgentNotifier interface + refreshBotsAndNotify helper in API
- Agent create/update/delete handlers trigger registry refresh + cluster notify
- Runtime tests: converter, registry lookup, force-refresh flag
- Nil config guard in EnsureBots for test safety

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add TypeScript types for UserAgent, CreateAgentRequest, UpdateAgentRequest, ServiceInfo
- Add agent CRUD API client functions (getAgents, createAgent, updateAgent, deleteAgent, uploadAgentAvatar, getServices)
- Add Redux agents reducer for state caching
- Create AgentsPage root component with URL-based visibility at /plug/mattermost-ai/agents
- Create AgentRow component with avatar, name, tool badge, and actions menu
- Create AgentsList component with All/Your agents tabs, loading/error/empty states, and delete flow
- Create DeleteAgentDialog confirmation modal
- Create AgentsLicenseGate for E20+ license enforcement
- Register root component and main menu navigation entry point

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1. getServices() was calling /agents/services but the backend route is
   /services on the base router — the gin router would match /agents/services
   against /:agentid with agentid="services" → 404.

2. EnabledTool TS type had mismatched fields (type/id/name/serverName) vs
   Go struct (server_origin/tool_name). Aligned to match backend.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 5 implementation: three-tab modal for creating/editing agents.
- Config tab: display name, username, avatar, service selection, custom instructions
- Access tab: channel access, user access, agent admin controls
- MCPs tab: per-server and per-tool toggle switches with search
- Wire modal into agents_list.tsx for create and edit flows

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Go UserAgent struct uses snake_case JSON tags (bot_user_id,
creator_id, display_name, etc.) but the TS type used camelCase,
causing JSON deserialization to silently drop all fields.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wire UserAgent.EnabledTools into the tool discovery pipeline so
DB-backed agents only expose their selected MCP tools. Adds
EnabledMCPTool type to BotConfig, convertEnabledTools mapping,
RetainOnlyMCPTools filter on ToolStore, and the wiring call in
getToolsStoreForUser. Nil = all tools allowed (backward compat),
empty = no MCP tools, populated = allowlist.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… and MCP tools

Phase 7: Creates agent container factory, API helper, page object,
and three test spec files covering create/edit/delete flows, user
access level enforcement, and MCP tool selection.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Search input on listing page filters by display name and username
- Field-level validation errors in config modal with clear-on-edit
- Server-side 409 (username taken) mapped to inline field error
- Service deleted edge case: warning badge in agent row, fallback
  option in service dropdown
- MCP server removed edge case: orphaned tools warning banner with
  auto-cleanup on tab load

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ServiceID stores UUIDs (36 chars) but was defined as VARCHAR(26),
causing POST /agents to fail with a Postgres truncation error.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
navigateToAgentsPage() was pushing /plug/mattermost-ai/agents without
the team prefix, causing Mattermost's router to treat it as an unknown
team and redirect to "Team Not Found". Now extracts the current team
name from the URL path and builds the full team-scoped route.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use registerProduct (internal MM API) instead of registerMainMenuAction
so "Agents" appears in the product switcher grid icon rather than the
team dropdown menu. Simplify AgentsPage to a normal component — routing
is handled by registerProduct, so no URL-matching overlay is needed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reverts the registerProduct change (internal API not reliable) and
restores the original rootComponent overlay + registerMainMenuAction
approach. Fixes two issues in the original code:

1. navigateToAgentsPage() now extracts the current team name from the
   URL and navigates to /${teamName}/plug/mattermost-ai/agents
2. AgentsPage visibility check uses includes() instead of endsWith()
   to match team-prefixed URLs

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ation errors

Add pre-validation for username format (must match ^[a-z][a-z0-9._-]*$)
returning 400 for invalid usernames. Detect duplicate username errors from
bot creation and return 409 Conflict instead of 500.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add app__body and channel-view CSS classes on mount in AgentsPage,
matching what Playbooks and Boards do in their product components.
ChannelController normally sets these classes but isn't loaded in
product views, causing the header to render with a white background.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SetEnabledToolsFromJSON was treating "[]" the same as "" (empty string),
mapping both to nil. This destroyed the nil-vs-empty distinction:
- nil EnabledTools = all tools allowed (config-defined bots)
- [] EnabledTools = no tools allowed (user disabled all toggles)

When an agent was saved with no MCP tools enabled, the DB stored "[]"
but on read it was converted to nil, bypassing the RetainOnlyMCPTools
filter in llm_context.go and allowing all tools at runtime.

Fix: only map "" (empty DB column from pre-feature agents) to nil.
Let json.Unmarshal handle "[]" → empty slice and "null" → nil correctly.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Tools popover in the RHS chat header was showing ALL available MCP
tool providers regardless of the selected agent's configured tools.

Changes:
- Backend: Add EnabledMCPTools to AIBotInfo so /ai_bots exposes each
  bot's allowed MCP tools to the frontend
- Frontend: Add enabledMCPTools to LLMBot interface, pass it to
  ToolProviderPopover, and filter the server list so only servers with
  at least one enabled tool are shown
- Config-defined bots (null enabledMCPTools) continue to show all tools

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rness

- Register tests/agents/*.spec.ts in e2e-shard-3; validate ci-test-groups
- crud: duplicate username text, search no-results, regular user listing, unprivileged 403 UI
- access-control: allowlist negative, UserAccessLevel None listing, delegated admin edit
- mcp-tools: RHS tool provider cases; longer container beforeAll timeouts
- api: RHS MCP filtering + tests; ai-plugin helpers for Tools menu

Made-with: Cursor
…rules)

- MattermostPage: assert DM outcomes via bot posts (Client4) instead of thread reply UI
- access-control: use expectNoBotDmReplyFromApi / expectBotDmReplyFromApi
- openai-mock: buildChatCompletionMockRule for ordered body matchers
- mcp-tools: layered Smocker rules to tie responses to read_post in completion payload

Made-with: Cursor
- expectNoBotDmReplyFromApi: poll until observeDurationMs elapses (default 45s, matches positive reply timeout)
- Remove early return after minObserve; extend block/allowlist-negative tests to 120s

Made-with: Cursor
Ensure the positive enabled-tools flow only passes after a mocked tool-call round trip, so the suite proves runtime MCP execution instead of just mocked response text.

Made-with: Cursor
…migration

- Persist model, vision, tools, native tools, reasoning, structured output on user agents (DB, API, runtime bot mapping).
- Expose ServiceInfo fields and POST /agents/models/fetch for server-side model listing.
- Agents modal Configuration tab: full parity with legacy bot form; MCPs tab disabled when tools off.
- Structured output vs extended thinking: mutual exclusion, restore reasoning when structured off, inline note.
- Modal scroll fixes (min-height) for long config forms; service selection hint for advanced options.
- System Console: replace AI Bots editor with moved notice + link; default bot from getAIBots().
- Idempotent startup migration from config.bots to DB agents; sysadmin can manage migrated agents (empty creator).
- Tests: API/store, E2E crud and system console; skip legacy bot UI specs pending agents coverage.
- Export NativeToolsItem from bot.tsx for reuse.

Made-with: Cursor
Add agent-builder coverage for the provider-specific settings that moved out of the system console, and rerun legacy config-bot migration after later config updates so seeded bots still appear in the UI.

Made-with: Cursor
Apply fixes from CodeRabbit review: add username validation on agent
update, guard against nil config in service validation, bound avatar
upload size (10MB / 413), align SetEnabledNativeToolsFromJSON empty-slice
semantics, remove legacy bot-reasoning-config spec (coverage moved to
provider-config), add optional chaining in e2e afterAll teardown, and
improve webapp accessibility (avatar alt text, dialog ARIA attributes).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extend user agent model, API, store, and bots integration
- Migrate legacy config bots to user agents on plugin enable
- Sync Mattermost bot display name and deactivate on agent update/delete
- Update agents UI, system console config, and i18n strings
- Add permission helper and e2e agent CRUD coverage
- Fix golangci errcheck and shadow issues in agent handlers and migration

Made-with: Cursor
Remove .planning/phase-1/PLAN.md from version control (keep local copy).
.gitignore: use .planning pattern for the planning directory.

Made-with: Cursor
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 2, 2026

Note

Reviews 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 DB-backed self-service AI agents with REST endpoints, migrations, bots registry integration and cluster events, per-agent MCP tool allowlists, frontend Agents UI and client APIs, E2E test suites and helpers, and assorted plumbing and dependency updates.

Changes

Cohort / File(s) Summary
API & Handlers
api/api.go, api/api_agents.go, api/api_admin_test.go, api/api_agents_test.go
Introduces AgentStore and ClusterAgentNotifier interfaces; extends API constructor; registers authenticated, license-gated /agents and /services endpoints (CRUD, avatar, fetch models); adds extensive handler logic and tests.
Store & Migrations
store/agents.go, store/agents_test.go, store/migrations/000004_create_user_agents_table.*, store/migrations/000005_user_agent_bot_fields.*, store/store_test.go
Adds Agents_UserAgents table and migrations; implements Store CRUD with JSON-backed slice fields, soft-delete semantics, mappers, and comprehensive tests; test harness updated for schema.
Domain Models & Conversion
useragents/model.go, llm/configuration.go
Adds UserAgent and EnabledTool models with DB JSON helpers; adds llm.EnabledMCPTool and BotConfig.EnabledMCPTools (nil vs empty semantics) for per-agent MCP allowlists.
Bots Registry & Sync
bots/bots.go, bots/bots_test.go, bots/agents_test.go, bots/permissions_test.go
MMBots accepts an AgentStore; adds ForceRefreshOnNextEnsure(); EnsureBots() loads DB-backed agents, converts to bot configs (including MCP tools) and resets force-refresh; tests include failing-agent-store case and agent-backed usage restrictions.
Cluster Events & Migration
server/cluster_events.go, server/legacy_bot_migration.go, server/main.go
Adds agent_update cluster event and generic publisher; implements distributed migration of legacy config bots into UserAgents with mutex/flagging; activation runs migration → refresh → publish agent update.
LLM / MCP Tooling
llm/tools.go, llm/tools_test.go, llmcontext/llm_context.go, llmcontext/llm_context_test.go
Adds ToolStore.RetainOnlyMCPTools and integrates allowlist filtering into LLM context builder; normalizes server origins and adds tests for filtering/normalization.
Server Config API
config/config.go, server/cluster_events.go
Adds StorePersistedConfigWithoutNotify to update in-memory persisted config without notifying listeners; refactors cluster event publishing to support agent events.
Frontend: Agents UI & Client
webapp/src/components/agents/*, webapp/src/client.tsx, webapp/src/types/agents.ts, webapp/src/index.tsx, webapp/src/utils/permissions.ts, webapp/src/bots.tsx, webapp/src/i18n/en.json
Adds Agents product route/page and components (list, modal, tabs, MCPs UI, row, delete dialog, license gate), client APIs for agents/services/models, TS types, permission helper, i18n strings, and integrates enabled-MCP-tools into RHS/tool-provider UI.
System Console & Notices
webapp/src/components/system_console/*, e2e/helpers/system-console.ts
Replaces legacy inline bot editor with BotsMovedNotice; fetches runtime bots for default-bot dropdown; exports native-tools item; updates console flows to point to Agents page.
E2E Helpers & Tests
e2e/helpers/*, e2e/tests/agents/*, e2e/tests/system-console/*, e2e/scripts/ci-test-groups.mjs
Adds E2E helpers (agent API, container, page object, OpenAI mock, mm client assertions), many agent-focused Playwright suites (access control, CRUD, MCP tools, provider config), retires/skips legacy System Console suites, and updates CI shard assignments.
OpenAI Mock & Test Utilities
e2e/helpers/openai-mock.ts, e2e/helpers/agent-api.ts, e2e/helpers/agent-container.ts, e2e/helpers/agent-page.ts, e2e/helpers/ai-plugin.ts, e2e/helpers/mm.ts
New helpers for chat-completion mock rules, agent CRUD helpers, test container provisioning, Agents page object, RHS tool interactions, and API-based DM assertion utilities.
Test Harness & Call Sites
api/api_test.go, api/api_admin_test.go, conversations/*, many tests
Updated test harness and numerous tests to match changed constructor arity and added agentStore wiring (many calls adjusted with extra nil parameters).
Misc & Build
go.mod, .gitignore
Bumps Go dependencies and adds replace directives; .gitignore rule changed from excluding .planning/ to .planning.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant API
  participant Store
  participant BotRegistry
  participant Cluster

  Client->>API: POST /agents (CreateAgentRequest)
  API->>Store: CreateAgent(agent)
  Store-->>API: created agent (ID)
  API->>BotRegistry: create/update Mattermost bot account
  BotRegistry-->>API: bot_user_id
  API->>Cluster: PublishAgentUpdate()
  Cluster->>BotRegistry: ForceRefreshOnNextEnsure() & EnsureBots()
  BotRegistry-->>Cluster: EnsureBots result
  API-->>Client: 201 Created (agent)
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

Suggested labels

Setup Cloud Test Server

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch MM-65671

@nickmisasi nickmisasi requested a review from ogi-m April 7, 2026 18:30
Made-with: Cursor

# Conflicts:
#	api/api.go
#	api/api_test.go
#	bots/bots.go
#	bots/bots_test.go
#	bots/permissions_test.go
#	go.mod
#	go.sum
@mm-cloud-bot
Copy link
Copy Markdown

Test server creation failed. Review the error details here.

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Security Review — No new findings

Reviewed all new server-side code introduced by this PR: agent CRUD API (api/api_agents.go), the store layer (store/agents.go), the data model (useragents/model.go), legacy bot migration (server/legacy_bot_migration.go), bot registry integration (bots/bots.go), LLM context tool filtering (llmcontext/llm_context.go), webapp client (webapp/src/client.tsx), and configuration changes.

Areas verified — no issues found:

  • Authentication: All agent endpoints sit behind MattermostAuthorizationRequired, which checks the server-injected Mattermost-User-Id header. User identity is never taken from request body or query params.
  • Authorization: handleCreateAgent requires PermissionManageOwnAgent or PermissionManageSystem. Update/delete operations use canManageAgent() — checks creator, explicit admin list, PermissionManageOthersAgent, and PermissionManageSystem for ownerless migrated bots. handleListAgents/handleGetAgent filter through canUserAccessAgent(), which properly handles all access levels (All, Allow, Block, None) with safe defaults.
  • Secrets exposure: handleListServices projects ServiceConfig into a ServiceInfo struct that excludes APIKey, AWSSecretAccessKey, APIURL, and all other sensitive fields. handleFetchModelsForService uses admin-configured credentials server-side and only returns model name/ID pairs — no credential data reaches the response.
  • SQL injection: All database queries in store/agents.go use parameterized placeholders ($1, $2, ...) via sqlx. No string interpolation of user input into SQL.
  • Input validation: Username regex (^[a-z][a-z0-9._-]*$) validates format before bot creation. Service IDs are validated against the existing config. Username immutability is enforced on update. Invalid access level integers fall through to a safe default: return false in the access control switch.
  • MCP tool filtering: Per-agent EnabledMCPTools allowlists can only restrict the set of tools loaded from mcpToolProvider.GetToolsForUser() — they cannot grant access to tools not already available to the user. RetainOnlyMCPTools correctly preserves built-in tools (empty ServerOrigin).
  • Legacy migration: Uses cluster mutex and idempotency flag (legacy_config_bots_migrated). Migrated agents get CreatorID = "", requiring PermissionManageOthersAgent or PermissionManageSystem for management — matching pre-existing System Console access semantics.
  • License gating: All agent endpoints are gated by agentLicenseRequired middleware checking IsMultiLLMLicensed().

No medium, high, or critical vulnerabilities identified.

Open in Web View Automation 

Sent by Cursor Automation: Find vulnerabilities

Copy link
Copy Markdown
Member

@crspeller crspeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome!

The biggest issue is the whole useragents package which seems entirely unnecessary duplication.

One other issue I seen when testing is that newly created agents do not appear for the user without a refresh. We should make sure that newly created agents are available immediately.

One suggestion is that maybe we should have a manage agents option in the agents drop-down like the one for custom prompts.

Comment thread bots/agents_test.go Outdated
Comment thread useragents/model.go Outdated
Comment on lines +12 to +38
type UserAgent struct {
ID string `json:"id" db:"ID"`
BotUserID string `json:"bot_user_id" db:"BotUserID"`
CreatorID string `json:"creator_id" db:"CreatorID"`
DisplayName string `json:"display_name" db:"DisplayName"`
Username string `json:"username" db:"Username"`
ServiceID string `json:"service_id" db:"ServiceID"`
CustomInstructions string `json:"custom_instructions" db:"CustomInstructions"`
ChannelAccessLevel int `json:"channel_access_level" db:"ChannelAccessLevel"`
ChannelIDs []string `json:"channel_ids"`
UserAccessLevel int `json:"user_access_level" db:"UserAccessLevel"`
UserIDs []string `json:"user_ids"`
TeamIDs []string `json:"team_ids"`
AdminUserIDs []string `json:"admin_user_ids"`
EnabledTools []EnabledTool `json:"enabled_tools"`
Model string `json:"model"`
EnableVision bool `json:"enable_vision"`
DisableTools bool `json:"disable_tools"`
EnabledNativeTools []string `json:"enabled_native_tools"`
ReasoningEnabled bool `json:"reasoning_enabled"`
ReasoningEffort string `json:"reasoning_effort"`
ThinkingBudget int `json:"thinking_budget"`
StructuredOutputEnabled bool `json:"structured_output_enabled"`
CreateAt int64 `json:"create_at" db:"CreateAt"`
UpdateAt int64 `json:"update_at" db:"UpdateAt"`
DeleteAt int64 `json:"delete_at" db:"DeleteAt"`
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can eliminate UserAgent and use the existing structs instead? We can do some renaming to make things more clear. This package is causing a lot of duplication I think.

Some help from Claude:

UserAgent shares ~18 fields with llm.BotConfig, and at runtime it's immediately converted to BotConfig via userAgentToBotConfig() before any consumer sees it. The only fields UserAgent adds are ownership/lifecycle metadata: BotUserID, CreatorID, AdminUserIDs, CreateAt, UpdateAt, DeleteAt.

These 6 fields could be added directly to BotConfig (with omitempty — they'd be zero-valued for config-defined bots). This would eliminate:

  • This entire useragents package
  • The duplicate EnabledTool / EnabledMCPTool types (the "import cycle" comment on EnabledMCPTool is incorrect — both packages are leaves with no internal imports, verified)
  • userAgentToBotConfig() and convertEnabledTools() in bots/bots.go
  • mcpToolsToEnabled() in legacy_bot_migration.go
  • The duplicated canUserAccessAgent logic in api_agents.go (could reuse CheckUsageRestrictionsForUser from bots/permissions.go since the types would match)
  • The risk of field drift between the two types when new config fields are added

The store layer already uses its own agentRow struct for DB scanning, so the DB schema doesn't need to change. The API request/response types (CreateAgentRequest, UpdateAgentRequest) are separate DTOs and stay as-is.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've refactored this to use BotConfig

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed earlier today: the separate useragents package is gone in the current branch state, and persisted agents now use llm.BotConfig directly end-to-end. That removed the conversion helpers and field-drift risk from having two parallel models.

Comment thread api/api_agents.go
Comment on lines +48 to +70
// UpdateAgentRequest is the JSON body for PUT /agents/:agentid.
// All fields are optional — only provided fields are applied via read-modify-write.
type UpdateAgentRequest struct {
DisplayName *string `json:"display_name"`
Username *string `json:"username"`
ServiceID *string `json:"service_id"`
CustomInstructions *string `json:"custom_instructions"`
ChannelAccessLevel *int `json:"channel_access_level"`
ChannelIDs *[]string `json:"channel_ids"`
UserAccessLevel *int `json:"user_access_level"`
UserIDs *[]string `json:"user_ids"`
TeamIDs *[]string `json:"team_ids"`
AdminUserIDs *[]string `json:"admin_user_ids"`
EnabledTools *[]useragents.EnabledTool `json:"enabled_tools"`
Model *string `json:"model"`
EnableVision *bool `json:"enable_vision"`
DisableTools *bool `json:"disable_tools"`
EnabledNativeTools *[]string `json:"enabled_native_tools"`
ReasoningEnabled *bool `json:"reasoning_enabled"`
ReasoningEffort *string `json:"reasoning_effort"`
ThinkingBudget *int `json:"thinking_budget"`
StructuredOutputEnabled *bool `json:"structured_output_enabled"`
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never use the functionality for partial updates we have here. Do we need the extra complexity? Maybe just use CreateAgentRequest? (with a new name)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to be a PUT instead of PATCH

Comment thread api/api_agents.go Outdated
Comment thread api/api_agents.go Outdated
Comment on lines +210 to +250
// Build the UserAgent record (defaults match legacy System Console new bot defaults).
agent := &useragents.UserAgent{
BotUserID: mmBot.UserId,
CreatorID: userID,
DisplayName: req.DisplayName,
Username: req.Username,
ServiceID: req.ServiceID,
CustomInstructions: req.CustomInstructions,
ChannelAccessLevel: req.ChannelAccessLevel,
ChannelIDs: req.ChannelIDs,
UserAccessLevel: req.UserAccessLevel,
UserIDs: req.UserIDs,
TeamIDs: req.TeamIDs,
AdminUserIDs: req.AdminUserIDs,
EnabledTools: req.EnabledTools,
Model: req.Model,
EnableVision: true,
DisableTools: false,
ReasoningEnabled: true,
ReasoningEffort: "medium",
ThinkingBudget: req.ThinkingBudget,
StructuredOutputEnabled: false,
}
if req.EnableVision != nil {
agent.EnableVision = *req.EnableVision
}
if req.DisableTools != nil {
agent.DisableTools = *req.DisableTools
}
if req.ReasoningEnabled != nil {
agent.ReasoningEnabled = *req.ReasoningEnabled
}
if req.StructuredOutputEnabled != nil {
agent.StructuredOutputEnabled = *req.StructuredOutputEnabled
}
if req.ReasoningEffort != "" {
agent.ReasoningEffort = req.ReasoningEffort
}
if len(req.EnabledNativeTools) > 0 {
agent.EnabledNativeTools = req.EnabledNativeTools
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of logic here to set defaults and stuff. Adds extra complexity for not a lot of gain. Maybe we can just handle it like we did before where the system console deals with setting defaults. Changed defaults here: #617

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've refactored this to leverage the front-end as the defaults for now. My intention behind this originally was to allow admins in the future to configure what they want the defaults to be, but for now we can just go with simplification.

Comment thread server/cluster_events.go
p.configuration.Update(cfg)
case clusterEventAgentUpdate:
// Force the next EnsureBots to re-read DB agents, then trigger it.
p.bots.ForceRefreshOnNextEnsure()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an odd way to do this. Why not just delete the cache and let it sort itself out naturally?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ForceRefreshOnNextEnsure() doesn't actually clear the bot registry. It only nils the fast-path snapshot
(lastEnsuredBotCfgs/lastEnsuredServiceCfgs) and flips forceRefresh = true. The in-memory b.bots slice stays untouched until something calls EnsureBots(). EnsureBots() is only called from three places: plugin activation, the config-update listener, and refreshBotsAndNotify, and here.

If we went lazy with it, it would cause weirdness with an agent created on node A but talked to on node B

Comment thread llm/configuration.go Outdated
}

// If any legacy config bot still needs migration but has no Mattermost bot row,
// defer the entire migration: do not create partial agents, wipe config, or set the flag.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we should fail hard here. What are we going to do about this? When would this actually happen?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this path earlier today so a missing Mattermost bot row no longer fails the migration. We now log and soft-defer the migration so activation can retry later instead of partially migrating or erroring.

try {
let savedAgent: UserAgent;

if (mode === 'create') {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes. I think we can get rid of this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned up because of the simplification around PUT vs PATCH and front-end source of truth.

onCancel: () => void;
}

const DeleteAgentDialog = (props: Props) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should really have some standard components we can actually use. Lots of duplication from my modal for the custom prompts.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be even better if they came from core product :P

@nickmisasi nickmisasi requested a review from edgarbellot April 16, 2026 12:39
Bring in custom prompts, docs, bifrost/OAuth fixes, and related changes.
Resolve API test New() arity for customPromptsStore, merge webapp client
helpers (agents + custom prompts), and renumber custom prompts migration
to 000006 to avoid clashing with user-agents 000004/000005.

Made-with: Cursor
@mm-cloud-bot
Copy link
Copy Markdown

Test server creation failed. Review the error details here.

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Security Review — No new findings

Reviewed all new server-side code introduced by this PR: agent CRUD API (api/api_agents.go), the store layer (store/agents.go), the data model (useragents/model.go), the legacy bot migration (server/legacy_bot_migration.go), bot registry integration (bots/bots.go), LLM context and tool filtering (llmcontext/llm_context.go, llm/tools.go), cluster event handling (server/cluster_events.go), configuration additions (config/config.go, llm/configuration.go), and all frontend TypeScript additions.

Authentication: All agent CRUD endpoints sit behind MattermostAuthorizationRequired middleware which reads Mattermost-User-Id from the server-injected header. No user identity is sourced from request body or query parameters.

Authorization: Each endpoint enforces permission checks using the Mattermost permission system (PermissionManageOwnAgent, PermissionManageOthersAgent, PermissionManageSystem). Create, update, delete, and avatar upload all verify the requesting user's access. The list and get endpoints filter results via canUserAccessAgent. The creator always retains access via CreatorID check, preventing delegated admins from locking out the owner.

Secret protection: The handleListServices endpoint returns a ServiceInfo struct that explicitly excludes API keys and all other secret fields. The handleFetchModelsForService endpoint uses server-stored credentials internally but never exposes them in the response — only the model list is returned. Service URLs are admin-configured, mitigating SSRF.

SQL injection: All database queries in store/agents.go use parameterized queries ($1, $2, ...). No string concatenation of user input into SQL.

Input validation: Username format is validated against a strict regex (^[a-z][a-z0-9._-]*$). Service IDs are validated against the configured service list before use. Avatar uploads enforce a 10MB size limit.

MCP tool filtering: Per-agent MCP tool allowlists are enforced server-side via RetainOnlyMCPTools, which runs after admin policy filtering and before per-user filtering. The nil-vs-empty semantics are correctly preserved (nil = all tools, empty = no tools).

License gating: All agent endpoints are gated behind agentLicenseRequired middleware checking for E20+ license.

XSS: No dangerouslySetInnerHTML usage in any of the new React components.

No medium, high, or critical severity vulnerabilities identified in this change.

Open in Web View Automation 

Sent by Cursor Automation: Find vulnerabilities

…tool types, shared confirm dialog

Made-with: Cursor
@mm-cloud-bot
Copy link
Copy Markdown

Test server creation failed. Review the error details here.

Keep the self-serve agent flow aligned with the merged master changes by making MCP always available, preserving OpenAI responses defaults across the UI and API, and covering the updated behavior in tests.

Made-with: Cursor
@mm-cloud-bot
Copy link
Copy Markdown

Test server creation failed. Review the error details here.

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Security Review — No new findings

Reviewed all added/modified code in this PR with focus on the new agent CRUD API, service listing, model fetching, legacy migration, per-agent MCP tool filtering, store layer, and webapp client.

Areas verified:

  • AuthN/AuthZ: All agent endpoints authenticate via server-injected Mattermost-User-Id header and enforce granular permissions (ManageOwnAgent, ManageOthersAgent, ManageSystem). Creator/admin delegation model is properly checked in canManageAgent().
  • Secrets protection: GET /services returns a ServiceInfo projection that strips API keys, AWS secrets, and other credentials. POST /agents/models/fetch uses server-stored credentials and never exposes them to clients.
  • SQL injection: All store queries use parameterized statements.
  • Access control: canUserAccessAgent() properly filters agent visibility based on UserAccessLevel, UserIDs, and TeamIDs. handleListAgents applies this filter before returning results.
  • MCP tool scoping: Per-agent EnabledMCPTools allowlist correctly flows through userAgentToBotConfigBotConfig.EnabledMCPToolsRetainOnlyMCPTools() in llmcontext.go.
  • SSRF: handleFetchModelsForService only uses admin-configured service URLs from stored config, not user-supplied values.
  • Input validation: Username regex, service ID existence checks, 10MB avatar size limit with io.LimitReader.
  • License gating: All agent routes behind agentLicenseRequired middleware.

No medium, high, or critical vulnerabilities identified.

Open in Web View Automation 

Sent by Cursor Automation: Find vulnerabilities

Made-with: Cursor

# Conflicts:
#	api/api_admin.go
#	bifrost/config.go
#	e2e/tests/bot-configuration/reasoning-config.spec.ts
#	mcp/client_manager_filter_test.go
#	server/main.go
#	webapp/src/components/rhs/tool_provider_popover.tsx
#	webapp/src/components/system_console/bot.tsx
#	webapp/src/components/system_console/mcp_servers.tsx
#	webapp/src/components/system_console/reasoning_config.tsx
#	webapp/src/components/system_console/service.tsx
@mm-cloud-bot
Copy link
Copy Markdown

Test server creation failed. Review the error details here.

Keep the new default web search behavior intact by making the MCP-focused e2e agents explicitly disable native tools, and align bifrost expectations with the current empty-slice filtering behavior.

Made-with: Cursor
@mm-cloud-bot
Copy link
Copy Markdown

Test server creation failed. Review the error details here.

Deduplicate agent service validation, add a Manage agents action, and remove low-signal bot tests.

Made-with: Cursor
@mm-cloud-bot
Copy link
Copy Markdown

Test server creation failed. Review the error details here.

@mm-cloud-bot
Copy link
Copy Markdown

Test server creation failed. Review the error details here.

…ull-object create/update payloads.

- Remove the unshipped SelfServiceAgentDefaults config and its create-time
  default injection so the create modal owns all defaults.
- Convert CreateAgentRequest / UpdateAgentRequest to full-object shapes;
  update is now a full replacement rather than a pointer-based patch.
- Make enabledMCPTools a required tri-state field on both endpoints
  (null = all, [] = none, [items] = allowlist; omission is rejected).
- Drop defaultControlledFieldsTouched plumbing from the create modal so
  both create and update send the full draft on every save.
- Update tests and e2e helpers for the new contract; e2e updateAgent
  helper fetches + merges to preserve partial-override ergonomics.

Made-with: Cursor
@mm-cloud-bot
Copy link
Copy Markdown

Test server creation failed. Review the error details here.

@edgarbellot edgarbellot added the 3: Security Review Review requested from Security Team label Apr 17, 2026
@nickmisasi nickmisasi requested a review from crspeller April 17, 2026 12:52
Export mergeAgentIntoUpdate and use GET+merge+PUT for migrated bots so
partial displayName/service updates match the agents API after the
pointer-patch removal.

Made-with: Cursor
@mm-cloud-bot
Copy link
Copy Markdown

Test server creation failed. Review the error details here.

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Review — No Findings

I performed a thorough security review of the Agents CRUD, legacy bot migration, and bot account sync changes across all modified files.

Areas Reviewed

  • Authentication: All new endpoints (/agents, /services, /agents/models/fetch) use the server-injected Mattermost-User-Id header. No identity spoofing vectors found.
  • Authorization: Proper permission gates on all CRUD operations (canCreateAgent, canManageAgent, canUserAccessAgent, canConfigureAgentServices). License gating via agentLicenseRequired middleware. Delegated admin model (AdminUserIDs, IsAdmin) correctly handled, including legacy migrated bots with empty CreatorID.
  • Secrets exposure: handleListServices returns ServiceInfo without API keys or credentials. handleFetchModelsForService uses admin-configured credentials server-side; only model names are returned. Response bodies do not leak service secrets.
  • SQL injection: All queries in store/agents.go use parameterized statements ($1, $2, ...). No string interpolation in queries.
  • Tool/MCP enforcement: Per-agent EnabledMCPTools allowlist is enforced at tool-availability time via RetainOnlyMCPTools in llmcontext/llm_context.go, before any tool execution.
  • SSRF: handleFetchModelsForService validates serviceID against admin-configured services only; the outbound URL is not user-controlled.
  • Data isolation: Agent listing is filtered per-user via canUserAccessAgent which delegates to CheckUsageRestrictionsForUserConfig. Access levels (allow/block/none) are respected.
  • Input validation: Username format validation via regex, enabledMCPTools tri-state contract enforcement, service ID existence checks, and file size limits on avatar uploads.

No medium, high, or critical vulnerabilities identified in the added or modified code.

Open in Web View Automation 

Sent by Cursor Automation: Find vulnerabilities

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2: Dev Review Requires review by a developer 3: Security Review Review requested from Security Team Setup Cloud Test Server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants