Conversation
There was a problem hiding this comment.
💡 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".
030dce0 to
f1a21d5
Compare
8898ed3 to
efcc15f
Compare
d5f686c to
b66bb7e
Compare
jif-oai
left a comment
There was a problem hiding this comment.
I might be wrong but I think we have a bug in here.
For example:
- an
exec_commandopen a shell - A wrapped child exec is intercepted by your code
- the new detached worker handle it (somewhere in
handle_escalate_session_with_policy) - We approve
- It reaches
command.spawn()and then thechild.wait().await - The terminal session get closed or pruned
- Approval is granted, so it reaches command.spawn() and then child.wait().await: escalate_server.rs:282-303.
- Now the terminal session is closed or pruned.
EscalationSessionis dropped whchi doesself.task.abort();- 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?
jif-oai
left a comment
There was a problem hiding this comment.
lgtm once those 2 are fixed or discussed
e0ac1e7 to
3960093
Compare
|
@jif-oai Thanks for your thoughtful comments: I think everything is addressed now? |
Why
shell_zsh_forkalready 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
EscalationSessionandEscalateServer::start_session(...)inshell-escalationso callers can get the wrapper/socket env overlay and keep the escalation server alive without immediately running a one-shot command.EscalationSession::env()andShellCommandExecutor::run(...)exchange only that env overlay, which callers must merge into their own base shell environment.coreby naming the new helper APIs aroundExecRequest, while keeping the legacyexecute_env(...)entrypoints as thin compatibility wrappers for existing callers that still use the older naming.EscalationSessionor canceling its parent request now stops intercept workers, and the server-spawned escalated child useskill_on_drop(true)so teardown cannot orphan an already-approved child.UnifiedExecBackendConfigplumbing throughToolsConfig, ashell::zsh_fork_backendfacade, and an opaque unified-exec spawn-lifecycle hook so unified exec can prepare a wrappedzsh -c/-lcrequest without storingEscalationSessiondirectly in generic process/runtime code.shell_commandzsh-fork behavior intact on top of the new bootstrap path. Tool selection is unchanged in this PR: whenshell_zsh_forkis enabled,ShellCommandstill wins overexec_command.Verification
cargo test -p codex-shell-escalationstart_session_exposes_wrapper_env_overlayexec_closes_parent_socket_after_shell_spawndropping_session_aborts_intercept_workers_and_kills_spawned_childcargo test -p codex-core shell_zsh_fork_prefers_shell_command_over_unified_execcargo test -p codex-core --test all shell_zsh_fork_prompts_for_skill_script_executionStack created with Sapling. Best reviewed with ReviewStack.