fix: approval pill count mismatch with duplicate MCP tool names

When multiple MCP servers expose tools with the same name, the approval
pill showed an incorrect count (e.g. 150/300) even when everything was
approved.  The numerator used a Set<string> to deduplicate by tool ID,
so shared tools were counted once, while the denominator counted each
group occurrence independently.

Extract countApprovedToolsInGroups() into shared/domain/tooling.ts and
switch to per-group counting that mirrors the totalItemCount formula.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
David Kaya
2026-03-30 10:07:41 +02:00
co-authored by Copilot
parent 8a4d23c22a
commit c702cf88e2
3 changed files with 129 additions and 11 deletions
+2 -11
View File
@@ -30,6 +30,7 @@ import { type PatternDefinition, type ReasoningEffort } from '@shared/domain/pat
import { isScratchpadProject, type ProjectRecord } from '@shared/domain/project';
import { resolveSessionToolingSelection, type SessionBranchOriginAction, type SessionRecord } from '@shared/domain/session';
import {
countApprovedToolsInGroups,
groupApprovalToolsByProvider,
listApprovalToolDefinitions,
type RuntimeToolDefinition,
@@ -162,17 +163,7 @@ export function ChatPane({
);
const effectiveAutoApprovedCount = useMemo(() => {
const groups = groupApprovalToolsByProvider(approvalTools, toolingSettings);
const counted = new Set<string>();
for (const group of groups) {
if (group.serverApprovalKey && effectiveAutoApproved.has(group.serverApprovalKey)) {
for (const tool of group.tools) counted.add(tool.id);
} else {
for (const tool of group.tools) {
if (effectiveAutoApproved.has(tool.id)) counted.add(tool.id);
}
}
}
return counted.size;
return countApprovedToolsInGroups(groups, effectiveAutoApproved);
}, [approvalTools, effectiveAutoApproved, toolingSettings]);
const isProbingMcp = (mcpProbingServerIds?.length ?? 0) > 0;
const hasApprovalContent = approvalTools.length > 0 || isProbingMcp;
+28
View File
@@ -395,6 +395,34 @@ function resolveApprovalToolGroupKey(
return { id: 'other', label: 'Other', kind: 'mixed' };
}
/**
* Count how many tools are effectively auto-approved across all groups.
*
* This must use per-group counting (not unique-tool-ID deduplication) to stay
* consistent with the total-item formula used in InlineApprovalPill, which
* counts each group occurrence independently. Without this, shared tool names
* across MCP servers produce a numerator smaller than the denominator even when
* everything is approved.
*/
export function countApprovedToolsInGroups(
groups: ReadonlyArray<ApprovalToolGroup>,
approved: ReadonlySet<string>,
): number {
let count = 0;
for (const group of groups) {
if (group.serverApprovalKey && approved.has(group.serverApprovalKey)) {
// Server-level approval covers all tools. Servers with 0 tools still
// count as 1 (matching the totalItemCount formula).
count += Math.max(group.tools.length, 1);
} else {
for (const tool of group.tools) {
if (approved.has(tool.id)) count += 1;
}
}
}
return count;
}
export function validateMcpServerDefinition(server: McpServerDefinition): string | undefined {
if (!server.name.trim()) {
return 'MCP server name is required.';
+99
View File
@@ -2,6 +2,7 @@ import { describe, expect, test } from 'bun:test';
import {
buildMcpServerApprovalKey,
countApprovedToolsInGroups,
groupApprovalToolsByProvider,
listApprovalToolDefinitions,
listApprovalToolNames,
@@ -658,3 +659,101 @@ describe('probed tools', () => {
expect(names).toContain('mcp_server:Probed Keys');
});
});
describe('countApprovedToolsInGroups', () => {
const TS = '2026-03-28T00:00:00.000Z';
function makeTooling(
mcpServers: McpServerDefinition[] = [],
lspProfiles: LspProfileDefinition[] = [],
): WorkspaceToolingSettings {
return { mcpServers, lspProfiles };
}
function makeMcpServer(id: string, name: string, tools: string[]): McpServerDefinition {
return { id, name, transport: 'local', command: 'node', args: [], tools, createdAt: TS, updatedAt: TS };
}
test('counts all tools approved when all server keys are set', () => {
const tooling = makeTooling([
makeMcpServer('git', 'Git MCP', ['git.status', 'git.diff']),
makeMcpServer('fs', 'Filesystem', ['fs.read']),
]);
const tools = listApprovalToolDefinitions(tooling);
const groups = groupApprovalToolsByProvider(tools, tooling);
const mcpGroups = groups.filter((g) => g.kind === 'mcp');
const approved = new Set(['mcp_server:Git MCP', 'mcp_server:Filesystem']);
const mcpTotal = mcpGroups.reduce(
(sum, g) => sum + Math.max(g.tools.length, g.serverApprovalKey ? 1 : 0), 0,
);
const count = countApprovedToolsInGroups(mcpGroups, approved);
expect(count).toBe(mcpTotal);
});
test('counts shared tool names per group, not as unique IDs', () => {
const tooling = makeTooling([
makeMcpServer('git-a', 'Git A', ['git.status', 'git.diff']),
makeMcpServer('git-b', 'Git B', ['git.status', 'git.diff']),
]);
const tools = listApprovalToolDefinitions(tooling);
const groups = groupApprovalToolsByProvider(tools, tooling);
const mcpGroups = groups.filter((g) => g.kind === 'mcp');
expect(mcpGroups.length).toBe(2);
// Both servers share the same tool names — MCP total should be 4 (2 per group)
const mcpTotal = mcpGroups.reduce(
(sum, g) => sum + Math.max(g.tools.length, g.serverApprovalKey ? 1 : 0), 0,
);
expect(mcpTotal).toBe(4);
// Approve all via server keys — count must match total
const approved = new Set(['mcp_server:Git A', 'mcp_server:Git B']);
const count = countApprovedToolsInGroups(mcpGroups, approved);
expect(count).toBe(mcpTotal);
});
test('counts individual tool approvals per group when no server key', () => {
const tooling = makeTooling([
makeMcpServer('git-a', 'Git A', ['git.status']),
makeMcpServer('git-b', 'Git B', ['git.status']),
]);
const tools = listApprovalToolDefinitions(tooling);
const groups = groupApprovalToolsByProvider(tools, tooling);
// Approve only the individual tool ID (shared across both groups)
const approved = new Set(['git.status']);
const mcpGroups = groups.filter((g) => g.kind === 'mcp');
expect(mcpGroups.length).toBe(2);
// Each group has the same tool → count should be 2 (once per group)
const mcpCount = countApprovedToolsInGroups(mcpGroups, approved);
expect(mcpCount).toBe(2);
});
test('counts empty server with approved key as 1', () => {
const tooling = makeTooling([makeMcpServer('empty', 'Empty Server', [])]);
const tools = listApprovalToolDefinitions(tooling);
const groups = groupApprovalToolsByProvider(tools, tooling);
const approved = new Set(['mcp_server:Empty Server']);
const mcpGroups = groups.filter((g) => g.kind === 'mcp');
expect(mcpGroups.length).toBe(1);
expect(mcpGroups[0].tools.length).toBe(0);
const count = countApprovedToolsInGroups(mcpGroups, approved);
expect(count).toBe(1);
});
test('returns 0 when nothing is approved', () => {
const tooling = makeTooling([
makeMcpServer('git', 'Git MCP', ['git.status']),
]);
const tools = listApprovalToolDefinitions(tooling);
const groups = groupApprovalToolsByProvider(tools, tooling);
const approved = new Set<string>();
const count = countApprovedToolsInGroups(groups, approved);
expect(count).toBe(0);
});
});