.NET: Enforce ApprovalRequiredAIFunction in GitHub Copilot provider#6674
.NET: Enforce ApprovalRequiredAIFunction in GitHub Copilot provider#6674giles17 wants to merge 2 commits into
Conversation
The GitHub Copilot SDK owns the tool-calling loop and invokes registered custom functions directly, so the standard FunctionInvokingChatClient approval round-trip never runs for this provider. As a result a tool wrapped in ApprovalRequiredAIFunction (only a marker) could execute without any Agent Framework approval. Add an agent-level onFunctionApproval callback and wrap approval-required tools in an ApprovalGatedAIFunction that enforces approval before invoking the underlying function. Secure-by-default: with no callback, or when the callback denies or throws, execution is denied. The gate forwards tool metadata (including the Copilot skip_permission flag) so it stays transparent to the SDK. This mirrors the Python provider's behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR restores the Agent Framework tool-approval boundary for the .NET Microsoft.Agents.AI.GitHub.Copilot provider by wrapping ApprovalRequiredAIFunction tools in an approval-enforcing delegating function before they reach the Copilot SDK’s tool loop.
Changes:
- Added an optional
onFunctionApprovalcallback toGitHubCopilotAgentconstructors and applied tool wrapping viaWrapApprovalRequiredTools. - Introduced
ApprovalGatedAIFunctionto enforce approval checks at invocation time while preserving tool metadata (e.g.,skip_permission). - Added unit tests and an integration test validating deny-by-default, approve/deny/throw behaviors, and metadata preservation; suppressed
MEAI001where needed.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI.GitHub.Copilot/GitHubCopilotAgent.cs | Wraps approval-required tools and enforces approval in a delegating function before Copilot SDK invocation. |
| dotnet/src/Microsoft.Agents.AI.GitHub.Copilot/Microsoft.Agents.AI.GitHub.Copilot.csproj | Suppresses MEAI001 for experimental approval-required function API usage. |
| dotnet/tests/Microsoft.Agents.AI.GitHub.Copilot.UnitTests/GitHubCopilotAgentTests.cs | Adds unit tests covering deny-by-default, approve/deny/throw, skip-permission preservation, and pass-through behavior. |
| dotnet/tests/Microsoft.Agents.AI.GitHub.Copilot.UnitTests/Microsoft.Agents.AI.GitHub.Copilot.UnitTests.csproj | Suppresses MEAI001 for experimental API usage in tests. |
| dotnet/tests/Microsoft.Agents.AI.GitHub.Copilot.IntegrationTests/GitHubCopilotAgentTests.cs | Adds an integration test verifying approval-required tool execution when the callback approves. |
| dotnet/tests/Microsoft.Agents.AI.GitHub.Copilot.IntegrationTests/Microsoft.Agents.AI.GitHub.Copilot.IntegrationTests.csproj | Suppresses MEAI001 for experimental API usage in tests. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 87%
✓ Correctness
The PR is well-structured and the core approval-gate mechanism is correct. The type hierarchy (AprovalRequiredAIFunction → DelegatingAIFunction → AIFunction → AIFunctionDeclaration) ensures detection via
GetService<AprovalRequiredAIFunction>()works, metadata forwarding throughDelegatingAIFunctionis sound, and the secure-by-default deny policy is properly implemented. One correctness concern: thecatch (Exception)inIsApprovedAsyncswallowsOperationCanceledException, which breaks .NET's cooperative cancellation pattern — a cancelled operation would silently return a denial string instead of propagating the cancellation to the caller.
✓ Security Reliability
This PR correctly enforces the AprovalRequiredAIFunction contract for the GitHub Copilot provider. The security design is sound: secure-by-default denial when no callback is configured, when the callback returns false, and when the callback throws. The approval gate is properly applied at construction time and the inner class is private sealed, preventing bypasses. One reliability concern: the catch-all in IsApprovedAsync swallows OperationCanceledException, which prevents proper cancellation propagation — though this is a tradeoff against the secure-by-default posture, not a security hole.
✓ Test Coverage
Test coverage is thorough for the new approval-gating feature. All four approval decision paths (approve, deny, throw, no callback) are covered, metadata preservation is verified, and non-approval tool passthrough is tested. One minor gap: there is no test with a mixed tools list (both approval-required and regular tools in the same configuration), which is the realistic deployment scenario and exercises the
WrapApprovalRequiredToolsloop's per-tool branching in a single pass.
✗ Failure Modes
The implementation is solid overall — secure-by-default denial, proper metadata forwarding, clone-on-write for SessionConfig, and comprehensive test coverage. One concrete failure mode: the catch-all
catch (Exception)inIsApprovedAsync(line 688) silently swallowsOperationCanceledException, converting caller-requested cancellation into a tool-denial string instead of propagating it. This hides timeouts and cancellation from the SDK tool loop and the caller.
✓ Design Approach
The approval wrapper fixes the main bypass, but it currently collapses cancellation into a normal approval denial. That changes the control-flow contract for aborted runs and can surface a misleading "request was denied" result where the rest of the repo preserves OperationCanceledException.
Flagged Issues
-
catch (Exception)at GitHubCopilotAgent.cs:688 swallowsOperationCanceledException, silently converting cancellation into a denial string. Callers cancelling via CancellationToken will never observe the cancellation — the tool loop continues with a fake 'denied' result.
Automated review by giles17's agents
|
Flagged issue
Source: automated DevFlow PR review |
Let OperationCanceledException propagate from the approval callback instead of swallowing it into a denial, so cooperative cancellation is honored. Other callback failures still deny by default. Added a unit test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| string? description = null, | ||
| JsonSerializerOptions? jsonSerializerOptions = null) | ||
| JsonSerializerOptions? jsonSerializerOptions = null, | ||
| Func<FunctionCallContent, CancellationToken, ValueTask<bool>>? onFunctionApproval = null) |
There was a problem hiding this comment.
The SDK supports the OnPreToolUse hook, which seems to already provide the same functionality:
https://docs.github.com/en/copilot/how-tos/copilot-sdk/hooks/pre-tool-use
There is also the OnPermissionRequest hook. See: https://docs.github.com/en/copilot/how-tos/copilot-sdk/features/hooks
Do we really need our own?
Motivation & Context
The .NET
Microsoft.Agents.AI.GitHub.Copilotprovider did not honor the Agent Framework approval boundary for custom function tools wrapped inApprovalRequiredAIFunction. The GitHub Copilot SDK owns the model tool-calling loop and invokes registered custom functions directly, so the standardFunctionInvokingChatClientapproval round-trip (emitting aToolApprovalRequestContentand waiting for aToolApprovalResponseContent) never runs for this provider.ApprovalRequiredAIFunctionis only a marker and does not enforce approval on its own, so a function a developer explicitly marked as approval-required could execute with no Agent Framework approval.This restores the expected Tool Approval contract and brings the .NET provider in line with the Python GitHub Copilot provider, which already gates
approval_mode="always_require"tools through a dedicated function-approval callback.Description & Review Guide
What are the major changes?
onFunctionApprovalcallback to bothGitHubCopilotAgentconstructors, distinct fromSessionConfig.OnPermissionRequest(which gates the Copilot SDK's built-in shell/file actions).WrapApprovalRequiredTools, which replaces every tool resolving to anApprovalRequiredAIFunctionwith a new privateApprovalGatedAIFunctionbefore the tools reach the SDK. Non-approval tools pass through unchanged, and a caller-suppliedSessionConfigis only cloned when wrapping is needed.ApprovalGatedAIFunction(aDelegatingAIFunction) enforces approval inInvokeCoreAsyncand forwards all tool metadata (name, description, JSON schema, and additional properties such as the Copilotskip_permissionflag) so it stays transparent to the SDK.MEAI001(experimentalApprovalRequiredAIFunctionAPI) in the affected projects.What is the impact of these changes?
onFunctionApprovalcallback explicitly approves the specific call; a missing callback, or a callback that denies or throws, results in denial.What do you want reviewers to focus on?
WrapApprovalRequiredToolsand that the gate remains transparent to the SDK (metadata +skip_permissionpreserved).Related Issue
Fixes #6671
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.