[codex] Bind shell snapshots to retained thread environments - #28421
Conversation
| @@ -48,12 +48,6 @@ impl CodexHomeUserInstructionsProvider { | |||
| continue; | |||
| } | |||
| }; | |||
| if let Err(err) = std::str::from_utf8(&data) { | |||
There was a problem hiding this comment.
this warning wasn't worth the complexity of carrying it around
828233c to
9480a13
Compare
9480a13 to
a6b4a55
Compare
|
@codex review this |
![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: a6b4a554b7
ℹ️ 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".
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6577a4e8f6
ℹ️ 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".
| .turn_environments | ||
| .snapshot() | ||
| .await, |
There was a problem hiding this comment.
This agent logic now inherits the parent TurnEnvironmentSnapshot, but I found no core/suite test_codex coverage that spawns a subagent and verifies inherited turn environments/shell snapshots; please add it, per guidance.
Useful? React with 👍 / 👎.
| maybe_wrap_shell_lc_with_snapshot( | ||
| base_command, | ||
| session_shell.as_ref(), | ||
| shell, | ||
| shell_snapshot_location.as_ref(), |
There was a problem hiding this comment.
When unified exec targets a PowerShell turn environment while the host/session shell is not PowerShell (for example a remote Windows environment or an inherited parent environment), this path now builds the command with shell but the UTF-8 PowerShell prefix check just below still uses session_shell. Those commands will skip the PowerShell UTF-8 wrapper even though they are executed by PowerShell, unlike ShellRuntime which checks the selected environment shell.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
existing bug, will followup
2 participants
Why
Shell snapshots are currently session-scoped even though shell and cwd are properties of a selected turn environment. That makes snapshot refresh depend on separate session-cwd plumbing, prevents retained environments from retaining their snapshot work, and can make snapshot construction use a different shell than command execution.
This follows #27955 by making the retained thread-environment service own environment snapshot lifecycles. Session configuration remains the requested selection state, while
ThreadEnvironmentsremains the source of successfully resolved environments.What changed
TurnEnvironmentis built and retain that shared task while environment ID and cwd still match.TurnEnvironmentthrough shell runtimes so snapshot construction and command execution use the same environment-specific shell and cwd.SessionConfiguration; failed or duplicate resolutions only affect the resolved environment snapshot.Validation
cargo check -p codex-core --testsjust test -p codex-home instructions(6 passed)