- Notifications
You must be signed in to change notification settings - Fork3.1k
Fix handling of multi path resources#1458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix handling of multi path resources#1458
Conversation
…cp-server into omgitsads/go-sdk-repo-resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR fixes the handling of multi-path resources in URI templates by properly extracting path components fromuritemplate.Value. The issue was thatString() doesn't work for multi-segment paths with capture groups - the proper approach is to check if the value is a list and handle both cases accordingly.
Key Changes:
- Fixed path extraction logic in
RepositoryResourceContentsHandlerto handle both single path values and multi-segment paths usingList()andString()methods - Added new utility functions for creating tool results in
pkg/utils/result.go - Migrated from
mark3labs/mcp-gotomodelcontextprotocol/go-sdkthroughout the codebase - Updated third-party license files to reflect dependency changes
Reviewed Changes
Copilot reviewed 90 out of 91 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/github/repository_resource.go | Fixed multi-path URI template handling by checking for list values and joining path components |
pkg/github/repository_resource_test.go | Refactored tests to use new SDK patterns and added multi-path test case |
pkg/utils/result.go | Added utility functions for creating MCP tool results |
pkg/toolsets/toolsets.go | Migrated to new MCP SDK with updated type definitions |
pkg/log/io.go | Added Close method to IOLogger |
pkg/github/*.go | Large-scale migration from old MCP SDK to new one |
| Third-party license files | Updated to reflect dependency changes (removed old deps, added new ones) |
| returnmcp.NewToolResultError(err.Error()),nil | ||
| } | ||
| funcGetDependabotAlert(getClientGetClientFn,t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any,any]) { | ||
| tool:= mcp.Tool{ |
CopilotAINov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This definition of tool is never used.
| returnmcp.NewToolResultError(err.Error()),nil | ||
| } | ||
| funcListDependabotAlerts(getClientGetClientFn,t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any,any]) { | ||
| tool:= mcp.Tool{ |
CopilotAINov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This definition of tool is never used.
| returnmcp.NewToolResultError(err.Error()),nil | ||
| } | ||
| funcListGists(getClientGetClientFn,t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any,any]) { | ||
| tool:= mcp.Tool{ |
CopilotAINov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This definition of tool is never used.
| returnmcp.NewToolResultError(err.Error()),nil | ||
| } | ||
| funcGetGist(getClientGetClientFn,t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any,any]) { | ||
| tool:= mcp.Tool{ |
CopilotAINov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This definition of tool is never used.
| returnmcp.NewToolResultError(err.Error()),nil | ||
| } | ||
| funcCreateGist(getClientGetClientFn,t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any,any]) { | ||
| tool:= mcp.Tool{ |
CopilotAINov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This definition of tool is never used.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
mattdholloway left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lgtm!
948fe76 intogithub:omgitsads/go-sdkUh oh!
There was an error while loading.Please reload this page.
Follow up to#1457, as I missed that
uritemplate.Valueseparates the capture group andString()doesn't work.