feat: tool prefix name #657
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds support for an optional tool-prefix flag to disambiguate tool names by prepending a user-specified string.
- Introduce a
tool-prefixCLI flag and bind it via Viper. - Extend
MCPServerConfigandStdioServerConfigto includeToolPrefixand propagate it into server registration. - Update
Toolset.RegisterToolsandToolsetGroup.RegisterAllto accept a prefix and register tools with prefixed names.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/toolsets/toolsets.go | Added createToolWithPrefixedName and updated RegisterTools to apply the prefix when registering tools. |
| internal/ghmcp/server.go | Added ToolPrefix to MCPServerConfig/StdioServerConfig and passed it into RegisterAll and dynamic toolset registration. |
| cmd/github-mcp-server/main.go | Defined the tool-prefix flag in Cobra, bound it to Viper, and populated the CLI config. |
Comments suppressed due to low confidence (2)
pkg/toolsets/toolsets.go:106
- No unit tests were added to verify the new prefix logic in
RegisterTools. Adding tests to confirm that tools are registered with the correct prefixed names would help prevent regressions.
func (t *Toolset) RegisterTools(s *server.MCPServer, prefix string) {
pkg/toolsets/toolsets.go:110
- [nitpick] The parameter name
toolshadows the outer loop variable and may be confusing. Consider renaming it (e.g., tosrvTool) to make the code clearer.
registerToolWithPrefix := func(tool server.ServerTool) {
hungran
force-pushed
the
hungran/tool-prefix
branch
from
c65be5c to
ca62561
Compare
almaleksia
left a comment
almaleksia
left a comment
There was a problem hiding this comment.
This looks good. Would be great if you resolve merge conflicts
Signed-off-by: hungran <[email protected]>
Signed-off-by: hungran <[email protected]>
Signed-off-by: hungran <[email protected]>
Signed-off-by: hungran <[email protected]>
hungran
force-pushed
the
hungran/tool-prefix
branch
from
ca62561 to
c60428e
Compare
thanks @almaleksia, resolved |
|
Hey @hungran, thank you for the contribution! We discussed internally and atm we won't proceed with prefixing. This easily can be done by a client, plus there's a spec for namespaces under review, so we might implement this approach instead if it gets accepted: modelcontextprotocol/modelcontextprotocol#1292 But the idea is we do not touch the tool name and rely on client to resolve collisions. |
@almaleksia thank you |
2 participants
Closes: #333
introduce arg flag
tool-prefixso client able to add prefix name of tools, avoid conflict tool name with another mcp server