feat: add explicit AgentIdentity auth mode#18785
feat: add explicit AgentIdentity auth mode#18785efrazer-oai wants to merge 3 commits intodev/efrazer/agent-identity-libfrom
Conversation
5259e6a to
375d41a
Compare
1e23097 to
97f8580
Compare
117d4ac to
b647fd7
Compare
e8525b0 to
742bf2d
Compare
b647fd7 to
5acde03
Compare
742bf2d to
86413e2
Compare
86413e2 to
0b61c82
Compare
5acde03 to
342ad35
Compare
0b61c82 to
52e547c
Compare
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/login/src/auth/manager.rs
Lines 1571 to 1572 in 0b61c82
chatgpt_authorization_header_for_auth immediately returns None when is_chatgpt_auth() is false, which excludes CodexAuth::AgentIdentity. Existing backend callers still use this helper for authorization, so Agent Identity sessions fail to attach any auth header and behave as unauthenticated.
codex/codex-rs/core/src/client.rs
Line 677 in 0b61c82
current_client_setup only uses auth-manager header injection when auth.is_chatgpt_auth() is true. AgentIdentity fails that check, so execution falls back to provider.api_auth(), which calls get_token() and errors for Agent Identity. Result: normal model requests fail before dispatch when AgentIdentity is active.
ℹ️ 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".
b718d3e to
b23a44f
Compare
|
Codex review comments resolved. |
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/models-manager/src/manager.rs
Line 415 in b23a44f
refresh_available_models exits unless auth_mode == Some(AuthMode::Chatgpt). After introducing explicit AgentIdentity, authenticated AgentIdentity users take this early-return path and skip remote model-catalog refresh (unless command auth is configured), leaving stale or missing model availability.
ℹ️ 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 r
PR 3 is responsible for this fix as it does a broader migration of is_chatgpt_auth callsites |
| Ok(BASE64_STANDARD.encode(signing_key.sign(payload.as_bytes()).to_bytes())) | ||
| } | ||
|
|
||
| pub fn register_agent_task_blocking( |
There was a problem hiding this comment.
Intentionally kept sync here as we need the task on startup and want to block on it, some of the alternatives are in PR description. But lmk if any of the other alternatives (e.g. making this async, propagating upwards into making AuthManager::new async) work better; i don't have a strong opinion and the choice was hard for me to reason about fully
There was a problem hiding this comment.
Can we resolve when .auth is called on AuthManager?
| auth_mode: auth_mode.map(|mode| match mode { | ||
| AuthMode::ApiKey => "ApiKey", | ||
| AuthMode::Chatgpt | AuthMode::ChatgptAuthTokens => "Chatgpt", | ||
| AuthMode::Chatgpt | AuthMode::ChatgptAuthTokens | AuthMode::AgentIdentity => { |
There was a problem hiding this comment.
maybe worth mentioning agent identity
342ad35 to
517af31
Compare
b23a44f to
4c2b315
Compare
efedf63 to
e02b5c8
Compare
4c2b315 to
be0d99c
Compare
e02b5c8 to
570e766
Compare
be0d99c to
0881c58
Compare
570e766 to
5f85536
Compare
0881c58 to
0f90a02
Compare
## Summary This PR fully reverts the previously merged Agent Identity runtime integration from the old stack: https://github.com/openai/codex/pull/17387/changes It removes the Codex-side task lifecycle wiring, rollout/session persistence, feature flag plumbing, lazy `auth.json` mutation, background task auth paths, and request callsite changes introduced by that stack. This leaves the repo in a clean pre-AgentIdentity integration state so the follow-up PRs can reintroduce the pieces in smaller reviewable layers. ## Stack 1. This PR: full revert 2. #18871: move Agent Identity business logic into a crate 3. #18785: add explicit AgentIdentity auth mode and startup task allocation 4. #18811: migrate auth callsites through AuthProvider ## Testing Tests: targeted Rust checks, cargo-shear, Bazel lock check, and CI.
0f90a02 to
8bca584
Compare
5f85536 to
52fc527
Compare
| config.cli_auth_credentials_store_mode(), | ||
| ); | ||
| auth_manager.set_forced_chatgpt_workspace_id(config.forced_chatgpt_workspace_id()); | ||
| auth_manager.set_chatgpt_backend_base_url(Some(config.chatgpt_base_url())); |
There was a problem hiding this comment.
Why don't we construct the manager with correct values and avoid RwLock?
| self.chatgpt_authorization_header_for_auth(&auth).await | ||
| } | ||
|
|
||
| pub async fn chatgpt_authorization_header_for_auth( |
There was a problem hiding this comment.
do we still need this? I thought we are doing headers via AuthProvider ?
There was a problem hiding this comment.
Removed -> these were unused after PR after this, but no point having them here.
| } | ||
| } | ||
|
|
||
| pub fn chatgpt_base_url(&self) -> Option<String> { |
There was a problem hiding this comment.
we shouldn't expose this publicly
| /// Sets the ChatGPT backend URL override for future auth runtime initialization. | ||
| /// Passing `None` clears the override and returns future initialization to the | ||
| /// default backend URL. | ||
| pub fn set_chatgpt_backend_base_url(&self, chatgpt_base_url: Option<String>) { |
There was a problem hiding this comment.
should just be set in ctor
| Self::chatgpt_bearer_token_for_auth(auth).map(|token| format!("Bearer {token}")) | ||
| } | ||
|
|
||
| pub fn subscribe_auth_state(&self) -> watch::Receiver<()> { |
There was a problem hiding this comment.
Do we need this subscribe?
There was a problem hiding this comment.
No, was used for syncing chatgpt_url but we're passing in top level now, removed.
| impl PartialEq for CodexAuth { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.api_auth_mode() == other.api_auth_mode() | ||
| match (self, other) { |
There was a problem hiding this comment.
do we care about this implemenation?
8bca584 to
9679412
Compare
9679412 to
c9e8046
Compare
Summary
This PR adds
CodexAuth::AgentIdentityas an explicit auth mode.An AgentIdentity auth record is a standalone
auth.jsonmode. WhenAuthManager::auth().awaitloads that mode, it registers one process-scoped task and stores it in runtime-only state on the auth value. Header creation stays synchronous after that because the task is initialized before callers receive the auth object.This PR also removes the old feature flag path. AgentIdentity is selected by explicit auth mode, not by a hidden flag or lazy mutation of ChatGPT auth records.
Reference old stack: https://github.com/openai/codex/pull/17387/changes
Design Decisions
auth.json.is_chatgpt_auth()remains token-specific.uses_codex_backend()is the broader predicate for ChatGPT-token auth and AgentIdentity auth.Stack
CODEX_AGENT_IDENTITYTesting
Tests: targeted Rust checks, cargo-shear, Bazel lock check, and CI.