-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add no-result permission handling for extensions #802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
b0c1b8e
9ae7eac
42474eb
5e31212
f1681db
1cb363a
0938b0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -51,6 +51,8 @@ import ( | |||||
| "github.com/github/copilot-sdk/go/rpc" | ||||||
| ) | ||||||
|
|
||||||
| const noResultPermissionV2Error = "permission handlers cannot return 'no-result' when connected to a protocol v2 server" | ||||||
|
||||||
| const noResultPermissionV2Error = "permission handlers cannot return 'no-result' when connected to a protocol v2 server" | |
| const noResultPermissionV2Error = "Permission handlers cannot return 'no-result' when connected to a protocol v2 server." |
Copilot
AI
Mar 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New behavior: v2 permission adapters now fail loudly when a handler returns no-result, but there is no Go test covering this path. Adding a unit test that exercises handlePermissionRequestV2 with a PermissionHandler.NoResult session would prevent regressions and confirm the JSON-RPC error message/code.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,6 +123,9 @@ const ( | |
|
|
||
| // PermissionRequestResultKindDeniedInteractivelyByUser indicates the permission was denied interactively by the user. | ||
| PermissionRequestResultKindDeniedInteractivelyByUser PermissionRequestResultKind = "denied-interactively-by-user" | ||
|
|
||
| // PermissionRequestResultKindNoResult indicates the SDK should not answer the pending permission request. | ||
| PermissionRequestResultKindNoResult PermissionRequestResultKind = "no-result" | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cross-SDK Consistency: Go is missing a named constant for Other SDKs provide well-known constants/static properties for all permission result kinds:
Suggestion: Add a named constant for consistency: // PermissionRequestResultKindNoResult indicates the permission handler chose not to respond
PermissionRequestResultKindNoResult PermissionRequestResultKind = "no-result"This would align with Go's existing pattern and make the constant available for developers (e.g., in extensions that want to explicitly return this kind). |
||
|
|
||
| // PermissionRequestResult represents the result of a permission request | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,10 +63,13 @@ import { approveAll } from "@github/copilot-sdk"; | |
| import { joinSession } from "@github/copilot-sdk/extension"; | ||
|
|
||
| await joinSession({ | ||
| onPermissionRequest: approveAll, // Required — handle permission requests | ||
| tools: [], // Optional — custom tools | ||
| hooks: {}, // Optional — lifecycle hooks | ||
| }); | ||
|
|
||
| // Optional — provide this if your extension should actively answer | ||
| // permission requests instead of leaving them pending for another client. | ||
| await joinSession({ onPermissionRequest: approveAll }); | ||
| ``` | ||
|
||
|
|
||
| --- | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -43,14 +43,17 @@ import { approveAll } from "@github/copilot-sdk"; | |||||||||||||||||||
| import { joinSession } from "@github/copilot-sdk/extension"; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const session = await joinSession({ | ||||||||||||||||||||
| onPermissionRequest: approveAll, | ||||||||||||||||||||
| tools: [ | ||||||||||||||||||||
| /* ... */ | ||||||||||||||||||||
| ], | ||||||||||||||||||||
| hooks: { | ||||||||||||||||||||
| /* ... */ | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Optional: override the default "no result" behavior if this extension | ||||||||||||||||||||
| // should actively answer permission requests itself. | ||||||||||||||||||||
| await joinSession({ onPermissionRequest: approveAll }); | ||||||||||||||||||||
|
||||||||||||||||||||
| }); | |
| // Optional: override the default "no result" behavior if this extension | |
| // should actively answer permission requests itself. | |
| await joinSession({ onPermissionRequest: approveAll }); | |
| // Optional: override the default "no result" behavior if this extension | |
| // should actively answer permission requests itself. | |
| onPermissionRequest: approveAll, | |
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The v2 adapter now throws when a permission handler returns
NoResult, but the .NET test suite here only validates the new kind constant. Adding a test that drivesOnPermissionRequestV2(or the underlying session handler) and asserts thatNoResultcauses an exception would lock in the intended “fail loudly” behavior.