Skip to content

refactor: prepare unified exec for zsh-fork backend#13392

Merged
bolinfest merged 1 commit intomainfrom
pr13392
Mar 5, 2026
Merged

refactor: prepare unified exec for zsh-fork backend#13392
bolinfest merged 1 commit intomainfrom
pr13392

Conversation

@bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Mar 3, 2026

Why

shell_zsh_fork already provides stronger guarantees around which executables receive elevated permissions. To reuse that machinery from unified exec without pushing Unix-specific escalation details through generic runtime code, the escalation bootstrap and session lifetime handling need a cleaner boundary.

That boundary also needs to be safe for long-lived sessions: when an intercepted shell session is closed or pruned, any in-flight approval workers and any already-approved escalated child they spawned must be torn down with the session, and the inherited escalation socket must not leak into unrelated subprocesses.

What Changed

  • Extracted a reusable EscalationSession and EscalateServer::start_session(...) in shell-escalation so callers can get the wrapper/socket env overlay and keep the escalation server alive without immediately running a one-shot command.
  • Documented that EscalationSession::env() and ShellCommandExecutor::run(...) exchange only that env overlay, which callers must merge into their own base shell environment.
  • Clarified the prepared-exec helper boundary in core by naming the new helper APIs around ExecRequest, while keeping the legacy execute_env(...) entrypoints as thin compatibility wrappers for existing callers that still use the older naming.
  • Added a small post-spawn hook on the prepared execution path so the parent copy of the inheritable escalation socket is closed immediately after both the existing one-shot shell-command spawn and the unified-exec spawn.
  • Made session teardown explicit with session-scoped cancellation: dropping an EscalationSession or canceling its parent request now stops intercept workers, and the server-spawned escalated child uses kill_on_drop(true) so teardown cannot orphan an already-approved child.
  • Added UnifiedExecBackendConfig plumbing through ToolsConfig, a shell::zsh_fork_backend facade, and an opaque unified-exec spawn-lifecycle hook so unified exec can prepare a wrapped zsh -c/-lc request without storing EscalationSession directly in generic process/runtime code.
  • Kept the existing shell_command zsh-fork behavior intact on top of the new bootstrap path. Tool selection is unchanged in this PR: when shell_zsh_fork is enabled, ShellCommand still wins over exec_command.

Verification

  • cargo test -p codex-shell-escalation
    • includes coverage for start_session_exposes_wrapper_env_overlay
    • includes coverage for exec_closes_parent_socket_after_shell_spawn
    • includes coverage for dropping_session_aborts_intercept_workers_and_kills_spawned_child
  • cargo test -p codex-core shell_zsh_fork_prefers_shell_command_over_unified_exec
  • cargo test -p codex-core --test all shell_zsh_fork_prompts_for_skill_script_execution

Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest changed the title refactor: prepare unified exec for zsh-fork backend refactor: add unified-exec zsh-fork backend plumbing Mar 3, 2026
Copy link
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f18ba2858e

ℹ️ 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".

@bolinfest bolinfest force-pushed the pr13392 branch 2 times, most recently from 030dce0 to f1a21d5 Compare March 3, 2026 23:56
@bolinfest bolinfest force-pushed the pr13392 branch 3 times, most recently from 8898ed3 to efcc15f Compare March 4, 2026 05:59
@bolinfest bolinfest changed the title refactor: add unified-exec zsh-fork backend plumbing refactor: prepare unified exec for zsh-fork backend Mar 4, 2026
@bolinfest bolinfest force-pushed the pr13392 branch 2 times, most recently from d5f686c to b66bb7e Compare March 4, 2026 06:24
@bolinfest bolinfest requested a review from jif-oai March 4, 2026 06:34
Copy link
Collaborator

@jif-oai jif-oai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong but I think we have a bug in here.

For example:

  1. an exec_command open a shell
  2. A wrapped child exec is intercepted by your code
  3. the new detached worker handle it (somewhere in handle_escalate_session_with_policy)
  4. We approve
  5. It reaches command.spawn() and then the child.wait().await
  6. The terminal session get closed or pruned
  7. Approval is granted, so it reaches command.spawn() and then child.wait().await: escalate_server.rs:282-303.
  8. Now the terminal session is closed or pruned.
  9. EscalationSession is dropped whchi does self.task.abort();
  10. this does not closed the per-intercept worker

As a result, we have an "orphan" process running that can't be cancelled as not attached to a unified_exec session

What do you think?

Copy link
Collaborator

@jif-oai jif-oai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm once those 2 are fixed or discussed

@bolinfest bolinfest force-pushed the pr13392 branch 5 times, most recently from e0ac1e7 to 3960093 Compare March 5, 2026 07:45
@bolinfest
Copy link
Collaborator Author

@jif-oai Thanks for your thoughtful comments: I think everything is addressed now?

@bolinfest bolinfest enabled auto-merge (squash) March 5, 2026 08:07
@bolinfest bolinfest merged commit b4cb989 into main Mar 5, 2026
61 of 70 checks passed
@bolinfest bolinfest deleted the pr13392 branch March 5, 2026 08:55
@github-actions github-actions bot locked and limited conversation to collaborators Mar 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants