- Notifications
You must be signed in to change notification settings - Fork899
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this 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)dbf1375 tob4ca6a9Comparepkg/github/code_scanning.go Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
b4ca6a9 tof210d9aCompareThere was a problem hiding this 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!
pkg/github/server_test.go Outdated
| @@ -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) { returnmockClient, nil }, translations.NullTranslationHelper) | |||
williammartinApr 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
f210d9a to88b709dCompare3ec8699 intomainUh oh!
There was an error while loading.Please reload this page.
In order to facilitate multi-user (MCP Host) to server communication, we need to support new
*github.Clientclients per request, rather than globally.This change is a NOOP that makes it possible.