diff --git a/.github/ISSUE_TEMPLATE/insiders-feedback.md b/.github/ISSUE_TEMPLATE/insiders-feedback.md new file mode 100644 index 0000000000..5b1f87f8ce --- /dev/null +++ b/.github/ISSUE_TEMPLATE/insiders-feedback.md @@ -0,0 +1,14 @@ +--- +name: Insiders Feedback +about: Give feedback related to a GitHub MCP Server Insiders feature +title: "Insiders Feedback: " +labels: '' +assignees: '' + +--- + +Version: Insiders + +Feature: + +Feedback: diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index de53eb0aae..4ce7356f37 100644 --- a/.github/workflows/docker-publish.yml +++ b/.github/workflows/docker-publish.yml @@ -46,7 +46,7 @@ jobs: # https://github.com/sigstore/cosign-installer - name: Install cosign if: github.event_name != 'pull_request' - uses: sigstore/cosign-installer@faadad0cce49287aee09b3a48701e75088a2c6ad #v4.0.0 + uses: sigstore/cosign-installer@ba7bc0a3fef59531c69a25acd34668d6d3fe6f22 #v4.1.0 with: cosign-release: "v2.2.4" @@ -70,7 +70,7 @@ jobs: # https://github.com/docker/metadata-action - name: Extract Docker metadata id: meta - uses: docker/metadata-action@c299e40c65443455700f0fdfc63efafe5b349051 # v5.10.0 + uses: docker/metadata-action@030e881283bb7a6894de51c315a6bfe6a94e05cf # v6.0.0 with: images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }} tags: | @@ -93,7 +93,7 @@ jobs: key: ${{ runner.os }}-go-build-cache-${{ hashFiles('**/go.sum') }} - name: Inject go-build-cache - uses: reproducible-containers/buildkit-cache-dance@6f699a72a59e4252f05a7435430009b77e25fe06 # v3.3.1 + uses: reproducible-containers/buildkit-cache-dance@1b8ab18fbda5ad3646e3fcc9ed9dd41ce2f297b4 # v3.3.2 with: cache-map: | { @@ -106,7 +106,7 @@ jobs: # https://github.com/docker/build-push-action - name: Build and push Docker image id: build-and-push - uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0 + uses: docker/build-push-action@d08e5c354a6adb9ed34480a06d141179aa583294 # v7.0.0 with: context: . push: ${{ github.event_name != 'pull_request' }} diff --git a/Dockerfile b/Dockerfile index 90c8b40079..b13ae62d17 100644 --- a/Dockerfile +++ b/Dockerfile @@ -7,7 +7,7 @@ COPY ui/ ./ui/ RUN mkdir -p ./pkg/github/ui_dist && \ cd ui && npm run build -FROM golang:1.25.7-alpine@sha256:f6751d823c26342f9506c03797d2527668d095b0a15f1862cddb4d927a7a4ced AS build +FROM golang:1.25.8-alpine@sha256:8e02eb337d9e0ea459e041f1ee5eece41cbb61f1d83e7d883a3e2fb4862063fa AS build ARG VERSION="dev" # Set the working directory diff --git a/README.md b/README.md index 1b926b1329..419f892979 100644 --- a/README.md +++ b/README.md @@ -1119,6 +1119,7 @@ The following sets of tools are available: - `owner`: Repository owner (string, required) - `pullNumber`: Pull request number (number, required) - `repo`: Repository name (string, required) + - `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 - **Required OAuth Scopes**: `repo` @@ -1241,9 +1242,12 @@ The following sets of tools are available: - `author`: Author username or email address to filter commits by (string, optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) + - `path`: Only commits containing this file path will be returned (string, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: Repository name (string, required) - `sha`: 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. (string, optional) + - `since`: Only commits after this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD) (string, optional) + - `until`: Only commits before this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD) (string, optional) - **list_releases** - List releases - **Required OAuth Scopes**: `repo` @@ -1536,6 +1540,34 @@ set the following environment variable: export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description" ``` +### Overriding Server Name and Title + +The same override mechanism can be used to customize the MCP server's `name` and +`title` fields in the initialization response. This is useful when running +multiple GitHub MCP Server instances (e.g., one for github.com and one for +GitHub Enterprise Server) so that agents can distinguish between them. + +| Key | Environment Variable | Default | +|-----|---------------------|---------| +| `SERVER_NAME` | `GITHUB_MCP_SERVER_NAME` | `github-mcp-server` | +| `SERVER_TITLE` | `GITHUB_MCP_SERVER_TITLE` | `GitHub MCP Server` | + +For example, to configure a server instance for GitHub Enterprise Server: + +```json +{ + "SERVER_NAME": "ghes-mcp-server", + "SERVER_TITLE": "GHES MCP Server" +} +``` + +Or using environment variables: + +```sh +export GITHUB_MCP_SERVER_NAME="ghes-mcp-server" +export GITHUB_MCP_SERVER_TITLE="GHES MCP Server" +``` + ## Library Usage The exported Go API of this module should currently be considered unstable, and subject to breaking changes. In the future, we may offer stability; please file an issue if there is a use case where this would be valuable. diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index 05c2c6e0be..8f2ae58525 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -105,6 +105,28 @@ var ( Short: "Start HTTP server", Long: `Start an HTTP server that listens for MCP requests over HTTP.`, RunE: func(_ *cobra.Command, _ []string) error { + // Parse toolsets (same approach as stdio — see comment there) + var enabledToolsets []string + if viper.IsSet("toolsets") { + if err := viper.UnmarshalKey("toolsets", &enabledToolsets); err != nil { + return fmt.Errorf("failed to unmarshal toolsets: %w", err) + } + } + + var enabledTools []string + if viper.IsSet("tools") { + if err := viper.UnmarshalKey("tools", &enabledTools); err != nil { + return fmt.Errorf("failed to unmarshal tools: %w", err) + } + } + + var excludeTools []string + if viper.IsSet("exclude_tools") { + if err := viper.UnmarshalKey("exclude_tools", &excludeTools); err != nil { + return fmt.Errorf("failed to unmarshal exclude-tools: %w", err) + } + } + ttl := viper.GetDuration("repo-access-cache-ttl") httpConfig := ghhttp.ServerConfig{ Version: version, @@ -119,6 +141,12 @@ var ( LockdownMode: viper.GetBool("lockdown-mode"), RepoAccessCacheTTL: &ttl, ScopeChallenge: viper.GetBool("scope-challenge"), + ReadOnly: viper.GetBool("read-only"), + EnabledToolsets: enabledToolsets, + EnabledTools: enabledTools, + DynamicToolsets: viper.GetBool("dynamic_toolsets"), + ExcludeTools: excludeTools, + InsidersMode: viper.GetBool("insiders"), } return ghhttp.RunHTTPServer(httpConfig) diff --git a/docs/server-configuration.md b/docs/server-configuration.md index a334eb1a20..87d48e01e3 100644 --- a/docs/server-configuration.md +++ b/docs/server-configuration.md @@ -15,6 +15,7 @@ We currently support the following ways in which the GitHub MCP Server can be co | Lockdown Mode | `X-MCP-Lockdown` header | `--lockdown-mode` flag or `GITHUB_LOCKDOWN_MODE` env var | | Insiders Mode | `X-MCP-Insiders` header or `/insiders` URL | `--insiders` flag or `GITHUB_INSIDERS` env var | | Scope Filtering | Always enabled | Always enabled | +| Server Name/Title | Not available | `GITHUB_MCP_SERVER_NAME` / `GITHUB_MCP_SERVER_TITLE` env vars or `github-mcp-server-config.json` | > **Default behavior:** If you don't specify any configuration, the server uses the **default toolsets**: `context`, `issues`, `pull_requests`, `repos`, `users`. diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 5c4e7f6f1b..5dfaf596c6 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -18,6 +18,8 @@ import ( "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/lockdown" mcplog "github.com/github/github-mcp-server/pkg/log" + "github.com/github/github-mcp-server/pkg/observability" + "github.com/github/github-mcp-server/pkg/observability/metrics" "github.com/github/github-mcp-server/pkg/raw" "github.com/github/github-mcp-server/pkg/scopes" "github.com/github/github-mcp-server/pkg/translations" @@ -116,6 +118,10 @@ func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Se featureChecker := createFeatureChecker(cfg.EnabledFeatures) // Create dependencies for tool handlers + obs, err := observability.NewExporters(cfg.Logger, metrics.NewNoopMetrics()) + if err != nil { + return nil, fmt.Errorf("failed to create observability exporters: %w", err) + } deps := github.NewBaseDeps( clients.rest, clients.gql, @@ -128,6 +134,7 @@ func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Se }, cfg.ContentWindowSize, featureChecker, + obs, ) // Build and register the tool/resource/prompt inventory inventoryBuilder := github.NewInventory(cfg.Translator). diff --git a/pkg/github/__toolsnaps__/add_pull_request_review_comment.snap b/pkg/github/__toolsnaps__/add_pull_request_review_comment.snap new file mode 100644 index 0000000000..1e27c5645e --- /dev/null +++ b/pkg/github/__toolsnaps__/add_pull_request_review_comment.snap @@ -0,0 +1,75 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Add Pull Request Review Comment" + }, + "description": "Add a review comment to the current user's pending pull request review.", + "inputSchema": { + "properties": { + "body": { + "description": "The comment body", + "type": "string" + }, + "line": { + "description": "The line number in the diff to comment on (optional)", + "type": "number" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "path": { + "description": "The relative path of the file to comment on", + "type": "string" + }, + "pullNumber": { + "description": "The pull request number", + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "side": { + "description": "The side of the diff to comment on (optional)", + "enum": [ + "LEFT", + "RIGHT" + ], + "type": "string" + }, + "startLine": { + "description": "The start line of a multi-line comment (optional)", + "type": "number" + }, + "startSide": { + "description": "The start side of a multi-line comment (optional)", + "enum": [ + "LEFT", + "RIGHT" + ], + "type": "string" + }, + "subjectType": { + "description": "The subject type of the comment", + "enum": [ + "FILE", + "LINE" + ], + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "pullNumber", + "path", + "body", + "subjectType" + ], + "type": "object" + }, + "name": "add_pull_request_review_comment" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/add_sub_issue.snap b/pkg/github/__toolsnaps__/add_sub_issue.snap new file mode 100644 index 0000000000..ef9df400c6 --- /dev/null +++ b/pkg/github/__toolsnaps__/add_sub_issue.snap @@ -0,0 +1,41 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Add Sub-Issue" + }, + "description": "Add a sub-issue to a parent issue.", + "inputSchema": { + "properties": { + "issue_number": { + "description": "The parent issue number", + "minimum": 1, + "type": "number" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "replace_parent": { + "description": "If true, reparent the sub-issue if it already has a parent", + "type": "boolean" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "sub_issue_id": { + "description": "The ID of the sub-issue to add. ID is not the same as issue number", + "type": "number" + } + }, + "required": [ + "owner", + "repo", + "issue_number", + "sub_issue_id" + ], + "type": "object" + }, + "name": "add_sub_issue" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/create_issue.snap b/pkg/github/__toolsnaps__/create_issue.snap index d11c41c0ed..51923c47cc 100644 --- a/pkg/github/__toolsnaps__/create_issue.snap +++ b/pkg/github/__toolsnaps__/create_issue.snap @@ -1,35 +1,18 @@ { "annotations": { - "title": "Open new issue", - "readOnlyHint": false + "destructiveHint": false, + "openWorldHint": true, + "title": "Create Issue" }, - "description": "Create a new issue in a GitHub repository.", + "description": "Create a new issue in a GitHub repository with a title and optional body.", "inputSchema": { "properties": { - "assignees": { - "description": "Usernames to assign to this issue", - "items": { - "type": "string" - }, - "type": "array" - }, "body": { - "description": "Issue body content", + "description": "Issue body content (optional)", "type": "string" }, - "labels": { - "description": "Labels to apply to this issue", - "items": { - "type": "string" - }, - "type": "array" - }, - "milestone": { - "description": "Milestone number", - "type": "number" - }, "owner": { - "description": "Repository owner", + "description": "Repository owner (username or organization)", "type": "string" }, "repo": { @@ -39,10 +22,6 @@ "title": { "description": "Issue title", "type": "string" - }, - "type": { - "description": "Type of this issue", - "type": "string" } }, "required": [ diff --git a/pkg/github/__toolsnaps__/create_pull_request_review.snap b/pkg/github/__toolsnaps__/create_pull_request_review.snap new file mode 100644 index 0000000000..1986b2cfff --- /dev/null +++ b/pkg/github/__toolsnaps__/create_pull_request_review.snap @@ -0,0 +1,49 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Create Pull Request Review" + }, + "description": "Create a review on a pull request. If event is provided, the review is submitted immediately; otherwise a pending review is created.", + "inputSchema": { + "properties": { + "body": { + "description": "The review body text (optional)", + "type": "string" + }, + "commitID": { + "description": "The SHA of the commit to review (optional, defaults to latest)", + "type": "string" + }, + "event": { + "description": "The review action to perform. If omitted, creates a pending review.", + "enum": [ + "APPROVE", + "REQUEST_CHANGES", + "COMMENT" + ], + "type": "string" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "pullNumber": { + "description": "The pull request number", + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "pullNumber" + ], + "type": "object" + }, + "name": "create_pull_request_review" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/delete_pending_pull_request_review.snap b/pkg/github/__toolsnaps__/delete_pending_pull_request_review.snap new file mode 100644 index 0000000000..b457e415a8 --- /dev/null +++ b/pkg/github/__toolsnaps__/delete_pending_pull_request_review.snap @@ -0,0 +1,32 @@ +{ + "annotations": { + "destructiveHint": true, + "openWorldHint": true, + "title": "Delete Pending Pull Request Review" + }, + "description": "Delete a pending pull request review.", + "inputSchema": { + "properties": { + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "pullNumber": { + "description": "The pull request number", + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "pullNumber" + ], + "type": "object" + }, + "name": "delete_pending_pull_request_review" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_commits.snap b/pkg/github/__toolsnaps__/list_commits.snap index 38b63736fa..1a773f217e 100644 --- a/pkg/github/__toolsnaps__/list_commits.snap +++ b/pkg/github/__toolsnaps__/list_commits.snap @@ -19,6 +19,10 @@ "minimum": 1, "type": "number" }, + "path": { + "description": "Only commits containing this file path will be returned", + "type": "string" + }, "perPage": { "description": "Results per page for pagination (min 1, max 100)", "maximum": 100, @@ -32,6 +36,14 @@ "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.", "type": "string" + }, + "since": { + "description": "Only commits after this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD)", + "type": "string" + }, + "until": { + "description": "Only commits before this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD)", + "type": "string" } }, "required": [ diff --git a/pkg/github/__toolsnaps__/pull_request_review_write.snap b/pkg/github/__toolsnaps__/pull_request_review_write.snap index 7b533f4723..7e314005f5 100644 --- a/pkg/github/__toolsnaps__/pull_request_review_write.snap +++ b/pkg/github/__toolsnaps__/pull_request_review_write.snap @@ -2,7 +2,7 @@ "annotations": { "title": "Write operations (create, submit, delete) on pull request reviews." }, - "description": "Create and/or submit, delete review of a pull request.\n\nAvailable methods:\n- create: Create a new review of a pull request. If \"event\" parameter is provided, the review is submitted. If \"event\" is omitted, a pending review is created.\n- submit_pending: Submit an existing pending review of a pull request. This requires that a pending review exists for the current user on the specified pull request. The \"body\" and \"event\" parameters are used when submitting the review.\n- delete_pending: Delete an existing pending review of a pull request. This requires that a pending review exists for the current user on the specified pull request.\n", + "description": "Create and/or submit, delete review of a pull request.\n\nAvailable methods:\n- create: Create a new review of a pull request. If \"event\" parameter is provided, the review is submitted. If \"event\" is omitted, a pending review is created.\n- submit_pending: Submit an existing pending review of a pull request. This requires that a pending review exists for the current user on the specified pull request. The \"body\" and \"event\" parameters are used when submitting the review.\n- delete_pending: Delete an existing pending review of a pull request. This requires that a pending review exists for the current user on the specified pull request.\n- resolve_thread: Resolve a review thread. Requires only \"threadId\" parameter with the thread's node ID (e.g., PRRT_kwDOxxx). The owner, repo, and pullNumber parameters are not used for this method. Resolving an already-resolved thread is a no-op.\n- unresolve_thread: Unresolve a previously resolved review thread. Requires only \"threadId\" parameter. The owner, repo, and pullNumber parameters are not used for this method. Unresolving an already-unresolved thread is a no-op.\n", "inputSchema": { "properties": { "body": { @@ -27,7 +27,9 @@ "enum": [ "create", "submit_pending", - "delete_pending" + "delete_pending", + "resolve_thread", + "unresolve_thread" ], "type": "string" }, @@ -42,6 +44,10 @@ "repo": { "description": "Repository name", "type": "string" + }, + "threadId": { + "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.", + "type": "string" } }, "required": [ diff --git a/pkg/github/__toolsnaps__/push_files.snap b/pkg/github/__toolsnaps__/push_files.snap index c36c236f98..df6c4d1e79 100644 --- a/pkg/github/__toolsnaps__/push_files.snap +++ b/pkg/github/__toolsnaps__/push_files.snap @@ -12,6 +12,7 @@ "files": { "description": "Array of file objects to push, each object with path (string) and content (string)", "items": { + "additionalProperties": false, "properties": { "content": { "description": "file content", diff --git a/pkg/github/__toolsnaps__/remove_sub_issue.snap b/pkg/github/__toolsnaps__/remove_sub_issue.snap new file mode 100644 index 0000000000..31fdcbb3e2 --- /dev/null +++ b/pkg/github/__toolsnaps__/remove_sub_issue.snap @@ -0,0 +1,37 @@ +{ + "annotations": { + "destructiveHint": true, + "openWorldHint": true, + "title": "Remove Sub-Issue" + }, + "description": "Remove a sub-issue from a parent issue.", + "inputSchema": { + "properties": { + "issue_number": { + "description": "The parent issue number", + "minimum": 1, + "type": "number" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "sub_issue_id": { + "description": "The ID of the sub-issue to remove. ID is not the same as issue number", + "type": "number" + } + }, + "required": [ + "owner", + "repo", + "issue_number", + "sub_issue_id" + ], + "type": "object" + }, + "name": "remove_sub_issue" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/reprioritize_sub_issue.snap b/pkg/github/__toolsnaps__/reprioritize_sub_issue.snap new file mode 100644 index 0000000000..d4e1ea4be4 --- /dev/null +++ b/pkg/github/__toolsnaps__/reprioritize_sub_issue.snap @@ -0,0 +1,45 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Reprioritize Sub-Issue" + }, + "description": "Reprioritize (reorder) a sub-issue relative to other sub-issues.", + "inputSchema": { + "properties": { + "after_id": { + "description": "The ID of the sub-issue to place this after (either after_id OR before_id should be specified)", + "type": "number" + }, + "before_id": { + "description": "The ID of the sub-issue to place this before (either after_id OR before_id should be specified)", + "type": "number" + }, + "issue_number": { + "description": "The parent issue number", + "minimum": 1, + "type": "number" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "sub_issue_id": { + "description": "The ID of the sub-issue to reorder. ID is not the same as issue number", + "type": "number" + } + }, + "required": [ + "owner", + "repo", + "issue_number", + "sub_issue_id" + ], + "type": "object" + }, + "name": "reprioritize_sub_issue" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/request_pull_request_reviewers.snap b/pkg/github/__toolsnaps__/request_pull_request_reviewers.snap new file mode 100644 index 0000000000..67b7014474 --- /dev/null +++ b/pkg/github/__toolsnaps__/request_pull_request_reviewers.snap @@ -0,0 +1,40 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Request Pull Request Reviewers" + }, + "description": "Request reviewers for a pull request.", + "inputSchema": { + "properties": { + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "pullNumber": { + "description": "The pull request number", + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "reviewers": { + "description": "GitHub usernames to request reviews from", + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [ + "owner", + "repo", + "pullNumber", + "reviewers" + ], + "type": "object" + }, + "name": "request_pull_request_reviewers" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/resolve_review_thread.snap b/pkg/github/__toolsnaps__/resolve_review_thread.snap new file mode 100644 index 0000000000..afcd407841 --- /dev/null +++ b/pkg/github/__toolsnaps__/resolve_review_thread.snap @@ -0,0 +1,21 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Resolve Review Thread" + }, + "description": "Resolve a review thread on a pull request. Resolving an already-resolved thread is a no-op.", + "inputSchema": { + "properties": { + "threadID": { + "description": "The node ID of the review thread to resolve (e.g., PRRT_kwDOxxx)", + "type": "string" + } + }, + "required": [ + "threadID" + ], + "type": "object" + }, + "name": "resolve_review_thread" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/submit_pending_pull_request_review.snap b/pkg/github/__toolsnaps__/submit_pending_pull_request_review.snap new file mode 100644 index 0000000000..81223e2a9d --- /dev/null +++ b/pkg/github/__toolsnaps__/submit_pending_pull_request_review.snap @@ -0,0 +1,46 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Submit Pending Pull Request Review" + }, + "description": "Submit a pending pull request review.", + "inputSchema": { + "properties": { + "body": { + "description": "The review body text (optional)", + "type": "string" + }, + "event": { + "description": "The review action to perform", + "enum": [ + "APPROVE", + "REQUEST_CHANGES", + "COMMENT" + ], + "type": "string" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "pullNumber": { + "description": "The pull request number", + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "pullNumber", + "event" + ], + "type": "object" + }, + "name": "submit_pending_pull_request_review" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/unresolve_review_thread.snap b/pkg/github/__toolsnaps__/unresolve_review_thread.snap new file mode 100644 index 0000000000..d58ba31a6f --- /dev/null +++ b/pkg/github/__toolsnaps__/unresolve_review_thread.snap @@ -0,0 +1,21 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Unresolve Review Thread" + }, + "description": "Unresolve a previously resolved review thread on a pull request. Unresolving an already-unresolved thread is a no-op.", + "inputSchema": { + "properties": { + "threadID": { + "description": "The node ID of the review thread to unresolve (e.g., PRRT_kwDOxxx)", + "type": "string" + } + }, + "required": [ + "threadID" + ], + "type": "object" + }, + "name": "unresolve_review_thread" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_issue_assignees.snap b/pkg/github/__toolsnaps__/update_issue_assignees.snap new file mode 100644 index 0000000000..9c7261c9aa --- /dev/null +++ b/pkg/github/__toolsnaps__/update_issue_assignees.snap @@ -0,0 +1,40 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Update Issue Assignees" + }, + "description": "Update the assignees of an existing issue. This replaces the current assignees with the provided list.", + "inputSchema": { + "properties": { + "assignees": { + "description": "GitHub usernames to assign to this issue", + "items": { + "type": "string" + }, + "type": "array" + }, + "issue_number": { + "description": "The issue number to update", + "minimum": 1, + "type": "number" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "issue_number", + "assignees" + ], + "type": "object" + }, + "name": "update_issue_assignees" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_issue_body.snap b/pkg/github/__toolsnaps__/update_issue_body.snap new file mode 100644 index 0000000000..c54d69172a --- /dev/null +++ b/pkg/github/__toolsnaps__/update_issue_body.snap @@ -0,0 +1,37 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Update Issue Body" + }, + "description": "Update the body content of an existing issue.", + "inputSchema": { + "properties": { + "body": { + "description": "The new body content for the issue", + "type": "string" + }, + "issue_number": { + "description": "The issue number to update", + "minimum": 1, + "type": "number" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "issue_number", + "body" + ], + "type": "object" + }, + "name": "update_issue_body" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_issue_labels.snap b/pkg/github/__toolsnaps__/update_issue_labels.snap new file mode 100644 index 0000000000..3acf98d93f --- /dev/null +++ b/pkg/github/__toolsnaps__/update_issue_labels.snap @@ -0,0 +1,40 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Update Issue Labels" + }, + "description": "Update the labels of an existing issue. This replaces the current labels with the provided list.", + "inputSchema": { + "properties": { + "issue_number": { + "description": "The issue number to update", + "minimum": 1, + "type": "number" + }, + "labels": { + "description": "Labels to apply to this issue", + "items": { + "type": "string" + }, + "type": "array" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "issue_number", + "labels" + ], + "type": "object" + }, + "name": "update_issue_labels" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_issue_milestone.snap b/pkg/github/__toolsnaps__/update_issue_milestone.snap new file mode 100644 index 0000000000..9188779f0a --- /dev/null +++ b/pkg/github/__toolsnaps__/update_issue_milestone.snap @@ -0,0 +1,38 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Update Issue Milestone" + }, + "description": "Update the milestone of an existing issue.", + "inputSchema": { + "properties": { + "issue_number": { + "description": "The issue number to update", + "minimum": 1, + "type": "number" + }, + "milestone": { + "description": "The milestone number to set on the issue", + "minimum": 1, + "type": "integer" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "issue_number", + "milestone" + ], + "type": "object" + }, + "name": "update_issue_milestone" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_issue_state.snap b/pkg/github/__toolsnaps__/update_issue_state.snap new file mode 100644 index 0000000000..b14d737b7d --- /dev/null +++ b/pkg/github/__toolsnaps__/update_issue_state.snap @@ -0,0 +1,50 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Update Issue State" + }, + "description": "Update the state of an existing issue (open or closed), with an optional state reason.", + "inputSchema": { + "properties": { + "issue_number": { + "description": "The issue number to update", + "minimum": 1, + "type": "number" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "state": { + "description": "The new state for the issue", + "enum": [ + "open", + "closed" + ], + "type": "string" + }, + "state_reason": { + "description": "The reason for the state change (only for closed state)", + "enum": [ + "completed", + "not_planned", + "duplicate" + ], + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "issue_number", + "state" + ], + "type": "object" + }, + "name": "update_issue_state" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_issue_title.snap b/pkg/github/__toolsnaps__/update_issue_title.snap new file mode 100644 index 0000000000..825fab0655 --- /dev/null +++ b/pkg/github/__toolsnaps__/update_issue_title.snap @@ -0,0 +1,37 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Update Issue Title" + }, + "description": "Update the title of an existing issue.", + "inputSchema": { + "properties": { + "issue_number": { + "description": "The issue number to update", + "minimum": 1, + "type": "number" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "title": { + "description": "The new title for the issue", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "issue_number", + "title" + ], + "type": "object" + }, + "name": "update_issue_title" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_issue_type.snap b/pkg/github/__toolsnaps__/update_issue_type.snap new file mode 100644 index 0000000000..6354a42e16 --- /dev/null +++ b/pkg/github/__toolsnaps__/update_issue_type.snap @@ -0,0 +1,37 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Update Issue Type" + }, + "description": "Update the type of an existing issue (e.g. 'bug', 'feature').", + "inputSchema": { + "properties": { + "issue_number": { + "description": "The issue number to update", + "minimum": 1, + "type": "number" + }, + "issue_type": { + "description": "The issue type to set", + "type": "string" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "issue_number", + "issue_type" + ], + "type": "object" + }, + "name": "update_issue_type" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_pull_request_body.snap b/pkg/github/__toolsnaps__/update_pull_request_body.snap new file mode 100644 index 0000000000..1e6040bd4d --- /dev/null +++ b/pkg/github/__toolsnaps__/update_pull_request_body.snap @@ -0,0 +1,37 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Update Pull Request Body" + }, + "description": "Update the body description of an existing pull request.", + "inputSchema": { + "properties": { + "body": { + "description": "The new body content for the pull request", + "type": "string" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "pullNumber": { + "description": "The pull request number", + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "pullNumber", + "body" + ], + "type": "object" + }, + "name": "update_pull_request_body" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_pull_request_draft_state.snap b/pkg/github/__toolsnaps__/update_pull_request_draft_state.snap new file mode 100644 index 0000000000..2a397951ab --- /dev/null +++ b/pkg/github/__toolsnaps__/update_pull_request_draft_state.snap @@ -0,0 +1,37 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Update Pull Request Draft State" + }, + "description": "Mark a pull request as draft or ready for review.", + "inputSchema": { + "properties": { + "draft": { + "description": "Set to true to convert to draft, false to mark as ready for review", + "type": "boolean" + }, + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "pullNumber": { + "description": "The pull request number", + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "pullNumber", + "draft" + ], + "type": "object" + }, + "name": "update_pull_request_draft_state" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_pull_request_state.snap b/pkg/github/__toolsnaps__/update_pull_request_state.snap new file mode 100644 index 0000000000..9cbdb81124 --- /dev/null +++ b/pkg/github/__toolsnaps__/update_pull_request_state.snap @@ -0,0 +1,41 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Update Pull Request State" + }, + "description": "Update the state of an existing pull request (open or closed).", + "inputSchema": { + "properties": { + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "pullNumber": { + "description": "The pull request number", + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "state": { + "description": "The new state for the pull request", + "enum": [ + "open", + "closed" + ], + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "pullNumber", + "state" + ], + "type": "object" + }, + "name": "update_pull_request_state" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_pull_request_title.snap b/pkg/github/__toolsnaps__/update_pull_request_title.snap new file mode 100644 index 0000000000..e6398ed40a --- /dev/null +++ b/pkg/github/__toolsnaps__/update_pull_request_title.snap @@ -0,0 +1,37 @@ +{ + "annotations": { + "destructiveHint": false, + "openWorldHint": true, + "title": "Update Pull Request Title" + }, + "description": "Update the title of an existing pull request.", + "inputSchema": { + "properties": { + "owner": { + "description": "Repository owner (username or organization)", + "type": "string" + }, + "pullNumber": { + "description": "The pull request number", + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "title": { + "description": "The new title for the pull request", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "pullNumber", + "title" + ], + "type": "object" + }, + "name": "update_pull_request_title" +} \ No newline at end of file diff --git a/pkg/github/context_tools_test.go b/pkg/github/context_tools_test.go index 3925019853..39f2058bec 100644 --- a/pkg/github/context_tools_test.go +++ b/pkg/github/context_tools_test.go @@ -96,9 +96,10 @@ func Test_GetMe(t *testing.T) { t.Run(tc.name, func(t *testing.T) { var deps ToolDependencies if tc.clientErr != "" { - deps = stubDeps{clientFn: stubClientFnErr(tc.clientErr)} + deps = stubDeps{clientFn: stubClientFnErr(tc.clientErr), obsv: stubExporters()} } else { - deps = BaseDeps{Client: github.NewClient(tc.mockedClient)} + obs := stubExporters() + deps = BaseDeps{Client: github.NewClient(tc.mockedClient), Obsv: obs} } handler := serverTool.Handler(deps) @@ -304,7 +305,7 @@ func Test_GetTeams(t *testing.T) { { name: "getting client fails", makeDeps: func() ToolDependencies { - return stubDeps{clientFn: stubClientFnErr("expected test error")} + return stubDeps{clientFn: stubClientFnErr("expected test error"), obsv: stubExporters()} }, requestArgs: map[string]any{}, expectToolError: true, @@ -315,6 +316,7 @@ func Test_GetTeams(t *testing.T) { makeDeps: func() ToolDependencies { return BaseDeps{ Client: github.NewClient(httpClientUserFails()), + Obsv: stubExporters(), } }, requestArgs: map[string]any{}, @@ -327,6 +329,7 @@ func Test_GetTeams(t *testing.T) { return stubDeps{ clientFn: stubClientFnFromHTTP(httpClientWithUser()), gqlClientFn: stubGQLClientFnErr("GraphQL client error"), + obsv: stubExporters(), } }, requestArgs: map[string]any{}, @@ -469,7 +472,7 @@ func Test_GetTeamMembers(t *testing.T) { }, { name: "getting GraphQL client fails", - deps: stubDeps{gqlClientFn: stubGQLClientFnErr("GraphQL client error")}, + deps: stubDeps{gqlClientFn: stubGQLClientFnErr("GraphQL client error"), obsv: stubExporters()}, requestArgs: map[string]any{ "org": "testorg", "team_slug": "testteam", diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index f966c531e5..57c6133a8a 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "log/slog" "net/http" "os" @@ -11,6 +12,8 @@ import ( "github.com/github/github-mcp-server/pkg/http/transport" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/lockdown" + "github.com/github/github-mcp-server/pkg/observability" + "github.com/github/github-mcp-server/pkg/observability/metrics" "github.com/github/github-mcp-server/pkg/raw" "github.com/github/github-mcp-server/pkg/scopes" "github.com/github/github-mcp-server/pkg/translations" @@ -94,6 +97,14 @@ type ToolDependencies interface { // IsFeatureEnabled checks if a feature flag is enabled. IsFeatureEnabled(ctx context.Context, flagName string) bool + + // Logger returns the structured logger, optionally enriched with + // request-scoped data from ctx. Integrators provide their own slog.Handler + // to control where logs are sent. + Logger(ctx context.Context) *slog.Logger + + // Metrics returns the metrics client + Metrics(ctx context.Context) metrics.Metrics } // BaseDeps is the standard implementation of ToolDependencies for the local server. @@ -113,6 +124,9 @@ type BaseDeps struct { // Feature flag checker for runtime checks featureChecker inventory.FeatureFlagChecker + + // Observability exporters (includes logger) + Obsv observability.Exporters } // Compile-time assertion to verify that BaseDeps implements the ToolDependencies interface. @@ -128,6 +142,7 @@ func NewBaseDeps( flags FeatureFlags, contentWindowSize int, featureChecker inventory.FeatureFlagChecker, + obsv observability.Exporters, ) *BaseDeps { return &BaseDeps{ Client: client, @@ -138,6 +153,7 @@ func NewBaseDeps( Flags: flags, ContentWindowSize: contentWindowSize, featureChecker: featureChecker, + Obsv: obsv, } } @@ -170,6 +186,16 @@ func (d BaseDeps) GetFlags(_ context.Context) FeatureFlags { return d.Flags } // GetContentWindowSize implements ToolDependencies. func (d BaseDeps) GetContentWindowSize() int { return d.ContentWindowSize } +// Logger implements ToolDependencies. +func (d BaseDeps) Logger(_ context.Context) *slog.Logger { + return d.Obsv.Logger() +} + +// Metrics implements ToolDependencies. +func (d BaseDeps) Metrics(ctx context.Context) metrics.Metrics { + return d.Obsv.Metrics(ctx) +} + // IsFeatureEnabled checks if a feature flag is enabled. // Returns false if the feature checker is nil, flag name is empty, or an error occurs. // This allows tools to conditionally change behavior based on feature flags. @@ -247,6 +273,9 @@ type RequestDeps struct { // Feature flag checker for runtime checks featureChecker inventory.FeatureFlagChecker + + // Observability exporters (includes logger) + obsv observability.Exporters } // NewRequestDeps creates a RequestDeps with the provided clients and configuration. @@ -258,6 +287,7 @@ func NewRequestDeps( t translations.TranslationHelperFunc, contentWindowSize int, featureChecker inventory.FeatureFlagChecker, + obsv observability.Exporters, ) *RequestDeps { return &RequestDeps{ apiHosts: apiHosts, @@ -267,6 +297,7 @@ func NewRequestDeps( T: t, ContentWindowSize: contentWindowSize, featureChecker: featureChecker, + obsv: obsv, } } @@ -374,6 +405,16 @@ func (d *RequestDeps) GetFlags(ctx context.Context) FeatureFlags { // GetContentWindowSize implements ToolDependencies. func (d *RequestDeps) GetContentWindowSize() int { return d.ContentWindowSize } +// Logger implements ToolDependencies. +func (d *RequestDeps) Logger(_ context.Context) *slog.Logger { + return d.obsv.Logger() +} + +// Metrics implements ToolDependencies. +func (d *RequestDeps) Metrics(ctx context.Context) metrics.Metrics { + return d.obsv.Metrics(ctx) +} + // IsFeatureEnabled checks if a feature flag is enabled. func (d *RequestDeps) IsFeatureEnabled(ctx context.Context, flagName string) bool { if d.featureChecker == nil || flagName == "" { diff --git a/pkg/github/dependencies_test.go b/pkg/github/dependencies_test.go index d13160d4c6..1d747cae47 100644 --- a/pkg/github/dependencies_test.go +++ b/pkg/github/dependencies_test.go @@ -3,13 +3,21 @@ package github_test import ( "context" "errors" + "log/slog" "testing" "github.com/github/github-mcp-server/pkg/github" + "github.com/github/github-mcp-server/pkg/observability" + "github.com/github/github-mcp-server/pkg/observability/metrics" "github.com/github/github-mcp-server/pkg/translations" "github.com/stretchr/testify/assert" ) +func testExporters() observability.Exporters { + obs, _ := observability.NewExporters(slog.New(slog.DiscardHandler), metrics.NewNoopMetrics()) + return obs +} + func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) { t.Parallel() @@ -28,6 +36,7 @@ func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) { github.FeatureFlags{}, 0, // contentWindowSize checker, // featureChecker + testExporters(), ) // Test enabled flag @@ -52,6 +61,7 @@ func TestIsFeatureEnabled_WithoutChecker(t *testing.T) { github.FeatureFlags{}, 0, // contentWindowSize nil, // featureChecker (nil) + testExporters(), ) // Should return false when checker is nil @@ -76,6 +86,7 @@ func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) { github.FeatureFlags{}, 0, // contentWindowSize checker, // featureChecker + testExporters(), ) // Should return false for empty flag name @@ -100,6 +111,7 @@ func TestIsFeatureEnabled_CheckerError(t *testing.T) { github.FeatureFlags{}, 0, // contentWindowSize checker, // featureChecker + testExporters(), ) // Should return false and log error (not crash) diff --git a/pkg/github/dynamic_tools_test.go b/pkg/github/dynamic_tools_test.go index 3e63c5d7b4..ec559099ef 100644 --- a/pkg/github/dynamic_tools_test.go +++ b/pkg/github/dynamic_tools_test.go @@ -136,7 +136,7 @@ func TestDynamicTools_EnableToolset(t *testing.T) { deps := DynamicToolDependencies{ Server: server, Inventory: reg, - ToolDeps: NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0, nil), + ToolDeps: NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0, nil, stubExporters()), T: translations.NullTranslationHelper, } diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go index 2f0a435c95..0f08c4f12f 100644 --- a/pkg/github/feature_flags_test.go +++ b/pkg/github/feature_flags_test.go @@ -104,6 +104,7 @@ func TestHelloWorld_ConditionalBehavior_Featureflag(t *testing.T) { FeatureFlags{}, 0, checker, + stubExporters(), ) // Get the tool and its handler @@ -166,6 +167,7 @@ func TestHelloWorld_ConditionalBehavior_Config(t *testing.T) { FeatureFlags{InsidersMode: tt.insidersMode}, 0, nil, + stubExporters(), ) // Get the tool and its handler diff --git a/pkg/github/granular_tools_test.go b/pkg/github/granular_tools_test.go new file mode 100644 index 0000000000..d50f6c5529 --- /dev/null +++ b/pkg/github/granular_tools_test.go @@ -0,0 +1,776 @@ +package github + +import ( + "context" + "net/http" + "testing" + + "github.com/github/github-mcp-server/internal/githubv4mock" + "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/translations" + gogithub "github.com/google/go-github/v82/github" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +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 { + result = append(result, tool) + } + } + return result +} + +func TestGranularToolSnaps(t *testing.T) { + // Test toolsnaps for all granular tools + toolConstructors := []func(translations.TranslationHelperFunc) inventory.ServerTool{ + GranularCreateIssue, + GranularUpdateIssueTitle, + GranularUpdateIssueBody, + GranularUpdateIssueAssignees, + GranularUpdateIssueLabels, + GranularUpdateIssueMilestone, + GranularUpdateIssueType, + GranularUpdateIssueState, + GranularAddSubIssue, + GranularRemoveSubIssue, + GranularReprioritizeSubIssue, + GranularUpdatePullRequestTitle, + GranularUpdatePullRequestBody, + GranularUpdatePullRequestState, + GranularUpdatePullRequestDraftState, + GranularRequestPullRequestReviewers, + GranularCreatePullRequestReview, + GranularSubmitPendingPullRequestReview, + GranularDeletePendingPullRequestReview, + GranularAddPullRequestReviewComment, + GranularResolveReviewThread, + GranularUnresolveReviewThread, + } + + for _, constructor := range toolConstructors { + serverTool := constructor(translations.NullTranslationHelper) + t.Run(serverTool.Tool.Name, func(t *testing.T) { + require.NoError(t, toolsnaps.Test(serverTool.Tool.Name, serverTool.Tool)) + }) + } +} + +func TestIssuesGranularToolset(t *testing.T) { + t.Run("toolset contains expected granular tools", func(t *testing.T) { + tools := granularToolsForToolset(ToolsetMetadataIssues.ID, FeatureFlagIssuesGranular) + + toolNames := make([]string, 0, len(tools)) + for _, tool := range tools { + toolNames = append(toolNames, tool.Tool.Name) + } + + expected := []string{ + "create_issue", + "update_issue_title", + "update_issue_body", + "update_issue_assignees", + "update_issue_labels", + "update_issue_milestone", + "update_issue_type", + "update_issue_state", + "add_sub_issue", + "remove_sub_issue", + "reprioritize_sub_issue", + } + for _, name := range expected { + assert.Contains(t, toolNames, name) + } + assert.Len(t, tools, len(expected)) + }) + + t.Run("all granular tools have correct feature flag", func(t *testing.T) { + for _, tool := range granularToolsForToolset(ToolsetMetadataIssues.ID, FeatureFlagIssuesGranular) { + assert.Equal(t, FeatureFlagIssuesGranular, tool.FeatureFlagEnable, "tool %s", tool.Tool.Name) + } + }) +} + +func TestPullRequestsGranularToolset(t *testing.T) { + t.Run("toolset contains expected granular tools", func(t *testing.T) { + tools := granularToolsForToolset(ToolsetMetadataPullRequests.ID, FeatureFlagPullRequestsGranular) + + toolNames := make([]string, 0, len(tools)) + for _, tool := range tools { + toolNames = append(toolNames, tool.Tool.Name) + } + + expected := []string{ + "update_pull_request_title", + "update_pull_request_body", + "update_pull_request_state", + "update_pull_request_draft_state", + "request_pull_request_reviewers", + "create_pull_request_review", + "submit_pending_pull_request_review", + "delete_pending_pull_request_review", + "add_pull_request_review_comment", + "resolve_review_thread", + "unresolve_review_thread", + } + for _, name := range expected { + assert.Contains(t, toolNames, name) + } + assert.Len(t, tools, len(expected)) + }) + + t.Run("all granular tools have correct feature flag", func(t *testing.T) { + for _, tool := range granularToolsForToolset(ToolsetMetadataPullRequests.ID, FeatureFlagPullRequestsGranular) { + assert.Equal(t, FeatureFlagPullRequestsGranular, tool.FeatureFlagEnable, "tool %s", tool.Tool.Name) + } + }) +} + +// --- Issue granular tool handler tests --- + +func TestGranularCreateIssue(t *testing.T) { + mockIssue := &gogithub.Issue{ + Number: gogithub.Ptr(1), + Title: gogithub.Ptr("Test Issue"), + Body: gogithub.Ptr("Test body"), + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + expectedErrMsg string + }{ + { + name: "successful creation", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesByOwnerByRepo: expectRequestBody(t, map[string]any{ + "title": "Test Issue", + "body": "Test body", + }).andThen(mockResponse(t, http.StatusCreated, mockIssue)), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "title": "Test Issue", + "body": "Test body", + }, + }, + { + name: "missing required parameter", + mockedClient: MockHTTPClientWithHandlers(nil), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + }, + expectedErrMsg: "missing required parameter: title", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := gogithub.NewClient(tc.mockedClient) + deps := BaseDeps{Client: client} + serverTool := GranularCreateIssue(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(tc.requestArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + if tc.expectedErrMsg != "" { + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, tc.expectedErrMsg) + return + } + assert.False(t, result.IsError) + }) + } +} + +func TestGranularUpdateIssueTitle(t *testing.T) { + client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, &gogithub.Issue{ + Number: gogithub.Ptr(42), + Title: gogithub.Ptr("New Title"), + }), + })) + deps := BaseDeps{Client: client} + serverTool := GranularUpdateIssueTitle(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "title": "New Title", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +func TestGranularUpdateIssueBody(t *testing.T) { + client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{ + "body": "Updated body", + }).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{ + Number: gogithub.Ptr(1), + Body: gogithub.Ptr("Updated body"), + })), + })) + deps := BaseDeps{Client: client} + serverTool := GranularUpdateIssueBody(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "body": "Updated body", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +func TestGranularUpdateIssueAssignees(t *testing.T) { + client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{ + "assignees": []any{"user1", "user2"}, + }).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(1)})), + })) + deps := BaseDeps{Client: client} + serverTool := GranularUpdateIssueAssignees(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "assignees": []string{"user1", "user2"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +func TestGranularUpdateIssueLabels(t *testing.T) { + client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{ + "labels": []any{"bug", "enhancement"}, + }).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(1)})), + })) + deps := BaseDeps{Client: client} + serverTool := GranularUpdateIssueLabels(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "labels": []string{"bug", "enhancement"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +func TestGranularUpdateIssueMilestone(t *testing.T) { + client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{ + "milestone": float64(5), + }).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(1)})), + })) + deps := BaseDeps{Client: client} + serverTool := GranularUpdateIssueMilestone(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "milestone": float64(5), + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +func TestGranularUpdateIssueType(t *testing.T) { + client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{ + "type": "bug", + }).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(1)})), + })) + deps := BaseDeps{Client: client} + serverTool := GranularUpdateIssueType(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "issue_type": "bug", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +func TestGranularUpdateIssueState(t *testing.T) { + tests := []struct { + name string + requestArgs map[string]any + expectedReq map[string]any + }{ + { + name: "close with reason", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "state": "closed", + "state_reason": "completed", + }, + expectedReq: map[string]any{ + "state": "closed", + "state_reason": "completed", + }, + }, + { + name: "reopen without reason", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "state": "open", + }, + expectedReq: map[string]any{ + "state": "open", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, tc.expectedReq). + andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{ + Number: gogithub.Ptr(1), + State: gogithub.Ptr(tc.requestArgs["state"].(string)), + })), + })) + deps := BaseDeps{Client: client} + serverTool := GranularUpdateIssueState(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(tc.requestArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) + }) + } +} + +// --- Pull request granular tool handler tests --- + +func TestGranularUpdatePullRequestTitle(t *testing.T) { + client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposPullsByOwnerByRepoByPullNumber: expectRequestBody(t, map[string]any{ + "title": "New PR Title", + }).andThen(mockResponse(t, http.StatusOK, &gogithub.PullRequest{ + Number: gogithub.Ptr(1), + Title: gogithub.Ptr("New PR Title"), + })), + })) + deps := BaseDeps{Client: client} + serverTool := GranularUpdatePullRequestTitle(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(1), + "title": "New PR Title", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +func TestGranularUpdatePullRequestBody(t *testing.T) { + client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposPullsByOwnerByRepoByPullNumber: expectRequestBody(t, map[string]any{ + "body": "Updated description", + }).andThen(mockResponse(t, http.StatusOK, &gogithub.PullRequest{ + Number: gogithub.Ptr(1), + Body: gogithub.Ptr("Updated description"), + })), + })) + deps := BaseDeps{Client: client} + serverTool := GranularUpdatePullRequestBody(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(1), + "body": "Updated description", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +func TestGranularUpdatePullRequestState(t *testing.T) { + client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposPullsByOwnerByRepoByPullNumber: expectRequestBody(t, map[string]any{ + "state": "closed", + }).andThen(mockResponse(t, http.StatusOK, &gogithub.PullRequest{ + Number: gogithub.Ptr(1), + State: gogithub.Ptr("closed"), + })), + })) + deps := BaseDeps{Client: client} + serverTool := GranularUpdatePullRequestState(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(1), + "state": "closed", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +func TestGranularRequestPullRequestReviewers(t *testing.T) { + client := gogithub.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, &gogithub.PullRequest{Number: gogithub.Ptr(1)}), + })) + deps := BaseDeps{Client: client} + serverTool := GranularRequestPullRequestReviewers(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(1), + "reviewers": []string{"user1", "user2"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +func TestGranularCreatePullRequestReview(t *testing.T) { + mockedClient := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "prNum": githubv4.Int(1), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{ + "id": "PR_123", + }, + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + AddPullRequestReview struct { + PullRequestReview struct { + ID githubv4.ID + } + } `graphql:"addPullRequestReview(input: $input)"` + }{}, + githubv4.AddPullRequestReviewInput{ + PullRequestID: githubv4.ID("PR_123"), + Body: githubv4.NewString("LGTM"), + Event: githubv4mock.Ptr(githubv4.PullRequestReviewEventApprove), + }, + nil, + githubv4mock.DataResponse(map[string]any{}), + ), + ) + gqlClient := githubv4.NewClient(mockedClient) + deps := BaseDeps{GQLClient: gqlClient} + serverTool := GranularCreatePullRequestReview(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(1), + "body": "LGTM", + "event": "APPROVE", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +func TestGranularUpdatePullRequestDraftState(t *testing.T) { + tests := []struct { + name string + draft bool + }{ + {name: "convert to draft", draft: true}, + {name: "mark ready for review", draft: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var matchers []githubv4mock.Matcher + + matchers = append(matchers, githubv4mock.NewQueryMatcher( + struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + } `graphql:"pullRequest(number: $number)"` + } `graphql:"repository(owner: $owner, name: $name)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "name": githubv4.String("repo"), + "number": githubv4.Int(1), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{"id": "PR_123"}, + }, + }), + )) + + if tc.draft { + matchers = append(matchers, githubv4mock.NewMutationMatcher( + struct { + ConvertPullRequestToDraft struct { + PullRequest struct { + ID githubv4.ID + IsDraft githubv4.Boolean + } + } `graphql:"convertPullRequestToDraft(input: $input)"` + }{}, + githubv4.ConvertPullRequestToDraftInput{PullRequestID: githubv4.ID("PR_123")}, + nil, + githubv4mock.DataResponse(map[string]any{ + "convertPullRequestToDraft": map[string]any{ + "pullRequest": map[string]any{"id": "PR_123", "isDraft": true}, + }, + }), + )) + } else { + matchers = append(matchers, githubv4mock.NewMutationMatcher( + struct { + MarkPullRequestReadyForReview struct { + PullRequest struct { + ID githubv4.ID + IsDraft githubv4.Boolean + } + } `graphql:"markPullRequestReadyForReview(input: $input)"` + }{}, + githubv4.MarkPullRequestReadyForReviewInput{PullRequestID: githubv4.ID("PR_123")}, + nil, + githubv4mock.DataResponse(map[string]any{ + "markPullRequestReadyForReview": map[string]any{ + "pullRequest": map[string]any{"id": "PR_123", "isDraft": false}, + }, + }), + )) + } + + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matchers...)) + deps := BaseDeps{GQLClient: gqlClient} + serverTool := GranularUpdatePullRequestDraftState(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(1), + "draft": tc.draft, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) + }) + } +} + +func TestGranularAddPullRequestReviewComment(t *testing.T) { + mockedClient := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Viewer struct { + Login githubv4.String + } + }{}, + nil, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{"login": "testuser"}, + }), + ), + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + PullRequest struct { + Reviews struct { + Nodes []struct { + ID githubv4.ID + State githubv4.PullRequestReviewState + URL githubv4.URI + } + } `graphql:"reviews(first: 1, author: $author)"` + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $name)"` + }{}, + map[string]any{ + "author": githubv4.String("testuser"), + "owner": githubv4.String("owner"), + "name": githubv4.String("repo"), + "prNum": githubv4.Int(1), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{ + "reviews": map[string]any{ + "nodes": []map[string]any{ + {"id": "PRR_123", "state": "PENDING", "url": "https://github.com/owner/repo/pull/1#pullrequestreview-123"}, + }, + }, + }, + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + AddPullRequestReviewThread struct { + Thread struct { + ID githubv4.ID + } + } `graphql:"addPullRequestReviewThread(input: $input)"` + }{}, + githubv4.AddPullRequestReviewThreadInput{ + Path: githubv4.String("src/main.go"), + Body: githubv4.String("This needs a fix"), + SubjectType: githubv4mock.Ptr(githubv4.PullRequestReviewThreadSubjectTypeLine), + Line: githubv4mock.Ptr(githubv4.Int(42)), + Side: githubv4mock.Ptr(githubv4.DiffSideRight), + PullRequestReviewID: githubv4mock.Ptr(githubv4.ID("PRR_123")), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "addPullRequestReviewThread": map[string]any{ + "thread": map[string]any{"id": "PRRT_456"}, + }, + }), + ), + ) + gqlClient := githubv4.NewClient(mockedClient) + deps := BaseDeps{GQLClient: gqlClient} + serverTool := GranularAddPullRequestReviewComment(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(1), + "path": "src/main.go", + "body": "This needs a fix", + "subjectType": "LINE", + "line": float64(42), + "side": "RIGHT", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +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) + + 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) +} + +func TestGranularUnresolveReviewThread(t *testing.T) { + mockedClient := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + UnresolveReviewThread struct { + Thread struct { + ID githubv4.ID + IsResolved githubv4.Boolean + } + } `graphql:"unresolveReviewThread(input: $input)"` + }{}, + githubv4.UnresolveReviewThreadInput{ + ThreadID: githubv4.ID("PRRT_123"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "unresolveReviewThread": map[string]any{ + "thread": map[string]any{"id": "PRRT_123", "isResolved": false}, + }, + }), + ), + ) + gqlClient := githubv4.NewClient(mockedClient) + deps := BaseDeps{GQLClient: gqlClient} + serverTool := GranularUnresolveReviewThread(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + 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) +} diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 05af64cab4..81161626bb 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -677,7 +677,7 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool // SubIssueWrite creates a tool to add a sub-issue to a parent issue. func SubIssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { - return NewTool( + st := NewTool( ToolsetMetadataIssues, mcp.Tool{ Name: "sub_issue_write", @@ -787,6 +787,8 @@ Options are: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil } }) + st.FeatureFlagDisable = FeatureFlagIssuesGranular + return st } func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo string, issueNumber int, subIssueID int, replaceParent bool) (*mcp.CallToolResult, error) { @@ -970,7 +972,7 @@ func SearchIssues(t translations.TranslationHelperFunc) inventory.ServerTool { const IssueWriteUIResourceURI = "ui://github-mcp-server/issue-write" func IssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { - return NewTool( + st := NewTool( ToolsetMetadataIssues, mcp.Tool{ Name: "issue_write", @@ -1179,6 +1181,8 @@ Options are: return utils.NewToolResultError("invalid method, must be either 'create' or 'update'"), nil, nil } }) + st.FeatureFlagDisable = FeatureFlagIssuesGranular + return st } func CreateIssue(ctx context.Context, client *github.Client, owner string, repo string, title string, body string, assignees []string, labels []string, milestoneNum int, issueType string) (*mcp.CallToolResult, error) { diff --git a/pkg/github/issues_granular.go b/pkg/github/issues_granular.go new file mode 100644 index 0000000000..3daa1a62e4 --- /dev/null +++ b/pkg/github/issues_granular.go @@ -0,0 +1,595 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + "maps" + "strings" + + ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/scopes" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/github/github-mcp-server/pkg/utils" + "github.com/google/go-github/v82/github" + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// issueUpdateTool is a helper to create single-field issue update tools. +func issueUpdateTool( + t translations.TranslationHelperFunc, + name, description, title string, + extraProps map[string]*jsonschema.Schema, + extraRequired []string, + buildRequest func(args map[string]any) (*github.IssueRequest, error), +) inventory.ServerTool { + props := map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner (username or organization)", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "issue_number": { + Type: "number", + Description: "The issue number to update", + Minimum: jsonschema.Ptr(1.0), + }, + } + maps.Copy(props, extraProps) + + required := append([]string{"owner", "repo", "issue_number"}, extraRequired...) + + st := NewTool( + ToolsetMetadataIssues, + mcp.Tool{ + Name: name, + Description: t("TOOL_"+strings.ToUpper(name)+"_DESCRIPTION", description), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_"+strings.ToUpper(name)+"_USER_TITLE", title), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: props, + Required: required, + }, + }, + []scopes.Scope{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 + } + issueNumber, err := RequiredInt(args, "issue_number") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + issueReq, err := buildRequest(args) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + client, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } + + issue, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueReq) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to update issue", resp, err), nil, nil + } + defer func() { _ = resp.Body.Close() }() + + r, err := json.Marshal(MinimalResponse{ + ID: fmt.Sprintf("%d", issue.GetID()), + URL: issue.GetHTMLURL(), + }) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil + } + return utils.NewToolResultText(string(r)), nil, nil + }, + ) + st.FeatureFlagEnable = FeatureFlagIssuesGranular + return st +} + +// GranularCreateIssue creates a tool to create a new issue. +func GranularCreateIssue(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataIssues, + mcp.Tool{ + Name: "create_issue", + Description: t("TOOL_CREATE_ISSUE_DESCRIPTION", "Create a new issue in a GitHub repository with a title and optional body."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_CREATE_ISSUE_USER_TITLE", "Create Issue"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner (username or organization)", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "title": { + Type: "string", + Description: "Issue title", + }, + "body": { + Type: "string", + Description: "Issue body content (optional)", + }, + }, + Required: []string{"owner", "repo", "title"}, + }, + }, + []scopes.Scope{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 + } + title, err := RequiredParam[string](args, "title") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + body, _ := OptionalParam[string](args, "body") + + issueReq := &github.IssueRequest{ + Title: &title, + } + if body != "" { + issueReq.Body = &body + } + + client, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } + + issue, resp, err := client.Issues.Create(ctx, owner, repo, issueReq) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create issue", resp, err), nil, nil + } + defer func() { _ = resp.Body.Close() }() + + r, err := json.Marshal(MinimalResponse{ + ID: fmt.Sprintf("%d", issue.GetID()), + URL: issue.GetHTMLURL(), + }) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil + } + return utils.NewToolResultText(string(r)), nil, nil + }, + ) + st.FeatureFlagEnable = FeatureFlagIssuesGranular + return st +} + +// GranularUpdateIssueTitle creates a tool to update an issue's title. +func GranularUpdateIssueTitle(t translations.TranslationHelperFunc) inventory.ServerTool { + return issueUpdateTool(t, + "update_issue_title", + "Update the title of an existing issue.", + "Update Issue Title", + map[string]*jsonschema.Schema{ + "title": {Type: "string", Description: "The new title for the issue"}, + }, + []string{"title"}, + func(args map[string]any) (*github.IssueRequest, error) { + title, err := RequiredParam[string](args, "title") + if err != nil { + return nil, err + } + return &github.IssueRequest{Title: &title}, nil + }, + ) +} + +// GranularUpdateIssueBody creates a tool to update an issue's body. +func GranularUpdateIssueBody(t translations.TranslationHelperFunc) inventory.ServerTool { + return issueUpdateTool(t, + "update_issue_body", + "Update the body content of an existing issue.", + "Update Issue Body", + map[string]*jsonschema.Schema{ + "body": {Type: "string", Description: "The new body content for the issue"}, + }, + []string{"body"}, + func(args map[string]any) (*github.IssueRequest, error) { + body, err := RequiredParam[string](args, "body") + if err != nil { + return nil, err + } + return &github.IssueRequest{Body: &body}, nil + }, + ) +} + +// GranularUpdateIssueAssignees creates a tool to update an issue's assignees. +func GranularUpdateIssueAssignees(t translations.TranslationHelperFunc) inventory.ServerTool { + return issueUpdateTool(t, + "update_issue_assignees", + "Update the assignees of an existing issue. This replaces the current assignees with the provided list.", + "Update Issue Assignees", + map[string]*jsonschema.Schema{ + "assignees": { + Type: "array", + Description: "GitHub usernames to assign to this issue", + Items: &jsonschema.Schema{Type: "string"}, + }, + }, + []string{"assignees"}, + func(args map[string]any) (*github.IssueRequest, error) { + if _, ok := args["assignees"]; !ok { + return nil, fmt.Errorf("missing required parameter: assignees") + } + assignees, err := OptionalStringArrayParam(args, "assignees") + if err != nil { + return nil, err + } + return &github.IssueRequest{Assignees: &assignees}, nil + }, + ) +} + +// GranularUpdateIssueLabels creates a tool to update an issue's labels. +func GranularUpdateIssueLabels(t translations.TranslationHelperFunc) inventory.ServerTool { + return issueUpdateTool(t, + "update_issue_labels", + "Update the labels of an existing issue. This replaces the current labels with the provided list.", + "Update Issue Labels", + map[string]*jsonschema.Schema{ + "labels": { + Type: "array", + Description: "Labels to apply to this issue", + Items: &jsonschema.Schema{Type: "string"}, + }, + }, + []string{"labels"}, + func(args map[string]any) (*github.IssueRequest, error) { + if _, ok := args["labels"]; !ok { + return nil, fmt.Errorf("missing required parameter: labels") + } + labels, err := OptionalStringArrayParam(args, "labels") + if err != nil { + return nil, err + } + return &github.IssueRequest{Labels: &labels}, nil + }, + ) +} + +// GranularUpdateIssueMilestone creates a tool to update an issue's milestone. +func GranularUpdateIssueMilestone(t translations.TranslationHelperFunc) inventory.ServerTool { + return issueUpdateTool(t, + "update_issue_milestone", + "Update the milestone of an existing issue.", + "Update Issue Milestone", + map[string]*jsonschema.Schema{ + "milestone": { + Type: "integer", + Description: "The milestone number to set on the issue", + Minimum: jsonschema.Ptr(1.0), + }, + }, + []string{"milestone"}, + func(args map[string]any) (*github.IssueRequest, error) { + milestone, err := RequiredInt(args, "milestone") + if err != nil { + return nil, err + } + return &github.IssueRequest{Milestone: &milestone}, nil + }, + ) +} + +// GranularUpdateIssueType creates a tool to update an issue's type. +func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.ServerTool { + return issueUpdateTool(t, + "update_issue_type", + "Update the type of an existing issue (e.g. 'bug', 'feature').", + "Update Issue Type", + map[string]*jsonschema.Schema{ + "issue_type": { + Type: "string", + Description: "The issue type to set", + }, + }, + []string{"issue_type"}, + func(args map[string]any) (*github.IssueRequest, error) { + issueType, err := RequiredParam[string](args, "issue_type") + if err != nil { + return nil, err + } + return &github.IssueRequest{Type: &issueType}, nil + }, + ) +} + +// GranularUpdateIssueState creates a tool to update an issue's state. +func GranularUpdateIssueState(t translations.TranslationHelperFunc) inventory.ServerTool { + return issueUpdateTool(t, + "update_issue_state", + "Update the state of an existing issue (open or closed), with an optional state reason.", + "Update Issue State", + map[string]*jsonschema.Schema{ + "state": { + Type: "string", + Description: "The new state for the issue", + Enum: []any{"open", "closed"}, + }, + "state_reason": { + Type: "string", + Description: "The reason for the state change (only for closed state)", + Enum: []any{"completed", "not_planned", "duplicate"}, + }, + }, + []string{"state"}, + func(args map[string]any) (*github.IssueRequest, error) { + state, err := RequiredParam[string](args, "state") + if err != nil { + return nil, err + } + req := &github.IssueRequest{State: &state} + + stateReason, _ := OptionalParam[string](args, "state_reason") + if stateReason != "" { + req.StateReason = &stateReason + } + return req, nil + }, + ) +} + +// GranularAddSubIssue creates a tool to add a sub-issue. +func GranularAddSubIssue(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataIssues, + mcp.Tool{ + Name: "add_sub_issue", + Description: t("TOOL_ADD_SUB_ISSUE_DESCRIPTION", "Add a sub-issue to a parent issue."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_ADD_SUB_ISSUE_USER_TITLE", "Add Sub-Issue"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner (username or organization)", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "issue_number": { + Type: "number", + Description: "The parent issue number", + Minimum: jsonschema.Ptr(1.0), + }, + "sub_issue_id": { + Type: "number", + Description: "The ID of the sub-issue to add. ID is not the same as issue number", + }, + "replace_parent": { + Type: "boolean", + Description: "If true, reparent the sub-issue if it already has a parent", + }, + }, + Required: []string{"owner", "repo", "issue_number", "sub_issue_id"}, + }, + }, + []scopes.Scope{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 + } + issueNumber, err := RequiredInt(args, "issue_number") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + subIssueID, err := RequiredInt(args, "sub_issue_id") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + replaceParent, _ := OptionalParam[bool](args, "replace_parent") + + client, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } + + result, err := AddSubIssue(ctx, client, owner, repo, issueNumber, subIssueID, replaceParent) + return result, nil, err + }, + ) + st.FeatureFlagEnable = FeatureFlagIssuesGranular + return st +} + +// GranularRemoveSubIssue creates a tool to remove a sub-issue. +func GranularRemoveSubIssue(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataIssues, + mcp.Tool{ + Name: "remove_sub_issue", + Description: t("TOOL_REMOVE_SUB_ISSUE_DESCRIPTION", "Remove a sub-issue from a parent issue."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_REMOVE_SUB_ISSUE_USER_TITLE", "Remove Sub-Issue"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(true), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner (username or organization)", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "issue_number": { + Type: "number", + Description: "The parent issue number", + Minimum: jsonschema.Ptr(1.0), + }, + "sub_issue_id": { + Type: "number", + Description: "The ID of the sub-issue to remove. ID is not the same as issue number", + }, + }, + Required: []string{"owner", "repo", "issue_number", "sub_issue_id"}, + }, + }, + []scopes.Scope{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 + } + issueNumber, err := RequiredInt(args, "issue_number") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + subIssueID, err := RequiredInt(args, "sub_issue_id") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + client, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } + + result, err := RemoveSubIssue(ctx, client, owner, repo, issueNumber, subIssueID) + return result, nil, err + }, + ) + st.FeatureFlagEnable = FeatureFlagIssuesGranular + return st +} + +// GranularReprioritizeSubIssue creates a tool to reorder a sub-issue. +func GranularReprioritizeSubIssue(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataIssues, + mcp.Tool{ + Name: "reprioritize_sub_issue", + Description: t("TOOL_REPRIORITIZE_SUB_ISSUE_DESCRIPTION", "Reprioritize (reorder) a sub-issue relative to other sub-issues."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_REPRIORITIZE_SUB_ISSUE_USER_TITLE", "Reprioritize Sub-Issue"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner (username or organization)", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "issue_number": { + Type: "number", + Description: "The parent issue number", + Minimum: jsonschema.Ptr(1.0), + }, + "sub_issue_id": { + Type: "number", + Description: "The ID of the sub-issue to reorder. ID is not the same as issue number", + }, + "after_id": { + Type: "number", + Description: "The ID of the sub-issue to place this after (either after_id OR before_id should be specified)", + }, + "before_id": { + Type: "number", + Description: "The ID of the sub-issue to place this before (either after_id OR before_id should be specified)", + }, + }, + Required: []string{"owner", "repo", "issue_number", "sub_issue_id"}, + }, + }, + []scopes.Scope{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 + } + issueNumber, err := RequiredInt(args, "issue_number") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + subIssueID, err := RequiredInt(args, "sub_issue_id") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + afterID, err := OptionalIntParam(args, "after_id") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + beforeID, err := OptionalIntParam(args, "before_id") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + client, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } + + result, err := ReprioritizeSubIssue(ctx, client, owner, repo, issueNumber, subIssueID, afterID, beforeID) + return result, nil, err + }, + ) + st.FeatureFlagEnable = FeatureFlagIssuesGranular + return st +} diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index e5e0855eac..9c2a098755 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -751,7 +751,7 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo Required: []string{"owner", "repo", "pullNumber"}, } - return NewTool( + st := NewTool( ToolsetMetadataPullRequests, mcp.Tool{ Name: "update_pull_request", @@ -990,6 +990,8 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo return utils.NewToolResultText(string(r)), nil, nil }) + st.FeatureFlagDisable = FeatureFlagPullRequestsGranular + return st } // AddReplyToPullRequestComment creates a tool to add a reply to an existing pull request comment. @@ -1507,6 +1509,7 @@ type PullRequestReviewWriteParams struct { Body string Event string CommitID *string + ThreadID string } func PullRequestReviewWrite(t translations.TranslationHelperFunc) inventory.ServerTool { @@ -1519,7 +1522,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"}, + Enum: []any{"create", "submit_pending", "delete_pending", "resolve_thread", "unresolve_thread"}, }, "owner": { Type: "string", @@ -1546,11 +1549,15 @@ func PullRequestReviewWrite(t translations.TranslationHelperFunc) inventory.Serv Type: "string", Description: "SHA of commit to review", }, + "threadId": { + Type: "string", + 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"}, } - return NewTool( + st := NewTool( ToolsetMetadataPullRequests, mcp.Tool{ Name: "pull_request_review_write", @@ -1560,6 +1567,8 @@ Available methods: - create: Create a new review of a pull request. If "event" parameter is provided, the review is submitted. If "event" is omitted, a pending review is created. - submit_pending: Submit an existing pending review of a pull request. This requires that a pending review exists for the current user on the specified pull request. The "body" and "event" parameters are used when submitting the review. - delete_pending: Delete an existing pending review of a pull request. This requires that a pending review exists for the current user on the specified pull request. +- resolve_thread: Resolve a review thread. Requires only "threadId" parameter with the thread's node ID (e.g., PRRT_kwDOxxx). The owner, repo, and pullNumber parameters are not used for this method. Resolving an already-resolved thread is a no-op. +- unresolve_thread: Unresolve a previously resolved review thread. Requires only "threadId" parameter. The owner, repo, and pullNumber parameters are not used for this method. Unresolving an already-unresolved thread is a no-op. `), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_PULL_REQUEST_REVIEW_WRITE_USER_TITLE", "Write operations (create, submit, delete) on pull request reviews."), @@ -1590,10 +1599,18 @@ Available methods: case "delete_pending": result, err := DeletePendingPullRequestReview(ctx, client, params) return result, nil, err + case "resolve_thread": + result, err := ResolveReviewThread(ctx, client, params.ThreadID, true) + return result, nil, err + case "unresolve_thread": + 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 } }) + st.FeatureFlagDisable = FeatureFlagPullRequestsGranular + return st } func CreatePullRequestReview(ctx context.Context, client *githubv4.Client, params PullRequestReviewWriteParams) (*mcp.CallToolResult, error) { @@ -1819,6 +1836,167 @@ func DeletePendingPullRequestReview(ctx context.Context, client *githubv4.Client return utils.NewToolResultText("pending pull request review successfully deleted"), nil } +// 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) { + if threadID == "" { + return utils.NewToolResultError("threadId is required for resolve_thread and unresolve_thread methods"), nil + } + + if resolve { + var mutation struct { + ResolveReviewThread struct { + Thread struct { + ID githubv4.ID + IsResolved githubv4.Boolean + } + } `graphql:"resolveReviewThread(input: $input)"` + } + + input := githubv4.ResolveReviewThreadInput{ + ThreadID: githubv4.ID(threadID), + } + + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, + "failed to resolve review thread", + err, + ), nil + } + + return utils.NewToolResultText("review thread resolved successfully"), nil + } + + // Unresolve + var mutation struct { + UnresolveReviewThread struct { + Thread struct { + ID githubv4.ID + IsResolved githubv4.Boolean + } + } `graphql:"unresolveReviewThread(input: $input)"` + } + + input := githubv4.UnresolveReviewThreadInput{ + ThreadID: githubv4.ID(threadID), + } + + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, + "failed to unresolve review thread", + err, + ), nil + } + + return utils.NewToolResultText("review thread unresolved successfully"), nil +} + +// AddCommentToPendingReviewParams contains the parameters for adding a comment to a pending review. +type AddCommentToPendingReviewParams struct { + Owner string + Repo string + PullNumber int32 + Path string + Body string + SubjectType string + Line *int32 + Side *string + StartLine *int32 + StartSide *string +} + +// AddCommentToPendingReviewCall adds a review comment to the viewer's pending pull request review. +func AddCommentToPendingReviewCall(ctx context.Context, client *githubv4.Client, params AddCommentToPendingReviewParams) (*mcp.CallToolResult, error) { + // Get the current user + var getViewerQuery struct { + Viewer struct { + Login githubv4.String + } + } + + if err := client.Query(ctx, &getViewerQuery, nil); err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, + "failed to get current user", + err, + ), nil + } + + var getLatestReviewForViewerQuery struct { + Repository struct { + PullRequest struct { + Reviews struct { + Nodes []struct { + ID githubv4.ID + State githubv4.PullRequestReviewState + URL githubv4.URI + } + } `graphql:"reviews(first: 1, author: $author)"` + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + + vars := map[string]any{ + "author": githubv4.String(getViewerQuery.Viewer.Login), + "owner": githubv4.String(params.Owner), + "name": githubv4.String(params.Repo), + "prNum": githubv4.Int(params.PullNumber), + } + + if err := client.Query(ctx, &getLatestReviewForViewerQuery, vars); err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, + "failed to get latest review for current user", + err, + ), nil + } + + // Validate there is one review and the state is pending + if len(getLatestReviewForViewerQuery.Repository.PullRequest.Reviews.Nodes) == 0 { + return utils.NewToolResultError("No pending review found for the viewer"), nil + } + + review := getLatestReviewForViewerQuery.Repository.PullRequest.Reviews.Nodes[0] + if review.State != githubv4.PullRequestReviewStatePending { + errText := fmt.Sprintf("The latest review, found at %s is not pending", review.URL) + return utils.NewToolResultError(errText), nil + } + + // Create a new review thread comment on the review. + var addPullRequestReviewThreadMutation struct { + AddPullRequestReviewThread struct { + Thread struct { + ID githubv4.ID + } + } `graphql:"addPullRequestReviewThread(input: $input)"` + } + + if err := client.Mutate( + ctx, + &addPullRequestReviewThreadMutation, + githubv4.AddPullRequestReviewThreadInput{ + Path: githubv4.String(params.Path), + Body: githubv4.String(params.Body), + SubjectType: newGQLStringlikePtr[githubv4.PullRequestReviewThreadSubjectType](¶ms.SubjectType), + Line: newGQLIntPtr(params.Line), + Side: newGQLStringlikePtr[githubv4.DiffSide](params.Side), + StartLine: newGQLIntPtr(params.StartLine), + StartSide: newGQLStringlikePtr[githubv4.DiffSide](params.StartSide), + PullRequestReviewID: &review.ID, + }, + nil, + ); err != nil { + return utils.NewToolResultError(err.Error()), nil + } + + if addPullRequestReviewThreadMutation.AddPullRequestReviewThread.Thread.ID == nil { + return utils.NewToolResultError(`Failed to add comment to pending review. Possible reasons: + - The line number doesn't exist in the pull request diff + - The file path is incorrect + - The side (LEFT/RIGHT) is invalid for the specified line +`), nil + } + + return utils.NewToolResultText("pull request review comment successfully added to pending review"), nil +} + // AddCommentToPendingReview creates a tool to add a comment to a pull request review. func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.ServerTool { schema := &jsonschema.Schema{ @@ -1880,7 +2058,7 @@ func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.S Required: []string{"owner", "repo", "pullNumber", "path", "body", "subjectType"}, } - return NewTool( + st := NewTool( ToolsetMetadataPullRequests, mcp.Tool{ Name: "add_comment_to_pending_review", @@ -1914,99 +2092,22 @@ func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.S return utils.NewToolResultErrorFromErr("failed to get GitHub GQL client", err), nil, nil } - // First we'll get the current user - var getViewerQuery struct { - Viewer struct { - Login githubv4.String - } - } - - if err := client.Query(ctx, &getViewerQuery, nil); err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get current user", - err, - ), nil, nil - } - - var getLatestReviewForViewerQuery struct { - Repository struct { - PullRequest struct { - Reviews struct { - Nodes []struct { - ID githubv4.ID - State githubv4.PullRequestReviewState - URL githubv4.URI - } - } `graphql:"reviews(first: 1, author: $author)"` - } `graphql:"pullRequest(number: $prNum)"` - } `graphql:"repository(owner: $owner, name: $name)"` - } - - vars := map[string]any{ - "author": githubv4.String(getViewerQuery.Viewer.Login), - "owner": githubv4.String(params.Owner), - "name": githubv4.String(params.Repo), - "prNum": githubv4.Int(params.PullNumber), - } - - if err := client.Query(ctx, &getLatestReviewForViewerQuery, vars); err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get latest review for current user", - err, - ), nil, nil - } - - // Validate there is one review and the state is pending - if len(getLatestReviewForViewerQuery.Repository.PullRequest.Reviews.Nodes) == 0 { - return utils.NewToolResultError("No pending review found for the viewer"), nil, nil - } - - review := getLatestReviewForViewerQuery.Repository.PullRequest.Reviews.Nodes[0] - if review.State != githubv4.PullRequestReviewStatePending { - errText := fmt.Sprintf("The latest review, found at %s is not pending", review.URL) - return utils.NewToolResultError(errText), nil, nil - } - - // Then we can create a new review thread comment on the review. - var addPullRequestReviewThreadMutation struct { - AddPullRequestReviewThread struct { - Thread struct { - ID githubv4.ID // We don't need this, but a selector is required or GQL complains. - } - } `graphql:"addPullRequestReviewThread(input: $input)"` - } - - if err := client.Mutate( - ctx, - &addPullRequestReviewThreadMutation, - githubv4.AddPullRequestReviewThreadInput{ - Path: githubv4.String(params.Path), - Body: githubv4.String(params.Body), - SubjectType: newGQLStringlikePtr[githubv4.PullRequestReviewThreadSubjectType](¶ms.SubjectType), - Line: newGQLIntPtr(params.Line), - Side: newGQLStringlikePtr[githubv4.DiffSide](params.Side), - StartLine: newGQLIntPtr(params.StartLine), - StartSide: newGQLStringlikePtr[githubv4.DiffSide](params.StartSide), - PullRequestReviewID: &review.ID, - }, - nil, - ); err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - - if addPullRequestReviewThreadMutation.AddPullRequestReviewThread.Thread.ID == nil { - return utils.NewToolResultError(`Failed to add comment to pending review. Possible reasons: - - The line number doesn't exist in the pull request diff - - The file path is incorrect - - The side (LEFT/RIGHT) is invalid for the specified line -`), nil, nil - } - - // Return nothing interesting, just indicate success for the time being. - // In future, we may want to return the review ID, but for the moment, we're not leaking - // API implementation details to the LLM. - return utils.NewToolResultText("pull request review comment successfully added to pending review"), nil, nil + result, err := AddCommentToPendingReviewCall(ctx, client, AddCommentToPendingReviewParams{ + Owner: params.Owner, + Repo: params.Repo, + PullNumber: params.PullNumber, + Path: params.Path, + Body: params.Body, + SubjectType: params.SubjectType, + Line: params.Line, + Side: params.Side, + StartLine: params.StartLine, + StartSide: params.StartSide, + }) + return result, nil, err }) + st.FeatureFlagDisable = FeatureFlagPullRequestsGranular + return st } // newGQLString like takes something that approximates a string (of which there are many types in shurcooL/githubv4) diff --git a/pkg/github/pullrequests_granular.go b/pkg/github/pullrequests_granular.go new file mode 100644 index 0000000000..4a616f1b25 --- /dev/null +++ b/pkg/github/pullrequests_granular.go @@ -0,0 +1,739 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + "maps" + "strings" + + ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/scopes" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/github/github-mcp-server/pkg/utils" + gogithub "github.com/google/go-github/v82/github" + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/shurcooL/githubv4" +) + +// prUpdateTool is a helper to create single-field pull request update tools via REST. +func prUpdateTool( + t translations.TranslationHelperFunc, + name, description, title string, + extraProps map[string]*jsonschema.Schema, + extraRequired []string, + buildRequest func(args map[string]any) (*gogithub.PullRequest, error), +) inventory.ServerTool { + props := map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner (username or organization)", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "pullNumber": { + Type: "number", + Description: "The pull request number", + Minimum: jsonschema.Ptr(1.0), + }, + } + maps.Copy(props, extraProps) + + required := append([]string{"owner", "repo", "pullNumber"}, extraRequired...) + + st := NewTool( + ToolsetMetadataPullRequests, + mcp.Tool{ + Name: name, + Description: t("TOOL_"+strings.ToUpper(name)+"_DESCRIPTION", description), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_"+strings.ToUpper(name)+"_USER_TITLE", title), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: props, + Required: required, + }, + }, + []scopes.Scope{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 + } + pullNumber, err := RequiredInt(args, "pullNumber") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + prReq, err := buildRequest(args) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + client, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } + + pr, resp, err := client.PullRequests.Edit(ctx, owner, repo, pullNumber, prReq) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to update pull request", resp, err), nil, nil + } + defer func() { _ = resp.Body.Close() }() + + r, err := json.Marshal(MinimalResponse{ + ID: fmt.Sprintf("%d", pr.GetID()), + URL: pr.GetHTMLURL(), + }) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil + } + return utils.NewToolResultText(string(r)), nil, nil + }, + ) + st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + return st +} + +// GranularUpdatePullRequestTitle creates a tool to update a PR's title. +func GranularUpdatePullRequestTitle(t translations.TranslationHelperFunc) inventory.ServerTool { + return prUpdateTool(t, + "update_pull_request_title", + "Update the title of an existing pull request.", + "Update Pull Request Title", + map[string]*jsonschema.Schema{ + "title": {Type: "string", Description: "The new title for the pull request"}, + }, + []string{"title"}, + func(args map[string]any) (*gogithub.PullRequest, error) { + title, err := RequiredParam[string](args, "title") + if err != nil { + return nil, err + } + return &gogithub.PullRequest{Title: &title}, nil + }, + ) +} + +// GranularUpdatePullRequestBody creates a tool to update a PR's body. +func GranularUpdatePullRequestBody(t translations.TranslationHelperFunc) inventory.ServerTool { + return prUpdateTool(t, + "update_pull_request_body", + "Update the body description of an existing pull request.", + "Update Pull Request Body", + map[string]*jsonschema.Schema{ + "body": {Type: "string", Description: "The new body content for the pull request"}, + }, + []string{"body"}, + func(args map[string]any) (*gogithub.PullRequest, error) { + body, err := RequiredParam[string](args, "body") + if err != nil { + return nil, err + } + return &gogithub.PullRequest{Body: &body}, nil + }, + ) +} + +// GranularUpdatePullRequestState creates a tool to update a PR's state. +func GranularUpdatePullRequestState(t translations.TranslationHelperFunc) inventory.ServerTool { + return prUpdateTool(t, + "update_pull_request_state", + "Update the state of an existing pull request (open or closed).", + "Update Pull Request State", + map[string]*jsonschema.Schema{ + "state": { + Type: "string", + Description: "The new state for the pull request", + Enum: []any{"open", "closed"}, + }, + }, + []string{"state"}, + func(args map[string]any) (*gogithub.PullRequest, error) { + state, err := RequiredParam[string](args, "state") + if err != nil { + return nil, err + } + return &gogithub.PullRequest{State: &state}, nil + }, + ) +} + +// GranularUpdatePullRequestDraftState creates a tool to toggle draft state. +func GranularUpdatePullRequestDraftState(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataPullRequests, + mcp.Tool{ + Name: "update_pull_request_draft_state", + Description: t("TOOL_UPDATE_PULL_REQUEST_DRAFT_STATE_DESCRIPTION", "Mark a pull request as draft or ready for review."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_UPDATE_PULL_REQUEST_DRAFT_STATE_USER_TITLE", "Update Pull Request Draft State"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": {Type: "string", Description: "Repository owner (username or organization)"}, + "repo": {Type: "string", Description: "Repository name"}, + "pullNumber": {Type: "number", Description: "The pull request number", Minimum: jsonschema.Ptr(1.0)}, + "draft": {Type: "boolean", Description: "Set to true to convert to draft, false to mark as ready for review"}, + }, + Required: []string{"owner", "repo", "pullNumber", "draft"}, + }, + }, + []scopes.Scope{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 + } + pullNumber, err := RequiredInt(args, "pullNumber") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + // Use presence check + OptionalParam since RequiredParam rejects false (zero-value for bool) + if _, ok := args["draft"]; !ok { + return utils.NewToolResultError("missing required parameter: draft"), nil, nil + } + draft, err := OptionalParam[bool](args, "draft") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + gqlClient, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil + } + + // Get PR node ID + var prQuery struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + } `graphql:"pullRequest(number: $number)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + if err := gqlClient.Query(ctx, &prQuery, map[string]any{ + "owner": githubv4.String(owner), + "name": githubv4.String(repo), + "number": githubv4.Int(pullNumber), // #nosec G115 - PR numbers are always small positive integers + }); err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to get pull request", err), nil, nil + } + + if draft { + var mutation struct { + ConvertPullRequestToDraft struct { + PullRequest struct { + ID githubv4.ID + IsDraft githubv4.Boolean + } + } `graphql:"convertPullRequestToDraft(input: $input)"` + } + if err := gqlClient.Mutate(ctx, &mutation, githubv4.ConvertPullRequestToDraftInput{ + PullRequestID: prQuery.Repository.PullRequest.ID, + }, nil); err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to convert to draft", err), nil, nil + } + return utils.NewToolResultText("pull request converted to draft"), nil, nil + } + + var mutation struct { + MarkPullRequestReadyForReview struct { + PullRequest struct { + ID githubv4.ID + IsDraft githubv4.Boolean + } + } `graphql:"markPullRequestReadyForReview(input: $input)"` + } + if err := gqlClient.Mutate(ctx, &mutation, githubv4.MarkPullRequestReadyForReviewInput{ + PullRequestID: prQuery.Repository.PullRequest.ID, + }, nil); err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to mark ready for review", err), nil, nil + } + return utils.NewToolResultText("pull request marked as ready for review"), nil, nil + }, + ) + st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + return st +} + +// GranularRequestPullRequestReviewers creates a tool to request reviewers. +func GranularRequestPullRequestReviewers(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataPullRequests, + mcp.Tool{ + Name: "request_pull_request_reviewers", + Description: t("TOOL_REQUEST_PULL_REQUEST_REVIEWERS_DESCRIPTION", "Request reviewers for a pull request."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_REQUEST_PULL_REQUEST_REVIEWERS_USER_TITLE", "Request Pull Request Reviewers"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": {Type: "string", Description: "Repository owner (username or organization)"}, + "repo": {Type: "string", Description: "Repository name"}, + "pullNumber": {Type: "number", Description: "The pull request number", Minimum: jsonschema.Ptr(1.0)}, + "reviewers": { + Type: "array", + Description: "GitHub usernames to request reviews from", + Items: &jsonschema.Schema{Type: "string"}, + }, + }, + Required: []string{"owner", "repo", "pullNumber", "reviewers"}, + }, + }, + []scopes.Scope{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 + } + pullNumber, err := RequiredInt(args, "pullNumber") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + reviewers, err := OptionalStringArrayParam(args, "reviewers") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + if len(reviewers) == 0 { + return utils.NewToolResultError("missing required parameter: reviewers"), nil, nil + } + + client, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } + + pr, resp, err := client.PullRequests.RequestReviewers(ctx, owner, repo, pullNumber, gogithub.ReviewersRequest{Reviewers: reviewers}) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to request reviewers", resp, err), nil, nil + } + defer func() { _ = resp.Body.Close() }() + + r, err := json.Marshal(MinimalResponse{ + ID: fmt.Sprintf("%d", pr.GetID()), + URL: pr.GetHTMLURL(), + }) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil + } + return utils.NewToolResultText(string(r)), nil, nil + }, + ) + st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + return st +} + +// GranularCreatePullRequestReview creates a tool to create a PR review. +func GranularCreatePullRequestReview(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataPullRequests, + mcp.Tool{ + Name: "create_pull_request_review", + Description: t("TOOL_CREATE_PULL_REQUEST_REVIEW_DESCRIPTION", "Create a review on a pull request. If event is provided, the review is submitted immediately; otherwise a pending review is created."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_CREATE_PULL_REQUEST_REVIEW_USER_TITLE", "Create Pull Request Review"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": {Type: "string", Description: "Repository owner (username or organization)"}, + "repo": {Type: "string", Description: "Repository name"}, + "pullNumber": {Type: "number", Description: "The pull request number", Minimum: jsonschema.Ptr(1.0)}, + "body": {Type: "string", Description: "The review body text (optional)"}, + "event": {Type: "string", Description: "The review action to perform. If omitted, creates a pending review.", Enum: []any{"APPROVE", "REQUEST_CHANGES", "COMMENT"}}, + "commitID": {Type: "string", Description: "The SHA of the commit to review (optional, defaults to latest)"}, + }, + Required: []string{"owner", "repo", "pullNumber"}, + }, + }, + []scopes.Scope{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 + } + pullNumber, err := RequiredInt(args, "pullNumber") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + body, _ := OptionalParam[string](args, "body") + event, _ := OptionalParam[string](args, "event") + commitID, _ := OptionalParam[string](args, "commitID") + + gqlClient, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil + } + + var commitIDPtr *string + if commitID != "" { + commitIDPtr = &commitID + } + + result, err := CreatePullRequestReview(ctx, gqlClient, PullRequestReviewWriteParams{ + Owner: owner, + Repo: repo, + PullNumber: int32(pullNumber), // #nosec G115 - PR numbers are always small positive integers + Body: body, + Event: event, + CommitID: commitIDPtr, + }) + return result, nil, err + }, + ) + st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + return st +} + +// GranularSubmitPendingPullRequestReview creates a tool to submit a pending review. +func GranularSubmitPendingPullRequestReview(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataPullRequests, + mcp.Tool{ + Name: "submit_pending_pull_request_review", + Description: t("TOOL_SUBMIT_PENDING_PULL_REQUEST_REVIEW_DESCRIPTION", "Submit a pending pull request review."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_SUBMIT_PENDING_PULL_REQUEST_REVIEW_USER_TITLE", "Submit Pending Pull Request Review"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": {Type: "string", Description: "Repository owner (username or organization)"}, + "repo": {Type: "string", Description: "Repository name"}, + "pullNumber": {Type: "number", Description: "The pull request number", Minimum: jsonschema.Ptr(1.0)}, + "event": {Type: "string", Description: "The review action to perform", Enum: []any{"APPROVE", "REQUEST_CHANGES", "COMMENT"}}, + "body": {Type: "string", Description: "The review body text (optional)"}, + }, + Required: []string{"owner", "repo", "pullNumber", "event"}, + }, + }, + []scopes.Scope{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 + } + pullNumber, err := RequiredInt(args, "pullNumber") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + event, err := RequiredParam[string](args, "event") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + body, _ := OptionalParam[string](args, "body") + + gqlClient, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil + } + + result, err := SubmitPendingPullRequestReview(ctx, gqlClient, PullRequestReviewWriteParams{ + Owner: owner, + Repo: repo, + PullNumber: int32(pullNumber), // #nosec G115 - PR numbers are always small positive integers + Event: event, + Body: body, + }) + return result, nil, err + }, + ) + st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + return st +} + +// GranularDeletePendingPullRequestReview creates a tool to delete a pending review. +func GranularDeletePendingPullRequestReview(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataPullRequests, + mcp.Tool{ + Name: "delete_pending_pull_request_review", + Description: t("TOOL_DELETE_PENDING_PULL_REQUEST_REVIEW_DESCRIPTION", "Delete a pending pull request review."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_DELETE_PENDING_PULL_REQUEST_REVIEW_USER_TITLE", "Delete Pending Pull Request Review"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(true), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": {Type: "string", Description: "Repository owner (username or organization)"}, + "repo": {Type: "string", Description: "Repository name"}, + "pullNumber": {Type: "number", Description: "The pull request number", Minimum: jsonschema.Ptr(1.0)}, + }, + Required: []string{"owner", "repo", "pullNumber"}, + }, + }, + []scopes.Scope{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 + } + pullNumber, err := RequiredInt(args, "pullNumber") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + gqlClient, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil + } + + result, err := DeletePendingPullRequestReview(ctx, gqlClient, PullRequestReviewWriteParams{ + Owner: owner, + Repo: repo, + PullNumber: int32(pullNumber), // #nosec G115 - PR numbers are always small positive integers + }) + return result, nil, err + }, + ) + st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + return st +} + +// GranularAddPullRequestReviewComment creates a tool to add a review comment. +func GranularAddPullRequestReviewComment(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataPullRequests, + mcp.Tool{ + Name: "add_pull_request_review_comment", + Description: t("TOOL_ADD_PULL_REQUEST_REVIEW_COMMENT_DESCRIPTION", "Add a review comment to the current user's pending pull request review."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_ADD_PULL_REQUEST_REVIEW_COMMENT_USER_TITLE", "Add Pull Request Review Comment"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": {Type: "string", Description: "Repository owner (username or organization)"}, + "repo": {Type: "string", Description: "Repository name"}, + "pullNumber": {Type: "number", Description: "The pull request number", Minimum: jsonschema.Ptr(1.0)}, + "path": {Type: "string", Description: "The relative path of the file to comment on"}, + "body": {Type: "string", Description: "The comment body"}, + "subjectType": {Type: "string", Description: "The subject type of the comment", Enum: []any{"FILE", "LINE"}}, + "line": {Type: "number", Description: "The line number in the diff to comment on (optional)"}, + "side": {Type: "string", Description: "The side of the diff to comment on (optional)", Enum: []any{"LEFT", "RIGHT"}}, + "startLine": {Type: "number", Description: "The start line of a multi-line comment (optional)"}, + "startSide": {Type: "string", Description: "The start side of a multi-line comment (optional)", Enum: []any{"LEFT", "RIGHT"}}, + }, + Required: []string{"owner", "repo", "pullNumber", "path", "body", "subjectType"}, + }, + }, + []scopes.Scope{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 + } + pullNumber, err := RequiredInt(args, "pullNumber") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + path, err := RequiredParam[string](args, "path") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + body, err := RequiredParam[string](args, "body") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + subjectType, err := RequiredParam[string](args, "subjectType") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + line, err := OptionalIntParam(args, "line") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + side, _ := OptionalParam[string](args, "side") + startLine, err := OptionalIntParam(args, "startLine") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + startSide, _ := OptionalParam[string](args, "startSide") + + gqlClient, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil + } + + // Convert optional int params to *int32 for the helper + var linePtr, startLinePtr *int32 + if line != 0 { + l := int32(line) // #nosec G115 + linePtr = &l + } + if startLine != 0 { + sl := int32(startLine) // #nosec G115 + startLinePtr = &sl + } + + // Convert optional string params: pass nil (not empty string) when absent + var sidePtr, startSidePtr *string + if side != "" { + sidePtr = &side + } + if startSide != "" { + startSidePtr = &startSide + } + + result, err := AddCommentToPendingReviewCall(ctx, gqlClient, AddCommentToPendingReviewParams{ + Owner: owner, + Repo: repo, + PullNumber: int32(pullNumber), // #nosec G115 - PR numbers are always small positive integers + Path: path, + Body: body, + SubjectType: subjectType, + Line: linePtr, + Side: sidePtr, + StartLine: startLinePtr, + StartSide: startSidePtr, + }) + return result, nil, err + }, + ) + st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + return st +} + +// GranularResolveReviewThread creates a tool to resolve a review thread. +func GranularResolveReviewThread(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataPullRequests, + mcp.Tool{ + Name: "resolve_review_thread", + Description: t("TOOL_RESOLVE_REVIEW_THREAD_DESCRIPTION", "Resolve a review thread on a pull request. Resolving an already-resolved thread is a no-op."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_RESOLVE_REVIEW_THREAD_USER_TITLE", "Resolve Review Thread"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + 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"}, + }, + }, + []scopes.Scope{scopes.Repo}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + threadID, err := RequiredParam[string](args, "threadID") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + 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) + return result, nil, err + }, + ) + st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + return st +} + +// GranularUnresolveReviewThread creates a tool to unresolve a review thread. +func GranularUnresolveReviewThread(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataPullRequests, + mcp.Tool{ + Name: "unresolve_review_thread", + Description: t("TOOL_UNRESOLVE_REVIEW_THREAD_DESCRIPTION", "Unresolve a previously resolved review thread on a pull request. Unresolving an already-unresolved thread is a no-op."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_UNRESOLVE_REVIEW_THREAD_USER_TITLE", "Unresolve Review Thread"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + 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 unresolve (e.g., PRRT_kwDOxxx)", + }, + }, + Required: []string{"threadID"}, + }, + }, + []scopes.Scope{scopes.Repo}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + threadID, err := RequiredParam[string](args, "threadID") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + 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, false) + return result, nil, err + }, + ) + st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + return st +} diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 5375773299..801122dca8 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -3609,3 +3609,198 @@ func TestAddReplyToPullRequestComment(t *testing.T) { }) } } + +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: "successful resolve thread", + requestArgs: map[string]any{ + "method": "resolve_thread", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "threadId": "PRRT_kwDOTest123", + }, + 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_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 unresolve thread", + requestArgs: map[string]any{ + "method": "unresolve_thread", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "threadId": "PRRT_kwDOTest123", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + UnresolveReviewThread struct { + Thread struct { + ID githubv4.ID + IsResolved githubv4.Boolean + } + } `graphql:"unresolveReviewThread(input: $input)"` + }{}, + githubv4.UnresolveReviewThreadInput{ + ThreadID: githubv4.ID("PRRT_kwDOTest123"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "unresolveReviewThread": map[string]any{ + "thread": map[string]any{ + "id": "PRRT_kwDOTest123", + "isResolved": false, + }, + }, + }), + ), + ), + expectedResult: "review thread unresolved successfully", + }, + { + name: "empty threadId for resolve", + requestArgs: map[string]any{ + "method": "resolve_thread", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "threadId": "", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedToolErrMsg: "threadId is required", + }, + { + name: "empty threadId for unresolve", + requestArgs: map[string]any{ + "method": "unresolve_thread", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "threadId": "", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedToolErrMsg: "threadId is required", + }, + { + name: "omitted threadId for resolve", + requestArgs: map[string]any{ + "method": "resolve_thread", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedToolErrMsg: "threadId is required", + }, + { + name: "omitted threadId for unresolve", + requestArgs: map[string]any{ + "method": "unresolve_thread", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedToolErrMsg: "threadId is required", + }, + { + name: "thread not found", + requestArgs: map[string]any{ + "method": "resolve_thread", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "threadId": "PRRT_invalid", + }, + 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_invalid"), + }, + nil, + githubv4mock.ErrorResponse("Could not resolve to a PullRequestReviewThread with the id of 'PRRT_invalid'"), + ), + ), + expectToolError: true, + expectedToolErrMsg: "Could not resolve to a PullRequestReviewThread", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + // Setup client with mock + client := githubv4.NewClient(tc.mockedClient) + serverTool := PullRequestReviewWrite(translations.NullTranslationHelper) + deps := BaseDeps{ + GQLClient: client, + } + handler := serverTool.Handler(deps) + + // Create call request + request := createMCPRequest(tc.requestArgs) + + // Call handler + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + + if tc.expectToolError { + require.True(t, result.IsError) + assert.Contains(t, textContent.Text, tc.expectedToolErrMsg) + return + } + + require.False(t, result.IsError) + assert.Equal(t, tc.expectedResult, textContent.Text) + }) + } +} diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 9376ddad43..9577b37b69 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -147,6 +147,18 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { Type: "string", Description: "Author username or email address to filter commits by", }, + "path": { + Type: "string", + Description: "Only commits containing this file path will be returned", + }, + "since": { + Type: "string", + Description: "Only commits after this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD)", + }, + "until": { + Type: "string", + Description: "Only commits before this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD)", + }, }, Required: []string{"owner", "repo"}, }), @@ -169,6 +181,18 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { 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 + } + sinceStr, err := OptionalParam[string](args, "since") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + untilStr, err := OptionalParam[string](args, "until") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } pagination, err := OptionalPaginationParams(args) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil @@ -180,12 +204,27 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { } opts := &github.CommitsListOptions{ SHA: sha, + Path: path, Author: author, ListOptions: github.ListOptions{ Page: pagination.Page, PerPage: perPage, }, } + if sinceStr != "" { + sinceTime, err := parseISOTimestamp(sinceStr) + 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 err != nil { + return utils.NewToolResultError(fmt.Sprintf("invalid until timestamp: %s", err)), nil, nil + } + opts.Until = untilTime + } client, err := deps.GetClient(ctx) if err != nil { @@ -1233,7 +1272,8 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { Type: "array", Description: "Array of file objects to push, each object with path (string) and content (string)", Items: &jsonschema.Schema{ - Type: "object", + Type: "object", + AdditionalProperties: &jsonschema.Schema{Not: &jsonschema.Schema{}}, Properties: map[string]*jsonschema.Schema{ "path": { Type: "string", diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index ae2ece0f60..d7bb487382 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -900,6 +900,9 @@ func Test_ListCommits(t *testing.T) { assert.Contains(t, schema.Properties, "repo") assert.Contains(t, schema.Properties, "sha") assert.Contains(t, schema.Properties, "author") + assert.Contains(t, schema.Properties, "path") + assert.Contains(t, schema.Properties, "since") + assert.Contains(t, schema.Properties, "until") assert.Contains(t, schema.Properties, "page") assert.Contains(t, schema.Properties, "perPage") assert.ElementsMatch(t, schema.Required, []string{"owner", "repo"}) @@ -1020,6 +1023,80 @@ func Test_ListCommits(t *testing.T) { expectError: false, expectedCommits: mockCommits, }, + { + name: "successful commits fetch with path filter", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposCommitsByOwnerByRepo: expectQueryParams(t, map[string]string{ + "path": "src/main.go", + "page": "1", + "per_page": "30", + }).andThen( + mockResponse(t, http.StatusOK, mockCommits), + ), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "src/main.go", + }, + expectError: false, + expectedCommits: mockCommits, + }, + { + name: "successful commits fetch with since and until", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposCommitsByOwnerByRepo: expectQueryParams(t, map[string]string{ + "since": "2023-01-01T00:00:00Z", + "until": "2023-12-31T23:59:59Z", + "page": "1", + "per_page": "30", + }).andThen( + mockResponse(t, http.StatusOK, mockCommits), + ), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "since": "2023-01-01T00:00:00Z", + "until": "2023-12-31T23:59:59Z", + }, + expectError: false, + expectedCommits: mockCommits, + }, + { + name: "successful commits fetch with path, since, and author", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposCommitsByOwnerByRepo: expectQueryParams(t, map[string]string{ + "path": "projects/plugins/boost", + "since": "2023-06-15T00:00:00Z", + "author": "username", + "page": "1", + "per_page": "30", + }).andThen( + mockResponse(t, http.StatusOK, mockCommits), + ), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": "projects/plugins/boost", + "since": "2023-06-15T00:00:00Z", + "author": "username", + }, + expectError: false, + expectedCommits: mockCommits, + }, + { + name: "invalid since timestamp returns error", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "since": "not-a-date", + }, + expectError: true, + expectedErrMsg: "invalid since timestamp", + }, { name: "successful commits fetch with pagination", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ diff --git a/pkg/github/server.go b/pkg/github/server.go index 06c12575d2..ee41e90e9e 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -101,7 +101,7 @@ func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependenci } } - ghServer := NewServer(cfg.Version, serverOpts) + ghServer := NewServer(cfg.Version, cfg.Translator("SERVER_NAME", "github-mcp-server"), cfg.Translator("SERVER_TITLE", "GitHub MCP Server"), serverOpts) // Add middlewares. Order matters - for example, the error context middleware should be applied last so that it runs FIRST (closest to the handler) to ensure all errors are captured, // and any middleware that needs to read or modify the context should be before it. @@ -176,16 +176,25 @@ func addGitHubAPIErrorToContext(next mcp.MethodHandler) mcp.MethodHandler { } } -// NewServer creates a new GitHub MCP server with the specified GH client and logger. -func NewServer(version string, opts *mcp.ServerOptions) *mcp.Server { +// NewServer creates a new GitHub MCP server with the given version, server +// name, display title, and options. If name or title are empty the defaults +// "github-mcp-server" and "GitHub MCP Server" are used. +func NewServer(version, name, title string, opts *mcp.ServerOptions) *mcp.Server { if opts == nil { opts = &mcp.ServerOptions{} } + if name == "" { + name = "github-mcp-server" + } + if title == "" { + title = "GitHub MCP Server" + } + // Create a new MCP server s := mcp.NewServer(&mcp.Implementation{ - Name: "github-mcp-server", - Title: "GitHub MCP Server", + Name: name, + Title: title, Version: version, Icons: octicons.Icons("mark-github"), }, opts) diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index 2b99cab12c..bf29ed1329 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -5,14 +5,18 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "net/http" "testing" "time" "github.com/github/github-mcp-server/pkg/lockdown" + "github.com/github/github-mcp-server/pkg/observability" + "github.com/github/github-mcp-server/pkg/observability/metrics" "github.com/github/github-mcp-server/pkg/raw" "github.com/github/github-mcp-server/pkg/translations" gogithub "github.com/google/go-github/v82/github" + "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -29,6 +33,7 @@ type stubDeps struct { t translations.TranslationHelperFunc flags FeatureFlags contentWindowSize int + obsv observability.Exporters } func (s stubDeps) GetClient(ctx context.Context) (*gogithub.Client, error) { @@ -59,8 +64,21 @@ func (s stubDeps) GetT() translations.TranslationHelperFunc { return s. func (s stubDeps) GetFlags(_ context.Context) FeatureFlags { return s.flags } func (s stubDeps) GetContentWindowSize() int { return s.contentWindowSize } func (s stubDeps) IsFeatureEnabled(_ context.Context, _ string) bool { return false } +func (s stubDeps) Logger(_ context.Context) *slog.Logger { + return s.obsv.Logger() +} +func (s stubDeps) Metrics(ctx context.Context) metrics.Metrics { + return s.obsv.Metrics(ctx) +} // Helper functions to create stub client functions for error testing + +// stubExporters returns a discard-logger + noop-metrics Exporters for tests. +func stubExporters() observability.Exporters { + obs, _ := observability.NewExporters(slog.New(slog.DiscardHandler), metrics.NewNoopMetrics()) + return obs +} + func stubClientFnFromHTTP(httpClient *http.Client) func(context.Context) (*gogithub.Client, error) { return func(_ context.Context) (*gogithub.Client, error) { return gogithub.NewClient(httpClient), nil @@ -124,7 +142,7 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) { InsidersMode: false, } - deps := stubDeps{} + deps := stubDeps{obsv: stubExporters()} // Build inventory inv, err := NewInventory(cfg.Translator). @@ -150,6 +168,93 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) { // is already tested in pkg/github/*_test.go. } +// TestNewServer_NameAndTitleViaTranslation verifies that server name and title +// can be overridden via the translation helper (GITHUB_MCP_SERVER_NAME / +// GITHUB_MCP_SERVER_TITLE env vars or github-mcp-server-config.json) and +// fall back to sensible defaults when not overridden. +func TestNewServer_NameAndTitleViaTranslation(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + translator translations.TranslationHelperFunc + expectedName string + expectedTitle string + }{ + { + name: "defaults when using NullTranslationHelper", + translator: translations.NullTranslationHelper, + expectedName: "github-mcp-server", + expectedTitle: "GitHub MCP Server", + }, + { + name: "custom name and title via translator", + translator: func(key, defaultValue string) string { + switch key { + case "SERVER_NAME": + return "my-github-server" + case "SERVER_TITLE": + return "My GitHub MCP Server" + default: + return defaultValue + } + }, + expectedName: "my-github-server", + expectedTitle: "My GitHub MCP Server", + }, + { + name: "custom name only via translator", + translator: func(key, defaultValue string) string { + if key == "SERVER_NAME" { + return "ghes-server" + } + return defaultValue + }, + expectedName: "ghes-server", + expectedTitle: "GitHub MCP Server", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + srv := NewServer("v1.0.0", tt.translator("SERVER_NAME", "github-mcp-server"), tt.translator("SERVER_TITLE", "GitHub MCP Server"), nil) + require.NotNil(t, srv) + + // Connect a client to retrieve the initialize result and verify ServerInfo. + st, ct := mcp.NewInMemoryTransports() + client := mcp.NewClient(&mcp.Implementation{Name: "test-client"}, nil) + + type clientResult struct { + result *mcp.InitializeResult + err error + } + clientResultCh := make(chan clientResult, 1) + go func() { + cs, err := client.Connect(context.Background(), ct, nil) + if err != nil { + clientResultCh <- clientResult{err: err} + return + } + t.Cleanup(func() { _ = cs.Close() }) + clientResultCh <- clientResult{result: cs.InitializeResult()} + }() + + ss, err := srv.Connect(context.Background(), st, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = ss.Close() }) + + got := <-clientResultCh + require.NoError(t, got.err) + require.NotNil(t, got.result) + require.NotNil(t, got.result.ServerInfo) + assert.Equal(t, tt.expectedName, got.result.ServerInfo.Name) + assert.Equal(t, tt.expectedTitle, got.result.ServerInfo.Title) + }) + } +} + // TestResolveEnabledToolsets verifies the toolset resolution logic. func TestResolveEnabledToolsets(t *testing.T) { t.Parallel() diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 3f1c291a7d..e5e9502800 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -141,6 +141,11 @@ var ( Icon: "copilot", } + // Feature flag names for granular tool variants. + // When active, consolidated tools are replaced by single-purpose granular tools. + FeatureFlagIssuesGranular = "issues_granular" + FeatureFlagPullRequestsGranular = "pull_requests_granular" + // Remote-only toolsets - these are only available in the remote MCP server // but are documented here for consistency and to enable automated documentation. ToolsetMetadataCopilotSpaces = inventory.ToolsetMetadata{ @@ -274,6 +279,32 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { GetLabelForLabelsToolset(t), ListLabels(t), LabelWrite(t), + + // Granular issue tools (feature-flagged, replace consolidated issue_write/sub_issue_write) + GranularCreateIssue(t), + GranularUpdateIssueTitle(t), + GranularUpdateIssueBody(t), + GranularUpdateIssueAssignees(t), + GranularUpdateIssueLabels(t), + GranularUpdateIssueMilestone(t), + GranularUpdateIssueType(t), + GranularUpdateIssueState(t), + GranularAddSubIssue(t), + GranularRemoveSubIssue(t), + GranularReprioritizeSubIssue(t), + + // Granular pull request tools (feature-flagged, replace consolidated update_pull_request/pull_request_review_write) + GranularUpdatePullRequestTitle(t), + GranularUpdatePullRequestBody(t), + GranularUpdatePullRequestState(t), + GranularUpdatePullRequestDraftState(t), + GranularRequestPullRequestReviewers(t), + GranularCreatePullRequestReview(t), + GranularSubmitPendingPullRequestReview(t), + GranularDeletePendingPullRequestReview(t), + GranularAddPullRequestReviewComment(t), + GranularResolveReviewThread(t), + GranularUnresolveReviewThread(t), } } diff --git a/pkg/http/handler.go b/pkg/http/handler.go index 2e828211d1..37906a03e6 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -236,13 +236,56 @@ func DefaultGitHubMCPServerFactory(r *http.Request, deps github.ToolDependencies return github.NewMCPServer(r.Context(), cfg, deps, inventory) } -// DefaultInventoryFactory creates the default inventory factory for HTTP mode -func DefaultInventoryFactory(_ *ServerConfig, t translations.TranslationHelperFunc, featureChecker inventory.FeatureFlagChecker, scopeFetcher scopes.FetcherInterface) InventoryFactoryFunc { +// DefaultInventoryFactory creates the default inventory factory for HTTP mode. +// When the ServerConfig includes static flags (--toolsets, --read-only, etc.), +// a static inventory is built once at factory creation to pre-filter the tool +// universe. Per-request headers can only narrow within these bounds. +func DefaultInventoryFactory(cfg *ServerConfig, t translations.TranslationHelperFunc, featureChecker inventory.FeatureFlagChecker, scopeFetcher scopes.FetcherInterface) InventoryFactoryFunc { + // Build the static tool/resource/prompt universe from CLI flags. + // This is done once at startup and captured in the closure. + staticTools, staticResources, staticPrompts := buildStaticInventory(cfg, t, featureChecker) + hasStaticFilters := hasStaticConfig(cfg) + + // Pre-compute valid tool names for filtering per-request tool headers. + // When a request asks for a tool by name that's been excluded from the + // static universe, we silently drop it rather than returning an error. + validToolNames := make(map[string]bool, len(staticTools)) + for i := range staticTools { + validToolNames[staticTools[i].Tool.Name] = true + } + return func(r *http.Request) (*inventory.Inventory, error) { - b := github.NewInventory(t). + b := inventory.NewBuilder(). + SetTools(staticTools). + SetResources(staticResources). + SetPrompts(staticPrompts). WithDeprecatedAliases(github.DeprecatedToolAliases). WithFeatureChecker(featureChecker) + // When static flags constrain the universe, default to showing + // everything within those bounds (per-request filters narrow further). + // When no static flags are set, preserve existing behavior where + // the default toolsets apply. + if hasStaticFilters { + b = b.WithToolsets([]string{"all"}) + } + + // Static read-only is an upper bound — enforce before request filters + if cfg.ReadOnly { + b = b.WithReadOnly(true) + } + + // Static insiders mode — enforce before request filters + if cfg.InsidersMode { + b = b.WithInsidersMode(true) + } + + // Filter request tool names to only those in the static universe, + // so requests for statically-excluded tools degrade gracefully. + if hasStaticFilters { + r = filterRequestTools(r, validToolNames) + } + b = InventoryFiltersForRequest(r, b) b = PATScopeFilter(b, r, scopeFetcher) @@ -252,6 +295,69 @@ func DefaultInventoryFactory(_ *ServerConfig, t translations.TranslationHelperFu } } +// filterRequestTools returns a shallow copy of the request with any per-request +// tool names (from X-MCP-Tools header) filtered to only include tools that exist +// in validNames. This ensures requests for statically-excluded tools are silently +// ignored rather than causing build errors. +func filterRequestTools(r *http.Request, validNames map[string]bool) *http.Request { + reqTools := ghcontext.GetTools(r.Context()) + if len(reqTools) == 0 { + return r + } + + filtered := make([]string, 0, len(reqTools)) + for _, name := range reqTools { + if validNames[name] { + filtered = append(filtered, name) + } + } + ctx := ghcontext.WithTools(r.Context(), filtered) + return r.WithContext(ctx) +} + +// hasStaticConfig returns true if any static filtering flags are set on the ServerConfig. +func hasStaticConfig(cfg *ServerConfig) bool { + return cfg.ReadOnly || + cfg.EnabledToolsets != nil || + cfg.EnabledTools != nil || + cfg.DynamicToolsets || + len(cfg.ExcludeTools) > 0 || + cfg.InsidersMode +} + +// buildStaticInventory pre-filters the full tool/resource/prompt universe using +// the static CLI flags (--toolsets, --read-only, --exclude-tools, etc.). +// The returned slices serve as the upper bound for per-request inventory builders. +func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFunc, featureChecker inventory.FeatureFlagChecker) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt) { + if !hasStaticConfig(cfg) { + return github.AllTools(t), github.AllResources(t), github.AllPrompts(t) + } + + b := github.NewInventory(t). + WithFeatureChecker(featureChecker). + WithReadOnly(cfg.ReadOnly). + WithToolsets(github.ResolvedEnabledToolsets(cfg.DynamicToolsets, cfg.EnabledToolsets, cfg.EnabledTools)). + WithInsidersMode(cfg.InsidersMode) + + if len(cfg.EnabledTools) > 0 { + b = b.WithTools(github.CleanTools(cfg.EnabledTools)) + } + + if len(cfg.ExcludeTools) > 0 { + b = b.WithExcludeTools(cfg.ExcludeTools) + } + + inv, err := b.Build() + if err != nil { + // Fall back to all tools if there's an error (e.g. unknown tool names). + // The error will surface again at per-request time if relevant. + return github.AllTools(t), github.AllResources(t), github.AllPrompts(t) + } + + ctx := context.Background() + return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx) +} + // InventoryFiltersForRequest applies filters to the inventory builder // based on the request context and headers func InventoryFiltersForRequest(r *http.Request, builder *inventory.Builder) *inventory.Builder { diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index 2a19e0a231..ee465c174e 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -23,6 +23,10 @@ import ( ) func mockTool(name, toolsetID string, readOnly bool) inventory.ServerTool { + return mockToolFull(name, toolsetID, readOnly, false) +} + +func mockToolFull(name, toolsetID string, readOnly bool, isDefault bool) inventory.ServerTool { return inventory.ServerTool{ Tool: mcp.Tool{ Name: name, @@ -31,6 +35,7 @@ func mockTool(name, toolsetID string, readOnly bool) inventory.ServerTool { Toolset: inventory.ToolsetMetadata{ ID: inventory.ToolsetID(toolsetID), Description: "Test: " + toolsetID, + Default: isDefault, }, } } @@ -409,3 +414,253 @@ func TestHTTPHandlerRoutes(t *testing.T) { }) } } + +func TestStaticConfigEnforcement(t *testing.T) { + // Use default toolsets to match real-world behavior where repos/issues/pull_requests are defaults + tools := []inventory.ServerTool{ + mockToolFull("get_file_contents", "repos", true, true), + mockToolFull("create_repository", "repos", false, true), + mockToolFull("list_issues", "issues", true, true), + mockToolFull("create_issue", "issues", false, true), + mockToolFull("list_pull_requests", "pull_requests", true, true), + mockToolFull("create_pull_request", "pull_requests", false, true), + mockToolWithFeatureFlag("hidden_by_holdback", "repos", true, "", "mcp_holdback_consolidated_projects"), + } + + tests := []struct { + name string + config *ServerConfig + path string + headers map[string]string + expectedTools []string + }{ + { + name: "no static config preserves existing behavior", + config: &ServerConfig{Version: "test"}, + path: "/", + expectedTools: []string{"get_file_contents", "create_repository", "list_issues", "create_issue", "list_pull_requests", "create_pull_request", "hidden_by_holdback"}, + }, + { + name: "static read-only filters write tools", + config: &ServerConfig{Version: "test", ReadOnly: true}, + path: "/", + expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests", "hidden_by_holdback"}, + }, + { + name: "static read-only cannot be overridden by header", + config: &ServerConfig{Version: "test", ReadOnly: true}, + path: "/", + headers: map[string]string{ + headers.MCPReadOnlyHeader: "false", + }, + expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests", "hidden_by_holdback"}, + }, + { + name: "static toolsets restricts available tools", + config: &ServerConfig{Version: "test", EnabledToolsets: []string{"repos"}}, + path: "/", + expectedTools: []string{"get_file_contents", "create_repository", "hidden_by_holdback"}, + }, + { + name: "static toolsets cannot be expanded by header", + config: &ServerConfig{Version: "test", EnabledToolsets: []string{"repos"}}, + path: "/", + headers: map[string]string{ + headers.MCPToolsetsHeader: "issues", + }, + // Header asks for "issues" but only "repos" tools exist in the static universe + expectedTools: []string{}, + }, + { + name: "per-request header can narrow within static toolset bounds", + config: &ServerConfig{Version: "test", EnabledToolsets: []string{"repos", "issues"}}, + path: "/", + headers: map[string]string{ + headers.MCPToolsetsHeader: "repos", + }, + expectedTools: []string{"get_file_contents", "create_repository", "hidden_by_holdback"}, + }, + { + name: "static exclude-tools removes tools", + config: &ServerConfig{Version: "test", ExcludeTools: []string{"create_repository", "create_issue"}}, + path: "/", + expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests", "create_pull_request", "hidden_by_holdback"}, + }, + { + name: "static exclude-tools cannot be re-included by header", + config: &ServerConfig{Version: "test", ExcludeTools: []string{"create_repository"}}, + path: "/", + headers: map[string]string{ + headers.MCPToolsHeader: "create_repository,list_issues", + }, + // create_repository was excluded at static level, only list_issues available + expectedTools: []string{"list_issues"}, + }, + { + name: "static read-only combined with per-request toolset", + config: &ServerConfig{Version: "test", ReadOnly: true}, + path: "/", + headers: map[string]string{ + headers.MCPToolsetsHeader: "repos", + }, + expectedTools: []string{"get_file_contents", "hidden_by_holdback"}, + }, + { + name: "static toolset with URL readonly", + config: &ServerConfig{Version: "test", EnabledToolsets: []string{"repos", "issues"}}, + path: "/readonly", + expectedTools: []string{"get_file_contents", "list_issues", "hidden_by_holdback"}, + }, + { + name: "static tools enables specific tools only", + config: &ServerConfig{Version: "test", EnabledTools: []string{"list_issues", "get_file_contents"}}, + path: "/", + expectedTools: []string{"list_issues", "get_file_contents"}, + }, + { + name: "static tools cannot be expanded by header", + config: &ServerConfig{Version: "test", EnabledTools: []string{"list_issues"}}, + path: "/", + headers: map[string]string{ + headers.MCPToolsHeader: "create_repository", + }, + // create_repository isn't in the static universe so it's silently dropped; + // the empty filter shows all tools within static bounds + expectedTools: []string{"list_issues"}, + }, + { + name: "static exclude-tools combined with per-request exclude", + config: &ServerConfig{Version: "test", ExcludeTools: []string{"create_repository"}}, + path: "/", + headers: map[string]string{ + headers.MCPExcludeToolsHeader: "create_issue", + }, + // Both static and per-request exclusions apply + expectedTools: []string{"get_file_contents", "list_issues", "list_pull_requests", "create_pull_request", "hidden_by_holdback"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var capturedInventory *inventory.Inventory + var capturedCtx context.Context + + featureChecker := func(ctx context.Context, flag string) (bool, error) { + return slices.Contains(ghcontext.GetHeaderFeatures(ctx), flag), nil + } + + apiHost, err := utils.NewAPIHost("https://api.github.com") + require.NoError(t, err) + + // Build static tools the same way the production code does + staticTools, staticResources, staticPrompts := buildStaticInventoryFromTools(tt.config, tools, featureChecker) + hasStatic := hasStaticConfig(tt.config) + + validToolNames := make(map[string]bool, len(staticTools)) + for _, tool := range staticTools { + validToolNames[tool.Tool.Name] = true + } + + inventoryFactory := func(r *http.Request) (*inventory.Inventory, error) { + capturedCtx = r.Context() + builder := inventory.NewBuilder(). + SetTools(staticTools). + SetResources(staticResources). + SetPrompts(staticPrompts). + WithDeprecatedAliases(github.DeprecatedToolAliases). + WithFeatureChecker(featureChecker) + + if hasStatic { + builder = builder.WithToolsets([]string{"all"}) + } + if tt.config.ReadOnly { + builder = builder.WithReadOnly(true) + } + if tt.config.InsidersMode { + builder = builder.WithInsidersMode(true) + } + + if hasStatic { + r = filterRequestTools(r, validToolNames) + } + + builder = InventoryFiltersForRequest(r, builder) + inv, buildErr := builder.Build() + if buildErr != nil { + return nil, buildErr + } + capturedInventory = inv + return inv, nil + } + + mcpServerFactory := func(_ *http.Request, _ github.ToolDependencies, _ *inventory.Inventory, _ *github.MCPServerConfig) (*mcp.Server, error) { + return mcp.NewServer(&mcp.Implementation{Name: "test", Version: "0.0.1"}, nil), nil + } + + handler := NewHTTPMcpHandler( + context.Background(), + tt.config, + nil, + translations.NullTranslationHelper, + slog.Default(), + apiHost, + WithInventoryFactory(inventoryFactory), + WithGitHubMCPServerFactory(mcpServerFactory), + WithScopeFetcher(allScopesFetcher{}), + ) + + r := chi.NewRouter() + handler.RegisterMiddleware(r) + handler.RegisterRoutes(r) + + req := httptest.NewRequest(http.MethodPost, tt.path, nil) + req.Header.Set(headers.AuthorizationHeader, "Bearer ghp_testtoken") + for k, v := range tt.headers { + req.Header.Set(k, v) + } + + rr := httptest.NewRecorder() + r.ServeHTTP(rr, req) + + require.NotNil(t, capturedInventory, "inventory should have been created") + + toolNames := extractToolNames(capturedCtx, capturedInventory) + expectedSorted := make([]string, len(tt.expectedTools)) + copy(expectedSorted, tt.expectedTools) + sort.Strings(expectedSorted) + + assert.Equal(t, expectedSorted, toolNames, "tools should match expected") + }) + } +} + +// buildStaticInventoryFromTools is a test helper that mirrors buildStaticInventory +// but uses the provided mock tools instead of calling github.AllTools. +func buildStaticInventoryFromTools(cfg *ServerConfig, tools []inventory.ServerTool, featureChecker inventory.FeatureFlagChecker) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt) { + if !hasStaticConfig(cfg) { + return tools, nil, nil + } + + b := inventory.NewBuilder(). + SetTools(tools). + WithFeatureChecker(featureChecker). + WithReadOnly(cfg.ReadOnly). + WithToolsets(github.ResolvedEnabledToolsets(cfg.DynamicToolsets, cfg.EnabledToolsets, cfg.EnabledTools)). + WithInsidersMode(cfg.InsidersMode) + + if len(cfg.EnabledTools) > 0 { + b = b.WithTools(github.CleanTools(cfg.EnabledTools)) + } + + if len(cfg.ExcludeTools) > 0 { + b = b.WithExcludeTools(cfg.ExcludeTools) + } + + inv, err := b.Build() + if err != nil { + return tools, nil, nil + } + + ctx := context.Background() + return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx) +} diff --git a/pkg/http/server.go b/pkg/http/server.go index 8723039408..38ea0de301 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -17,6 +17,8 @@ import ( "github.com/github/github-mcp-server/pkg/http/oauth" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/lockdown" + "github.com/github/github-mcp-server/pkg/observability" + "github.com/github/github-mcp-server/pkg/observability/metrics" "github.com/github/github-mcp-server/pkg/scopes" "github.com/github/github-mcp-server/pkg/translations" "github.com/github/github-mcp-server/pkg/utils" @@ -67,6 +69,28 @@ type ServerConfig struct { // ScopeChallenge indicates if we should return OAuth scope challenges, and if we should perform // tool filtering based on token scopes. ScopeChallenge bool + + // ReadOnly indicates if we should only register read-only tools. + // When set via CLI flag, this acts as an upper bound — per-request headers + // cannot re-enable write tools. + ReadOnly bool + + // EnabledToolsets is a list of toolsets to enable. + // When set via CLI flag, per-request headers can only narrow within these toolsets. + EnabledToolsets []string + + // EnabledTools is a list of specific tools to enable (additive to toolsets). + EnabledTools []string + + // DynamicToolsets enables dynamic toolset discovery mode. + DynamicToolsets bool + + // ExcludeTools is a list of tool names to disable regardless of other settings. + // When set via CLI flag, per-request headers cannot re-include these tools. + ExcludeTools []string + + // InsidersMode indicates if we should enable experimental features. + InsidersMode bool } func RunHTTPServer(cfg ServerConfig) error { @@ -90,7 +114,7 @@ func RunHTTPServer(cfg ServerConfig) error { slogHandler = slog.NewTextHandler(logOutput, &slog.HandlerOptions{Level: slog.LevelInfo}) } logger := slog.New(slogHandler) - logger.Info("starting server", "version", cfg.Version, "host", cfg.Host, "lockdownEnabled", cfg.LockdownMode) + logger.Info("starting server", "version", cfg.Version, "host", cfg.Host, "lockdownEnabled", cfg.LockdownMode, "readOnly", cfg.ReadOnly, "insidersMode", cfg.InsidersMode) apiHost, err := utils.NewAPIHost(cfg.Host) if err != nil { @@ -106,6 +130,11 @@ func RunHTTPServer(cfg ServerConfig) error { featureChecker := createHTTPFeatureChecker() + obs, err := observability.NewExporters(logger, metrics.NewNoopMetrics()) + if err != nil { + return fmt.Errorf("failed to create observability exporters: %w", err) + } + deps := github.NewRequestDeps( apiHost, cfg.Version, @@ -114,6 +143,7 @@ func RunHTTPServer(cfg ServerConfig) error { t, cfg.ContentWindowSize, featureChecker, + obs, ) // Initialize the global tool scope map diff --git a/pkg/observability/metrics/metrics.go b/pkg/observability/metrics/metrics.go new file mode 100644 index 0000000000..5e861b3e05 --- /dev/null +++ b/pkg/observability/metrics/metrics.go @@ -0,0 +1,13 @@ +package metrics + +import "time" + +// Metrics is a backend-agnostic interface for emitting metrics. +// Implementations can route to DataDog, log to slog, or discard (noop). +type Metrics interface { + Increment(key string, tags map[string]string) + Counter(key string, tags map[string]string, value int64) + Distribution(key string, tags map[string]string, value float64) + DistributionMs(key string, tags map[string]string, value time.Duration) + WithTags(tags map[string]string) Metrics +} diff --git a/pkg/observability/metrics/noop_sink.go b/pkg/observability/metrics/noop_sink.go new file mode 100644 index 0000000000..4ce9e337d8 --- /dev/null +++ b/pkg/observability/metrics/noop_sink.go @@ -0,0 +1,19 @@ +package metrics + +import "time" + +// NoopMetrics is a no-op implementation of the Metrics interface. +type NoopMetrics struct{} + +var _ Metrics = (*NoopMetrics)(nil) + +// NewNoopMetrics returns a new NoopMetrics. +func NewNoopMetrics() *NoopMetrics { + return &NoopMetrics{} +} + +func (n *NoopMetrics) Increment(_ string, _ map[string]string) {} +func (n *NoopMetrics) Counter(_ string, _ map[string]string, _ int64) {} +func (n *NoopMetrics) Distribution(_ string, _ map[string]string, _ float64) {} +func (n *NoopMetrics) DistributionMs(_ string, _ map[string]string, _ time.Duration) {} +func (n *NoopMetrics) WithTags(_ map[string]string) Metrics { return n } diff --git a/pkg/observability/metrics/noop_sink_test.go b/pkg/observability/metrics/noop_sink_test.go new file mode 100644 index 0000000000..21d3dccd6c --- /dev/null +++ b/pkg/observability/metrics/noop_sink_test.go @@ -0,0 +1,42 @@ +package metrics + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestNoopMetrics_ImplementsInterface(_ *testing.T) { + var _ Metrics = (*NoopMetrics)(nil) +} + +func TestNoopMetrics_NoPanics(t *testing.T) { + m := NewNoopMetrics() + + assert.NotPanics(t, func() { + m.Increment("key", map[string]string{"a": "b"}) + m.Counter("key", map[string]string{"a": "b"}, 1) + m.Distribution("key", map[string]string{"a": "b"}, 1.5) + m.DistributionMs("key", map[string]string{"a": "b"}, time.Second) + }) +} + +func TestNoopMetrics_NilTags(t *testing.T) { + m := NewNoopMetrics() + + assert.NotPanics(t, func() { + m.Increment("key", nil) + m.Counter("key", nil, 1) + m.Distribution("key", nil, 1.5) + m.DistributionMs("key", nil, time.Second) + }) +} + +func TestNoopMetrics_WithTags(t *testing.T) { + m := NewNoopMetrics() + tagged := m.WithTags(map[string]string{"env": "prod"}) + + assert.NotNil(t, tagged) + assert.Equal(t, m, tagged) +} diff --git a/pkg/observability/observability.go b/pkg/observability/observability.go new file mode 100644 index 0000000000..3741b05c75 --- /dev/null +++ b/pkg/observability/observability.go @@ -0,0 +1,46 @@ +package observability + +import ( + "context" + "errors" + "log/slog" + + "github.com/github/github-mcp-server/pkg/observability/metrics" +) + +// Exporters bundles observability primitives (logger + metrics) for dependency injection. +// The logger is Go's stdlib *slog.Logger — integrators provide their own slog.Handler. +type Exporters interface { + Logger() *slog.Logger + Metrics(context.Context) metrics.Metrics +} + +type exporters struct { + logger *slog.Logger + metrics metrics.Metrics +} + +// NewExporters creates an Exporters bundle. Pass a configured *slog.Logger +// (with whatever slog.Handler you need) and a Metrics implementation. +// Neither may be nil; use slog.New(slog.DiscardHandler) and metrics.NewNoopMetrics() +// if logging or metrics are unwanted. +func NewExporters(logger *slog.Logger, m metrics.Metrics) (Exporters, error) { + if logger == nil { + return nil, errors.New("logger must not be nil: use slog.New(slog.DiscardHandler) to discard logs") + } + if m == nil { + return nil, errors.New("metrics must not be nil: use metrics.NewNoopMetrics() to discard metrics") + } + return &exporters{ + logger: logger, + metrics: m, + }, nil +} + +func (e *exporters) Logger() *slog.Logger { + return e.logger +} + +func (e *exporters) Metrics(_ context.Context) metrics.Metrics { + return e.metrics +} diff --git a/pkg/observability/observability_test.go b/pkg/observability/observability_test.go new file mode 100644 index 0000000000..c8949fdbd4 --- /dev/null +++ b/pkg/observability/observability_test.go @@ -0,0 +1,46 @@ +package observability + +import ( + "context" + "log/slog" + "testing" + + "github.com/github/github-mcp-server/pkg/observability/metrics" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewExporters(t *testing.T) { + logger := slog.Default() + m := metrics.NewNoopMetrics() + exp, err := NewExporters(logger, m) + ctx := context.Background() + + require.NoError(t, err) + assert.NotNil(t, exp) + assert.Equal(t, logger, exp.Logger()) + assert.Equal(t, m, exp.Metrics(ctx)) +} + +func TestNewExporters_WithNilLogger(t *testing.T) { + _, err := NewExporters(nil, metrics.NewNoopMetrics()) + require.Error(t, err) + assert.Contains(t, err.Error(), "logger must not be nil") +} + +func TestNewExporters_WithNilMetrics(t *testing.T) { + _, err := NewExporters(slog.New(slog.DiscardHandler), nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "metrics must not be nil") +} + +func TestNewExporters_WithDiscardLogger(t *testing.T) { + logger := slog.New(slog.DiscardHandler) + m := metrics.NewNoopMetrics() + exp, err := NewExporters(logger, m) + + require.NoError(t, err) + assert.NotNil(t, exp) + assert.Equal(t, logger, exp.Logger()) + assert.Equal(t, m, exp.Metrics(context.Background())) +} diff --git a/ui/src/components/AppProvider.tsx b/ui/src/components/AppProvider.tsx index 7848c38197..18e81c5b03 100644 --- a/ui/src/components/AppProvider.tsx +++ b/ui/src/components/AppProvider.tsx @@ -1,6 +1,7 @@ import { ThemeProvider, BaseStyles, Box } from "@primer/react"; import type { ReactNode } from "react"; import { useEffect } from "react"; +import { FeedbackFooter } from "./FeedbackFooter"; interface AppProviderProps { children: ReactNode; @@ -19,7 +20,10 @@ export function AppProvider({ children }: AppProviderProps) { return ( - {children} + + {children} + + ); diff --git a/ui/src/components/FeedbackFooter.tsx b/ui/src/components/FeedbackFooter.tsx new file mode 100644 index 0000000000..10fbdf44e6 --- /dev/null +++ b/ui/src/components/FeedbackFooter.tsx @@ -0,0 +1,17 @@ +import { Box, Text } from "@primer/react"; + +export function FeedbackFooter() { + return ( + + + Help us improve MCP Apps support in the GitHub MCP Server +
+ github.com/github/github-mcp-server/issues/new?template=insiders-feedback.md +
+
+ ); +}