-
Notifications
You must be signed in to change notification settings - Fork 682
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
to
f17eac6
Compare
@@ -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.
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 I think there might be a chance to trigger one. However, if the server behaves correctly, it shouldn't happen.
f17eac6
to
03465f3
Compare
This gets you things like:
github-mcp-server/<version> (Visual Studio Code - Insiders/<version>)
if provided by the host.