Skip to content

Add server audit logging for all state-changing plugin operations - #934

Open
crspeller wants to merge 20 commits into
masterfrom
cursor/bc-d4f37040-f5bf-4916-9f1e-43113be00133-455d
Open

Add server audit logging for all state-changing plugin operations#934
crspeller wants to merge 20 commits into
masterfrom
cursor/bc-d4f37040-f5bf-4916-9f1e-43113be00133-455d

Conversation

@crspeller

@crspeller crspeller commented Jul 28, 2026

Copy link
Copy Markdown
Member

Summary

Every state-changing operation in the plugin now emits a server audit record identifying who did it, what they did, and which object it touched — 22 events total (21 HTTP routes + the non-HTTP MCP session grant). Previously the plugin emitted none. This brings the plugin to parity with Playbooks (playbooks#2072) using the plugin audit API introduced in mattermost#31204.

Architecture — mirrors the plugin's existing instrumentation seams:

  • api/audit_middleware.go: a gin middleware (stacked alongside otelgin/ginlogger/metricsMiddleware) creates a record for every route registered in the audit event registry, and emits it exactly once on every exit path — success, handler errors, auth denials (401/403 still produce fail records), and panics (defer + re-panic for gin Recovery). Success is status < 400 (the OAuth start 302 counts). Records carry the actor (user/session/IP/client from plugin.Context), the request path (query strings excluded — they carry OAuth code/state), the outcome, and the OpenTelemetry trace_id so an auditor can pivot straight to the request trace.
  • api/audit_events.go: all event constants and the route registry keyed on c.HandlerName() — the same per-route identifier the metrics middleware uses. Auditing is explicit per-route opt-in, never inferred from the HTTP method (two of the most security-relevant operations are mutating GETs).
  • audit/ package: carries the record through context.Context so handlers and services enrich it like they add attributes to the ambient otel span (auditRec(c) / audit.RecordFromContext(ctx) + nil-safe audit.AddParam). Object-identifier parameter keys reuse the telemetry/attributes.go strings (agents.post.id, agents.channel.id, …) so audit records and traces share one vocabulary.
  • Dependency bump: server/public → v0.4.3 for pluginapi.Client.Audit (the pinned replace directive was dropped per its own "drop when tagged" comment). One-line test fallout: CreateUserAccessToken gained an expiresAt param.

Audited events (enrichment in parentheses; all failures and permission denials also produce records):

Cluster Events
Config saveConfig (changed top-level key names only + persisted flag — never values)
Admin ops reindexPosts (clear_index), cancelReindexJob/catchUpReindex (job_status), clearMCPToolsCache (cleared_servers), updateMCPPluginServer (target plugin, effective enabled, tool_configs_changed)
Agent CRUD createAgent/updateAgent/deleteAgent/updateAgentAvatar (agent/bot IDs, name, changed_fields as field-name list — never custom instructions)
Custom prompts createCustomPrompt/updateCustomPrompt/deleteCustomPrompt (prompt_id, is_shared — never title/template)
Credentials mcpOAuthStart/mcpOAuthCallback/mcpOAuthDisconnect (server name; provider error clamped to the RFC 6749 enum — never code/state/descriptions/auth URLs), updateMCPUserPreferences (disabled servers intersected with known server names + full count)
MCP registration registerMCPPluginServer/unregisterMCPPluginServer (agents.caller_plugin.id from the trusted inter-plugin header; length-clamped name/path)
Tool approval toolCallApproval/toolResultApproval (approver, post/channel/agent IDs, accepted/rejected tool names — never arguments, results, or user answers)
MCP session grant mcpSessionGrant — an external MCP client obtained a session holding API access as the user. Emitted on mint, on re-enabling a lapsed session, and on first external adoption of an internally-minted session (per-session KV marker); reconnects are silent.

Content policy (enforced by tests): no prompt/template content, conversation or channel content, tool arguments/results, tokens, OAuth code/state, config values, session IDs, or free-form error text ever enter a record. Fail records carry the HTTP status code (plus a static class marker for panics and session-grant failures); full error detail stays in the server log, correlated by timestamp, actor, or trace_id — so handler error messages remain rich for debugging without becoming an audit leakage channel. Remaining free-text injection channels are closed structurally: user preference entries are intersected with known server names, OAuth error codes are clamped to the spec enum, and pre-validation identifiers are length-clamped. Tests plant sentinel secrets/content through the real code paths and assert absence over the whole marshaled record.

There is no plugin-side flag: persistence is governed entirely by the server's audit configuration (ExperimentalAuditSettings). Docs added to docs/admin_guide.md.

QA (live, terminal-driven) — Mattermost enterprise master + this plugin, MM_EXPERIMENTALAUDITSETTINGS_FILEENABLED=true:

  • 26 requests across all clusters produced records with correct event names, statuses, actors (including the empty-actor 401 case), and parameters; the server stamps plugin_id: mattermost-ai into each; read-only GETs emit nothing.
  • All 10 planted sentinels (API key, custom instructions, prompt title/template, OAuth state/code/description, injected error text, fake server name) verified absent from the emitted audit log.
  • mcpSessionGrant emitted exactly once across two consecutive external MCP connects.
  • Full live tool-approval flow with a real Anthropic model: a pending create_channel tool call approved via the API produced a toolCallApproval success record (accepted_tools: ["mattermost__create_channel"], approver/post/channel/agent IDs, trace_id) that joins with the server's own createChannel record — the previously-missing link between the human decision and its effect:
{"event_name":"toolCallApproval","status":"success",
 "actor":{"user_id":"nrf3dug1ybbczj561uoq7cxwue","session_id":"...","ip_address":"172.18.0.1"},
 "meta":{"api_path":"/post/ocj4rsw4y3dwbkmpiwp5thzk4c/tool_call","trace_id":"0d78fc81e6930a123dbaaa171ec41fe7"},
 "event":{"parameters":{"plugin_id":"mattermost-ai",
   "accepted_tools":["mattermost__create_channel"],
   "accepted_tool_ids":["toolu_011CmGeoXv2ZSqj1qf5PBLVA"],
   "agents.agent.id":"s9a8x1qfq3rqbxpnw5dfb6sofe",
   "agents.channel.id":"35paqyhuajgbfdo7qqcmqc91sr",
   "agents.post.id":"ocj4rsw4y3dwbkmpiwp5thzk4c"}}}

Not live-verified (covered by unit tests): bridge register/unregister (requires a second plugin as caller), the session-grant adoption path, and the tool-result share flow.

Known cosmetic artifact: empty []string parameters (e.g. rejected_tools when nothing was rejected) arrive as JSON null in the persisted log — the plugin RPC gob round-trip collapses empty slices; key presence is preserved. This affects any plugin using the audit API.

About the failing security/snyk check (non-required): the findings are pre-existing on master and surface here only because this PR touches go.mod (Snyk reports "no manifest changes detected" on other PRs and skips scanning). This PR already patches every fixable Go-module advisory govulncheck reports (grpc v1.82.1, x/text v0.39.0, quic-go v0.59.1, x/net v0.56.0, otel v1.44.0, edwards25519 v1.1.1, klauspost/compress v1.18.7 — all pre-existing on master). What remains is out of this PR's control: two Go stdlib advisories fixed only in go1.26.5 (toolchain decision), one x/crypto advisory with no released fix (GO-2026-5932, not reachable from this code per govulncheck), and ~20 pre-existing npm advisories in webapp/ (untouched by this PR).

Ticket Link

Screenshots

N/A — server-side only; audit-log evidence included above.

Release Note

Added server audit logging for all state-changing plugin operations: configuration saves, admin reindex and MCP cache operations, agent and custom prompt management, MCP OAuth credential grants and revocations, per-user MCP tool preferences, inter-plugin MCP server registration, in-channel tool call approvals, and MCP session grants for external clients. Records include the actor, outcome (including failures and permission denials), affected object identifiers, and the OpenTelemetry trace ID, and never contain prompt content, tool arguments, credentials, configuration values, or free-form error text. Persistence is governed by the server's audit logging configuration (ExperimentalAuditSettings); no plugin configuration is required. Updated mattermost/server/public to v0.4.3 and patched known-vulnerable versions of grpc, x/text, x/net, quic-go, otel, edwards25519, and klauspost/compress.
Open in Web Open in Cursor 

Summary by CodeRabbit

  • New Features

    • Added centralized, route-based server audit logging across configuration, admin operations, agents, custom prompts, MCP flows, and tool approvals.
    • Audit events now include consistent metadata (including trace correlation) and enhanced MCP session grant auditing (new vs reused) with duplicate suppression.
    • Tool approval audits record accepted vs rejected tool names.
  • Documentation

    • Expanded the admin guide with an audit logging overview and privacy safeguards.
  • Bug Fixes

    • Hardened audit and auditable error messaging to avoid including user/LLM-provided values in audit payloads, and improved safe handling of error/status details.

cursoragent and others added 13 commits July 28, 2026 13:51
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…rence case

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>
…il record

Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…coverage, persisted flag, test hardening

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>
… coverage gaps

Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…e fail-path test gaps

Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
@mm-prodsec-bot

mm-prodsec-bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Snyk checks have failed. 2 issues have been found so far.

Status Scan Engine Critical High Medium Low Total (2)
Open Source Security 0 0 2 0 2 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

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

This change adds comprehensive server-side audit logging across API operations, including centralized event registration, middleware persistence with trace propagation, endpoint-specific enrichment, sensitive-value filtering, MCP session creation tracking, and audit-focused test coverage. The audit infrastructure captures operation outcomes, identifiers, state changes, and failure modes while strictly excluding prompt content, tool arguments/results, configuration values, and authentication secrets.

Changes

Audit platform and core infrastructure

Layer / File(s) Summary
Audit event names, registry, and parameter helpers
api/audit_events.go, audit/context.go, audit/keys.go, audit/diff.go, audit/params.go
Exported audit event name constants, handler-to-event registry mapping, context propagation helpers, JSON key diffing, and parameter truncation (IDs, descriptions, lists) are defined.
Audit middleware and API integration
api/audit_middleware.go, api/api.go
Middleware creates fail-mode audit records, attaches request context, captures trace IDs, handles panics, records success/error status and descriptions, and is registered into the router.
Middleware test coverage
api/audit_middleware_test.go, audit/params_test.go, audit/diff_test.go
Tests validate middleware emission, trace ID propagation, panic recovery, registry completeness, config diffing, and audit output for success/failure/unaudited routes; truncation helpers are unit-tested.

API endpoint audit enrichment

Layer / File(s) Summary
Configuration and admin operations
api/api_config.go, api/api_admin.go, api/api_admin_test.go
Configuration saves record changed top-level keys and persistence status; admin reindex/cancel/catch-up/cache/plugin-server endpoints record operation status, affected resource identifiers, and job state transitions with comprehensive test coverage.
Agent and custom-prompt CRUD
api/api_agents.go, api/api_agents_test.go, api/api_custom_prompts.go, api/api_custom_prompts_test.go
Agent handlers record identifiers, changed field names (no values), and bot user IDs; custom prompt handlers record prompt IDs and sharing status, avoiding user-supplied content, with full audit test coverage.
MCP preferences, OAuth, and bridge registration
api/api_mcp.go, api/api_oauth.go, api/api_bridge_mcp.go, api/api_mcp_test.go, api/api_mcp_user_preferences_test.go, api/api_bridge_mcp_test.go
MCP preferences record disabled server names; OAuth handlers record server identifiers and clamped error codes while excluding provider URLs and secrets; plugin registration records caller IDs and effective configuration states.
Tool call and result approvals
api/api_post.go, api/api_post_test.go, conversations/tool_approval.go, conversations/tool_approval_audit_test.go
Tool approval handlers and conversation service record agent IDs and which tools were accepted vs rejected by decision type (user-interaction, auto-execution, default rejection), without including tool arguments or results.

MCP session tracking and grant audit deduplication

Layer / File(s) Summary
Session creation vs reuse tracking
mcp/client_manager.go, mcp/embedded_session_store.go, mcp/embedded_session_store_test.go
EnsureMCPSessionID and embedded session helpers return a boolean indicating session creation or renewal; try-reuse logic differentiates non-expired reuse (created=false) from expired-session extension (created=true).
MCP grant audit with deduplication
api/mcp_handlers.go, api/mcp_handlers_test.go
Delegation handler emits grant audit records only when sessions are newly created or no prior KV marker exists, with per-session KV markers and TTL-based suppression to prevent duplicate audit events across reconnects.

Supporting infrastructure and documentation

Layer / File(s) Summary
Test environment, validation, and dependency updates
api/api_test.go, api/api_config_test.go, mmtools/ask_user_question.go, go.mod, mcpserver/test_helpers_test.go
Mock audit record capture helpers, configuration store failure injection, value-free validation error messages, and dependency version updates are provided; test environment registers permissive audit logging expectations.
User-facing documentation
docs/admin_guide.md
Audit logging section documents server audit record generation, included metadata, excluded sensitive categories (prompts, tool content, configuration values), and server-controlled persistence.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant AuditMiddleware
  participant Handler
  participant Mattermost
  Client->>AuditMiddleware: audited API request
  AuditMiddleware->>AuditMiddleware: create fail-mode record
  AuditMiddleware->>Handler: attach record + context
  Handler->>Handler: enrich with identifiers + state
  Handler->>Mattermost: perform operation
  AuditMiddleware->>Mattermost: persist final record (status + description)
Loading

Possibly related PRs

Suggested labels: Setup Cloud Test Server

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 53.06% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding server audit logging for state-changing plugin operations.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cursor/bc-d4f37040-f5bf-4916-9f1e-43113be00133-455d

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (6)
conversations/tool_approval.go (1)

245-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Confirm the intended ordering here versus HandleToolResult.

accepted_tools/rejected_tools are added before UpdateTurnContent (Line 253) and CreateTurnAutoSequence (Line 304). If either fails, the emitted fail record still lists the decision. HandleToolResult deliberately defers the same two parameters until after its gate so a no-op click "must not claim a share resolution" (Lines 466-467). The tools here have already executed by Line 245, so recording them may well be correct — but the divergence from the sibling path is not documented. Worth a one-line comment stating that these names describe executed side effects, not persisted state.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@conversations/tool_approval.go` around lines 245 - 247, Document the
intentional ordering in the approval flow near the accepted_tools and
rejected_tools audit parameters: clarify that these names represent tools
already executed, not persisted state, so they remain recorded even if
UpdateTurnContent or CreateTurnAutoSequence later fails. Keep HandleToolResult’s
deferred recording behavior unchanged.
api/api_custom_prompts.go (1)

68-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider clamping the unvalidated prompt_id path parameter.

promptID is recorded before any validation or ownership check, so an arbitrary-length URL segment lands verbatim in the audit record. handleCreateAgent in api/api_agents.go (Line 389) applies audit.TruncateID to comparable unvalidated request text; the same treatment here would keep the record bounded. The delete handler at Line 109 has the identical pattern.

♻️ Proposed change
-	audit.AddParam(auditRec(c), "prompt_id", promptID)
+	audit.AddParam(auditRec(c), "prompt_id", audit.TruncateID(promptID))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/api_custom_prompts.go` around lines 68 - 71, Clamp the unvalidated
promptID before recording it in the audit record, reusing audit.TruncateID as
handleCreateAgent does. Apply this to both the current prompt handler and the
delete handler’s equivalent audit.AddParam call, while preserving the existing
target identification behavior.
api/api_mcp.go (1)

322-326: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

serverName is recorded unfiltered while disabled_servers two functions above is allowlisted.

The comment at Line 288 treats user-supplied server names as an "arbitrary-content injection channel into the audit log" and filters them through knownMCPServerNames. Here the same class of value — an unvalidated path parameter — is recorded verbatim, so the mitigation is inconsistent across the two routes on the same object type. Clamping with audit.TruncateID (as api/api_agents.go Line 389 does) or reusing the known-server filter would close the gap.

♻️ Proposed change
-	audit.AddParam(auditRec(c), audit.KeyMCPServer, serverName)
+	audit.AddParam(auditRec(c), audit.KeyMCPServer, audit.TruncateID(serverName))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/api_mcp.go` around lines 322 - 326, Sanitize the serverName value before
recording it in audit data within handleDeleteUserMCPOAuth. Reuse the
established knownMCPServerNames allowlist or apply audit.TruncateID, matching
the protection used by the nearby disabled_servers route, while preserving the
target server on every failure path.
api/api_admin_test.go (1)

1280-1339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make TestAuditClearMCPToolsCache table-driven.

It contains two cases written as sequential t.Run blocks, while every neighbouring audit test in this file (TestAuditReindexPosts, TestAuditCancelReindexJob, TestAuditUpdatePluginServer) uses a table. A setup func(e *TestEnvironment) plus expectedStatus/validateRecord fields would fold both cases in.

As per coding guidelines, "Go tests must be table-driven when they contain more than one case."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/api_admin_test.go` around lines 1280 - 1339, Refactor
TestAuditClearMCPToolsCache into a table-driven test with a setup func(e
*TestEnvironment), expectedStatus, and validateRecord fields. Move the existing
success and missing-cache arrangements into table entries, execute each through
a shared t.Run loop, and preserve their request setup, mock expectations,
response assertions, and audit-record validation.

Source: Coding guidelines

api/api_agents.go (1)

301-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Two copies of the same JSON-diff helper. changedAgentConfigFields and changedTopLevelConfigKeys are the same routine — marshal both sides to map[string]json.RawMessage, compare raw values with bytes.Equal, add removed keys, sort — differing only in the parameter type and the nil-prev handling. Extracting one generic helper (e.g. audit.ChangedJSONKeys(prev, next any) []string) keeps the "which fields changed, never their values" invariant defined in one place as more audited entities are added.

  • api/api_agents.go#L301-L334: replace the body of changedAgentConfigFields with a call to the shared helper, or drop it and call the helper directly at Line 550.
  • api/api_config.go#L65-L102: replace changedTopLevelConfigKeys with the shared helper, keeping the nil-prev case (pass a zero-value config.Config or let the helper treat a nil pointer as an empty map).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/api_agents.go` around lines 301 - 334, Extract the duplicated JSON
key-diff logic from changedAgentConfigFields and changedTopLevelConfigKeys into
one shared audit helper, such as audit.ChangedJSONKeys(prev, next any), that
marshals top-level values, compares raw JSON values, includes removed keys, and
returns sorted keys without exposing values. In api/api_agents.go lines 301-334,
replace changedAgentConfigFields with the shared helper or call it directly at
its use site; in api/api_config.go lines 65-102, replace
changedTopLevelConfigKeys and preserve nil-prev behavior by passing an empty
value or having the helper treat nil as an empty map.
api/audit_middleware.go (1)

30-86: 🔒 Security & Privacy | 🔵 Trivial

Solid design; one residual note on error-description content.

The middleware's success/fail/panic handling is correct (verified the panic re-raise is caught by gin.Default()'s outer Recovery(), and c.HandlerName() resolves the final route handler regardless of later aborts).

One residual gap: audit.TruncateDescription(last.Error()) (line 77) bounds length but not content — a future handler that wraps an error containing partial secrets/PII would still leak a truncated fragment into the audit log. Today this is safe only because handlers are individually disciplined to keep errors value-free (as seen in mmtools/ask_user_question.go). Since this middleware is the last line of defense for every audited route, it may be worth documenting this as an explicit contract for handler authors (or adding a lightweight sentinel/wrapper type for "audit-safe" errors) rather than relying on convention alone.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/audit_middleware.go` around lines 30 - 86, Document an explicit
audit-safety contract for errors exposed through c.Errors in auditMiddleware:
handler errors must not contain secrets, PII, or request-derived values because
TruncateDescription only limits length. Add the guidance at the relevant
handler/error-reporting boundary without changing the existing success, failure,
panic, or truncation behavior.
🤖 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_oauth.go`:
- Around line 16-22: Clamp unvalidated audit inputs in all affected handlers: in
api/api_oauth.go lines 16-22, wrap serverName in audit.TruncateID before
audit.AddParam; in api/api_post.go lines 343-344, cap data.AcceptedToolIDs and
truncate each entry before recording them; apply the identical AcceptedToolIDs
change at handleToolResult around line 407.

---

Nitpick comments:
In `@api/api_admin_test.go`:
- Around line 1280-1339: Refactor TestAuditClearMCPToolsCache into a
table-driven test with a setup func(e *TestEnvironment), expectedStatus, and
validateRecord fields. Move the existing success and missing-cache arrangements
into table entries, execute each through a shared t.Run loop, and preserve their
request setup, mock expectations, response assertions, and audit-record
validation.

In `@api/api_agents.go`:
- Around line 301-334: Extract the duplicated JSON key-diff logic from
changedAgentConfigFields and changedTopLevelConfigKeys into one shared audit
helper, such as audit.ChangedJSONKeys(prev, next any), that marshals top-level
values, compares raw JSON values, includes removed keys, and returns sorted keys
without exposing values. In api/api_agents.go lines 301-334, replace
changedAgentConfigFields with the shared helper or call it directly at its use
site; in api/api_config.go lines 65-102, replace changedTopLevelConfigKeys and
preserve nil-prev behavior by passing an empty value or having the helper treat
nil as an empty map.

In `@api/api_custom_prompts.go`:
- Around line 68-71: Clamp the unvalidated promptID before recording it in the
audit record, reusing audit.TruncateID as handleCreateAgent does. Apply this to
both the current prompt handler and the delete handler’s equivalent
audit.AddParam call, while preserving the existing target identification
behavior.

In `@api/api_mcp.go`:
- Around line 322-326: Sanitize the serverName value before recording it in
audit data within handleDeleteUserMCPOAuth. Reuse the established
knownMCPServerNames allowlist or apply audit.TruncateID, matching the protection
used by the nearby disabled_servers route, while preserving the target server on
every failure path.

In `@api/audit_middleware.go`:
- Around line 30-86: Document an explicit audit-safety contract for errors
exposed through c.Errors in auditMiddleware: handler errors must not contain
secrets, PII, or request-derived values because TruncateDescription only limits
length. Add the guidance at the relevant handler/error-reporting boundary
without changing the existing success, failure, panic, or truncation behavior.

In `@conversations/tool_approval.go`:
- Around line 245-247: Document the intentional ordering in the approval flow
near the accepted_tools and rejected_tools audit parameters: clarify that these
names represent tools already executed, not persisted state, so they remain
recorded even if UpdateTurnContent or CreateTurnAutoSequence later fails. Keep
HandleToolResult’s deferred recording behavior unchanged.
🪄 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: 9389a458-c2b3-4bdc-bbe3-1a8b95dcaa90

📥 Commits

Reviewing files that changed from the base of the PR and between 9df85e2 and 86d41d0.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (36)
  • api/api.go
  • api/api_admin.go
  • api/api_admin_test.go
  • api/api_agents.go
  • api/api_agents_test.go
  • api/api_bridge_mcp.go
  • api/api_bridge_mcp_test.go
  • api/api_config.go
  • api/api_config_test.go
  • api/api_custom_prompts.go
  • api/api_custom_prompts_test.go
  • api/api_mcp.go
  • api/api_mcp_test.go
  • api/api_mcp_user_preferences_test.go
  • api/api_oauth.go
  • api/api_post.go
  • api/api_post_test.go
  • api/api_test.go
  • api/audit_events.go
  • api/audit_middleware.go
  • api/audit_middleware_test.go
  • api/mcp_handlers.go
  • api/mcp_handlers_test.go
  • audit/context.go
  • audit/keys.go
  • audit/params.go
  • audit/params_test.go
  • conversations/tool_approval.go
  • conversations/tool_approval_audit_test.go
  • docs/admin_guide.md
  • go.mod
  • mcp/client_manager.go
  • mcp/embedded_session_store.go
  • mcp/embedded_session_store_test.go
  • mcpserver/test_helpers_test.go
  • mmtools/ask_user_question.go

Comment thread api/api_oauth.go Outdated
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
go.mod (1)

31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Align the OpenTelemetry exporter version with the other OpenTelemetry modules.

otel, sdk, and trace are now v1.44.0, but otlptracegrpc remains at v1.43.0. Confirm this is intentional; otherwise, update it to v1.44.0 and regenerate go.sum. The v1.44.0 exporter is available. (go.opentelemetry.io)

Proposed alignment
-	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.43.0
+	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.44.0
🤖 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 `@go.mod` around lines 31 - 34, Update the
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc dependency to
v1.44.0 to match the otel, sdk, and trace modules, then regenerate go.sum.
🤖 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.

Nitpick comments:
In `@go.mod`:
- Around line 31-34: Update the
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc dependency to
v1.44.0 to match the otel, sdk, and trace modules, then regenerate go.sum.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 43518d0d-9359-455c-804a-263cabf2d832

📥 Commits

Reviewing files that changed from the base of the PR and between 550418a and e8fcd97.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (1)
  • go.mod

Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown

🤖 LLM Evaluation Results

OpenAI

⚠️ Overall: 22/28 tests passed (78.6%)

Provider Total Passed Failed Pass Rate
⚠️ OPENAI 28 22 6 78.6%

❌ Failed Evaluations

Show 6 failures

OPENAI

1. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json

  • Score: 0.00
  • Rubric: is a list of bugs
  • Reason: The output does not provide an actual list of bugs; it states it cannot access the source data and offers a template for a list instead.

2. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json

  • Score: 0.00
  • Rubric: includes a description of each bug
  • Reason: The output does not include any actual bug descriptions; it states it cannot create a list without source data and only provides a template.

3. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json

  • Score: 0.00
  • Rubric: attributes each bug to a user
  • Reason: The output does not list any bugs and therefore does not attribute each bug to a specific user; it only asks for bug reports and provides a template with a 'Reported by' field left blank.

4. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json

  • Score: 0.00
  • Rubric: attributes the bug about trying to save without a color and the save button not doing anything to @maria.nunez
  • Reason: The output does not mention the specific bug (saving without a color; save button does nothing) or attribute it to @maria.nunez. It only asks for bug reports and provides a template.

5. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json

  • Score: 0.00
  • Rubric: the bug about the end user being able to change channel banner is attributed to @maria.nunez
  • Reason: The output does not mention the specific bug (end user being able to change channel banner) nor does it attribute it to @maria.nunez. It only provides a generic template and requests source data.

6. TestDirectMessageConversations/[openai]_bot_dm_tool_introspection

  • Score: 0.00
  • Rubric: mentions Github and refers to the documentation
  • Reason: The output refers to documentation (docs.mattermost.com) but does not mention GitHub. Since the rubric requires both mentioning GitHub and referring to the documentation, it fails.

Anthropic

⚠️ Overall: 20/28 tests passed (71.4%)

Provider Total Passed Failed Pass Rate
⚠️ ANTHROPIC 28 20 8 71.4%

❌ Failed Evaluations

Show 8 failures

ANTHROPIC

1. TestReactEval/[anthropic]_react_cat_message

  • Score: 0.00
  • Rubric: The word/emoji is a cat emoji or a heart/love emoji
  • Reason: The output is the text string "heart_eyes_cat", not an actual cat emoji (e.g., 😺) or heart/love emoji (e.g., ❤️).

2. TestChannelSummarization/[anthropic]_channel_summarization_developers_webapp_channel

  • Score: 0.00
  • Rubric: mentions claudio and harrison discussing exactly what should be tracked for code coverage
  • Reason: The output mentions Claudio working on code coverage tracking and Harrison raising a concern about snapshot tests inflating coverage, but it does not describe Claudio and Harrison discussing exactly what should be tracked for code coverage (i.e., specific tracked items/metrics).

3. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json

  • Score: 0.00
  • Rubric: is a list of bugs
  • Reason: The output states it cannot access channel history and provides suggestions on how to search for bugs, but it does not actually provide a list of bugs.

4. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json

  • Score: 0.00
  • Rubric: includes a description of each bug
  • Reason: The output states it cannot access channel history and suggests ways to search, but it does not list any bugs or provide descriptions of each bug.

5. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json

  • Score: 0.00
  • Rubric: attributes each bug to a user
  • Reason: The output states it cannot access channel history and suggests ways to search, but it does not list any bugs nor attribute any bug to a user.

6. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json

  • Score: 0.00
  • Rubric: attributes the bug about trying to save without a color and the save button not doing anything to @maria.nunez
  • Reason: The output does not mention @maria.nunez and does not attribute any specific bug (saving without a color / save button not doing anything) to anyone; it only states it cannot access history and suggests ways to search.

7. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json

  • Score: 0.00
  • Rubric: the bug about the end user being able to change channel banner is attributed to @maria.nunez
  • Reason: The output does not mention the specific bug about end users changing the channel banner, nor does it attribute it to @maria.nunez. It only states inability to access history and suggests ways to search.

8. TestDirectMessageConversations/[anthropic]_bot_dm_tool_introspection

  • Score: 0.00
  • Rubric: mentions Github and refers to the documentation
  • Reason: The output refers to documentation (docs.mattermost.com) but does not mention GitHub, so it does not satisfy the requirement to both mention GitHub and refer to the documentation.

This comment was automatically generated by the eval CI pipeline.

… inputs, align otel exporter

Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f9dde2fcf2

ℹ️ 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".

Comment thread api/audit_middleware.go Outdated
Comment thread api/audit_events.go
cursoragent and others added 2 commits July 31, 2026 15:05
…or messages

Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…t into audit records

Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>

@nickmisasi nickmisasi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Implementation LGTM. I think we should update AGENTS.md to include information about this being required

Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>

@edgarbellot edgarbellot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks a mil for adding this!

@edgarbellot

edgarbellot commented Aug 6, 2026

Copy link
Copy Markdown

Snyk checks have failed. 2 issues have been found so far.

Both are false positives - the pinned server/public@v0.4.3 already contains the fixes for both CVEs. Snyk's advisory is mis-mapping the affected version. No action needed, the PR can be merged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants