From ccf54a5504655f9567b1b86e04c9942e359950ee Mon Sep 17 00:00:00 2001 From: gustavo-sec <289506718+gustavo-sec@users.noreply.github.com> Date: Tue, 16 Jun 2026 23:14:15 -0300 Subject: [PATCH] Enforce and advertise the method enum for method-dispatch tools The consolidated method-dispatch tools returned a bare "unknown method: " with no hint of valid values, and each tool's valid method set lived only in its schema enum (or, for sub_issue_write, only in prose). labels.go already listed the supported methods on an unknown value; this brings the rest in line and makes the advertised methods the single source of truth. - Each tool's method set is a single []string that feeds both the input-schema enum (methodEnum) and the unknown-method error (unknownMethodError), so the advertised methods and the accepted methods cannot drift apart. - An unknown method now lists the supported set: "unknown method: x. Supported methods are: ...". - pull_request_review_write decoded args with WeakDecode (no RequiredParam), so an omitted method surfaced as "unknown method: " (empty). It now reports "missing required parameter: method", consistent with the other tools. - sub_issue_write advertised its method options only in the description; it now declares the enum like every other method-dispatch tool. Closes #2712 --- pkg/github/__toolsnaps__/sub_issue_write.snap | 5 +++ pkg/github/actions.go | 39 +++++++------------ pkg/github/issues.go | 14 +++++-- pkg/github/params.go | 23 +++++++++++ pkg/github/params_test.go | 16 ++++++++ pkg/github/projects.go | 38 +++++++----------- pkg/github/projects_test.go | 4 +- pkg/github/pullrequests.go | 24 ++++++++++-- pkg/github/pullrequests_test.go | 14 +++++++ pkg/github/ui_tools.go | 8 +++- pkg/github/ui_tools_test.go | 2 +- 11 files changed, 129 insertions(+), 58 deletions(-) diff --git a/pkg/github/__toolsnaps__/sub_issue_write.snap b/pkg/github/__toolsnaps__/sub_issue_write.snap index 1e4fcceabf..ad22767ac1 100644 --- a/pkg/github/__toolsnaps__/sub_issue_write.snap +++ b/pkg/github/__toolsnaps__/sub_issue_write.snap @@ -19,6 +19,11 @@ }, "method": { "description": "The action to perform on a single sub-issue\nOptions are:\n- 'add' - add a sub-issue to a parent issue in a GitHub repository.\n- 'remove' - remove a sub-issue from a parent issue in a GitHub repository.\n- 'reprioritize' - change the order of sub-issues within a parent issue in a GitHub repository. Use either 'after_id' or 'before_id' to specify the new position.\n\t\t\t\t", + "enum": [ + "add", + "remove", + "reprioritize" + ], "type": "string" }, "owner": { diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 9dac877736..18c2395284 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -46,6 +46,15 @@ const ( actionsMethodDeleteWorkflowRunLogs = "delete_workflow_run_logs" ) +// Method sets for the consolidated actions tools. Each slice is the single +// source for its tool: it feeds both the schema enum (methodEnum) and the +// unknown-method error (unknownMethodError). Order matches the advertised enum. +var ( + actionsWorkflowListMethods = []string{actionsMethodListWorkflows, actionsMethodListWorkflowRuns, actionsMethodListWorkflowJobs, actionsMethodListWorkflowArtifacts} + actionsWorkflowGetMethods = []string{actionsMethodGetWorkflow, actionsMethodGetWorkflowRun, actionsMethodGetWorkflowJob, actionsMethodDownloadWorkflowArtifact, actionsMethodGetWorkflowRunUsage, actionsMethodGetWorkflowRunLogsURL} + actionsWorkflowRunMethods = []string{actionsMethodRunWorkflow, actionsMethodRerunWorkflowRun, actionsMethodRerunFailedJobs, actionsMethodCancelWorkflowRun, actionsMethodDeleteWorkflowRunLogs} +) + // handleFailedJobLogs gets logs for all failed jobs in a workflow run func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo string, runID int64, returnContent bool, tailLines int, contentWindowSize int) (*mcp.CallToolResult, any, error) { // First, get all jobs for the workflow run @@ -217,12 +226,7 @@ Use this tool to list workflows in a repository, or list workflow runs, jobs, an "method": { Type: "string", Description: "The action to perform", - Enum: []any{ - actionsMethodListWorkflows, - actionsMethodListWorkflowRuns, - actionsMethodListWorkflowJobs, - actionsMethodListWorkflowArtifacts, - }, + Enum: methodEnum(actionsWorkflowListMethods), }, "owner": { Type: "string", @@ -397,7 +401,7 @@ Use this tool to list workflows in a repository, or list workflow runs, jobs, an result, payload, err := listWorkflowArtifacts(ctx, client, owner, repo, resourceIDInt, pagination) return attachIFC(result), payload, err default: - return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil + return unknownMethodError(method, actionsWorkflowListMethods), nil, nil } }, ) @@ -423,14 +427,7 @@ Use this tool to get details about individual workflows, workflow runs, jobs, an "method": { Type: "string", Description: "The method to execute", - Enum: []any{ - actionsMethodGetWorkflow, - actionsMethodGetWorkflowRun, - actionsMethodGetWorkflowJob, - actionsMethodDownloadWorkflowArtifact, - actionsMethodGetWorkflowRunUsage, - actionsMethodGetWorkflowRunLogsURL, - }, + Enum: methodEnum(actionsWorkflowGetMethods), }, "owner": { Type: "string", @@ -519,7 +516,7 @@ Use this tool to get details about individual workflows, workflow runs, jobs, an result, payload, err := getWorkflowRunLogsURL(ctx, client, owner, repo, resourceIDInt) return attachIFC(result), payload, err default: - return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil + return unknownMethodError(method, actionsWorkflowGetMethods), nil, nil } }, ) @@ -544,13 +541,7 @@ func ActionsRunTrigger(t translations.TranslationHelperFunc) inventory.ServerToo "method": { Type: "string", Description: "The method to execute", - Enum: []any{ - actionsMethodRunWorkflow, - actionsMethodRerunWorkflowRun, - actionsMethodRerunFailedJobs, - actionsMethodCancelWorkflowRun, - actionsMethodDeleteWorkflowRunLogs, - }, + Enum: methodEnum(actionsWorkflowRunMethods), }, "owner": { Type: "string", @@ -636,7 +627,7 @@ func ActionsRunTrigger(t translations.TranslationHelperFunc) inventory.ServerToo case actionsMethodDeleteWorkflowRunLogs: return deleteWorkflowRunLogs(ctx, client, owner, repo, int64(runID)) default: - return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil + return unknownMethodError(method, actionsWorkflowRunMethods), nil, nil } }, ) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 8ed3b90571..e127018ee5 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -24,6 +24,13 @@ import ( "github.com/shurcooL/githubv4" ) +// Method sets for the consolidated issue tools (single source for the schema +// enum via methodEnum and the unknown-method error via unknownMethodError). +var ( + issueReadMethods = []string{"get", "get_comments", "get_sub_issues", "get_labels"} + subIssueWriteMethods = []string{"add", "remove", "reprioritize"} +) + // CloseIssueInput represents the input for closing an issue via the GraphQL API. // Used to extend the functionality of the githubv4 library to support closing issues as duplicates. type CloseIssueInput struct { @@ -736,7 +743,7 @@ Options are: 3. get_sub_issues - Get sub-issues of the issue. 4. get_labels - Get labels assigned to the issue. `, - Enum: []any{"get", "get_comments", "get_sub_issues", "get_labels"}, + Enum: methodEnum(issueReadMethods), }, "owner": { Type: "string", @@ -820,7 +827,7 @@ Options are: result, err := GetIssueLabels(ctx, gqlClient, owner, repo, issueNumber) return attachIFC(result), nil, err default: - return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil + return unknownMethodError(method, issueReadMethods), nil, nil } }) } @@ -1234,6 +1241,7 @@ func SubIssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { Properties: map[string]*jsonschema.Schema{ "method": { Type: "string", + Enum: methodEnum(subIssueWriteMethods), Description: `The action to perform on a single sub-issue Options are: - 'add' - add a sub-issue to a parent issue in a GitHub repository. @@ -1327,7 +1335,7 @@ Options are: result, err := ReprioritizeSubIssue(ctx, client, owner, repo, issueNumber, subIssueID, afterID, beforeID) return result, nil, err default: - return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil + return unknownMethodError(method, subIssueWriteMethods), nil, nil } }) st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular} diff --git a/pkg/github/params.go b/pkg/github/params.go index a6b43375ef..805f86d02d 100644 --- a/pkg/github/params.go +++ b/pkg/github/params.go @@ -5,9 +5,12 @@ import ( "fmt" "math" "strconv" + "strings" + "github.com/github/github-mcp-server/pkg/utils" "github.com/google/go-github/v87/github" "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" ) // OptionalParamOK is a helper function that can be used to fetch a requested parameter from the request. @@ -488,3 +491,23 @@ func (p PaginationParams) ToGraphQLParams() (*GraphQLPaginationParams, error) { } return cursor.ToGraphQLParams() } + +// methodEnum renders a tool's supported method list as the []any value the +// input-schema Enum field expects. Declaring the enum from the same []string +// the handler validates against keeps the advertised methods and the runtime +// check from drifting apart. +func methodEnum(methods []string) []any { + out := make([]any, len(methods)) + for i, m := range methods { + out[i] = m + } + return out +} + +// unknownMethodError is the tool-result error returned when a method-dispatch +// tool is called with a value outside its advertised method enum. Listing the +// supported methods lets the caller (often an LLM) self-correct. supported must +// be the same list advertised in the tool's input-schema enum. +func unknownMethodError(method string, supported []string) *mcp.CallToolResult { + return utils.NewToolResultError(fmt.Sprintf("unknown method: %s. Supported methods are: %s", method, strings.Join(supported, ", "))) +} diff --git a/pkg/github/params_test.go b/pkg/github/params_test.go index b00efeb10c..06b1e0f6a6 100644 --- a/pkg/github/params_test.go +++ b/pkg/github/params_test.go @@ -642,3 +642,19 @@ func TestOptionalPaginationParams(t *testing.T) { }) } } + +func TestMethodEnum(t *testing.T) { + // methodEnum must preserve methods and order so the advertised schema enum + // stays byte-identical to the source slice. + assert.Equal(t, []any{"create", "submit_pending"}, methodEnum([]string{"create", "submit_pending"})) + assert.Empty(t, methodEnum(nil)) +} + +func TestUnknownMethodError(t *testing.T) { + res := unknownMethodError("bogus", []string{"create", "submit_pending", "delete_pending"}) + assert.NotNil(t, res) + assert.True(t, res.IsError) + txt := getErrorResult(t, res).Text + assert.Contains(t, txt, "unknown method: bogus") + assert.Contains(t, txt, "Supported methods are: create, submit_pending, delete_pending") +} diff --git a/pkg/github/projects.go b/pkg/github/projects.go index 85774490de..4f9bf163e0 100644 --- a/pkg/github/projects.go +++ b/pkg/github/projects.go @@ -51,6 +51,15 @@ const ( projectsMethodCreateIterationField = "create_iteration_field" ) +// Method sets for the consolidated project tools. Single source per tool: feeds +// both the schema enum (methodEnum) and the unknown-method error +// (unknownMethodError). Order matches the advertised enum. +var ( + projectsListMethods = []string{projectsMethodListProjects, projectsMethodListProjectFields, projectsMethodListProjectItems, projectsMethodListProjectStatusUpdates} + projectsGetMethods = []string{projectsMethodGetProject, projectsMethodGetProjectField, projectsMethodGetProjectItem, projectsMethodGetProjectStatusUpdate} + projectsWriteMethods = []string{projectsMethodAddProjectItem, projectsMethodUpdateProjectItem, projectsMethodDeleteProjectItem, projectsMethodCreateProjectStatusUpdate, projectsMethodCreateProject, projectsMethodCreateIterationField} +) + // GraphQL types for ProjectV2 status updates type statusUpdateNode struct { @@ -159,12 +168,7 @@ Use this tool to list projects for a user or organization, or list project field "method": { Type: "string", Description: "The action to perform", - Enum: []any{ - projectsMethodListProjects, - projectsMethodListProjectFields, - projectsMethodListProjectItems, - projectsMethodListProjectStatusUpdates, - }, + Enum: methodEnum(projectsListMethods), }, "owner_type": { Type: "string", @@ -269,7 +273,7 @@ Use this tool to list projects for a user or organization, or list project field result, payload, err := listProjectStatusUpdates(ctx, gqlClient, args, owner, ownerType) return attachIFC(result), payload, err default: - return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil + return unknownMethodError(method, projectsListMethods), nil, nil } } }, @@ -296,12 +300,7 @@ Use this tool to get details about individual projects, project fields, and proj "method": { Type: "string", Description: "The method to execute", - Enum: []any{ - projectsMethodGetProject, - projectsMethodGetProjectField, - projectsMethodGetProjectItem, - projectsMethodGetProjectStatusUpdate, - }, + Enum: methodEnum(projectsGetMethods), }, "owner_type": { Type: "string", @@ -419,7 +418,7 @@ Use this tool to get details about individual projects, project fields, and proj result, payload, err := getProjectItem(ctx, client, owner, ownerType, projectNumber, itemID, fields) return attachIFC(result), payload, err default: - return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil + return unknownMethodError(method, projectsGetMethods), nil, nil } }, ) @@ -444,14 +443,7 @@ func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool { "method": { Type: "string", Description: "The method to execute", - Enum: []any{ - projectsMethodAddProjectItem, - projectsMethodUpdateProjectItem, - projectsMethodDeleteProjectItem, - projectsMethodCreateProjectStatusUpdate, - projectsMethodCreateProject, - projectsMethodCreateIterationField, - }, + Enum: methodEnum(projectsWriteMethods), }, "owner_type": { Type: "string", @@ -669,7 +661,7 @@ func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool { case projectsMethodCreateIterationField: return createIterationField(ctx, gqlClient, owner, ownerType, projectNumber, args) default: - return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil + return unknownMethodError(method, projectsWriteMethods), nil, nil } }, ) diff --git a/pkg/github/projects_test.go b/pkg/github/projects_test.go index ad5ce6db86..b3dc13b020 100644 --- a/pkg/github/projects_test.go +++ b/pkg/github/projects_test.go @@ -93,7 +93,7 @@ func Test_ProjectsList_ListProjects(t *testing.T) { "owner_type": "org", }, expectError: true, - expectedErrMsg: "unknown method: unknown_method", + expectedErrMsg: "unknown method: unknown_method. Supported methods are:", }, } @@ -435,6 +435,7 @@ func Test_ProjectsGet_GetProject(t *testing.T) { require.True(t, result.IsError) textContent := getTextResult(t, result) assert.Contains(t, textContent.Text, "unknown method: unknown_method") + assert.Contains(t, textContent.Text, "Supported methods are:") }) } @@ -850,6 +851,7 @@ func Test_ProjectsWrite_AddProjectItem(t *testing.T) { require.True(t, result.IsError) textContent := getTextResult(t, result) assert.Contains(t, textContent.Text, "unknown method: unknown_method") + assert.Contains(t, textContent.Text, "Supported methods are:") }) } diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 07ff6a87f0..1bfc9f4e0d 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -23,6 +23,13 @@ import ( "github.com/github/github-mcp-server/pkg/utils" ) +// Method sets for the consolidated pull request tools (single source for the +// schema enum via methodEnum and the unknown-method error via unknownMethodError). +var ( + pullRequestReadMethods = []string{"get", "get_diff", "get_status", "get_files", "get_commits", "get_review_comments", "get_reviews", "get_comments", "get_check_runs"} + pullRequestReviewWriteMethods = []string{"create", "submit_pending", "delete_pending", "resolve_thread", "unresolve_thread"} +) + // PullRequestRead creates a tool to get details of a specific pull request. func PullRequestRead(t translations.TranslationHelperFunc) inventory.ServerTool { schema := &jsonschema.Schema{ @@ -42,7 +49,7 @@ Possible options: 8. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned. 9. get_check_runs - Get check runs for the head commit of a pull request. Check runs are the individual CI/CD jobs and checks that run on the PR. `, - Enum: []any{"get", "get_diff", "get_status", "get_files", "get_commits", "get_review_comments", "get_reviews", "get_comments", "get_check_runs"}, + Enum: methodEnum(pullRequestReadMethods), }, "owner": { Type: "string", @@ -155,7 +162,7 @@ Possible options: result, err := GetPullRequestCheckRuns(ctx, client, owner, repo, pullNumber, pagination) return attachIFC(result), nil, err default: - return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil + return unknownMethodError(method, pullRequestReadMethods), nil, nil } }) } @@ -1728,7 +1735,7 @@ func PullRequestReviewWrite(t translations.TranslationHelperFunc) inventory.Serv "method": { Type: "string", Description: `The write operation to perform on pull request review.`, - Enum: []any{"create", "submit_pending", "delete_pending", "resolve_thread", "unresolve_thread"}, + Enum: methodEnum(pullRequestReviewWriteMethods), }, "owner": { Type: "string", @@ -1789,6 +1796,15 @@ Available methods: return utils.NewToolResultError(err.Error()), nil, nil } + // Unlike the other method-dispatch tools, this handler decodes args + // with WeakDecode rather than RequiredParam, so an omitted method + // would otherwise reach the switch default as an empty value. Enforce + // it explicitly so a missing method is reported the same way as every + // other missing required parameter. + if _, err := RequiredParam[string](args, "method"); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + // Given our owner, repo and PR number, lookup the GQL ID of the PR. client, err := deps.GetGQLClient(ctx) if err != nil { @@ -1812,7 +1828,7 @@ Available methods: result, err := ResolveReviewThread(ctx, client, params.ThreadID, false) return result, nil, err default: - return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", params.Method)), nil, nil + return unknownMethodError(params.Method, pullRequestReviewWriteMethods), nil, nil } }) st.FeatureFlagDisable = []string{FeatureFlagPullRequestsGranular} diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 0f372519e5..5438c1acb9 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -3333,6 +3333,20 @@ func TestCreatePendingPullRequestReview(t *testing.T) { expectToolError: true, expectedToolErrMsg: "expected test failure", }, + { + name: "missing method is reported as a required parameter", + mockedClient: githubv4mock.NewMockedHTTPClient(), + requestArgs: map[string]any{"owner": "owner", "repo": "repo", "pullNumber": float64(42)}, + expectToolError: true, + expectedToolErrMsg: "missing required parameter: method", + }, + { + name: "unknown method lists the supported methods", + mockedClient: githubv4mock.NewMockedHTTPClient(), + requestArgs: map[string]any{"method": "bogus", "owner": "owner", "repo": "repo", "pullNumber": float64(42)}, + expectToolError: true, + expectedToolErrMsg: "unknown method: bogus. Supported methods are: create, submit_pending, delete_pending, resolve_thread, unresolve_thread", + }, } for _, tc := range tests { diff --git a/pkg/github/ui_tools.go b/pkg/github/ui_tools.go index 640250dea3..90595034ad 100644 --- a/pkg/github/ui_tools.go +++ b/pkg/github/ui_tools.go @@ -20,6 +20,10 @@ import ( "github.com/shurcooL/githubv4" ) +// uiGetMethods is the single source for ui_get's schema enum (methodEnum) and its +// unknown-method error (unknownMethodError). +var uiGetMethods = []string{"labels", "assignees", "milestones", "issue_types", "branches", "issue_fields", "reviewers"} + // UIGet creates a tool to fetch UI data for MCP Apps. func UIGet(t translations.TranslationHelperFunc) inventory.ServerTool { st := NewTool( @@ -44,7 +48,7 @@ func UIGet(t translations.TranslationHelperFunc) inventory.ServerTool { Properties: map[string]*jsonschema.Schema{ "method": { Type: "string", - Enum: []any{"labels", "assignees", "milestones", "issue_types", "branches", "issue_fields", "reviewers"}, + Enum: methodEnum(uiGetMethods), Description: "The type of data to fetch", }, "owner": { @@ -87,7 +91,7 @@ func UIGet(t translations.TranslationHelperFunc) inventory.ServerTool { case "reviewers": return uiGetReviewers(ctx, deps, args, owner) default: - return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil + return unknownMethodError(method, uiGetMethods), nil, nil } }) st.FeatureFlagEnable = MCPAppsFeatureFlag diff --git a/pkg/github/ui_tools_test.go b/pkg/github/ui_tools_test.go index 2fded6b20e..ef32311277 100644 --- a/pkg/github/ui_tools_test.go +++ b/pkg/github/ui_tools_test.go @@ -313,7 +313,7 @@ func Test_UIGet(t *testing.T) { "repo": "repo", }, expectError: true, - expectedErrMsg: "unknown method: unknown", + expectedErrMsg: "unknown method: unknown. Supported methods are:", }, }