- Notifications
You must be signed in to change notification settings - Fork937
chore: extend user agent#237
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.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -13,6 +13,7 @@ import ( | ||
| iolog "github.com/github/github-mcp-server/pkg/log" | ||
| "github.com/github/github-mcp-server/pkg/translations" | ||
| gogithub "github.com/google/go-github/v69/github" | ||
| "github.com/mark3labs/mcp-go/mcp" | ||
| "github.com/mark3labs/mcp-go/server" | ||
| log "github.com/sirupsen/logrus" | ||
| "github.com/spf13/cobra" | ||
| @@ -137,11 +138,19 @@ func runStdioServer(cfg runConfig) error { | ||
| t, dumpTranslations := translations.TranslationHelper() | ||
| beforeInit := func(_ context.Context, _ any, message *mcp.InitializeRequest) { | ||
| ghClient.UserAgent = fmt.Sprintf("github-mcp-server/%s (%s/%s)", version, message.Params.ClientInfo.Name, message.Params.ClientInfo.Version) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Haven't looked into how hooks work in depth, so bear with me, one question that I have is whether we need to protect this with a mutex. Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Disregard my comment. Even we wanted to protect it with a mutex, we couldn't because this is in the github.Client. Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. So, if I understand this correctly. This is going to be OK from a race condition standpoint if host/client always initializes the server before doing anything else. The only way to cause a race here is by calling a tool and initializing at the same time. Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. If you wanted to completely remove the possibility of a race, you could make things slightly more complex and set the client info into a different variable protected by a mutex. And then, you could set the client agent accordingly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yeah, that's a good shout. I was thinking about this too, but it is only a one-shot event, and if it happens a second time it can also update the info, that's fine. | ||
| } | ||
| getClient := func(_ context.Context) (*gogithub.Client, error) { | ||
| return ghClient, nil // closing over client | ||
| } | ||
| hooks := &server.Hooks{ | ||
| OnBeforeInitialize: []server.OnBeforeInitializeFunc{beforeInit}, | ||
| } | ||
| // Create | ||
| ghServer := github.NewServer(getClient, version, cfg.readOnly, t, server.WithHooks(hooks)) | ||
| stdioServer := server.NewStdioServer(ghServer) | ||
| stdLogger := stdlog.New(cfg.logger.Writer(), "stdioserver", 0) | ||