Register agent tasks behind use_agent_identity#17387
Register agent tasks behind use_agent_identity#17387adrian-openai wants to merge 12 commits intomainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f089f4a53
ℹ️ 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 let Some(agent_task) = state.agent_task() { | ||
| debug!( | ||
| agent_runtime_id = %agent_task.agent_runtime_id, | ||
| task_id = %agent_task.task_id, | ||
| "reusing cached agent task" | ||
| ); | ||
| return Ok(Some(agent_task)); |
There was a problem hiding this comment.
Revalidate cached agent task against current auth binding
ensure_agent_task_registered returns a cached agent_task without checking whether auth/workspace binding changed. After re-auth or workspace switch, turns can keep using a task minted for the old binding because register_task() (the only path that recomputes current_binding) is skipped. This can cause authorization failures or cross-account credential reuse.
Useful? React with 👍 / 👎.
cd2ed35 to
7587c88
Compare
5f089f4 to
f530190
Compare
7587c88 to
0913111
Compare
b6ea8a1 to
fd5337b
Compare
6687bb9 to
fb82a31
Compare
d25b68d to
321c151
Compare
fb82a31 to
bbeb95e
Compare
321c151 to
56cda90
Compare
bbeb95e to
5735241
Compare
3cbb522 to
c1c3c37
Compare
5735241 to
458a631
Compare
fa9d205 to
c66f085
Compare
efrazer-oai
left a comment
There was a problem hiding this comment.
I don't think we need this auth_binding concept with our auth.json changes. Can do smth like below:
match auth { CodexAuth::ChatgptAuthTokens(tokens) => { if let Some(agent_identity) = tokens.registered_agent_identity() { // preregistered path register_task_with_agent_identity(agent_identity).await } else if let Some(access_token) = tokens.authorization_bearer_token() { // human bootstrap path bootstrap_and_register_task(access_token, tokens.workspace_id()).await } else { Ok(None) } } _ => Ok(None), }
We shouldn't exist in a state where the agent workspace_id doesn't match up with the user workspace_id
There was a problem hiding this comment.
Session lifecycle
In the status quo, we fully persist session details on disk. With this change, we're creating an ephemeral 'task' in memory associated with a session. This is a bit off for a few reasons I think:
- It means if we turn off codex and start working on the same session later, it shows up as two different 'tasks' which seems like undesirable backend state.
- It means we add latency to every initial turn in order to make this HTTP call to create the task (idt it'd be too severe, but latency is a huge focus and we try to make most interactions go through websockets; if we can avoid i would).
- There are some API calls we make that are not session scoped, and some that are session scoped but aren't in the path where we create agent_task. If we're in a regime where there's no user token (i.e. programmatic codex), i believe these will just fail -- would need to make it robust to those.
I wonder if there's a cleaner implementation that loses some fidelity but just creates it on start?
Or if we can pass the session id itself as the task id?
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ 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". |
1176180 to
e0cef11
Compare
616d25c to
2fb2fc2
Compare
| @@ -0,0 +1,15 @@ | |||
| # PR 17387 Agent Task Persistence Assumption | |||
There was a problem hiding this comment.
File should probably be rm'd from commit
There was a problem hiding this comment.
Weird that notes are actually making it into the PR. I'll remove that.
There was a problem hiding this comment.
Should there be a way to allow users to soft fail after registration fails even when features.use_agent_identity is enabled? Calling shutdown if registration fails, even by accident, might result in some grotty UX. For instance, shutdown calls abort_all_tasks(TurnAbortReason::Interrupted), which iiuc will abort all active session tasks even if they're unrelated to this session.
There was a problem hiding this comment.
This is a fair point. Probably worth a better UX here, where there's an error for this thread, but doesn't necessarily stop all other threads.
| @@ -6391,6 +6373,11 @@ pub(crate) async fn run_turn( | |||
| })) | |||
| .await; | |||
| } | |||
| if let Err(error) = sess.ensure_agent_task_registered().await { | |||
| warn!(error = %error, "agent task registration failed"); | |||
| sess.fail_agent_identity_registration(error).await; | |||
There was a problem hiding this comment.
Could we treat task registration fails as a turn error that can be retried and let the next turn attempt registration again instead of ending the session?
There was a problem hiding this comment.
I'm not sure if I'm following - how will that turn be completed without a task? Do you mean that this 'turn error' just returns control back to the user who can try again in case it is a transient error? I think that seems reasonable!
There was a problem hiding this comment.
Yep the latter! The turn shouldn't be completed but also shouldn't cause a hard fail, and the user should be able to attempt a retry
There was a problem hiding this comment.
The other thing we might want to consider is have some progress loader/event so if this event hangs the user has some indicator that it's still doing the registration
|
Most of my comments (other than the shutdown invocation) are pretty minor, otherwise looking in good shape! |
0e37c8f to
49f5037
Compare
5c0a4ec to
9b2c16f
Compare
## Summary Stack PR 2 of 4 for feature-gated agent identity support. This PR adds agent identity registration behind `features.use_agent_identity`. It keeps the app-server protocol unchanged and starts registration after ChatGPT auth exists rather than requiring a client restart. ## Stack - PR1: #17385 - add `features.use_agent_identity` - PR2: #17386 - this PR - PR3: #17387 - register agent tasks when enabled - PR4: #17388 - 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.
09bfb36 to
4ec295b
Compare
4ec295b to
84e7fb9
Compare
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
features.use_agent_identityAgentAssertiondownstream when enabledValidation
Covered as part of the local stack validation pass:
just fmtcargo test -p codex-core --lib agent_identitycargo test -p codex-core --lib agent_assertioncargo test -p codex-core --lib websocket_agent_taskcargo test -p codex-api api_bridgecargo build -p codex-cli --bin codexNotes
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.