Add turn-scoped environment selections#18416
Conversation
7986b94 to
8fa3ca6
Compare
127619c to
74bfde4
Compare
8fa3ca6 to
890e4cb
Compare
74bfde4 to
b1dcb7a
Compare
1578eb9 to
88861aa
Compare
b1dcb7a to
bf60062
Compare
88861aa to
0fcde7d
Compare
bf60062 to
dd5a687
Compare
d28fd65 to
7fdd192
Compare
dd5a687 to
ec3596e
Compare
7fdd192 to
fa62276
Compare
ec3596e to
8f1d61b
Compare
fa62276 to
410171c
Compare
8f1d61b to
70d3a74
Compare
af9a36d to
21807b7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82608fb065
ℹ️ 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".
| access: proc_macro2::TokenStream, | ||
| ty: &Type, | ||
| ) -> proc_macro2::TokenStream { | ||
| if let Some(inner) = option_inner(ty) { |
There was a problem hiding this comment.
ooc why did this have to change?
There was a problem hiding this comment.
weird - maybe a rebase conflict or other change i had.- let me remove
There was a problem hiding this comment.
ah this was for the experimental config stuff. removing that, I don't think we really need to gate it
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn turn_environment_selection_sets_primary_environment() { |
There was a problem hiding this comment.
up to you but I don't find these unit tests that assert internal implementation details particularly useful long-term.
| #[tokio::test] | ||
| async fn turn_environment_selection_sets_primary_environment() { | ||
| let (session, _turn_context, _rx) = make_session_and_context_with_rx().await; | ||
| let selected_cwd = |
There was a problem hiding this comment.
Are we turning AbsolutePath into pathbuf just to parse it back?
| models_manager: &ModelsManager, | ||
| network: Option<NetworkProxy>, | ||
| environment: Option<Arc<Environment>>, | ||
| environments: Option<Vec<TurnEnvironment>>, |
There was a problem hiding this comment.
should this be optional?
There was a problem hiding this comment.
Feels like it should be materialized either into an empty list or non-empty list but never None.
There was a problem hiding this comment.
we're still using the Option state to determine if env is enabled or not for tool access. will make this explicit in the later pr once this is attached to persistent thread state
| .services | ||
| .environment_manager | ||
| .get_environment(&environment_selection.environment_id) | ||
| .ok_or_else(|| codex_config::ConstraintError::InvalidValue { |
There was a problem hiding this comment.
is this the correct error type?
There was a problem hiding this comment.
I think ConstraintError::InvalidValue is for requirements.toml
| sub_id, | ||
| session_configuration, | ||
| /*final_output_json_schema*/ None, | ||
| /*turn_environments*/ None, |
There was a problem hiding this comment.
Is this correct? WebSocket prewarm uses this.
We establish a websocket connection and send an initial empty responses api request to warm up inference. That request includes tools. If we pass None environemnt here the first request will be sent without tools even for local environment which will break prewarming.
We might want to add environments onto thread/start to help the prewarm send the right request.
| &per_turn_config.to_models_manager_config(), | ||
| ) | ||
| .await; | ||
| let environment = match turn_environments.as_ref() { |
There was a problem hiding this comment.
nit: maybe grab turn_environments.first() first and then set both environment and cwd
| per_turn_config.cwd = cwd.clone(); | ||
| let plugin_outcome = self |
There was a problem hiding this comment.
nit: can we pass cwd into build_per_turn_config and have it build the correct value from the getgo?
pakrym-oai
left a comment
There was a problem hiding this comment.
Should we make environments non-optional?
3b5e90b to
154be3f
Compare
c2fbdee to
675777c
Compare
981cdd6 to
3ec48a2
Compare
b49f5c0 to
3fdd893
Compare
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>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
3fdd893 to
8dac6ca
Compare
Use the generated executable path directly in the Unix script execution test so the assertion covers shebang execution without depending on PATH lookup behavior in the test runner. Co-authored-by: Codex <noreply@openai.com>
15b61dc to
734e853
Compare
Summary
Testing
just fmtjust write-app-server-schema