- Notifications
You must be signed in to change notification settings - Fork897
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.
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 extends the user agent string by integrating host-supplied client details into the server initialization process.
- Introduces a new hook function, beforeInit, that modifies the user agent with client information.
- Updates the server initialization to include hooks.
- Adds a new package import required for the extended functionality.
1216fe5 tof17eac6Compare| @@ -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 comment
The 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.
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.
Disregard my comment. Even we wanted to protect it with a mutex, we couldn't because this is in the github.Client.
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.
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.
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 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 comment
The 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.
juruen left a comment• 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.
LGTM! I added a comment re: races as Ithink there might be a chance to trigger one. However, if the server behaves correctly, it shouldn't happen.
f17eac6 to03465f3Compare9dacf70 intomainUh oh!
There was an error while loading.Please reload this page.
This gets you things like:
github-mcp-server/<version> (Visual Studio Code - Insiders/<version>)if provided by the host.