fix: honor MCP server auto-approvals in hook flow

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
David Kaya
2026-03-31 10:11:15 +02:00
co-authored by Copilot
parent 36126f1c74
commit 4bc2c327f7
4 changed files with 323 additions and 28 deletions
@@ -98,7 +98,7 @@ internal sealed class CopilotApprovalCoordinator
{ {
string? toolName = ResolveApprovalToolName(request, toolNamesByCallId); string? toolName = ResolveApprovalToolName(request, toolNamesByCallId);
string? autoApprovedToolName = ResolveAutoApprovedToolName(request); string? autoApprovedToolName = ResolveAutoApprovedToolName(request);
string? mcpServerApprovalKey = ResolveMcpServerApprovalKey(request); string? mcpServerApprovalKey = ResolveMcpServerApprovalKey(request, command.Tooling?.McpServers);
string? approvalCacheKey = ResolveApprovalCacheKey(toolName, autoApprovedToolName); string? approvalCacheKey = ResolveApprovalCacheKey(toolName, autoApprovedToolName);
AgentActivityEventDto? fileChangeActivity = BuildToolCallFileChangeActivity(command, agent, request, toolName); AgentActivityEventDto? fileChangeActivity = BuildToolCallFileChangeActivity(command, agent, request, toolName);
@@ -155,18 +155,7 @@ internal sealed class CopilotApprovalCoordinator
string approvalId, string approvalId,
string? toolName) string? toolName)
{ {
string permissionKind = string.IsNullOrWhiteSpace(request.Kind) string permissionKind = ResolvePermissionKind(request, command.Tooling?.McpServers);
? "tool access"
: request.Kind.Trim();
if (request is PermissionRequestHook hook)
{
string? resolvedCategory = ResolveHookToolCategory(hook.ToolName);
if (resolvedCategory is not null)
{
permissionKind = resolvedCategory;
}
}
string agentName = string.IsNullOrWhiteSpace(agent.Name) ? agent.Id : agent.Name; string agentName = string.IsNullOrWhiteSpace(agent.Name) ? agent.Id : agent.Name;
string? sessionId = NormalizeOptionalString(invocation.SessionId); string? sessionId = NormalizeOptionalString(invocation.SessionId);
@@ -208,7 +197,7 @@ internal sealed class CopilotApprovalCoordinator
PermissionKind = permissionKind, PermissionKind = permissionKind,
Title = title, Title = title,
Detail = detail, 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<RunTurnMcpServerConfigDto>? configuredMcpServers = null)
{ {
ArgumentNullException.ThrowIfNull(request); ArgumentNullException.ThrowIfNull(request);
@@ -309,12 +300,7 @@ internal sealed class CopilotApprovalCoordinator
ToolDescription = NormalizeOptionalString(customTool.ToolDescription), ToolDescription = NormalizeOptionalString(customTool.ToolDescription),
Args = customTool.Args, Args = customTool.Args,
}, },
PermissionRequestHook hook => new PermissionDetailDto PermissionRequestHook hook => BuildHookPermissionDetail(hook, configuredMcpServers),
{
Kind = HookPermissionKind,
Args = hook.ToolArgs,
HookMessage = NormalizeOptionalString(hook.HookMessage),
},
_ => new PermissionDetailDto _ => new PermissionDetailDto
{ {
Kind = NormalizeOptionalString(request.Kind) ?? "unknown", Kind = NormalizeOptionalString(request.Kind) ?? "unknown",
@@ -430,15 +416,45 @@ internal sealed class CopilotApprovalCoordinator
private const string McpServerApprovalPrefix = "mcp_server:"; private const string McpServerApprovalPrefix = "mcp_server:";
private static string? ResolveMcpServerApprovalKey(PermissionRequest request) private static string? ResolveMcpServerApprovalKey(
PermissionRequest request,
IReadOnlyList<RunTurnMcpServerConfigDto>? 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<RunTurnMcpServerConfigDto>? configuredMcpServers)
=> BuildMcpServerApprovalKey(ResolveHookMcpServerName(toolName, configuredMcpServers));
internal static string? ResolveHookMcpServerName(
string? toolName,
IReadOnlyList<RunTurnMcpServerConfigDto>? configuredMcpServers)
{
string? normalizedToolName = NormalizeOptionalString(toolName);
if (normalizedToolName is null || configuredMcpServers is null || configuredMcpServers.Count == 0)
{ {
return null; return null;
} }
string? serverName = NormalizeOptionalString(mcp.ServerName); return configuredMcpServers
return serverName is not null ? $"{McpServerApprovalPrefix}{serverName}" : null; .Select(ResolveConfiguredMcpServerName)
.OfType<string>()
.Distinct(StringComparer.OrdinalIgnoreCase)
.OrderByDescending(static serverName => serverName.Length)
.FirstOrDefault(serverName => MatchesHookMcpServerToolName(normalizedToolName, serverName));
} }
private static string? ResolveApprovalCacheKey( private static string? ResolveApprovalCacheKey(
@@ -520,6 +536,87 @@ internal sealed class CopilotApprovalCoordinator
return HookToolCategories.TryGetValue(normalized, out string? category) ? category : null; return HookToolCategories.TryGetValue(normalized, out string? category) ? category : null;
} }
private static string ResolvePermissionKind(
PermissionRequest request,
IReadOnlyList<RunTurnMcpServerConfigDto>? 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<RunTurnMcpServerConfigDto>? 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( private static bool MatchesAutoApprovedTool(
IReadOnlyList<string> autoApprovedToolNames, IReadOnlyList<string> autoApprovedToolNames,
string? toolName, string? toolName,
@@ -249,12 +249,16 @@ internal static class CopilotSessionHooks
} }
string? autoApprovedToolName = CopilotApprovalCoordinator.ResolveHookToolCategory(toolName) ?? toolName; string? autoApprovedToolName = CopilotApprovalCoordinator.ResolveHookToolCategory(toolName) ?? toolName;
string? mcpServerApprovalKey = CopilotApprovalCoordinator.ResolveHookMcpServerApprovalKey(
toolName,
command.Tooling?.McpServers);
bool requiresApproval = CopilotApprovalCoordinator.RequiresToolCallApproval( bool requiresApproval = CopilotApprovalCoordinator.RequiresToolCallApproval(
command.Pattern.ApprovalPolicy, command.Pattern.ApprovalPolicy,
agentDefinition.Id, agentDefinition.Id,
toolName, toolName,
autoApprovedToolName); autoApprovedToolName,
mcpServerApprovalKey);
return new PreToolUseHookOutput return new PreToolUseHookOutput
{ {
@@ -171,6 +171,40 @@ public sealed class CopilotSessionHooksTests
Assert.Equal("allow", decision?.PermissionDecision); 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] [Fact]
public async Task Create_RunsConfiguredNonPreToolHooks() public async Task Create_RunsConfiguredNonPreToolHooks()
{ {
@@ -368,6 +402,43 @@ public sealed class CopilotSessionHooksTests
}; };
} }
private static RunTurnCommandDto CreateCommandWithConfiguredMcpServers(
IReadOnlyList<string> serverNames,
IReadOnlyList<string>? 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) private static HookCommandDefinition CreateHookCommand(string name)
=> new() => new()
{ {
@@ -1325,6 +1325,29 @@ public sealed class CopilotWorkflowRunnerTests
Assert.Equal("https://example.com", args["url"]); 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<string, object?>
{
["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<string, object?> args = Assert.IsType<Dictionary<string, object?>>(detail.Args);
Assert.Equal(769904783, args["incidentId"]);
}
[Theory] [Theory]
[InlineData("view", "read")] [InlineData("view", "read")]
[InlineData("glob", "read")] [InlineData("glob", "read")]
@@ -1362,6 +1385,16 @@ public sealed class CopilotWorkflowRunnerTests
Assert.Null(CopilotApprovalCoordinator.ResolveHookToolCategory(" ")); 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] [Fact]
public void TryGetApprovalToolName_ResolvesHookToolToCategory() public void TryGetApprovalToolName_ResolvesHookToolToCategory()
{ {
@@ -1448,6 +1481,41 @@ public sealed class CopilotWorkflowRunnerTests
Assert.Contains("read permission", approvalEvent.Detail); 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] [Fact]
public void BuildPermissionApprovalEvent_KeepsHookKindForUnknownHookTools() public void BuildPermissionApprovalEvent_KeepsHookKindForUnknownHookTools()
{ {
@@ -1613,6 +1681,43 @@ public sealed class CopilotWorkflowRunnerTests
Assert.Equal(PermissionRequestResultKind.Approved, result.Kind); 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<string, object?>
{
["incidentId"] = 769904783,
},
},
new PermissionInvocation
{
SessionId = "copilot-session-1",
},
new Dictionary<string, string>(StringComparer.Ordinal),
approval =>
{
sawApproval = true;
return Task.CompletedTask;
},
CancellationToken.None);
Assert.False(sawApproval);
Assert.Equal(PermissionRequestResultKind.Approved, result.Kind);
}
[Fact] [Fact]
public async Task RequestApprovalAsync_AlwaysApproveCachesRuntimeApprovalForCurrentTurn() public async Task RequestApprovalAsync_AlwaysApproveCachesRuntimeApprovalForCurrentTurn()
{ {
@@ -1853,12 +1958,21 @@ public sealed class CopilotWorkflowRunnerTests
null!); null!);
} }
private static RunTurnCommandDto CreateApprovalCommand(string requestId = "turn-1") private static RunTurnCommandDto CreateApprovalCommand(
string requestId = "turn-1",
IReadOnlyList<string>? autoApprovedToolNames = null,
IReadOnlyList<RunTurnMcpServerConfigDto>? mcpServers = null)
{ {
return new RunTurnCommandDto return new RunTurnCommandDto
{ {
RequestId = requestId, RequestId = requestId,
SessionId = "session-1", SessionId = "session-1",
Tooling = mcpServers is null
? null
: new RunTurnToolingConfigDto
{
McpServers = [.. mcpServers],
},
Pattern = new PatternDefinitionDto Pattern = new PatternDefinitionDto
{ {
Id = "pattern-1", Id = "pattern-1",
@@ -1875,7 +1989,9 @@ public sealed class CopilotWorkflowRunnerTests
AgentIds = ["agent-1"], AgentIds = ["agent-1"],
}, },
], ],
AutoApprovedToolNames = ["web_fetch"], AutoApprovedToolNames = autoApprovedToolNames is null
? ["web_fetch"]
: [.. autoApprovedToolNames],
}, },
Agents = 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 private sealed class StubChatClient : IChatClient
{ {
public Task<ChatResponse> GetResponseAsync( public Task<ChatResponse> GetResponseAsync(