enable toolset flag #215
enable toolset flag #215
Conversation
jaylonmcshan19-x
force-pushed
the
enable-toolset-flag
branch
from
b19a95e to
ce72f96
Compare
|
this should fix #30 |
|
@gautambaghel Yup, just waiting on a review of my implementation and will add tests tomorrow! Once approved will merge 👍 |
pkg/toolsets/mapping.go
Outdated
| package toolsets | ||
|
|
||
| // ToolToToolset maps tool names to their toolsets | ||
| var ToolToToolset = map[string]Toolset{ |
There was a problem hiding this comment.
I think maintaining a map like this is OK for now, but I suspect in future we'll want to make this easier to maintain by having the tool initialization and assignment happening in the same place, i.e. in tools.go.
The GitHub MCP server project does something similar here: https://github.com/github/github-mcp-server/blob/ada4bc05a9c7c8246bd0651386a558802e23dec0/pkg/github/tools.go#L163-L379
pkg/toolsets/toolsets.go
Outdated
| import "strings" | ||
|
|
||
| // Toolset represents a group of related tools | ||
| type Toolset string |
There was a problem hiding this comment.
I'd perhaps call this ToolsetName because that's what it actually is.
pkg/toolsets/toolsets.go
Outdated
| ToolsetDefault Toolset = "default" | ||
| ) | ||
|
|
||
| type ToolsetMetadata struct { |
There was a problem hiding this comment.
This one I might just call Toolset. Metadata such an overloaded concept already throughout our projects.
pkg/toolsets/toolsets.go
Outdated
| ) | ||
|
|
||
| type ToolsetMetadata struct { | ||
| ID string |
There was a problem hiding this comment.
Above you defined a type for this, but here you're using string and calling it ID. If you rename you Toolset type to ToolsetName this line becomes
Name ToolsetName
|
Tried this out, looks good. Left some nitpicky comments about
|
pkg/toolsets/toolsets.go
Outdated
| Name: All, | ||
| Description: "Special toolset that enables all available toolsets", | ||
| } | ||
| Default_Toolset = Toolset{ |
There was a problem hiding this comment.
Nit: we're not supposed to use underscores in variable names.
3 participants
PCI review checklist
This pr Implements support for the
--toolsetsflag.This PR includes only the core toolset logic without the tool filtering implementation due to me seeking feedback on:
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.