Movatterモバイル変換

internal/githubv4mock/githubv4mock.go

		@@ -0,0 +1,235 @@
		// githubv4mock package provides a mock GraphQL server used for testing queries produced via

Copy link

CollaboratorAuthor

williammartinMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This whole package should have its own tests but I want to get feedback on it before the extra effort.

williammartin commented

pkg/github/pullrequests_test.go

		w.Header().Set("Content-Type", "application/json")
		_, _ = w.Write([]byte(`{"message": "Validation Failed"}`))
		}),
		name: "failure to get pull request",

Copy link

CollaboratorAuthor

williammartinMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I didn't do these GQL error cases in all the other tests intentionally yet, I just wanted to get a feel for whether it made sense. Every handler follows the same pattern onif err := ... ; err != nil

williammartin commented

pkg/github/server_test.go

		@@ -157,7 +164,7 @@ func Test_OptionalStringParam(t *testing.T) {
		}
		}

		funcTest_RequiredNumberParam(t *testing.T) {
		funcTest_RequiredInt(t *testing.T) {

Copy link

CollaboratorAuthor

williammartinMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

These names just didn't match the actual functions.

williammartin commented

e2e/e2e_test.go Outdated

Copy link

CollaboratorAuthor

williammartinMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm starting to feel the tension in the repetition in this file now, which is a good thing because it means we probably have sufficient examples to start teasing out shared abstractions, asserting the MCP result isn't an error and has text content etc etc.

Would prefer not to address in this PR since it's about behaviour rather than maintenance.

williammartin commented

internal/ghmcp/server.go

		// Construct our REST client
		restClient := gogithub.NewClient(nil).WithAuthToken(cfg.Token)
		restClient.UserAgent = fmt.Sprintf("github-mcp-server/%s", cfg.Version)
		restClient.BaseURL = apiHost.baseRESTURL

Copy link

CollaboratorAuthor

williammartinMay 19, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I chose to go away fromWithEnterpriseURLS because there are hosts that are not enterprise but require different URLs e.g. review labs or development environments.

Sadly,githubv4 doesn't offer quite the same options, so I commented below why that is. It also doesn't export the internalgraphql.Client so we can't just set it.

Copy link

Collaborator

SamMorrowDrumsMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

That's a shame! Might need to propose exporting it.

williammartin commented

internal/ghmcp/server.go Outdated

		return apiHost{}, fmt.Errorf("could not parse host as URL: %s", s)
		}

		if url.Scheme == "" {

Copy link

CollaboratorAuthor

williammartinMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'd be happy to loosen this later, but it is currently the expectation of the REST client.

williammartin force-pushed the343-remove-comments-from-create_pull_request_review branch from3d58333 to249064bCompare

May 19, 2025 10:53

williammartin commented

pkg/github/helper_test.goShow resolvedHide resolved

williammartin force-pushed the343-remove-comments-from-create_pull_request_review branch from249064b to1c1082eCompare

May 19, 2025 11:09

williammartin commented

		@@ -75,8 +77,123 @@ func GetPullRequest(getClient GetClientFn, t translations.TranslationHelperFunc)
		}
		}

		// CreatePullRequest creates a tool to create a new pull request.
		func CreatePullRequest(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) {

Copy link

CollaboratorAuthor

williammartinMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This was just moved to a more natural place in the file.

williammartin force-pushed the343-remove-comments-from-create_pull_request_review branch from1c1082e to6273564Compare

May 19, 2025 11:15

williammartin commented

Comment on lines +1140 to +1151

		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)"`

Copy link

CollaboratorAuthor

williammartinMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could we DRY this out?

Copy link

CollaboratorAuthor

williammartinMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We could because a number of tools share it, but I'm keen to let it sit and soak in for a bit before choosing the wrong abstraction.

Copy link

CollaboratorAuthor

williammartinMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Author note: this came from GPT-4.1

williammartin marked this pull request as ready for review

May 19, 2025 11:22

williammartin requested a review froma team as acode owner

May 19, 2025 11:22

williammartin commented

Comment on lines +1140 to +1151

		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)"`

Copy link

CollaboratorAuthor

williammartinMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could we DRY this out?

Copy link

CollaboratorAuthor

williammartinMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Author note: this came from Gemini 2.5

williammartin requested a review fromCopilot

May 19, 2025 11:27

CopilotAI reviewed

Copy link

Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Pull Request Overview

This PR refactors the PR review workflow by splitting existing REST-based review/comment tools into finer-grained GraphQL-backed operations, adds diff retrieval support, and introduces host-specific URL parsing for GitHub API endpoints.

Extracts separate GraphQL tools: create pending reviews, add comments to pending reviews, submit/delete pending reviews, and atomic create-and-submit.
Addsget_pull_request_diff and end-to-end mocks, including a local GraphQL mock server for testing.
IntroducesparseAPIHost and custom transports to support dotcom, GHEC, and GHES URL patterns.

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
third-party/github.com/shurcooL/graphql/LICENSE	Added MIT license file for the shurcooL/graphql dependency
third-party/github.com/shurcooL/githubv4/LICENSE	Added MIT license file for the shurcooL/githubv4 dependency
third-party-licenses.*.md	Registered new GraphQL libraries in platform-specific license docs
pkg/github/tools.go	Updated`InitToolsets` signature to accept a GraphQL client and registered new review tools
pkg/github/server_test.go	Added`stubGetGQLClientFn`; renamed tests for integer params
pkg/github/helper_test.go	Enhanced`mockResponse` to handle raw string bodies
internal/githubv4mock/*	Introduced a local GraphQL mock server, query builder, and equality checks for tests
internal/ghmcp/server.go	Added`parseAPIHost`, custom HTTP transports, and GraphQL client setup
go.mod	Bumped and added`githubv4`,`graphql`, and`oauth2` dependencies

Comments suppressed due to low confidence (5)

pkg/github/server_test.go:167

[nitpick] The test nameTest_RequiredInt is inconsistent withTest_OptionalIntParam. Consider renaming toTest_RequiredIntParam to match the naming convention.

func Test_RequiredInt(t *testing.T) {

internal/githubv4mock/query.go:1

Typo in comment:Ths should beThe.

// Ths contents of this file are taken from https://github.com/shurcooL/graphql/blob/ed46e5a4646634fc16cb07c3b8db389542cc8847/query.go

internal/ghmcp/server.go:335

There are no tests coveringparseAPIHost. Adding unit tests for dotcom, GHEC, and GHES cases would help ensure correctness.

func parseAPIHost(s string) (apiHost, error) {

third-party-licenses.windows.md:20

The indentation of this new list item does not align with existing entries (two spaces before the dash). Please adjust to match the surrounding style.

- [github.com/shurcooL/githubv4](https://pkg.go.dev/github.com/shurcooL/githubv4) ([MIT](https://github.com/shurcooL/githubv4/blob/48295856cce7/LICENSE))

third-party-licenses.linux.md:20

This list item’s indentation differs from the rest of the file; align the dash under the others (two spaces) for consistency.

- [github.com/shurcooL/graphql](https://pkg.go.dev/github.com/shurcooL/graphql) ([MIT](https://github.com/shurcooL/graphql/blob/ed46e5a46466/LICENSE))

internal/githubv4mock/githubv4mock.go OutdatedShow resolvedHide resolved

internal/ghmcp/server.go OutdatedShow resolvedHide resolved

williammartin force-pushed the343-remove-comments-from-create_pull_request_review branch from6273564 to1089f84Compare

May 19, 2025 11:30

williammartin requested review fromSamMorrowDrums andtoby

May 19, 2025 12:05

SamMorrowDrums approved these changes

internal/githubv4mock/query.go

Copy link

Collaborator

SamMorrowDrums left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Amazing work, I think without the graphql parts it's not too big, and those parts are going to be shared by other APIs, so I say shipit!

I submitted this using it.

Comment on lines +1 to +3

		// Ths contents of this file are taken from https://github.com/shurcooL/graphql/blob/ed46e5a4646634fc16cb07c3b8db389542cc8847/query.go
		// because they are not exported by the module, and we would like to use them in building the githubv4mock test utility.
		//

Copy link

Collaborator

SamMorrowDrumsMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I feel like eventually we might want a mock library to exist in standalone form, so we could pull this out, but it's OK to coexist here for now

internal/ghmcp/server.goShow resolvedHide resolved

Comment on lines +82 to +117

		return mcp.NewTool("create_pull_request",
		mcp.WithDescription(t("TOOL_CREATE_PULL_REQUEST_DESCRIPTION", "Create a new pull request in a GitHub repository.")),
		mcp.WithToolAnnotation(mcp.ToolAnnotation{
		Title: t("TOOL_CREATE_PULL_REQUEST_USER_TITLE", "Open new pull request"),
		ReadOnlyHint: toBoolPtr(false),
		}),
		mcp.WithString("owner",
		mcp.Required(),
		mcp.Description("Repository owner"),
		),
		mcp.WithString("repo",
		mcp.Required(),
		mcp.Description("Repository name"),
		),
		mcp.WithString("title",
		mcp.Required(),
		mcp.Description("PR title"),
		),
		mcp.WithString("body",
		mcp.Description("PR description"),
		),
		mcp.WithString("head",
		mcp.Required(),
		mcp.Description("Branch containing changes"),
		),
		mcp.WithString("base",
		mcp.Required(),
		mcp.Description("Branch to merge into"),
		),
		mcp.WithBoolean("draft",
		mcp.Description("Create as draft PR"),
		),
		mcp.WithBoolean("maintainer_can_modify",
		mcp.Description("Allow maintainer edits"),
		),
		),

Copy link

Collaborator

SamMorrowDrumsMay 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I hope the GO SDK makes these definitions and parsing much closer to standard JSON parsing into structs as discussed!

Split PR review creation, commenting, submission and deletion

1de146b

SamMorrowDrums force-pushed the343-remove-comments-from-create_pull_request_review branch from1089f84 to1de146bCompare

May 19, 2025 15:37

SamMorrowDrums merged commiteca853b intomain