fix: persist copilot token metadata#441
fix: persist copilot token metadata#441GrothKeiran wants to merge 2 commits intorouter-for-me:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the GitHub Copilot authentication process by ensuring that critical token metadata, including the access token, token type, and scope, is persistently stored. This improvement allows the model registry to load dynamic models immediately upon authentication, significantly streamlining operations by removing the previous requirement for a service restart. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the GitHub Copilot authentication flow to include the access_token, token_type, and scope in the auth record's metadata. This allows other parts of the system, like the model registry, to access this information immediately without needing a server restart.
The change is correct and addresses the issue described. However, it introduces data duplication between the Storage object and the Metadata map within the Auth record. I've added a comment with a suggestion to refactor this by deriving the metadata from the storage object directly, which would improve long-term maintainability.
| Metadata: map[string]any{ | ||
| "email": userInfo.Email, | ||
| "username": username, | ||
| "name": userInfo.Name, | ||
| "email": userInfo.Email, | ||
| "username": username, | ||
| "name": userInfo.Name, | ||
| "access_token": tokenData.AccessToken, | ||
| "token_type": tokenData.TokenType, | ||
| "scope": tokenData.Scope, | ||
| }, |
There was a problem hiding this comment.
While this correctly adds the necessary fields to the metadata, it duplicates data already present in the tokenStorage object. This could become a maintenance issue if the CopilotTokenStorage struct is updated in the future, as those changes would need to be manually mirrored here.
To make this more robust and avoid data duplication, you could generate the metadata map directly from the tokenStorage object. This would ensure the metadata is always a complete and accurate reflection of the storage object.
For example, you could use JSON marshaling and unmarshaling to achieve this:
// A helper to convert storage to metadata
func tokenStorageToMetadata(storage any) (map[string]any, error) {
data, err := json.Marshal(storage)
if err != nil {
return nil, err
}
var metadata map[string]any
if err := json.Unmarshal(data, &metadata); err != nil {
return nil, err
}
return metadata, nil
}
// In RequestGitHubToken:
tokenStorage := copilotAuth.CreateTokenStorage(bundle)
metadata, err := tokenStorageToMetadata(tokenStorage)
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("failed to create token metadata: %v", err)})
return
}
record := &cliproxyauth.Auth{
Provider: "github-copilot",
FileName: fileName,
Storage: tokenStorage,
Metadata: metadata,
}This approach centralizes the definition of the persisted data in the CopilotTokenStorage struct, improving maintainability.
Summary
Implementation
CopilotTokenStorageintoMetadatato persistaccess_token,token_type, andscopealongside user identity fields.