From 26847cf00d95d89d9bddc9a4ec2c91f4147884ae Mon Sep 17 00:00:00 2001 From: David Kaya Date: Fri, 17 Apr 2026 14:21:53 +0200 Subject: [PATCH] fix: consume canonical approvalToolKey from sidecar Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/main/services/approvalCoordinator.ts | 7 +-- .../components/chat/ApprovalBanner.tsx | 4 +- src/shared/contracts/sidecar.ts | 1 + src/shared/domain/approval.ts | 18 ++++++++ tests/shared/approval.test.ts | 45 +++++++++++++++++++ 5 files changed, 70 insertions(+), 5 deletions(-) diff --git a/src/main/services/approvalCoordinator.ts b/src/main/services/approvalCoordinator.ts index 58e11ca..cf50858 100644 --- a/src/main/services/approvalCoordinator.ts +++ b/src/main/services/approvalCoordinator.ts @@ -7,9 +7,9 @@ import type { import { dequeuePendingApprovalState, enqueuePendingApprovalState, + getPendingApprovalToolKey, listPendingApprovals, resolvePendingApproval, - resolveApprovalToolKey, type ApprovalDecision, type PendingApprovalRecord, } from '@shared/domain/approval'; @@ -109,7 +109,7 @@ export class ApprovalCoordinator { this.setSessionPendingApprovalState(session, dequeuePendingApprovalState(session, approvalId)); session.updatedAt = resolvedAt; - const approvalKey = resolveApprovalToolKey(approval.toolName, approval.permissionKind); + const approvalKey = getPendingApprovalToolKey(approval); if (decision === 'approved' && alwaysApprove && approvalKey) { const existing = session.approvalSettings?.autoApprovedToolNames ?? []; if (!existing.includes(approvalKey)) { @@ -127,7 +127,7 @@ export class ApprovalCoordinator { continue; } - const queuedKey = resolveApprovalToolKey(queued.toolName, queued.permissionKind); + const queuedKey = getPendingApprovalToolKey(queued); if (queuedKey !== approvalKey) { continue; } @@ -361,6 +361,7 @@ export class ApprovalCoordinator { agentName: event.agentName, toolName: event.toolName, permissionKind: event.permissionKind, + approvalToolKey: event.approvalToolKey, title: event.title, detail: event.detail, permissionDetail: event.permissionDetail, diff --git a/src/renderer/components/chat/ApprovalBanner.tsx b/src/renderer/components/chat/ApprovalBanner.tsx index 1ab2702..573ebef 100644 --- a/src/renderer/components/chat/ApprovalBanner.tsx +++ b/src/renderer/components/chat/ApprovalBanner.tsx @@ -3,7 +3,7 @@ import { Bot, Check, ChevronDown, Loader2, ShieldAlert, ShieldBan, ShieldCheck, import { MarkdownContent } from '@renderer/components/MarkdownContent'; import { permissionDetailSummary, PermissionDetailView } from '@renderer/components/chat/PermissionDetailView'; -import { resolveApprovalToolKey } from '@shared/domain/approval'; +import { getPendingApprovalToolKey } from '@shared/domain/approval'; import type { ApprovalDecision, PendingApprovalRecord } from '@shared/domain/approval'; import { resolveToolLabel } from '@shared/domain/tooling'; @@ -25,7 +25,7 @@ export function ApprovalBanner({ const kindLabel = approval.kind === 'final-response' ? 'Final response review' : 'Tool call approval'; const hasMessages = approval.messages && approval.messages.length > 0; const showPosition = position !== undefined && total !== undefined && total > 1; - const approvalToolKey = resolveApprovalToolKey(approval.toolName, approval.permissionKind); + const approvalToolKey = getPendingApprovalToolKey(approval); const canAlwaysApprove = approval.kind === 'tool-call' && !!approvalToolKey; const approvalToolLabel = approvalToolKey ? resolveToolLabel(approvalToolKey) : undefined; diff --git a/src/shared/contracts/sidecar.ts b/src/shared/contracts/sidecar.ts index b984936..7ea5f4f 100644 --- a/src/shared/contracts/sidecar.ts +++ b/src/shared/contracts/sidecar.ts @@ -518,6 +518,7 @@ export interface ApprovalRequestedEvent { agentName?: string; toolName?: string; permissionKind?: string; + approvalToolKey?: string; title: string; detail?: string; permissionDetail?: PermissionDetail; diff --git a/src/shared/domain/approval.ts b/src/shared/domain/approval.ts index a3d2c20..8f3320f 100644 --- a/src/shared/domain/approval.ts +++ b/src/shared/domain/approval.ts @@ -34,6 +34,7 @@ export interface PendingApprovalRecord { agentName?: string; toolName?: string; permissionKind?: string; + approvalToolKey?: string; title: string; detail?: string; messages?: PendingApprovalMessageRecord[]; @@ -215,6 +216,22 @@ export function resolveApprovalToolKey( return toolName; } +/** + * Returns the canonical approval key for a pending approval. + * + * Prefers the explicit `approvalToolKey` supplied by the sidecar. Falls back to + * re-deriving the key from `toolName` and `permissionKind` to support legacy + * records persisted before the sidecar contract change. + */ +export function getPendingApprovalToolKey( + record: Pick, +): string | undefined { + return ( + normalizeOptionalString(record.approvalToolKey) + ?? resolveApprovalToolKey(record.toolName, record.permissionKind) + ); +} + export function approvalPolicyAutoApprovesTool( policy: ApprovalPolicy | undefined, toolName?: string, @@ -312,6 +329,7 @@ export function normalizePendingApproval( agentName: normalizeOptionalString(approval?.agentName), toolName: normalizeOptionalString(approval?.toolName), permissionKind: normalizeOptionalString(approval?.permissionKind), + approvalToolKey: normalizeOptionalString(approval?.approvalToolKey), title, detail: normalizeOptionalString(approval?.detail), messages: normalizePendingApprovalMessages(approval?.messages), diff --git a/tests/shared/approval.test.ts b/tests/shared/approval.test.ts index 31bb53d..b748f1d 100644 --- a/tests/shared/approval.test.ts +++ b/tests/shared/approval.test.ts @@ -10,6 +10,7 @@ import { normalizeSessionApprovalSettings, pruneSessionApprovalSettings, resolveApprovalToolKey, + getPendingApprovalToolKey, dequeuePendingApprovalState, enqueuePendingApprovalState, listPendingApprovals, @@ -261,4 +262,48 @@ describe('approval helpers', () => { expect(resolveApprovalToolKey(undefined, undefined)).toBeUndefined(); expect(resolveApprovalToolKey(undefined, 'mcp')).toBeUndefined(); }); + + test('getPendingApprovalToolKey prefers the explicit approval tool key from the sidecar', () => { + expect( + getPendingApprovalToolKey({ + approvalToolKey: 'write', + toolName: 'apply_patch', + permissionKind: 'hook', + }), + ).toBe('write'); + + expect( + getPendingApprovalToolKey({ + approvalToolKey: 'web_fetch', + toolName: 'web_search', + permissionKind: 'hook', + }), + ).toBe('web_fetch'); + + expect( + getPendingApprovalToolKey({ + approvalToolKey: 'mcp_server:icm-mcp', + toolName: 'icm-mcp-get_schedule', + permissionKind: 'mcp', + }), + ).toBe('mcp_server:icm-mcp'); + }); + + test('getPendingApprovalToolKey falls back to derivation for legacy approvals without the explicit key', () => { + expect( + getPendingApprovalToolKey({ + toolName: 'view', + permissionKind: 'read', + }), + ).toBe('read'); + + expect( + getPendingApprovalToolKey({ + toolName: 'git.status', + permissionKind: 'mcp', + }), + ).toBe('git.status'); + + expect(getPendingApprovalToolKey({})).toBeUndefined(); + }); });