[codex][device-stable-id][12/n] Send source stable ID with managed auth requests#28815
[codex][device-stable-id][12/n] Send source stable ID with managed auth requests#28815rpinkerton-oai wants to merge 1 commit into
Conversation
|
@codex review |
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
![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: 916b1c0d7c
ℹ️ 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".
| Ok(config) => { | ||
| let auth_manager = | ||
| AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ true).await; | ||
| crate::shared_cli_auth_manager(config, /*enable_codex_api_key_env*/ true).await; |
There was a problem hiding this comment.
When codex doctor runs with a fresh or invalid $CODEX_HOME/installation_id, this now calls shared_cli_auth_manager, which resolves auth stable IDs via resolve_installation_id; that helper creates the codex home directory and creates or rewrites the installation_id file. For the doctor diagnostic path, that means merely generating a report mutates user state before the checks run, contrary to the command's read-mostly behavior. Keep the direct auth-manager construction here or use a read-only stable-id lookup.
Useful? React with 👍 / 👎.
| auth_manager | ||
| .update_stable_ids(AuthStableIds::source_surface(Some(installation_id.clone()))); |
There was a problem hiding this comment.
This only seeds the default sourceSurfaceStableId for the in-process app server; the standalone daemon path in app-server/src/lib.rs still constructs its AuthManager without calling update_stable_ids even though it has already resolved installation_id. In daemon/stdio/unix-socket mode, any managed-auth refresh triggered by model/catalog/remote-control work before a client sends account/read or account/login/start will still omit the new source-surface stable ID, so the feature is inconsistent across supported app-server transports.
Useful? React with 👍 / 👎.
rpinkerton-oai
force-pushed
the
codex/stable-id-auth-requests
branch
from
916b1c0 to
cbd11ef
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbd11ef2ce
ℹ️ 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".
| if newer.oaicom_stable_id.is_some() { | ||
| self.oaicom_stable_id = newer.oaicom_stable_id; | ||
| } | ||
| if newer.source_surface_stable_id.is_some() { | ||
| self.source_surface_stable_id = newer.source_surface_stable_id; |
There was a problem hiding this comment.
When a long-running app-server client first provides an oaicomStableId or sourceSurfaceStableId and later sends null because the browser ID is unavailable or rotated, these guards leave the old value in AuthManager; both account/read and account/login/start call update_stable_ids, and login/refresh then forwards the stored value on authorize, exchange, and refresh requests. That means subsequent managed-auth requests can keep carrying a stale browser ID instead of omitting the optional context, corrupting experiment attribution; distinguish omission from explicit null or replace the stored value for fields supplied by a request.
Useful? React with 👍 / 👎.
| config: &codex_core::config::Config, | ||
| enable_codex_api_key_env: bool, | ||
| ) -> Arc<AuthManager> { | ||
| let auth_manager = AuthManager::shared_from_config(config, enable_codex_api_key_env).await; | ||
| auth_manager.update_stable_ids(resolve_cli_auth_stable_ids(config).await); |
There was a problem hiding this comment.
This helper now adds the installation-backed sourceSurfaceStableId for the CLI paths that use it, but doctor::build_report still constructs AuthManager directly before running the websocket reachability check. When codex doctor runs with managed ChatGPT auth that needs a proactive refresh, AuthManager::auth() can refresh the token without these stable IDs, so the doctor path is still missing the attribution this change is trying to add; use the same helper there as well.
Useful? React with 👍 / 👎.
rpinkerton-oai
force-pushed
the
codex/stable-id-auth-requests
branch
from
cbd11ef to
6060f40
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6060f405f7
ℹ️ 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".
| if newer.oaicom_stable_id.is_some() { | ||
| self.oaicom_stable_id = newer.oaicom_stable_id; | ||
| } |
There was a problem hiding this comment.
Because AuthManager is process-wide for app-server connections, account/read and account/login/start calls from different clients update the same AuthStableIds. With this merge, a request that omits oaicomStableId after another client supplied one keeps the old OAICOM ID and sends it on later managed login or refresh requests even though the current client did not provide it, which leaks/misattributes the optional analysis context. Replace the per-field value for each update, or scope the values to the connection/login attempt instead of only overwriting Some values.
Useful? React with 👍 / 👎.
| enable_codex_api_key_env: bool, | ||
| ) -> Arc<AuthManager> { | ||
| let auth_manager = AuthManager::shared_from_config(config, enable_codex_api_key_env).await; | ||
| auth_manager.update_stable_ids(resolve_cli_auth_stable_ids(config).await); |
There was a problem hiding this comment.
This helper seeds the CLI AuthManager for the main.rs call sites, but codex doctor still constructs its AuthManager directly in cli/src/doctor.rs and passes it to websocket_reachability_check, whose provider auth().await can refresh managed ChatGPT tokens. When that refresh happens during codex doctor, the refresh request still has no source_surface_stable_id, so the doctor path is the remaining CLI path that misses this stable-ID propagation; use the exported resolver there too or share this helper.
Useful? React with 👍 / 👎.
| auth_manager | ||
| .update_stable_ids(AuthStableIds::source_surface(Some(installation_id.clone()))); |
There was a problem hiding this comment.
This seeds only the in-process app-server runtime; the standalone app-server path still constructs its shared AuthManager without an installation/source-surface ID while passing that manager to remote-control and socket request handling. In a codex app-server --listen or remote-control session that refreshes managed ChatGPT auth before a client has sent account/read with IDs, the refresh request still omits source_surface_stable_id, so provenance is missing for that managed-auth path; seed the standalone manager for non-stdio transports while preserving the stdio explicit-client exception.
Useful? React with 👍 / 👎.
rpinkerton-oai
force-pushed
the
codex/stable-id-auth-requests
branch
from
6060f40 to
adde7df
Compare
|
@codex review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adde7dfed2
ℹ️ 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".
| self.auth_manager.update_stable_ids(stable_ids); | ||
| let opts = LoginServerOptions { | ||
| open_browser: false, | ||
| codex_streamlined_login, | ||
| stable_ids: self.auth_manager.stable_ids(), |
There was a problem hiding this comment.
In a long-lived app-server process with multiple clients (or after one client/user logs out and another starts login), a later account/login/start that omits or sends null for either stable ID still builds LoginServerOptions from self.auth_manager.stable_ids() after update_stable_ids has only merged non-None values. That causes the new authorize/code-exchange flow to carry a previous client's stable ID, misattributing the managed-auth request; build the login options from the current request's IDs (or clear fields when null is supplied) instead of the whole cached AuthManager state.
Useful? React with 👍 / 👎.
| auth_manager | ||
| .update_stable_ids(AuthStableIds::source_surface(Some(installation_id.clone()))); |
There was a problem hiding this comment.
This seeds the AuthManager only on the in-process path, but the standalone app-server path in app-server/src/lib.rs also resolves an installation_id and then passes an unseeded AuthManager into remote control and message processing. When remote control starts (or any early auth_manager.auth() call triggers a proactive managed-token refresh) before a client has sent account/read, request_chatgpt_token_refresh sees the default empty IDs and sends the refresh without source_surface_stable_id; add the same initialization to the standalone app-server manager.
Useful? React with 👍 / 👎.
| pub struct AuthStableIds { | ||
| pub oaicom_stable_id: Option<String>, | ||
| pub source_surface_stable_id: Option<String>, |
There was a problem hiding this comment.
Because AuthStableIds is re-exported and both fields are public, any caller can construct it with a struct literal and bypass the length/blank filtering in new; those unchecked values are later appended to OAuth authorize/token-exchange URLs and refresh bodies. A malformed or very large source_surface_stable_id from an external Rust caller can therefore make managed login or refresh fail, so keep the fields private and expose only validating constructors/accessors.
AGENTS.md reference: AGENTS.md:L88-L90
Useful? React with 👍 / 👎.
rpinkerton-oai
force-pushed
the
codex/stable-id-auth-requests
branch
from
adde7df to
b26a5ec
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b26a5ec6f9
ℹ️ 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".
| self.source_surface_stable_id.as_deref() | ||
| } | ||
|
|
||
| pub(crate) fn merge(&mut self, newer: Self) { |
There was a problem hiding this comment.
When a long-lived app-server handles multiple clients, or the current client later omits/nulls one of these optional fields, this merge never clears the cached oaicomStableId or sourceSurfaceStableId. That means account/read or account/login/start with an unavailable/currently absent ID can still cause later authorize/code-exchange/refresh requests to be tagged with a previous surface's ID, which corrupts the provenance data this change is adding. Please replace the cached pair for each request (or otherwise represent an explicit clear) instead of only overwriting Some values.
Useful? React with 👍 / 👎.
rpinkerton-oai
force-pushed
the
codex/stable-id-auth-requests
branch
from
b26a5ec to
ae1cec0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae1cec08a2
ℹ️ 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".
| if newer.oaicom_stable_id.is_some() { | ||
| self.oaicom_stable_id = newer.oaicom_stable_id; | ||
| } | ||
| if newer.source_surface_stable_id.is_some() { | ||
| self.source_surface_stable_id = newer.source_surface_stable_id; |
There was a problem hiding this comment.
When a long-lived AuthManager has already recorded an oaicom/source ID, this merge-only update keeps that value whenever a later account/read or login request sends null/omits it. Generated clients now serialize unavailable IDs as null, so a subsequent CLI/TUI or other app-server client that lacks an oaicom ID can still forward a previous client's stable ID on authorize/token-refresh requests, corrupting the provenance experiment and crossing surfaces. Treat the request's explicit absence as clearing/replacing the stored value rather than preserving the old one.
Useful? React with 👍 / 👎.
rpinkerton-oai
force-pushed
the
codex/stable-id-auth-requests
branch
from
ae1cec0 to
d8ea3e7
Compare
rpinkerton-oai
changed the title
rpinkerton-oai
force-pushed
the
codex/stable-id-auth-requests
branch
from
d8ea3e7 to
6d7bfab
Compare
rpinkerton-oai
force-pushed
the
codex/stable-id-auth-requests
branch
from
6d7bfab to
9c24041
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c24041bf1
ℹ️ 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".
| &self, | ||
| params: GetAccountParams, | ||
| ) -> Result<GetAccountResponse, JSONRPCErrorError> { | ||
| self.set_client_source_surface_stable_id(params.source_surface_stable_id); |
There was a problem hiding this comment.
In single-client stdio mode, a client can start login with sourceSurfaceStableId so the authorize and code-exchange requests use it, but a later account/read with refreshToken: true that omits this optional field deserializes to None here and clears the stored value before refresh_token_if_requested() runs. That makes the refresh request omit the source ID even though Codex still had a current value from login; preserve the existing value on omission, or distinguish omission from an explicit null if clearing is intended.
Useful? React with 👍 / 👎.
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| #[ts(optional = nullable)] | ||
| source_surface_stable_id: Option<String>, |
There was a problem hiding this comment.
This adds a v2 API payload field but also skips it whenever it is None, so Rust-generated requests and schema fixtures emit { "type": "chatgpt" } instead of carrying the nullable field shape that the v2 contract requires. Please remove the skip_serializing_if from the new nullable v2 payload fields (and the matching account/read field) so generated wire examples stay aligned with the API invariant.
AGENTS.md reference: AGENTS.md:L267-L269
Useful? React with 👍 / 👎.
| outgoing.clone(), | ||
| Arc::clone(&config), | ||
| config_manager.clone(), | ||
| matches!(rpc_transport, AppServerRpcTransport::Stdio), |
There was a problem hiding this comment.
When the server is launched on stdio with remote control enabled (or enables it later), the same processor also handles remote-control websocket connections, but this process-wide flag treats every request as single-client stdio. That lets a shared remote client send sourceSurfaceStableId (or null) on account/login/start/account/read and replace the AuthManager value used for later managed-auth refreshes or login requests; make this decision per connection origin, or keep client-supplied IDs disabled whenever remote control can add another client.
Useful? React with 👍 / 👎.
rpinkerton-oai
force-pushed
the
codex/stable-id-auth-requests
branch
from
9c24041 to
c40fc1a
Compare
|
@codex review exact current head; please focus on whether the source-surface StableID ownership is correctly scoped per connection and whether any remaining implementation can be simplified |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c40fc1a03f
ℹ️ 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".
| if !single_client_mode { | ||
| auth_manager.set_source_surface_stable_id(Some(installation_id.clone())); |
There was a problem hiding this comment.
When codex app-server --stdio also has remote control enabled or persisted, the server still calls start_remote_control later in this function, but single_client_mode is true here so the shared AuthManager is not seeded with the installation ID. RemoteControl connections cannot provide sourceSurfaceStableId, yet their managed-auth login/refresh paths read the same global AuthManager::source_surface_stable_id(), so they either send no source ID or inherit the last stdio client's ID. Seed the installation ID whenever remote-control/shared connections may be accepted, or keep the source ID on the connection/request instead of the global auth manager.
Useful? React with 👍 / 👎.
rpinkerton-oai
force-pushed
the
codex/stable-id-auth-requests
branch
from
c40fc1a to
bf656e0
Compare
|
@codex review exact current head; verify that shared and remote account-auth requests now restore the installation ID before login or refresh, while stdio preserves the App-provided source ID, and flag any remaining unnecessary complexity |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf656e0c4f
ℹ️ 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".
| let value = if accepts_client_value { | ||
| value | ||
| } else { | ||
| Some(self.installation_id.clone()) |
There was a problem hiding this comment.
In a stdio app-server that also has remote control enabled, remote/shared account requests take this branch and overwrite the shared AuthManager with the installation ID. A later stdio account/read refresh or login that omits sourceSurfaceStableId (after the App previously supplied one) will then forward the installation ID rather than preserving the App-provided source ID. Keeping this value in global auth state means the two origins can clobber each other; store it per origin/connection or restore the stdio value before stdio-managed auth requests.
Useful? React with 👍 / 👎.
rpinkerton-oai
force-pushed
the
codex/stable-id-auth-requests
branch
from
bf656e0 to
1e4491c
Compare
|
@codex review exact current head; the only change since the prior request is the required argument-name comment, but please record review against the live head |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
rpinkerton-oai
force-pushed
the
codex/stable-id-auth-requests
branch
from
1e4491c to
0f1b043
Compare
|
@codex review exact current head; verify the stdio source value is preserved separately across intervening shared or remote account-auth requests, and flag any remaining unnecessary complexity |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f1b043d9d
ℹ️ 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".
| Some(self.installation_id.clone()) | ||
| }; | ||
| if let Some(value) = value { | ||
| self.auth_manager.set_source_surface_stable_id(Some(value)); |
There was a problem hiding this comment.
In a stdio app-server that also has remote-control clients, this writes the stdio-supplied StableID into the shared AuthManager. Several account endpoints still do not receive the connection-origin flag before they call AuthManager::auth() (account/rateLimits/read, account/usage/read, and account/sendAddCreditsNudgeEmail), so if one of those remote requests triggers a proactive token refresh after a stdio request set its browser StableID, the refresh is sent with the stdio value instead of Codex's installation ID. Keep the source scoped to the request/connection or reset it for every auth-using account handler.
Useful? React with 👍 / 👎.
1 participant
Summary
sourceSurfaceStableIdfields to managed ChatGPT app-server login and account requests.The source ID is experiment-analysis context only. It does not affect authentication, authorization, or token contents, and Codex omits it when unavailable.
Project context
Stable IDs Across OpenAI Login Surfaces
This is the client counterpart to AuthAPI token provenance. The Codex App sender supplies its existing browser StableID through the new app-server field.
Test plan
just test -p codex-loginjust test -p codex-app-server-protocoljust test -p codex-app-server login_account_chatgpt_includes_forced_workspace_query_paramjust test -p codex-cli device_login_revokes_existing_auth_before_requesting_new_tokenscargo check -p codex-tui -p codex-mcp-server -p codex-cli -p codex-app-serverjust fix -p codex-login -p codex-app-server-protocol -p codex-app-server -p codex-app-server-client -p codex-app-server-test-client -p codex-cli -p codex-mcp-server -p codex-tuijust write-app-server-schemacargo fmtgit diff --check