Used typed tool handler for `get_me` tool #447

williammartin · 2025-05-28T13:11:32Z

Description

This PR is the first of many that moves handlers to use NewTypedToolHandler which is also a step towards the official Go SDK. While reworking this, I also took the opportunity to improve testing confidence.

E2E Test

➜  github-mcp-server git:(wm/get-me-typed-tool) GOMAXPROCS=1 GITHUB_MCP_SERVER_E2E_HOST=https://github.com GITHUB_MCP_SERVER_E2E_TOKEN=$(gh auth token) go test -v -run 'GetMe' -count=1 --tags e2e ./e2e

=== RUN   TestGetMe
=== PAUSE TestGetMe
=== CONT  TestGetMe
    e2e_test.go:80: Building Docker image for e2e tests...
    e2e_test.go:163: Starting Stdio MCP client...
--- PASS: TestGetMe (7.14s)
PASS
ok      github.com/github/github-mcp-server/e2e 7.410s

Copilot

Pull Request Overview

This PR standardizes tool handlers using NewTypedToolHandler for the get_me tool, boosts testing confidence with schema snapshots, and adds missing third-party licenses and entries in platform-specific license summaries.

Refactor GetMe to use a typed tool handler and centralize JSON marshalling
Introduce schema snapshot tests (toolsnaps) and enhance existing unit tests
Add license files for new third-party dependencies and update third-party-licenses.*.md

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/github/context_tools.go	Refactored `GetMe` to use `NewTypedToolHandler` and removed manual JSON handling
pkg/github/server.go	Added `marshalledTextResult` helper for consistent text results
pkg/github/context_tools_test.go	Updated `Test_GetMe` to use table-driven checks and snapshot tests
pkg/toolsets/toolsets_test.go	Renamed test for clarity and simplified assertions
docs/testing.md	Documented usage of `toolsnaps` and updated test patterns
third-party-licenses.windows.md (and linux/darwin variants)	Added entries for recently added dependencies

Comments suppressed due to low confidence (5)

pkg/github/server.go:219

[nitpick] The function name uses the British spelling marshalledTextResult. For consistency with Go's json.Marshal and typical API conventions, consider renaming to marshaledTextResult.

func marshalledTextResult(v any) *mcp.CallToolResult {

third-party-licenses.windows.md:12

List item indentation is inconsistent with the surrounding entries; align leading spaces (e.g., two spaces before the dash) so the markdown renders correctly.

- [github.com/go-openapi/jsonpointer](https://pkg.go.dev/github.com/go-openapi/jsonpointer) ([Apache-2.0](https://github.com/go-openapi/jsonpointer/blob/v0.19.5/LICENSE))

third-party-licenses.linux.md:12

List item indentation is inconsistent with the surrounding entries; align leading spaces so the markdown list formatting remains uniform.

- [github.com/go-openapi/jsonpointer](https://pkg.go.dev/github.com/go-openapi/jsonpointer) ([Apache-2.0](https://github.com/go-openapi/jsonpointer/blob/v0.19.5/LICENSE))

third-party-licenses.darwin.md:12

List item indentation is inconsistent; ensure two spaces before each dash for consistent markdown rendering across all OS-specific license files.

- [github.com/go-openapi/jsonpointer](https://pkg.go.dev/github.com/go-openapi/jsonpointer) ([Apache-2.0](https://github.com/go-openapi/jsonpointer/blob/v0.19.5/LICENSE))

docs/testing.md:29

The line committed. is not indented to match the preceding list item; add two spaces before the dash or indent so it continues the markdown bullet correctly.

committed.

pkg/toolsets/toolsets_test.go

williammartin · 2025-05-28T13:28:39Z

pkg/github/context_tools.go

-			}
+	type args struct{}
+	handler := mcp.NewTypedToolHandler(func(ctx context.Context, _ mcp.CallToolRequest, _ args) (*mcp.CallToolResult, error) {
+		client, err := getClient(ctx)


I have some ideas about having a handler function that injects:

type Clients struct { rest.. gql.. }

Then all handlers don't have this getClient boilerplate. Added a lot of types though, would rather feel a bit more pain.

SamMorrowDrums

I think you just left a comment that agrees with my comment about structs, broadly I agree with this direction, and some nits, and a general question about how much we extract our types from mcp-go types.

SamMorrowDrums · 2025-05-28T13:26:48Z

pkg/github/__toolsnaps__/get_me.snap

+    "type": "object"
+  },
+  "name": "get_me"
+}


Suggested change

}

}

It doesn't really matter, but I do love EOF newlines being added by automation rather than omitted if it's possible.

I can add it, but I don't really understand why it's useful to you? Does it help in your IDE or something?

SamMorrowDrums · 2025-05-28T13:30:01Z

pkg/github/context_tools.go

-			mcp.WithString("reason",
-				mcp.Description("Optional: the reason for requesting the user information"),
-			),
+func GetMe(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) {


Do we want to return an mcp.Tool or have our own struct that gets converted later, I suppose it's an utter pain to make versions of all their structs.

I want our own types that can be adapted for mcp-go/go-sdk but I'm not paying that price right now. I want to do the move over to the typed handlers and then iterate from there.

SamMorrowDrums · 2025-05-28T13:31:57Z

pkg/github/context_tools.go

-				return nil, fmt.Errorf("failed to marshal user: %w", err)
-			}
+	type args struct{}
+	handler := mcp.NewTypedToolHandler(func(ctx context.Context, _ mcp.CallToolRequest, _ args) (*mcp.CallToolResult, error) {


Nice to see use of the typed handler (although this tool is not much of a showcase 😂).

I was going to ask the same thing as above, do we want to use mcp-go structs here. I suppose we should for now.

A use case I can imagine is wrapping an existing handler with another one (to add something like chunking, or other aggregations like returning only the count). That sad part with the string output is that instead of Marshalling the data just-in-time, I would have to unmarshall the json again to process it in this form. 😓 this is a tough problem to solve without having to just re-write tools.

I had imagined we'd have a tool handler type that returns an interface (can be marshalled into json whatever that interface is called), and error. We would then give that a thin wrapper that binds it into mcp-go. Maybe overkill for now, just something that is coming onto my radar.

All good thoughts and as per #447 (comment) there's lots I do want to tackle but I want to do it piecemeal where we move over to having a struct for args, then we can look at the abstractions that make sense around that, rather than biting it all off at once.

pkg/github/server.go

pkg/github/context_tools.go

Co-authored-by: Sam Morrow <info@sam-morrow.com>

williammartin added 2 commits May 28, 2025 15:11

Use typed tool handler in get_me tool

40d5c7d

Fix weird golangci-lint complaint about nil checks

9797505

williammartin marked this pull request as ready for review May 28, 2025 13:19

Copilot AI review requested due to automatic review settings May 28, 2025 13:19

williammartin requested a review from a team as a code owner May 28, 2025 13:19

Copilot AI reviewed May 28, 2025

View reviewed changes

pkg/toolsets/toolsets_test.go Show resolved Hide resolved

williammartin commented May 28, 2025

View reviewed changes

SamMorrowDrums previously approved these changes May 28, 2025

View reviewed changes

Update pkg/github/server.go

af27689

Co-authored-by: Sam Morrow <info@sam-morrow.com>

williammartin dismissed SamMorrowDrums’s stale review via af27689 May 28, 2025 13:48

Update pkg/github/context_tools.go

c5ad2a0

Co-authored-by: Sam Morrow <info@sam-morrow.com>

SamMorrowDrums approved these changes May 28, 2025

View reviewed changes

SamMorrowDrums merged commit 023f59d into main May 28, 2025
16 checks passed

SamMorrowDrums deleted the wm/get-me-typed-tool branch May 28, 2025 13:56

BrewTestBot mentioned this pull request Jun 12, 2025

github-mcp-server 0.5.0 Homebrew/homebrew-core#226745

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Used typed tool handler for `get_me` tool #447

Used typed tool handler for `get_me` tool #447

Uh oh!

williammartin commented May 28, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

williammartin May 28, 2025

Uh oh!

SamMorrowDrums left a comment

Uh oh!

SamMorrowDrums May 28, 2025

Uh oh!

williammartin May 28, 2025 •

edited

Loading

Uh oh!

SamMorrowDrums May 28, 2025

Uh oh!

williammartin May 28, 2025

Uh oh!

SamMorrowDrums May 28, 2025

Uh oh!

williammartin May 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Used typed tool handler for get_me tool #447

Used typed tool handler for get_me tool #447

Uh oh!

Conversation

williammartin commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

E2E Test

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

williammartin May 28, 2025

Choose a reason for hiding this comment

Uh oh!

SamMorrowDrums left a comment

Choose a reason for hiding this comment

Uh oh!

SamMorrowDrums May 28, 2025

Choose a reason for hiding this comment

Uh oh!

williammartin May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamMorrowDrums May 28, 2025

Choose a reason for hiding this comment

Uh oh!

williammartin May 28, 2025

Choose a reason for hiding this comment

Uh oh!

SamMorrowDrums May 28, 2025

Choose a reason for hiding this comment

Uh oh!

williammartin May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Used typed tool handler for `get_me` tool #447

Used typed tool handler for `get_me` tool #447

williammartin commented May 28, 2025 •

edited

Loading

williammartin May 28, 2025 •

edited

Loading