diff --git a/README.md b/README.md index dc08311a4..dff62321b 100644 --- a/README.md +++ b/README.md @@ -1221,7 +1221,7 @@ The following sets of tools are available: - **get_commit** - Get commit details - **Required OAuth Scopes**: `repo` - - `include_diff`: Whether to include file diffs and stats in the response. Default is true. (boolean, optional) + - `detail`: Level of detail to include for changed files. "none" omits stats and files entirely. "stats" (default) includes per-file metadata: filename, status, and lines-of-code counts (additions, deletions, changes), with no patch content. "full_patch" additionally includes the unified diff content for each file and can be very large. (string, optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) diff --git a/pkg/github/__toolsnaps__/get_commit.snap b/pkg/github/__toolsnaps__/get_commit.snap index 9e2346b59..122e6210b 100644 --- a/pkg/github/__toolsnaps__/get_commit.snap +++ b/pkg/github/__toolsnaps__/get_commit.snap @@ -6,10 +6,15 @@ "description": "Get details for a commit from a GitHub repository", "inputSchema": { "properties": { - "include_diff": { - "default": true, - "description": "Whether to include file diffs and stats in the response. Default is true.", - "type": "boolean" + "detail": { + "default": "stats", + "description": "Level of detail to include for changed files. \"none\" omits stats and files entirely. \"stats\" (default) includes per-file metadata: filename, status, and lines-of-code counts (additions, deletions, changes), with no patch content. \"full_patch\" additionally includes the unified diff content for each file and can be very large.", + "enum": [ + "none", + "stats", + "full_patch" + ], + "type": "string" }, "owner": { "description": "Repository owner", diff --git a/pkg/github/minimal_types.go b/pkg/github/minimal_types.go index a93d29ead..5200be297 100644 --- a/pkg/github/minimal_types.go +++ b/pkg/github/minimal_types.go @@ -108,6 +108,7 @@ type MinimalCommitFile struct { Additions int `json:"additions,omitempty"` Deletions int `json:"deletions,omitempty"` Changes int `json:"changes,omitempty"` + Patch string `json:"patch,omitempty"` } // MinimalPRFile represents a file changed in a pull request. @@ -1463,8 +1464,34 @@ func newMinimalCommitFromCore(sha, htmlURL string, commit *github.Commit, author return minimalCommit } -// convertToMinimalCommit converts a GitHub API RepositoryCommit to MinimalCommit -func convertToMinimalCommit(commit *github.RepositoryCommit, includeDiffs bool) MinimalCommit { +// commitDetail controls how much per-file information convertToMinimalCommit +// includes in its output. +type commitDetail string + +const ( + // commitDetailNone omits Stats and Files entirely. + commitDetailNone commitDetail = "none" + // commitDetailStats includes Stats and Files with metadata only + // (filename, status, additions, deletions, changes) but no patch text. + commitDetailStats commitDetail = "stats" + // commitDetailFullPatch additionally includes the unified diff for each file. + commitDetailFullPatch commitDetail = "full_patch" +) + +// parseCommitDetail validates the user-supplied detail value and returns the +// default (stats) when the value is empty. +func parseCommitDetail(s string) (commitDetail, error) { + switch s { + case "": + return commitDetailStats, nil + case string(commitDetailNone), string(commitDetailStats), string(commitDetailFullPatch): + return commitDetail(s), nil + default: + return "", fmt.Errorf("invalid detail %q: must be one of \"none\", \"stats\", \"full_patch\"", s) + } +} + +func convertToMinimalCommit(commit *github.RepositoryCommit, detail commitDetail) MinimalCommit { minimalCommit := newMinimalCommitFromCore( commit.GetSHA(), commit.GetHTMLURL(), @@ -1473,28 +1500,32 @@ func convertToMinimalCommit(commit *github.RepositoryCommit, includeDiffs bool) commit.Committer, ) - // Only include stats and files if includeDiffs is true - if includeDiffs { - if commit.Stats != nil { - minimalCommit.Stats = &MinimalCommitStats{ - Additions: commit.Stats.GetAdditions(), - Deletions: commit.Stats.GetDeletions(), - Total: commit.Stats.GetTotal(), - } + if detail == commitDetailNone { + return minimalCommit + } + + if commit.Stats != nil { + minimalCommit.Stats = &MinimalCommitStats{ + Additions: commit.Stats.GetAdditions(), + Deletions: commit.Stats.GetDeletions(), + Total: commit.Stats.GetTotal(), } + } - if len(commit.Files) > 0 { - minimalCommit.Files = make([]MinimalCommitFile, 0, len(commit.Files)) - for _, file := range commit.Files { - minimalFile := MinimalCommitFile{ - Filename: file.GetFilename(), - Status: file.GetStatus(), - Additions: file.GetAdditions(), - Deletions: file.GetDeletions(), - Changes: file.GetChanges(), - } - minimalCommit.Files = append(minimalCommit.Files, minimalFile) + if len(commit.Files) > 0 { + minimalCommit.Files = make([]MinimalCommitFile, 0, len(commit.Files)) + for _, file := range commit.Files { + minimalFile := MinimalCommitFile{ + Filename: file.GetFilename(), + Status: file.GetStatus(), + Additions: file.GetAdditions(), + Deletions: file.GetDeletions(), + Changes: file.GetChanges(), + } + if detail == commitDetailFullPatch { + minimalFile.Patch = file.GetPatch() } + minimalCommit.Files = append(minimalCommit.Files, minimalFile) } } diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index d682b5c3d..040a968cf 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -46,10 +46,11 @@ func GetCommit(t translations.TranslationHelperFunc) inventory.ServerTool { Type: "string", Description: "Commit SHA, branch name, or tag name", }, - "include_diff": { - Type: "boolean", - Description: "Whether to include file diffs and stats in the response. Default is true.", - Default: json.RawMessage(`true`), + "detail": { + Type: "string", + Enum: []any{"none", "stats", "full_patch"}, + Description: "Level of detail to include for changed files. \"none\" omits stats and files entirely. \"stats\" (default) includes per-file metadata: filename, status, and lines-of-code counts (additions, deletions, changes), with no patch content. \"full_patch\" additionally includes the unified diff content for each file and can be very large.", + Default: json.RawMessage(`"stats"`), }, }, Required: []string{"owner", "repo", "sha"}, @@ -69,7 +70,11 @@ func GetCommit(t translations.TranslationHelperFunc) inventory.ServerTool { if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - includeDiff, err := OptionalBoolParamWithDefault(args, "include_diff", true) + detailRaw, err := OptionalParam[string](args, "detail") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + detail, err := parseCommitDetail(detailRaw) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -106,7 +111,7 @@ func GetCommit(t translations.TranslationHelperFunc) inventory.ServerTool { } // Convert to minimal commit - minimalCommit := convertToMinimalCommit(commit, includeDiff) + minimalCommit := convertToMinimalCommit(commit, detail) r, err := json.Marshal(minimalCommit) if err != nil { @@ -252,7 +257,7 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { // Convert to minimal commits minimalCommits := make([]MinimalCommit, len(commits)) for i, commit := range commits { - minimalCommits[i] = convertToMinimalCommit(commit, false) + minimalCommits[i] = convertToMinimalCommit(commit, commitDetailNone) } r, err := json.Marshal(minimalCommits) diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 03535f1d2..e1b7f94f5 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -1028,6 +1028,120 @@ func Test_GetCommit(t *testing.T) { } } +func Test_GetCommit_Detail(t *testing.T) { + mockCommit := &github.RepositoryCommit{ + SHA: github.Ptr("abc123def456"), + HTMLURL: github.Ptr("https://github.com/owner/repo/commit/abc123def456"), + Commit: &github.Commit{ + Message: github.Ptr("First commit"), + }, + Stats: &github.CommitStats{ + Additions: github.Ptr(10), + Deletions: github.Ptr(2), + Total: github.Ptr(12), + }, + Files: []*github.CommitFile{ + { + Filename: github.Ptr("file1.go"), + Status: github.Ptr("modified"), + Additions: github.Ptr(10), + Deletions: github.Ptr(2), + Changes: github.Ptr(12), + Patch: github.Ptr("@@ -1,2 +1,10 @@\n+new line"), + }, + }, + } + + cases := []struct { + name string + args map[string]any + expectFiles bool + expectStats bool + expectPatch bool + expectError string + }{ + { + name: "default returns stats", + args: map[string]any{"owner": "owner", "repo": "repo", "sha": "abc123def456"}, + expectFiles: true, + expectStats: true, + expectPatch: false, + }, + { + name: "detail=none omits stats and files", + args: map[string]any{"owner": "owner", "repo": "repo", "sha": "abc123def456", "detail": "none"}, + expectFiles: false, + expectStats: false, + expectPatch: false, + }, + { + name: "detail=stats returns metadata without patch", + args: map[string]any{"owner": "owner", "repo": "repo", "sha": "abc123def456", "detail": "stats"}, + expectFiles: true, + expectStats: true, + expectPatch: false, + }, + { + name: "detail=full_patch includes patch text", + args: map[string]any{"owner": "owner", "repo": "repo", "sha": "abc123def456", "detail": "full_patch"}, + expectFiles: true, + expectStats: true, + expectPatch: true, + }, + { + name: "invalid detail value is rejected", + args: map[string]any{"owner": "owner", "repo": "repo", "sha": "abc123def456", "detail": "everything"}, + expectError: `invalid detail "everything"`, + }, + } + + serverTool := GetCommit(translations.NullTranslationHelper) + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposCommitsByOwnerByRepoByRef: mockResponse(t, http.StatusOK, mockCommit), + }) + client := mustNewGHClient(t, mockedClient) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + + request := createMCPRequest(tc.args) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + if tc.expectError != "" { + require.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, tc.expectError) + return + } + require.False(t, result.IsError) + + var returned MinimalCommit + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returned)) + + if tc.expectStats { + require.NotNil(t, returned.Stats) + assert.Equal(t, 12, returned.Stats.Total) + } else { + assert.Nil(t, returned.Stats) + } + + if tc.expectFiles { + require.Len(t, returned.Files, 1) + assert.Equal(t, "file1.go", returned.Files[0].Filename) + if tc.expectPatch { + assert.Equal(t, "@@ -1,2 +1,10 @@\n+new line", returned.Files[0].Patch) + } else { + assert.Empty(t, returned.Files[0].Patch) + } + } else { + assert.Empty(t, returned.Files) + } + }) + } +} + func Test_ListCommits(t *testing.T) { // Verify tool definition once serverTool := ListCommits(translations.NullTranslationHelper)