diff --git a/sidecar/src/Aryx.AgentHost/Services/CopilotApprovalCoordinator.cs b/sidecar/src/Aryx.AgentHost/Services/CopilotApprovalCoordinator.cs index de903a3..fd0e29e 100644 --- a/sidecar/src/Aryx.AgentHost/Services/CopilotApprovalCoordinator.cs +++ b/sidecar/src/Aryx.AgentHost/Services/CopilotApprovalCoordinator.cs @@ -98,7 +98,7 @@ internal sealed class CopilotApprovalCoordinator { string? toolName = ResolveApprovalToolName(request, toolNamesByCallId); string? autoApprovedToolName = ResolveAutoApprovedToolName(request); - string? mcpServerApprovalKey = ResolveMcpServerApprovalKey(request); + string? mcpServerApprovalKey = ResolveMcpServerApprovalKey(request, command.Tooling?.McpServers); string? approvalCacheKey = ResolveApprovalCacheKey(toolName, autoApprovedToolName); AgentActivityEventDto? fileChangeActivity = BuildToolCallFileChangeActivity(command, agent, request, toolName); @@ -155,18 +155,7 @@ internal sealed class CopilotApprovalCoordinator string approvalId, string? toolName) { - string permissionKind = string.IsNullOrWhiteSpace(request.Kind) - ? "tool access" - : request.Kind.Trim(); - - if (request is PermissionRequestHook hook) - { - string? resolvedCategory = ResolveHookToolCategory(hook.ToolName); - if (resolvedCategory is not null) - { - permissionKind = resolvedCategory; - } - } + string permissionKind = ResolvePermissionKind(request, command.Tooling?.McpServers); string agentName = string.IsNullOrWhiteSpace(agent.Name) ? agent.Id : agent.Name; string? sessionId = NormalizeOptionalString(invocation.SessionId); @@ -208,7 +197,7 @@ internal sealed class CopilotApprovalCoordinator PermissionKind = permissionKind, Title = title, Detail = detail, - PermissionDetail = BuildPermissionDetail(request), + PermissionDetail = BuildPermissionDetail(request, command.Tooling?.McpServers), }; } @@ -252,7 +241,9 @@ internal sealed class CopilotApprovalCoordinator }; } - internal static PermissionDetailDto BuildPermissionDetail(PermissionRequest request) + internal static PermissionDetailDto BuildPermissionDetail( + PermissionRequest request, + IReadOnlyList? configuredMcpServers = null) { ArgumentNullException.ThrowIfNull(request); @@ -309,12 +300,7 @@ internal sealed class CopilotApprovalCoordinator ToolDescription = NormalizeOptionalString(customTool.ToolDescription), Args = customTool.Args, }, - PermissionRequestHook hook => new PermissionDetailDto - { - Kind = HookPermissionKind, - Args = hook.ToolArgs, - HookMessage = NormalizeOptionalString(hook.HookMessage), - }, + PermissionRequestHook hook => BuildHookPermissionDetail(hook, configuredMcpServers), _ => new PermissionDetailDto { Kind = NormalizeOptionalString(request.Kind) ?? "unknown", @@ -430,15 +416,45 @@ internal sealed class CopilotApprovalCoordinator private const string McpServerApprovalPrefix = "mcp_server:"; - private static string? ResolveMcpServerApprovalKey(PermissionRequest request) + private static string? ResolveMcpServerApprovalKey( + PermissionRequest request, + IReadOnlyList? configuredMcpServers) { - if (request is not PermissionRequestMcp mcp) + return request switch + { + PermissionRequestMcp mcp => BuildMcpServerApprovalKey(mcp.ServerName), + PermissionRequestHook hook => ResolveHookMcpServerApprovalKey(hook.ToolName, configuredMcpServers), + _ => null, + }; + } + + internal static string? BuildMcpServerApprovalKey(string? serverName) + { + string? normalizedServerName = NormalizeOptionalString(serverName); + return normalizedServerName is not null ? $"{McpServerApprovalPrefix}{normalizedServerName}" : null; + } + + internal static string? ResolveHookMcpServerApprovalKey( + string? toolName, + IReadOnlyList? configuredMcpServers) + => BuildMcpServerApprovalKey(ResolveHookMcpServerName(toolName, configuredMcpServers)); + + internal static string? ResolveHookMcpServerName( + string? toolName, + IReadOnlyList? configuredMcpServers) + { + string? normalizedToolName = NormalizeOptionalString(toolName); + if (normalizedToolName is null || configuredMcpServers is null || configuredMcpServers.Count == 0) { return null; } - string? serverName = NormalizeOptionalString(mcp.ServerName); - return serverName is not null ? $"{McpServerApprovalPrefix}{serverName}" : null; + return configuredMcpServers + .Select(ResolveConfiguredMcpServerName) + .OfType() + .Distinct(StringComparer.OrdinalIgnoreCase) + .OrderByDescending(static serverName => serverName.Length) + .FirstOrDefault(serverName => MatchesHookMcpServerToolName(normalizedToolName, serverName)); } private static string? ResolveApprovalCacheKey( @@ -520,6 +536,87 @@ internal sealed class CopilotApprovalCoordinator return HookToolCategories.TryGetValue(normalized, out string? category) ? category : null; } + private static string ResolvePermissionKind( + PermissionRequest request, + IReadOnlyList? configuredMcpServers) + { + string permissionKind = string.IsNullOrWhiteSpace(request.Kind) + ? "tool access" + : request.Kind.Trim(); + + if (request is not PermissionRequestHook hook) + { + return permissionKind; + } + + string? resolvedCategory = ResolveHookToolCategory(hook.ToolName); + if (resolvedCategory is not null) + { + return resolvedCategory; + } + + return ResolveHookMcpServerName(hook.ToolName, configuredMcpServers) is not null + ? McpPermissionKind + : permissionKind; + } + + private static PermissionDetailDto BuildHookPermissionDetail( + PermissionRequestHook hook, + IReadOnlyList? configuredMcpServers) + { + string? serverName = ResolveHookMcpServerName(hook.ToolName, configuredMcpServers); + if (serverName is null) + { + return new PermissionDetailDto + { + Kind = HookPermissionKind, + Args = hook.ToolArgs, + HookMessage = NormalizeOptionalString(hook.HookMessage), + }; + } + + return new PermissionDetailDto + { + Kind = McpPermissionKind, + ServerName = serverName, + ToolTitle = ResolveHookMcpToolTitle(hook.ToolName, serverName), + Args = hook.ToolArgs, + }; + } + + private static string? ResolveConfiguredMcpServerName(RunTurnMcpServerConfigDto configuredServer) + => NormalizeOptionalString(configuredServer.Name) ?? NormalizeOptionalString(configuredServer.Id); + + private static bool MatchesHookMcpServerToolName(string toolName, string serverName) + { + if (string.Equals(toolName, serverName, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + + return toolName.StartsWith($"{serverName}-", StringComparison.OrdinalIgnoreCase); + } + + private static string? ResolveHookMcpToolTitle(string? toolName, string serverName) + { + string? normalizedToolName = NormalizeOptionalString(toolName); + if (normalizedToolName is null) + { + return null; + } + + string prefix = $"{serverName}-"; + if (!normalizedToolName.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) + { + return normalizedToolName; + } + + string strippedToolName = normalizedToolName[prefix.Length..]; + return string.IsNullOrWhiteSpace(strippedToolName) + ? normalizedToolName + : strippedToolName; + } + private static bool MatchesAutoApprovedTool( IReadOnlyList autoApprovedToolNames, string? toolName, diff --git a/sidecar/src/Aryx.AgentHost/Services/CopilotSessionHooks.cs b/sidecar/src/Aryx.AgentHost/Services/CopilotSessionHooks.cs index acd03a7..f4be24f 100644 --- a/sidecar/src/Aryx.AgentHost/Services/CopilotSessionHooks.cs +++ b/sidecar/src/Aryx.AgentHost/Services/CopilotSessionHooks.cs @@ -249,12 +249,16 @@ internal static class CopilotSessionHooks } string? autoApprovedToolName = CopilotApprovalCoordinator.ResolveHookToolCategory(toolName) ?? toolName; + string? mcpServerApprovalKey = CopilotApprovalCoordinator.ResolveHookMcpServerApprovalKey( + toolName, + command.Tooling?.McpServers); bool requiresApproval = CopilotApprovalCoordinator.RequiresToolCallApproval( command.Pattern.ApprovalPolicy, agentDefinition.Id, toolName, - autoApprovedToolName); + autoApprovedToolName, + mcpServerApprovalKey); return new PreToolUseHookOutput { diff --git a/sidecar/tests/Aryx.AgentHost.Tests/CopilotSessionHooksTests.cs b/sidecar/tests/Aryx.AgentHost.Tests/CopilotSessionHooksTests.cs index e5e2f8a..d91727a 100644 --- a/sidecar/tests/Aryx.AgentHost.Tests/CopilotSessionHooksTests.cs +++ b/sidecar/tests/Aryx.AgentHost.Tests/CopilotSessionHooksTests.cs @@ -171,6 +171,40 @@ public sealed class CopilotSessionHooksTests Assert.Equal("allow", decision?.PermissionDecision); } + [Fact] + public async Task Create_PreToolUseAutoAllowsWhenMcpServerIsApproved() + { + RunTurnCommandDto command = CreateCommandWithConfiguredMcpServers( + ["icm-mcp"], + ["mcp_server:icm-mcp"]); + SessionHooks hooks = CopilotSessionHooks.Create(command, command.Pattern.Agents[0], ResolvedHookSet.Empty, new RecordingHookCommandRunner()); + + PreToolUseHookOutput? decision = await hooks.OnPreToolUse!( + new PreToolUseHookInput + { + ToolName = "icm-mcp-get_incident_details_by_id", + }, + null!); + + Assert.Equal("allow", decision?.PermissionDecision); + } + + [Fact] + public async Task Create_PreToolUseRequiresApprovalWhenMcpServerIsNotApproved() + { + RunTurnCommandDto command = CreateCommandWithConfiguredMcpServers(["icm-mcp"]); + SessionHooks hooks = CopilotSessionHooks.Create(command, command.Pattern.Agents[0], ResolvedHookSet.Empty, new RecordingHookCommandRunner()); + + PreToolUseHookOutput? decision = await hooks.OnPreToolUse!( + new PreToolUseHookInput + { + ToolName = "icm-mcp-get_incident_details_by_id", + }, + null!); + + Assert.Equal("ask", decision?.PermissionDecision); + } + [Fact] public async Task Create_RunsConfiguredNonPreToolHooks() { @@ -368,6 +402,43 @@ public sealed class CopilotSessionHooksTests }; } + private static RunTurnCommandDto CreateCommandWithConfiguredMcpServers( + IReadOnlyList serverNames, + IReadOnlyList? autoApprovedToolNames = null) + { + RunTurnCommandDto command = CreateCommandWithToolApproval(); + return new RunTurnCommandDto + { + RequestId = command.RequestId, + SessionId = command.SessionId, + ProjectPath = command.ProjectPath, + Tooling = new RunTurnToolingConfigDto + { + McpServers = [.. serverNames.Select(CreateMcpServerConfig)], + }, + Pattern = new PatternDefinitionDto + { + Id = command.Pattern.Id, + Name = command.Pattern.Name, + Mode = command.Pattern.Mode, + Availability = command.Pattern.Availability, + ApprovalPolicy = new ApprovalPolicyDto + { + Rules = command.Pattern.ApprovalPolicy?.Rules ?? [], + AutoApprovedToolNames = autoApprovedToolNames ?? [], + }, + Agents = command.Pattern.Agents, + }, + }; + } + + private static RunTurnMcpServerConfigDto CreateMcpServerConfig(string serverName) + => new() + { + Id = serverName, + Name = serverName, + }; + private static HookCommandDefinition CreateHookCommand(string name) => new() { diff --git a/sidecar/tests/Aryx.AgentHost.Tests/CopilotWorkflowRunnerTests.cs b/sidecar/tests/Aryx.AgentHost.Tests/CopilotWorkflowRunnerTests.cs index f359a43..773b8e0 100644 --- a/sidecar/tests/Aryx.AgentHost.Tests/CopilotWorkflowRunnerTests.cs +++ b/sidecar/tests/Aryx.AgentHost.Tests/CopilotWorkflowRunnerTests.cs @@ -1325,6 +1325,29 @@ public sealed class CopilotWorkflowRunnerTests Assert.Equal("https://example.com", args["url"]); } + [Fact] + public void BuildPermissionDetail_MapsConfiguredMcpHookToMcpDetail() + { + PermissionDetailDto detail = CopilotApprovalCoordinator.BuildPermissionDetail( + new PermissionRequestHook + { + Kind = "hook", + ToolName = "icm-mcp-get_incident_details_by_id", + ToolArgs = new Dictionary + { + ["incidentId"] = 769904783, + }, + }, + [CreateMcpServerConfig("icm-mcp")]); + + Assert.Equal("mcp", detail.Kind); + Assert.Equal("icm-mcp", detail.ServerName); + Assert.Equal("get_incident_details_by_id", detail.ToolTitle); + + Dictionary args = Assert.IsType>(detail.Args); + Assert.Equal(769904783, args["incidentId"]); + } + [Theory] [InlineData("view", "read")] [InlineData("glob", "read")] @@ -1362,6 +1385,16 @@ public sealed class CopilotWorkflowRunnerTests Assert.Null(CopilotApprovalCoordinator.ResolveHookToolCategory(" ")); } + [Fact] + public void ResolveHookMcpServerApprovalKey_PrefersLongestConfiguredServerName() + { + string? approvalKey = CopilotApprovalCoordinator.ResolveHookMcpServerApprovalKey( + "icm-mcp-get_on_call_schedule", + [CreateMcpServerConfig("icm"), CreateMcpServerConfig("icm-mcp")]); + + Assert.Equal("mcp_server:icm-mcp", approvalKey); + } + [Fact] public void TryGetApprovalToolName_ResolvesHookToolToCategory() { @@ -1448,6 +1481,41 @@ public sealed class CopilotWorkflowRunnerTests Assert.Contains("read permission", approvalEvent.Detail); } + [Fact] + public void BuildPermissionApprovalEvent_UsesMcpKindForConfiguredMcpHookTools() + { + ApprovalRequestedEventDto approvalEvent = CopilotApprovalCoordinator.BuildPermissionApprovalEvent( + new RunTurnCommandDto + { + RequestId = "turn-1", + SessionId = "session-1", + Tooling = new RunTurnToolingConfigDto + { + McpServers = [CreateMcpServerConfig("icm-mcp")], + }, + }, + CreateAgent("agent-1", "Primary"), + new PermissionRequestHook + { + Kind = "hook", + ToolName = "icm-mcp-get_schedule", + ToolArgs = """{"teamIds":[91982]}""", + }, + new PermissionInvocation + { + SessionId = "copilot-session-1", + }, + "approval-1", + "icm-mcp-get_schedule"); + + Assert.Equal("mcp", approvalEvent.PermissionKind); + Assert.Contains("mcp permission", approvalEvent.Detail); + Assert.NotNull(approvalEvent.PermissionDetail); + Assert.Equal("mcp", approvalEvent.PermissionDetail!.Kind); + Assert.Equal("icm-mcp", approvalEvent.PermissionDetail.ServerName); + Assert.Equal("get_schedule", approvalEvent.PermissionDetail.ToolTitle); + } + [Fact] public void BuildPermissionApprovalEvent_KeepsHookKindForUnknownHookTools() { @@ -1613,6 +1681,43 @@ public sealed class CopilotWorkflowRunnerTests Assert.Equal(PermissionRequestResultKind.Approved, result.Kind); } + [Fact] + public async Task RequestApprovalAsync_AutoApprovesHookRequestsForApprovedMcpServer() + { + CopilotApprovalCoordinator coordinator = new(); + bool sawApproval = false; + RunTurnCommandDto command = CreateApprovalCommand( + autoApprovedToolNames: ["mcp_server:icm-mcp"], + mcpServers: [CreateMcpServerConfig("icm-mcp")]); + + PermissionRequestResult result = await coordinator.RequestApprovalAsync( + command, + command.Pattern.Agents[0], + new PermissionRequestHook + { + Kind = "hook", + ToolName = "icm-mcp-get_incident_details_by_id", + ToolArgs = new Dictionary + { + ["incidentId"] = 769904783, + }, + }, + new PermissionInvocation + { + SessionId = "copilot-session-1", + }, + new Dictionary(StringComparer.Ordinal), + approval => + { + sawApproval = true; + return Task.CompletedTask; + }, + CancellationToken.None); + + Assert.False(sawApproval); + Assert.Equal(PermissionRequestResultKind.Approved, result.Kind); + } + [Fact] public async Task RequestApprovalAsync_AlwaysApproveCachesRuntimeApprovalForCurrentTurn() { @@ -1853,12 +1958,21 @@ public sealed class CopilotWorkflowRunnerTests null!); } - private static RunTurnCommandDto CreateApprovalCommand(string requestId = "turn-1") + private static RunTurnCommandDto CreateApprovalCommand( + string requestId = "turn-1", + IReadOnlyList? autoApprovedToolNames = null, + IReadOnlyList? mcpServers = null) { return new RunTurnCommandDto { RequestId = requestId, SessionId = "session-1", + Tooling = mcpServers is null + ? null + : new RunTurnToolingConfigDto + { + McpServers = [.. mcpServers], + }, Pattern = new PatternDefinitionDto { Id = "pattern-1", @@ -1875,7 +1989,9 @@ public sealed class CopilotWorkflowRunnerTests AgentIds = ["agent-1"], }, ], - AutoApprovedToolNames = ["web_fetch"], + AutoApprovedToolNames = autoApprovedToolNames is null + ? ["web_fetch"] + : [.. autoApprovedToolNames], }, Agents = [ @@ -1885,6 +2001,13 @@ public sealed class CopilotWorkflowRunnerTests }; } + private static RunTurnMcpServerConfigDto CreateMcpServerConfig(string serverName) + => new() + { + Id = serverName, + Name = serverName, + }; + private sealed class StubChatClient : IChatClient { public Task GetResponseAsync(