Conversation
When users register tools via SessionConfig with names matching built-in CLI tools, automatically add those names to excludedTools in the RPC payload so the SDK's local handler takes precedence. Implemented consistently across all 4 SDKs (Node.js, Python, Go, .NET) with unit tests and documentation updates. Closes #411 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Cross-SDK Consistency ReviewExcellent work! This PR demonstrates exemplary cross-SDK consistency for implementing the tool override feature. All four SDK implementations (Node.js, Python, Go, and .NET) are well-aligned. Summary of ChangesAll SDKs now allow user-registered tools to automatically override built-in CLI tools by merging tool names into the Consistency Analysis✅ Implementation ApproachAll four SDKs implement the same logic:
✅ API ConsistencyThe feature is applied consistently to both session operations:
✅ Test CoverageAll SDKs have equivalent test scenarios:
✅ DocumentationAll four README files include:
No Issues FoundThis PR maintains excellent feature parity across all SDK implementations. No consistency issues detected. Recommendation: ✅ Approve from a cross-SDK consistency perspective.
|
There was a problem hiding this comment.
Pull request overview
This PR implements automatic tool overriding across all four SDKs (Node.js, Python, Go, .NET) to allow user-registered tools to take precedence over built-in CLI tools when they share the same name. The implementation adds tool names from SessionConfig.tools to the excludedTools list before sending RPC requests to the CLI, ensuring the CLI doesn't handle those tools and instead delegates to the SDK's local dispatcher.
Changes:
- Implemented automatic tool name merging into excludedTools across all SDKs
- Added comprehensive unit tests for the merge logic in all SDKs
- Updated documentation in all four SDK READEs with examples of overriding built-in tools
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| nodejs/src/client.ts | Added mergeExcludedTools helper and integrated it into session create/resume |
| nodejs/test/client.test.ts | Added 4 unit tests covering tool override scenarios |
| nodejs/README.md | Added documentation section explaining tool override feature |
| python/copilot/client.py | Inline merge logic in both create_session and resume_session |
| python/test_client.py | Added 4 unit tests for tool exclusion behavior |
| python/README.md | Added documentation section with override example |
| go/client.go | Added mergeExcludedTools helper used in CreateSession and ResumeSessionWithOptions |
| go/client_test.go | Added 4 unit tests for the merge helper function |
| go/README.md | Added documentation section explaining override capability |
| dotnet/src/Client.cs | Added internal MergeExcludedTools helper called in both session methods |
| dotnet/src/GitHub.Copilot.SDK.csproj | Added InternalsVisibleTo for test assembly access |
| dotnet/test/MergeExcludedToolsTests.cs | New test file with 5 unit tests for the merge logic |
| dotnet/README.md | Added documentation section with C# override example |
| // buildUnsupportedToolResult creates a failure ToolResult for an unsupported tool. | ||
| // mergeExcludedTools returns a deduplicated list combining excludedTools with | ||
| // the names of any SDK-registered tools, so the CLI won't handle them. |
There was a problem hiding this comment.
The comment for mergeExcludedTools (lines 1355-1356) is positioned between the comment for buildUnsupportedToolResult (line 1354) and the function definition (line 1357). This creates confusion about which function the comment belongs to. Move the comment for mergeExcludedTools to immediately precede the function definition on line 1357.
|
|
||
| @define_tool(name="edit_file", description="Custom file editor with project-specific validation") | ||
| async def edit_file(params: EditFileParams) -> str: | ||
| # your logic |
There was a problem hiding this comment.
The documentation example shows an incomplete function handler with only a comment placeholder. While concise, it would be more helpful to show a minimal but complete example that returns a value, such as return f"Edited {path}" to demonstrate the expected return type and give developers a working starting point.
| # your logic | |
| # Example: perform custom validation and editing logic here | |
| return f"Edited {params.path}" |
| defineTool("edit_file", { | ||
| description: "Custom file editor with project-specific validation", | ||
| parameters: z.object({ path: z.string(), content: z.string() }), | ||
| handler: async ({ path, content }) => { /* your logic */ }, |
There was a problem hiding this comment.
The documentation example shows an incomplete function handler with only a comment placeholder. While concise, it would be more helpful to show a minimal but complete example that returns a value, such as return "File edited successfully" to demonstrate the expected return type and give developers a working starting point.
| handler: async ({ path, content }) => { /* your logic */ }, | |
| handler: async ({ path, content }) => { | |
| // your logic here, e.g. validate and write file contents | |
| return "File edited successfully"; | |
| }, |
| func(params EditFileParams, inv copilot.ToolInvocation) (any, error) { | ||
| // your logic |
There was a problem hiding this comment.
The documentation example references EditFileParams which is not defined anywhere in the example. Either define this type in the example or use a simpler inline parameter list. For example: func(path string, content string, inv copilot.ToolInvocation) (any, error) { return "File edited", nil }
| func(params EditFileParams, inv copilot.ToolInvocation) (any, error) { | |
| // your logic | |
| func(path string, content string, inv copilot.ToolInvocation) (any, error) { | |
| // your logic | |
| return "File edited", nil |
| AIFunctionFactory.Create( | ||
| async ([Description("File path")] string path, [Description("New content")] string content) => { | ||
| // your logic | ||
| }, | ||
| "edit_file", | ||
| "Custom file editor with project-specific validation") | ||
| ``` |
There was a problem hiding this comment.
The documentation example shows an incomplete function handler with only a comment placeholder. While concise, it would be more helpful to show a minimal but complete example that returns a value, such as return "File edited successfully"; to demonstrate the expected return type and give developers a working starting point.
1 participant
Summary
Closes #411
When users register tools via
SessionConfig.toolswith names matching built-in CLI tools (e.g.,edit_file,read_file), the CLI previously handled them itself instead of dispatching to the user's handler.This PR makes user-registered tools automatically take precedence by adding their names to
excludedToolsin the RPC payload, so the CLI's built-in version is excluded and the SDK's local dispatch handles the call instead.Changes
Implemented consistently across all 4 SDKs:
mergeExcludedTools()helper inclient.tsclient.test.tsclient.pytest_client.pymergeExcludedTools()helper inclient.goclient_test.goMergeExcludedTools()helper inClient.csMergeExcludedToolsTests.csHow it works
session.create/session.resumeRPC, collect tool names from user-registered toolsexcludedTools(with deduplication)Backward compatibility
excludedTools, deduplication avoids issues