diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 0de5bdd722..71fa65008c 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -349,4 +349,18 @@ runtime behavior (such as output formatting) won't appear here. - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: The name of the repository (string, required) +### `thread_resolution_reason` + +- **pull_request_review_write** - Write operations (create, submit, delete) on pull request reviews + - **Required OAuth Scopes**: `repo` + - `body`: Review comment text (string, optional) + - `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) + - `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/feature_flags.go b/pkg/github/feature_flags.go index 4ecd42b653..27202c5c83 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -34,6 +34,9 @@ const FeatureFlagIssueDependencies = "issue_dependencies" // opt-in. const FeatureFlagDuplicateDetection = "duplicate_detection" +// FeatureFlagThreadResolutionReason exposes resolution reasons for Copilot review threads. +const FeatureFlagThreadResolutionReason = "thread_resolution_reason" + // AllowedFeatureFlags is the allowlist of feature flags that can be enabled // by users via --features CLI flag or X-MCP-Features HTTP header. // Only flags in this list are accepted; unknown flags are silently ignored. @@ -48,6 +51,7 @@ var AllowedFeatureFlags = []string{ FeatureFlagFileBlame, FeatureFlagIssueDependencies, FeatureFlagDuplicateDetection, + FeatureFlagThreadResolutionReason, } // InsidersFeatureFlags is the list of feature flags that insiders mode enables. diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go index 0b73ddeb3b..59df45a9ef 100644 --- a/pkg/github/feature_flags_test.go +++ b/pkg/github/feature_flags_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/github/github-mcp-server/pkg/translations" + "github.com/google/jsonschema-go/jsonschema" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -200,6 +201,11 @@ func TestResolveFeatureFlags(t *testing.T) { insidersMode: false, expectedFlags: []string{FeatureFlagIssuesGranular}, }, + { + name: "thread resolution reason can be directly enabled", + enabledFeatures: []string{FeatureFlagThreadResolutionReason}, + expectedFlags: []string{FeatureFlagThreadResolutionReason}, + }, { name: "insiders does not enable user-only allowed flags", enabledFeatures: nil, @@ -227,3 +233,55 @@ func TestResolveFeatureFlags(t *testing.T) { }) } } + +func TestThreadResolutionReasonToolVariants(t *testing.T) { + tests := []struct { + name string + flags []string + toolName string + hasReason bool + }{ + { + name: "consolidated flag off", + toolName: "pull_request_review_write", + }, + { + name: "consolidated flag on", + flags: []string{FeatureFlagThreadResolutionReason}, + toolName: "pull_request_review_write", + hasReason: true, + }, + { + name: "granular flag off", + flags: []string{FeatureFlagPullRequestsGranular}, + toolName: "resolve_review_thread", + }, + { + name: "granular flag on", + flags: []string{FeatureFlagPullRequestsGranular, FeatureFlagThreadResolutionReason}, + toolName: "resolve_review_thread", + hasReason: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + inv, err := NewInventory(translations.NullTranslationHelper). + WithToolsets([]string{"all"}). + WithFeatureChecker(featureCheckerFor(tt.flags...)). + Build() + require.NoError(t, err) + + var matches []inventory.ServerTool + for _, tool := range inv.AvailableTools(context.Background()) { + if tool.Tool.Name == tt.toolName { + matches = append(matches, tool) + } + } + require.Len(t, matches, 1) + schema := matches[0].Tool.InputSchema.(*jsonschema.Schema) + _, hasReason := schema.Properties["resolutionReason"] + assert.Equal(t, tt.hasReason, hasReason) + }) + } +} diff --git a/pkg/github/granular_tools_test.go b/pkg/github/granular_tools_test.go index d70dd568dc..9ef7559e8b 100644 --- a/pkg/github/granular_tools_test.go +++ b/pkg/github/granular_tools_test.go @@ -23,7 +23,7 @@ import ( func granularToolsForToolset(toolsetID inventory.ToolsetID, featureFlag string) []inventory.ServerTool { var result []inventory.ServerTool for _, tool := range AllTools(translations.NullTranslationHelper) { - if tool.Toolset.ID == toolsetID && tool.FeatureFlagEnable == featureFlag { + if tool.Toolset.ID == toolsetID && tool.FeatureFlagEnable == featureFlag && len(tool.FeatureFlagEnableAll) == 0 { result = append(result, tool) } } @@ -1728,38 +1728,64 @@ func TestGranularAddPullRequestReviewComment(t *testing.T) { } func TestGranularResolveReviewThread(t *testing.T) { - mockedClient := githubv4mock.NewMockedHTTPClient( - githubv4mock.NewMutationMatcher( - struct { - ResolveReviewThread struct { - Thread struct { - ID githubv4.ID - IsResolved githubv4.Boolean - } - } `graphql:"resolveReviewThread(input: $input)"` - }{}, - githubv4.ResolveReviewThreadInput{ - ThreadID: githubv4.ID("PRRT_123"), - }, - nil, - githubv4mock.DataResponse(map[string]any{ - "resolveReviewThread": map[string]any{ - "thread": map[string]any{"id": "PRRT_123", "isResolved": true}, - }, - }), - ), - ) - gqlClient := githubv4.NewClient(mockedClient) - deps := BaseDeps{GQLClient: gqlClient} - serverTool := GranularResolveReviewThread(translations.NullTranslationHelper) - handler := serverTool.Handler(deps) + tests := []struct { + name string + withResolutionReason bool + resolutionReason *string + expectedReason *string + }{ + { + name: "enabled variant forwards resolution reason", + withResolutionReason: true, + resolutionReason: gogithub.Ptr("addressed"), + expectedReason: gogithub.Ptr("addressed"), + }, + {name: "default variant omits resolution reason", resolutionReason: gogithub.Ptr("addressed")}, + {name: "without resolution reason"}, + } - request := createMCPRequest(map[string]any{ - "threadID": "PRRT_123", - }) - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - require.NoError(t, err) - assert.False(t, result.IsError) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + 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_123"), + ResolutionReason: newGQLStringlikePtr[githubv4.String](tc.expectedReason), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "resolveReviewThread": map[string]any{ + "thread": map[string]any{"id": "PRRT_123", "isResolved": true}, + }, + }), + ), + ) + gqlClient := githubv4.NewClient(mockedClient) + deps := BaseDeps{GQLClient: gqlClient} + serverTool := GranularResolveReviewThread(translations.NullTranslationHelper) + if tc.withResolutionReason { + serverTool = GranularResolveReviewThreadWithResolutionReason(translations.NullTranslationHelper) + } + handler := serverTool.Handler(deps) + + args := map[string]any{"threadID": "PRRT_123"} + if tc.resolutionReason != nil { + args["resolutionReason"] = *tc.resolutionReason + } + request := createMCPRequest(args) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) + }) + } } func TestGranularUnresolveReviewThread(t *testing.T) { diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 8801ec2894..b06d53ff5b 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1760,17 +1760,27 @@ func UpdatePullRequestBranch(t translations.TranslationHelperFunc) inventory.Ser } type PullRequestReviewWriteParams struct { - Method string - Owner string - Repo string - PullNumber int32 - Body string - Event string - CommitID *string - ThreadID string + Method string + Owner string + Repo string + PullNumber int32 + Body string + Event string + CommitID *string + ThreadID string + ResolutionReason *string } func PullRequestReviewWrite(t translations.TranslationHelperFunc) inventory.ServerTool { + return pullRequestReviewWrite(t, false) +} + +// PullRequestReviewWriteWithResolutionReason creates the feature-gated review write variant with resolution reasons. +func PullRequestReviewWriteWithResolutionReason(t translations.TranslationHelperFunc) inventory.ServerTool { + return pullRequestReviewWrite(t, true) +} + +func pullRequestReviewWrite(t translations.TranslationHelperFunc, withResolutionReason bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -1814,6 +1824,12 @@ func PullRequestReviewWrite(t translations.TranslationHelperFunc) inventory.Serv }, Required: []string{"method", "owner", "repo", "pullNumber"}, } + if withResolutionReason { + schema.Properties["resolutionReason"] = &jsonschema.Schema{ + Type: "string", + Description: "Optional reason for resolving a Copilot code review thread: addressed, wont-fix, or invalid.", + } + } st := NewTool( ToolsetMetadataPullRequests, @@ -1858,7 +1874,11 @@ Available methods: result, err := DeletePendingPullRequestReview(ctx, client, params) return result, nil, err case "resolve_thread": - result, err := ResolveReviewThread(ctx, client, params.ThreadID, true) + if !withResolutionReason { + result, err := ResolveReviewThread(ctx, client, params.ThreadID, true) + return result, nil, err + } + result, err := ResolveReviewThreadWithReason(ctx, client, params.ThreadID, params.ResolutionReason, true) return result, nil, err case "unresolve_thread": result, err := ResolveReviewThread(ctx, client, params.ThreadID, false) @@ -1867,7 +1887,12 @@ Available methods: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", params.Method)), nil, nil } }) - st.FeatureFlagDisable = []string{FeatureFlagPullRequestsGranular} + if withResolutionReason { + st.FeatureFlagEnable = FeatureFlagThreadResolutionReason + st.FeatureFlagDisable = []string{FeatureFlagPullRequestsGranular} + } else { + st.FeatureFlagDisable = []string{FeatureFlagPullRequestsGranular, FeatureFlagThreadResolutionReason} + } return st } @@ -2094,8 +2119,18 @@ func DeletePendingPullRequestReview(ctx context.Context, client *githubv4.Client return utils.NewToolResultText("pending pull request review successfully deleted"), nil } +type resolveReviewThreadInput struct { + ThreadID githubv4.ID `json:"threadId"` + ResolutionReason *githubv4.String `json:"resolutionReason,omitempty"` +} + // ResolveReviewThread resolves or unresolves a PR review thread using GraphQL mutations. func ResolveReviewThread(ctx context.Context, client *githubv4.Client, threadID string, resolve bool) (*mcp.CallToolResult, error) { + return ResolveReviewThreadWithReason(ctx, client, threadID, nil, resolve) +} + +// ResolveReviewThreadWithReason resolves or unresolves a PR review thread with an optional resolution reason. +func ResolveReviewThreadWithReason(ctx context.Context, client *githubv4.Client, threadID string, resolutionReason *string, resolve bool) (*mcp.CallToolResult, error) { if threadID == "" { return utils.NewToolResultError("threadId is required for resolve_thread and unresolve_thread methods"), nil } @@ -2110,8 +2145,9 @@ func ResolveReviewThread(ctx context.Context, client *githubv4.Client, threadID } `graphql:"resolveReviewThread(input: $input)"` } - input := githubv4.ResolveReviewThreadInput{ - ThreadID: githubv4.ID(threadID), + input := resolveReviewThreadInput{ + ThreadID: githubv4.ID(threadID), + ResolutionReason: newGQLStringlikePtr[githubv4.String](resolutionReason), } if err := client.Mutate(ctx, &mutation, input, nil); err != nil { diff --git a/pkg/github/pullrequests_granular.go b/pkg/github/pullrequests_granular.go index c727beb6e5..23ad7771ef 100644 --- a/pkg/github/pullrequests_granular.go +++ b/pkg/github/pullrequests_granular.go @@ -672,6 +672,28 @@ func GranularAddPullRequestReviewComment(t translations.TranslationHelperFunc) i // GranularResolveReviewThread creates a tool to resolve a review thread. func GranularResolveReviewThread(t translations.TranslationHelperFunc) inventory.ServerTool { + return granularResolveReviewThread(t, false) +} + +// GranularResolveReviewThreadWithResolutionReason creates the feature-gated variant with resolution reasons. +func GranularResolveReviewThreadWithResolutionReason(t translations.TranslationHelperFunc) inventory.ServerTool { + return granularResolveReviewThread(t, true) +} + +func granularResolveReviewThread(t translations.TranslationHelperFunc, withResolutionReason bool) inventory.ServerTool { + properties := map[string]*jsonschema.Schema{ + "threadID": { + Type: "string", + Description: "The node ID of the review thread to resolve (e.g., PRRT_kwDOxxx)", + }, + } + if withResolutionReason { + properties["resolutionReason"] = &jsonschema.Schema{ + Type: "string", + Description: "Optional reason for resolving a Copilot code review thread: addressed, wont-fix, or invalid.", + } + } + st := NewTool( ToolsetMetadataPullRequests, mcp.Tool{ @@ -684,14 +706,9 @@ func GranularResolveReviewThread(t translations.TranslationHelperFunc) inventory OpenWorldHint: jsonschema.Ptr(true), }, InputSchema: &jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{ - "threadID": { - Type: "string", - Description: "The node ID of the review thread to resolve (e.g., PRRT_kwDOxxx)", - }, - }, - Required: []string{"threadID"}, + Type: "object", + Properties: properties, + Required: []string{"threadID"}, }, }, []scopes.Scope{scopes.Repo}, @@ -700,17 +717,36 @@ func GranularResolveReviewThread(t translations.TranslationHelperFunc) inventory if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + var resolutionReasonPtr *string + if withResolutionReason { + resolutionReason, hasResolutionReason, err := OptionalParamOK[string](args, "resolutionReason") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + if hasResolutionReason { + resolutionReasonPtr = &resolutionReason + } + } gqlClient, err := deps.GetGQLClient(ctx) if err != nil { return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil } - result, err := ResolveReviewThread(ctx, gqlClient, threadID, true) + if !withResolutionReason { + result, err := ResolveReviewThread(ctx, gqlClient, threadID, true) + return result, nil, err + } + result, err := ResolveReviewThreadWithReason(ctx, gqlClient, threadID, resolutionReasonPtr, true) return result, nil, err }, ) st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + if withResolutionReason { + st.FeatureFlagEnableAll = []string{FeatureFlagThreadResolutionReason} + } else { + st.FeatureFlagDisable = []string{FeatureFlagThreadResolutionReason} + } return st } diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 03ec851cf4..128b49ed43 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -4546,12 +4546,13 @@ func TestResolveReviewThread(t *testing.T) { t.Parallel() tests := []struct { - name string - requestArgs map[string]any - mockedClient *http.Client - expectToolError bool - expectedToolErrMsg string - expectedResult string + name string + requestArgs map[string]any + mockedClient *http.Client + withResolutionReason bool + expectToolError bool + expectedToolErrMsg string + expectedResult string }{ { name: "successful resolve thread", @@ -4572,7 +4573,81 @@ func TestResolveReviewThread(t *testing.T) { } } `graphql:"resolveReviewThread(input: $input)"` }{}, - githubv4.ResolveReviewThreadInput{ + 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, + requestArgs: map[string]any{ + "method": "resolve_thread", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "threadId": "PRRT_kwDOTest123", + "resolutionReason": "wont-fix", + }, + 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"), + ResolutionReason: newGQLStringlike[githubv4.String]("wont-fix"), + }, + 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: "default variant omits resolution reason", + requestArgs: map[string]any{ + "method": "resolve_thread", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "threadId": "PRRT_kwDOTest123", + "resolutionReason": "wont-fix", + }, + 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, @@ -4692,7 +4767,7 @@ func TestResolveReviewThread(t *testing.T) { } } `graphql:"resolveReviewThread(input: $input)"` }{}, - githubv4.ResolveReviewThreadInput{ + resolveReviewThreadInput{ ThreadID: githubv4.ID("PRRT_invalid"), }, nil, @@ -4711,6 +4786,9 @@ func TestResolveReviewThread(t *testing.T) { // Setup client with mock client := githubv4.NewClient(tc.mockedClient) serverTool := PullRequestReviewWrite(translations.NullTranslationHelper) + if tc.withResolutionReason { + serverTool = PullRequestReviewWriteWithResolutionReason(translations.NullTranslationHelper) + } deps := BaseDeps{ GQLClient: client, } diff --git a/pkg/github/tools.go b/pkg/github/tools.go index f9b51159b5..6b4124752f 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -273,6 +273,7 @@ func AllTools(t translations.TranslationHelperFunc, opts ...ToolOption) []invent CreatePullRequest(t), UpdatePullRequest(t), PullRequestReviewWrite(t), + PullRequestReviewWriteWithResolutionReason(t), AddCommentToPendingReview(t), AddReplyToPullRequestComment(t), @@ -372,6 +373,7 @@ func AllTools(t translations.TranslationHelperFunc, opts ...ToolOption) []invent GranularDeletePendingPullRequestReview(t), GranularAddPullRequestReviewComment(t), GranularResolveReviewThread(t), + GranularResolveReviewThreadWithResolutionReason(t), GranularUnresolveReviewThread(t), GranularAddPullRequestReviewCommentReaction(t), }) diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index fd3579fa6f..96e46f215f 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -43,6 +43,7 @@ func (r *Inventory) checkFeatureFlag(ctx context.Context, flagName string) bool // installed when WithFeatureChecker received a non-nil checker). // // - If FeatureFlagEnable is set, the item is only allowed if the flag is enabled. +// - Every FeatureFlagEnableAll entry must also be enabled. // - If FeatureFlagDisable is non-empty, the item is excluded if any listed flag is enabled. func featureFlagAllowed(ctx context.Context, checker FeatureFlagChecker, enableFlag string, disableFlags []string) bool { // Error semantics match the previous checkFeatureFlag helper: a checker @@ -64,14 +65,22 @@ func featureFlagAllowed(ctx context.Context, checker FeatureFlagChecker, enableF } // createFeatureFlagFilter returns a ToolFilter that gates tools on their -// FeatureFlagEnable / FeatureFlagDisable annotations using the given checker. +// FeatureFlagEnable / FeatureFlagEnableAll / FeatureFlagDisable annotations using the given checker. // Builder.Build() installs this filter exactly once when WithFeatureChecker // has been called with a non-nil checker, so "no feature filtering" is // expressed structurally — by the absence of the filter — rather than by a // runtime nil check inside the filter itself. func createFeatureFlagFilter(checker FeatureFlagChecker) ToolFilter { return func(ctx context.Context, tool *ServerTool) (bool, error) { - return featureFlagAllowed(ctx, checker, tool.FeatureFlagEnable, tool.FeatureFlagDisable), nil + if !featureFlagAllowed(ctx, checker, tool.FeatureFlagEnable, tool.FeatureFlagDisable) { + return false, nil + } + for _, flag := range tool.FeatureFlagEnableAll { + if !featureFlagAllowed(ctx, checker, flag, nil) { + return false, nil + } + } + return true, nil } } diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index 1d8cbcf885..49c591450a 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -71,13 +71,17 @@ type ServerTool struct { // to be available. If set and the flag is not enabled, the tool is omitted. FeatureFlagEnable string + // FeatureFlagEnableAll specifies additional feature flags that must all be enabled + // for this tool to be available. + FeatureFlagEnableAll []string + // FeatureFlagDisable specifies feature flags that, when any is enabled, cause this // tool to be omitted. Used to disable tools when a feature flag is on. FeatureFlagDisable []string // Enabled is an optional function called at build/filter time to determine // if this tool should be available. If nil, the tool is considered enabled - // (subject to FeatureFlagEnable/FeatureFlagDisable checks). + // (subject to feature flag checks). // The context carries request-scoped information for the consumer to use. // Returns (enabled, error). On error, the tool should be treated as disabled. Enabled func(ctx context.Context) (bool, error)