[device-auth] When headless environment is detected, show device login flow instead. #8756
Conversation
| use codex_core::auth::CLIENT_ID; | ||
| use codex_core::auth::login_with_api_key; | ||
| use codex_core::auth::read_openai_api_key_from_env; | ||
| use codex_core::env::is_headless_environment; |
There was a problem hiding this comment.
Note that the diffs in this file is pretty much the same as in tui/src/onboarding/auth.rs and this is intentional since tui2 is a synced copy of tui
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 983c2956d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
codex-rs/tui/src/onboarding/auth.rs
Outdated
| match r { | ||
| Ok(()) => { | ||
| auth_manager.reload(); | ||
| *sign_in_state.write().unwrap() = | ||
| SignInState::ChatGptSuccessMessage; |
There was a problem hiding this comment.
In the headless-device-code fallback path, this async task unconditionally overwrites sign_in_state after child.block_until_done().await. If the user presses Esc or switches to API key entry while the fallback browser flow is running (i.e., device code endpoint returned 404), the task can reset the UI back to PickMode or ChatGptSuccessMessage, clobbering the user’s new state. The rest of the device-code flow uses a cancel/Arc::ptr_eq guard to prevent stale updates; the fallback should do the same before writing. The same unguarded updates exist in codex-rs/tui2/src/onboarding/auth.rs.
Useful? React with 👍 / 👎.
codex-rs/tui/src/onboarding/auth.rs
Outdated
| }); | ||
| self.request_frame.schedule_frame(); | ||
|
|
||
| tokio::spawn(async move { |
There was a problem hiding this comment.
This looks complex (lots of nested behavior). Someone looking at this to validate it has to read every single line to understand whether it's correct as there's no grouping of parts of this that make it easy to understand small concrete pieces. Break it down into smaller pieces with documentation that captures the intent / context of each part of this and how each fits in the whole.
It's likely that each of the individual behaviors here have some property encoded that should always hold that is security sensitive, and it's important to make sure that is tested. Breaking it down smaller should facilitate that too.
Basically rewrite this 3 times:
- documentation capturing intent
- code capturing small understandable chunks
- tests capturing assertable invariants
There was a problem hiding this comment.
Agreed, my bad for not pushing codex hard enough to make it more modular. Updated.
joshka-oai
left a comment
joshka-oai
left a comment
There was a problem hiding this comment.
These were a couple of small comments that I made the other day, but didn't hit submit - apologies
When headless environment is detected, show device login flow instead.