diff --git a/pkg/github/__toolsnaps__/get_commit.snap b/pkg/github/__toolsnaps__/get_commit.snap index ad6a805515..8aef83dbc5 100644 --- a/pkg/github/__toolsnaps__/get_commit.snap +++ b/pkg/github/__toolsnaps__/get_commit.snap @@ -5,6 +5,18 @@ "title": "Get commit details" }, "description": "Get details for a commit from a GitHub repository", + "icons": [ + { + "mimeType": "image/png", + "src": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAABmJLR0QA/wD/AP+gvaeTAAABTklEQVRIie2UPUvDUBSGn5PW2qnYwUXExWhNpY2CUKT+BB10E3R0dnbrppuKLgouHbr5X5qlKRIFB+cuguBHj4O05KYfNsVJ+m7n3Hue54ZcrhDKQqGQTX1YVwg7QIbx0gh8b7VTJMMrqS+5RtgD7oDX2GhhC6UcbhkCVLZVuX1sesex4YCdX6uAGgIrsicjlrTGgQ9KVPDnmQgmgongPwgkXNiOq8ADMAtkx4UGvtflJvus20ANeIlN/vW5/kkt8L3D2HD6P9dRgSI8dQcc9w1IR3acBE3vbFSp8ZMVnoFSqJ/unZDe3pAYXyDIJarn9opbDZrewaAh2y7Oy5S1r6h5QG2Xxbw3piDw6xe244oKyxFmC5ihc+tS1qaqngKJyAEBGmZvSGzHVYT790T7aPozsaFoFZGboFGvDJsbOYuOuxuuc7n1uaV8sRSH8Q1DUVLnYLty3gAAAABJRU5ErkJggg==", + "theme": "light" + }, + { + "mimeType": "image/png", + "src": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAABmJLR0QA/wD/AP+gvaeTAAAA+ElEQVRIie2WLU5DQRRGzwUEirQCQ9BsoSGtZQHgSFgIsg4JXQCGP8V2QCBIEATZkJDgOJi+MJnMo30TFHmfmzvzfeeKyZ2BROpQvVHfrddDmhkZ4BY4Ai6BD7prAowjIoq7i85nFcGNf6qa1tayM1vAvBZQUg74c/WAHtAD/gMgn6YCT8A2MKwOTaZpCfAF3AGvFdlLx7XqdUVw4186rgWeE8Nn4cU67QLNAS/ASG3qmwVPqdaqjWw9A86BK+CkzaTuAseFBse/AiLiQg1gLzs3Bwb8XIp94AxYL/Af2xordap6v/htHKhv6nTlgBUAh9l6Rx11yfgG8ne/zwh2OysAAAAASUVORK5CYII=", + "theme": "dark" + } + ], "inputSchema": { "properties": { "detail": { @@ -19,7 +31,8 @@ }, "owner": { "description": "Repository owner", - "type": "string" + "type": "string", + "x-mcp-header": "owner" }, "page": { "description": "Page number for pagination (min 1)", @@ -34,7 +47,8 @@ }, "repo": { "description": "Repository name", - "type": "string" + "type": "string", + "x-mcp-header": "repo" }, "sha": { "description": "Commit SHA, branch name, or tag name", @@ -48,5 +62,311 @@ ], "type": "object" }, - "name": "get_commit" + "name": "get_commit", + "outputSchema": { + "additionalProperties": false, + "properties": { + "author": { + "additionalProperties": false, + "properties": { + "avatar_url": { + "type": "string" + }, + "details": { + "additionalProperties": false, + "properties": { + "bio": { + "type": "string" + }, + "blog": { + "type": "string" + }, + "company": { + "type": "string" + }, + "created_at": { + "description": "Account creation time in RFC3339 format.", + "type": "string" + }, + "email": { + "type": "string" + }, + "followers": { + "type": "integer" + }, + "following": { + "type": "integer" + }, + "hireable": { + "type": "boolean" + }, + "location": { + "type": "string" + }, + "name": { + "type": "string" + }, + "owned_private_repos": { + "type": "integer" + }, + "private_gists": { + "type": "integer" + }, + "public_gists": { + "type": "integer" + }, + "public_repos": { + "type": "integer" + }, + "total_private_repos": { + "type": "integer" + }, + "twitter_username": { + "type": "string" + }, + "updated_at": { + "description": "Last profile update time in RFC3339 format.", + "type": "string" + } + }, + "required": [ + "public_repos", + "public_gists", + "followers", + "following", + "created_at", + "updated_at" + ], + "type": [ + "null", + "object" + ] + }, + "id": { + "type": "integer" + }, + "login": { + "type": "string" + }, + "profile_url": { + "type": "string" + } + }, + "required": [ + "login" + ], + "type": [ + "null", + "object" + ] + }, + "commit": { + "additionalProperties": false, + "properties": { + "author": { + "additionalProperties": false, + "properties": { + "date": { + "type": "string" + }, + "email": { + "type": "string" + }, + "name": { + "type": "string" + } + }, + "type": [ + "null", + "object" + ] + }, + "committer": { + "additionalProperties": false, + "properties": { + "date": { + "type": "string" + }, + "email": { + "type": "string" + }, + "name": { + "type": "string" + } + }, + "type": [ + "null", + "object" + ] + }, + "message": { + "type": "string" + } + }, + "required": [ + "message" + ], + "type": [ + "null", + "object" + ] + }, + "committer": { + "additionalProperties": false, + "properties": { + "avatar_url": { + "type": "string" + }, + "details": { + "additionalProperties": false, + "properties": { + "bio": { + "type": "string" + }, + "blog": { + "type": "string" + }, + "company": { + "type": "string" + }, + "created_at": { + "description": "Account creation time in RFC3339 format.", + "type": "string" + }, + "email": { + "type": "string" + }, + "followers": { + "type": "integer" + }, + "following": { + "type": "integer" + }, + "hireable": { + "type": "boolean" + }, + "location": { + "type": "string" + }, + "name": { + "type": "string" + }, + "owned_private_repos": { + "type": "integer" + }, + "private_gists": { + "type": "integer" + }, + "public_gists": { + "type": "integer" + }, + "public_repos": { + "type": "integer" + }, + "total_private_repos": { + "type": "integer" + }, + "twitter_username": { + "type": "string" + }, + "updated_at": { + "description": "Last profile update time in RFC3339 format.", + "type": "string" + } + }, + "required": [ + "public_repos", + "public_gists", + "followers", + "following", + "created_at", + "updated_at" + ], + "type": [ + "null", + "object" + ] + }, + "id": { + "type": "integer" + }, + "login": { + "type": "string" + }, + "profile_url": { + "type": "string" + } + }, + "required": [ + "login" + ], + "type": [ + "null", + "object" + ] + }, + "files": { + "items": { + "additionalProperties": false, + "properties": { + "additions": { + "type": "integer" + }, + "changes": { + "type": "integer" + }, + "deletions": { + "type": "integer" + }, + "filename": { + "type": "string" + }, + "patch": { + "type": "string" + }, + "status": { + "type": "string" + } + }, + "required": [ + "filename" + ], + "type": "object" + }, + "type": [ + "null", + "array" + ] + }, + "html_url": { + "type": "string" + }, + "sha": { + "type": "string" + }, + "stats": { + "additionalProperties": false, + "properties": { + "additions": { + "type": "integer" + }, + "deletions": { + "type": "integer" + }, + "total": { + "type": "integer" + } + }, + "type": [ + "null", + "object" + ] + } + }, + "required": [ + "sha", + "html_url" + ], + "type": "object" + } } \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_commits.snap b/pkg/github/__toolsnaps__/list_commits.snap index bc4ffd1753..ddd55bac91 100644 --- a/pkg/github/__toolsnaps__/list_commits.snap +++ b/pkg/github/__toolsnaps__/list_commits.snap @@ -5,6 +5,18 @@ "title": "List commits" }, "description": "Get list of commits of a branch in a GitHub repository. Returns at least 30 results per page by default, but can return more if specified using the perPage parameter (up to 100).", + "icons": [ + { + "mimeType": "image/png", + "src": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAABmJLR0QA/wD/AP+gvaeTAAABTklEQVRIie2UPUvDUBSGn5PW2qnYwUXExWhNpY2CUKT+BB10E3R0dnbrppuKLgouHbr5X5qlKRIFB+cuguBHj4O05KYfNsVJ+m7n3Hue54ZcrhDKQqGQTX1YVwg7QIbx0gh8b7VTJMMrqS+5RtgD7oDX2GhhC6UcbhkCVLZVuX1sesex4YCdX6uAGgIrsicjlrTGgQ9KVPDnmQgmgongPwgkXNiOq8ADMAtkx4UGvtflJvus20ANeIlN/vW5/kkt8L3D2HD6P9dRgSI8dQcc9w1IR3acBE3vbFSp8ZMVnoFSqJ/unZDe3pAYXyDIJarn9opbDZrewaAh2y7Oy5S1r6h5QG2Xxbw3piDw6xe244oKyxFmC5ihc+tS1qaqngKJyAEBGmZvSGzHVYT790T7aPozsaFoFZGboFGvDJsbOYuOuxuuc7n1uaV8sRSH8Q1DUVLnYLty3gAAAABJRU5ErkJggg==", + "theme": "light" + }, + { + "mimeType": "image/png", + "src": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAABmJLR0QA/wD/AP+gvaeTAAAA+ElEQVRIie2WLU5DQRRGzwUEirQCQ9BsoSGtZQHgSFgIsg4JXQCGP8V2QCBIEATZkJDgOJi+MJnMo30TFHmfmzvzfeeKyZ2BROpQvVHfrddDmhkZ4BY4Ai6BD7prAowjIoq7i85nFcGNf6qa1tayM1vAvBZQUg74c/WAHtAD/gMgn6YCT8A2MKwOTaZpCfAF3AGvFdlLx7XqdUVw4186rgWeE8Nn4cU67QLNAS/ASG3qmwVPqdaqjWw9A86BK+CkzaTuAseFBse/AiLiQg1gLzs3Bwb8XIp94AxYL/Af2xordap6v/htHKhv6nTlgBUAh9l6Rx11yfgG8ne/zwh2OysAAAAASUVORK5CYII=", + "theme": "dark" + } + ], "inputSchema": { "properties": { "author": { @@ -27,7 +39,8 @@ }, "owner": { "description": "Repository owner", - "type": "string" + "type": "string", + "x-mcp-header": "owner" }, "page": { "description": "Page number for pagination (min 1)", @@ -46,7 +59,8 @@ }, "repo": { "description": "Repository name", - "type": "string" + "type": "string", + "x-mcp-header": "repo" }, "sha": { "description": "Commit SHA, branch or tag name to list commits of. If not provided, uses the default branch of the repository. If a commit SHA is provided, will list commits up to that SHA.", @@ -67,5 +81,268 @@ ], "type": "object" }, - "name": "list_commits" + "name": "list_commits", + "outputSchema": { + "items": { + "additionalProperties": false, + "properties": { + "author": { + "additionalProperties": false, + "properties": { + "avatar_url": { + "type": "string" + }, + "details": { + "additionalProperties": false, + "properties": { + "bio": { + "type": "string" + }, + "blog": { + "type": "string" + }, + "company": { + "type": "string" + }, + "created_at": { + "description": "Account creation time in RFC3339 format.", + "type": "string" + }, + "email": { + "type": "string" + }, + "followers": { + "type": "integer" + }, + "following": { + "type": "integer" + }, + "hireable": { + "type": "boolean" + }, + "location": { + "type": "string" + }, + "name": { + "type": "string" + }, + "owned_private_repos": { + "type": "integer" + }, + "private_gists": { + "type": "integer" + }, + "public_gists": { + "type": "integer" + }, + "public_repos": { + "type": "integer" + }, + "total_private_repos": { + "type": "integer" + }, + "twitter_username": { + "type": "string" + }, + "updated_at": { + "description": "Last profile update time in RFC3339 format.", + "type": "string" + } + }, + "required": [ + "public_repos", + "public_gists", + "followers", + "following", + "created_at", + "updated_at" + ], + "type": [ + "null", + "object" + ] + }, + "id": { + "type": "integer" + }, + "login": { + "type": "string" + }, + "profile_url": { + "type": "string" + } + }, + "required": [ + "login" + ], + "type": [ + "null", + "object" + ] + }, + "commit": { + "additionalProperties": false, + "properties": { + "author": { + "additionalProperties": false, + "properties": { + "date": { + "type": "string" + }, + "email": { + "type": "string" + }, + "name": { + "type": "string" + } + }, + "type": [ + "null", + "object" + ] + }, + "committer": { + "additionalProperties": false, + "properties": { + "date": { + "type": "string" + }, + "email": { + "type": "string" + }, + "name": { + "type": "string" + } + }, + "type": [ + "null", + "object" + ] + }, + "message": { + "type": "string" + } + }, + "required": [ + "message" + ], + "type": [ + "null", + "object" + ] + }, + "committer": { + "additionalProperties": false, + "properties": { + "avatar_url": { + "type": "string" + }, + "details": { + "additionalProperties": false, + "properties": { + "bio": { + "type": "string" + }, + "blog": { + "type": "string" + }, + "company": { + "type": "string" + }, + "created_at": { + "description": "Account creation time in RFC3339 format.", + "type": "string" + }, + "email": { + "type": "string" + }, + "followers": { + "type": "integer" + }, + "following": { + "type": "integer" + }, + "hireable": { + "type": "boolean" + }, + "location": { + "type": "string" + }, + "name": { + "type": "string" + }, + "owned_private_repos": { + "type": "integer" + }, + "private_gists": { + "type": "integer" + }, + "public_gists": { + "type": "integer" + }, + "public_repos": { + "type": "integer" + }, + "total_private_repos": { + "type": "integer" + }, + "twitter_username": { + "type": "string" + }, + "updated_at": { + "description": "Last profile update time in RFC3339 format.", + "type": "string" + } + }, + "required": [ + "public_repos", + "public_gists", + "followers", + "following", + "created_at", + "updated_at" + ], + "type": [ + "null", + "object" + ] + }, + "id": { + "type": "integer" + }, + "login": { + "type": "string" + }, + "profile_url": { + "type": "string" + } + }, + "required": [ + "login" + ], + "type": [ + "null", + "object" + ] + }, + "html_url": { + "type": [ + "null", + "string" + ] + }, + "sha": { + "type": [ + "null", + "string" + ] + } + }, + "type": "object" + }, + "type": [ + "null", + "array" + ] + } } \ No newline at end of file diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index f1c0194efc..df3dac1af9 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -26,8 +26,17 @@ import ( "github.com/shurcooL/githubv4" ) +type GetCommitInput struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + SHA string `json:"sha"` + Detail string `json:"detail,omitempty"` + Page *int `json:"page,omitempty"` + PerPage *int `json:"perPage,omitempty"` +} + func GetCommit(t translations.TranslationHelperFunc) inventory.ServerTool { - return NewTool( + return NewTool[GetCommitInput, MinimalCommit]( ToolsetMetadataRepos, mcp.Tool{ Name: "get_commit", @@ -62,30 +71,26 @@ func GetCommit(t translations.TranslationHelperFunc) inventory.ServerTool { }), }, scopes.PublicRead(scopes.Repo), - func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - owner, err := RequiredParam[string](args, "owner") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, input GetCommitInput) (*mcp.CallToolResult, MinimalCommit, error) { + if input.Owner == "" { + return utils.NewToolResultError("missing required parameter: owner"), MinimalCommit{}, nil } - repo, err := RequiredParam[string](args, "repo") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + if input.Repo == "" { + return utils.NewToolResultError("missing required parameter: repo"), MinimalCommit{}, nil } - sha, err := RequiredParam[string](args, "sha") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + if input.SHA == "" { + return utils.NewToolResultError("missing required parameter: sha"), MinimalCommit{}, nil } - detailRaw, err := OptionalParam[string](args, "detail") + detail, err := parseCommitDetail(input.Detail) if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + return utils.NewToolResultError(err.Error()), MinimalCommit{}, nil } - detail, err := parseCommitDetail(detailRaw) - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + pagination := PaginationParams{Page: 1, PerPage: 30} + if input.Page != nil { + pagination.Page = *input.Page } - pagination, err := OptionalPaginationParams(args) - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + if input.PerPage != nil { + pagination.PerPage = *input.PerPage } opts := &github.ListOptions{ @@ -95,24 +100,24 @@ func GetCommit(t translations.TranslationHelperFunc) inventory.ServerTool { client, err := deps.GetClient(ctx) if err != nil { - return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + return nil, MinimalCommit{}, fmt.Errorf("failed to get GitHub client: %w", err) } - commit, resp, err := client.Repositories.GetCommit(ctx, owner, repo, sha, opts) + commit, resp, err := client.Repositories.GetCommit(ctx, input.Owner, input.Repo, input.SHA, opts) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, - fmt.Sprintf("failed to get commit: %s", sha), + fmt.Sprintf("failed to get commit: %s", input.SHA), resp, err, - ), nil, nil + ), MinimalCommit{}, nil } defer func() { _ = resp.Body.Close() }() if resp.StatusCode != 200 { body, err := io.ReadAll(resp.Body) if err != nil { - return nil, nil, fmt.Errorf("failed to read response body: %w", err) + return nil, MinimalCommit{}, fmt.Errorf("failed to read response body: %w", err) } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get commit", resp, body), nil, nil + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get commit", resp, body), MinimalCommit{}, nil } // Convert to minimal commit @@ -120,7 +125,7 @@ func GetCommit(t translations.TranslationHelperFunc) inventory.ServerTool { r, err := json.Marshal(minimalCommit) if err != nil { - return nil, nil, fmt.Errorf("failed to marshal response: %w", err) + return nil, MinimalCommit{}, fmt.Errorf("failed to marshal response: %w", err) } result := utils.NewToolResultText(string(r)) @@ -128,12 +133,61 @@ func GetCommit(t translations.TranslationHelperFunc) inventory.ServerTool { // repos anyone can land it via a PR (untrusted), in private repos // only collaborators can (trusted). Confidentiality follows repo // visibility. - result = attachRepoVisibilityIFCLabel(ctx, deps, client, owner, repo, result, ifc.LabelCommitContents) - return result, nil, nil + result = attachRepoVisibilityIFCLabel(ctx, deps, client, input.Owner, input.Repo, result, ifc.LabelCommitContents) + return result, minimalCommit, nil }, + normalizeTypedReadArguments(nil, false), ) } +type ListCommitsInput struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + SHA string `json:"sha,omitempty"` + Author string `json:"author,omitempty"` + Path string `json:"path,omitempty"` + Since string `json:"since,omitempty"` + Until string `json:"until,omitempty"` + Fields []string `json:"fields,omitempty"` + Page *int `json:"page,omitempty"` + PerPage *int `json:"perPage,omitempty"` +} + +type ListCommitOutput struct { + SHA *string `json:"sha,omitempty"` + HTMLURL *string `json:"html_url,omitempty"` + Commit *MinimalCommitInfo `json:"commit,omitempty"` + Author *MinimalUser `json:"author,omitempty"` + Committer *MinimalUser `json:"committer,omitempty"` +} + +func structuredListCommitsOutput(commits []MinimalCommit, fields []string) []ListCommitOutput { + output := make([]ListCommitOutput, 0, len(commits)) + selected := func(field string) bool { + return len(fields) == 0 || slices.Contains(fields, field) + } + for _, commit := range commits { + item := ListCommitOutput{} + if selected("sha") { + item.SHA = new(commit.SHA) + } + if selected("html_url") { + item.HTMLURL = new(commit.HTMLURL) + } + if selected("commit") { + item.Commit = commit.Commit + } + if selected("author") { + item.Author = commit.Author + } + if selected("committer") { + item.Committer = commit.Committer + } + output = append(output, item) + } + return output +} + // ListCommits creates a tool to get the list of commits of a branch in a GitHub // repository. func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { @@ -177,7 +231,7 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { ) WithPagination(schema) - return NewTool( + return NewTool[ListCommitsInput, []ListCommitOutput]( ToolsetMetadataRepos, mcp.Tool{ Name: "list_commits", @@ -189,42 +243,19 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { InputSchema: schema, }, scopes.PublicRead(scopes.Repo), - func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - owner, err := RequiredParam[string](args, "owner") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - repo, err := RequiredParam[string](args, "repo") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - sha, err := OptionalParam[string](args, "sha") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - author, err := OptionalParam[string](args, "author") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - path, err := OptionalParam[string](args, "path") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - fields, err := OptionalStringArrayParam(args, "fields") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, input ListCommitsInput) (*mcp.CallToolResult, []ListCommitOutput, error) { + if input.Owner == "" { + return utils.NewToolResultError("missing required parameter: owner"), nil, nil } - sinceStr, err := OptionalParam[string](args, "since") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + if input.Repo == "" { + return utils.NewToolResultError("missing required parameter: repo"), nil, nil } - untilStr, err := OptionalParam[string](args, "until") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + pagination := PaginationParams{Page: 1, PerPage: 30} + if input.Page != nil { + pagination.Page = *input.Page } - pagination, err := OptionalPaginationParams(args) - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + if input.PerPage != nil { + pagination.PerPage = *input.PerPage } // Set default perPage to 30 if not provided perPage := pagination.PerPage @@ -232,23 +263,23 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { perPage = 30 } opts := &github.CommitsListOptions{ - SHA: sha, - Path: path, - Author: author, + SHA: input.SHA, + Path: input.Path, + Author: input.Author, ListOptions: github.ListOptions{ Page: pagination.Page, PerPage: perPage, }, } - if sinceStr != "" { - sinceTime, err := parseISOTimestamp(sinceStr) + if input.Since != "" { + sinceTime, err := parseISOTimestamp(input.Since) if err != nil { return utils.NewToolResultError(fmt.Sprintf("invalid since timestamp: %s", err)), nil, nil } opts.Since = sinceTime } - if untilStr != "" { - untilTime, err := parseISOTimestamp(untilStr) + if input.Until != "" { + untilTime, err := parseISOTimestamp(input.Until) if err != nil { return utils.NewToolResultError(fmt.Sprintf("invalid until timestamp: %s", err)), nil, nil } @@ -259,10 +290,10 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { if err != nil { return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) } - commits, resp, err := client.Repositories.ListCommits(ctx, owner, repo, opts) + commits, resp, err := client.Repositories.ListCommits(ctx, input.Owner, input.Repo, opts) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, - fmt.Sprintf("failed to list commits: %s", sha), + fmt.Sprintf("failed to list commits: %s", input.SHA), resp, err, ), nil, nil @@ -285,8 +316,8 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { filtered := false var payload any = minimalCommits - if len(fields) > 0 { - filteredCommits, err := filterEachField(minimalCommits, fields) + if len(input.Fields) > 0 { + filteredCommits, err := filterEachField(minimalCommits, input.Fields) if err != nil { return utils.NewToolResultErrorFromErr("failed to filter commits", err), nil, nil } @@ -305,9 +336,10 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { // Commit content is reachable from the repo's history; integrity // follows the same public-untrusted / private-trusted rule as file // contents. Confidentiality follows repo visibility. - result = attachRepoVisibilityIFCLabel(ctx, deps, client, owner, repo, result, ifc.LabelCommitContents) - return result, nil, nil + result = attachRepoVisibilityIFCLabel(ctx, deps, client, input.Owner, input.Repo, result, ifc.LabelCommitContents) + return result, structuredListCommitsOutput(minimalCommits, input.Fields), nil }, + normalizeTypedReadArguments(nil, false), ) } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 0c930ae8c0..11c6d210a4 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -1508,7 +1508,6 @@ func Test_GetCommit(t *testing.T) { // Verify tool definition once serverTool := GetCommit(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) schema, ok := tool.InputSchema.(*jsonschema.Schema) require.True(t, ok, "InputSchema should be *jsonschema.Schema") @@ -1751,7 +1750,6 @@ func Test_ListCommits(t *testing.T) { // Verify tool definition once serverTool := ListCommits(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) schema, ok := tool.InputSchema.(*jsonschema.Schema) require.True(t, ok, "InputSchema should be *jsonschema.Schema") diff --git a/pkg/github/typed_read_normalizers.go b/pkg/github/typed_read_normalizers.go index 6dcd56b658..20ba37ab94 100644 --- a/pkg/github/typed_read_normalizers.go +++ b/pkg/github/typed_read_normalizers.go @@ -36,9 +36,6 @@ func normalizeTypedReadArguments(uppercaseFields []string, preserveZeroPage bool if !exists { continue } - if field == "page" && preserveZeroPage { - continue - } var number any if err := json.Unmarshal(value, &number); err != nil { return nil, err diff --git a/pkg/github/typed_repository_commit_outputs_test.go b/pkg/github/typed_repository_commit_outputs_test.go new file mode 100644 index 0000000000..9c1218eab5 --- /dev/null +++ b/pkg/github/typed_repository_commit_outputs_test.go @@ -0,0 +1,218 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "testing" + + "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/google/go-github/v92/github" + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTypedRepositoryCommitPaginationDefaults(t *testing.T) { + for _, protocol := range []string{"2025-11-25", inventory.ProtocolVersionMultiRoundTrip} { + t.Run(protocol, func(t *testing.T) { + for _, tool := range []inventory.ServerTool{ + GetCommit(translations.NullTranslationHelper), + ListCommits(translations.NullTranslationHelper), + } { + t.Run(tool.Tool.Name, func(t *testing.T) { + for _, tc := range []struct { + name string + args map[string]any + }{ + {"omitted", map[string]any{}}, + {"numeric_zero", map[string]any{"page": 0, "perPage": 0}}, + {"string_zero", map[string]any{"page": "0", "perPage": "0"}}, + } { + t.Run(tc.name, func(t *testing.T) { + calls := 0 + handler := func(w http.ResponseWriter, r *http.Request) { + calls++ + assert.Equal(t, "1", r.URL.Query().Get("page")) + assert.Equal(t, "30", r.URL.Query().Get("per_page")) + var response any = &github.RepositoryCommit{SHA: new("abc123")} + if tool.Tool.Name == "list_commits" { + response = []*github.RepositoryCommit{{SHA: new("abc123")}} + } + mockResponse(t, http.StatusOK, response)(w, r) + } + deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposCommitsByOwnerByRepoByRef: handler, + GetReposCommitsByOwnerByRepo: handler, + }))} + server := mcp.NewServer(&mcp.Implementation{Name: "commit-pagination-test", Version: "v1"}, nil) + server.AddReceivingMiddleware(InjectDepsMiddleware(deps)) + tool.RegisterFunc(server, deps) + session := connectCommentVisibilityClient(t, server, protocol) + tc.args["owner"] = "owner" + tc.args["repo"] = "repo" + if tool.Tool.Name == "get_commit" { + tc.args["sha"] = "abc123" + } + result, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: tool.Tool.Name, Arguments: tc.args, + }) + require.NoError(t, err) + require.False(t, result.IsError, "%+v", result) + assert.Equal(t, 1, calls) + request := createMCPRequest(tc.args) + directResult, err := tool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, directResult.IsError, "%+v", directResult) + assert.Equal(t, 2, calls) + }) + } + }) + } + }) + } +} +func TestTypedRepositoryCommitOutputs(t *testing.T) { + commit := &github.RepositoryCommit{ + SHA: new("abc123"), + HTMLURL: new("https://github.com/owner/repo/commit/abc123"), + Commit: &github.Commit{Message: new("A commit")}, + } + handlers := map[string]http.HandlerFunc{ + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, &github.Repository{Private: new(false)}), + GetReposCommitsByOwnerByRepoByRef: func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "2", r.URL.Query().Get("page")) + assert.Equal(t, "7", r.URL.Query().Get("per_page")) + mockResponse(t, http.StatusOK, commit)(w, r) + }, + GetReposCommitsByOwnerByRepo: func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "4", r.URL.Query().Get("page")) + assert.Equal(t, "9", r.URL.Query().Get("per_page")) + if r.URL.Query().Get("sha") == "null" { + mockResponse(t, http.StatusOK, nil)(w, r) + return + } + if r.URL.Query().Get("sha") == "empty" { + mockResponse(t, http.StatusOK, []*github.RepositoryCommit{})(w, r) + return + } + mockResponse(t, http.StatusOK, []*github.RepositoryCommit{commit})(w, r) + }, + } + deps := BaseDeps{ + Client: mustNewGHClient(t, MockHTTPClientWithHandlers(handlers)), + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), + } + tools := []inventory.ServerTool{ + GetCommit(translations.NullTranslationHelper), + ListCommits(translations.NullTranslationHelper), + } + expectedGet := `{"sha":"abc123","html_url":"https://github.com/owner/repo/commit/abc123","commit":{"message":"A commit"}}` + expectedList := `[{"sha":"abc123","html_url":"https://github.com/owner/repo/commit/abc123","commit":{"message":"A commit"}}]` + + for _, protocolVersion := range []string{"2025-11-25", inventory.ProtocolVersionMultiRoundTrip} { + t.Run(protocolVersion, func(t *testing.T) { + server := mcp.NewServer(&mcp.Implementation{Name: "typed-repository-commit-test", Version: "v1"}, nil) + server.AddReceivingMiddleware(InjectDepsMiddleware(deps)) + for _, tool := range tools { + tool.RegisterFunc(server, deps) + } + + session := connectCommentVisibilityClient(t, server, protocolVersion) + list, err := session.ListTools(context.Background(), nil) + require.NoError(t, err) + require.Len(t, list.Tools, len(tools)) + outputSchemas := make(map[string]*mcp.Tool, len(list.Tools)) + for _, tool := range list.Tools { + outputSchemas[tool.Name] = tool + if protocolVersion == "2025-11-25" { + assert.Nil(t, tool.OutputSchema, "%s must not expose outputSchema to legacy clients", tool.Name) + } else { + require.NotNil(t, tool.OutputSchema, "%s must expose its typed output schema", tool.Name) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + } + } + + calls := []struct { + name string + args map[string]any + text string + }{ + { + name: "get_commit", + args: map[string]any{"owner": "owner", "repo": "repo", "sha": "abc123", "detail": "none", "page": "2", "perPage": "7"}, + text: expectedGet, + }, + { + name: "list_commits", + args: map[string]any{"owner": "owner", "repo": "repo", "page": "4", "perPage": "9"}, + text: expectedList, + }, + { + name: "list_commits", + args: map[string]any{"owner": "owner", "repo": "repo", "page": "4", "perPage": "9", "fields": []any{"sha"}}, + text: `[{"sha":"abc123"}]`, + }, + { + name: "list_commits", + args: map[string]any{"owner": "owner", "repo": "repo", "page": "4", "perPage": "9", "fields": []any{"commit"}}, + text: `[{"commit":{"message":"A commit"}}]`, + }, + { + name: "list_commits", + args: map[string]any{"owner": "owner", "repo": "repo", "page": "4", "perPage": "9", "sha": "empty"}, + text: `[]`, + }, + { + name: "list_commits", + args: map[string]any{"owner": "owner", "repo": "repo", "page": "4", "perPage": "9", "sha": "empty", "fields": []any{"sha", "commit"}}, + text: `[]`, + }, + { + name: "list_commits", + args: map[string]any{"owner": "owner", "repo": "repo", "page": "4", "perPage": "9", "sha": "null"}, + text: `[]`, + }, + } + for _, call := range calls { + result, err := session.CallTool(context.Background(), &mcp.CallToolParams{Name: call.name, Arguments: call.args}) + require.NoError(t, err, call.name) + require.False(t, result.IsError, "%s: %s", call.name, result) + require.Len(t, result.Content, 1, "%s must preserve one legacy text result", call.name) + assert.NotNil(t, result.Meta["ifc"], "typed and legacy outputs retain IFC labeling") + text := getTextResult(t, result).Text + if protocolVersion == "2025-11-25" { + assert.Equal(t, call.text, text, "%s legacy text is byte-exact", call.name) + assert.Nil(t, result.StructuredContent) + continue + } + + require.NotNil(t, result.StructuredContent, "%s must return structured content", call.name) + structuredJSON := mustMarshalJSON(t, result.StructuredContent) + assert.JSONEq(t, call.text, text) + assert.JSONEq(t, text, structuredJSON, "modern text must serialize the typed DTO") + + var schema jsonschema.Schema + require.NoError(t, json.Unmarshal([]byte(mustMarshalJSON(t, outputSchemas[call.name].OutputSchema)), &schema)) + resolved, err := schema.Resolve(nil) + require.NoError(t, err) + var output any + require.NoError(t, json.Unmarshal([]byte(structuredJSON), &output)) + require.NoError(t, resolved.Validate(output), "%s output must conform to its schema", call.name) + } + for _, call := range []mcp.CallToolParams{ + {Name: "get_commit", Arguments: map[string]any{"owner": "owner", "repo": "repo", "sha": "abc123", "detail": "invalid"}}, + {Name: "list_commits", Arguments: map[string]any{"owner": "owner", "repo": "repo", "since": "invalid"}}, + } { + result, err := session.CallTool(context.Background(), &call) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Nil(t, result.StructuredContent, "errors must not expose a success DTO") + } + + }) + } +}