[3/5] Retain the active step context#31735
Conversation
f57a336 to
11cbaf7
Compare
c40fc26 to
b607801
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b607801ef9
ℹ️ 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.emit_turn_start_lifecycle(turn_context.as_ref(), &token_usage_at_turn_start) | ||
| .await; | ||
|
|
||
| let step_context = self.capture_step_context(Arc::clone(&turn_context)).await; |
There was a problem hiding this comment.
Publish the active task before refreshing MCP
When the initial step capture has to refresh the MCP runtime, capture_step_context can start MCP clients with the Guardian elicitation reviewer before this code installs the RunningTask into active_turn. If one of those servers immediately emits an approval elicitation during startup, review_guardian_mcp_elicitation now asks for active_step_context_and_cancellation_token() and gets None, so Guardian auto-review is skipped and the request falls back to the normal elicitation path. Install/publish the task’s initial step before starting MCP refresh work, or otherwise make the active step visible before MCP clients can issue elicitations.
Useful? React with 👍 / 👎.
| self: &Arc<Self>, | ||
| turn_context: Arc<TurnContext>, | ||
| ) -> Arc<StepContext> { | ||
| let step_context = self.capture_step_context(turn_context).await; |
There was a problem hiding this comment.
Publish refreshed steps before MCP startup can elicit
On later sampling/pre-compaction boundaries, this captures the new step first and only stores it in active_turn after capture_step_context returns. If that capture refreshes MCP and a newly started server immediately sends a Guardian approval elicitation, the reviewer reads active_step_context_and_cancellation_token() while it still points at the previous sampling step, so the review can be built with stale environment/tool context for the new request. Publish the step used by the refresh before MCP clients can invoke the reviewer, or pass the captured step directly through the reviewer path.
Useful? React with 👍 / 👎.
| self.emit_turn_start_lifecycle(turn_context.as_ref(), &token_usage_at_turn_start) | ||
| .await; | ||
|
|
||
| let step_context = self.capture_step_context(Arc::clone(&turn_context)).await; |
There was a problem hiding this comment.
Avoid leaving the active turn without its task
This new await leaves active_turn populated but task unset for the duration of step capture. If another user input arrives in that window, steer_input treats it as NoActiveTurn, starts a replacement task, and the original start_task can later resume and overwrite whichever task was installed (or trip the debug assertion), leaving two spawned task futures racing with only one tracked in active_turn. Capture the initial step before publishing the empty active turn, or install a placeholder/running task state before any await that lets other submissions observe it.
Useful? React with 👍 / 👎.
11cbaf7 to
4055519
Compare
b607801 to
a369b88
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a369b88a46
ℹ️ 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 Some((step_context, _cancellation_token)) = | ||
| session.active_step_context_and_cancellation_token().await |
There was a problem hiding this comment.
Add integration coverage for retained active step
This changes the MCP elicitation review path to depend on the retained active StepContext, but the commit adds no integration coverage for the new agent behavior. The handoff now relies on refresh_env publishing the refreshed step onto the running task before an out-of-band elicitation asks Guardian for review; a regression would only show up in that async sequence, so please add a test_codex integration test that drives an active turn through step refresh and MCP elicitation review.
AGENTS.md reference: AGENTS.md:L114-L118
Useful? React with 👍 / 👎.
98bb4cd to
27064c1
Compare
a369b88 to
3e61417
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e61417eb3
ℹ️ 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".
| && task.step_context.turn.sub_id == step_context.turn.sub_id | ||
| { | ||
| task.step_context = Arc::clone(&step_context); |
There was a problem hiding this comment.
Publish only the step that is actually active
This side effect publishes every refreshed StepContext solely by matching sub_id. In the pre-turn model-switch compaction path, maybe_run_previous_model_inline_compact refreshes the previous-model step, then refreshes the current-model fallback step before calling run_auto_compact with the previous step; the second refresh leaves active_step_context_and_cancellation_token() pointing at the fallback while the primary compaction request is running. Any out-of-band MCP elicitation during that request is then reviewed against the wrong request-scoped step, defeating the active-step handoff this change adds. Consider publishing only the step about to be used, or restoring the primary step before starting the primary attempt.
Useful? React with 👍 / 👎.
3e61417 to
d884a54
Compare
27064c1 to
db2a273
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d884a54b82
ℹ️ 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(task) = active.as_mut().and_then(|turn| turn.task.as_mut()) | ||
| && task.step_context.turn.sub_id == step_context.turn.sub_id | ||
| { | ||
| task.step_context = Arc::clone(&step_context); |
There was a problem hiding this comment.
Preserve the compact request step
When previous-model auto-compaction captures a current-model fallback step before calling run_auto_compact, that fallback capture also hits this assignment because the turn id is the same. The active task then points at the fallback/current step while the compact request is still built and sent with the previous-model step_context; if that compact request's advertised MCP tools produce an elicitation, active_step_context_and_cancellation_token() will review it against the wrong MCP runtime/tool snapshot. Consider only publishing the step that is actually about to run, or avoiding publication while capturing fallback contexts.
Useful? React with 👍 / 👎.
| task, | ||
| cancellation_token, | ||
| turn_context: Arc::clone(&turn_context), | ||
| step_context, |
There was a problem hiding this comment.
Publish the refreshed step after task installation
If the spawned task gets scheduled immediately, run_turn can complete its first refresh_env() before turn.task is assigned here, so the publish in refresh_env() finds no RunningTask and is lost. The active task is then left with only the lightweight pre-refresh step while the first sampling request is using a different refreshed step; an MCP elicitation that arrives during that request will be reviewed against the stale latest_mcp_runtime/empty step data instead of the tools that were actually advertised. Install the task before the spawned future can refresh, or make this assignment use the already-refreshed step when available.
Useful? React with 👍 / 👎.
|
Migrated to https://github.com/openai/codex-internal/pull/1671 following the codex-internal cutover. Please continue review there; this public PR is superseded. |
Why
An MCP elicitation can arrive outside the sampling call stack, where only the active task is available. Guardian review needs the current request-scoped
StepContextat that boundary. The preceding source-level capture split lets every installed task retain a lightweight step without refreshing environments or starting MCP before the task is visible.Startup guardian initialization has the same handoff need: it should receive the prewarm step that will build the advertised tools.
What changed
capture_step_contextbefore installingRunningTask, retain it non-optionally, and read the task turn throughstep_context.turn.refresh_envunder the active-turn lock, with a turn-ID check after the asynchronous refresh.Stacked on #31912. This is PR 3 of 5 and does not move reasoning effort.
Validation
cargo check -p codex-core --tests --message-format shortjust test -p codex-core --lib(2,007 passed, 3 skipped at the stack head)