Three standalone MCP/customizations fixes: group-header casing, unclickable Show Output, duplicated server rows

src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostCustomizationService.ts

@@ -184,7 +184,7 @@ export abstract class AbstractAgentHostCustomizationService extends Disposable i
 		if (!target) {
 			return [];
 		}
-		return this._flattenMcpServers(target.customizations)
+		return getPresentableMcpServerCustomizations(target.customizations)
 			.map(({ server, plugin }): IAgentHostMcpServer => ({
 				id: this._scopedMcpServerId(sessionResource, server.id),
 				name: server.name,
@@ -208,7 +208,7 @@ export abstract class AbstractAgentHostCustomizationService extends Disposable i
 		if (!target) {
 			return Promise.resolve();
 		}
-		const entry = this._flattenMcpServers(target.customizations).find(({ server }) => this._scopedMcpServerId(sessionResource, server.id) === serverId);
+		const entry = flattenMcpServerCustomizations(target.customizations).find(({ server }) => this._scopedMcpServerId(sessionResource, server.id) === serverId);
 		if (!entry) {
 			return Promise.resolve();
 		}
@@ -226,7 +226,7 @@ export abstract class AbstractAgentHostCustomizationService extends Disposable i
 	 */
 	private _trackMcpDiagnostics(sessionResource: URI, target: IAgentHostCustomizationTarget): void {
 		this._mcpDiagnosticSessions.add(sessionResource);
-		for (const { server, plugin } of this._flattenMcpServers(target.customizations)) {
+		for (const { server, plugin } of flattenMcpServerCustomizations(target.customizations)) {
 			this._mcpLogRegistry.record({ sessionResource, rawId: server.id, name: server.name, enabled: isCustomizationEnabled(server) && (!plugin || isCustomizationEnabled(plugin)), state: server.state });
 		}
 	}
@@ -238,7 +238,7 @@ export abstract class AbstractAgentHostCustomizationService extends Disposable i
 			if (!target) {
 				continue;
 			}
-			for (const { server, plugin } of this._flattenMcpServers(target.customizations)) {
+			for (const { server, plugin } of flattenMcpServerCustomizations(target.customizations)) {
 				this._mcpLogRegistry.record({ sessionResource, rawId: server.id, name: server.name, enabled: isCustomizationEnabled(server) && (!plugin || isCustomizationEnabled(plugin)), state: server.state });
 			}
 		}
@@ -327,17 +327,8 @@ export abstract class AbstractAgentHostCustomizationService extends Disposable i
 		this._onDidChangeCustomizations.fire();
 	}
 
-	private _flattenMcpServers(customizations: readonly Customization[]): readonly { readonly server: McpServerCustomization; readonly plugin?: PluginCustomization }[] {
-		return customizations.flatMap(customization => customization.type === CustomizationType.McpServer
-			? [{ server: customization }]
-			: customization.children?.filter(child => child.type === CustomizationType.McpServer).map(server => ({
-				server,
-				plugin: customization.type === CustomizationType.Plugin ? customization : undefined,
-			})) ?? []);
-	}
-
 	private _findMcpServer(customizations: readonly Customization[], serverId: string): McpServerCustomization | undefined {
-		for (const { server } of this._flattenMcpServers(customizations)) {
+		for (const { server } of flattenMcpServerCustomizations(customizations)) {
 			if (server.id === serverId || this._isScopedMcpServerIdForRawId(serverId, server.id)) {
 				return server;
 			}
@@ -368,6 +359,58 @@ export abstract class AbstractAgentHostCustomizationService extends Disposable i
 	}
 }
 
+/** One MCP server customization, with the position it was published at. */
+export interface IMcpServerCustomizationEntry {
+	readonly server: McpServerCustomization;
+	/**
+	 * The plugin that declares this server. Absent both for a server published at the top level
+	 * and for one declared by a {@link CustomizationType.Directory} container, so it says nothing
+	 * about where in the tree the server sits -- use {@link isTopLevel} for that.
+	 */
+	readonly plugin?: PluginCustomization;
+	/** Whether the agent host published this server as a customization of the session itself. */
+	readonly isTopLevel: boolean;
+}
+
+/** Every MCP server customization in a session, including duplicates of the same server. */
+export function flattenMcpServerCustomizations(customizations: readonly Customization[]): readonly IMcpServerCustomizationEntry[] {
+	return customizations.flatMap((customization): IMcpServerCustomizationEntry[] => customization.type === CustomizationType.McpServer
+		? [{ server: customization, isTopLevel: true }]
+		: customization.children?.filter(child => child.type === CustomizationType.McpServer).map(server => ({
+			server,
+			plugin: customization.type === CustomizationType.Plugin ? customization : undefined,
+			isTopLevel: false,
+		})) ?? []);
+}
+
+/**
+ * The MCP servers to *show* for a session: one entry per server.
+ *
+ * A session can carry two customizations for one server: the declaration, published as a child of
+ * whatever declared it, and a top-level entry the agent host mints for a server the SDK reports
+ * before that child resolves by name. A child is dropped when a top-level customization already
+ * speaks for its name, because the top-level copy is the one the host keeps live and resolves for
+ * lifecycle and enablement.
+ *
+ * Tree position is the signal, not the shape of the minted id and not the absence of an owning
+ * plugin -- a directory-declared child has none either. Only presentation dedupes; lookups
+ * elsewhere walk every customization, so an id from either copy still resolves. Servers of the
+ * same name from different containers are left alone, because they are different servers.
+ */
+export function getPresentableMcpServerCustomizations(customizations: readonly Customization[]): readonly IMcpServerCustomizationEntry[] {
+	const entries = flattenMcpServerCustomizations(customizations);
+	const topLevelNames = new Set<string>();
+	for (const entry of entries) {
+		if (entry.isTopLevel) {
+			topLevelNames.add(entry.server.name);
+		}
+	}
+	if (topLevelNames.size === 0) {
+		return entries;
+	}
+	return entries.filter(entry => entry.isTopLevel || !topLevelNames.has(entry.server.name));
+}
+
 class WorkbenchAgentHostCustomizationService extends AbstractAgentHostCustomizationService {
 
 	private readonly _sessionStateSubscriptions = this._register(new DisposableResourceMap<IDisposable & { readonly connection: IAgentConnection; readonly backendSession: URI; readonly sub: IAgentSubscription<SessionState> }>());

src/vs/workbench/contrib/chat/browser/aiCustomization/mcpListWidget.ts

@@ -162,12 +162,22 @@ interface IMcpServerItemTemplateData {
 	readonly actions: HTMLElement;
 	readonly elementDisposables: DisposableStore;
 	readonly actionDisposables: DisposableStore;
+	/** Which row the actions currently belong to, so a recycled template cannot reuse another row's. */
+	renderedRowKey?: string;
+	/** What the actions currently show, so an unchanged status does not rebuild them. */
+	renderedStatusSignature?: string;
 }
 
 /**
  * Renderer for local MCP server list items.
  */
-class McpServerItemRenderer implements IListRenderer<IMcpServerItemEntry | IMcpSessionServerItemEntry | IMcpBuiltinItemEntry, IMcpServerItemTemplateData> {
+/**
+ * Renderer for local MCP server list items.
+ *
+ * Exported for testing: the guard that keeps a row's actions alive across no-op updates is only
+ * observable by driving the renderer itself.
+ */
+export class McpServerItemRenderer implements IListRenderer<IMcpServerItemEntry | IMcpSessionServerItemEntry | IMcpBuiltinItemEntry, IMcpServerItemTemplateData> {
 	readonly templateId = 'mcpServerItem';
 
 	constructor(
@@ -206,8 +216,18 @@ class McpServerItemRenderer implements IListRenderer<IMcpServerItemEntry | IMcpS
 	}
 
 	renderElement(element: IMcpServerItemEntry | IMcpSessionServerItemEntry | IMcpBuiltinItemEntry, index: number, templateData: IMcpServerItemTemplateData): void {
+		// Tearing down the actions is what makes a click land on a node that is about to be
+		// replaced, so only do it when this template starts showing a different row. Whether the
+		// same row's actions need rebuilding is decided by `updateStatus` from its own signature.
+		const rowKey = getMcpRowKey(element);
+		if (templateData.renderedRowKey !== rowKey) {
+			templateData.renderedRowKey = rowKey;
+			templateData.renderedStatusSignature = undefined;
+			templateData.actionDisposables.clear();
+			DOM.clearNode(templateData.actions);
+		}
+		// Always re-created: these capture `element`, which is a fresh object on every refresh.
 		templateData.elementDisposables.clear();
-		templateData.actionDisposables.clear();
 
 		if (element.type === 'builtin-item') {
 			templateData.container.classList.add('builtin');
@@ -314,17 +334,40 @@ class McpServerItemRenderer implements IListRenderer<IMcpServerItemEntry | IMcpS
 	}
 
 	private updateStatus(templateData: IMcpServerItemTemplateData, element: IMcpServerItemEntry | IMcpSessionServerItemEntry | IMcpBuiltinItemEntry, state: McpStatusKind | undefined, disabledReason?: CustomizationDisabledReason): void {
+		const presentation = getMcpStatusPresentation(state, disabledReason);
+		const activeSessionServer = getActiveSessionServer(element);
+		const label = getMcpEntryLabel(element);
+		const activeSessionResource = this.customizationHarnessService.activeSessionResource.get();
+		const localServer = element.type === 'session-server-item' ? undefined : element.localServer;
+
+		// This runs from an autorun over the server's connection state, and an erroring server
+		// re-runs it about twice a second with byte-identical content. Rebuilding regardless meant
+		// a node replaced between mousedown and mouseup never saw the click, so `Show Output` did
+		// nothing on precisely the rows that needed it.
+		const signature = getMcpStatusRenderSignature({
+			rowKey: getMcpRowKey(element),
+			label,
+			state,
+			statusLabel: presentation?.label,
+			statusClassName: presentation?.className,
+			statusIconId: presentation?.icon?.id,
+			activeSessionServerId: activeSessionServer?.id,
+			logOutputChannelId: activeSessionServer?.logOutputChannelId,
+			localServerId: localServer?.definition.id,
+			activeSessionResource: activeSessionResource.toString(),
+		});
+		if (templateData.renderedStatusSignature === signature) {
+			return;
+		}
+		templateData.renderedStatusSignature = signature;
+
 		templateData.actionDisposables.clear();
 		DOM.clearNode(templateData.actions);
 
-		const presentation = getMcpStatusPresentation(state, disabledReason);
 		if (!presentation) {
 			return;
 		}
 
-		const activeSessionServer = getActiveSessionServer(element);
-		const label = getMcpEntryLabel(element);
-		const activeSessionResource = this.customizationHarnessService.activeSessionResource.get();
 		const showActiveSessionOutput = activeSessionServer !== undefined
 			? (beforeShow?: () => Promise<void>) => this.agentHostCustomizationService.showMcpServerLog(activeSessionResource, activeSessionServer.id, beforeShow)
 			: undefined;
@@ -354,7 +397,7 @@ class McpServerItemRenderer implements IListRenderer<IMcpServerItemEntry | IMcpS
 		}
 
 		const showOutput = state === McpServerStatus.Error || state === McpConnectionState.Kind.Error
-			? getMcpServerOutputHandler(this.outputService, element.type === 'session-server-item' ? undefined : element.localServer, activeSessionServer, this._afterShowOutput, showActiveSessionOutput)
+			? getMcpServerOutputHandler(this.outputService, localServer, activeSessionServer, this._afterShowOutput, showActiveSessionOutput)
 			: undefined;
 		if (showOutput) {
 			const showOutputLabel = localize('showMcpServerOutput', "Show output for {0}", label);
@@ -451,6 +494,64 @@ function getActiveSessionServer(entry: IMcpServerItemEntry | IMcpSessionServerIt
 	return entry.type === 'session-server-item' ? entry.server : entry.activeSessionServer;
 }
 
+/**
+ * Which row a template is currently showing. List entries are recreated on every refresh, so
+ * object identity says nothing about whether this is still the same server in the same place.
+ */
+function getMcpRowKey(entry: IMcpServerItemEntry | IMcpSessionServerItemEntry | IMcpBuiltinItemEntry): string {
+	switch (entry.type) {
+		case 'server-item':
+			return `server:${entry.server.id}:${entry.marketplace ? 1 : 0}`;
+		case 'session-server-item':
+			return `session:${entry.server.id}`;
+		case 'builtin-item':
+			return `builtin:${entry.id}`;
+	}
+}
+
+/** Everything the status actions of a row are built from: what they show, and what they act on. */
+export interface IMcpStatusRenderInput {
+	/** Identifies the row, so a recycled template never mistakes one server's actions for another's. */
+	readonly rowKey: string;
+	/** The server's name, which appears in the button titles and aria labels. */
+	readonly label: string;
+	/** Decides which actions exist at all: sign-in when auth is required, output on error. */
+	readonly state: McpStatusKind | undefined;
+	readonly statusLabel: string | undefined;
+	readonly statusClassName: string | undefined;
+	readonly statusIconId: string | undefined;
+	/** The active-session twin the sign-in and output actions are bound to. */
+	readonly activeSessionServerId: string | undefined;
+	readonly logOutputChannelId: string | undefined;
+	/** The local server the output action falls back to. */
+	readonly localServerId: string | undefined;
+	/** Captured when the output action is built, so switching sessions has to rebuild it. */
+	readonly activeSessionResource: string | undefined;
+}
+
+/**
+ * Reduces a row's status actions to a comparable value, so they are rebuilt only when they would
+ * actually differ. Rebuilding replaces the button nodes, and a node replaced between mousedown and
+ * mouseup never receives the click.
+ *
+ * Must cover every value the actions are built from -- what they render and what they act on --
+ * or a change that matters is dropped. The tests enforce completeness at compile time.
+ */
+export function getMcpStatusRenderSignature(input: IMcpStatusRenderInput): string {
+	return JSON.stringify([
+		input.rowKey,
+		input.label,
+		input.state ?? null,
+		input.statusLabel ?? null,
+		input.statusClassName ?? null,
+		input.statusIconId ?? null,
+		input.activeSessionServerId ?? null,
+		input.logOutputChannelId ?? null,
+		input.localServerId ?? null,
+		input.activeSessionResource ?? null,
+	]);
+}
+
 function getMcpEntryLabel(element: IMcpServerItemEntry | IMcpSessionServerItemEntry | IMcpBuiltinItemEntry): string {
 	return element.type === 'session-server-item'
 		? element.server.name

src/vs/workbench/contrib/chat/browser/aiCustomization/media/aiCustomizationManagement.css

@@ -479,9 +479,10 @@
 	min-width: 0;
 }
 
+/* No text-transform: these labels are localized strings that are already cased correctly, and
+per-word capitalization does not survive translation. */
 .ai-customization-group-header .group-label {
 	font-weight: var(--vscode-agents-fontWeight-semiBold);
-	text-transform: capitalize;
 	color: var(--vscode-sideBarSectionHeader-foreground, var(--vscode-foreground));
 	overflow: hidden;
 	text-overflow: ellipsis;

src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostMcpServerCustomizations.test.ts

@@ -0,0 +1,75 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import assert from 'assert';
+import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js';
+import { CustomizationType, McpServerStatus, type Customization, type DirectoryCustomization, type McpServerCustomization, type McpServerState, type PluginCustomization } from '../../../../../../platform/agentHost/common/state/protocol/state.js';
+import { getPresentableMcpServerCustomizations } from '../../../browser/agentSessions/agentHost/agentHostCustomizationService.js';
+
+suite('agent host MCP server customizations', () => {
+
+	ensureNoDisposablesAreLeakedInTestSuite();
+
+	function mcpServer(id: string, name: string, state: McpServerState = { kind: McpServerStatus.Stopped }): McpServerCustomization {
+		return { type: CustomizationType.McpServer, id, uri: `file:///${encodeURIComponent(id)}`, name, state };
+	}
+
+	function plugin(id: string, children: McpServerCustomization[]): PluginCustomization {
+		return { type: CustomizationType.Plugin, id, uri: `file:///${encodeURIComponent(id)}`, name: id, children };
+	}
+
+	function directory(id: string, children: McpServerCustomization[]): DirectoryCustomization {
+		return { type: CustomizationType.Directory, id, uri: `file:///${encodeURIComponent(id)}`, name: id, enabled: true, writable: true, contents: CustomizationType.McpServer, children };
+	}
+
+	function shown(customizations: readonly Customization[]): [string, string][] {
+		return getPresentableMcpServerCustomizations(customizations).map(({ server }) => [server.name, server.id]);
+	}
+
+	test('a server a plugin declares and the host also minted is shown once, as the live copy', () => {
+		// The shape an agent host actually publishes: the synced `.mcp.json` declares the server
+		// and never leaves `stopped`, while the minted top-level entry is the one it keeps current.
+		const declaration = mcpServer('file:///synced/.mcp.json#mcp=notion', 'notion');
+		const live = mcpServer('mcp-top-level:copilotcli:session:notion', 'notion', { kind: McpServerStatus.Ready });
+
+		assert.deepStrictEqual(shown([plugin('synced', [declaration]), live]), [
+			['notion', 'mcp-top-level:copilotcli:session:notion'],
+		]);
+	});
+
+	test('a server a directory declares and the host also minted is shown once, as the live copy', () => {
+		// A directory-declared child carries no owning plugin, so "has no plugin" cannot stand in
+		// for "is top-level": doing so would let this child claim the name and survive shadowing.
+		const declaration = mcpServer('file:///.mcp.json#mcp=notion', 'notion');
+		const live = mcpServer('mcp-top-level:copilotcli:session:notion', 'notion', { kind: McpServerStatus.Ready });
+
+		assert.deepStrictEqual(shown([directory('mcp-config', [declaration]), live]), [
+			['notion', 'mcp-top-level:copilotcli:session:notion'],
+		]);
+	});
+
+	test('a server only a container declares is kept, and so is one only the host minted', () => {
+		const declared = mcpServer('file:///synced/.mcp.json#mcp=cleanshot', 'cleanshot');
+		const inDirectory = mcpServer('file:///.mcp.json#mcp=playwright', 'playwright');
+		const minted = mcpServer('mcp-top-level:copilotcli:session:notion', 'notion', { kind: McpServerStatus.Ready });
+
+		// Source order is preserved, exactly as it was before duplicates were collapsed.
+		assert.deepStrictEqual(shown([plugin('synced', [declared]), directory('mcp-config', [inDirectory]), minted]), [
+			['cleanshot', 'file:///synced/.mcp.json#mcp=cleanshot'],
+			['playwright', 'file:///.mcp.json#mcp=playwright'],
+			['notion', 'mcp-top-level:copilotcli:session:notion'],
+		]);
+	});
+
+	test('two plugins declaring the same name stay two servers, because they are two servers', () => {
+		assert.deepStrictEqual(shown([
+			plugin('a', [mcpServer('file:///a/.mcp.json#mcp=search', 'search')]),
+			plugin('b', [mcpServer('file:///b/.mcp.json#mcp=search', 'search')]),
+		]), [
+			['search', 'file:///a/.mcp.json#mcp=search'],
+			['search', 'file:///b/.mcp.json#mcp=search'],
+		]);
+	});
+});