Require permission handler on session creation

[SteveSandersonMS]

Summary

Requires an onPermissionRequest / OnPermissionRequest / on_permission_request handler when creating or resuming sessions across all 4 SDKs (Node.js, Python, Go, .NET). Previously, omitting the handler automatically denied all permission requests, which created a bad experience for the developer when upgrading.

Usage

Node.js/TypeScript

import { CopilotClient, approveAll } from "@github/copilot-sdk";

const client = new CopilotClient();
const session = await client.createSession({ onPermissionRequest: approveAll });

Python

from copilot import CopilotClient, PermissionHandler

client = CopilotClient()
await client.start()
session = await client.create_session({"on_permission_request": PermissionHandler.approve_all})

Go

client := copilot.NewClient(nil)
session, err := client.CreateSession(ctx, &copilot.SessionConfig{
    OnPermissionRequest: copilot.PermissionHandler.ApproveAll,
})

C#

using var client = new CopilotClient();
var session = await client.CreateSessionAsync(new() { OnPermissionRequest = PermissionHandler.ApproveAll });

Changes

API changes

• Node.js/TypeScript: createSession(config) and resumeSession(id, config) now require config (no default). onPermissionRequest is required in the SessionConfig type.
• Python: create_session(config) and resume_session(id, config) now require config (no default None). Runtime validation ensures on_permission_request is set.
• Go: CreateSession and ResumeSessionWithOptions return an error if config is nil or OnPermissionRequest is not set.
• C#: CreateSessionAsync(config) and ResumeSessionAsync(id, config) now require config (no default null). Runtime validation ensures OnPermissionRequest is set.

Error messages

Each SDK throws/returns a clear error guiding the user to set the handler, e.g.:

An onPermissionRequest handler is required when creating a session. For example, to allow all permissions, use { onPermissionRequest: approveAll }.

Tests

• Added unit tests in all 4 SDKs verifying the error is thrown when the permission handler is missing.
• Updated all existing tests to provide a permission handler.
• Added CreateSessionAsync/ResumeSessionAsync helpers in the .NET E2E test base to auto-inject PermissionHandler.ApproveAll.

Cleanup

• Removed tautological nil checks in Go after the early-return guard.
• Updated doc comments and examples across all SDKs.

[github-actions]

✅ Cross-SDK Consistency Review: PASSED

I've reviewed PR #554 for consistency across all four SDK implementations (Node.js/TypeScript, Python, Go, and .NET). This PR demonstrates excellent cross-language consistency in requiring permission handlers for session creation and resumption.

Summary of Changes

All 4 SDKs consistently implement the following changes:

1. API Changes: Made config parameter required (removed default null/None/{}) for:

   • createSession/create_session/CreateSession/CreateSessionAsync
   • resumeSession/resume_session/ResumeSessionWithOptions/ResumeSessionAsync

2. Runtime Validation: All SDKs validate that the permission handler is set and throw/return a descriptive error with helpful examples if missing

3. Error Messages: Consistent, helpful messages across all languages (appropriately adapted for language syntax):

   • Node.js: 'An onPermissionRequest handler is required...'
   • Python: 'An on_permission_request handler is required...'
   • Go: "an OnPermissionRequest handler is required..."
   • C#: "An OnPermissionRequest handler is required..."

4. Type System Changes:

   • TypeScript: Changed SessionConfig.onPermissionRequest from optional (?) to required; ResumeSessionConfig inherits this via Pick(SessionConfig, ...)
   • Python: Runtime validation (TypedDict has total=False, making all fields optional at type level)
   • Go: Runtime validation (Go struct fields don't have required/optional type annotations)
   • .NET: Runtime validation (uses nullable PermissionRequestHandler? with null check)

5. Test Coverage: All 4 SDKs added unit tests verifying:

   • Error thrown when createSession called without permission handler
   • Error thrown when resumeSession called without permission handler
   • All existing tests updated to provide permission handlers

Consistency Verification

Aspect	Node.js	Python	Go	.NET	Status
Config required	✅	✅	✅	✅	✅ Consistent
Runtime validation	✅	✅	✅	✅	✅ Consistent
Error message with example	✅	✅	✅	✅	✅ Consistent
createSession test	✅	✅	✅	✅	✅ Consistent
resumeSession test	✅	✅	✅	✅	✅ Consistent
Existing tests updated	✅	✅	✅	✅	✅ Consistent

Conclusion

This PR maintains excellent feature parity across all SDK implementations. The API changes follow appropriate language conventions (camelCase vs snake_case vs PascalCase), and the behavior is identical across all platforms. No consistency issues found. 🎉

AI generated by SDK Consistency Review Agent

[Copilot]

Pull request overview

This PR requires permission handlers when creating or resuming Copilot sessions across all four SDKs (Node.js, Python, Go, .NET). Previously, omitting the handler silently denied all permission requests, which was a source of confusion. Now, the SDKs throw clear errors guiding users to set the handler explicitly (e.g., using approveAll/ApproveAll/approve_all for permissive behavior).

Changes:

• Made onPermissionRequest/OnPermissionRequest/on_permission_request required in session configuration types and added runtime validation
• Added clear error messages across all SDKs when handlers are missing
• Updated all existing tests to include permission handlers
• Added new unit tests verifying the error is thrown when handlers are missing
• Added .NET E2E test helper methods that auto-inject ApproveAll handler for convenience

Reviewed changes

Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
nodejs/src/types.ts	Changed onPermissionRequest from optional to required in SessionConfig
nodejs/src/client.ts	Added validation in createSession and resumeSession to require handler
nodejs/test/client.test.ts	Added tests for missing handler validation and updated existing tests
nodejs/samples/package-lock.json	Updated Copilot CLI dependency version
python/copilot/client.py	Added validation in create_session and resume_session to require handler
python/test_client.py	Added tests for missing handler validation and updated existing tests
go/client.go	Added validation in CreateSession and ResumeSessionWithOptions, removed redundant nil checks
go/client_test.go	Added tests for missing handler validation and updated existing tests
dotnet/src/Client.cs	Added validation in CreateSessionAsync and ResumeSessionAsync, updated doc examples
dotnet/src/Session.cs	Updated doc examples to include permission handler
dotnet/test/ClientTests.cs	Added tests for missing handler validation and updated existing tests
dotnet/test/Harness/E2ETestBase.cs	Added helper methods that auto-inject ApproveAll for E2E tests
dotnet/test/*.cs (multiple)	Updated all E2E tests to use new helper methods

Files not reviewed (1)

• nodejs/samples/package-lock.json: Language not supported

Comments suppressed due to low confidence (7)

dotnet/test/PermissionTests.cs:217

• Similar issue: this test name indicates it's testing deny-by-default behavior after resuming without a handler, but CreateSessionAsync and ResumeSessionAsync helpers auto-inject PermissionHandler.ApproveAll. The test is therefore not testing the scenario its name suggests.

The test should be updated to align with its intended purpose or renamed to reflect what it actually tests.

    public async Task Should_Deny_Tool_Operations_By_Default_When_No_Handler_Is_Provided_After_Resume()
    {
        var session1 = await CreateSessionAsync(new SessionConfig
        {
            OnPermissionRequest = PermissionHandler.ApproveAll
        });
        var sessionId = session1.SessionId;
        await session1.SendAndWaitAsync(new MessageOptions { Prompt = "What is 1+1?" });

        var session2 = await ResumeSessionAsync(sessionId);
        var permissionDenied = false;

        session2.On(evt =>
        {
            if (evt is ToolExecutionCompleteEvent toolEvt &&
                !toolEvt.Data.Success &&
                toolEvt.Data.Error?.Message.Contains("Permission denied") == true)
            {
                permissionDenied = true;
            }
        });

        await session2.SendAndWaitAsync(new MessageOptions
        {
            Prompt = "Run 'node --version'"
        });

        Assert.True(permissionDenied, "Expected a tool.execution_complete event with Permission denied result");
    }

nodejs/src/client.ts:652

• Similar to createSession, the conditional check for config.onPermissionRequest on line 650 is redundant since it's validated as required on lines 601-605. The handler is guaranteed to be present, so this should be simplified to unconditional registration:
  session.registerPermissionHandler(config.onPermissionRequest);

        if (config.onPermissionRequest) {
            session.registerPermissionHandler(config.onPermissionRequest);
        }

python/copilot/client.py:615

• The documentation examples for resume_session are missing the required on_permission_request handler. Both examples on lines 610 and 613-615 should include it. For instance:
  session = await client.resume_session("session-123", {"on_permission_request": PermissionHandler.approve_all})

And:
session = await client.resume_session("session-123", {
"on_permission_request": PermissionHandler.approve_all,
"tools": [my_new_tool]
})

        Example:
            >>> # Resume a previous session
            >>> session = await client.resume_session("session-123")
            >>>
            >>> # Resume with new tools
            >>> session = await client.resume_session("session-123", {
            ...     "tools": [my_new_tool]
            ... })

go/client.go:541

• The documentation example is missing the required OnPermissionRequest handler. Both examples on lines 527 and 539-541 should include it. For instance:
  session, err := client.ResumeSession(context.Background(), "session-123", &copilot.ResumeSessionConfig{
  OnPermissionRequest: copilot.PermissionHandler.ApproveAll,
  })

And:
session, err := client.ResumeSessionWithOptions(context.Background(), "session-123", &copilot.ResumeSessionConfig{
OnPermissionRequest: copilot.PermissionHandler.ApproveAll,
Tools: []copilot.Tool{myNewTool},
})

// Example:
//
//	session, err := client.ResumeSession(context.Background(), "session-123")
func (c *Client) ResumeSession(ctx context.Context, sessionID string) (*Session, error) {
	return c.ResumeSessionWithOptions(ctx, sessionID, nil)
}

// ResumeSessionWithOptions resumes an existing conversation session with additional configuration.
//
// This allows you to continue a previous conversation, maintaining all conversation history.
// The session must have been previously created and not deleted.
//
// Example:
//
//	session, err := client.ResumeSessionWithOptions(context.Background(), "session-123", &copilot.ResumeSessionConfig{
//	    Tools: []copilot.Tool{myNewTool},
//	})

dotnet/test/PermissionTests.cs:110

• Similar to the test above, this test name suggests it's testing behavior without a permission handler, but it uses CreateSessionAsync which automatically injects PermissionHandler.ApproveAll when OnPermissionRequest is null. This contradicts the test name's claim of working "Without Permission Handler."

Consider either:

1. Renaming the test to reflect that it's testing with an approve-all handler
2. Removing this test if it's now redundant given that permission handlers are required

    public async Task Should_Work_Without_Permission_Handler__Default_Behavior_()
    {
        // Create session without permission handler
        var session = await CreateSessionAsync(new SessionConfig());

        await session.SendAsync(new MessageOptions
        {
            Prompt = "What is 2+2?"
        });

        var message = await TestHelper.GetFinalAssistantMessageAsync(session);
        Assert.Contains("4", message?.Data.Content ?? string.Empty);
    }

python/copilot/client.py:760

• Similar to create_session, the conditional check for on_permission_request on line 759 is redundant since it's validated as required on lines 625-629. The handler is guaranteed to be present, so this should be simplified to unconditional registration:
  session._register_permission_handler(on_permission_request)

        if on_permission_request:
            session._register_permission_handler(on_permission_request)

nodejs/src/client.ts:595

• The documentation examples for resumeSession are missing the required onPermissionRequest handler. Both examples on lines 589 and 592-594 should include the handler. For instance:
  const session = await client.resumeSession("session-123", { onPermissionRequest: approveAll });

And:
const session = await client.resumeSession("session-123", {
onPermissionRequest: approveAll,
tools: [myNewTool]
});

     * @example
     * ```typescript
     * // Resume a previous session
     * const session = await client.resumeSession("session-123");
     *
     * // Resume with new tools
     * const session = await client.resumeSession("session-123", {
     *   tools: [myNewTool]
     * });
     * ```

[github-actions]

✅ Cross-SDK Consistency Review: All Clear!

After reviewing this PR across all four SDK implementations (Node.js/TypeScript, Python, Go, and .NET), I can confirm excellent cross-SDK consistency. This is a well-executed breaking change that maintains complete feature parity.

Summary

All four SDKs consistently implement the requirement for a permission handler when creating or resuming sessions:

API Signatures

• ✅ Node.js: createSession(config) & resumeSession(sessionId, config) - config required
• ✅ Python: create_session(config) & resume_session(session_id, config) - config required
• ✅ Go: CreateSession(ctx, config) & ResumeSession(ctx, sessionId, config) - config required
• ✅ .NET: CreateSessionAsync(config) & ResumeSessionAsync(sessionId, config) - config required

Validation

All SDKs validate that the permission handler is set and throw/return descriptive errors with helpful examples showing the correct usage pattern.

Error Messages

Consistently structured across all languages (accounting for naming conventions):

• Node.js: "An onPermissionRequest handler is required when creating a session. For example, to allow all permissions, use { onPermissionRequest: approveAll }."
• Python: "An on_permission_request handler is required when creating a session. For example, to allow all permissions, use {"on_permission_request": PermissionHandler.approve_all}."
• Go: "an OnPermissionRequest handler is required when creating a session. For example, to allow all permissions, use &copilot.SessionConfig{OnPermissionRequest: copilot.PermissionHandler.ApproveAll}"
• .NET: "An OnPermissionRequest handler is required when creating a session. For example, to allow all permissions, use CreateSessionAsync(new() { OnPermissionRequest = PermissionHandler.ApproveAll });"

Type Safety

• ✅ TypeScript: Changed onPermissionRequest?: to onPermissionRequest: (required field)
• ✅ Other SDKs: Runtime validation ensures handler is provided

Tests

All SDKs added consistent unit tests verifying the validation logic, plus updated all existing tests to provide the required handler.

Documentation

• ✅ Class/package-level examples updated
• ✅ Method documentation updated
• ✅ Go README updated with new signature
• ✅ Test snapshots updated

No Issues Found

This PR maintains excellent feature parity while implementing a breaking change that improves the developer experience by making permission handling explicit and discoverable. No cross-SDK inconsistencies detected.

AI generated by SDK Consistency Review Agent