refactor: unify external auth resolution by pakrym-oai · Pull Request #31421 · openai/codex

pakrym-oai · 2026-07-07T15:39:26Z

Summary

External auth had two paths: provider-command credentials were resolved through ExternalAuth, while app-provided ChatGPT credentials were installed separately and only used the provider for refresh. ExternalAuth also declared an auth mode independently from the CodexAuth value it returned, so the declaration and credentials could disagree.

This change makes the provider-owned CodexAuth authoritative for initial resolution, credential kind, and refresh.

remove ExternalAuth::auth_mode and require providers to return their current auth from resolve
route external auth registration, resolution, and unauthorized refresh through AuthManager::set_external_auth
keep the last resolved credential with its provider so synchronous consumers and unauthorized recovery observe the credential's actual mode
make the app-server bridge own both initial and refreshed ChatGPT credentials, and detach it on logout
keep model listing free of auth-refresh side effects, including in offline mode

Provider-command auth still follows its configured cache interval. App-provided ChatGPT auth still asks the parent app once after a 401 and retries the request once.

Stacked on #31355.

Testing

just test -p codex-login
just test -p codex-models-manager
focused codex-app-server tests for external login/logout, unauthorized refresh, and workspace mismatch

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1b77736c0

ℹ️ 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".

pakrym-oai · 2026-07-07T15:55:25Z

@codex review this

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 31e538d06e

ℹ️ 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".

pakrym-oai · 2026-07-07T16:05:06Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f8db315033

ℹ️ 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".

pakrym-oai · 2026-07-07T16:37:40Z

@codex review this

chatgpt-codex-connector · 2026-07-07T16:42:04Z

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: fb0e47f322

ℹ️ 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".

pakrym-oai · 2026-07-07T17:05:33Z

            )));
        }

-        login_with_chatgpt_auth_tokens(


This was saving tokens to ephemeral storage directly and had normal reload pick them up instead of having external auth bridge be the source of truth.

bolinfest · 2026-07-07T19:35:55Z

+                save_auth(
+                    &self.codex_home,
+                    &auth_dot_json,
+                    AuthCredentialsStoreMode::Ephemeral,


Why is this hardcoded to ephemeral?

Deleted, this was to support old reload path through ephemeral store.

Actually, brought it back with a comment. There is come nasty synchronization that happens over statics that I don't want to unravel in this PR.

pakrym-oai · 2026-07-07T20:32:33Z

@codex review this

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b31594dc16

ℹ️ 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".

bolinfest · 2026-07-07T23:05:14Z

            remote_control_handle,
            plugin_startup_tasks,
        } = args;
-        auth_manager.set_external_auth(Arc::new(ExternalAuthRefreshBridge {


At this point in the MessageProcessor:new() constructor, auth_manager.set_external_auth() was called before a bunch of calls to auth_manager.clone(), giving out references of Arc<AuthManager> to other objects. This seemed like a potentially useful invariant.

But in the PR as written, now the set_external_auth() call does not happen until login_chatgpt_auth_tokens_response(), correct? Does that mean there is some time where a bunch of objects have references to Arc<AuthManager> before set_external_auth() has been called, and if so, is that OK?

I guess my higher level question is: can we make external_auth: Arc<dyn ExternalAuth>> a required param to AuthManager::new() so it is no longer Option internally?

Hmm, I guess the problem is that we start the app server even if the user is logged out and then ClientRequest::LoginAccount is a request type that it accepts, which is what ultimately sets external_auth?

No, you can create AuthManager without ExternalAuth. Setting ExternalAuth is like logging in so having AuthManager without ExternalAuth is like having auth manager without it being logged in.

Ah, I see, and then there are cases like let manager = AuthManager::external_bearer_only(auth_config); where external_auth is set to Some in the constructor...

bolinfest · 2026-07-07T23:18:23Z

+    /// we can assume that the source already refreshed it. Otherwise, ask the
+    /// token authority to refresh.
    pub async fn refresh_token(&self) -> Result<(), RefreshTokenError> {
        let _refresh_guard = self.refresh_lock.acquire().await.map_err(|_| {


Ah, I see the semaphore is used here and the implementation of this method and refresh_token_from_authority() stay the same in this PR, but both docstrings are updated...

bolinfest · 2026-07-07T23:44:30Z

-
-    /// Refreshes auth in response to a manager-driven refresh attempt.
+    /// Refreshes auth and makes the returned value current for future `resolve()` calls.
    fn refresh(&self, context: ExternalAuthRefreshContext) -> ExternalAuthFuture<'_, CodexAuth>;


Maybe too complicated, but if this took &mut self instead of &self, then callers would be responsible for ensuring refresh() calls were serialized.

I think it's too complicated, the AuthManager ownership is too distributed and it's always inside an Arc. I don't know how we'll ever get to the state where we can take mut self

bolinfest · 2026-07-07T23:50:34Z

+                        .await
+                }
+                CodexAuth::ApiKey(_)
+                | CodexAuth::ChatgptAuthTokens(_)


Confirming: nothing special for this case?

Yep, we don't expect to manually refresh ChatgptAuthTokens

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread codex-rs/models-manager/src/manager.rs Outdated

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread codex-rs/models-manager/src/manager.rs Outdated

Comment thread codex-rs/login/src/auth/manager.rs Outdated

Comment thread codex-rs/login/src/auth/manager.rs Outdated

pakrym-oai added the code-reviewed label Jul 7, 2026

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread codex-rs/app-server/src/request_processors/account_processor.rs

Comment thread codex-rs/login/src/auth/manager.rs Outdated

Base automatically changed from codex/external-auth-codex-auth-precursor to main July 7, 2026 16:47

pakrym-oai added 7 commits July 7, 2026 09:51

refactor: unify external auth resolution

74e1df4

refactor: make external auth own credentials

1b6c68b

fix: retain message processor timeout import

77f76f1

refactor: tighten external auth state

544653f

refactor: commit external auth in one step

8393b8e

fix: preserve external auth 401 recovery

9f8d177

test: exercise external auth refresh in cloud config

ce1f9e4

pakrym-oai force-pushed the pakrym/unify-external-auth-resolution branch from 85053fa to ce1f9e4 Compare July 7, 2026 16:51

pakrym-oai commented Jul 7, 2026

View reviewed changes

bolinfest reviewed Jul 7, 2026

View reviewed changes

Comment thread codex-rs/login/src/auth/manager.rs Outdated

bolinfest reviewed Jul 7, 2026

View reviewed changes

Comment thread codex-rs/login/src/auth/manager.rs Outdated

bolinfest reviewed Jul 7, 2026

View reviewed changes

Comment thread codex-rs/login/src/auth/manager.rs Outdated

bolinfest reviewed Jul 7, 2026

View reviewed changes

Comment thread codex-rs/app-server/src/external_auth.rs

bolinfest reviewed Jul 7, 2026

View reviewed changes

Comment thread codex-rs/login/src/auth/manager.rs Outdated

Simplify external auth ownership

b31594d

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread codex-rs/app-server/src/external_auth.rs

Comment thread codex-rs/app-server/src/request_processors/account_processor.rs

pakrym-oai added 2 commits July 7, 2026 13:45

codex: fix CI failure on PR #31421

7077b30

codex: address PR review feedback (#31421)

3ae9aab

bolinfest self-requested a review July 7, 2026 22:41

bolinfest reviewed Jul 7, 2026

View reviewed changes

bolinfest approved these changes Jul 7, 2026

View reviewed changes

pakrym-oai merged commit e60af81 into main Jul 8, 2026
63 of 66 checks passed

pakrym-oai deleted the pakrym/unify-external-auth-resolution branch July 8, 2026 00:05

github-actions Bot locked and limited conversation to collaborators Jul 8, 2026

Uh oh!

Conversation

pakrym-oai commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

pakrym-oai commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pakrym-oai commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

pakrym-oai commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jul 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pakrym-oai commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

pakrym-oai commented Jul 7, 2026 •

edited

Loading