-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: apply auth type from arg or env variables #705
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
base: main
Are you sure you want to change the base?
fix: apply auth type from arg or env variables #705
Conversation
|
I just tried your branch and it works now with a local model. When do you think your fix will be merged with the main one? |
|
Thank you for trying my fix, if you want you can install it or write an alias to use it until it's released for more real-life testing! I'm no maintainer here and I don't have permissions to request a review. Alternatively, a workflow could assign reviewers at random, I guess we just have to be patient 😄 After looking at the merged pull requests during the last week it seems it could only take a week until it gets reviewed, maybe even faster. |
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.
I found some issue testing the auth switching behavior. Changes are required to keep the auth switch working.
| this.isTrusted, | ||
| ); | ||
| // Overwrite from user config (arg or env or settings.json) | ||
| if (this.authType) { |
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.
This will break the auth type switching behavior.
Currently, switching the authentication type will mutate user scope settings, which will be merged here to take effect. With the overwrite process, the mutation will not take effect.
| onSelect(AuthType.USE_OPENAI, SettingScope.User); |
We may have two options:
- Consider users' auth selection as system override, which will always take the highest priority;
- Add merge strategy in
loadCliConfigto respect env variables and cli arguments.
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.
Thank you for finding this issue, which option would be preferable?
I can't find a merge strategy anywhere, loadCliConfig() is already being called from main() with settings.merged and my addition there is to respect env and args 😄
Do you have a a quick example (pseudo) code for both options?
I guess by option 1 you mean overriding systemSettings inside loadSettings() itself which happens much earlier in main() before env and args have been processed?
If you already know an acceptable solution, feel free to push a commit, maintainers have all permissions.
Bugfix
Run local LLM when passing --openai-api-key or configuring env variables
After the "Get started" wizard has been completed, or settings.json has been configured manually, the command line flag
--openai-api-keyand env variables are not being respected anymore to determine the authType.For example, if settings.json contains
{ "selectedAuthType": "qwen-oauth" }, local LLMs can't be used anymore by providing theOPENAI_...env variables, the login page is being opened instead.This implementation configures this setting now:
{ "selectedAuthType": "openai" }in settings.json is usedWorkaround for the current release:
The user always has to keep settings.json in sync with his intended use, for example if he now passes the
--openai-api-keyflag or switches providers in the .env file, i.e. from OPENAI to GEMINI or GOOGLE (vertex.ai).Dive Deeper
Please note that not only OPENAI is affected, even if the env is configured with other providers, only settings.json is being respected at the moment to determine the authType.
This also means, that the user has to change settings.json after the "Get started" wizard if he doesn't want to use qwen-oauth or openai.
Reviewer Test Plan
Install the current release, then try all steps:
{ "selectedAuthType": "openai" }-> OPENAI is being used (no login page)Checkout/install this PR, delete settings.json, then retry all steps:
{ "selectedAuthType": "openai" }-> OPENAI is being used (no login page)Keep in mind that qwen doesn't respect Ctrl+C or Escape even if
(Press ESC to cancel)is being displayed while waiting for the qwen-oauth login page, the node process has to be stopped (killall -9 node).Testing Matrix
Linked issues / bugs