Skip to content

Add stdio exec-server client transport#20664

Open
starr-openai wants to merge 6 commits intomainfrom
starr/exec-env-stdio-stack-2-client
Open

Add stdio exec-server client transport#20664
starr-openai wants to merge 6 commits intomainfrom
starr/exec-env-stdio-stack-2-client

Conversation

@starr-openai
Copy link
Copy Markdown
Contributor

@starr-openai starr-openai commented May 1, 2026

Why

Configured environments need to connect to exec-server instances that are not necessarily already listening on a websocket URL. A command-backed stdio transport lets Codex start an exec-server process, speak JSON-RPC over its stdio streams, and clean up that child process with the client lifetime.

Stack position: this is PR 2 of 5. It builds on the server-side stdio listener from PR 1 and provides the client transport used by later environment/config PRs.

What Changed

  • Add ExecServerTransport variants for websocket URLs and stdio shell commands.
  • Add stdio command connection support for ExecServerClient.
  • Move websocket/stdio transport setup into client_transport.rs so client.rs stays focused on shared JSON-RPC client, session, HTTP, and notification behavior.
  • Tie stdio child process cleanup to the JSON-RPC connection lifetime with a RAII lifetime guard.
  • Keep existing websocket environment behavior by adapting URL-backed remotes to ExecServerTransport::WebSocketUrl.

Stack

Split from original draft: #20508

Validation

Not run locally; this was split out of the original draft stack and then refactored to separate transport setup from the base client.

@starr-openai starr-openai force-pushed the starr/exec-env-stdio-stack-2-client branch 7 times, most recently from 1a11c8b to 751ed42 Compare May 1, 2026 22:02
@starr-openai starr-openai force-pushed the starr/exec-env-stdio-stack-1-server branch from b8272bb to 80a3c55 Compare May 4, 2026 17:35
starr-openai added a commit that referenced this pull request May 4, 2026
## Why

This stack adds configured exec-server environments, including
environments reached over stdio. Before client-side stdio transports or
config can use that path, the exec-server binary itself needs a
first-class stdio listen mode so it can speak the same JSON-RPC protocol
over stdin/stdout that it already speaks over websockets.

**Stack position:** this is PR 1 of 5. It is the server-side transport
foundation for the stack.

## What Changed

- Accept `stdio` and `stdio://` for `codex exec-server --listen`.
- Promote the existing stdio `JsonRpcConnection` helper from test-only
code into normal exec-server transport code.
- Add parse coverage for stdio listen URLs while preserving the existing
websocket default.

## Stack

- **1. This PR:** #20663 - Add stdio
exec-server listener
- 2. #20664 - Add stdio exec-server
client transport
- 3. #20665 - Make environment
providers own default selection
- 4. #20666 - Add CODEX_HOME
environments TOML provider
- 5. #20667 - Load configured
environments from CODEX_HOME

Split from original draft: #20508

## Validation

Not run locally; this was split out of the original draft stack.

---------

Co-authored-by: Codex <noreply@openai.com>
Base automatically changed from starr/exec-env-stdio-stack-1-server to main May 4, 2026 18:40
starr-openai and others added 4 commits May 4, 2026 11:41
Allow exec-server clients to connect through a shell command over stdio. The connection can now retain a drop resource so the spawned child is terminated when the JSON-RPC client is dropped.

Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
@starr-openai starr-openai force-pushed the starr/exec-env-stdio-stack-2-client branch from 3d9bb84 to 4c6d799 Compare May 4, 2026 18:41
Use the existing process-group cleanup pattern for stdio command transports so wrapper shell children are terminated with the client lifetime. Add a regression test that drops the client after spawning a background shell child through the command-backed transport.

Co-authored-by: Codex <noreply@openai.com>
@starr-openai starr-openai force-pushed the starr/exec-env-stdio-stack-2-client branch from 7bc1d8f to 309db97 Compare May 4, 2026 19:17
@starr-openai starr-openai marked this pull request as ready for review May 4, 2026 19:26
Keep environment transport connection policy on ExecServerClient instead of the transport enum, and replace the JSON-RPC connection tuple alias with named connection parts.

Co-authored-by: Codex <noreply@openai.com>

/// Stdio connection arguments for a command-backed exec-server.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct StdioExecServerConnectArgs {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A single shell string means callers have to hand-roll quoting, and we lose any way to control argv/env/cwd... is it on purpose?

#[cfg(not(windows))]
{
let mut command = Command::new("sh");
command.arg("-lc").arg(shell_command);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure we want a login shell here? For example, what if the .profile has any kind of echo "hello world" (as lots of people do)
Then the stdout become

hello world
{"id":1,"result":{"sessionId":"..."}}

and so we break the JSON-RPC

And I think this problem might become true in multiple other cases...

JsonRpcConnection::from_stdio(
stdout,
stdin,
format!("exec-server stdio command `{shell_command}`"),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid putting the raw command in the connection label? Any token/env/... assignment in the command now gets echoed back through transport errors/logs


let should_escalate = match terminate_process_group(process_group_id) {
Ok(exists) => exists,
Err(err) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(found by Codex)
If killpg(SIGTERM) fails with anything other than “not found”, we just log it and then hand the child to wait(). i.e. dead lock

disconnected_rx: watch::Receiver<bool>,
next_request_id: AtomicI64,
transport_tasks: Vec<JoinHandle<()>>,
_transport_lifetime: Option<TransportLifetime>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This keeps the child alive for the cached client lifetime, not the live connection lifetime. Is it on purpose?

.expect("json-rpc line should write");
}

#[cfg(not(windows))]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a windows test as well? Not sure this is a big deal for now but it might become one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants