[codex] migrate environment context to model world state - #29249
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2058d91cd
ℹ️ 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".
9e39952 to
e2058d9
Compare
6665363 to
9fddcad
Compare
| .values() | ||
| .all(|update| matches!(update, EnvironmentUpdate::Current(_))); | ||
| (!updates.is_empty() || turn_context_values_changed).then(|| { | ||
| ContextualUserFragment::into(RenderedEnvironments { |
There was a problem hiding this comment.
This doesn’t distinguish unchanged, changed, and removed value s
An environment-only update re-renders the full date/network/fs state, while a Some -> None transition merely omits the element and never prompt the model to clear
| /// A snapshot of the model-visible world with one section per concrete type. | ||
| #[derive(Default)] | ||
| pub(crate) struct WorldState { | ||
| sections: IndexMap<TypeId, Box<dyn ErasedWorldStateSection>>, |
There was a problem hiding this comment.
The design talks about a stable structured snapshots and merge patches that can be replayed after resume/ fork/rollback. This is not possible yet but this is planned right? I'm a bit scared of this TypeId that is not serialized anywhere now tbh
There was a problem hiding this comment.
It is planned, yes, didn't want to cram everything into one PR.
| // inputs or add explicit replay events so fork/resume can diff everything | ||
| // deterministically. | ||
| let contextual_user_message = build_environment_update_item(previous, next, shell); | ||
| let world_state_updates = previous |
There was a problem hiding this comment.
This only evaluates world state through turn-boundary settings updates
We need this a higher grannularity no?
There was a problem hiding this comment.
Yes, we'll do the injection on per-step basis.
| .await; | ||
| contextual_user_sections.push( | ||
| crate::context::EnvironmentContext::from_turn_context(turn_context, shell.as_ref()) | ||
| crate::context::EnvironmentsState::from_turn_context(turn_context) |
There was a problem hiding this comment.
Can initial and post-compaction context render through the same WorldStates builder used for diffs?
123ad8a to
92cad21
Compare
92cad21 to
f9b60c8
Compare
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dd9bc50c0
ℹ️ 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 mut updates = self | ||
| .environments | ||
| .iter() | ||
| .filter(|(id, environment)| { | ||
| previous | ||
| .environments | ||
| .get(*id) | ||
| .is_none_or(|previous| !environment.has_same_diff_value(previous)) | ||
| }) | ||
| .map(|(id, environment)| (id.clone(), EnvironmentUpdate::Current(environment.clone()))) | ||
| .collect::<BTreeMap<_, _>>(); |
There was a problem hiding this comment.
When a session initially renders a single environment, the model sees the legacy <cwd>/<shell> form with no environment id. If a later turn or deferred-executor snapshot adds another environment while the original environment is unchanged, this filter emits only the new/changed environment, so the model enters a multi-environment state without ever learning the id for the pre-existing environment and cannot reliably target it in environment-scoped tool calls. Include the unchanged existing environment when diffing from a legacy-single render into a multi-environment render.
AGENTS.md reference: AGENTS.md:L92-L101
Useful? React with 👍 / 👎.
| current_date: self.current_date.clone(), | ||
| timezone: self.timezone.clone(), | ||
| network: self.network.clone(), | ||
| filesystem: self.filesystem.clone(), |
There was a problem hiding this comment.
For an environment-only diff, such as a DeferredExecutor environment moving from starting to ready, turn_context_values_changed is false but the emitted fragment still carries the current date, timezone, network, and full filesystem profile. That re-appends unchanged static context every time an environment changes; with many workspace roots or permission entries this can add a large repeated prompt fragment and cause avoidable cache misses, so these fields should only render on full state or when they changed.
AGENTS.md reference: AGENTS.md:L96-L100
Useful? React with 👍 / 👎.
| let world_state_baseline = if reference_context_item.is_some() { | ||
| Some(self.build_world_state(turn_context).await) |
There was a problem hiding this comment.
When mid-turn compaction injects full initial context, the replacement items were rendered earlier by build_initial_context, but this recomputes the baseline from a fresh snapshot. If a DeferredExecutor environment changes from starting to ready in that window, the stored baseline can say the model already saw the ready state even though the compacted history still contains the starting state, so the subsequent environment diff is suppressed. Pass through the same WorldState that rendered the inserted context instead of rebuilding it here.
AGENTS.md reference: AGENTS.md:L92-L101
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
more generically, compaction uses the potentially stale environment snapshot from TurnContext. seems like we need to recapture an environment snapshot so we inject the right stuff postcompaction.
follow up?
There was a problem hiding this comment.
Yep, followup. We need to have compaction use step context.
| } | ||
| } | ||
|
|
||
| impl ContextualUserFragment for EnvironmentsState { |
There was a problem hiding this comment.
why does EnvironmentsState also impl the fragment? it seems like we render via RenderedEnvironments?
Why
Environment context is model-visible state, but it is currently assembled from transient turn values and diffed through environment-specific paths. That makes initial injection, turn-to-turn updates, and changes that happen within a turn use different baselines.
This PR introduces the smallest useful model world-state slice: environments only, with one in-memory baseline and one renderer for full state and diffs.
What changed
WorldStatecontainer whose sections render fragments relative to an optional previous value. Full rendering uses the same diff path with no previous state.EnvironmentContextrepresentation with anEnvironmentsStatesection keyed by environment ID and rendered in deterministic order.WorldStateonContextManagerand use it for both turn-boundary and mid-turn environment diffs.TurnContextItemwhen resuming an existing rollout; the world state itself is not serialized.Known limitation
A legacy
TurnContextItemonly reconstructs the primary environment aslocal; it cannot faithfully recover a remote-primary environment ID after resume. Live state uses the exact environment IDs once a complete baseline is established.Test plan
just test -p codex-core world_statejust test -p codex-core record_context_updatesjust test -p codex-core deferred_executor_just test -p codex-core build_initial_contextjust test -p codex-core rollout_reconstructionjust test -p codex-core process_compacted_history_reinjects_full_initial_context