diff --git a/sidecar/src/Aryx.AgentHost/Contracts/ProtocolModels.cs b/sidecar/src/Aryx.AgentHost/Contracts/ProtocolModels.cs index 82ae5eb..e727ebe 100644 --- a/sidecar/src/Aryx.AgentHost/Contracts/ProtocolModels.cs +++ b/sidecar/src/Aryx.AgentHost/Contracts/ProtocolModels.cs @@ -678,6 +678,7 @@ public sealed class ApprovalRequestedEventDto : SidecarEventDto public string? AgentName { get; init; } public string? ToolName { get; init; } public string? PermissionKind { get; init; } + public string? ApprovalToolKey { get; init; } public string Title { get; init; } = string.Empty; public string? Detail { get; init; } public PermissionDetailDto? PermissionDetail { get; init; } diff --git a/sidecar/src/Aryx.AgentHost/Services/Providers/Copilot/CopilotApprovalCoordinator.cs b/sidecar/src/Aryx.AgentHost/Services/Providers/Copilot/CopilotApprovalCoordinator.cs index 293094d..3125892 100644 --- a/sidecar/src/Aryx.AgentHost/Services/Providers/Copilot/CopilotApprovalCoordinator.cs +++ b/sidecar/src/Aryx.AgentHost/Services/Providers/Copilot/CopilotApprovalCoordinator.cs @@ -21,22 +21,33 @@ internal sealed class CopilotApprovalCoordinator private const string HookPermissionKind = "hook"; private const string ToolCallingActivityType = "tool-calling"; - private static readonly Dictionary HookToolCategories = new(StringComparer.OrdinalIgnoreCase) + private static readonly Dictionary HookToolApprovals = new(StringComparer.OrdinalIgnoreCase) { - ["view"] = ReadPermissionKind, - ["glob"] = ReadPermissionKind, - ["grep"] = ReadPermissionKind, - ["lsp"] = ReadPermissionKind, - ["edit"] = WritePermissionKind, - ["create"] = WritePermissionKind, - ["powershell"] = ShellPermissionKind, - ["read_powershell"] = ShellPermissionKind, - ["write_powershell"] = ShellPermissionKind, - ["stop_powershell"] = ShellPermissionKind, - ["list_powershell"] = ShellPermissionKind, - ["web_fetch"] = UrlPermissionKind, - ["web_search"] = UrlPermissionKind, - ["store_memory"] = MemoryPermissionKind, + ["view"] = new(ReadPermissionKind, ReadPermissionKind), + ["show_file"] = new(ReadPermissionKind, ReadPermissionKind), + ["read_file"] = new(ReadPermissionKind, ReadPermissionKind), + ["glob"] = new(ReadPermissionKind, ReadPermissionKind), + ["grep"] = new(ReadPermissionKind, ReadPermissionKind), + ["rg"] = new(ReadPermissionKind, ReadPermissionKind), + ["lsp"] = new(ReadPermissionKind, ReadPermissionKind), + ["edit"] = new(WritePermissionKind, WritePermissionKind), + ["create"] = new(WritePermissionKind, WritePermissionKind), + ["write_file"] = new(WritePermissionKind, WritePermissionKind), + ["apply_patch"] = new(WritePermissionKind, WritePermissionKind), + ["bash"] = new(ShellPermissionKind, ShellPermissionKind), + ["read_bash"] = new(ShellPermissionKind, ShellPermissionKind), + ["write_bash"] = new(ShellPermissionKind, ShellPermissionKind), + ["stop_bash"] = new(ShellPermissionKind, ShellPermissionKind), + ["list_bash"] = new(ShellPermissionKind, ShellPermissionKind), + ["powershell"] = new(ShellPermissionKind, ShellPermissionKind), + ["read_powershell"] = new(ShellPermissionKind, ShellPermissionKind), + ["write_powershell"] = new(ShellPermissionKind, ShellPermissionKind), + ["stop_powershell"] = new(ShellPermissionKind, ShellPermissionKind), + ["list_powershell"] = new(ShellPermissionKind, ShellPermissionKind), + ["web_fetch"] = new(UrlPermissionKind, WebFetchToolName), + ["web_search"] = new(UrlPermissionKind, WebFetchToolName), + ["store_memory"] = new(MemoryPermissionKind, StoreMemoryToolName), + ["remember_fact"] = new(MemoryPermissionKind, StoreMemoryToolName), }; private readonly ConcurrentDictionary _pendingApprovals = new(StringComparer.Ordinal); @@ -96,19 +107,22 @@ internal sealed class CopilotApprovalCoordinator Func onApproval, CancellationToken cancellationToken) { - string? toolName = ResolveApprovalToolName(request, toolCalls); - string? autoApprovedToolName = ResolveAutoApprovedToolName(request); - string? mcpServerApprovalKey = ResolveMcpServerApprovalKey(request, command.Tooling?.McpServers); - string? approvalCacheKey = ResolveApprovalCacheKey(toolName, autoApprovedToolName); + ResolvedApprovalContext approval = ResolveApprovalContext(request, toolCalls, command.Tooling?.McpServers); + string? approvalCacheKey = ResolveApprovalCacheKey(approval.ToolName, approval.ApprovalToolKey); - AgentActivityEventDto? fileChangeActivity = BuildToolCallFileChangeActivity(command, agent, request, toolName); + AgentActivityEventDto? fileChangeActivity = BuildToolCallFileChangeActivity(command, agent, request, approval.ToolName); if (fileChangeActivity is not null && onActivity is not null) { await onActivity(fileChangeActivity).ConfigureAwait(false); } if (IsToolApprovedForRequest(command.RequestId, approvalCacheKey) - || !RequiresToolCallApproval(command.Workflow.Settings.ApprovalPolicy, agent.GetAgentId(), toolName, autoApprovedToolName, mcpServerApprovalKey)) + || !RequiresToolCallApproval( + command.Workflow.Settings.ApprovalPolicy, + agent.GetAgentId(), + approval.ToolName, + approval.ApprovalToolKey, + approval.McpServerApprovalKey)) { return CreateApprovalResult(PermissionRequestResultKind.Approved); } @@ -127,7 +141,7 @@ internal sealed class CopilotApprovalCoordinator request, invocation, pending.ApprovalId, - toolName)) + approval.ToolName)) .ConfigureAwait(false); using CancellationTokenRegistration registration = cancellationToken.Register( @@ -155,21 +169,22 @@ internal sealed class CopilotApprovalCoordinator string approvalId, string? toolName) { - string permissionKind = ResolvePermissionKind(request, command.Tooling?.McpServers); + ResolvedApprovalContext approval = ResolveApprovalContext(request, toolName, command.Tooling?.McpServers); string agentId = agent.GetAgentId(); string agentName = agent.GetAgentName(); string? sessionId = NormalizeOptionalString(invocation.SessionId); - string? normalizedToolName = NormalizeOptionalString(toolName); + string? normalizedToolName = approval.ToolName; + string? displayToolName = normalizedToolName ?? NormalizeOptionalString(approval.ApprovalToolKey); string? requestedUrl = request is PermissionRequestUrl urlRequest ? NormalizeOptionalString(urlRequest.Url) : null; - string title = normalizedToolName is null - ? $"Approve {permissionKind}" - : $"Approve {normalizedToolName}"; - string detail = normalizedToolName is null - ? $"{agentName} requested {permissionKind} permission" - : $"{agentName} requested {permissionKind} permission for tool \"{normalizedToolName}\""; + string title = displayToolName is null + ? $"Approve {approval.PermissionKind}" + : $"Approve {displayToolName}"; + string detail = displayToolName is null + ? $"{agentName} requested {approval.PermissionKind} permission" + : $"{agentName} requested {approval.PermissionKind} permission for tool \"{displayToolName}\""; if (requestedUrl is not null) { @@ -195,7 +210,8 @@ internal sealed class CopilotApprovalCoordinator AgentId = NormalizeOptionalString(agentId), AgentName = NormalizeOptionalString(agentName), ToolName = normalizedToolName, - PermissionKind = permissionKind, + PermissionKind = approval.PermissionKind, + ApprovalToolKey = NormalizeOptionalString(approval.ApprovalToolKey), Title = title, Detail = detail, PermissionDetail = BuildPermissionDetail(request, command.Tooling?.McpServers), @@ -314,7 +330,7 @@ internal sealed class CopilotApprovalCoordinator ApprovalPolicyDto? approvalPolicy, string agentId, string? toolName, - string? autoApprovedToolName = null, + string? approvalToolKey = null, string? mcpServerApprovalKey = null) { if (approvalPolicy?.Rules is null || approvalPolicy.Rules.Count == 0) @@ -333,7 +349,7 @@ internal sealed class CopilotApprovalCoordinator return true; } - return !MatchesAutoApprovedTool(autoApprovedToolNames, toolName, autoApprovedToolName) + return !MatchesAutoApprovedTool(autoApprovedToolNames, toolName, approvalToolKey) && !MatchesAutoApprovedToolName(autoApprovedToolNames, mcpServerApprovalKey); } @@ -411,9 +427,67 @@ internal sealed class CopilotApprovalCoordinator ?? GetFallbackToolName(request); } - private static string? ResolveAutoApprovedToolName(PermissionRequest request) + private static ResolvedApprovalContext ResolveApprovalContext( + PermissionRequest request, + ToolCallRegistry? toolCalls, + IReadOnlyList? configuredMcpServers) { - return GetFallbackToolName(request); + return ResolveApprovalContext( + request, + ResolveApprovalToolName(request, toolCalls), + configuredMcpServers); + } + + private static ResolvedApprovalContext ResolveApprovalContext( + PermissionRequest request, + string? toolName, + IReadOnlyList? configuredMcpServers) + { + string? normalizedToolName = NormalizeOptionalString(toolName); + return request switch + { + PermissionRequestShell => new( + normalizedToolName, + ShellPermissionKind, + ShellPermissionKind, + null), + PermissionRequestWrite => new( + normalizedToolName, + WritePermissionKind, + WritePermissionKind, + null), + PermissionRequestRead => new( + normalizedToolName, + ReadPermissionKind, + ReadPermissionKind, + null), + PermissionRequestUrl => new( + normalizedToolName, + UrlPermissionKind, + WebFetchToolName, + null), + PermissionRequestMemory => new( + normalizedToolName, + MemoryPermissionKind, + StoreMemoryToolName, + null), + PermissionRequestMcp => new( + normalizedToolName, + McpPermissionKind, + normalizedToolName, + ResolveMcpServerApprovalKey(request, configuredMcpServers)), + PermissionRequestCustomTool => new( + normalizedToolName, + CustomToolPermissionKind, + normalizedToolName, + null), + PermissionRequestHook => ResolveHookApprovalContext(normalizedToolName, configuredMcpServers), + _ => new( + normalizedToolName, + NormalizeOptionalString(request.Kind) ?? "tool access", + normalizedToolName, + ResolveMcpServerApprovalKey(request, configuredMcpServers)), + }; } private const string McpServerApprovalPrefix = "mcp_server:"; @@ -522,44 +596,56 @@ internal sealed class CopilotApprovalCoordinator PermissionRequestWrite => WritePermissionKind, PermissionRequestRead => ReadPermissionKind, PermissionRequestMemory => StoreMemoryToolName, - PermissionRequestHook hook => ResolveHookToolCategory(hook.ToolName), + PermissionRequestHook hook => ResolveHookApprovalToolKey(hook.ToolName), _ => null, }; } internal static string? ResolveHookToolCategory(string? toolName) { - string? normalized = NormalizeOptionalString(toolName); - if (normalized is null) - { - return null; - } - - return HookToolCategories.TryGetValue(normalized, out string? category) ? category : null; + return TryResolveHookToolApproval(toolName, out HookToolApprovalMapping? mapping) && mapping is not null + ? mapping.PermissionKind + : null; } - private static string ResolvePermissionKind( - PermissionRequest request, + internal static string? ResolveHookApprovalToolKey(string? toolName) + { + return TryResolveHookToolApproval(toolName, out HookToolApprovalMapping? mapping) && mapping is not null + ? mapping.ApprovalToolKey + : null; + } + + internal static ResolvedApprovalContext ResolveHookApprovalContext( + string? toolName, IReadOnlyList? configuredMcpServers) { - string permissionKind = string.IsNullOrWhiteSpace(request.Kind) - ? "tool access" - : request.Kind.Trim(); - - if (request is not PermissionRequestHook hook) + string? normalizedToolName = NormalizeOptionalString(toolName); + if (normalizedToolName is null) { - return permissionKind; + return new ResolvedApprovalContext(null, HookPermissionKind, null, null); } - string? resolvedCategory = ResolveHookToolCategory(hook.ToolName); - if (resolvedCategory is not null) + if (TryResolveHookToolApproval(normalizedToolName, out HookToolApprovalMapping? mapping) && mapping is not null) { - return resolvedCategory; + return new ResolvedApprovalContext( + normalizedToolName, + mapping.PermissionKind, + mapping.ApprovalToolKey, + null); } - return ResolveHookMcpServerName(hook.ToolName, configuredMcpServers) is not null - ? McpPermissionKind - : permissionKind; + string? mcpServerApprovalKey = ResolveHookMcpServerApprovalKey(normalizedToolName, configuredMcpServers); + return mcpServerApprovalKey is not null + ? new ResolvedApprovalContext( + normalizedToolName, + McpPermissionKind, + normalizedToolName, + mcpServerApprovalKey) + : new ResolvedApprovalContext( + normalizedToolName, + HookPermissionKind, + normalizedToolName, + null); } private static PermissionDetailDto BuildHookPermissionDetail( @@ -619,13 +705,29 @@ internal sealed class CopilotApprovalCoordinator : strippedToolName; } + private static bool TryResolveHookToolApproval( + string? toolName, + out HookToolApprovalMapping? mapping) + { + string? normalizedToolName = NormalizeOptionalString(toolName); + if (normalizedToolName is not null + && HookToolApprovals.TryGetValue(normalizedToolName, out HookToolApprovalMapping? resolvedMapping)) + { + mapping = resolvedMapping; + return true; + } + + mapping = null; + return false; + } + private static bool MatchesAutoApprovedTool( IReadOnlyList autoApprovedToolNames, string? toolName, - string? autoApprovedToolName) + string? approvalToolKey) { return MatchesAutoApprovedToolName(autoApprovedToolNames, toolName) - || MatchesAutoApprovedToolName(autoApprovedToolNames, autoApprovedToolName); + || MatchesAutoApprovedToolName(autoApprovedToolNames, approvalToolKey); } private static bool MatchesAutoApprovedToolName( @@ -720,6 +822,16 @@ internal sealed class CopilotApprovalCoordinator return normalized.Count > 0 ? normalized : null; } + internal sealed record ResolvedApprovalContext( + string? ToolName, + string PermissionKind, + string? ApprovalToolKey, + string? McpServerApprovalKey); + + private sealed record HookToolApprovalMapping( + string PermissionKind, + string ApprovalToolKey); + private sealed record PendingApprovalRequest( string RequestId, string SessionId, diff --git a/sidecar/src/Aryx.AgentHost/Services/Providers/Copilot/CopilotSessionHooks.cs b/sidecar/src/Aryx.AgentHost/Services/Providers/Copilot/CopilotSessionHooks.cs index 6886186..266ce3f 100644 --- a/sidecar/src/Aryx.AgentHost/Services/Providers/Copilot/CopilotSessionHooks.cs +++ b/sidecar/src/Aryx.AgentHost/Services/Providers/Copilot/CopilotSessionHooks.cs @@ -248,17 +248,16 @@ internal static class CopilotSessionHooks }; } - string? autoApprovedToolName = CopilotApprovalCoordinator.ResolveHookToolCategory(toolName) ?? toolName; - string? mcpServerApprovalKey = CopilotApprovalCoordinator.ResolveHookMcpServerApprovalKey( + CopilotApprovalCoordinator.ResolvedApprovalContext approval = CopilotApprovalCoordinator.ResolveHookApprovalContext( toolName, command.Tooling?.McpServers); bool requiresApproval = CopilotApprovalCoordinator.RequiresToolCallApproval( command.Workflow.Settings.ApprovalPolicy, agentDefinition.GetAgentId(), - toolName, - autoApprovedToolName, - mcpServerApprovalKey); + approval.ToolName, + approval.ApprovalToolKey, + approval.McpServerApprovalKey); return new PreToolUseHookOutput { diff --git a/sidecar/tests/Aryx.AgentHost.Tests/CopilotSessionHooksTests.cs b/sidecar/tests/Aryx.AgentHost.Tests/CopilotSessionHooksTests.cs index 161e27a..a5ee2ec 100644 --- a/sidecar/tests/Aryx.AgentHost.Tests/CopilotSessionHooksTests.cs +++ b/sidecar/tests/Aryx.AgentHost.Tests/CopilotSessionHooksTests.cs @@ -154,11 +154,15 @@ public sealed class CopilotSessionHooksTests [Theory] [InlineData("view", "read")] [InlineData("grep", "read")] + [InlineData("rg", "read")] [InlineData("edit", "write")] + [InlineData("apply_patch", "write")] [InlineData("powershell", "shell")] - public async Task Create_PreToolUseAutoAllowsWhenCategoryIsApproved(string toolName, string category) + [InlineData("web_search", "web_fetch")] + [InlineData("store_memory", "store_memory")] + public async Task Create_PreToolUseAutoAllowsWhenApprovalToolKeyIsApproved(string toolName, string approvalToolKey) { - RunTurnCommandDto command = CreateCommandWithAutoApprovedCategory(category); + RunTurnCommandDto command = CreateCommandWithAutoApprovedCategory(approvalToolKey); SessionHooks hooks = CopilotSessionHooks.Create(command, command.Workflow.GetAgentNodes()[0], ResolvedHookSet.Empty, new RecordingHookCommandRunner()); PreToolUseHookOutput? decision = await hooks.OnPreToolUse!( diff --git a/sidecar/tests/Aryx.AgentHost.Tests/CopilotWorkflowRunnerTests.cs b/sidecar/tests/Aryx.AgentHost.Tests/CopilotWorkflowRunnerTests.cs index 4cb3405..d39aae8 100644 --- a/sidecar/tests/Aryx.AgentHost.Tests/CopilotWorkflowRunnerTests.cs +++ b/sidecar/tests/Aryx.AgentHost.Tests/CopilotWorkflowRunnerTests.cs @@ -1487,6 +1487,28 @@ public sealed class CopilotWorkflowRunnerTests Assert.True(CopilotApprovalCoordinator.RequiresToolCallApproval(policy, "agent-1", "git.status")); } + [Fact] + public void RequiresToolCallApproval_HonorsCanonicalHookApprovalKeysForModernToolNames() + { + ApprovalPolicyDto policy = new() + { + Rules = + [ + new ApprovalCheckpointRuleDto + { + Kind = "tool-call", + AgentIds = ["agent-1"], + }, + ], + AutoApprovedToolNames = ["read", "write", "web_fetch"], + }; + + Assert.False(CopilotApprovalCoordinator.RequiresToolCallApproval(policy, "agent-1", "rg", "read")); + Assert.False(CopilotApprovalCoordinator.RequiresToolCallApproval(policy, "agent-1", "apply_patch", "write")); + Assert.False(CopilotApprovalCoordinator.RequiresToolCallApproval(policy, "agent-1", "web_search", "web_fetch")); + Assert.True(CopilotApprovalCoordinator.RequiresToolCallApproval(policy, "agent-1", "powershell", "shell")); + } + [Fact] public void RequiresToolCallApproval_HonorsMcpServerLevelApprovalKey() { @@ -1738,6 +1760,7 @@ public sealed class CopilotWorkflowRunnerTests "lsp_ts_hover"); Assert.Equal("lsp_ts_hover", approvalEvent.ToolName); + Assert.Equal("lsp_ts_hover", approvalEvent.ApprovalToolKey); Assert.Equal("Approve lsp_ts_hover", approvalEvent.Title); Assert.Contains("tool \"lsp_ts_hover\"", approvalEvent.Detail); Assert.NotNull(approvalEvent.PermissionDetail); @@ -1770,6 +1793,7 @@ public sealed class CopilotWorkflowRunnerTests "web_fetch"); Assert.Equal("web_fetch", approvalEvent.ToolName); + Assert.Equal("web_fetch", approvalEvent.ApprovalToolKey); Assert.Equal("Approve web_fetch", approvalEvent.Title); Assert.Contains("url permission", approvalEvent.Detail); Assert.Contains("tool \"web_fetch\"", approvalEvent.Detail); @@ -1992,11 +2016,21 @@ public sealed class CopilotWorkflowRunnerTests [Theory] [InlineData("view", "read")] + [InlineData("show_file", "read")] + [InlineData("read_file", "read")] [InlineData("glob", "read")] [InlineData("grep", "read")] + [InlineData("rg", "read")] [InlineData("lsp", "read")] [InlineData("edit", "write")] [InlineData("create", "write")] + [InlineData("write_file", "write")] + [InlineData("apply_patch", "write")] + [InlineData("bash", "shell")] + [InlineData("read_bash", "shell")] + [InlineData("write_bash", "shell")] + [InlineData("stop_bash", "shell")] + [InlineData("list_bash", "shell")] [InlineData("powershell", "shell")] [InlineData("read_powershell", "shell")] [InlineData("write_powershell", "shell")] @@ -2005,6 +2039,7 @@ public sealed class CopilotWorkflowRunnerTests [InlineData("web_fetch", "url")] [InlineData("web_search", "url")] [InlineData("store_memory", "memory")] + [InlineData("remember_fact", "memory")] public void ResolveHookToolCategory_ReturnsExpectedCategoryForKnownTools(string toolName, string expectedCategory) { Assert.Equal(expectedCategory, CopilotApprovalCoordinator.ResolveHookToolCategory(toolName)); @@ -2027,6 +2062,29 @@ public sealed class CopilotWorkflowRunnerTests Assert.Null(CopilotApprovalCoordinator.ResolveHookToolCategory(" ")); } + [Theory] + [InlineData("view", "read")] + [InlineData("rg", "read")] + [InlineData("apply_patch", "write")] + [InlineData("bash", "shell")] + [InlineData("web_fetch", "web_fetch")] + [InlineData("web_search", "web_fetch")] + [InlineData("store_memory", "store_memory")] + [InlineData("remember_fact", "store_memory")] + public void ResolveHookApprovalToolKey_ReturnsExpectedKeyForKnownTools(string toolName, string expectedKey) + { + Assert.Equal(expectedKey, CopilotApprovalCoordinator.ResolveHookApprovalToolKey(toolName)); + } + + [Fact] + public void ResolveHookApprovalToolKey_ReturnsNullForUnknownTools() + { + Assert.Null(CopilotApprovalCoordinator.ResolveHookApprovalToolKey("custom_tool")); + Assert.Null(CopilotApprovalCoordinator.ResolveHookApprovalToolKey(null)); + Assert.Null(CopilotApprovalCoordinator.ResolveHookApprovalToolKey("")); + Assert.Null(CopilotApprovalCoordinator.ResolveHookApprovalToolKey(" ")); + } + [Fact] public void ResolveHookMcpServerApprovalKey_PrefersLongestConfiguredServerName() { @@ -2119,6 +2177,7 @@ public sealed class CopilotWorkflowRunnerTests "view"); Assert.Equal("view", approvalEvent.ToolName); + Assert.Equal("read", approvalEvent.ApprovalToolKey); Assert.Equal("read", approvalEvent.PermissionKind); Assert.Contains("read permission", approvalEvent.Detail); } @@ -2151,6 +2210,7 @@ public sealed class CopilotWorkflowRunnerTests "icm-mcp-get_schedule"); Assert.Equal("mcp", approvalEvent.PermissionKind); + Assert.Equal("icm-mcp-get_schedule", approvalEvent.ApprovalToolKey); Assert.Contains("mcp permission", approvalEvent.Detail); Assert.NotNull(approvalEvent.PermissionDetail); Assert.Equal("mcp", approvalEvent.PermissionDetail!.Kind); @@ -2182,6 +2242,7 @@ public sealed class CopilotWorkflowRunnerTests "icm-mcp-get_schedule"); Assert.Equal("hook", approvalEvent.PermissionKind); + Assert.Equal("icm-mcp-get_schedule", approvalEvent.ApprovalToolKey); } [Fact] @@ -2214,6 +2275,7 @@ public sealed class CopilotWorkflowRunnerTests Assert.False(pending.IsCompleted); Assert.NotNull(observedApproval); + Assert.Equal("lsp_ts_definition", observedApproval!.ApprovalToolKey); await coordinator.ResolveApprovalAsync( new ResolveApprovalCommandDto @@ -2270,6 +2332,8 @@ public sealed class CopilotWorkflowRunnerTests Assert.Equal("tool-calling", observedActivity!.ActivityType); Assert.Equal("apply_patch", observedActivity.ToolName); Assert.Equal("tool-call-write-1", observedActivity.ToolCallId); + Assert.Equal("apply_patch", observedApproval!.ToolName); + Assert.Equal("write", observedApproval.ApprovalToolKey); ToolCallFileChangeDto preview = Assert.Single(observedActivity.FileChanges!); Assert.Equal("README.md", preview.Path); @@ -2388,6 +2452,7 @@ public sealed class CopilotWorkflowRunnerTests Assert.False(firstPending.IsCompleted); Assert.NotNull(firstApproval); + Assert.Equal("read", firstApproval!.ApprovalToolKey); await coordinator.ResolveApprovalAsync( new ResolveApprovalCommandDto @@ -2428,6 +2493,82 @@ public sealed class CopilotWorkflowRunnerTests Assert.Equal(PermissionRequestResultKind.Approved, secondResult.Kind); } + [Fact] + public async Task RequestApprovalAsync_AlwaysApproveCachesCanonicalWriteApprovalForModernHookTools() + { + CopilotApprovalCoordinator coordinator = new(); + ApprovalRequestedEventDto? firstApproval = null; + RunTurnCommandDto command = CreateApprovalCommand(); + + Task firstPending = coordinator.RequestApprovalAsync( + command, + command.Workflow.GetAgentNodes()[0], + new PermissionRequestWrite + { + Kind = "write", + ToolCallId = "tool-call-write-1", + Intention = "Update the README", + FileName = "README.md", + Diff = "@@ -1 +1 @@", + NewFileContents = "# Aryx\n", + }, + new PermissionInvocation + { + SessionId = "copilot-session-1", + }, + CreateToolCallRegistry(("tool-call-write-1", "apply_patch")), + approval => + { + firstApproval = approval; + return Task.CompletedTask; + }, + CancellationToken.None); + + Assert.False(firstPending.IsCompleted); + Assert.NotNull(firstApproval); + Assert.Equal("write", firstApproval!.ApprovalToolKey); + + await coordinator.ResolveApprovalAsync( + new ResolveApprovalCommandDto + { + ApprovalId = firstApproval.ApprovalId, + Decision = "approved", + AlwaysApprove = true, + }, + CancellationToken.None); + + PermissionRequestResult firstResult = await firstPending; + Assert.Equal(PermissionRequestResultKind.Approved, firstResult.Kind); + + bool sawSecondApproval = false; + PermissionRequestResult secondResult = await coordinator.RequestApprovalAsync( + command, + command.Workflow.GetAgentNodes()[0], + new PermissionRequestWrite + { + Kind = "write", + ToolCallId = "tool-call-write-2", + Intention = "Create docs draft", + FileName = "docs\\guide.md", + Diff = "@@ -0,0 +1 @@", + NewFileContents = "# Guide\n", + }, + new PermissionInvocation + { + SessionId = "copilot-session-1", + }, + CreateToolCallRegistry(("tool-call-write-2", "write_file")), + approval => + { + sawSecondApproval = true; + return Task.CompletedTask; + }, + CancellationToken.None); + + Assert.False(sawSecondApproval); + Assert.Equal(PermissionRequestResultKind.Approved, secondResult.Kind); + } + [Fact] public async Task RequestApprovalAsync_AlwaysApproveCacheDoesNotCarryAcrossTurnRequests() { diff --git a/sidecar/tests/Aryx.AgentHost.Tests/SidecarProtocolHostTests.cs b/sidecar/tests/Aryx.AgentHost.Tests/SidecarProtocolHostTests.cs index 7b85013..17aa546 100644 --- a/sidecar/tests/Aryx.AgentHost.Tests/SidecarProtocolHostTests.cs +++ b/sidecar/tests/Aryx.AgentHost.Tests/SidecarProtocolHostTests.cs @@ -412,6 +412,7 @@ public sealed class SidecarProtocolHostTests AgentId = "agent-1", AgentName = "Primary", PermissionKind = "tool access", + ApprovalToolKey = "shell", Title = "Approve tool access", PermissionDetail = new PermissionDetailDto { @@ -437,6 +438,7 @@ public sealed class SidecarProtocolHostTests Assert.Equal("turn-approval", approvalEvent.GetProperty("requestId").GetString()); Assert.Equal("approval-1", approvalEvent.GetProperty("approvalId").GetString()); Assert.Equal("tool-call", approvalEvent.GetProperty("approvalKind").GetString()); + Assert.Equal("shell", approvalEvent.GetProperty("approvalToolKey").GetString()); Assert.Equal("Approve tool access", approvalEvent.GetProperty("title").GetString()); JsonElement permissionDetail = approvalEvent.GetProperty("permissionDetail"); Assert.Equal("shell", permissionDetail.GetProperty("kind").GetString()); 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(); + }); });