mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix: keep zsh-fork release assets after removing shell-tool-mcp (#15644)
## Why `shell-tool-mcp` and the Bash fork are no longer needed, but the patched zsh fork is still relevant for shell escalation and for the DotSlash-backed zsh-fork integration tests. Deleting the old `shell-tool-mcp` workflow also deleted the only pipeline that rebuilt those patched zsh binaries. This keeps the package removal, while preserving a small release path that can be reused whenever `codex-rs/shell-escalation/patches/zsh-exec-wrapper.patch` changes. ## What changed - removed the `shell-tool-mcp` workspace package, its npm packaging/release jobs, the Bash test fixture, and the remaining Bash-specific compatibility wiring - deleted the old `.github/workflows/shell-tool-mcp.yml` and `.github/workflows/shell-tool-mcp-ci.yml` workflows now that their responsibilities have been replaced or removed - kept the zsh patch under `codex-rs/shell-escalation/patches/zsh-exec-wrapper.patch` and updated the `codex-rs/shell-escalation` docs/code to describe the zsh-based flow directly - added `.github/workflows/rust-release-zsh.yml` to build only the three zsh binaries that `codex-rs/app-server/tests/suite/zsh` needs today: - `aarch64-apple-darwin` on `macos-15` - `x86_64-unknown-linux-musl` on `ubuntu-24.04` - `aarch64-unknown-linux-musl` on `ubuntu-24.04` - extracted the shared zsh build/smoke-test/stage logic into `.github/scripts/build-zsh-release-artifact.sh`, made that helper directly executable, and now invoke it directly from the workflow so the Linux and macOS jobs only keep the OS-specific setup in YAML - wired those standalone `codex-zsh-*.tar.gz` assets into `rust-release.yml` and added `.github/dotslash-zsh-config.json` so releases also publish a `codex-zsh` DotSlash file - updated the checked-in `codex-rs/app-server/tests/suite/zsh` fixture comments to explain that new releases come from the standalone zsh assets, while the checked-in fixture remains pinned to the latest historical release until a newer zsh artifact is published - tightened a couple of follow-on cleanups in `codex-rs/shell-escalation`: the `ExecParams::command` comment now describes the shell `-c`/`-lc` string more clearly, and the README now points at the same `git.code.sf.net` zsh source URL that the workflow uses ## Testing - `cargo test -p codex-shell-escalation` - `just argument-comment-lint` - `bash -n .github/scripts/build-zsh-release-artifact.sh` - attempted `cargo test -p codex-core`; unrelated existing failures remain, but the touched `tools::runtimes::shell::unix_escalation::*` coverage passed during that run
This commit is contained in:
Michael Bolin
committed by
GitHub
Unverified
GitHub
parent
363b373979
commit
e89e5136bd
@@ -1,75 +0,0 @@
|
||||
#!/usr/bin/env dotslash
|
||||
|
||||
// This is an instance of the fork of Bash that we bundle with
|
||||
// https://www.npmjs.com/package/@openai/codex-shell-tool-mcp.
|
||||
// Fetching the prebuilt version via DotSlash makes it easier to write
|
||||
// integration tests for shell execution flows.
|
||||
//
|
||||
// TODO(mbolin): Currently, we use a .tgz artifact that includes binaries for
|
||||
// multiple platforms, but we could save a bit of space by making arch-specific
|
||||
// artifacts available in the GitHub releases and referencing those here.
|
||||
{
|
||||
"name": "codex-bash",
|
||||
"platforms": {
|
||||
// macOS 13 builds (and therefore x86_64) were dropped in
|
||||
// https://github.com/openai/codex/pull/7295, so we only provide an
|
||||
// Apple Silicon build for now.
|
||||
"macos-aarch64": {
|
||||
"size": 37003612,
|
||||
"hash": "blake3",
|
||||
"digest": "d9cd5928c993b65c340507931c61c02bd6e9179933f8bf26a548482bb5fa53bb",
|
||||
"format": "tar.gz",
|
||||
"path": "package/vendor/aarch64-apple-darwin/bash/macos-15/bash",
|
||||
"providers": [
|
||||
{
|
||||
"url": "https://github.com/openai/codex/releases/download/rust-v0.65.0/codex-shell-tool-mcp-npm-0.65.0.tgz"
|
||||
},
|
||||
{
|
||||
"type": "github-release",
|
||||
"repo": "openai/codex",
|
||||
"tag": "rust-v0.65.0",
|
||||
"name": "codex-shell-tool-mcp-npm-0.65.0.tgz"
|
||||
}
|
||||
]
|
||||
},
|
||||
// Note the `musl` parts of the Linux paths are misleading: the Bash
|
||||
// binaries are actually linked against `glibc`, but the
|
||||
// `codex-execve-wrapper` that invokes them is linked against `musl`.
|
||||
"linux-x86_64": {
|
||||
"size": 37003612,
|
||||
"hash": "blake3",
|
||||
"digest": "d9cd5928c993b65c340507931c61c02bd6e9179933f8bf26a548482bb5fa53bb",
|
||||
"format": "tar.gz",
|
||||
"path": "package/vendor/x86_64-unknown-linux-musl/bash/ubuntu-24.04/bash",
|
||||
"providers": [
|
||||
{
|
||||
"url": "https://github.com/openai/codex/releases/download/rust-v0.65.0/codex-shell-tool-mcp-npm-0.65.0.tgz"
|
||||
},
|
||||
{
|
||||
"type": "github-release",
|
||||
"repo": "openai/codex",
|
||||
"tag": "rust-v0.65.0",
|
||||
"name": "codex-shell-tool-mcp-npm-0.65.0.tgz"
|
||||
}
|
||||
]
|
||||
},
|
||||
"linux-aarch64": {
|
||||
"size": 37003612,
|
||||
"hash": "blake3",
|
||||
"digest": "d9cd5928c993b65c340507931c61c02bd6e9179933f8bf26a548482bb5fa53bb",
|
||||
"format": "tar.gz",
|
||||
"path": "package/vendor/aarch64-unknown-linux-musl/bash/ubuntu-24.04/bash",
|
||||
"providers": [
|
||||
{
|
||||
"url": "https://github.com/openai/codex/releases/download/rust-v0.65.0/codex-shell-tool-mcp-npm-0.65.0.tgz"
|
||||
},
|
||||
{
|
||||
"type": "github-release",
|
||||
"repo": "openai/codex",
|
||||
"tag": "rust-v0.65.0",
|
||||
"name": "codex-shell-tool-mcp-npm-0.65.0.tgz"
|
||||
}
|
||||
]
|
||||
},
|
||||
}
|
||||
}
|
||||
@@ -1,13 +1,14 @@
|
||||
#!/usr/bin/env dotslash
|
||||
|
||||
// This is the patched zsh fork built by
|
||||
// `.github/workflows/shell-tool-mcp.yml` for the shell-tool-mcp package.
|
||||
// This is the patched zsh fork corresponding to
|
||||
// `codex-rs/shell-escalation/patches/zsh-exec-wrapper.patch`.
|
||||
// Fetching the prebuilt version via DotSlash makes it easier to write
|
||||
// integration tests that exercise the zsh fork behavior in app-server tests.
|
||||
//
|
||||
// TODO(mbolin): Currently, we use a .tgz artifact that includes binaries for
|
||||
// multiple platforms, but we could save a bit of space by making arch-specific
|
||||
// artifacts available in the GitHub releases and referencing those here.
|
||||
// This checked-in fixture is still pinned to the latest released bundle that
|
||||
// contains this binary. New releases publish standalone `codex-zsh-*.tar.gz`
|
||||
// assets plus a generated `codex-zsh` DotSlash release asset, so this file can
|
||||
// be retargeted when a newer fork build needs to be exercised in tests.
|
||||
{
|
||||
"name": "codex-zsh",
|
||||
"platforms": {
|
||||
|
||||
@@ -899,7 +899,7 @@ impl ShellCommandExecutor for CoreShellCommandExecutor {
|
||||
let mut exec_env = self.env.clone();
|
||||
// `env_overlay` comes from `EscalationSession::env()`, so merge only the
|
||||
// wrapper/socket variables into the base shell environment.
|
||||
for var in ["CODEX_ESCALATE_SOCKET", "EXEC_WRAPPER", "BASH_EXEC_WRAPPER"] {
|
||||
for var in ["CODEX_ESCALATE_SOCKET", "EXEC_WRAPPER"] {
|
||||
if let Some(value) = env_overlay.get(var) {
|
||||
exec_env.insert(var.to_string(), value.clone());
|
||||
}
|
||||
|
||||
@@ -15,14 +15,15 @@ decision to the shell-escalation protocol over a shared file descriptor (specifi
|
||||
- `Deny`: the server has declared the proposed command to be forbidden, so
|
||||
`codex-execve-wrapper` prints an error to `stderr` and exits with `1`.
|
||||
|
||||
## Patched Bash
|
||||
## Patched zsh
|
||||
|
||||
We carry a small patch to `execute_cmd.c` (see `patches/bash-exec-wrapper.patch`) that adds support for `EXEC_WRAPPER`. The original commit message is “add support for BASH_EXEC_WRAPPER” and the patch applies cleanly to `a8a1c2fac029404d3f42cd39f5a20f24b6e4fe4b` from https://github.com/bminor/bash. To rebuild manually:
|
||||
We carry a small patch to `Src/exec.c` (see `patches/zsh-exec-wrapper.patch`) that adds support for `EXEC_WRAPPER`. The patch applies to `77045ef899e53b9598bebc5a41db93a548a40ca6` from https://git.code.sf.net/p/zsh/code. To rebuild manually:
|
||||
|
||||
```bash
|
||||
git clone https://git.savannah.gnu.org/git/bash
|
||||
git checkout a8a1c2fac029404d3f42cd39f5a20f24b6e4fe4b
|
||||
git apply /path/to/patches/bash-exec-wrapper.patch
|
||||
./configure --without-bash-malloc
|
||||
git clone https://git.code.sf.net/p/zsh/code
|
||||
git checkout 77045ef899e53b9598bebc5a41db93a548a40ca6
|
||||
git apply /path/to/patches/zsh-exec-wrapper.patch
|
||||
./Util/preconfig
|
||||
./configure
|
||||
make -j"$(nproc)"
|
||||
```
|
||||
|
||||
@@ -0,0 +1,34 @@
|
||||
diff --git a/Src/exec.c b/Src/exec.c
|
||||
index 27bca11..baea760 100644
|
||||
--- a/Src/exec.c
|
||||
+++ b/Src/exec.c
|
||||
@@ -507,7 +507,9 @@ zexecve(char *pth, char **argv, char **newenvp)
|
||||
{
|
||||
int eno;
|
||||
static char buf[PATH_MAX * 2+1];
|
||||
- char **eep;
|
||||
+ char **eep, **exec_argv;
|
||||
+ char *orig_pth = pth;
|
||||
+ char *exec_wrapper;
|
||||
|
||||
unmetafy(pth, NULL);
|
||||
for (eep = argv; *eep; eep++)
|
||||
@@ -526,8 +528,17 @@ zexecve(char *pth, char **argv, char **newenvp)
|
||||
|
||||
if (newenvp == NULL)
|
||||
newenvp = environ;
|
||||
+ exec_argv = argv;
|
||||
+ if ((exec_wrapper = getenv("EXEC_WRAPPER")) &&
|
||||
+ *exec_wrapper && !inblank(*exec_wrapper)) {
|
||||
+ exec_argv = argv - 2;
|
||||
+ exec_argv[0] = exec_wrapper;
|
||||
+ exec_argv[1] = orig_pth;
|
||||
+ pth = exec_wrapper;
|
||||
+ }
|
||||
winch_unblock();
|
||||
- execve(pth, argv, newenvp);
|
||||
+ execve(pth, exec_argv, newenvp);
|
||||
+ pth = orig_pth;
|
||||
|
||||
/* If the execve returns (which in general shouldn't happen), *
|
||||
* then check for an errno equal to ENOEXEC. This errno is set *
|
||||
@@ -11,7 +11,6 @@ use crate::unix::escalate_protocol::EXEC_WRAPPER_ENV_VAR;
|
||||
use crate::unix::escalate_protocol::EscalateAction;
|
||||
use crate::unix::escalate_protocol::EscalateRequest;
|
||||
use crate::unix::escalate_protocol::EscalateResponse;
|
||||
use crate::unix::escalate_protocol::LEGACY_BASH_EXEC_WRAPPER_ENV_VAR;
|
||||
use crate::unix::escalate_protocol::SuperExecMessage;
|
||||
use crate::unix::escalate_protocol::SuperExecResult;
|
||||
use crate::unix::socket::AsyncDatagramSocket;
|
||||
@@ -46,12 +45,7 @@ pub async fn run_shell_escalation_execve_wrapper(
|
||||
.await
|
||||
.context("failed to send handshake datagram")?;
|
||||
let env = std::env::vars()
|
||||
.filter(|(k, _)| {
|
||||
!matches!(
|
||||
k.as_str(),
|
||||
ESCALATE_SOCKET_ENV_VAR | EXEC_WRAPPER_ENV_VAR | LEGACY_BASH_EXEC_WRAPPER_ENV_VAR
|
||||
)
|
||||
})
|
||||
.filter(|(k, _)| !matches!(k.as_str(), ESCALATE_SOCKET_ENV_VAR | EXEC_WRAPPER_ENV_VAR))
|
||||
.collect();
|
||||
client
|
||||
.send(EscalateRequest {
|
||||
|
||||
@@ -13,9 +13,6 @@ pub const ESCALATE_SOCKET_ENV_VAR: &str = "CODEX_ESCALATE_SOCKET";
|
||||
/// Patched shells use this to wrap exec() calls.
|
||||
pub const EXEC_WRAPPER_ENV_VAR: &str = "EXEC_WRAPPER";
|
||||
|
||||
/// Compatibility alias for older patched bash builds.
|
||||
pub const LEGACY_BASH_EXEC_WRAPPER_ENV_VAR: &str = "BASH_EXEC_WRAPPER";
|
||||
|
||||
/// The client sends this to the server to request an exec() call.
|
||||
#[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq)]
|
||||
pub struct EscalateRequest {
|
||||
|
||||
@@ -20,7 +20,6 @@ use crate::unix::escalate_protocol::EscalateRequest;
|
||||
use crate::unix::escalate_protocol::EscalateResponse;
|
||||
use crate::unix::escalate_protocol::EscalationDecision;
|
||||
use crate::unix::escalate_protocol::EscalationExecution;
|
||||
use crate::unix::escalate_protocol::LEGACY_BASH_EXEC_WRAPPER_ENV_VAR;
|
||||
use crate::unix::escalate_protocol::SuperExecMessage;
|
||||
use crate::unix::escalate_protocol::SuperExecResult;
|
||||
use crate::unix::escalation_policy::EscalationPolicy;
|
||||
@@ -64,13 +63,13 @@ pub trait ShellCommandExecutor: Send + Sync {
|
||||
|
||||
#[derive(Debug, serde::Deserialize, serde::Serialize)]
|
||||
pub struct ExecParams {
|
||||
/// The the string of Zsh/shell to execute.
|
||||
/// The command string to pass to the shell via `-c` or `-lc`.
|
||||
pub command: String,
|
||||
/// The working directory to execute the command in. Must be an absolute path.
|
||||
pub workdir: String,
|
||||
/// The timeout for the command in milliseconds.
|
||||
pub timeout_ms: Option<u64>,
|
||||
/// Launch Bash with -lc instead of -c: defaults to true.
|
||||
/// Launch the shell with -lc instead of -c: defaults to true.
|
||||
pub login: Option<bool>,
|
||||
}
|
||||
|
||||
@@ -126,18 +125,18 @@ impl Drop for EscalationSession {
|
||||
}
|
||||
|
||||
pub struct EscalateServer {
|
||||
bash_path: PathBuf,
|
||||
shell_path: PathBuf,
|
||||
execve_wrapper: PathBuf,
|
||||
policy: Arc<dyn EscalationPolicy>,
|
||||
}
|
||||
|
||||
impl EscalateServer {
|
||||
pub fn new<Policy>(bash_path: PathBuf, execve_wrapper: PathBuf, policy: Policy) -> Self
|
||||
pub fn new<Policy>(shell_path: PathBuf, execve_wrapper: PathBuf, policy: Policy) -> Self
|
||||
where
|
||||
Policy: EscalationPolicy + Send + Sync + 'static,
|
||||
{
|
||||
Self {
|
||||
bash_path,
|
||||
shell_path,
|
||||
execve_wrapper,
|
||||
policy: Arc::new(policy),
|
||||
}
|
||||
@@ -153,7 +152,7 @@ impl EscalateServer {
|
||||
let env_overlay = session.env().clone();
|
||||
let client_socket = Arc::clone(&session.client_socket);
|
||||
let command = vec![
|
||||
self.bash_path.to_string_lossy().to_string(),
|
||||
self.shell_path.to_string_lossy().to_string(),
|
||||
if params.login == Some(false) {
|
||||
"-c".to_string()
|
||||
} else {
|
||||
@@ -211,10 +210,6 @@ impl EscalateServer {
|
||||
EXEC_WRAPPER_ENV_VAR.to_string(),
|
||||
self.execve_wrapper.to_string_lossy().to_string(),
|
||||
);
|
||||
env.insert(
|
||||
LEGACY_BASH_EXEC_WRAPPER_ENV_VAR.to_string(),
|
||||
self.execve_wrapper.to_string_lossy().to_string(),
|
||||
);
|
||||
Ok(EscalationSession {
|
||||
env,
|
||||
task,
|
||||
@@ -595,7 +590,7 @@ mod tests {
|
||||
/// overlay and does not need to touch the configured shell or wrapper
|
||||
/// executable paths.
|
||||
///
|
||||
/// The `/bin/bash` and `/tmp/codex-execve-wrapper` values here are
|
||||
/// The `/bin/zsh` and `/tmp/codex-execve-wrapper` values here are
|
||||
/// intentionally fake sentinels: this test asserts that the paths are
|
||||
/// copied into the exported environment and that the socket fd stays valid
|
||||
/// until `close_client_socket()` is called.
|
||||
@@ -605,7 +600,7 @@ mod tests {
|
||||
let execve_wrapper = PathBuf::from("/tmp/codex-execve-wrapper");
|
||||
let execve_wrapper_str = execve_wrapper.to_string_lossy().to_string();
|
||||
let server = EscalateServer::new(
|
||||
PathBuf::from("/bin/bash"),
|
||||
PathBuf::from("/bin/zsh"),
|
||||
execve_wrapper.clone(),
|
||||
DeterministicEscalationPolicy {
|
||||
decision: EscalationDecision::run(),
|
||||
@@ -618,10 +613,6 @@ mod tests {
|
||||
)?;
|
||||
let env = session.env();
|
||||
assert_eq!(env.get(EXEC_WRAPPER_ENV_VAR), Some(&execve_wrapper_str));
|
||||
assert_eq!(
|
||||
env.get(LEGACY_BASH_EXEC_WRAPPER_ENV_VAR),
|
||||
Some(&execve_wrapper_str)
|
||||
);
|
||||
let socket_fd = env
|
||||
.get(ESCALATE_SOCKET_ENV_VAR)
|
||||
.expect("session should export shell escalation socket");
|
||||
|
||||
Reference in New Issue
Block a user