Conversation
126a88a to
58b5b99
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58b5b99d2b
ℹ️ 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".
| features.enabled(Feature::ShellTool) | ||
| && features.enabled(Feature::UnifiedExec) | ||
| && features.enabled(Feature::ShellZshFork) | ||
| && features.enabled(Feature::UnifiedExecZshFork) |
There was a problem hiding this comment.
Keep zsh-fork composition off remote execs
When all three flags are enabled, this makes TurnContext.unified_exec_shell_mode globally ZshFork for the whole turn, not just local environments. Any exec_command targeting a remote turn environment then goes through the zsh-fork path; UnifiedExecRuntime explicitly rejects that path for remote environments ("unified_exec zsh-fork is not supported for remote environments"), so sessions with a remote environment and these rollout flags will have their unified exec commands fail instead of using the remote exec backend. Gate the zsh-fork mode per selected environment or fall back to Direct for remote execs.
Useful? React with 👍 / 👎.
| ConfigShellToolType::Disabled | ||
| } else if features.enabled(Feature::ShellZshFork) { | ||
| } else if features.enabled(Feature::ShellZshFork) | ||
| && !unified_exec_uses_zsh_fork_for_features(features) |
There was a problem hiding this comment.
Preserve explicit shell selection in composition mode
With the composition flag enabled for a zsh user, this path exposes exec_command while UnifiedExecShellMode::ZshFork is selected for the whole session. That mode ignores the tool's documented shell argument and always builds a zsh -c/-lc command, so requests such as { "cmd": "...", "shell": "/bin/bash" } silently run under zsh instead of the requested shell. Either fall back to direct unified exec when shell names a non-zsh shell, reject the request, or stop advertising the shell parameter for this mode.
Useful? React with 👍 / 👎.
Why
shell_zsh_forkand unified exec need to remain independently controllable for enterprise rollouts, but we also need a third mode that composes them. That composed mode is intended to preserve unified exec command lifecycle support while letting the zsh fork provide more accurateexecv(2)interception.Enabling
unified_exec_zsh_forkby itself is intentionally not sufficient. It is a composition gate, not a dependency-enabling shortcut:unified_execselects the PTY-backed unified exec tool.shell_zsh_forkopts into the zsh fork backend.unified_exec_zsh_forkonly allows those two already-enabled modes to be composed so local zsh unified exec commands can launch through the zsh fork.This separation is deliberate. Enterprises and staged rollouts must be able to enable or disable unified exec and zsh-fork independently. If
unified_exec_zsh_forkimplied either dependency, then enabling one under-development composition flag would silently activate a shell backend that the configured feature set left disabled.This PR introduces only the configuration and planning gate for that composition. Existing
shell_zsh_forkbehavior continues to use the standalone shell tool unless the new composition feature is explicitly enabled alongside both dependencies.What Changed
unified_exec_zsh_fork.UnifiedExecFeatureModeso the three input feature flags collapse intoDisabled,Direct, orZshForkmode before tool planning.unified_exec,shell_zsh_fork, andunified_exec_zsh_fork.shell_zsh_forkis enabled.Verification
shell_zsh_forkremains standalone until composition is explicitly enabled.codex-features,codex-tools, and the affectedcodex-coreplanner case.Stack created with Sapling. Best reviewed with ReviewStack.