codex

Separate interactive and non-interactive sessions (#4612 )

Do not show exec session in VSCode/TUI selector.

pakrym-oai · 2025-10-02 13:06:21 -07:00

4c566d484a

show "Viewed Image" when the model views an image (#4475 )

<img width="1022" height="339" alt="Screenshot 2025-09-29 at 4 22 00 PM"
src="https://github.com/user-attachments/assets/12da7358-19be-4010-a71b-496ede6dfbbf"
/>

Jeremy Rose · 2025-10-02 18:36:03 +00:00

45936f8fbd

Support CODEX_API_KEY for codex exec (#4615 )

Allows to set API key per invocation of `codex exec`

pakrym-oai · 2025-10-02 09:59:45 -07:00

2f6fb37d72

fix: remove mcp-types from app server protocol (#4537 )

We continue the separation between `codex app-server` and `codex
mcp-server`.

In particular, we introduce a new crate, `codex-app-server-protocol`,
and migrate `codex-rs/protocol/src/mcp_protocol.rs` into it, renaming it
`codex-rs/app-server-protocol/src/protocol.rs`.

Because `ConversationId` was defined in `mcp_protocol.rs`, we move it
into its own file, `codex-rs/protocol/src/conversation_id.rs`, and
because it is referenced in a ton of places, we have to touch a lot of
files as part of this PR.

We also decide to get away from proper JSON-RPC 2.0 semantics, so we
also introduce `codex-rs/app-server-protocol/src/jsonrpc_lite.rs`, which
is basically the same `JSONRPCMessage` type defined in `mcp-types`
except with all of the `"jsonrpc": "2.0"` removed.

Getting rid of `"jsonrpc": "2.0"` makes our serialization logic
considerably simpler, as we can lean heavier on serde to serialize
directly into the wire format that we use now.

Michael Bolin · 2025-10-01 02:16:26 +00:00

5881c0d6d4

fix: separate codex mcp into codex mcp-server and codex app-server (#4471 )

This is a very large PR with some non-backwards-compatible changes.

Historically, `codex mcp` (or `codex mcp serve`) started a JSON-RPC-ish
server that had two overlapping responsibilities:

- Running an MCP server, providing some basic tool calls.
- Running the app server used to power experiences such as the VS Code
extension.

This PR aims to separate these into distinct concepts:

- `codex mcp-server` for the MCP server
- `codex app-server` for the "application server"

Note `codex mcp` still exists because it already has its own subcommands
for MCP management (`list`, `add`, etc.)

The MCP logic continues to live in `codex-rs/mcp-server` whereas the
refactored app server logic is in the new `codex-rs/app-server` folder.
Note that most of the existing integration tests in
`codex-rs/mcp-server/tests/suite` were actually for the app server, so
all the tests have been moved with the exception of
`codex-rs/mcp-server/tests/suite/mod.rs`.

Because this is already a large diff, I tried not to change more than I
had to, so `codex-rs/app-server/tests/common/mcp_process.rs` still uses
the name `McpProcess` for now, but I will do some mechanical renamings
to things like `AppServer` in subsequent PRs.

While `mcp-server` and `app-server` share some overlapping functionality
(like reading streams of JSONL and dispatching based on message types)
and some differences (completely different message types), I ended up
doing a bit of copypasta between the two crates, as both have somewhat
similar `message_processor.rs` and `outgoing_message.rs` files for now,
though I expect them to diverge more in the near future.

One material change is that of the initialize handshake for `codex
app-server`, as we no longer use the MCP types for that handshake.
Instead, we update `codex-rs/protocol/src/mcp_protocol.rs` to add an
`Initialize` variant to `ClientRequest`, which takes the `ClientInfo`
object we need to update the `USER_AGENT_SUFFIX` in
`codex-rs/app-server/src/message_processor.rs`.

One other material change is in
`codex-rs/app-server/src/codex_message_processor.rs` where I eliminated
a use of the `send_event_as_notification()` method I am generally trying
to deprecate (because it blindly maps an `EventMsg` into a
`JSONNotification`) in favor of `send_server_notification()`, which
takes a `ServerNotification`, as that is intended to be a custom enum of
all notification types supported by the app server. So to make this
update, I had to introduce a new variant of `ServerNotification`,
`SessionConfigured`, which is a non-backwards compatible change with the
old `codex mcp`, and clients will have to be updated after the next
release that contains this PR. Note that
`codex-rs/app-server/tests/suite/list_resume.rs` also had to be update
to reflect this change.

I introduced `codex-rs/utils/json-to-toml/src/lib.rs` as a small utility
crate to avoid some of the copying between `mcp-server` and
`app-server`.

Michael Bolin · 2025-09-30 07:06:18 +00:00

d9dbf48828

[mcp-server] Expose fuzzy file search in MCP (#2677 )

## Summary
Expose a simple fuzzy file search implementation for mcp clients to work
with

## Testing
- [x] Tested locally

Dylan · 2025-09-29 12:19:09 -07:00

197f45a3be

make tests pass cleanly in sandbox (#4067 )

This changes the reqwest client used in tests to be sandbox-friendly,
and skips a bunch of other tests that don't work inside the
sandbox/without network.

Jeremy Rose · 2025-09-25 13:11:14 -07:00

4a5f05c136

Add exec output-schema parameter (#4079 )

Adds structured output to `exec` via the `--structured-output`
parameter.

pakrym-oai · 2025-09-23 13:59:16 -07:00

fdb8dadcae

Use anyhow::Result in tests for error propagation (#4105 )

pakrym-oai · 2025-09-23 13:31:36 -07:00

0f9a796617

feat: update default (#4076 )

Changes:
- Default model and docs now use gpt-5-codex. 
- Disables the GPT-5 Codex NUX by default.
- Keeps presets available for API key users.

Thibault Sottiaux · 2025-09-22 20:10:52 -07:00

c93e77b68b

chore: clippy on redundant closure (#4058 )

Add redundant closure clippy rules and let Codex fix it by minimising
FQP

jif-oai · 2025-09-22 19:30:16 +00:00

be366a31ab

chore: unify cargo versions (#4044 )

Unify cargo versions at root

jif-oai · 2025-09-22 16:47:01 +00:00

e5fe50d3ce

Add non_sandbox_test helper (#3880 )

Makes tests shorter

pakrym-oai · 2025-09-22 14:50:41 +00:00

14a115d488

fix: ensure cwd for conversation and sandbox are separate concerns (#3874 )

Previous to this PR, both of these functions take a single `cwd`:


https://github.com/openai/codex/blob/71038381aa0f51aa62e1a2bcc7cbf26a05b141f3/codex-rs/core/src/seatbelt.rs#L19-L25


https://github.com/openai/codex/blob/71038381aa0f51aa62e1a2bcc7cbf26a05b141f3/codex-rs/core/src/landlock.rs#L16-L23

whereas `cwd` and `sandbox_cwd` should be set independently (fixed in
this PR).

Added `sandbox_distinguishes_command_and_policy_cwds()` to
`codex-rs/exec/tests/suite/sandbox.rs` to verify this.

Michael Bolin · 2025-09-18 14:37:06 -07:00

8595237505

Switch to uuid_v7 and tighten ConversationId usage (#3819 )

Make sure conversations have a timestamp.

pakrym-oai · 2025-09-18 14:37:03 +00:00

d4aba772cb

Fix get_auth_status response when using custom provider (#3581 )

This PR addresses an edge-case bug that appears in the VS Code extension
in the following situation:
1. Log in using ChatGPT (using either the CLI or extension). This will
create an `auth.json` file.
2. Manually modify `config.toml` to specify a custom provider.
3. Start a fresh copy of the VS Code extension.

The profile menu in the VS Code extension will indicate that you are
logged in using ChatGPT even though you're not.

This is caused by the `get_auth_status` method returning an
`auth_method: 'chatgpt'` when a custom provider is configured and it
doesn't use OpenAI auth (i.e. `requires_openai_auth` is false). The
method should always return `auth_method: None` if
`requires_openai_auth` is false.

The same bug also causes the NUX (new user experience) screen to be
displayed in the VSCE in this situation.

Eric Traut · 2025-09-14 18:27:02 -07:00

e5dd7f0934

Review Mode (Core) (#3401 )

## 📝 Review Mode -- Core

This PR introduces the Core implementation for Review mode:

- New op `Op::Review { prompt: String }:` spawns a child review task
with isolated context, a review‑specific system prompt, and a
`Config.review_model`.
- `EnteredReviewMode`: emitted when the child review session starts.
Every event from this point onwards reflects the review session.
- `ExitedReviewMode(Option<ReviewOutputEvent>)`: emitted when the review
finishes or is interrupted, with optional structured findings:

```json
{
  "findings": [
    {
      "title": "<≤ 80 chars, imperative>",
      "body": "<valid Markdown explaining *why* this is a problem; cite files/lines/functions>",
      "confidence_score": <float 0.0-1.0>,
      "priority": <int 0-3>,
      "code_location": {
        "absolute_file_path": "<file path>",
        "line_range": {"start": <int>, "end": <int>}
      }
    }
  ],
  "overall_correctness": "patch is correct" | "patch is incorrect",
  "overall_explanation": "<1-3 sentence explanation justifying the overall_correctness verdict>",
  "overall_confidence_score": <float 0.0-1.0>
}
```

## Questions

### Why separate out its own message history?

We want the review thread to match the training of our review models as
much as possible -- that means using a custom prompt, removing user
instructions, and starting a clean chat history.

We also want to make sure the review thread doesn't leak into the parent
thread.

### Why do this as a mode, vs. sub-agents?

1. We want review to be a synchronous task, so it's fine for now to do a
bespoke implementation.
2. We're still unclear about the final structure for sub-agents. We'd
prefer to land this quickly and then refactor into sub-agents without
rushing that implementation.

dedrisian-oai · 2025-09-12 23:25:10 +00:00

90a0fd342f

feat: reasoning effort as optional (#3527 )

Allow the reasoning effort to be optional

jif-oai · 2025-09-12 12:06:33 -07:00

c6fd056aa6

feat: change the behavior of SetDefaultModel RPC so None clears the value. (#3529 )

It turns out that we want slightly different behavior for the
`SetDefaultModel` RPC because some models do not work with reasoning
(like GPT-4.1), so we should be able to explicitly clear this value.

Verified in `codex-rs/mcp-server/tests/suite/set_default_model.rs`.

Michael Bolin · 2025-09-12 11:35:51 -07:00

abdcb40f4c

feat: added SetDefaultModel to JSON-RPC server (#3512 )

This adds `SetDefaultModel`, which takes `model` and `reasoning_effort`
as optional fields. If set, the field will overwrite what is in the
user's `config.toml`.

This reuses logic that was added to support the `/model` command in the
TUI: https://github.com/openai/codex/pull/2799.

Michael Bolin · 2025-09-11 23:44:17 -07:00

c172e8e997

feat: include reasoning_effort in NewConversationResponse (#3506 )

`ClientRequest::NewConversation` picks up the reasoning level from the user's defaults in `config.toml`, so it should be reported in `NewConversationResponse`.

Michael Bolin · 2025-09-11 21:04:40 -07:00

9bbeb75361

chore: enable clippy::redundant_clone (#3489 )

Created this PR by:

- adding `redundant_clone` to `[workspace.lints.clippy]` in
`cargo-rs/Cargol.toml`
- running `cargo clippy --tests --fix`
- running `just fmt`

Though I had to clean up one instance of the following that resulted:

```rust
let codex = codex;
```

Michael Bolin · 2025-09-11 11:59:37 -07:00

bec51f6c05

Simplify auth flow and reconcile differences between ChatGPT and API Key auth (#3189 )

This PR does the following:
* Adds the ability to paste or type an API key.
* Removes the `preferred_auth_method` config option. The last login
method is always persisted in auth.json, so this isn't needed.
* If OPENAI_API_KEY env variable is defined, the value is used to
prepopulate the new UI. The env variable is otherwise ignored by the
CLI.
* Adds a new MCP server entry point "login_api_key" so we can implement
this same API key behavior for the VS Code extension.
<img width="473" height="140" alt="Screenshot 2025-09-04 at 3 51 04 PM"
src="https://github.com/user-attachments/assets/c11bbd5b-8a4d-4d71-90fd-34130460f9d9"
/>
<img width="726" height="254" alt="Screenshot 2025-09-04 at 3 51 32 PM"
src="https://github.com/user-attachments/assets/6cc76b34-309a-4387-acbc-15ee5c756db9"
/>

Eric Traut · 2025-09-11 09:16:34 -07:00

e13b35ecb0

Change forking to read the rollout from file (#3440 )

This PR changes get history op to get path. Then, forking will use a
path. This will help us have one unified codepath for resuming/forking
conversations. Will also help in having rollout history in order. It
also fixes a bug where you won't see the UI when resuming after forking.

Ahmed Ibrahim · 2025-09-10 17:42:54 -07:00

162e1235a8

feat: add UserInfo request to JSON-RPC server (#3428 )

This adds a simple endpoint that provides the email address encoded in
`$CODEX_HOME/auth.json`.

As noted, for now, we do not hit the server to verify this is the user's
true email address.

Michael Bolin · 2025-09-10 17:03:35 -07:00

65f3528cad

fix: ensure output of codex-rs/mcp-types/generate_mcp_types.py matches codex-rs/mcp-types/src/lib.rs (#3439 )

https://github.com/openai/codex/pull/3395 updated `mcp-types/src/lib.rs`
by hand, but that file is generated code that is produced by
`mcp-types/generate_mcp_types.py`. Unfortunately, we do not have
anything in CI to verify this right now, but I will address that in a
subsequent PR.

#3395 ended up introducing a change that added a required field when
deserializing `InitializeResult`, breaking Codex when used as an MCP
client, so the quick fix in #3436 was to make the new field `Optional`
with `skip_serializing_if = "Option::is_none"`, but that did not address
the problem that `mcp-types/generate_mcp_types.py` and
`mcp-types/src/lib.rs` are out of sync.

This PR gets things back to where they are in sync. It removes the
custom `mcp_types::McpClientInfo` type that was added to
`mcp-types/src/lib.rs` and forces us to use the generated
`mcp_types::Implementation` type. Though this PR also updates
`generate_mcp_types.py` to generate the additional `user_agent:
Optional<String>` field on `Implementation` so that we can continue to
specify it when Codex operates as an MCP server.

However, this also requires us to specify `user_agent: None` when Codex
operates as an MCP client.

We may want to introduce our own `InitializeResult` type that is
specific to when we run as a server to avoid this in the future, but my
immediate goal is just to get things back in sync.

Michael Bolin · 2025-09-10 16:14:41 -07:00

44262d8fd8

Make user_agent optional (#3436 )

# External (non-OpenAI) Pull Request Requirements

Currently, mcp server fail to start with:
```
🖐  MCP client for `<CLIENT>` failed to start: missing field `user_agent`
````

It isn't clear to me yet why this is happening. My understanding is that
this struct is simply added as a new field to the response but this
should fix it until I figure out the full story here.

<img width="714" height="262" alt="CleanShot 2025-09-10 at 13 58 59"
src="https://github.com/user-attachments/assets/946b1313-5c1c-43d3-8ae8-ecc3de3406fc"
/>

Gabriel Peal · 2025-09-10 14:15:02 -07:00

8d766088e6

Improved resiliency of two auth-related tests (#3427 )

This PR improves two existing auth-related tests. They were failing when
run in an environment where an `OPENAI_API_KEY` env variable was
defined. The change makes them more resilient.

Eric Traut · 2025-09-10 11:46:02 -07:00

acb28bf914

Set a user agent suffix when used as a mcp server (#3395 )

This automatically adds a user agent suffix whenever the CLI is used as
a MCP server

Gabriel Peal · 2025-09-10 02:32:57 +00:00

8636bff46d

Introduce rollout items (#3380 )

This PR introduces Rollout items. This enable us to rollout eventmsgs
and session meta.

This is mostly #3214 with rebase on main

Ahmed Ibrahim · 2025-09-09 23:52:33 +00:00

43809a454e

Replace config.responses_originator_header_internal_override with CODEX_INTERNAL_ORIGINATOR_OVERRIDE_ENV_VAR (#3388 )

The previous config approach had a few issues:
1. It is part of the config but not designed to be used externally
2. It had to be wired through many places (look at the +/- on this PR
3. It wasn't guaranteed to be set consistently everywhere because we
don't have a super well defined way that configs stack. For example, the
extension would configure during newConversation but anything that
happened outside of that (like login) wouldn't get it.

This env var approach is cleaner and also creates one less thing we have
to deal with when coming up with a better holistic story around configs.

One downside is that I removed the unit test testing for the override
because I don't want to deal with setting the global env or spawning
child processes and figuring out how to introspect their originator
header. The new code is sufficiently simple and I tested it e2e that I
feel as if this is still worth it.

Gabriel Peal · 2025-09-09 17:23:23 -04:00

5eab4c7ab4

feat: add ArchiveConversation to ClientRequest (#3353 )

Adds support for `ArchiveConversation` in the JSON-RPC server that takes
a `(ConversationId, PathBuf)` pair and:

- verifies the `ConversationId` corresponds to the rollout id at the
`PathBuf`
- if so, invokes
`ConversationManager.remove_conversation(ConversationId)`
- if the `CodexConversation` was in memory, send `Shutdown` and wait for
`ShutdownComplete` with a timeout
- moves the `.jsonl` file to `$CODEX_HOME/archived_sessions`

---------

Co-authored-by: Gabriel Peal <gabriel@openai.com>

Michael Bolin · 2025-09-09 11:39:00 -04:00

ace14e8d36

fix: include rollout_path in NewConversationResponse (#3352 )

Adding the `rollout_path` to the `NewConversationResponse` makes it so a
client can perform subsequent operations on a `(ConversationId,
PathBuf)` pair. #3353 will introduce support for `ArchiveConversation`.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/3352).
* #3353
* __->__ #3352

Michael Bolin · 2025-09-09 00:11:48 -07:00

2a76a08a9e

feat: Run cargo shear during CI (#3338 )

Run cargo shear as part of the CI to ensure no unused dependencies

jif-oai · 2025-09-09 01:05:08 +00:00

a9c68ea270

Generate more typescript types and return conversation id with ConversationSummary (#3219 )

This PR does multiple things that are necessary for conversation resume
to work from the extension. I wanted to make sure everything worked so
these changes wound up in one PR:
1. Generate more ts types
2. Resume rollout history files rather than create a new one every time
it is resumed so you don't see a duplicate conversation in history for
every resume. Chatted with @aibrahim-oai to verify this
3. Return conversation_id in conversation summaries
4. [Cleanup] Use serde and strong types for a lot of the rollout file
parsing

Gabriel Peal · 2025-09-08 17:54:47 -04:00

5eaaf307e1

Add a getUserAgent MCP method (#3320 )

This will allow the extension to pass this user agent + a suffix for its
requests

Gabriel Peal · 2025-09-08 13:30:13 -04:00

5c1416d99b

Use ConversationId instead of raw Uuids (#3282 )

We're trying to migrate from `session_id: Uuid` to `conversation_id:
ConversationId`. Not only does this give us more type safety but it
unifies our terminology across Codex and with the implementation of
session resuming, a conversation (which can span multiple sessions) is
more appropriate.

I started this impl on https://github.com/openai/codex/pull/3219 as part
of getting resume working in the extension but it's big enough that it
should be broken out.

Gabriel Peal · 2025-09-07 23:22:25 -04:00

c8fab51372

Never store requests (#3212 )

When item ids are sent to Responses API it will load them from the
database ignoring the provided values. This adds extra latency.

Not having the mode to store requests also allows us to simplify the
code.

## Breaking change

The `disable_response_storage` configuration option is removed.

pakrym-oai · 2025-09-05 10:41:47 -07:00

5775174ec2

chore: improve serialization of ServerNotification (#3193 )

This PR introduces introduces a new
`OutgoingMessage::AppServerNotification` variant that is designed to
wrap a `ServerNotification`, which makes the serialization more
straightforward compared to
`OutgoingMessage::Notification(OutgoingNotification)`. We still use the
latter for serializing an `Event` as a `JSONRPCMessage::Notification`,
but I will try to get away from that in the near future.

With this change, now the generated TypeScript type for
`ServerNotification` is:

```typescript
export type ServerNotification =
  | { "method": "authStatusChange", "params": AuthStatusChangeNotification }
  | { "method": "loginChatGptComplete", "params": LoginChatGptCompleteNotification };
```

whereas before it was:

```typescript
export type ServerNotification =
  | { type: "auth_status_change"; data: AuthStatusChangeNotification }
  | { type: "login_chat_gpt_complete"; data: LoginChatGptCompleteNotification };
```

Once the `Event`s are migrated to the `ServerNotification` enum in Rust,
it should be considerably easier to work with notifications on the
TypeScript side, as it will be possible to `switch (message.method)` and
check for exhaustiveness.

Though we will probably need to introduce:

```typescript
export type ServerMessage = ServerRequest | ServerNotification;
```

and then we still need to group all of the `ServerResponse` types
together, as well.

Michael Bolin · 2025-09-04 17:49:50 -07:00

3f40fbc0a8

MCP: add session resume + history listing; (#3185 )

# External (non-OpenAI) Pull Request Requirements

Before opening this Pull Request, please read the dedicated
"Contributing" markdown file or your PR may be closed:
https://github.com/openai/codex/blob/main/docs/contributing.md

If your PR conforms to our contribution guidelines, replace this text
with a detailed and high quality description of your changes.

Ahmed Ibrahim · 2025-09-04 23:44:18 +00:00

907d3dd348

[mcp-server] Update read config interface (#3093 )

## Summary
Follow-up to #3056

This PR updates the mcp-server interface for reading the config settings
saved by the user. At risk of introducing _another_ Config struct, I
think it makes sense to avoid tying our protocol to ConfigToml, as its
become a bit unwieldy. GetConfigTomlResponse was a de-facto struct for
this already - better to make it explicit, in my opinion.

This is technically a breaking change of the mcp-server protocol, but
given the previous interface was introduced so recently in #2725, and we
have not yet even started to call it, I propose proceeding with the
breaking change - but am open to preserving the old endpoint.

## Testing
- [x] Added additional integration test coverage

Dylan · 2025-09-04 16:26:41 -07:00

82ed7bd285

Dividing UserMsgs into categories to send it back to the tui (#3127 )

This PR does the following:

- divides user msgs into 3 categories: plain, user instructions, and
environment context
- Centralizes adding user instructions and environment context to a
degree
- Improve the integration testing

Building on top of #3123

Specifically this
[comment](https://github.com/openai/codex/pull/3123#discussion_r2319885089).
We need to send the user message while ignoring the User Instructions
and Environment Context we attach.

Ahmed Ibrahim · 2025-09-04 05:34:50 +00:00

2b96f9f569

Replay EventMsgs from Response Items when resuming a session with history. (#3123 )

### Overview

This PR introduces the following changes:
	1.	Adds a unified mechanism to convert ResponseItem into EventMsg.
2. Ensures that when a session is initialized with initial history, a
vector of EventMsg is sent along with the session configuration. This
allows clients to re-render the UI accordingly.
	3. 	Added integration testing

### Caveats

This implementation does not send every EventMsg that was previously
dispatched to clients. The excluded events fall into two categories:
	•	“Arguably” rolled-out events
Examples include tool calls and apply-patch calls. While these events
are conceptually rolled out, we currently only roll out ResponseItems.
These events are already being handled elsewhere and transformed into
EventMsg before being sent.
	•	Non-rolled-out events
Certain events such as TurnDiff, Error, and TokenCount are not rolled
out at all.

### Future Directions

At present, resuming a session involves maintaining two states:
	•	UI State
Clients can replay most of the important UI from the provided EventMsg
history.
	•	Model State
The model receives the complete session history to reconstruct its
internal state.

This design provides a solid foundation. If, in the future, more precise
UI reconstruction is needed, we have two potential paths:
1. Introduce a third data structure that allows us to derive both
ResponseItems and EventMsgs.
2. Clearly divide responsibilities: the core system ensures the
integrity of the model state, while clients are responsible for
reconstructing the UI.

Ahmed Ibrahim · 2025-09-04 04:47:00 +00:00

f2036572b6

MCP sandbox call (#3128 )

I have read the CLA Document and I hereby sign the CLA

jif-oai · 2025-09-03 17:05:03 -07:00

bea64569c1

Include originator in authentication URL parameters (#3117 )

Associates the client with an authentication session.

pakrym-oai · 2025-09-03 16:51:00 -07:00

e83c5f429c

Add a common way to create HTTP client (#3110 )

Ensure User-Agent and originator are always sent.

pakrym-oai · 2025-09-03 10:11:02 -07:00

c636f821ae

Move CodexAuth and AuthManager to the core crate (#3074 )

Fix a long standing layering issue.

pakrym-oai · 2025-09-02 18:36:19 -07:00

03e2796ca4

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.

Michael Bolin · 2025-08-28 22:41:10 -07:00

cb2f952143

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

Michael Bolin · 2025-08-28 22:20:10 -07:00

970e466ab3

Bug fix: clone of incoming_tx can lead to deadlock (#2747 )

POC code

```rust
use tokio::sync::mpsc;
use std::time::Duration;

#[tokio::main]
async fn main() {
    println!("=== Test 1: Simulating original MCP server pattern ===");
    test_original_pattern().await;
}

async fn test_original_pattern() {
    println!("Testing the original pattern from MCP server...");
    
    // Create channel - this simulates the original incoming_tx/incoming_rx
    let (tx, mut rx) = mpsc::channel::<String>(10);
    
    // Task 1: Simulates stdin reader that will naturally terminate
    let stdin_task = tokio::spawn({
        let tx_clone = tx.clone();
        async move {
            println!("  stdin_task: Started, will send 3 messages then exit");
            for i in 0..3 {
                let msg = format!("Message {}", i);
                if tx_clone.send(msg.clone()).await.is_err() {
                    println!("  stdin_task: Receiver dropped, exiting");
                    break;
                }
                println!("  stdin_task: Sent {}", msg);
                tokio::time::sleep(Duration::from_millis(300)).await;
            }
            println!("  stdin_task: Finished (simulating EOF)");
            // tx_clone is dropped here
        }
    });
    
    // Task 2: Simulates message processor
    let processor_task = tokio::spawn(async move {
        println!("  processor_task: Started, waiting for messages");
        while let Some(msg) = rx.recv().await {
            println!("  processor_task: Processing {}", msg);
            tokio::time::sleep(Duration::from_millis(100)).await;
        }
        println!("  processor_task: Finished (channel closed)");
    });
    
    // Task 3: Simulates stdout writer or other background task
    let background_task = tokio::spawn(async move {
        for i in 0..2 {
            tokio::time::sleep(Duration::from_millis(500)).await;
            println!("  background_task: Tick {}", i);
        }
        println!("  background_task: Finished");
    });
    
    println!("  main: Original tx is still alive here");
    println!("  main: About to call tokio::join! - will this deadlock?");
    
    // This is the pattern from the original code
    let _ = tokio::join!(stdin_task, processor_task, background_task);
}

```

---------

Co-authored-by: Michael Bolin <bolinfest@gmail.com>

unship · 2025-08-28 19:28:17 -07:00

f7cb2f87a0

160 Commits