Pair thread environment settings#26687
Conversation
| } | ||
|
|
||
| #[test] | ||
| fn thread_settings_update_params_round_trip_cwd() { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
…-environment-settings # Conflicts: # codex-rs/app-server/src/request_processors/thread_lifecycle.rs # codex-rs/app-server/src/request_processors/thread_processor.rs # codex-rs/app-server/src/request_processors/turn_processor.rs # codex-rs/core/src/session/tests.rs
…-environment-settings # Conflicts: # codex-rs/protocol/src/protocol.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc6f77e19b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…-environment-settings
anp-oai
left a comment
There was a problem hiding this comment.
LGTM, a couple of non-blocking API questions that would be IMO fine to address as follow-ups if they're worth it.
| /// Updated `cwd` for sandbox/tool calls. | ||
| pub cwd: Option<AbsolutePathBuf>, | ||
| /// Updated fallback `cwd` and environments supplied together as a complete pair. | ||
| pub environments: Option<TurnEnvironmentSelections>, |
There was a problem hiding this comment.
Are there any cases where would want to have an environment here but not a cwd, i.e. have non-optional TurnEnvironmentSelections and a cwd: Option<AbsolutePathBuf> field in the struct?
There was a problem hiding this comment.
I'd like to reduce how many combinations of option can be passed. cwd is related to local env so I'd rather everything was passed at once.
| async fn build_environment_override( | ||
| thread: &CodexThread, | ||
| cwd: Option<AbsolutePathBuf>, | ||
| environment_selections: Option<Vec<TurnEnvironmentSelection>>, |
There was a problem hiding this comment.
nit: if there's a meaningful difference between passing a None for environment_selections and passing an empty vector would be good to leave a comment, if there's no real difference maybe just pass an empty vec here when callers don't have any environments?
Why
Thread cwd and environment selections are a single logical setting in core: updating one without the other can silently desynchronize the next-turn execution context. This change makes that relationship explicit in the internal thread settings flow while preserving the existing app-server public API shape.
What changed
ThreadSettingsOverrides.environment_settingsinstead of a top-level internalcwdfield.thread/settings/updatepublic params unchanged, with app-server translating top-levelcwdinto the paired internal settings shape.Op::UserInputenvironment overrides into thread settings so user turns and settings updates use the same core path.Verification
git diff --check