Skip to content

chore: groundwork for multi-user to server #195

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 9, 2025

Conversation

SamMorrowDrums
Copy link
Collaborator

In order to facilitate multi-user (MCP Host) to server communication, we need to support new *github.Client clients per request, rather than globally.

This change is a NOOP that makes it possible.

@Copilot Copilot AI review requested due to automatic review settings April 9, 2025 13:47
Copy link
Contributor

@Copilot Copilot AI 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 lays the groundwork for multi‐user support by updating all GitHub client calls to accept a function returning a per-request GitHub client instead of a global client. Key changes include modifying tool/resource handler function signatures, updating tests to wrap the client in a closure, and propagating these changes across resources, pull requests, issues, and code scanning tools.

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

File Description
pkg/github/server_test.go Updated GetMe call to use a client-returning lambda.
pkg/github/server.go Introduced a getClient function to extract authToken from context and used it for all GitHub client invocations.
pkg/github/search*.go, repositories*.go, pullrequests*.go, issues*.go, code_scanning*.go Updated function signatures and calls to pass a getClient closure, ensuring per-request client configuration.
Comments suppressed due to low confidence (1)

pkg/github/server.go:21

  • [nitpick] Consider extracting the string literal "authToken" into a named constant to improve clarity and maintainability in contexts where it's used.
authToken, ok := ctx.Value("authToken").(string)

@SamMorrowDrums SamMorrowDrums force-pushed the multi-user-groundwork branch 3 times, most recently from dbf1375 to b4ca6a9 Compare April 9, 2025 14:30
@@ -13,7 +13,7 @@ import (
"github.com/mark3labs/mcp-go/server"
)

func GetCodeScanningAlert(client *github.Client, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {
func GetCodeScanningAlert(getClient func(ctx context.Context) (*github.Client, error), t translations.TranslationHelperFunc) (tool mcp.Tool, handler 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.

should we create a type with this function signature?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I knew I should really - not hard to fix.

@SamMorrowDrums SamMorrowDrums force-pushed the multi-user-groundwork branch from b4ca6a9 to f210d9a Compare April 9, 2025 16:19
@SamMorrowDrums SamMorrowDrums requested a review from juruen April 9, 2025 16:31
williammartin
williammartin previously approved these changes Apr 9, 2025
Copy link
Collaborator

@williammartin williammartin left a comment

Choose a reason for hiding this comment

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

Approved but minor request. Happy for you to bypass under the assumption you just address this if you make any changes!

@@ -18,7 +18,7 @@ import (
func Test_GetMe(t *testing.T) {
// Verify tool definition
mockClient := github.NewClient(nil)
tool, _ := GetMe(mockClient, translations.NullTranslationHelper)
tool, _ := GetMe(func(_ context.Context) (*github.Client, error) { return mockClient, nil }, translations.NullTranslationHelper)
Copy link
Collaborator

@williammartin williammartin Apr 9, 2025

Choose a reason for hiding this comment

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

If you wouldn't mind I think a lot of noise could go away with:

func stubGetClientFn(client *github.Client) GetClientFn {
	return func(_ context.Context) (*github.Client, error) {
		return client, nil
	}
}

func Test_GetMe(t *testing.T) {
	// Verify tool definition
	mockClient := github.NewClient(nil)
	tool, _ := GetMe(stubGetClientFn(mockClient), translations.NullTranslationHelper)
...

Or perhaps:

func stubbedGetClientFn() GetClientFn {
	return func(_ context.Context) (*github.Client, error) {
		return github.NewClient(nil), nil
	}
}

func Test_GetMe(t *testing.T) {
	// Verify tool definition
	tool, _ := GetMe(stubbedGetClientFn(), translations.NullTranslationHelper)
...

Slight preference for the former because it's clearer how the client is being injected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Made minor edit to names above, just in case you'd already read it.

@SamMorrowDrums SamMorrowDrums force-pushed the multi-user-groundwork branch from f210d9a to 88b709d Compare April 9, 2025 23:53
@SamMorrowDrums SamMorrowDrums merged commit 3ec8699 into main Apr 9, 2025
16 checks passed
@SamMorrowDrums SamMorrowDrums deleted the multi-user-groundwork branch April 9, 2025 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants