Add permission checks for SDK-registered custom tools#555
Add permission checks for SDK-registered custom tools#555SteveSandersonMS merged 5 commits intomainfrom
Conversation
Cross-SDK Consistency Review ✅Great work on maintaining consistency across all four SDK implementations! This PR properly adds What's Consistent ✅
Minor Suggestion for Test Parity 📝There's a small difference in test assertions:
Suggestion: Consider adding Go ( for _, req := range permissionRequests {
if req.Kind == "custom-tool" {
customToolReqs++
// Optionally verify toolName
if toolName, ok := req.Extra["toolName"].(string); ok && toolName != "encrypt_string" {
t.Errorf("Expected toolName 'encrypt_string', got '%s'", toolName)
}
}
}.NET ( // Should have received a custom-tool permission request
var customToolRequest = permissionRequests.FirstOrDefault(r => r.Kind == "custom-tool");
Assert.NotNull(customToolRequest);
Assert.Equal("encrypt_string", customToolRequest.ExtensionData?["toolName"]);This would bring all four SDKs to complete feature parity in test coverage. Not blocking—the current tests are solid and will catch the main behavior correctly! 🚀
|
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK type surfaces and cross-language E2E tests to support a new permission request kind (custom-tool) so that SDK-registered custom tools participate in the same permission flow as built-in tools.
Changes:
- Added
"custom-tool"to the permission-kind type unions in Node.js and Python. - Updated existing custom-tool E2E tests across languages to provide an
onPermissionRequesthandler. - Added new E2E scenarios (per language) that validate custom-tool invocation when approved and blocking when denied (plus new replay snapshots).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/snapshots/tools/invokes_custom_tool_with_permission_handler.yaml | Adds replay snapshot for approved custom-tool execution path. |
| test/snapshots/tools/denies_custom_tool_when_permission_denied.yaml | Adds replay snapshot for the denied custom-tool path. |
| python/e2e/test_tools.py | Injects permission handler into existing tool tests; adds approve/deny custom-tool permission tests. |
| python/copilot/types.py | Extends PermissionRequest.kind Literal union to include custom-tool. |
| nodejs/test/e2e/tools.test.ts | Adds permission handler to existing tool tests; adds approve/deny custom-tool permission tests. |
| nodejs/src/types.ts | Extends PermissionRequest.kind union to include custom-tool. |
| go/internal/e2e/tools_test.go | Injects permission handler into existing tool tests; adds approve/deny custom-tool permission tests. |
| dotnet/test/ToolsTests.cs | Injects permission handler into existing tool tests; adds approve/deny custom-tool permission tests. |
e6b0c88 to
1f2622a
Compare
✅ Cross-SDK Consistency Review: PASSEDThis PR maintains excellent cross-SDK consistency for the new custom tool permission checking feature. All four language implementations (Node.js/TypeScript, Python, Go, and .NET) have been updated consistently. What was verified:
SummaryThis PR exemplifies excellent multi-language SDK development. The feature is implemented consistently across all four languages with:
No cross-SDK consistency issues found. 🎉
|
✅ Cross-SDK Consistency ReviewThis PR maintains excellent consistency across all four SDK implementations. Great work! What I verified:1. Type Definitions ✅All four SDKs consistently add
2. E2E Test Coverage ✅Each SDK adds two new tests with consistent naming (following language conventions):
Both tests verify the same behavior:
3. Test Implementation ✅All languages access the
4. Existing Tests Updated ✅All existing custom tool tests now include an 5. Shared Test Snapshots ✅Two new YAML snapshot files are added and shared across all SDKs:
No consistency issues found. This is a model example of maintaining feature parity across a multi-language SDK! 🎉
|
Add 'custom-tool' to the PermissionRequest kind union in Node.js and Python types. Update all existing custom tool e2e tests across all four languages (Node.js, Python, Go, .NET) to provide an onPermissionRequest handler, and add new e2e tests verifying permission approval and denial flows for custom tools. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… Go and .NET tests - Remove unused PermissionRequestResult import from Node.js test - Add toolName assertion in Go test for cross-SDK parity - Add toolName assertion in .NET test for cross-SDK parity Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
14c3a81 to
b57d0d5
Compare
✅ Cross-SDK Consistency ReviewThis PR successfully maintains excellent cross-SDK consistency for the custom tool permission feature across all four SDK implementations (Node.js, Python, Go, .NET). Type Definitions ✅Permission Request Kind:
All implementations are consistent and appropriate for each language's type system. Test Coverage ✅All four SDKs have identical test coverage:
Test Implementation Consistency ✅All test implementations follow the same pattern:
Snapshot Files ✅Two new snapshot YAML files added for E2E test replay:
Summary: This PR demonstrates exemplary cross-SDK consistency. The feature is implemented uniformly across all four SDKs with matching semantics, equivalent test coverage, and appropriate language-specific adaptations. No inconsistencies found. 🎯
|
✅ Cross-SDK Consistency ReviewI've reviewed PR #555 for consistency across all four SDK implementations (Node.js/TypeScript, Python, Go, and .NET). The changes maintain excellent cross-SDK consistency. Summary of ChangesThis PR adds permission checking for SDK-registered custom tools across all SDKs with the following changes: 1. Type Definitions ✅
This follows the established pattern where TypeScript/Python use compile-time type safety with union/literal types, while Go/.NET use plain strings with runtime validation. 2. E2E Tests ✅All four SDKs received identical test coverage:
3. Test Implementation Consistency ✅The new tests are semantically identical across all languages, respecting language idioms:
VerdictNo consistency issues found. This PR exemplifies good multi-language SDK maintenance:
Great work maintaining consistency! 🎉
|
Overview
Fixes #254 across all languages. SDK-registered custom tools are now subject to the same permission checking flow as any other tool call.
E2E test updates
onPermissionRequesthandler (approveAll/ApproveAll) since the runtime now requires permission approval before executing custom toolsinvokes_custom_tool_with_permission_handler— verifies a custom tool executes when the permission handler approvesdenies_custom_tool_when_permission_denied— verifies a custom tool is blocked when the permission handler deniesCI note
The e2e tests in this PR depend on a corresponding runtime change that adds the
custom-toolpermission request kind. CI will fail until that runtime change is released. This is expected — the tests are correct and pass when run against a runtime build that includes the permission check.