[4/5] Propagate step context through execution paths#31736
Conversation
aibrahim-oai
force-pushed
the
step-context/active-step
branch
from
b17eb83 to
c40fc26
Compare
aibrahim-oai
force-pushed
the
step-context/propagate-step
branch
2 times, most recently
from
c1b75f0 to
fcc9c54
Compare
aibrahim-oai
force-pushed
the
step-context/active-step
branch
from
c40fc26 to
b607801
Compare
|
@codex review |
![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: fcc9c54d70
ℹ️ 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".
| .as_ref() | ||
| .map(|owner_call| owner_call.step_context.clone()) | ||
| else { | ||
| return NetworkDecision::deny(REASON_NOT_ALLOWED); |
There was a problem hiding this comment.
When Guardian is enabled and resolve_request_attribution returns no owner_call (for example, a managed-network request tagged only with an environment id and no execution attribution), this early return happens after get_or_create_pending_approval(...) has registered the host key. The owner request returns deny without setting a PendingApprovalDecision or removing pending_host_approvals, so any concurrent or subsequent request for the same host will block forever in pending.wait_for_decision(). Set/remove the pending entry, or fall back to the non-Guardian approval path, before returning.
Useful? React with 👍 / 👎.
| let turn_context = &step_context.turn; | ||
| let turn_environment = match args.environment_id.as_deref() { | ||
| Some(environment_id) => turn_context | ||
| .environments |
There was a problem hiding this comment.
Now that this path receives a StepContext, looking up the target environment through step_context.turn.environments ignores the dynamic snapshot captured for the request. With DeferredExecutor enabled, a delegated child can request permissions for an environment id/cwd that exists in step_context.environments but not in the turn-frozen list, causing this helper to return the default empty permission response or build the request from stale environment data.
Useful? React with 👍 / 👎.
| subagent_source: SubAgentSource, | ||
| initial_history: Option<InitialHistory>, | ||
| ) -> Result<Codex, CodexErr> { | ||
| let parent_ctx = step_context.turn.as_ref(); |
There was a problem hiding this comment.
This aliases the step down to only its turn, and the Codex::spawn arguments below still inherit parent_ctx.environments/to_selections(). In sessions where the step captured a refreshed environment snapshot, delegated reviewers or sub-Codex tasks started from this request will run against the old turn-frozen environments rather than the environments/tools that produced the request.
Useful? React with 👍 / 👎.
| let mut config = build_agent_spawn_config( | ||
| &session.get_base_instructions().await, | ||
| step_context.as_ref(), | ||
| )?; |
There was a problem hiding this comment.
The spawn config now comes from step_context, but the SpawnAgentOptions below still pass turn.environments.to_selections(). When the active step has a newer environment snapshot, agents created by this tool start in the stale turn environment instead of the one used for the tool call; the v2 spawn and agent-job spawn paths need the same step-environment handoff.
Useful? React with 👍 / 👎.
aibrahim-oai
changed the title
aibrahim-oai
force-pushed
the
step-context/active-step
branch
from
b607801 to
a369b88
Compare
aibrahim-oai
force-pushed
the
step-context/propagate-step
branch
from
fcc9c54 to
8d52444
Compare
aibrahim-oai
changed the title
|
@codex review |
aibrahim-oai
force-pushed
the
step-context/propagate-step
branch
from
8d52444 to
842106d
Compare
aibrahim-oai
force-pushed
the
step-context/active-step
branch
from
a369b88 to
3e61417
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/core/src/codex_delegate.rs
Lines 122 to 126 in 8d52444
Use the step snapshot when spawning delegates
When the deferred executor refreshes environments, step_context.environments can differ from the frozen step_context.turn.environments; this still seeds subagents and guardian reviewers from parent_ctx.environments, so a delegated agent spawned after an environment change starts with stale environment selections and cannot operate on the same snapshot whose tools were advertised for the parent step. Use the environments from the passed StepContext for both inherited environments and selections.
ℹ️ 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(environment_id) => turn_context | ||
| .environments |
There was a problem hiding this comment.
For permission requests forwarded with a StepContext, this lookup still walks turn_context.environments, which is stale after a deferred-executor environment refresh; a delegated request for an environment that exists in the current step but not in the turn snapshot falls through to the default empty permission response. The lookup should use step_context.environments so approvals are evaluated against the same environment snapshot that produced the request.
Useful? React with 👍 / 👎.
| let (new_history, world_state_baseline) = process_compacted_history( | ||
| sess.as_ref(), | ||
| compaction_turn_context.as_ref(), | ||
| step_context.as_ref(), | ||
| new_history, |
There was a problem hiding this comment.
When previous-model remote compaction falls back after an InvalidRequest, attempt and compaction_turn_context can come from fallback_step_context, but this call now still processes and persists context from the original step_context. That reinjects the old model/settings into the replacement history and reference TurnContextItem, so the next request or resumed rollout can see stale model-visible context after a successful fallback; keep the step paired with the successful attempt (the v2 path has the same substitution).
AGENTS.md reference: AGENTS.md:L91-L100
Useful? React with 👍 / 👎.
| let Some(step_context) = owner_call | ||
| .as_ref() | ||
| .map(|owner_call| owner_call.step_context.clone()) | ||
| else { | ||
| return NetworkDecision::deny(REASON_NOT_ALLOWED); |
There was a problem hiding this comment.
When guardian review is enabled and the network proxy reports a blocked request that is attributable only to the active turn (no execution_id/owner call), this new else returns not_allowed instead of asking guardian, even though the non-guardian branch below and GuardianApprovalRequest::NetworkAccess { trigger: None } both support that case. This regresses session-level/background proxy requests that previously could be reviewed and approved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 842106df92
ℹ️ 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".
| else { | ||
| return NetworkDecision::deny(REASON_NOT_ALLOWED); |
There was a problem hiding this comment.
When guardian routing is enabled and an inline network policy request reaches this branch without an attributable owner_call (for example, an environment-scoped request with no matching single active call), this early return denies the current request after get_or_create_pending_approval has inserted a pending entry, but it never calls pending.set_decision(...) or removes key from pending_host_approvals. A later request for the same host becomes a non-owner and waits on that unresolved pending approval indefinitely; use the same cleanup/decision path as the other denial branches before returning.
Useful? React with 👍 / 👎.
| subagent_source: SubAgentSource, | ||
| initial_history: Option<InitialHistory>, | ||
| ) -> Result<Codex, CodexErr> { | ||
| let parent_ctx = step_context.turn.as_ref(); |
There was a problem hiding this comment.
When DeferredExecutor is enabled, step_context.environments is refreshed for the exact sampling request, but this immediately collapses the new StepContext back to step_context.turn and the spawn args below still fill inherited_environments and environment_selections from parent_ctx.environments. Delegated review/subagent sessions can therefore start with the turn’s stale environment/cwd instead of the environment snapshot used to advertise and execute the parent tool call; pass step_context.environments into those spawn fields.
Useful? React with 👍 / 👎.
| step_context: Arc<StepContext>, | ||
| arguments: String, | ||
| ) -> Result<FunctionToolOutput, FunctionCallError> { | ||
| let turn = step_context.turn.clone(); |
There was a problem hiding this comment.
This clones the turn out of the new StepContext, and the rest of the CSV job path still uses turn.environments to choose the CSV cwd and to seed worker environment selections. In a deferred-executor step where the request snapshot has a different or newly-ready environment, spawn_agents_on_csv will read from the stale turn cwd or reject the job as not exactly one local environment even though the model saw the refreshed step; use step_context.environments for the cwd check and worker spawn selections.
Useful? React with 👍 / 👎.
aibrahim-oai
force-pushed
the
step-context/active-step
branch
from
3e61417 to
d884a54
Compare
aibrahim-oai
force-pushed
the
step-context/propagate-step
branch
from
842106d to
9f8d7de
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f8d7de0e7
ℹ️ 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) = owner_call | ||
| .as_ref() | ||
| .map(|owner_call| owner_call.step_context.clone()) | ||
| else { | ||
| return NetworkDecision::deny(REASON_NOT_ALLOWED); |
There was a problem hiding this comment.
When guardian review is active but a policy request cannot be attributed to an owner call, this new early return exits after get_or_create_pending_approval inserted key but before pending.set_decision(...) and before removing it from pending_host_approvals. A later request for the same host/environment will find the stale pending entry, take the !is_owner path, and wait forever in wait_for_decision() instead of receiving the denial. Please set a deny decision and remove the pending approval before returning, or route the request with an active step context.
Useful? React with 👍 / 👎.
| let mut config = build_agent_spawn_config( | ||
| &session.get_base_instructions().await, | ||
| step_context.as_ref(), | ||
| )?; |
There was a problem hiding this comment.
When DeferredExecutor refreshes environments for the current request, the model-visible world state is built from step_context.environments, but this spawn path only uses the step for the child config and still passes turn.environments.to_selections() in SpawnAgentOptions below. In a turn where a remote/attached environment becomes ready after the turn snapshot, spawn_agent creates a child without the environments the parent just saw, so the child can run in the wrong/default environment or miss the requested one. Please pass the step snapshot's selections into the spawn options.
Useful? React with 👍 / 👎.
| subagent_source: SubAgentSource, | ||
| initial_history: Option<InitialHistory>, | ||
| ) -> Result<Codex, CodexErr> { | ||
| let parent_ctx = step_context.turn.as_ref(); |
There was a problem hiding this comment.
This converts the delegate API to accept a StepContext, but immediately reduces it to step_context.turn; the CodexSpawnArgs below still inherit parent_ctx.environments and parent_ctx.environments.to_selections(). For review/guardian/delegated subagent sessions started after a step refresh, the child session therefore loses the exact environment snapshot that was advertised and used for the parent request. Please use step_context.environments for inherited environments and selections.
Useful? React with 👍 / 👎.
|
Migrated to https://github.com/openai/codex-internal/pull/1672 following the codex-internal cutover. Please continue review there; this public PR is superseded. |
1 participant
Why
Approvals, delegated agents, compaction, and execution runtimes need to carry the same request snapshot that selected environments, MCP state, and tools. Passing both a turn and a step through these paths duplicates context and makes it possible for asynchronous work to use a different snapshot from the request that produced it.
What changed
TurnContextparameters and stored fields withStepContextthrough guardian, delegated-agent, compaction, approval, tool, unified-exec, and shell-escalation paths.step_context.turnwhere turn-level data is still required.StepContextcarries the exact MCP snapshot, removing the former latest-MCP forwarding wrapper.Stacked on #31735. This is PR 4 of 5. Reasoning effort remains on
TurnContext; all existing raw-versus-effective reads are preserved.Validation
cargo check -p codex-core --tests --message-format short