diff --git a/pkg/github/consolidated_issue_types.go b/pkg/github/consolidated_issue_types.go index ccd84e0173..fa6b69f42d 100644 --- a/pkg/github/consolidated_issue_types.go +++ b/pkg/github/consolidated_issue_types.go @@ -488,6 +488,9 @@ func normalizeConsolidatedIssueArguments(kind string) func(json.RawMessage) (jso if err := json.Unmarshal(raw, &args); err != nil { return nil, err } + if err := normalizeInputAliases(args, map[string]string{"issueNumber": "issue_number"}); err != nil { + return nil, err + } for _, field := range []string{"method", "owner", "repo"} { if _, err := RequiredParam[string](args, field); err != nil { return nil, err diff --git a/pkg/github/consolidated_pull_request_types.go b/pkg/github/consolidated_pull_request_types.go index 6d6abaed63..f085b7feae 100644 --- a/pkg/github/consolidated_pull_request_types.go +++ b/pkg/github/consolidated_pull_request_types.go @@ -457,6 +457,9 @@ func normalizePullRequestArguments(kind string) func(json.RawMessage) (json.RawM if err := json.Unmarshal(raw, &args); err != nil { return nil, err } + if err := normalizeInputAliases(args, map[string]string{"pull_number": "pullNumber"}); err != nil { + return nil, err + } for _, field := range spec.required { if _, err := RequiredParam[string](args, field); err != nil { return nil, err diff --git a/pkg/github/input_aliases.go b/pkg/github/input_aliases.go new file mode 100644 index 0000000000..b3280db5f7 --- /dev/null +++ b/pkg/github/input_aliases.go @@ -0,0 +1,21 @@ +package github + +import "fmt" + +// normalizeInputAliases maps supported legacy parameter spellings to the +// canonical schema names before strict input validation. Supplying both forms +// is rejected so a caller cannot accidentally send conflicting values. +func normalizeInputAliases(args map[string]any, aliases map[string]string) error { + for alias, canonical := range aliases { + value, exists := args[alias] + if !exists { + continue + } + if _, canonicalExists := args[canonical]; canonicalExists { + return fmt.Errorf("parameters %q and %q cannot both be provided", alias, canonical) + } + args[canonical] = value + delete(args, alias) + } + return nil +} diff --git a/pkg/github/input_aliases_test.go b/pkg/github/input_aliases_test.go new file mode 100644 index 0000000000..fb730bff81 --- /dev/null +++ b/pkg/github/input_aliases_test.go @@ -0,0 +1,115 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "testing" + + "github.com/github/github-mcp-server/v2/pkg/inventory" + "github.com/github/github-mcp-server/v2/pkg/translations" + "github.com/google/go-github/v92/github" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPullRequestReadAcceptsSnakeCasePullNumber(t *testing.T) { + tool := PullRequestRead(translations.NullTranslationHelper) + normalized, err := tool.GetInputNormalizer()( + json.RawMessage(`{"method":"get","owner":"owner","repo":"repo","pull_number":"42"}`), + ) + require.NoError(t, err) + + var args map[string]any + require.NoError(t, json.Unmarshal(normalized, &args)) + assert.Equal(t, float64(42), args["pullNumber"]) + assert.NotContains(t, args, "pull_number") +} + +func TestIssueReadAcceptsCamelCaseIssueNumber(t *testing.T) { + tool := IssueRead(translations.NullTranslationHelper) + normalized, err := tool.GetInputNormalizer()( + json.RawMessage(`{"method":"get","owner":"owner","repo":"repo","issueNumber":"42"}`), + ) + require.NoError(t, err) + + var args map[string]any + require.NoError(t, json.Unmarshal(normalized, &args)) + assert.Equal(t, float64(42), args["issue_number"]) + assert.NotContains(t, args, "issueNumber") +} + +func TestTypedCallsAcceptParameterAliases(t *testing.T) { + tests := []struct { + name string + tool func() inventory.ServerTool + args map[string]any + route string + response any + }{ + { + name: "pull request snake case", + tool: func() inventory.ServerTool { return PullRequestRead(translations.NullTranslationHelper) }, + args: map[string]any{ + "method": "get", "owner": "owner", "repo": "repo", "pull_number": 42, + }, + route: GetReposPullsByOwnerByRepoByPullNumber, + response: &github.PullRequest{Number: new(42), Title: new("Test PR")}, + }, + { + name: "issue camel case", + tool: func() inventory.ServerTool { return IssueRead(translations.NullTranslationHelper) }, + args: map[string]any{ + "method": "get", "owner": "owner", "repo": "repo", "issueNumber": 42, + }, + route: GetReposIssuesByOwnerByRepoByIssueNumber, + response: &github.Issue{Number: new(42), Title: new("Test issue")}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + tool := tc.tool() + deps := BaseDeps{ + Client: mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + tc.route: mockResponse(t, http.StatusOK, tc.response), + })), + } + session := connectTypedSearchClient(t, tool, deps, inventory.ProtocolVersionMultiRoundTrip) + result, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: tool.Tool.Name, Arguments: tc.args, + }) + require.NoError(t, err) + assert.False(t, result.IsError, mustMarshalJSON(t, result)) + }) + } +} + +func TestInputAliasesRejectConflictingValues(t *testing.T) { + pullRequestRead := PullRequestRead(translations.NullTranslationHelper) + issueRead := IssueRead(translations.NullTranslationHelper) + tests := []struct { + name string + normalizer func(json.RawMessage) (json.RawMessage, error) + input string + }{ + { + name: "pull request number", + normalizer: pullRequestRead.GetInputNormalizer(), + input: `{"method":"get","owner":"owner","repo":"repo","pullNumber":42,"pull_number":42}`, + }, + { + name: "issue number", + normalizer: issueRead.GetInputNormalizer(), + input: `{"method":"get","owner":"owner","repo":"repo","issue_number":42,"issueNumber":42}`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := tc.normalizer(json.RawMessage(tc.input)) + require.ErrorContains(t, err, "cannot both be provided") + }) + } +} diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index b884f4888d..0bdedde607 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1024,7 +1024,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool InputSchema: schema, }, scopes.PublicRead(scopes.Repo), - func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, input GetFileContentsInput) (*mcp.CallToolResult, *RepositoryContentsOutput, error) { + func(ctx context.Context, deps ToolDependencies, request *mcp.CallToolRequest, input GetFileContentsInput) (*mcp.CallToolResult, *RepositoryContentsOutput, error) { owner := input.Owner if owner == "" { return utils.NewToolResultError("missing required parameter: owner"), nil, nil @@ -1101,7 +1101,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool if fallbackUsed { successNote = fmt.Sprintf(" Note: the provided ref '%s' does not exist, default branch '%s' was used instead.", originalRef, rawOpts.Ref) } - const maxContentSize = 1024 * 1024 // 1MB + maxContentSize := repositoryContentSizeLimit(request) read, respInspect, err := inspectRepositoryFile(ctx, client, owner, repo, ref, path, fileContent) if err != nil { @@ -1232,6 +1232,24 @@ func recordDirContentsFieldsUsage(ctx context.Context, deps ToolDependencies, fu recordFieldsUsageFor(ctx, deps, "get_file_contents", full, filtered, sentBytes) } +const ( + legacyRepositoryContentSizeLimit = 1024 * 1024 + modernRepositoryContentSizeLimit = 448 * 1024 +) + +// repositoryContentSizeLimit accounts for modern typed output mirroring file +// content in both MCP content blocks and structuredContent. +func repositoryContentSizeLimit(request *mcp.CallToolRequest) int { + if request == nil || request.Params == nil { + return legacyRepositoryContentSizeLimit + } + version, _ := request.Params.Meta[mcp.MetaKeyProtocolVersion].(string) + if inventory.ProtocolEraForVersion(version) == inventory.ProtocolEraModern { + return modernRepositoryContentSizeLimit + } + return legacyRepositoryContentSizeLimit +} + // ForkRepository creates a tool to fork a repository. func ForkRepository(t translations.TranslationHelperFunc) inventory.ServerTool { return NewTool[ForkRepositoryInput, *ForkRepositoryOutput]( diff --git a/pkg/github/repository_contents_protocol_test.go b/pkg/github/repository_contents_protocol_test.go new file mode 100644 index 0000000000..f7ed85e5b0 --- /dev/null +++ b/pkg/github/repository_contents_protocol_test.go @@ -0,0 +1,80 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "strings" + "testing" + + "github.com/github/github-mcp-server/v2/pkg/inventory" + "github.com/github/github-mcp-server/v2/pkg/translations" + "github.com/google/go-github/v92/github" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/require" +) + +func TestGetFileContentsModernProtocolUsesTransportSafeLimit(t *testing.T) { + const fileName = "medium-file.txt" + fileSHA := strings.Repeat("b", 40) + + mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, "{\"name\": \"repo\", \"default_branch\": \"main\"}"), + "GET /repos/owner/repo/git/trees/refs/heads/main": mockResponse(t, http.StatusOK, &github.Tree{ + Entries: []*github.TreeEntry{{ + Path: new(fileName), + Mode: new("100644"), + Type: new("blob"), + SHA: new(fileSHA), + }}, + }), + GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + fileContent := map[string]any{ + "name": fileName, + "path": fileName, + "sha": fileSHA, + "type": "file", + "size": 500 * 1024, + "download_url": "https://raw.githubusercontent.com/owner/repo/main/" + fileName, + } + contentBytes, err := json.Marshal(fileContent) + require.NoError(t, err) + _, err = w.Write(contentBytes) + require.NoError(t, err) + }, + }) + + deps := BaseDeps{Client: mustNewGHClient(t, mockedClient)} + tool := GetFileContents(translations.NullTranslationHelper) + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "path": fileName, + "ref": "refs/heads/main", + }) + request.Params.Meta = mcp.Meta{mcp.MetaKeyProtocolVersion: inventory.ProtocolVersionMultiRoundTrip} + + result, err := tool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + require.Len(t, result.Content, 2) + + resourceLink, ok := result.Content[1].(*mcp.ResourceLink) + require.True(t, ok, "modern clients should receive a ResourceLink before the 1 MiB wire limit") + require.Equal(t, "repo://owner/repo/refs/heads/main/contents/"+fileName, resourceLink.URI) + require.Equal(t, fileName, resourceLink.Name) +} + +func TestRepositoryContentSizeLimit(t *testing.T) { + modernRequest := createMCPRequest(map[string]any{}) + modernRequest.Params.Meta = mcp.Meta{mcp.MetaKeyProtocolVersion: inventory.ProtocolVersionMultiRoundTrip} + + legacyRequest := createMCPRequest(map[string]any{}) + legacyRequest.Params.Meta = mcp.Meta{mcp.MetaKeyProtocolVersion: "2025-11-25"} + + require.Equal(t, modernRepositoryContentSizeLimit, repositoryContentSizeLimit(&modernRequest)) + require.Equal(t, legacyRepositoryContentSizeLimit, repositoryContentSizeLimit(&legacyRequest)) + require.Equal(t, legacyRepositoryContentSizeLimit, repositoryContentSizeLimit(nil)) +}