-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Show login options when not signed in with ChatGPT #2440
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
Summary
Review
Suggestions
Overall, changes are cohesive and in the right crates; just ensure tests/snapshots are updated and consider the extra tests noted. |
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.
Looks good overall but please make the suggested enum-related changes!
codex-rs/login/src/lib.rs
Outdated
load_auth(codex_home, true) | ||
pub fn from_codex_home( | ||
codex_home: &Path, | ||
always_use_api_key_signing: bool, |
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.
Seeing this passed in as a raw bool
value is not great. Please introduce an enum with two variants like:
enum AuthCredentialStrategy {
ApiKeyOnly,
PreferChatGpt,
}
or whatever names you think are appropriate. Then update this function to take the new enum type instead of a bool
.
codex-rs/tui/src/lib.rs
Outdated
pub enum LoginStatus { | ||
ChatGPT, | ||
ApiKey { always_use_api_key_signing: bool }, | ||
None, |
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.
Maybe NotAuthenticated
?
|
||
impl OnboardingScreen { | ||
pub(crate) fn new(args: OnboardingScreenArgs) -> Self { | ||
let show_login_screen = matches!(args.login_status, LoginStatus::None) |
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 use a match
does Clippy pressure you to use matches!()
instead? I generally prefer match
so that if a new enum variant is introduced, you are forced to deal with it.
Motivation: we have users who uses their API key although they want to use ChatGPT account. We want to give them the chance to always login with their account.
This PR displays login options when the user is not signed in with ChatGPT. Even if you have set an OpenAI API key as an environment variable, you will still be prompted to log in with ChatGPT.
We’ve also added a new config,
preferred_auth_method
which ischatgpt
by default, which ensures you are never asked to log in with ChatGPT and always defaults to using your API key.Screen.Recording.2025-08-18.at.4.17.32.PM.mov
After ChatGPT sign in:
Screen.Recording.2025-08-18.at.4.18.37.PM.mov