[codex] Use AgentAssertion downstream behind use_agent_identity#17980
[codex] Use AgentAssertion downstream behind use_agent_identity#17980adrian-openai merged 4 commits intomainfrom
Conversation
efrazer-oai
left a comment
There was a problem hiding this comment.
I had Codex go find all the places that are still hardcoded to use Bearer as discussed on Slack, here are the results:
As it stands, we have successfully migrated the shared Responses HTTP, Responses websocket, compact, and ChatGPT-backend realtime-call paths.
But there are still several other backend surfaces that build Authorization: Bearer ... directly or go through helpers that only understand bearer tokens. Those still need to migrate if we want this change to be exhaustive.
Shared backend client paths that still need the new auth shape:
codex-rs/backend-client/src/client.rs/wham/usage/wham/tasks/list/wham/tasks/{id}/wham/tasks/{id}/turns/{turn_id}/sibling_turns/wham/config/requirementsPOST /wham/tasks
Current callers of that shared client include:
codex-rs/cloud-requirements/src/lib.rscodex-rs/app-server/src/codex_message_processor.rscodex-rs/cloud-tasks-client/src/http.rs
Direct backend callers that still attach bearer auth themselves:
codex-rs/codex-api/src/files.rs/files/files/{id}/uploaded
codex-rs/core/src/mcp_openai_file.rscodex-rs/models-manager/src/manager.rs/codex/models
codex-rs/codex-mcp/src/mcp/mod.rs.../wham/apps
codex-rs/chatgpt/src/chatgpt_client.rs- shared GET helper used by:
codex-rs/chatgpt/src/get_task.rs- connector directory loading paths
- shared GET helper used by:
codex-rs/core/src/connectors.rs- connector directory requests
codex-rs/core/src/plugins/remote.rs/plugins/list/plugins/featured/plugins/{id}/enable/plugins/{id}/uninstall
codex-rs/core-skills/src/remote.rs/hazelnuts/hazelnuts/{skill_id}/export
codex-rs/analytics/src/client.rs/codex/analytics-events/events
codex-rs/core/src/arc_monitor.rs/codex/safety/arc
codex-rs/app-server/src/transport/remote_control/enroll.rs/wham/remote/control/server/enroll
codex-rs/app-server/src/transport/remote_control/websocket.rs- websocket handshake for
/wham/remote/control/server
- websocket handshake for
codex-rs/cloud-tasks/src/util.rscodex-rs/cloud-tasks/src/env_detect.rs/wham/environments/wham/environments/by-repo/...
One backend hit I found that does not look like part of this auth migration is:
codex-rs/core/src/plugins/startup_sync.rs- backup archive fetch at
/backend-api/plugins/export/curated
- backup archive fetch at
So the main point is: this PR fixes the shared model path, but it does not yet migrate all of the places where we talk to codex-backend.
ed316a9 to
d1373d4
Compare
dfd9aa0 to
688aad3
Compare
| .map_err(|error| format!("failed to read ChatGPT auth for file upload: {error}"))?; | ||
| CoreAuthProvider::from_bearer_token(Some(token_data.access_token), token_data.account_id) | ||
| }; | ||
| let uploaded = upload_local_file( |
There was a problem hiding this comment.
Will file upload with ChatGPT auth be affected if the above code block fails?
## Summary Stack PR3 for feature-gated agent identity support. This PR adds per-thread agent task registration behind `features.use_agent_identity`. Tasks are minted on the first real user turn and cached in thread runtime state for later turns. ## Stack - PR1: #17385 - add `features.use_agent_identity` - PR2: #17386 - register agent identities when enabled - PR3: #17387 - this PR, original task registration slice - PR3.1: #17978 - persist and prewarm registered tasks per thread - PR4: #17980 - use `AgentAssertion` downstream when enabled ## Validation Covered as part of the local stack validation pass: - `just fmt` - `cargo test -p codex-core --lib agent_identity` - `cargo test -p codex-core --lib agent_assertion` - `cargo test -p codex-core --lib websocket_agent_task` - `cargo test -p codex-api api_bridge` - `cargo build -p codex-cli --bin codex` ## Notes The full local app-server E2E path is still being debugged after PR creation. The current branch stack is directionally ready for review while that follow-up continues.
ea5ec0a to
56f4b38
Compare
3dcdb8d to
67cff31
Compare
| use tokio::time::timeout; | ||
|
|
||
| const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10); | ||
| const DEFAULT_TIMEOUT: Duration = Duration::from_secs(20); |
There was a problem hiding this comment.
hm maybe we should drop this? I assume it has to do with windows test flake?
There was a problem hiding this comment.
Yes! The test is flaky locally as a result of the 10s.
| if let Some(token) = auth.bearer_token() { | ||
| request = request.bearer_auth(token); | ||
| if let Some(authorization) = auth.authorization_header_value() { | ||
| request = request.header(AUTHORIZATION, authorization); |
There was a problem hiding this comment.
This would enable us to use non bearer token (agent identity) for as the auth header vs bearer_auth is for bearer token only I assume.
| let shell_command = vec![ | ||
| "powershell".to_string(), | ||
| "-Command".to_string(), | ||
| "Start-Sleep -Seconds 1".to_string(), | ||
| "cmd.exe".to_string(), | ||
| "/d".to_string(), | ||
| "/c".to_string(), | ||
| "ping".to_string(), | ||
| "-n".to_string(), | ||
| "2".to_string(), | ||
| "127.0.0.1".to_string(), | ||
| ]; |
There was a problem hiding this comment.
No action: I think this is updated/different on latest main already.
| pub struct ModelClientSession { | ||
| client: ModelClient, | ||
| websocket_session: WebsocketSession, | ||
| agent_task: Option<RegisteredAgentTask>, |
There was a problem hiding this comment.
Do we need this? agent_task is scoped to a session (thread) and here we are on the top level?
There was a problem hiding this comment.
The field is in ModelClientSession,
- The registered agent task is owned by the Codex thread/session state.
- But each model turn needs to carry that task into all Responses requests made during that turn: HTTP stream, websocket prewarm, retries, continuation requests.
- Putting it on ModelClient would be wrong because ModelClient is longer-lived and shared for the Codex session.
- Passing it into every lower-level request method would be more error-prone.
- Storing it on the per-turn ModelClientSession also lets the code avoid reusing a cached bearer-auth websocket when an AgentAssertion task is present.
Let me know if you disagree with that ^
| let token_data = auth | ||
| .get_token_data() | ||
| .map_err(|error| format!("failed to read ChatGPT auth for file upload: {error}"))?; | ||
| let upload_auth = CoreAuthProvider { | ||
| token: Some(token_data.access_token), | ||
| account_id: token_data.account_id, |
There was a problem hiding this comment.
Did not realize that we have so many locally built headers....
shijie-oai
left a comment
There was a problem hiding this comment.
Makes sense to me overall.
e6f7ce3 to
0000662
Compare
68a45b4 to
c6d377f
Compare
2d2fd76 to
044222d
Compare
Summary
This is the AgentAssertion downstream slice for feature-gated agent identity support, replacing the oversized AgentAssertion slice from PR #17807.
It isolates task-scoped downstream AgentAssertion wiring on top of the merged PR3.1 work without re-carrying the earlier agent registration, task registration, or task-state history.
This PR includes the task-scoped bug-fix call sites from the review: generic file upload auth, MCP OpenAI file upload auth, and ARC monitor auth. Broader user/control-plane calls move to PR4.1 and PR4.2.
Stack
features.use_agent_identityAgentAssertiondownstream when enabledAgentAssertionauthWhat Changed
codex-coreWhy
The original PR had drifted ancestry and showed a much larger diff than the semantic change actually required. Restacking it onto PR3.1 keeps the reviewable surface down to the downstream assertion slice.
Validation
just fmtcargo check -p codex-core -p codex-login -p codex-analytics -p codex-app-server -p codex-cloud-requirements -p codex-cloud-tasks -p codex-models-manager -p codex-chatgpt -p codex-model-provider -p codex-mcp -p codex-core-skillscargo test -p codex-model-provider bearer_auth_providercargo test -p codex-core agent_assertioncargo test -p codex-app-server remote_controlcargo test -p codex-cloud-requirements fetch_cloud_requirementscargo test -p codex-models-manager manager::testscargo test -p codex-chatgptcargo test -p codex-cloud-taskscargo test -p codex-login agent_identityjust fix -p codex-core -p codex-login -p codex-analytics -p codex-app-server -p codex-cloud-requirements -p codex-cloud-tasks -p codex-models-manager -p codex-chatgpt -p codex-model-provider -p codex-mcp -p codex-core-skillsjust fix -p codex-app-servergit diff --check