Compare commits

...

8 Commits

Author SHA1 Message Date
Michael Bolin
991dff837e fix: stop using actions/cache@v4 for release builds 2025-08-28 23:20:21 -07:00
Michael Bolin
65636802f7 fix: drop Mutexes earlier in MCP server (#2878) 2025-08-28 22:50:16 -07:00
Michael Bolin
c988ce28fe fix: drop Mutex before calling tx_approve.send() (#2876) 2025-08-28 22:49:29 -07:00
Michael Bolin
cb2f952143 fix: remove unnecessary flush() calls (#2873)
Because we are writing to a pipe, these `flush()` calls are unnecessary,
so removing these saves us one syscall per write in these two cases.
2025-08-28 22:41:10 -07:00
Jeremy Rose
7d734bff65 suggest just fix -p in agents.md (#2881) 2025-08-28 22:32:53 -07:00
Michael Bolin
970e466ab3 fix: switch to unbounded channel (#2874)
#2747 encouraged me to audit our codebase for similar issues, as now I
am particularly suspicious that our flaky tests are due to a racy
deadlock.

I asked Codex to audit our code, and one of its suggestions was this:

> **High-Risk Patterns**
>
> All `send_*` methods await on a bounded
`mpsc::Sender<OutgoingMessage>`. If the writer blocks, the channel fills
and the processor task blocks on send, stops draining incoming requests,
and stdin reader eventually blocks on its send. This creates a
backpressure deadlock cycle across the three tasks.
>
> **Recommendations**
> * Server outgoing path: break the backpressure cycle
> * Option A (minimal risk): Change `OutgoingMessageSender` to use an
unbounded channel to decouple producer from stdout. Add rate logging so
floods are visible.
> * Option B (bounded + drop policy): Change `send_*` to try_send and
drop messages (or coalesce) when the queue is full, logging a warning.
This prevents processor stalls at the cost of losing messages under
extreme backpressure.
> * Option C (two-stage buffer): Keep bounded channel, but have a
dedicated “egress” task that drains an unbounded internal queue, writing
to stdout with retries and a shutdown timeout. This centralizes
backpressure policy.

So this PR is Option A.

Indeed, we previously used a bounded channel with a capacity of `128`,
but as we discovered recently with #2776, there are certainly cases
where we can get flooded with events.

That said, `test_shell_command_approval_triggers_elicitation` just
failed one one build when I put up this PR, so clearly we are not out of
the woods yet...

**Update:** I think I found the true source of the deadlock! See
https://github.com/openai/codex/pull/2876
2025-08-28 22:20:10 -07:00
Michael Bolin
5d2d3002ef fix: specify --profile to cargo clippy in CI (#2871)
Today we had a breakage in the release build that went unnoticed by CI.
Here is what happened:

- https://github.com/openai/codex/pull/2242 originally added some logic
to do release builds to prevent this from happening
- https://github.com/openai/codex/pull/2276 undid that change to try to
speed things up by removing the step to build all the individual crates
in release mode, assuming the `cargo check` call was sufficient
coverage, which it would have been, had it specified `--profile`

This PR adds `--profile` to the `cargo check` step so we should get the
desired coverage from our build matrix.

Indeed, enabling this in our CI uncovered a warning that is only present
in release mode that was going unnoticed.
2025-08-28 21:43:40 -07:00
agro
bb30996f7c Bugfix: Prevents brew install codex in comment to be executed (#2868)
The default install command causes unexpected code to be executed:

```
npm install -g @openai/codex # Alternatively: `brew install codex`
```

The problem is some environment will treat # as literal string, not
start of comment. Therefore the user will execute this instead (because
it's in backtick)

```
brew install codex
```

And then the npm command will error (because it's trying to install
package #)
2025-08-28 21:40:28 -07:00
9 changed files with 42 additions and 38 deletions

View File

@@ -134,7 +134,7 @@ jobs:
- name: cargo clippy
id: clippy
run: cargo clippy --target ${{ matrix.target }} --all-features --tests -- -D warnings
run: cargo clippy --target ${{ matrix.target }} --all-features --tests --profile ${{ matrix.profile }} -- -D warnings
# Running `cargo build` from the workspace root builds the workspace using
# the union of all features from third-party crates. This can mask errors

View File

@@ -79,15 +79,6 @@ jobs:
with:
targets: ${{ matrix.target }}
- uses: actions/cache@v4
with:
path: |
~/.cargo/bin/
~/.cargo/registry/index/
~/.cargo/registry/cache/
~/.cargo/git/db/
${{ github.workspace }}/codex-rs/target/
key: cargo-release-${{ matrix.runner }}-${{ matrix.target }}-release-${{ hashFiles('**/Cargo.lock') }}
- if: ${{ matrix.target == 'x86_64-unknown-linux-musl' || matrix.target == 'aarch64-unknown-linux-musl'}}
name: Install musl build tools

View File

@@ -8,7 +8,7 @@ In the codex-rs folder where the rust code lives:
- You operate in a sandbox where `CODEX_SANDBOX_NETWORK_DISABLED=1` will be set whenever you use the `shell` tool. Any existing code that uses `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` was authored with this fact in mind. It is often used to early exit out of tests that the author knew you would not be able to run given your sandbox limitations.
- Similarly, when you spawn a process using Seatbelt (`/usr/bin/sandbox-exec`), `CODEX_SANDBOX=seatbelt` will be set on the child process. Integration tests that want to run Seatbelt themselves cannot be run under Seatbelt, so checks for `CODEX_SANDBOX=seatbelt` are also often used to early exit out of tests, as appropriate.
Before finalizing a change to `codex-rs`, run `just fmt` (in `codex-rs` directory) to format the code and `just fix -p <project>` (in `codex-rs` directory) to fix any linter issues in the code. Additionally, run the tests:
Before finalizing a change to `codex-rs`, run `just fmt` (in `codex-rs` directory) to format the code and `just fix -p <project>` (in `codex-rs` directory) to fix any linter issues in the code. Prefer scoping with `-p` to avoid slow workspacewide Clippy builds; only run `just fix` without `-p` if you changed shared crates. Additionally, run the tests:
1. Run the test for the specific project that was changed. For example, if changes were made in `codex-rs/tui`, run `cargo test -p codex-tui`.
2. Once those pass, if any changes were made in common, core, or protocol, run the complete test suite with `cargo test --all-features`.
When running interactively, ask the user before running these commands to finalize.

View File

@@ -14,10 +14,16 @@
### Installing and running Codex CLI
Install globally with your preferred package manager:
Install globally with your preferred package manager. If you use npm:
```shell
npm install -g @openai/codex # Alternatively: `brew install codex`
npm install -g @openai/codex
```
Alternatively, if you use Homebrew:
```shell
brew install codex
```
Then simply run `codex` to get started:

View File

@@ -657,9 +657,17 @@ impl Session {
}
pub fn notify_approval(&self, sub_id: &str, decision: ReviewDecision) {
let mut state = self.state.lock_unchecked();
if let Some(tx_approve) = state.pending_approvals.remove(sub_id) {
tx_approve.send(decision).ok();
let entry = {
let mut state = self.state.lock_unchecked();
state.pending_approvals.remove(sub_id)
};
match entry {
Some(tx_approve) => {
tx_approve.send(decision).ok();
}
None => {
warn!("No pending approval found for sub_id: {sub_id}");
}
}
}

View File

@@ -129,10 +129,7 @@ impl McpClient {
error!("failed to write newline to child stdin");
break;
}
if stdin.flush().await.is_err() {
error!("failed to flush child stdin");
break;
}
// No explicit flush needed on a pipe; write_all is sufficient.
}
Err(e) => error!("failed to serialize JSONRPCMessage: {e}"),
}
@@ -365,7 +362,11 @@ impl McpClient {
}
};
if let Some(tx) = pending.lock().await.remove(&id) {
let tx_opt = {
let mut guard = pending.lock().await;
guard.remove(&id)
};
if let Some(tx) = tx_opt {
// Ignore send errors the receiver might have been dropped.
let _ = tx.send(JSONRPCMessage::Response(resp));
} else {
@@ -383,7 +384,11 @@ impl McpClient {
RequestId::String(_) => return, // see comment above
};
if let Some(tx) = pending.lock().await.remove(&id) {
let tx_opt = {
let mut guard = pending.lock().await;
guard.remove(&id)
};
if let Some(tx) = tx_opt {
let _ = tx.send(JSONRPCMessage::Error(err));
}
}

View File

@@ -59,7 +59,7 @@ pub async fn run_main(
// Set up channels.
let (incoming_tx, mut incoming_rx) = mpsc::channel::<JSONRPCMessage>(CHANNEL_CAPACITY);
let (outgoing_tx, mut outgoing_rx) = mpsc::channel::<OutgoingMessage>(CHANNEL_CAPACITY);
let (outgoing_tx, mut outgoing_rx) = mpsc::unbounded_channel::<OutgoingMessage>();
// Task: read from stdin, push to `incoming_tx`.
let stdin_reader_handle = tokio::spawn({
@@ -134,10 +134,6 @@ pub async fn run_main(
error!("Failed to write newline to stdout: {e}");
break;
}
if let Err(e) = stdout.flush().await {
error!("Failed to flush stdout: {e}");
break;
}
}
Err(e) => error!("Failed to serialize JSONRPCMessage: {e}"),
}

View File

@@ -24,12 +24,12 @@ use crate::error_code::INTERNAL_ERROR_CODE;
/// Sends messages to the client and manages request callbacks.
pub(crate) struct OutgoingMessageSender {
next_request_id: AtomicI64,
sender: mpsc::Sender<OutgoingMessage>,
sender: mpsc::UnboundedSender<OutgoingMessage>,
request_id_to_callback: Mutex<HashMap<RequestId, oneshot::Sender<Result>>>,
}
impl OutgoingMessageSender {
pub(crate) fn new(sender: mpsc::Sender<OutgoingMessage>) -> Self {
pub(crate) fn new(sender: mpsc::UnboundedSender<OutgoingMessage>) -> Self {
Self {
next_request_id: AtomicI64::new(0),
sender,
@@ -55,7 +55,7 @@ impl OutgoingMessageSender {
method: method.to_string(),
params,
});
let _ = self.sender.send(outgoing_message).await;
let _ = self.sender.send(outgoing_message);
rx_approve
}
@@ -81,7 +81,7 @@ impl OutgoingMessageSender {
match serde_json::to_value(response) {
Ok(result) => {
let outgoing_message = OutgoingMessage::Response(OutgoingResponse { id, result });
let _ = self.sender.send(outgoing_message).await;
let _ = self.sender.send(outgoing_message);
}
Err(err) => {
self.send_error(
@@ -130,17 +130,17 @@ impl OutgoingMessageSender {
};
let outgoing_message =
OutgoingMessage::Notification(OutgoingNotification { method, params });
let _ = self.sender.send(outgoing_message).await;
let _ = self.sender.send(outgoing_message);
}
pub(crate) async fn send_notification(&self, notification: OutgoingNotification) {
let outgoing_message = OutgoingMessage::Notification(notification);
let _ = self.sender.send(outgoing_message).await;
let _ = self.sender.send(outgoing_message);
}
pub(crate) async fn send_error(&self, id: RequestId, error: JSONRPCErrorError) {
let outgoing_message = OutgoingMessage::Error(OutgoingError { id, error });
let _ = self.sender.send(outgoing_message).await;
let _ = self.sender.send(outgoing_message);
}
}
@@ -250,7 +250,7 @@ mod tests {
#[tokio::test]
async fn test_send_event_as_notification() {
let (outgoing_tx, mut outgoing_rx) = mpsc::channel::<OutgoingMessage>(2);
let (outgoing_tx, mut outgoing_rx) = mpsc::unbounded_channel::<OutgoingMessage>();
let outgoing_message_sender = OutgoingMessageSender::new(outgoing_tx);
let event = Event {
@@ -281,7 +281,7 @@ mod tests {
#[tokio::test]
async fn test_send_event_as_notification_with_meta() {
let (outgoing_tx, mut outgoing_rx) = mpsc::channel::<OutgoingMessage>(2);
let (outgoing_tx, mut outgoing_rx) = mpsc::unbounded_channel::<OutgoingMessage>();
let outgoing_message_sender = OutgoingMessageSender::new(outgoing_tx);
let session_configured_event = SessionConfiguredEvent {

View File

@@ -64,8 +64,6 @@ mod chatwidget_stream_tests;
#[cfg(not(debug_assertions))]
mod updates;
#[cfg(not(debug_assertions))]
use color_eyre::owo_colors::OwoColorize;
pub use cli::Cli;