Skip to content

Commit 363ad33

Browse files
joshspicerCopilot
andcommitted
Fail approve-all in managed sessions
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 63c0e8f commit 363ad33

32 files changed

Lines changed: 245 additions & 126 deletions

dotnet/README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ using GitHub.Copilot;
3737
await using var client = new CopilotClient();
3838
await client.StartAsync();
3939

40-
// ApproveAll approves ordinary requests; managed requests still require a human decision.
40+
// ApproveAll is only valid when managed settings are disabled.
4141
await using var session = await client.CreateSessionAsync(new SessionConfig
4242
{
4343
Model = "gpt-5",
@@ -125,7 +125,7 @@ Create a new conversation session.
125125
- `Provider` - Custom API provider configuration (BYOK)
126126
- `Streaming` - Enable streaming of response chunks (default: false)
127127
- `InfiniteSessions` - Configure automatic context compaction (see below)
128-
- `OnPermissionRequest` - Optional handler called before each tool execution to approve or deny it. When omitted, permission requests are emitted as events and left pending for manual resolution. `PermissionHandler.ApproveAll` approves ordinary requests automatically; requests with `ManagedApprovalRequired == true` remain pending for explicit resolution through a human-facing host flow. See [Permission Handling](#permission-handling) section.
128+
- `OnPermissionRequest` - Optional handler called before each tool execution to approve or deny it. When omitted, permission requests are emitted as events and left pending for manual resolution. `PermissionHandler.ApproveAll` approves requests when managed settings are disabled and throws when `EnableManagedSettings` is true. Custom handlers can inspect `ManagedApprovalRequired` for human-facing confirmation logic. See [Permission Handling](#permission-handling) section.
129129
- `OnUserInputRequest` - Handler for user input requests from the agent (enables ask_user tool). See [User Input Requests](#user-input-requests) section.
130130
- `Hooks` - Hook handlers for session lifecycle events. See [Session Hooks](#session-hooks) section.
131131

dotnet/src/Client.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -783,7 +783,9 @@ private CopilotSession InitializeSession(
783783
_logger,
784784
this);
785785
session.RegisterTools(config.Tools ?? []);
786-
session.RegisterPermissionHandler(config.OnPermissionRequest);
786+
session.RegisterPermissionHandler(
787+
config.OnPermissionRequest,
788+
config.EnableManagedSettings is true);
787789
session.RegisterMcpAuthHandler(config.OnMcpAuthRequest);
788790
session.RegisterCommands(config.Commands);
789791
session.RegisterElicitationHandler(config.OnElicitationRequest);

dotnet/src/PermissionHandlers.cs

Lines changed: 5 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,22 +10,11 @@ namespace GitHub.Copilot;
1010
public static class PermissionHandler
1111
{
1212
/// <summary>
13-
/// A permission handler that approves ordinary requests and leaves managed
14-
/// requests pending for an explicit human decision.
13+
/// A permission handler that approves requests when managed settings are disabled.
1514
/// </summary>
1615
public static Func<PermissionRequest, PermissionInvocation, Task<PermissionDecision>> ApproveAll { get; } =
17-
(request, _) => Task.FromResult(
18-
RequiresManagedApproval(request)
19-
? PermissionDecision.NoResult()
20-
: PermissionDecision.ApproveOnce());
21-
22-
private static bool RequiresManagedApproval(PermissionRequest request) =>
23-
request switch
24-
{
25-
PermissionRequestShell { ManagedApprovalRequired: true } => true,
26-
PermissionRequestWrite { ManagedApprovalRequired: true } => true,
27-
PermissionRequestRead { ManagedApprovalRequired: true } => true,
28-
PermissionRequestUrl { ManagedApprovalRequired: true } => true,
29-
_ => false,
30-
};
16+
(_, invocation) => invocation.ManagedSettingsEnabled
17+
? Task.FromException<PermissionDecision>(
18+
new InvalidOperationException("ApproveAll cannot be used when managed settings are enabled"))
19+
: Task.FromResult(PermissionDecision.ApproveOnce());
3120
}

dotnet/src/Session.cs

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ public sealed partial class CopilotSession : IAsyncDisposable
6363
private readonly CopilotClient _parentClient;
6464

6565
private volatile Func<PermissionRequest, PermissionInvocation, Task<PermissionDecision>>? _permissionHandler;
66+
private bool _managedSettingsEnabled;
6667
private volatile Func<McpAuthContext, Task<McpAuthResult?>>? _mcpAuthHandler;
6768
private volatile Func<UserInputRequest, UserInputInvocation, Task<UserInputResponse>>? _userInputHandler;
6869
private volatile Func<ElicitationContext, Task<ElicitationResult>>? _elicitationHandler;
@@ -557,13 +558,17 @@ internal void RegisterTools(ICollection<AIFunctionDeclaration> tools)
557558
/// Registers a handler for permission requests.
558559
/// </summary>
559560
/// <param name="handler">The permission handler function.</param>
561+
/// <param name="managedSettingsEnabled">Whether managed settings are enabled for the session.</param>
560562
/// <remarks>
561563
/// When the assistant needs permission to perform certain actions (e.g., file operations),
562564
/// this handler is called to approve or deny the request.
563565
/// </remarks>
564-
internal void RegisterPermissionHandler(Func<PermissionRequest, PermissionInvocation, Task<PermissionDecision>>? handler)
566+
internal void RegisterPermissionHandler(
567+
Func<PermissionRequest, PermissionInvocation, Task<PermissionDecision>>? handler,
568+
bool managedSettingsEnabled)
565569
{
566570
_permissionHandler = handler;
571+
_managedSettingsEnabled = managedSettingsEnabled;
567572
}
568573

569574
internal void RegisterMcpAuthHandler(Func<McpAuthContext, Task<McpAuthResult?>>? handler)
@@ -590,7 +595,8 @@ internal async Task<PermissionDecision> HandlePermissionRequestAsync(JsonElement
590595

591596
var invocation = new PermissionInvocation
592597
{
593-
SessionId = SessionId
598+
SessionId = SessionId,
599+
ManagedSettingsEnabled = _managedSettingsEnabled
594600
};
595601

596602
var permissionTimestamp = Stopwatch.GetTimestamp();
@@ -932,7 +938,8 @@ private async Task ExecutePermissionAndRespondAsync(string requestId, Permission
932938
{
933939
var invocation = new PermissionInvocation
934940
{
935-
SessionId = SessionId
941+
SessionId = SessionId,
942+
ManagedSettingsEnabled = _managedSettingsEnabled
936943
};
937944

938945
var permissionTimestamp = Stopwatch.GetTimestamp();

dotnet/src/Types.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -833,6 +833,9 @@ public sealed class PermissionInvocation
833833
/// Identifier of the session that triggered the permission request.
834834
/// </summary>
835835
public string SessionId { get; set; } = string.Empty;
836+
837+
/// <summary>Whether managed settings are enabled for this session.</summary>
838+
public bool ManagedSettingsEnabled { get; set; }
836839
}
837840

838841
// ============================================================================

dotnet/test/Unit/PermissionHandlerTests.cs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public void PermissionEventExposesManagedApprovalRequired()
4141
}
4242

4343
[Fact]
44-
public async Task ApproveAllLeavesManagedRequestPending()
44+
public async Task ApproveAllThrowsWhenManagedSettingsEnabled()
4545
{
4646
var request = new PermissionRequestRead
4747
{
@@ -50,9 +50,11 @@ public async Task ApproveAllLeavesManagedRequestPending()
5050
Path = "/workspace/file.txt",
5151
};
5252

53-
var decision = await PermissionHandler.ApproveAll(request, new PermissionInvocation());
54-
55-
Assert.IsType<PermissionDecisionNoResult>(decision);
53+
await Assert.ThrowsAsync<InvalidOperationException>(() =>
54+
PermissionHandler.ApproveAll(request, new PermissionInvocation
55+
{
56+
ManagedSettingsEnabled = true,
57+
}));
5658
}
5759

5860
[Fact]

go/README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ func main() {
5555
}
5656
defer client.Stop()
5757

58-
// ApproveAll approves ordinary requests; managed requests still require a human decision.
58+
// ApproveAll is only valid when managed settings are disabled.
5959
session, err := client.CreateSession(context.Background(), &copilot.SessionConfig{
6060
Model: "gpt-5",
6161
OnPermissionRequest: copilot.PermissionHandler.ApproveAll,
@@ -215,7 +215,7 @@ Event types: `SessionLifecycleCreated`, `SessionLifecycleDeleted`, `SessionLifec
215215
- `Provider` (\*ProviderConfig): Custom API provider configuration (BYOK). See [Custom Providers](#custom-providers) section.
216216
- `Streaming` (*bool): Enable streaming delta events (nil = runtime default)
217217
- `InfiniteSessions` (\*InfiniteSessionConfig): Automatic context compaction configuration
218-
- `OnPermissionRequest` (PermissionHandlerFunc): Optional handler called before each tool execution to approve or deny it. When nil, permission requests are emitted as events and left pending for manual resolution. `copilot.PermissionHandler.ApproveAll` approves ordinary requests automatically; requests where `RequiresManagedApproval()` is `true` remain pending for explicit resolution through a human-facing host flow. See [Permission Handling](#permission-handling) section.
218+
- `OnPermissionRequest` (PermissionHandlerFunc): Optional handler called before each tool execution to approve or deny it. When nil, permission requests are emitted as events and left pending for manual resolution. `copilot.PermissionHandler.ApproveAll` approves requests when managed settings are disabled and returns an error when `EnableManagedSettings` is true. Custom handlers can inspect `RequiresManagedApproval()` for human-facing confirmation logic. See [Permission Handling](#permission-handling) section.
219219
- `OnUserInputRequest` (UserInputHandler): Handler for user input requests from the agent (enables ask_user tool). See [User Input Requests](#user-input-requests) section.
220220
- `Hooks` (\*SessionHooks): Hook handlers for session lifecycle events. See [Session Hooks](#session-hooks) section.
221221
- `Commands` ([]CommandDefinition): Slash-commands registered for this session. See [Commands](#commands) section.

go/client.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -907,6 +907,7 @@ func (c *Client) CreateSession(ctx context.Context, config *SessionConfig) (*Ses
907907
// routed to a registered session.
908908
initializeSession := func(sessionID string) (*Session, error) {
909909
s := newSession(sessionID, c.client, "")
910+
s.managedSettings = config.EnableManagedSettings != nil && *config.EnableManagedSettings
910911

911912
s.registerTools(config.Tools)
912913
s.registerPermissionHandler(config.OnPermissionRequest)
@@ -1228,6 +1229,7 @@ func (c *Client) ResumeSessionWithOptions(ctx context.Context, sessionID string,
12281229
// Create and register the session before issuing the RPC so that
12291230
// events emitted by the CLI (e.g. session.start) are not dropped.
12301231
session := newSession(sessionID, c.client, "")
1232+
session.managedSettings = config.EnableManagedSettings != nil && *config.EnableManagedSettings
12311233

12321234
session.registerTools(config.Tools)
12331235
session.registerPermissionHandler(config.OnPermissionRequest)

go/permissions.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,19 @@
11
package copilot
22

33
import (
4+
"errors"
5+
46
"github.com/github/copilot-sdk/go/rpc"
57
)
68

79
// PermissionHandler provides pre-built OnPermissionRequest implementations.
810
var PermissionHandler = struct {
9-
// ApproveAll approves ordinary permission requests. Requests that require
10-
// managed approval remain pending for an explicit human decision.
11+
// ApproveAll approves permission requests when managed settings are disabled.
1112
ApproveAll PermissionHandlerFunc
1213
}{
13-
ApproveAll: func(request PermissionRequest, _ PermissionInvocation) (rpc.PermissionDecision, error) {
14-
if request.RequiresManagedApproval() {
15-
return &rpc.PermissionDecisionNoResult{}, nil
14+
ApproveAll: func(_ PermissionRequest, invocation PermissionInvocation) (rpc.PermissionDecision, error) {
15+
if invocation.ManagedSettingsEnabled {
16+
return nil, errors.New("ApproveAll cannot be used when managed settings are enabled")
1617
}
1718
return &rpc.PermissionDecisionApproveOnce{}, nil
1819
},

go/permissions_test.go

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,16 @@ func TestPermissionEventExposesManagedApprovalRequired(t *testing.T) {
2828
}
2929
}
3030

31-
func TestApproveAllLeavesManagedRequestPending(t *testing.T) {
32-
required := true
31+
func TestApproveAllReturnsErrorWhenManagedSettingsEnabled(t *testing.T) {
3332
decision, err := copilot.PermissionHandler.ApproveAll(
34-
&copilot.PermissionRequestRead{ManagedApprovalRequired: &required},
35-
copilot.PermissionInvocation{SessionID: "session-1"},
33+
&copilot.PermissionRequestRead{},
34+
copilot.PermissionInvocation{SessionID: "session-1", ManagedSettingsEnabled: true},
3635
)
37-
if err != nil {
38-
t.Fatal(err)
36+
if err == nil {
37+
t.Fatal("expected an error")
3938
}
40-
if _, ok := decision.(*rpc.PermissionDecisionNoResult); !ok {
41-
t.Fatalf("expected PermissionDecisionNoResult, got %T", decision)
39+
if decision != nil {
40+
t.Fatalf("expected no decision, got %T", decision)
4241
}
4342
}
4443

0 commit comments

Comments
 (0)