Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Used typed tool handler forget_me tool#447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
SamMorrowDrums merged 4 commits intomainfromwm/get-me-typed-tool
May 28, 2025

Conversation

williammartin
Copy link
Collaborator

@williammartinwilliammartin commentedMay 28, 2025
edited
Loading

Description

This PR is the first of many that moves handlers to useNewTypedToolHandler 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)PASSok      github.com/github/github-mcp-server/e2e 7.410s

@williammartinwilliammartin marked this pull request as ready for reviewMay 28, 2025 13:19
@CopilotCopilotAI review requested due to automatic review settingsMay 28, 2025 13:19
@williammartinwilliammartin requested a review froma team as acode ownerMay 28, 2025 13:19
Copy link
Contributor

@CopilotCopilotAI 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 standardizes tool handlers usingNewTypedToolHandler for theget_me tool, boosts testing confidence with schema snapshots, and adds missing third-party licenses and entries in platform-specific license summaries.

  • RefactorGetMe 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 updatethird-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
FileDescription
pkg/github/context_tools.goRefactoredGetMe to useNewTypedToolHandler and removed manual JSON handling
pkg/github/server.goAddedmarshalledTextResult helper for consistent text results
pkg/github/context_tools_test.goUpdatedTest_GetMe to use table-driven checks and snapshot tests
pkg/toolsets/toolsets_test.goRenamed test for clarity and simplified assertions
docs/testing.mdDocumented usage oftoolsnaps 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 spellingmarshalledTextResult. For consistency with Go'sjson.Marshal and typical API conventions, consider renaming tomarshaledTextResult.
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 linecommitted. 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.

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

Choose a reason for hiding this comment

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

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

typeClientsstruct { rest..gql..}

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

SamMorrowDrums
SamMorrowDrums previously approved these changesMay 28, 2025
Copy link
Collaborator

@SamMorrowDrumsSamMorrowDrums left a comment

Choose a reason for hiding this comment

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

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 frommcp-go types.

"type": "object"
},
"name": "get_me"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

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

Copy link
CollaboratorAuthor

@williammartinwilliammartinMay 28, 2025
edited
Loading

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

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 reacted with thumbs up emoji
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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 usemcp-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 intomcp-go. Maybe overkill for now, just something that is coming onto my radar.

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

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.

Co-authored-by: Sam Morrow <info@sam-morrow.com>
Co-authored-by: Sam Morrow <info@sam-morrow.com>
@SamMorrowDrumsSamMorrowDrums merged commit023f59d intomainMay 28, 2025
16 checks passed
@SamMorrowDrumsSamMorrowDrums deleted the wm/get-me-typed-tool branchMay 28, 2025 13:56
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

Copilot code reviewCopilotCopilot left review comments

@SamMorrowDrumsSamMorrowDrumsSamMorrowDrums approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@williammartin@SamMorrowDrums

[8]ページ先頭

©2009-2025 Movatter.jp