From b81ca202bb05bf8b37be1b1d25fb86a464920866 Mon Sep 17 00:00:00 2001 From: ningyixi <146685776+ningyixi@users.noreply.github.com> Date: Thu, 15 Oct 2026 03:07:50 +0800 Subject: [PATCH] fix(pull-requests): relax pull_request_review_write required fields for resolve_thread resolve_thread and unresolve_thread only need threadId, but the advertised inputSchema required owner/repo/pullNumber for every method. Schema-validating MCP clients therefore rejected valid calls. Require only method at the schema root, document per-method fields, and validate create/submit/delete vs resolve/unresolve in the argument normalizer. Fixes #3501 --- README.md | 6 +-- docs/feature-flags.md | 6 +-- .../pull_request_review_write.snap | 11 ++--- ...equest_review_write_resolution_reason.snap | 11 ++--- pkg/github/consolidated_pull_request_types.go | 10 ++++ pkg/github/pullrequests.go | 10 ++-- pkg/github/pullrequests_test.go | 46 +++++++++++++++++-- ..._consolidated_pull_request_outputs_test.go | 44 ++++++++++++++++++ 8 files changed, 116 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index 6413b58760..30b62d53f5 100644 --- a/README.md +++ b/README.md @@ -1337,9 +1337,9 @@ For negotiated supported protocol versions `2026-07-28` or newer, the consolidat - `commitID`: SHA of commit to review (string, optional) - `event`: Review action to perform. (string, optional) - `method`: The write operation to perform on pull request review. (string, required) - - `owner`: Repository owner (string, required) - - `pullNumber`: Pull request number (number, required) - - `repo`: Repository name (string, required) + - `owner`: Repository owner (required for create, submit_pending, and delete_pending methods) (string, optional) + - `pullNumber`: Pull request number (required for create, submit_pending, and delete_pending methods) (number, optional) + - `repo`: Repository name (required for create, submit_pending, and delete_pending methods) (string, optional) - `threadId`: The node ID of the review thread (e.g., PRRT_kwDOxxx). Required for resolve_thread and unresolve_thread methods. Get thread IDs from pull_request_read with method get_review_comments. (string, optional) - **search_pull_requests** - Search pull requests diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 2924b9f3d7..d09eb1a5d6 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -414,9 +414,9 @@ as output formatting) won't appear here. - `commitID`: SHA of commit to review (string, optional) - `event`: Review action to perform. (string, optional) - `method`: The write operation to perform on pull request review. (string, required) - - `owner`: Repository owner (string, required) - - `pullNumber`: Pull request number (number, required) - - `repo`: Repository name (string, required) + - `owner`: Repository owner (required for create, submit_pending, and delete_pending methods) (string, optional) + - `pullNumber`: Pull request number (required for create, submit_pending, and delete_pending methods) (number, optional) + - `repo`: Repository name (required for create, submit_pending, and delete_pending methods) (string, optional) - `resolutionReason`: Optional reason for resolving a Copilot code review thread: addressed, wont-fix, or invalid. (string, optional) - `threadId`: The node ID of the review thread (e.g., PRRT_kwDOxxx). Required for resolve_thread and unresolve_thread methods. Get thread IDs from pull_request_read with method get_review_comments. (string, optional) diff --git a/pkg/github/__toolsnaps__/pull_request_review_write.snap b/pkg/github/__toolsnaps__/pull_request_review_write.snap index 70cf6471c1..1475510971 100644 --- a/pkg/github/__toolsnaps__/pull_request_review_write.snap +++ b/pkg/github/__toolsnaps__/pull_request_review_write.snap @@ -36,15 +36,15 @@ "type": "string" }, "owner": { - "description": "Repository owner", + "description": "Repository owner (required for create, submit_pending, and delete_pending methods)", "type": "string" }, "pullNumber": { - "description": "Pull request number", + "description": "Pull request number (required for create, submit_pending, and delete_pending methods)", "type": "number" }, "repo": { - "description": "Repository name", + "description": "Repository name (required for create, submit_pending, and delete_pending methods)", "type": "string" }, "threadId": { @@ -53,10 +53,7 @@ } }, "required": [ - "method", - "owner", - "repo", - "pullNumber" + "method" ], "type": "object" }, diff --git a/pkg/github/__toolsnaps__/pull_request_review_write_resolution_reason.snap b/pkg/github/__toolsnaps__/pull_request_review_write_resolution_reason.snap index 45e19ca7bf..150b36f840 100644 --- a/pkg/github/__toolsnaps__/pull_request_review_write_resolution_reason.snap +++ b/pkg/github/__toolsnaps__/pull_request_review_write_resolution_reason.snap @@ -36,15 +36,15 @@ "type": "string" }, "owner": { - "description": "Repository owner", + "description": "Repository owner (required for create, submit_pending, and delete_pending methods)", "type": "string" }, "pullNumber": { - "description": "Pull request number", + "description": "Pull request number (required for create, submit_pending, and delete_pending methods)", "type": "number" }, "repo": { - "description": "Repository name", + "description": "Repository name (required for create, submit_pending, and delete_pending methods)", "type": "string" }, "resolutionReason": { @@ -57,10 +57,7 @@ } }, "required": [ - "method", - "owner", - "repo", - "pullNumber" + "method" ], "type": "object" }, diff --git a/pkg/github/consolidated_pull_request_types.go b/pkg/github/consolidated_pull_request_types.go index 6d6abaed63..670b506783 100644 --- a/pkg/github/consolidated_pull_request_types.go +++ b/pkg/github/consolidated_pull_request_types.go @@ -594,6 +594,16 @@ func normalizePullRequestReviewWriteArguments(raw json.RawMessage) (json.RawMess if !slices.Contains([]string{"create", "submit_pending", "delete_pending", "resolve_thread", "unresolve_thread"}, params.Method) { return nil, fmt.Errorf("unknown method: %s", params.Method) } + switch params.Method { + case "resolve_thread", "unresolve_thread": + if params.ThreadID == "" { + return nil, fmt.Errorf("threadId is required for resolve_thread and unresolve_thread methods") + } + case "create", "submit_pending", "delete_pending": + if params.Owner == "" || params.Repo == "" || params.PullNumber == 0 { + return nil, fmt.Errorf("owner, repo, and pullNumber are required for %s method", params.Method) + } + } canonical := map[string]any{ "method": params.Method, "owner": params.Owner, "repo": params.Repo, "pullNumber": params.PullNumber, } diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 5545d43964..93cfd9656b 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -2015,15 +2015,15 @@ func pullRequestReviewWrite(t translations.TranslationHelperFunc, withResolution }, "owner": { Type: "string", - Description: "Repository owner", + Description: "Repository owner (required for create, submit_pending, and delete_pending methods)", }, "repo": { Type: "string", - Description: "Repository name", + Description: "Repository name (required for create, submit_pending, and delete_pending methods)", }, "pullNumber": { Type: "number", - Description: "Pull request number", + Description: "Pull request number (required for create, submit_pending, and delete_pending methods)", }, "body": { Type: "string", @@ -2043,7 +2043,9 @@ func pullRequestReviewWrite(t translations.TranslationHelperFunc, withResolution Description: "The node ID of the review thread (e.g., PRRT_kwDOxxx). Required for resolve_thread and unresolve_thread methods. Get thread IDs from pull_request_read with method get_review_comments.", }, }, - Required: []string{"method", "owner", "repo", "pullNumber"}, + // Only method is globally required: resolve_thread / unresolve_thread use + // threadId alone, matching discussion_comment_write and the granular tools. + Required: []string{"method"}, } if withResolutionReason { schema.Properties["resolutionReason"] = &jsonschema.Schema{ diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index d0f8887e6a..8f288f49e9 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -3203,7 +3203,7 @@ func TestCreateAndSubmitPullRequestReview(t *testing.T) { assert.Contains(t, schema.Properties, "body") assert.Contains(t, schema.Properties, "event") assert.Contains(t, schema.Properties, "commitID") - assert.ElementsMatch(t, schema.Required, []string{"method", "owner", "repo", "pullNumber"}) + assert.ElementsMatch(t, schema.Required, []string{"method"}) tests := []struct { name string @@ -3460,7 +3460,7 @@ func TestCreatePendingPullRequestReview(t *testing.T) { assert.Contains(t, schema.Properties, "repo") assert.Contains(t, schema.Properties, "pullNumber") assert.Contains(t, schema.Properties, "commitID") - assert.ElementsMatch(t, schema.Required, []string{"method", "owner", "repo", "pullNumber"}) + assert.ElementsMatch(t, schema.Required, []string{"method"}) tests := []struct { name string @@ -3918,7 +3918,7 @@ func TestSubmitPendingPullRequestReview(t *testing.T) { assert.Contains(t, schema.Properties, "pullNumber") assert.Contains(t, schema.Properties, "event") assert.Contains(t, schema.Properties, "body") - assert.ElementsMatch(t, schema.Required, []string{"method", "owner", "repo", "pullNumber"}) + assert.ElementsMatch(t, schema.Required, []string{"method"}) tests := []struct { name string @@ -4019,7 +4019,7 @@ func TestDeletePendingPullRequestReview(t *testing.T) { assert.Contains(t, schema.Properties, "owner") assert.Contains(t, schema.Properties, "repo") assert.Contains(t, schema.Properties, "pullNumber") - assert.ElementsMatch(t, schema.Required, []string{"method", "owner", "repo", "pullNumber"}) + assert.ElementsMatch(t, schema.Required, []string{"method"}) tests := []struct { name string @@ -4681,6 +4681,12 @@ func TestAddReplyToPullRequestComment(t *testing.T) { func TestResolveReviewThread(t *testing.T) { t.Parallel() + serverTool := PullRequestReviewWrite(translations.NullTranslationHelper) + schema := serverTool.Tool.InputSchema.(*jsonschema.Schema) + assert.ElementsMatch(t, schema.Required, []string{"method"}) + assert.Contains(t, schema.Properties["owner"].Description, "required for create") + assert.Contains(t, schema.Properties["threadId"].Description, "Required for resolve_thread") + tests := []struct { name string requestArgs map[string]any @@ -4725,6 +4731,38 @@ func TestResolveReviewThread(t *testing.T) { ), expectedResult: "review thread resolved successfully", }, + { + name: "successful resolve thread without owner repo pullNumber", + requestArgs: map[string]any{ + "method": "resolve_thread", + "threadId": "PRRT_kwDOTest123", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + ResolveReviewThread struct { + Thread struct { + ID githubv4.ID + IsResolved githubv4.Boolean + } + } `graphql:"resolveReviewThread(input: $input)"` + }{}, + ResolveReviewThreadInput{ + ThreadID: githubv4.ID("PRRT_kwDOTest123"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "resolveReviewThread": map[string]any{ + "thread": map[string]any{ + "id": "PRRT_kwDOTest123", + "isResolved": true, + }, + }, + }), + ), + ), + expectedResult: "review thread resolved successfully", + }, { name: "successful resolve thread with resolution reason", withResolutionReason: true, diff --git a/pkg/github/typed_consolidated_pull_request_outputs_test.go b/pkg/github/typed_consolidated_pull_request_outputs_test.go index 60afd2f1f1..880ddb059e 100644 --- a/pkg/github/typed_consolidated_pull_request_outputs_test.go +++ b/pkg/github/typed_consolidated_pull_request_outputs_test.go @@ -632,3 +632,47 @@ func TestConsolidatedPullRequestOutputTypesAreCurated(t *testing.T) { check(typ) } } + +func TestPullRequestReviewWriteResolveThreadSchema(t *testing.T) { + // Advertised schema must not require owner/repo/pullNumber so MCP clients can + // call resolve_thread / unresolve_thread with only threadId (#3501). + for _, protocol := range typedPullRequestProtocols { + t.Run("protocol="+protocol, func(t *testing.T) { + session, _ := consolidatedPullRequestSession(t, consolidatedPullRequestDeps(t), protocol, false) + list, err := session.ListTools(context.Background(), nil) + require.NoError(t, err) + var schema map[string]any + for _, tool := range list.Tools { + if tool.Name != "pull_request_review_write" { + continue + } + require.NoError(t, json.Unmarshal([]byte(mustMarshalJSON(t, tool.InputSchema)), &schema)) + } + require.NotEmpty(t, schema) + required, _ := schema["required"].([]any) + assert.Equal(t, []any{"method"}, required) + + result, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: "pull_request_review_write", + Arguments: map[string]any{ + "method": "resolve_thread", + "threadId": "T_1", + }, + }) + require.NoError(t, err) + require.False(t, result.IsError, mustMarshalJSON(t, result)) + assert.Contains(t, getTextResult(t, result).Text, "review thread resolved successfully") + + missing, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: "pull_request_review_write", + Arguments: map[string]any{ + "method": "create", + "pullNumber": 1, + }, + }) + require.NoError(t, err) + require.True(t, missing.IsError) + assert.Contains(t, getErrorResult(t, missing).Text, "owner, repo, and pullNumber are required for create method") + }) + } +}