Conversation
5f81440 to
9fb1355
Compare
f0cc39b to
0bb0c48
Compare
0401532 to
50ab543
Compare
3c3bedc to
a3821f1
Compare
…6972) This updates `ExecParams` so that instead of taking `timeout_ms: Option<u64>`, it now takes a more general cancellation mechanism, `ExecExpiration`, which is an enum that includes a `Cancellation(tokio_util::sync::CancellationToken)` variant. If the cancellation token is fired, then `process_exec_tool_call()` returns in the same way as if a timeout was exceeded. This is necessary so that in #6973, we can manage the timeout logic external to the `process_exec_tool_call()` because we want to "suspend" the timeout when an elicitation from a human user is pending. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/6972). * #7005 * #6973 * __->__ #6972
…l timeout (#6973) Previously, we were running into an issue where we would run the `shell` tool call with a timeout of 10s, but it fired an elicitation asking for user approval, the time the user took to respond to the elicitation was counted agains the 10s timeout, so the `shell` tool call would fail with a timeout error unless the user is very fast! This PR addresses this issue by introducing a "stopwatch" abstraction that is used to manage the timeout. The idea is: - `Stopwatch::new()` is called with the _real_ timeout of the `shell` tool call. - `process_exec_tool_call()` is called with the `Cancellation` variant of `ExecExpiration` because it should not manage its own timeout in this case - the `Stopwatch` expiration is wired up to the `cancel_rx` passed to `process_exec_tool_call()` - when an elicitation for the `shell` tool call is received, the `Stopwatch` pauses - because it is possible for multiple elicitations to arrive concurrently, it keeps track of the number of "active pauses" and does not resume until that counter goes down to zero I verified that I can test the MCP server using `@modelcontextprotocol/inspector` and specify `git status` as the `command` with a timeout of 500ms and that the elicitation pops up and I have all the time in the world to respond whereas previous to this PR, that would not have been possible. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/6973). * #7005 * __->__ #6973 * #6972
596b330 to
8c0fb09
Compare
336da53 to
9df5baf
Compare
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".
| - runner: macos-13 | ||
| target: x86_64-apple-darwin | ||
| variant: macos-13 |
There was a problem hiding this comment.
macOS 13 arm64 Bash artifact missing
The reusable build only produces a macOS 13 Bash binary for x86_64-apple-darwin (matrix lines 239-241). However the launcher uses selectDarwinBash to pick the macos-13 variant whenever os.release() reports Darwin 22 (macOS 13) regardless of CPU, and resolveTargetTriple returns aarch64-apple-darwin on Apple Silicon. On macOS 13 arm64 hosts this workflow never builds vendor/aarch64-apple-darwin/bash/macos-13/bash, so mcp-server.js will throw “Required binary missing” at startup on that platform. Please add an arm64 macOS 13 build or adjust selection to avoid targeting an absent binary.
Useful? React with 👍 / 👎.
This adds a GitHub workflow for building a new npm module we are experimenting with that contains an MCP server for running Bash commands. The new workflow,
shell-tool-mcp, is a dependency of the generalreleaseworkflow so that we continue to use one version number for all artifacts across the project in one GitHub release..github/workflows/shell-tool-mcp.ymlis the primary workflow introduced by this PR, which does the following:codex-exec-mcp-serverandcodex-execve-wrapperexecutables for both arm64 and x64 versions of Mac and Linux (preferring the MUSL version for Linux)shell-tool-mcp/patches/bash-exec-wrapper.patch:debian-11debian-12ubuntu-20.04ubuntu-22.04ubuntu-24.04centos-9macos-13(x64 only)macos-14(arm64 only)macos-15(arm64 only)shell-tool-mcp/folder, which createsbin/mcp-server.jsshell-tool-mcp/vendor/folder;bin/mcp-server.jsdoes a runtime check to determine which ones to executenpm packto create the.tgzfor the modulepublish: trueis set, invokes thenpm publishcall with the.tgzThe justification for building Bash for so many different operating systems is because, since it is dynamically linked, we want to increase our confidence that the version we build is compatible with the glibc whatever OS we end up running on. (Note this is less of a concern with
codex-exec-mcp-serverandcodex-execve-wrapperon Linux, as they are statically linked.)This PR also introduces the code for the npm module in
shell-tool-mcp/(the proposed module name is@openai/codex-shell-tool-mcp). Initially, I intended the module to be a single file of vanilla JavaScript (likecodex-cli/bin/codex.js), but some of the logic seemed a bit tricky, so I decided to port it to TypeScript and add unit tests.shell-tool-mcp/src/index.tsdefines themain()function for the module, which performs runtime checks to determine the clang triple to find the path to the Rust executables within thevendor/folder (resolveTargetTriple()). It uses a combination ofreadOsRelease()andresolveBashPath()to determine the correct Bash executable to run in the environment. Ultimately, it spawns a command like the following:Note
.github/workflows/shell-tool-mcp-ci.ymldefines a fairly standard CI job for the module (format/build/test).To test this PR, I pushed this branch to my personal fork of Codex and ran the CI job there:
https://github.com/bolinfest/codex/actions/runs/19564311320
Admittedly, the graph looks a bit wild now:
But when it finished, I was able to download
codex-shell-tool-mcp-npmfrom the Artifacts for the workflow in an empty temp directory, unzip the.zipand then the.tgzinside it, followed byxattr -rc .to remove the quarantine bits. Then I ran:which launched the MCP Inspector and I was able to use it as expected! This bodes well that this should work once the package is published to npm:
Also, to verify the package contains what I expect:
/tmp/foobar4/package$ tree . ├── bin │ └── mcp-server.js ├── package.json ├── README.md └── vendor ├── aarch64-apple-darwin │ ├── bash │ │ ├── macos-14 │ │ │ └── bash │ │ └── macos-15 │ │ └── bash │ ├── codex-exec-mcp-server │ └── codex-execve-wrapper ├── aarch64-unknown-linux-musl │ ├── bash │ │ ├── centos-9 │ │ │ └── bash │ │ ├── debian-11 │ │ │ └── bash │ │ ├── debian-12 │ │ │ └── bash │ │ ├── ubuntu-20.04 │ │ │ └── bash │ │ ├── ubuntu-22.04 │ │ │ └── bash │ │ └── ubuntu-24.04 │ │ └── bash │ ├── codex-exec-mcp-server │ └── codex-execve-wrapper ├── x86_64-apple-darwin │ ├── bash │ │ └── macos-13 │ │ └── bash │ ├── codex-exec-mcp-server │ └── codex-execve-wrapper └── x86_64-unknown-linux-musl ├── bash │ ├── centos-9 │ │ └── bash │ ├── debian-11 │ │ └── bash │ ├── debian-12 │ │ └── bash │ ├── ubuntu-20.04 │ │ └── bash │ ├── ubuntu-22.04 │ │ └── bash │ └── ubuntu-24.04 │ └── bash ├── codex-exec-mcp-server └── codex-execve-wrapper 26 directories, 26 files