fix(coderd): address MCP audit review feedback

Require a non-empty display name on create and update so the audit resource target logic needs no ID fallback, link MCP config audit entries to their settings page, trim audit table comment noise, and pin the created-row audit guarantee when the post-discovery credential update fails.
ibetitsmike · Aug 19, 2026 · Aug 7, 2026 · Aug 7, 2026 · Aug 8, 2026 · Aug 8, 2026
commit a04fd6f06fbd2978147360e2e3051391ddc4798f
diff --git a/coderd/audit.go b/coderd/audit.go
@@ -616,6 +616,8 @@ func (api *API) auditLogResourceLink(ctx context.Context, alog database.GetAudit
 		// Chats are surfaced at /agents/{id}. They are owner-scoped but
 		// not username-scoped in the URL like workspaces or tasks.
 		return fmt.Sprintf("/agents/%s", alog.AuditLog.ResourceID)
+	case database.ResourceTypeMCPServerConfig:
+		return fmt.Sprintf("/ai/settings/mcp-servers/%s", alog.AuditLog.ResourceID)
 	case database.ResourceTypeUserSecret:
 		// TODO(PLAT-102): point at the user secrets management page once
 		// it ships. Until then, the audit row links nowhere.

diff --git a/coderd/audit/request.go b/coderd/audit/request.go
@@ -155,11 +155,7 @@ func ResourceTarget[T Auditable](tgt T) string {
 		// filter but not the primary resource identifier.
 		return typed.ID.String()[:8]
 	case database.MCPServerConfig:
-		if typed.DisplayName != "" {
-			return typed.DisplayName
-		}
-		// The full ID equals resource_id, so the label is exactly filterable.
-		return typed.ID.String()
+		return typed.DisplayName
 	case database.UserSecret:
 		return typed.Name
 	case database.UserSkill:

diff --git a/coderd/audit_test.go b/coderd/audit_test.go
@@ -16,6 +16,7 @@ import (
 	"github.com/coder/coder/v2/coderd/coderdtest"
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/dbgen"
+	"github.com/coder/coder/v2/coderd/database/dbtestutil"
 	"github.com/coder/coder/v2/coderd/rbac"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/provisioner/echo"
@@ -143,6 +144,35 @@ func TestAuditLogs(t *testing.T) {
 			workspace.OwnerName, workspace.Name, buildNumberString))
 	})
 
+	t.Run("MCPServerConfigAuditLink", func(t *testing.T) {
+		t.Parallel()
+
+		ctx := context.Background()
+		db, ps := dbtestutil.NewDB(t)
+		client := coderdtest.New(t, &coderdtest.Options{Database: db, Pubsub: ps})
+		user := coderdtest.CreateFirstUser(t, client)
+
+		config := dbgen.MCPServerConfig(t, db, database.MCPServerConfig{
+			OrganizationID: user.OrganizationID,
+		})
+		err := client.CreateTestAuditLog(ctx, codersdk.CreateTestAuditLogRequest{
+			Action:         codersdk.AuditActionCreate,
+			ResourceType:   codersdk.ResourceTypeMCPServerConfig,
+			ResourceID:     config.ID,
+			OrganizationID: user.OrganizationID,
+		})
+		require.NoError(t, err)
+
+		auditLogs, err := client.AuditLogs(ctx, codersdk.AuditLogsRequest{
+			Pagination: codersdk.Pagination{
+				Limit: 1,
+			},
+		})
+		require.NoError(t, err)
+		require.Len(t, auditLogs.AuditLogs, 1)
+		require.Equal(t, fmt.Sprintf("/ai/settings/mcp-servers/%s", config.ID), auditLogs.AuditLogs[0].ResourceLink)
+	})
+
 	t.Run("Organization", func(t *testing.T) {
 		t.Parallel()
 

diff --git a/coderd/mcp.go b/coderd/mcp.go
@@ -272,6 +272,14 @@ func (api *API) createMCPServerConfig(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// The struct tag accepts whitespace-only names, which trim to "".
+	if strings.TrimSpace(req.DisplayName) == "" {
+		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
+			Message: "Display name is required.",
+		})
+		return
+	}
+
 	if trimmed := strings.TrimSpace(req.OAuth2RevocationURL); trimmed != "" {
 		if err := mcpclient.ValidateRevocationEndpoint(trimmed); err != nil {
 			httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
@@ -581,6 +589,13 @@ func (api *API) updateMCPServerConfig(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if req.DisplayName != nil && strings.TrimSpace(*req.DisplayName) == "" {
+		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
+			Message: "Display name cannot be empty.",
+		})
+		return
+	}
+
 	// Validated here rather than via a struct tag because an empty
 	// string is a valid value that clears the stored URL.
 	if req.OAuth2RevocationURL != nil {

diff --git a/coderd/mcp_test.go b/coderd/mcp_test.go
@@ -21,6 +21,7 @@ import (
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/xerrors"
 
 	"github.com/coder/coder/v2/coderd/audit"
 	"github.com/coder/coder/v2/coderd/coderdtest"
@@ -29,6 +30,7 @@ import (
 	"github.com/coder/coder/v2/coderd/database/dbgen"
 	"github.com/coder/coder/v2/coderd/database/dbtestutil"
 	"github.com/coder/coder/v2/coderd/rbac"
+	"github.com/coder/coder/v2/coderd/util/ptr"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/testutil"
 )
@@ -139,6 +141,35 @@ func TestMCPServerConfigLegacyRoutesRemoved(t *testing.T) {
 	}
 }
 
+func TestMCPServerConfigDisplayNameValidation(t *testing.T) {
+	t.Parallel()
+
+	ctx := testutil.Context(t, testutil.WaitLong)
+	client := newMCPClient(t)
+	firstUser := coderdtest.CreateFirstUser(t, client)
+
+	_, err := client.CreateMCPServerConfig(ctx, firstUser.OrganizationID, codersdk.CreateMCPServerConfigRequest{
+		DisplayName:  "   ",
+		Slug:         "whitespace-name",
+		Transport:    "streamable_http",
+		URL:          "https://mcp.example.com",
+		AuthType:     "none",
+		Availability: "default_off",
+	})
+	var sdkErr *codersdk.Error
+	require.ErrorAs(t, err, &sdkErr)
+	require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode())
+
+	config := createMCPServerConfig(t, client, firstUser.OrganizationID, "display-name-validation", true)
+	for _, name := range []string{"", "   "} {
+		_, err := client.UpdateMCPServerConfig(ctx, config.ID, codersdk.UpdateMCPServerConfigRequest{
+			DisplayName: ptr.Ref(name),
+		})
+		require.ErrorAs(t, err, &sdkErr)
+		require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode())
+	}
+}
+
 func TestMCPServerConfigsCRUD(t *testing.T) {
 	t.Parallel()
 
@@ -418,6 +449,87 @@ func TestMCPServerConfigsAudit(t *testing.T) {
 		require.Empty(t, mAudit.AuditLogs())
 	})
 
+	t.Run("CreateAuditedWhenDiscoveryUpdateFails", func(t *testing.T) {
+		t.Parallel()
+
+		ctx := testutil.Context(t, testutil.WaitLong)
+		mAudit := audit.NewMock()
+		providerKeys := coderdtest.FakeOpenAICompatProviderAPIKeys(t)
+		db, ps := dbtestutil.NewDB(t)
+		store := &failingMCPServerConfigUpdateStore{Store: db}
+		client := coderdtest.New(t, &coderdtest.Options{
+			DeploymentValues:    mcpDeploymentValues(t),
+			ChatProviderAPIKeys: &providerKeys,
+			Auditor:             mAudit,
+			Database:            store,
+			Pubsub:              ps,
+		})
+		firstUser := coderdtest.CreateFirstUser(t, client)
+
+		authServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			switch r.URL.Path {
+			case "/.well-known/oauth-authorization-server":
+				w.Header().Set("Content-Type", "application/json")
+				_, _ = w.Write([]byte(`{
+					"issuer": "` + r.Host + `",
+					"authorization_endpoint": "` + "http://" + r.Host + `/authorize",
+					"token_endpoint": "` + "http://" + r.Host + `/token",
+					"registration_endpoint": "` + "http://" + r.Host + `/register",
+					"response_types_supported": ["code"]
+				}`))
+			case "/register":
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(http.StatusCreated)
+				_, _ = w.Write([]byte(`{
+					"client_id": "update-failure-client-id",
+					"client_secret": "update-failure-client-secret"
+				}`))
+			default:
+				http.NotFound(w, r)
+			}
+		}))
+		t.Cleanup(authServer.Close)
+
+		mcpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			switch r.URL.Path {
+			case "/.well-known/oauth-protected-resource/v1/mcp":
+				w.Header().Set("Content-Type", "application/json")
+				_, _ = w.Write([]byte(`{
+					"resource": "` + "http://" + r.Host + `",
+					"authorization_servers": ["` + authServer.URL + `"]
+				}`))
+			default:
+				http.NotFound(w, r)
+			}
+		}))
+		t.Cleanup(mcpServer.Close)
+
+		// The inserted row must still be audited when the post-discovery update fails.
+		store.fail.Store(true)
+		mAudit.ResetLogs()
+		_, err := client.CreateMCPServerConfig(ctx, firstUser.OrganizationID, codersdk.CreateMCPServerConfigRequest{
+			DisplayName:  "Audit Update Failure",
+			Slug:         "audit-update-failure",
+			Transport:    "streamable_http",
+			URL:          mcpServer.URL + "/v1/mcp",
+			AuthType:     "oauth2",
+			Availability: "default_on",
+			Enabled:      true,
+		})
+		var sdkErr *codersdk.Error
+		require.ErrorAs(t, err, &sdkErr)
+		require.Equal(t, http.StatusInternalServerError, sdkErr.StatusCode())
+
+		configs, err := client.MCPServerConfigs(ctx, firstUser.OrganizationID)
+		require.NoError(t, err)
+		require.Len(t, configs, 1)
+
+		logs := mAudit.AuditLogs()
+		require.Len(t, logs, 1)
+		require.Equal(t, database.AuditActionCreate, logs[0].Action)
+		require.Equal(t, configs[0].ID, logs[0].ResourceID)
+	})
+
 	t.Run("DeletedResourceMarked", func(t *testing.T) {
 		t.Parallel()
 
@@ -510,6 +622,21 @@ func TestMCPServerConfigsAudit(t *testing.T) {
 	})
 }
 
+// failingMCPServerConfigUpdateStore fails config updates once armed,
+// outdating the discovery flow's post-insert credential update.
+type failingMCPServerConfigUpdateStore struct {
+	database.Store
+
+	fail atomic.Bool
+}
+
+func (s *failingMCPServerConfigUpdateStore) UpdateMCPServerConfig(ctx context.Context, arg database.UpdateMCPServerConfigParams) (database.MCPServerConfig, error) {
+	if s.fail.Load() {
+		return database.MCPServerConfig{}, xerrors.New("injected update failure")
+	}
+	return s.Store.UpdateMCPServerConfig(ctx, arg)
+}
+
 // staleMCPServerConfigReadStore corrupts plain config reads once armed,
 // simulating a concurrent update that outdates the param middleware's
 // snapshot. Locked ForUpdate reads stay untouched.

diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go
@@ -509,32 +509,32 @@ var auditableResourcesTypes = map[any]map[string]Action{
 		"description":                 ActionTrack,
 		"icon_url":                    ActionTrack,
 		"transport":                   ActionTrack,
-		"url":                         ActionSecret, // May embed credentials in userinfo or query; show change, never contents.
+		"url":                         ActionSecret, // May embed credentials in userinfo or query
 		"auth_type":                   ActionTrack,
 		"oauth2_client_id":            ActionTrack,
-		"oauth2_client_secret":        ActionSecret, // Credential; show change, never contents.
+		"oauth2_client_secret":        ActionSecret,
 		"oauth2_client_secret_key_id": ActionIgnore, // dbcrypt bookkeeping.
-		"oauth2_auth_url":             ActionSecret, // May embed credentials in userinfo or query; show change, never contents.
-		"oauth2_token_url":            ActionSecret, // May embed credentials in userinfo or query; show change, never contents.
+		"oauth2_auth_url":             ActionSecret, // May embed credentials in userinfo or query
+		"oauth2_token_url":            ActionSecret, // May embed credentials in userinfo or query
 		"oauth2_scopes":               ActionTrack,
 		"api_key_header":              ActionTrack,
-		"api_key_value":               ActionSecret, // Credential; show change, never contents.
+		"api_key_value":               ActionSecret,
 		"api_key_value_key_id":        ActionIgnore, // dbcrypt bookkeeping.
-		"custom_headers":              ActionSecret, // May contain credentials; show change, never contents.
+		"custom_headers":              ActionSecret, // May contain credentials
 		"custom_headers_key_id":       ActionIgnore, // dbcrypt bookkeeping.
 		"tool_allow_list":             ActionTrack,
 		"tool_deny_list":              ActionTrack,
 		"availability":                ActionTrack,
 		"enabled":                     ActionTrack,
 		"created_by":                  ActionTrack,
 		"updated_by":                  ActionTrack,
-		"created_at":                  ActionIgnore, // Never changes.
-		"updated_at":                  ActionIgnore, // Bumped on every mutation.
+		"created_at":                  ActionIgnore,
+		"updated_at":                  ActionIgnore,
 		"model_intent":                ActionTrack,
 		"allow_in_plan_mode":          ActionTrack,
 		"forward_coder_headers":       ActionTrack,
-		"oauth2_revocation_url":       ActionSecret, // May embed credentials in userinfo or query; show change, never contents.
-		"organization_id":             ActionIgnore, // Never changes after creation; carried by the audit log's organization ID.
+		"oauth2_revocation_url":       ActionSecret, // May embed credentials in userinfo or query
+		"organization_id":             ActionIgnore,
 	},
 	&database.UserSkill{}: {
 		"id":          ActionTrack,