Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| let ExecveWrapperCli { file, argv } = ExecveWrapperCli::parse(); | ||
| escalate_client::run(file, argv).await?; | ||
| Ok(()) |
There was a problem hiding this comment.
Propagate exec wrapper exit codes
The new main_execve_wrapper drops the exit code returned by escalate_client::run and always returns Ok(()), so any command that is escalated or denied now exits with status 0 instead of the real command/denial code. When the exec policy escalates or blocks a command, the shell will treat the operation as a success, changing control flow and hiding policy enforcement. The wrapper needs to exit with the code from escalate_client::run (as the prior Escalate subcommand did) to preserve the caller’s semantics.
Useful? React with 👍 / 👎.
This PR reorganizes things slightly so that:
codex-exec-server, we now have two executables:codex-exec-mcp-serverto launch the MCP servercodex-execve-wrapperis theexecve(2)wrapper to use with theBASH_EXEC_WRAPPERenvironment variableBASH_EXEC_WRAPPERmust be a single executable: it cannot be a command string composed of an executable with args (i.e., it no longer adds theescalatesubcommand, as before)codex-exec-mcp-servertakes--bashand--execveas options. Though if--execveis not specified, the MCP server will check the directory containingstd::env::current_exe()and attempt to use the file namedcodex-execve-wrapperwithin it. In development, this works out since these executables are side-by-side in thetarget/debugfolder.With respect to testing, this also fixes an important bug in
dummy_exec_policy(), as I was usingends_with()as if it applied to aString, but in this case, it is used with a&Path, so the semantics are slightly different.Putting this all together, I was able to test this by running the following:
If I try to run
git statusin/Users/mbolin/code/codexvia theshelltool from the MCP server:then I get prompted with the following elicitation, as expected:
Though a current limitation is that the
shelltool defaults to a timeout of 10s, which means I only have 10s to respond to the elicitation. Ideally, the time spent waiting for a response from a human should not count against the timeout for the command execution. I will address this in a subsequent PR.Note
~/code/bash/bashwas created by doing:The patch: