Merge master into bridge-service-independence

.gitignore

@@ -50,3 +50,7 @@ specs/
 opencode.json
 
 .planning
+
+# Locally built binaries
+cmd/evalviewer/evalviewer
+.pat

.golangci.yml

@@ -38,7 +38,13 @@ linters:
       - linters:
           - bodyclose
           - scopelint
+          - gosec
         path: _test\.go
+      # Local dev tooling: operator-supplied paths/URLs/log values are by design.
+      - linters:
+          - gosec
+        path: ^build/
+        text: "G70[346]"
       - linters:
           - revive
         text: unused-parameter

AGENTS.md

@@ -41,7 +41,7 @@ Most Go packages live at the **repo root**, not under `server/`.
 - `evals/`, `cmd/evalviewer/` — prompt evaluation harness and TUI.
 - `i18n/` — extracted translation strings.
 - `docs/` — user/admin docs.
-- `public/bridgeclient/` — Go package in the root `/v2` module, imported by other plugins and by the server (`github.com/mattermost/mattermost-plugin-agents/v2/public/bridgeclient`); it is not a separate module.
+- `public/bridgeclient/`, `public/mcptool/` — Go packages other plugins import (frozen public API); part of the root module, not HTTP assets.
 
 ## Conventions
 
@@ -52,6 +52,7 @@ Linters (golangci-lint, ESLint, gofmt/goimports, header check, editorconfig) alr
 - New user-facing strings must go through i18n (`make i18n-extract` picks them up).
 - Go tests must be table-driven when there is more than one case.
 - Never introduce a new test/mocking library; prefer to test against real implementations instead.
+- Test-only LLM helpers (mock stream generators, logging wrappers) live in `llm/llmtest`; never import `testing` from a production package.
 - All formatting of Mattermost entities (posts, users, channels, teams, members) for LLM consumption or tool output must go through the `format/` package. Never `fmt.Sprintf` model types inline; add a formatter to `format/` instead.
 - E2E shard maintenance: when adding a new spec that should run in CI, assign it in `e2e/scripts/ci-test-groups.mjs` in the same change. `make check-shards` validates coverage and is part of `make check`. Use the lightest `e2e-shard-*` group and balance by expected runtime, not alphabetically.
 - Test for behavior that could break due to a real bug. Before writing a test ask: "If this test fails, does it indicate a real bug in our code?" In particular, do not assert on implementation details like validation order or which error appears first.
@@ -87,7 +88,11 @@ The plugin emits OpenTelemetry traces. Agent-relevant rules:
 - `postgres/pgvector_test.go` boots its own pgvector container via `testcontainers-go` (`pgvector/pgvector:pg17`); `go test ./postgres/...` works on a fresh checkout as long as Docker is available. To run against an existing pgvector instance for fast iteration, set `PGVECTOR_TEST_DSN`.
 - Plugin config is migrated to the plugin DB on activation. For automation, read/write `GET`/`PUT /plugins/mattermost-ai/admin/config` rather than patching the Mattermost server config.
 - The embedded MCP server requires `SiteURL` to be set on the Mattermost server, and uses in-memory transport (no HTTP). On tool name collisions across MCP servers, first-registered wins; later duplicates are skipped with a warning.
-- `public/bridgeclient/` is a Go package of the root `/v2` module (there is no `public/go.mod`), not HTTP assets; `HAS_PUBLIC` is intentionally cleared in the Makefile. Changes there are covered by the root-module lint/test gates.
+- `public/bridgeclient/` and `public/mcptool/` are consumed by other plugins — treat their exported API as frozen. They are packages of the root module (no own `go.mod`), not HTTP assets; `HAS_PUBLIC` is intentionally cleared in the Makefile.
+- A fresh checkout does not compile: run `make apply` first to generate `server/manifest.go` (`undefined: manifest` errors otherwise).
+- The repo has three Go modules: the root, `loadtest/controller/`, and `cmd/evalviewer/`. Go version bumps, `go mod tidy`, and `go fix` must be run in each.
+- `webapp/node_modules/` contains stray Go files that `./...` matches; for sweeping Go commands use `$(go list ./... | grep -v node_modules)`.
+- Bumping the Go version: update the `go` directive in all three modules AND the `FIPS_IMAGE` tag in `build/fips.mk` — set the new tag without a digest, open a PR so the `build-fips` CI job pulls it, then pin the digest CI resolves. **Check the image exists first** (tag-listing procedure in `build/fips.mk`): the `go` directive is held at 1.26.x until `cgr.dev/mattermost.com/go-msft-fips` publishes a 1.27 toolchain — do not bump past what that registry offers. If `bin/golangci-lint` starts failing everywhere with "export data version N is greater than maximum supported version", the pinned linter predates the toolchain: bump `GOLANGCI_LINT_VERSION` in the Makefile and rerun `make install-go-tools`.
 
 ## Pull requests and commits
 

Makefile

@@ -266,9 +266,9 @@ apply:
 # Pinned tool versions. Bump these here, not at the install site — keeping the
 # pins in one place lets contributors update a tool with a single edit and
 # makes Go-version-skew fixes obvious.
-GOLANGCI_LINT_VERSION    ?= v2.0.2
+GOLANGCI_LINT_VERSION    ?= v2.13.1
 GOTESTSUM_VERSION        ?= v1.7.0
-MATTERMOST_GOVET_VERSION ?= 3f08281c344327ac09364f196b15f9a81c7eff08
+MATTERMOST_GOVET_VERSION ?= 2fbfca354651528bffd39e63d7c5a2b32e6adf3e
 
 ## Install go tools.
 install-go-tools:
@@ -301,7 +301,8 @@ ifneq ($(HAS_SERVER),)
 	@echo Running golangci-lint
 	$(GO) vet ./...
 	$(GOBIN)/golangci-lint run ./...
-	$(GO) vet -vettool=$(GOBIN)/mattermost-govet -license -license.year=2023 ./...
+	# npm dependencies can contain Go packages; do not lint third-party sources.
+	$(GO) vet -vettool=$(GOBIN)/mattermost-govet -license -license.year=2023 $$($(GO) list ./... | awk '!/\/node_modules\//')
 	$(MAKE) loadtest-controller-lint
 endif
 

api/agent_acl.go

@@ -0,0 +1,123 @@
+// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package api
+
+import (
+	"cmp"
+	"reflect"
+	"slices"
+
+	"github.com/mattermost/mattermost-plugin-agents/v2/llm"
+	"github.com/mattermost/mattermost/server/public/model"
+	"github.com/mattermost/mattermost/server/public/pluginapi"
+)
+
+// canManageAgent reports whether userID may update or delete cfg: agent admin, PermissionManageOthersAgent,
+// or (agent with empty CreatorID) PermissionManageSystem for migrated legacy bots.
+func canManageAgent(client *pluginapi.Client, cfg *llm.BotConfig, userID string) bool {
+	if cfg == nil {
+		return false
+	}
+	if cfg.IsAdmin(userID) {
+		return true
+	}
+	if client.User.HasPermissionTo(userID, model.PermissionManageOthersAgent) {
+		return true
+	}
+	if cfg.CreatorID == "" && client.User.HasPermissionTo(userID, model.PermissionManageSystem) {
+		return true
+	}
+	return false
+}
+
+// canCreateAgent returns true if the user may create new agents via POST /agents.
+func canCreateAgent(client *pluginapi.Client, userID string) bool {
+	if client.User.HasPermissionTo(userID, model.PermissionManageOwnAgent) {
+		return true
+	}
+	return client.User.HasPermissionTo(userID, model.PermissionManageSystem)
+}
+
+// isSystemAdmin reports whether userID has PermissionManageSystem.
+func isSystemAdmin(client *pluginapi.Client, userID string) bool {
+	return client.User.HasPermissionTo(userID, model.PermissionManageSystem)
+}
+
+// canConfigureAgentServices reports whether userID may list services or fetch models (ManageOwnAgent, ManageOthersAgent, or ManageSystem).
+func canConfigureAgentServices(client *pluginapi.Client, userID string) bool {
+	if client.User.HasPermissionTo(userID, model.PermissionManageOwnAgent) {
+		return true
+	}
+	if client.User.HasPermissionTo(userID, model.PermissionManageOthersAgent) {
+		return true
+	}
+	return client.User.HasPermissionTo(userID, model.PermissionManageSystem)
+}
+
+// clearManagerEditableFields zeroes the fields any agent manager may change while
+// service account auth stays on. Every other field is sensitive by default: the
+// proposed config starts as a copy of the stored one, so a field added to
+// applyAgentUpdateRequest later is admin-only until it is listed here.
+func clearManagerEditableFields(cfg *llm.BotConfig) {
+	cfg.DisplayName = ""
+	cfg.CustomInstructions = ""
+	cfg.Model = ""
+	cfg.ServiceID = ""
+	cfg.EnableVision = false
+	cfg.DisableTools = false
+	cfg.EnabledNativeTools = nil
+	cfg.MCPDynamicToolLoading = false
+	cfg.ReasoningEnabled = false
+	cfg.ReasoningEffort = ""
+	cfg.ThinkingBudget = 0
+	cfg.StructuredOutputEnabled = false //nolint:staticcheck // deprecated but still accepted on the wire
+	cfg.MaxToolTurns = 0
+	cfg.UseServiceAccountAuth = false
+
+	// Access/MCP-grant ID collections are sets on the wire; order and nil-vs-empty are not changes.
+	cfg.ChannelIDs = sortedOrNil(cfg.ChannelIDs)
+	cfg.UserIDs = sortedOrNil(cfg.UserIDs)
+	cfg.TeamIDs = sortedOrNil(cfg.TeamIDs)
+	cfg.AdminUserIDs = sortedOrNil(cfg.AdminUserIDs)
+	cfg.EnabledMCPTools = sortedToolsOrNil(cfg.EnabledMCPTools)
+}
+
+func sortedOrNil(s []string) []string {
+	if len(s) == 0 {
+		return nil
+	}
+	out := slices.Clone(s)
+	slices.Sort(out)
+	return out
+}
+
+func sortedToolsOrNil(s []llm.EnabledMCPTool) []llm.EnabledMCPTool {
+	if len(s) == 0 {
+		return nil
+	}
+	out := slices.Clone(s)
+	slices.SortFunc(out, func(a, b llm.EnabledMCPTool) int {
+		if c := cmp.Compare(a.ServerOrigin, b.ServerOrigin); c != 0 {
+			return c
+		}
+		return cmp.Compare(a.ToolName, b.ToolName)
+	})
+	return out
+}
+
+// serviceAccountChangeNeedsAdmin reports whether moving stored to proposed requires
+// manage_system: enabling service account auth, or changing Access / MCP grants
+// while it stays on. Turning it off is always allowed. Agent managers may still
+// change the fields cleared by clearManagerEditableFields while SA stays on.
+func serviceAccountChangeNeedsAdmin(stored, proposed llm.BotConfig) bool {
+	if !proposed.UseServiceAccountAuth {
+		return false
+	}
+	if !stored.UseServiceAccountAuth {
+		return true
+	}
+	clearManagerEditableFields(&stored)
+	clearManagerEditableFields(&proposed)
+	return !reflect.DeepEqual(stored, proposed)
+}