diff --git a/sidecar/src/Aryx.AgentHost/Services/CopilotApprovalCoordinator.cs b/sidecar/src/Aryx.AgentHost/Services/CopilotApprovalCoordinator.cs index 1cc7e41..decf91b 100644 --- a/sidecar/src/Aryx.AgentHost/Services/CopilotApprovalCoordinator.cs +++ b/sidecar/src/Aryx.AgentHost/Services/CopilotApprovalCoordinator.cs @@ -57,9 +57,10 @@ internal sealed class CopilotApprovalCoordinator { string? toolName = ResolveApprovalToolName(request, toolNamesByCallId); string? autoApprovedToolName = ResolveAutoApprovedToolName(request); + string? mcpServerApprovalKey = ResolveMcpServerApprovalKey(request); string? approvalCacheKey = ResolveApprovalCacheKey(toolName, autoApprovedToolName); if (IsToolApprovedForRequest(command.RequestId, approvalCacheKey) - || !RequiresToolCallApproval(command.Pattern.ApprovalPolicy, agent.Id, toolName, autoApprovedToolName)) + || !RequiresToolCallApproval(command.Pattern.ApprovalPolicy, agent.Id, toolName, autoApprovedToolName, mcpServerApprovalKey)) { return CreateApprovalResult(PermissionRequestResultKind.Approved); } @@ -227,7 +228,8 @@ internal sealed class CopilotApprovalCoordinator ApprovalPolicyDto? approvalPolicy, string agentId, string? toolName, - string? autoApprovedToolName = null) + string? autoApprovedToolName = null, + string? mcpServerApprovalKey = null) { if (approvalPolicy?.Rules is null || approvalPolicy.Rules.Count == 0) { @@ -245,7 +247,8 @@ internal sealed class CopilotApprovalCoordinator return true; } - return !MatchesAutoApprovedTool(autoApprovedToolNames, toolName, autoApprovedToolName); + return !MatchesAutoApprovedTool(autoApprovedToolNames, toolName, autoApprovedToolName) + && !MatchesAutoApprovedToolName(autoApprovedToolNames, mcpServerApprovalKey); } internal static bool TryGetApprovalToolName( @@ -327,6 +330,19 @@ internal sealed class CopilotApprovalCoordinator return GetFallbackToolName(request); } + private const string McpServerApprovalPrefix = "mcp_server:"; + + private static string? ResolveMcpServerApprovalKey(PermissionRequest request) + { + if (request is not PermissionRequestMcp mcp) + { + return null; + } + + string? serverName = NormalizeOptionalString(mcp.ServerName); + return serverName is not null ? $"{McpServerApprovalPrefix}{serverName}" : null; + } + private static string? ResolveApprovalCacheKey( string? toolName, string? autoApprovedToolName) diff --git a/sidecar/tests/Aryx.AgentHost.Tests/CopilotWorkflowRunnerTests.cs b/sidecar/tests/Aryx.AgentHost.Tests/CopilotWorkflowRunnerTests.cs index ed236cf..89d23ed 100644 --- a/sidecar/tests/Aryx.AgentHost.Tests/CopilotWorkflowRunnerTests.cs +++ b/sidecar/tests/Aryx.AgentHost.Tests/CopilotWorkflowRunnerTests.cs @@ -843,6 +843,36 @@ public sealed class CopilotWorkflowRunnerTests Assert.True(CopilotApprovalCoordinator.RequiresToolCallApproval(policy, "agent-1", "git.status")); } + [Fact] + public void RequiresToolCallApproval_HonorsMcpServerLevelApprovalKey() + { + ApprovalPolicyDto policy = new() + { + Rules = + [ + new ApprovalCheckpointRuleDto + { + Kind = "tool-call", + }, + ], + AutoApprovedToolNames = ["mcp_server:Git MCP"], + }; + + // Server-level key approves any tool from that server + Assert.False(CopilotApprovalCoordinator.RequiresToolCallApproval( + policy, "agent-1", "git.status", null, "mcp_server:Git MCP")); + Assert.False(CopilotApprovalCoordinator.RequiresToolCallApproval( + policy, "agent-1", "git.diff", null, "mcp_server:Git MCP")); + + // Different server still requires approval + Assert.True(CopilotApprovalCoordinator.RequiresToolCallApproval( + policy, "agent-1", "fs.read", null, "mcp_server:Filesystem")); + + // Non-MCP tools unaffected + Assert.True(CopilotApprovalCoordinator.RequiresToolCallApproval( + policy, "agent-1", "unknown_tool")); + } + [Fact] public void TryGetApprovalToolName_ResolvesDirectNamesAndRuntimeFallbacks() { diff --git a/src/renderer/components/ChatPane.tsx b/src/renderer/components/ChatPane.tsx index 478592a..0295d9c 100644 --- a/src/renderer/components/ChatPane.tsx +++ b/src/renderer/components/ChatPane.tsx @@ -26,6 +26,8 @@ import { type PatternDefinition, type ReasoningEffort } from '@shared/domain/pat import { isScratchpadProject, type ProjectRecord } from '@shared/domain/project'; import { resolveSessionToolingSelection, type SessionRecord } from '@shared/domain/session'; import { + groupApprovalToolsByProvider, + isMcpServerApprovalKey, listApprovalToolDefinitions, type RuntimeToolDefinition, type SessionToolingSelection, @@ -128,10 +130,19 @@ export function ChatPane({ ), [isApprovalOverridden, session.approvalSettings, pattern.approvalPolicy], ); - const effectiveAutoApprovedCount = useMemo( - () => approvalTools.filter((t) => effectiveAutoApproved.has(t.id)).length, - [approvalTools, effectiveAutoApproved], - ); + const effectiveAutoApprovedCount = useMemo(() => { + const groups = groupApprovalToolsByProvider(approvalTools, toolingSettings); + let count = 0; + for (const group of groups) { + if (group.serverApprovalKey && effectiveAutoApproved.has(group.serverApprovalKey)) { + // Server-level approval: count as 1 approved group even with 0 declared tools + count += Math.max(group.tools.length, 1); + } else { + count += group.tools.filter((t) => effectiveAutoApproved.has(t.id)).length; + } + } + return count; + }, [approvalTools, effectiveAutoApproved, toolingSettings]); useEffect(() => { transcriptRef.current?.scrollTo({ diff --git a/src/renderer/components/chat/InlinePills.tsx b/src/renderer/components/chat/InlinePills.tsx index c74f0be..8553740 100644 --- a/src/renderer/components/chat/InlinePills.tsx +++ b/src/renderer/components/chat/InlinePills.tsx @@ -367,7 +367,11 @@ export function InlineApprovalPill({ [approvalTools, toolingSettings], ); - const showSearch = approvalTools.length > SEARCH_THRESHOLD; + const totalItemCount = groups.reduce( + (sum, g) => sum + Math.max(g.tools.length, g.serverApprovalKey ? 1 : 0), + 0, + ); + const showSearch = totalItemCount > SEARCH_THRESHOLD; const searchLower = search.toLowerCase().trim(); const filteredGroups = useMemo(() => { @@ -382,7 +386,7 @@ export function InlineApprovalPill({ || group.label.toLowerCase().includes(searchLower), ), })) - .filter((g) => g.tools.length > 0); + .filter((g) => g.tools.length > 0 || g.label.toLowerCase().includes(searchLower)); }, [groups, searchLower]); function toggleTool(toolId: string) { @@ -396,18 +400,45 @@ export function InlineApprovalPill({ } function toggleGroup(group: ApprovalToolGroup) { - const allApproved = group.tools.every((t) => effectiveAutoApproved.has(t.id)); const next = new Set(effectiveAutoApproved); - for (const tool of group.tools) { - if (allApproved) { - next.delete(tool.id); + + if (group.serverApprovalKey) { + // MCP servers use server-level approval key + if (next.has(group.serverApprovalKey)) { + next.delete(group.serverApprovalKey); } else { - next.add(tool.id); + next.add(group.serverApprovalKey); + } + // Also remove individual tool entries when toggling server-level + for (const tool of group.tools) { + next.delete(tool.id); + } + } else { + // Non-MCP groups: toggle individual tools + const allApproved = group.tools.every((t) => next.has(t.id)); + for (const tool of group.tools) { + if (allApproved) { + next.delete(tool.id); + } else { + next.add(tool.id); + } } } + onUpdate({ autoApprovedToolNames: [...next] }); } + function isGroupApproved(group: ApprovalToolGroup): 'all' | 'some' | 'none' { + if (group.serverApprovalKey && effectiveAutoApproved.has(group.serverApprovalKey)) { + return 'all'; + } + if (group.tools.length === 0) return 'none'; + const approvedCount = group.tools.filter((t) => effectiveAutoApproved.has(t.id)).length; + if (approvedCount === group.tools.length) return 'all'; + if (approvedCount > 0) return 'some'; + return 'none'; + } + function toggleExpanded(groupId: string) { setExpandedGroups((prev) => { const next = new Set(prev); @@ -442,7 +473,7 @@ export function InlineApprovalPill({ type="button" > - {effectiveAutoApprovedCount}/{approvalTools.length} auto-approved + {effectiveAutoApprovedCount}/{totalItemCount} auto-approved @@ -494,9 +525,12 @@ export function InlineApprovalPill({ const isBuiltin = group.kind === 'builtin'; const isCollapsible = !isBuiltin; const expanded = isBuiltin || isGroupExpanded(group.id); - const approvedCount = group.tools.filter((t) => effectiveAutoApproved.has(t.id)).length; - const allApproved = approvedCount === group.tools.length; - const someApproved = approvedCount > 0 && !allApproved; + const groupState = isGroupApproved(group); + const allApproved = groupState === 'all'; + const someApproved = groupState === 'some'; + const approvedLabel = group.serverApprovalKey && allApproved + ? 'all' + : `${group.tools.filter((t) => effectiveAutoApproved.has(t.id)).length}/${group.tools.length}`; return (
@@ -511,10 +545,14 @@ export function InlineApprovalPill({ onClick={() => toggleExpanded(group.id)} type="button" > - + {group.tools.length > 0 ? ( + + ) : ( + + )} {group.label} - {approvedCount}/{group.tools.length} + {approvedLabel} = [ { id: 'store_memory', label: 'Store memory' }, ]; +const MCP_SERVER_APPROVAL_PREFIX = 'mcp_server:'; + +export function buildMcpServerApprovalKey(serverName: string): string { + return `${MCP_SERVER_APPROVAL_PREFIX}${serverName}`; +} + +export function isMcpServerApprovalKey(key: string): boolean { + return key.startsWith(MCP_SERVER_APPROVAL_PREFIX); +} + export function createWorkspaceSettings(): WorkspaceSettings { return { theme: 'dark', @@ -262,7 +272,11 @@ export function listApprovalToolNames( tooling: WorkspaceToolingSettings, runtimeTools?: ReadonlyArray, ): string[] { - return listApprovalToolDefinitions(tooling, runtimeTools).map((tool) => tool.id); + const toolNames = listApprovalToolDefinitions(tooling, runtimeTools).map((tool) => tool.id); + for (const server of tooling.mcpServers) { + toolNames.push(buildMcpServerApprovalKey(server.name)); + } + return [...new Set(toolNames)]; } export interface ApprovalToolGroup { @@ -270,6 +284,7 @@ export interface ApprovalToolGroup { label: string; kind: ApprovalToolKind; tools: ApprovalToolDefinition[]; + serverApprovalKey?: string; } const approvalToolGroupKindOrder: ApprovalToolKind[] = ['builtin', 'mcp', 'lsp', 'mixed']; @@ -292,6 +307,18 @@ export function groupApprovalToolsByProvider( group.tools.push(tool); } + // Ensure every MCP server has a group (even with no declared tools) and + // attach server-level approval keys. + for (const server of tooling.mcpServers) { + const groupId = `mcp:${server.id}`; + let group = groups.get(groupId); + if (!group) { + group = { id: groupId, label: server.name, kind: 'mcp', tools: [] }; + groups.set(groupId, group); + } + group.serverApprovalKey = buildMcpServerApprovalKey(server.name); + } + return [...groups.values()].sort((a, b) => { const kindDiff = approvalToolGroupKindOrder.indexOf(a.kind) - approvalToolGroupKindOrder.indexOf(b.kind); if (kindDiff !== 0) return kindDiff; diff --git a/tests/shared/tooling.test.ts b/tests/shared/tooling.test.ts index bd25da5..131ab98 100644 --- a/tests/shared/tooling.test.ts +++ b/tests/shared/tooling.test.ts @@ -1,8 +1,10 @@ import { describe, expect, test } from 'bun:test'; import { + buildMcpServerApprovalKey, groupApprovalToolsByProvider, listApprovalToolDefinitions, + listApprovalToolNames, normalizeWorkspaceSettings, resolveProjectToolingSettings, resolveToolLabel, @@ -441,7 +443,7 @@ describe('groupApprovalToolsByProvider', () => { expect(builtinGroups[0].tools.length).toBe(5); }); - test('multi-provider tools go into first provider group', () => { + test('multi-provider tools go into first provider group, second provider gets empty group', () => { const tooling = makeTooling([ makeMcpServer('git-a', 'Git A', ['git.status']), makeMcpServer('git-b', 'Git B', ['git.status']), @@ -450,9 +452,13 @@ describe('groupApprovalToolsByProvider', () => { const groups = groupApprovalToolsByProvider(tools, tooling); const mcpGroups = groups.filter((g) => g.kind === 'mcp'); - expect(mcpGroups.length).toBe(1); - expect(mcpGroups[0].label).toBe('Git A'); - expect(mcpGroups[0].tools[0].providerNames).toEqual(['Git A', 'Git B']); + expect(mcpGroups.length).toBe(2); + const gitA = mcpGroups.find((g) => g.label === 'Git A'); + expect(gitA).toBeDefined(); + expect(gitA!.tools[0].providerNames).toEqual(['Git A', 'Git B']); + const gitB = mcpGroups.find((g) => g.label === 'Git B'); + expect(gitB).toBeDefined(); + expect(gitB!.tools.length).toBe(0); }); test('sorts builtin first, then MCP by name, then LSP by name', () => { @@ -472,4 +478,61 @@ describe('groupApprovalToolsByProvider', () => { const mcpLabels = groups.filter((g) => g.kind === 'mcp').map((g) => g.label); expect(mcpLabels).toEqual(['Alpha', 'Zebra']); }); + + test('creates groups with serverApprovalKey for MCP servers', () => { + const tooling = makeTooling([ + makeMcpServer('git', 'Git MCP', ['git.status']), + ]); + const tools = listApprovalToolDefinitions(tooling); + const groups = groupApprovalToolsByProvider(tools, tooling); + + const mcpGroup = groups.find((g) => g.kind === 'mcp'); + expect(mcpGroup).toBeDefined(); + expect(mcpGroup!.serverApprovalKey).toBe('mcp_server:Git MCP'); + }); + + test('creates groups for MCP servers with empty tools array', () => { + const tooling = makeTooling([ + makeMcpServer('empty', 'Empty Server', []), + ]); + const tools = listApprovalToolDefinitions(tooling); + const groups = groupApprovalToolsByProvider(tools, tooling); + + const mcpGroups = groups.filter((g) => g.kind === 'mcp'); + expect(mcpGroups.length).toBe(1); + expect(mcpGroups[0].label).toBe('Empty Server'); + expect(mcpGroups[0].tools.length).toBe(0); + expect(mcpGroups[0].serverApprovalKey).toBe('mcp_server:Empty Server'); + }); + + test('builtin groups do not have serverApprovalKey', () => { + const tooling = makeTooling(); + const tools = listApprovalToolDefinitions(tooling); + const groups = groupApprovalToolsByProvider(tools, tooling); + + const builtinGroup = groups.find((g) => g.kind === 'builtin'); + expect(builtinGroup!.serverApprovalKey).toBeUndefined(); + }); +}); + +describe('server-level approval keys', () => { + test('buildMcpServerApprovalKey produces mcp_server: prefixed key', () => { + expect(buildMcpServerApprovalKey('My Server')).toBe('mcp_server:My Server'); + expect(buildMcpServerApprovalKey('git')).toBe('mcp_server:git'); + }); + + test('listApprovalToolNames includes server-level keys', () => { + const tooling: WorkspaceToolingSettings = { + mcpServers: [{ + id: 'git', name: 'Git MCP', transport: 'local', command: 'node', args: [], + tools: [], createdAt: '2026-03-28T00:00:00.000Z', updatedAt: '2026-03-28T00:00:00.000Z', + }], + lspProfiles: [], + }; + const names = listApprovalToolNames(tooling); + expect(names).toContain('mcp_server:Git MCP'); + // Also includes builtins + expect(names).toContain('read'); + expect(names).toContain('shell'); + }); });