[codex] simplify shell snapshot ownership#27756
Conversation
![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: 75b42ead5b
ℹ️ 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".
| if self.services.shell_snapshot.load().is_some() { | ||
| self.services.shell_snapshot.store(Some(ShellSnapshot::new( |
There was a problem hiding this comment.
This replaces the stored shell snapshot on cwd updates, but the integration suite has no coverage for cwd refresh or child inheritance. Because this is agent-visible behavior, the repo guidance requires an integration test/list of behaviors. guidance
Useful? React with 👍 / 👎.
| let shell_snapshot = ctx.session.services.shell_snapshot.load_full(); | ||
| let shell_snapshot_location = shell_snapshot | ||
| .as_ref() | ||
| .and_then(|snapshot| snapshot.location(&req.cwd)); |
There was a problem hiding this comment.
When a local unified exec is still starting and the session cwd is changed, the refresh path replaces services.shell_snapshot and drops the old snapshot file immediately. This local Arc is dropped as soon as run() returns, while the live UnifiedExecProcess does not retain it, so the wrapper can silently skip sourcing the snapshot and run with a bare environment. Keep the loaded snapshot alive with the process or spawn lifecycle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We do not update cwd mid turn.
| } | ||
| session_telemetry.counter("codex.shell_snapshot", /*inc*/ 1, &counter_tags); | ||
| let _ = shell_snapshot_tx.send(snapshot.ok()); | ||
| if let Ok(snapshot) = snapshot { |
There was a problem hiding this comment.
It leaves failed generations looking inheritable... On an err, load_full() still returns Some. So child init skips its own capture but this receiver can never produce a file. This is low frequency but it looks like a regression. Can we represent failure separately? Or fall back to a child capture? (no strong opinion)
| .session | ||
| .services | ||
| .shell_snapshot | ||
| .load_full() |
There was a problem hiding this comment.
Doesn’t this also inherit failed generations? load_full() stays Some($) after capture fails, so the child skips its own attempt and never retries.
Could we only inherit a ready snapshot?
|
Updated |
2 participants
Why
Shell snapshot lifecycle state was split between
ShellandSessionServices:Shellcarried the receiver while session code exposed and forwarded the raw sender. That coupled shell identity to mutable snapshot state and made refresh, inheritance, and file lifetime harder to reason about.What changed
Arc<ShellSnapshot>represent one cwd-specific snapshot generationSessionServiceswithArcSwapOptionShellSnapshotFileownership to delete snapshot files automaticallyShellArcwhile they are in useTest plan
cargo check -p codex-core --libjust test -p codex-core 'shell_snapshot::tests'just test -p codex-core shell_command_snapshot_still_intercepts_apply_patchjust test -p codex-core shell_snapshot_deleted_after_shutdown_with_skills