Skip to content

Conversation

aibrahim-oai
Copy link
Collaborator

@aibrahim-oai aibrahim-oai commented Aug 18, 2025

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 is chatgpt 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

@aibrahim-oai aibrahim-oai added the codex-rust-review Perform a detailed review of Rust changes. label Aug 18, 2025
@github-actions github-actions bot added codex-rust-review-in-progress and removed codex-rust-review Perform a detailed review of Rust changes. labels Aug 18, 2025
Copy link

Summary
This PR adds a new config flag to force API key signing and updates login/TUI flows to respect it. It introduces a LoginStatus enum to decide whether to show the onboarding login screen and updates docs to explain how to enable the behavior.

  • New flag: always_use_api_key_signing in config/TOML, default false.
  • TUI shows or skips login based on LoginStatus, allowing “Continue using API key”.
  • All call sites of CodexAuth::from_codex_home updated to accept the new parameter.
  • README documents flag usage and CLI override.

Review
Nice, focused change that cleanly threads a single config through login and UI. A few nits and test/snapshot follow-ups below.

  • README.md:@@ -167,0 +168,20
    • Clear docs; consider adding a short rationale (“use API key for originator signing to …”) in the PR body too.
  • codex-rs/core/src/config.rs:@@ -165,0 +166,3; @@ -411,0 +415,3; @@ -674,0 +681
    • Config plumbed correctly with sensible default. Good placement in core.
  • codex-rs/login/src/lib.rs:@@ -66,2 +66,5; @@ -168 +171,5; @@ -204 +211
    • Signature change and logic look good. Please add a unit test for “always_use_api_key_signing = true” when tokens would otherwise prefer ChatGPT, to assert AuthMode::ApiKey.
  • codex-rs/login/src/token_data.rs:@@ -26 +26,5
    • Rename + override behavior is clear. Consider a brief doc comment on always_use_api_key_signing.
  • codex-rs/tui/src/lib.rs:@@ -299 +300,8; @@ -304,3 +312,8
    • LoginStatus is a nice simplification. One suggestion: add a lightweight test of get_login_status mapping for each variant if feasible.
  • codex-rs/tui/src/app.rs:@@ -140,17 +141,22; @@ -158,11 +164,15
    • Good refactor from boolean to enum. This likely changes rendered text; snapshot updates may be needed.
  • codex-rs/tui/src/onboarding/onboarding_screen.rs:@@ -61,0 +63,7; @@ -71 +79
    • Using login_status to compute show_login_screen/is_logged_in makes the state explicit. Nice.
  • codex-rs/tui/src/onboarding/auth.rs:@@ -157,0 +160,5; @@ -309,3 +316,3
    • UX tweak (“Continue using API key”) reads well. Snapshot updates probably required.

Suggestions

  • Tests: add coverage for the forced API-key path and TUI LoginStatus mapping.
  • Snapshots: run cargo test -p codex-tui and accept updated snapshots if changes are intended.
  • PR body: include motivation and, if possible, a short TUI screenshot/gif showing the “Continue using API key” option.

Overall, changes are cohesive and in the right crates; just ensure tests/snapshots are updated and consider the extra tests noted.


View workflow run

Copy link
Collaborator

@bolinfest bolinfest left a 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!

load_auth(codex_home, true)
pub fn from_codex_home(
codex_home: &Path,
always_use_api_key_signing: bool,
Copy link
Collaborator

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.

pub enum LoginStatus {
ChatGPT,
ApiKey { always_use_api_key_signing: bool },
None,
Copy link
Collaborator

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)
Copy link
Collaborator

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.

@aibrahim-oai aibrahim-oai enabled auto-merge (squash) August 19, 2025 03:15
@aibrahim-oai aibrahim-oai merged commit 97f995a into main Aug 19, 2025
15 checks passed
@aibrahim-oai aibrahim-oai deleted the login-flow branch August 19, 2025 03:22
@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants