Add no-result permission handling for extensions#802
Conversation
Add a public no-result permission outcome across the SDKs, default TypeScript extension joinSession() to it, and make v2 permission adapters fail loudly if asked to return no-result. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Cross-SDK Consistency ReviewI've reviewed this PR for consistency across all four SDK implementations (TypeScript, Python, Go, and .NET). The changes maintain excellent consistency! Verified Consistency✅ Type definitions: All SDKs define
✅ Helper functions: All SDKs provide equivalent helpers following naming conventions
✅ v3 behavior: All SDKs skip responding when handler returns ✅ v2 error handling: All SDKs throw/return an error with the same message when ✅ Tests: All SDKs include appropriate test coverage ✅ Documentation: TypeScript docs updated to explain the new default behavior for Language-Specific FeaturesThe TypeScript No consistency issues found. Great work maintaining API parity across all four implementations! 🎉
|
There was a problem hiding this comment.
Pull request overview
Adds a cross-SDK “no-result” permission outcome so clients (notably TypeScript extensions via joinSession()) can attach to an existing session without actively answering permission requests, while ensuring protocol v2 adapters fail loudly if no-result is returned.
Changes:
- Introduces a new
no-resultpermission outcome + helper handlers across TypeScript, Python, Go, and .NET. - Updates v3 permission handling to “do nothing” when
no-resultis returned, and updates v2 adapters to throw/fail loudly instead of silently converting it. - Updates TypeScript extension
joinSession()to defaultonPermissionRequesttonoResult, and adds/updates tests + docs around the new behavior.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| python/test_client.py | Adds Python unit tests for no-result helper + v2 adapter rejection. |
| python/copilot/types.py | Adds "no-result" to permission result kinds and a PermissionHandler.no_result helper. |
| python/copilot/session.py | Skips responding to v3 permission requests when handler returns no-result. |
| python/copilot/client.py | Makes v2 permission adapter reject no-result with a specific error. |
| nodejs/test/extension.test.ts | Adds tests validating joinSession() defaults onPermissionRequest to no-result. |
| nodejs/test/client.test.ts | Adds tests for v3 “no response” behavior and v2 “fail loudly” behavior. |
| nodejs/src/types.ts | Adds noResult helper + extends permission result union with { kind: "no-result" }. |
| nodejs/src/session.ts | Implements v3 “no response” and v2 “throw” behaviors for no-result. |
| nodejs/src/index.ts | Exposes noResult helper from the main package entrypoint. |
| nodejs/src/extension.ts | Makes joinSession() accept optional permission handler and defaults it to noResult. |
| nodejs/src/client.ts | Propagates v2 “no-result” errors instead of converting to deny. |
| nodejs/docs/extensions.md | Updates extension docs to reflect the new default permission behavior. |
| nodejs/docs/agent-author.md | Updates agent authoring docs to reflect the new default permission behavior. |
| go/types_test.go | Extends Go tests for the new kind constant + adds PermissionHandler.NoResult test. |
| go/types.go | Adds PermissionRequestResultKindNoResult. |
| go/session.go | Skips responding to v3 permission requests when handler returns NoResult. |
| go/permissions.go | Adds PermissionHandler.NoResult. |
| go/client.go | Makes v2 permission adapter fail loudly on NoResult. |
| dotnet/test/PermissionRequestResultKindTests.cs | Extends .NET tests for the new kind constant. |
| dotnet/src/Types.cs | Adds PermissionRequestResultKind.NoResult and documents it in PermissionRequestResult. |
| dotnet/src/Session.cs | Skips responding to v3 permission requests when handler returns NoResult. |
| dotnet/src/PermissionHandlers.cs | Adds PermissionHandler.NoResult. |
| dotnet/src/Client.cs | Makes v2 permission adapter throw when handler returns NoResult. |
nodejs/docs/agent-author.md
Outdated
| // Optional — provide this if your extension should actively answer | ||
| // permission requests instead of leaving them pending for another client. | ||
| await joinSession({ onPermissionRequest: approveAll }); | ||
| ``` |
There was a problem hiding this comment.
This skeleton example calls joinSession() twice; the second call reads like an in-place override but actually attaches a new client again. It would be clearer to present the onPermissionRequest override as an alternative snippet or as part of the initial joinSession({ ... }) options.
| "github.com/github/copilot-sdk/go/rpc" | ||
| ) | ||
|
|
||
| const noResultPermissionV2Error = "permission handlers cannot return 'no-result' when connected to a protocol v2 server" |
There was a problem hiding this comment.
The v2 error message here is inconsistent with the other SDKs in this PR (casing + missing trailing period). Aligning the exact string across languages makes cross-SDK docs and tests easier to keep consistent.
| 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." |
go/client.go
Outdated
| if result.Kind == PermissionRequestResultKindNoResult { | ||
| return nil, &jsonrpc2.Error{Code: -32603, Message: noResultPermissionV2Error} | ||
| } |
There was a problem hiding this comment.
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.
| var result = await session.HandlePermissionRequestAsync(permissionRequest); | ||
| if (result.Kind == PermissionRequestResultKind.NoResult) | ||
| { | ||
| throw new InvalidOperationException(NoResultPermissionV2ErrorMessage); | ||
| } | ||
| return new PermissionRequestResponseV2(result); |
There was a problem hiding this comment.
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 drives OnPermissionRequestV2 (or the underlying session handler) and asserts that NoResult causes an exception would lock in the intended “fail loudly” behavior.
nodejs/docs/extensions.md
Outdated
| }); | ||
|
|
||
| // Optional: override the default "no result" behavior if this extension | ||
| // should actively answer permission requests itself. | ||
| await joinSession({ onPermissionRequest: approveAll }); |
There was a problem hiding this comment.
The example calls joinSession() a second time to override permission handling, which is misleading: it creates a second client/session attachment rather than changing the options of the already-joined session. Consider showing this as an alternative snippet (e.g., "// or") or include onPermissionRequest in the original joinSession({ ... }) call.
| }); | |
| // 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, | |
| }); |
Avoid serializing absent optional session.send fields as JSON null so resumed Python-created sessions do not fail CLI validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove extension doc suggestions to pass onPermissionRequest: approveAll when using joinSession(), since extensions now default to no-result permission handling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep no-result handling internal to joinSession and raw permission result values instead of exposing simplified PermissionHandler helper APIs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the remaining public named no-result PermissionRequestResultKind conveniences and use raw no-result values internally instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency Review ✅This PR adds the ✅ What's Consistent
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #802
| // PermissionRequestResultKindDeniedInteractivelyByUser indicates the permission was denied interactively by the user. | ||
| PermissionRequestResultKindDeniedInteractivelyByUser PermissionRequestResultKind = "denied-interactively-by-user" | ||
|
|
||
| ) |
There was a problem hiding this comment.
Cross-SDK Consistency: Go is missing a named constant for "no-result".
Other SDKs provide well-known constants/static properties for all permission result kinds:
- .NET has
PermissionRequestResultKind.Approved,.DeniedByRules,.DeniedInteractivelyByUser, etc. - Python has
"no-result"in thePermissionRequestResultKindliteral - TypeScript uses a union type that includes
{ kind: "no-result" }
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).
| {"DeniedByRules", PermissionRequestResultKindDeniedByRules, "denied-by-rules"}, | ||
| {"DeniedCouldNotRequestFromUser", PermissionRequestResultKindDeniedCouldNotRequestFromUser, "denied-no-approval-rule-and-could-not-request-from-user"}, | ||
| {"DeniedInteractivelyByUser", PermissionRequestResultKindDeniedInteractivelyByUser, "denied-interactively-by-user"}, | ||
| {"NoResult", PermissionRequestResultKind("no-result"), "no-result"}, |
There was a problem hiding this comment.
Cross-SDK Consistency: Go is missing behavioral tests for the no-result permission outcome.
TypeScript has comprehensive tests:
- ✅
test/client.test.ts: "does not respond to v3 permission requests when handler returns no-result" - ✅
test/client.test.ts: "throws when a v2 permission handler returns no-result"
Python has:
- ✅
test_client.py: "test_v2_permission_adapter_rejects_no_result"
Suggestion: Add tests in Go to verify:
- When a v3 permission handler returns
"no-result",HandlePendingPermissionRequestis NOT called - When a v2 permission adapter receives
"no-result", it returns a JSON-RPC error with the message"permission handlers cannot return 'no-result' when connected to a protocol v2 server"
This ensures the behavior is tested and won't regress.
| Assert.Equal("denied-by-rules", PermissionRequestResultKind.DeniedByRules.Value); | ||
| Assert.Equal("denied-no-approval-rule-and-could-not-request-from-user", PermissionRequestResultKind.DeniedCouldNotRequestFromUser.Value); | ||
| Assert.Equal("denied-interactively-by-user", PermissionRequestResultKind.DeniedInteractivelyByUser.Value); | ||
| Assert.Equal("no-result", new PermissionRequestResultKind("no-result").Value); |
There was a problem hiding this comment.
Cross-SDK Consistency: .NET is missing behavioral tests for the no-result permission outcome.
TypeScript has comprehensive tests:
- ✅
test/client.test.ts: "does not respond to v3 permission requests when handler returns no-result" - ✅
test/client.test.ts: "throws when a v2 permission handler returns no-result"
Python has:
- ✅
test_client.py: "test_v2_permission_adapter_rejects_no_result"
Suggestion: Add tests to verify:
- When a v3 permission handler returns
new PermissionRequestResultKind("no-result"),HandlePendingPermissionRequestAsyncis NOT called - When a v2 permission adapter receives
no-result, it throwsInvalidOperationExceptionwith messageCopilotClient.NoResultPermissionV2ErrorMessage
Example test structure:
[Fact]
public async Task V3PermissionNoResult_DoesNotRespond()
{
// Similar to nodejs test: verify HandlePendingPermissionRequestAsync is not called
}
[Fact]
public async Task V2PermissionAdapter_RejectsNoResult()
{
// Verify OnPermissionRequestV2 throws InvalidOperationException when handler returns no-result
}| "permissionRequest": {"kind": "write"}, | ||
| } | ||
| ) | ||
| finally: |
There was a problem hiding this comment.
Cross-SDK Consistency: Python is missing a test for v3 no-result behavior.
TypeScript has a test verifying that when a v3 permission handler returns no-result, the SDK does NOT call handlePendingPermissionRequest:
- ✅
nodejs/test/client.test.ts: "does not respond to v3 permission requests when handler returns no-result"
Suggestion: Add a test to verify that _execute_permission_and_respond does NOT call handle_pending_permission_request when the handler returns {"kind": "no-result"}:
`@pytest`.mark.asyncio
async def test_v3_permission_no_result_does_not_respond(self):
client = CopilotClient({"cli_path": CLI_PATH})
await client.start()
try:
session = await client.create_session({
"on_permission_request": lambda request, invocation: {"kind": "no-result"}
})
# Spy on handle_pending_permission_request to ensure it's not called
from unittest.mock import AsyncMock
session._client.rpc.permissions.handle_pending_permission_request = AsyncMock()
await session._execute_permission_and_respond("request-1", {"kind": "write"})
session._client.rpc.permissions.handle_pending_permission_request.assert_not_called()
finally:
await client.force_stop()Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Cross-SDK Consistency ReviewI've reviewed this PR for consistency across all four SDK implementations (TypeScript, Python, Go, .NET), and the changes look excellent! The PR successfully adds the What Was Checked1. Type Definitions ✅All four SDKs properly define the new
2. V3 Permission Handling (Broadcast Events) ✅All SDKs correctly handle
3. V2 Permission Handling (RPC Adapters) ✅All SDKs throw/return errors when
The error message has minor casing differences in Go ( 4. Extension API Enhancement ✅The TypeScript SDK appropriately:
This is TypeScript-specific and aligns with the extension use case - other SDKs don't have an equivalent extension API, so no changes needed there. 5. Test Coverage ✅All SDKs include tests for the new behavior:
6. Documentation ✅TypeScript docs properly updated to reflect the new optional permission handler:
SummaryNo consistency issues found. This PR maintains excellent cross-SDK parity: ✅ Feature exists in all four SDKs Great work on maintaining consistency across all SDK implementations!
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SDK Consistency Review ✅This PR implements the Summary of ChangesThe PR adds three key features uniformly across all SDKs:
Consistency Analysis✅ Type Definitions: All SDKs expose the new permission kind
✅ V2 Adapter Error Handling: Consistent error message across all SDKs
✅ V3 Handler Behavior: Early return (no response sent) when ✅ Test Coverage: All SDKs include tests for the new behavior ✅ Documentation: TypeScript examples updated to reflect optional Minor SuggestionGo SDK API Design: For consistency with other permission kinds, consider adding an exported constant for const (
// ... existing constants ...
// PermissionRequestResultKindNoResult indicates the handler chose not to answer the request.
PermissionRequestResultKindNoResult PermissionRequestResultKind = "no-result"
)This would match the pattern used for This is a minor API design consistency point - the current implementation works correctly, but having the constant would provide better discoverability and IDE support for Go developers.
|
Summary
no-resultpermission outcome across the TypeScript, Python, Go, and .NET SDKsjoinSession()to anoResultpermission handler so extensions can attach without actively answering permission requestsno-result, and add tests/docs for the new behaviorTesting
cd go && go test .cd dotnet && dotnet test test/GitHub.Copilot.SDK.Test.csproj --filter PermissionRequestResultKindTestscd python && uv run pytest test_client.pycd nodejs && npm run typecheck && npm test -- test/client.test.ts test/extension.test.ts