Fix tools read write consolidated #995
Conversation
…ers (#1) * Initial plan * Initial analysis and planning for CRUD tool consolidation Co-authored-by: mattdholloway <[email protected]> * Implement consolidated manage_gist tool with CRUD operations Co-authored-by: mattdholloway <[email protected]> * Complete phases 2-3: Consolidated code scanning and secret scanning alert tools Co-authored-by: mattdholloway <[email protected]> * Consolidate issue, pull request, and repository operations with manage_* tools Co-authored-by: mattdholloway <[email protected]> * Remove old individual tools replaced by consolidated manage_* tools Co-authored-by: mattdholloway <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: mattdholloway <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates multiple GitHub API tools into unified management tools to reduce API surface area and improve usability. The changes replace individual CRUD operations with single tools that handle multiple operations through an "operation" parameter.
Key changes:
- Consolidates read and write operations into single management tools for repositories, issues, pull requests, gists, and security scanning
- Moves some tools between read and write categories based on their actual functionality
- Updates documentation to reflect the new consolidated tool structure
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/tools.go | Replaces individual tools with consolidated management tools and reorganizes read/write categorization |
| pkg/github/repositories.go | Adds ManageRepository tool consolidating create, fork, and get_file_contents operations |
| pkg/github/issues.go | Adds ManageIssue tool consolidating list, get, create, update, and add_comment operations |
| pkg/github/pullrequests.go | Adds ManagePullRequest tool consolidating list, get, create, update, and merge operations |
| pkg/github/gists.go | Adds ManageGist tool consolidating list, create, update, and get operations |
| pkg/github/code_scanning.go | Adds ManageCodeScanningAlerts tool consolidating list and get operations |
| pkg/github/secret_scanning.go | Adds ManageSecretScanningAlerts tool consolidating list and get operations |
| Test files | Adds comprehensive test coverage for all new management tools |
| README.md | Updates documentation to reflect consolidated tool structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| repos := toolsets.NewToolset("repos", "GitHub Repository related tools"). | ||
| AddReadTools( | ||
| toolsets.NewServerTool(SearchRepositories(getClient, t)), | ||
| toolsets.NewServerTool(GetFileContents(getClient, getRawClient, t)), | ||
| toolsets.NewServerTool(ListCommits(getClient, t)), |
There was a problem hiding this comment.
The GetFileContents tool was moved from read tools to the ManageRepository write tool, but file content retrieval is typically a read-only operation. Consider keeping GetFileContents as a separate read tool or ensuring ManageRepository is properly categorized.
| AddReadTools( | ||
| toolsets.NewServerTool(GetCodeScanningAlert(getClient, t)), | ||
| toolsets.NewServerTool(ListCodeScanningAlerts(getClient, t)), | ||
| toolsets.NewServerTool(ManageCodeScanningAlerts(getClient, t)), |
There was a problem hiding this comment.
ManageCodeScanningAlerts is placed in read tools but supports both 'list' and 'get' operations. The tool name suggests it could handle write operations in the future. Consider naming it more specifically (e.g., 'ReadCodeScanningAlerts') or placing it in a more appropriate category.
Note: See the diff below for a potential fix:
@@ -96,7 +96,7 @@
)
codeSecurity := toolsets.NewToolset("code_security", "Code security related tools, such as GitHub Code Scanning").
AddReadTools(
- toolsets.NewServerTool(ManageCodeScanningAlerts(getClient, t)),
+ toolsets.NewServerTool(ReadCodeScanningAlerts(getClient, t)),
)
secretProtection := toolsets.NewToolset("secret_protection", "Secret protection related tools, such as GitHub Secret Scanning").
AddReadTools(
| AddReadTools( | ||
| toolsets.NewServerTool(GetSecretScanningAlert(getClient, t)), | ||
| toolsets.NewServerTool(ListSecretScanningAlerts(getClient, t)), | ||
| toolsets.NewServerTool(ManageSecretScanningAlerts(getClient, t)), |
There was a problem hiding this comment.
Similar to ManageCodeScanningAlerts, ManageSecretScanningAlerts is categorized as read-only but the naming suggests broader management capabilities. Consider more specific naming or verify the categorization is appropriate for future extensibility.
Note: See the diff below for a potential fix:
@@ -100,7 +100,7 @@
)
secretProtection := toolsets.NewToolset("secret_protection", "Secret protection related tools, such as GitHub Secret Scanning").
AddReadTools(
- toolsets.NewServerTool(ManageSecretScanningAlerts(getClient, t)),
+ toolsets.NewServerTool(ListSecretScanningAlerts(getClient, t)),
)
dependabot := toolsets.NewToolset("dependabot", "Dependabot tools").
AddReadTools(
| func min(a, b int) int { | ||
| if a < b { | ||
| return a | ||
| } | ||
| return b | ||
| } |
There was a problem hiding this comment.
The min function is a utility that could be moved to a shared utilities package to avoid duplication across the codebase. This promotes code reuse and maintainability.
1 participant
Closes: