Skip to content

chore: groundwork for multi-user to server - #195

Merged
SamMorrowDrums merged 1 commit into
mainfrom
multi-user-groundwork
Apr 9, 2025
Merged

chore: groundwork for multi-user to server#195
SamMorrowDrums merged 1 commit into
mainfrom
multi-user-groundwork

Conversation

@SamMorrowDrums

Copy link
Copy Markdown
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.

CopilotAI review requested due to automatic review settings April 9, 2025 13:47

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

FileDescription
pkg/github/server_test.goUpdated GetMe call to use a client-returning lambda.
pkg/github/server.goIntroduced 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*.goUpdated 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
SamMorrowDrumsforce-pushed the multi-user-groundwork branch 3 times, most recently from dbf1375 to b4ca6a9CompareApril 9, 2025 14:30
Comment threadpkg/github/code_scanning.go Outdated
)

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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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.

williammartin
williammartin previously approved these changes Apr 9, 2025

@williammartinwilliammartin left a comment

Copy link
Copy Markdown
Collaborator

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!

Comment threadpkg/github/server_test.go Outdated
// 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)

@williammartinwilliammartinApr 9, 2025

Copy link
Copy Markdown
Collaborator

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:

funcstubGetClientFn(client*github.Client) GetClientFn {
returnfunc(_ context.Context) (*github.Client, error) {
returnclient, nil
}
}
funcTest_GetMe(t*testing.T) {
// Verify tool definitionmockClient:=github.NewClient(nil)
tool, _:=GetMe(stubGetClientFn(mockClient), translations.NullTranslationHelper)
...

Or perhaps:

funcstubbedGetClientFn() GetClientFn {
returnfunc(_ context.Context) (*github.Client, error) {
returngithub.NewClient(nil), nil
}
}
funcTest_GetMe(t*testing.T) {
// Verify tool definitiontool, _:=GetMe(stubbedGetClientFn(), translations.NullTranslationHelper)
...

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

Copy link
Copy Markdown
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 merged commit 3ec8699 into mainApr 9, 2025
@SamMorrowDrums
SamMorrowDrums deleted the multi-user-groundwork branch April 9, 2025 23:59
DaleSeo pushed a commit to DaleSeo/github-mcp-server that referenced this pull request Oct 24, 2025
Sign up for freeto 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.

4 participants

@SamMorrowDrums@juruen@williammartin