Skip to content
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

Feature/GitHub copilot #49

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

aislasq
Copy link
Contributor

@aislasq aislasq commented Feb 27, 2024

This is a first draft to add github copilot to Oatmeal. It is working fine, but it is very hacky to adjust the Github Copilot login flow. I am out of my means here and would appreciate some help.

  1. A new GithubAuth struct was created to handle Github Authentication:
    • It checks the default ~/.config/github-copilot/hosts.json for an oauth token. If it does not exists it proceeds to ask the user to approve the device, requests the oauth token, and saves the result to the hosts.json file.
    • With the oauth token, it then requests a github copilot token (it is different), and saves that to the GithubAuth struct.
    • The GithubAuth flow is called on each request, this is not good because it is regenerating the github copilot token each time.
    • The request login Message shown by Author::Oatmeal does not call done: true, so the pending result will be appended to the message bubble.
    • It is mocking a vscode editor with the headers.
  2. The it_gets_completion is failing due to the GithubAuth flow inside the get_completion function.
  3. There are no extra parameters (URL, Token) to set, the url does not change and the token comes from the ~/.config/github-copilot/hosts.json file
  • Implement trait for new backend.
  • Update the BackendName enum with your new Backend name.
  • Update the BackendManager to provide your new backend.
  • Write tests
  • Update Readme
  • Update config

@dustinblackman
Copy link
Owner

dustinblackman commented Mar 9, 2024

@aislasq Hey there! Sorry for the late reply, weekends are my time for looking at open source.

This is exciting! I haven't played with Copilot since beta, I'd be really happy to play with again in Oatmeal.

A new GithubAuth struct was created to handle Github Authentication:

So your biggest issue here is the fact you want to do the initial authentication from inside Oatmeal. What if you didn't? I'm imagining on first run if ~/.config/github-copilot/hosts.json does not exist, Oatmeal can provide an error message stating authethencation does not exist and to run command gh extension install github/gh-copilot && gh auth login. This way if authentication ever changes on Github's side, no additional work is required in Oatmeal.

Fix GithubAuth flow called on each completion request. Should only be called once and use the token from then on, until expires_at

Is this a short lived token, 15 minutes? Or is it a long lived token managed by the Github CLI?

@aislasq
Copy link
Contributor Author

aislasq commented Mar 11, 2024

Refactored the code as you suggested. It is more straightforward. I do believe we could handle the login from within Oatmeal, but maybe later. The token's lifetime is 30 min, which is just long enough for a question/session. I have it set up to save it in the struct, but I cannot mutate it; a hacky thing I could do is to save it in the messages context with { role: "token", content: ":" }.
Let me know what you think

@dustinblackman
Copy link
Owner

could do is to save it in the messages context with { role: "token", content: ":" }.

Let's do that! I can refactor the model to allow mutations in the future. :) Thank you very much!

@aislasq
Copy link
Contributor Author

aislasq commented Mar 12, 2024

I added the serialized session token to the content and added expiration management, along with the 3 new tests.

It looks like the hosts.json file where the oauth token is for third party extensions so gh auth login nor VSCode login work. It does work for Android Studio, the neovim copilot and copilot chat plugins, and the implementation we are leaving pending for Oatmeal.

Although I suspect some users will already have this token from other clients, I think we should give a clearer message that the device login is not yet implemented in Oatmeal. What do you think that should be?

@dustinblackman
Copy link
Owner

dustinblackman commented Mar 14, 2024

Although I suspect some users will already have this token from other clients, I think we should give a clearer message that the device login is not yet implemented in Oatmeal. What do you think that should be?

Odd, so there's no token file provided anywhere for the gh command line such as the copilot extension? I don't have an active subscription at the moment to test myself (that's a job for next week!)

Although I suspect some users will already have this token from other clients, I think we should give a clearer message that the device login is not yet implemented in Oatmeal. What do you think that should be?

I agree! We for sure want to specify you must login in another form, but it's best to provide how. I'm finding it difficult to see answers when quickly searching online. Do all the other existing implementation do it themselves? I'm almost wondering if it's worth adding a CLI command to create the token initially as you've already done the bulk of the work for authorization, with the goal of taking the easiest/fastest solution. I wouldn't want you spending hours trying to recreate something that already exists externally.

@aislasq
Copy link
Contributor Author

aislasq commented Mar 15, 2024

Odd, so there's no token file provided anywhere for the gh command line such as the copilot extension?

At least on my tests there is not. I've been looking in my computer and cannot find where is the configuration saved for gh or vscode. I also tested with copilot.lua (my daily client) and they have the auth implementation (in lua) as a command as you suggested. I've seen other examples of the authentication flow in typescript and python, though I don't have the repos on hand.

I'll try to add the auth as a command with the authentication flow I have. I'll try to use /githubcopilot-auth inside oatmeal, or oatmeal --auth githubcopilot. It depends if I can recreate the Backend as it depends on the oauth token.

@dustinblackman
Copy link
Owner

I was thinking a command line call. Taking it out of the UI would probably be easiest as it gives you a bit more control to do what you need.

@aislasq
Copy link
Contributor Author

aislasq commented Mar 18, 2024

I added the new command oatmeal auth --service <service-name>, it works well right now for githubcopilot.

I want to check with you on a couple of things:

  • How do you feel about the service argument, should I leave it like that or change it to backend or something different?
  • The path is currently fixed to ~/.config/github-copilot/hosts.json, I was thinking of adding am optional --path <path-for-the-auth-file> argument on the cli command and a GITHUBCOPILOT_OAUTH_FILE config option to customize the file path. I'm not only thinking on github copilot, but if in the future other auth services are needed.
    Do you agree this is a necessary edge case to cover?

@dustinblackman
Copy link
Owner

I'm so sorry for the super late reply. I've been really out of the loop this month.

Service works for me!
And the path will all depends if it's cross platform or not. May be worth adding it if the path on Windows for example is unknown.

@aislasq aislasq marked this pull request as ready for review April 29, 2024 16:53
@aislasq
Copy link
Contributor Author

aislasq commented May 1, 2024

So I ended up adding a couple of things.

  • the service flag that accepts one parameter (githubcopilot)
  • an optional githubcopilot-auth-file global parameter (env:OATMEAL_GITHUBCOPILOT_AUTH_FILE) for the location of file to use for the token. It is global because it needs to read the token from there for the model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants