Add sticky environment API and thread state#18897
Conversation
ee1a5f1 to
c3f1adb
Compare
3fdd893 to
8dac6ca
Compare
e04ef64 to
6edcdf2
Compare
c8c8e86 to
47d2133
Compare
25decb8 to
606663d
Compare
b810587 to
a578544
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 606663dfae
ℹ️ 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".
a578544 to
309cfc8
Compare
352cf40 to
ca56b45
Compare
45a8abf to
28231ef
Compare
Allow thread/start to configure sticky environment selections that are used by turns when no per-turn override is supplied. Per-turn environments continue to take precedence, while omitted thread selections preserve the existing default behavior. Co-authored-by: Codex <noreply@openai.com>
Cover sticky thread environment selections and turn-level overrides through the app-server v2 thread/start and turn/start JSON-RPC flow. The matrix mirrors the manual smoke cases for omitted, empty, local, remote, and local plus remote selections. Co-authored-by: Codex <noreply@openai.com>
Group thread-start options for lint-friendly callsites and update generated v2 schema for sticky environment selections. Co-authored-by: Codex <noreply@openai.com>
Introduce a core-owned EnvironmentSelection type so app-server converts API environment params at the core boundary instead of passing protocol operation structs through session/thread state. Rename the internal sticky field from environment_selections to environments to match the v2 API shape. Co-authored-by: Codex <noreply@openai.com>
Reject named turn and sticky environments when environment access is disabled, while preserving explicit empty environment selections as no-environment turns. Validate sticky environment state before storing it so invalid selections cannot poison future turns. Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Resume and fork should not recover thread environment overrides from persisted rollout state; sticky environment selection stays scoped to the live thread/session configuration. Co-authored-by: Codex <noreply@openai.com>
Remove the duplicate SessionConfiguration environments initializer left by the rebase. Co-authored-by: Codex <noreply@openai.com>
Avoid requiring TurnEnvironment to implement PartialEq in the resume/fork test that verifies sticky environments are not restored from rollout history. Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Move default thread environment selection into a shared helper, validate explicit environment IDs at spawn, and resolve selections only when constructing turns. This keeps app-server optionality at the API edge while preserving sticky thread state as explicit selections. Co-authored-by: Codex <noreply@openai.com>
Use the EnvironmentManager default environment id when materializing default thread selections, carry environment ids through resolved turn environments, and make subagents inherit the parent turn environment list instead of re-defaulting. Keep turn-local environment overrides on SessionSettingsUpdate so turn context updates flow through one settings payload. Co-authored-by: Codex <noreply@openai.com>
Keep the effective turn environment selections on TurnContext so child agents can inherit selections directly without rebuilding them from resolved environments. Route app-server validation through ThreadManager and preserve turn-scoped selections for agent job workers. Co-authored-by: Codex <noreply@openai.com>
ce102d0 to
a1ec341
Compare
Remove the unused resolved turn environment id field now that effective selections are stored separately, and collapse app-server environment validation guards for clippy. Co-authored-by: Codex <noreply@openai.com>
| pub(crate) struct SpawnAgentOptions { | ||
| pub(crate) fork_parent_spawn_call_id: Option<String>, | ||
| pub(crate) fork_mode: Option<SpawnAgentForkMode>, | ||
| pub(crate) environments: Option<Vec<TurnEnvironmentSelection>>, |
There was a problem hiding this comment.
nit: non optional? non blocking
There was a problem hiding this comment.
this one still need Option for the 'use defaults' mode, which a few other callsites still use (e.g. memories/phase2.rs)
| reasoning_summary, | ||
| session_source, | ||
| environment: parent_turn_context.environment.clone(), | ||
| environment_selections: parent_turn_context.environment_selections.clone(), |
There was a problem hiding this comment.
nit: why are we storing both environment_selections and environments? should we store only materialized list?
| /// Turn-local environment override. This is consumed while building a turn | ||
| /// context and does not mutate the sticky thread environments stored on | ||
| /// `SessionConfiguration`. | ||
| pub(crate) environments: TurnEnvironmentOverride, |
There was a problem hiding this comment.
Should this be Option<> ? like other fields/
There was a problem hiding this comment.
good call - swapped to Option, which means we can drop what was previously an enum class serving pretty much the same purpose
There was a problem hiding this comment.
(codex used an enum class to be more explicit on the two modes, but Option<> is more consistent with how we're using env everywehre else)
| sub_id, | ||
| session_configuration, | ||
| updates.final_output_json_schema, | ||
| effective_environments, |
There was a problem hiding this comment.
doesn't feel like we should put effective_environments on turn considering we have the entire resolved list.
| let mut state = self.state.lock().await; | ||
| match state.session_configuration.clone().apply(&updates) { | ||
| Ok(next) => { | ||
| let effective_environments = |
There was a problem hiding this comment.
Not sure I understand what is happening here.
In theory state.session_configuration.clone().apply(&updates) will apply updates to the config and return the new environment list as part of next. Why do we need to resolve_ref here ?
There was a problem hiding this comment.
^ that's just the thread level settings - don't we still need to apply the turn level overrides?
Store the selected environment id on each resolved turn environment so turn context does not need to carry a parallel selection list. Sub-agent inheritance now projects selections from the materialized turn environments. Co-authored-by: Codex <noreply@openai.com>
Use Option<Vec<TurnEnvironmentSelection>> directly for turn-local environment overrides. None inherits sticky thread environments, while Some([]) explicitly disables environments for the turn. Co-authored-by: Codex <noreply@openai.com>
Summary
Stack
Validation