[codex] consume pushed exec-server process events

[richardopenai]

Summary

• complete unified-exec processes from the ordered event stream instead of issuing a final zero-wait process/read
• add optional executor sandbox-denial state to process/exited
• retain process/read as a retained-output and compatibility fallback for receiver lag, sequence gaps, and legacy servers
• recover sandbox-denial state across transport reconnection
• cover the real TestCodex remote-exec path without adding a public test-only event constructor

Why

A successful one-shot tool call currently receives its output and terminal notifications, then pays another wide-area process/read round trip before returning. Staging traces showed that remote response wait accounted for more than 99.8% of RPC time; local serialization, queueing, and deserialization were below 0.6 ms.

Measured impact

A direct staging A/B used the same build and route and changed only completion mode. Each arm ran three times with 30 one-shot /usr/bin/true calls per run. The table reports the median of the three per-run percentiles.

Metric	Final process/read	Pushed events	Change
End-to-end completion p50	159.5 ms	118.7 ms	-40.8 ms (-25.6%)
End-to-end completion p95	182.4 ms	131.7 ms	-50.6 ms (-27.8%)
Completion-wait p50	80.1 ms	41.5 ms	-38.5 ms (-48.1%)
Final process/read RPC p50	79.9 ms	eliminated	-79.9 ms

TCP_NODELAY was enabled in both A/B arms, so its effect cancels out. The successful, complete, in-order event path issued zero final process/read calls.

Compatibility and recovery

• new servers send sandboxDenied on process/exited
• legacy servers omit it, which triggers one compatibility process/read
• broadcast lag or a sequence gap triggers a retained-output read
• recovery remains bounded by the server's existing 1 MiB retained-output window
• complete, in-order event streams issue no completion read
• sandbox denial is attached to the exit event before consumers can observe process completion
• server-first and client-first rollouts remain wire-compatible; server-first realizes the latency win immediately

Integration coverage

The TestCodex suite exercises four distinct remote-exec contracts:

• complete pushed output/exit/close with zero reads
• direct pushed sandbox denial with zero reads
• legacy missing denial metadata with exactly one compatibility read
• count-bounded replay eviction recovered from retained output without duplication

Validation

• just test -p codex-core exec_command_consumes_pushed_remote_process_events: 4 passed
• just test -p codex-core unified_exec::process_tests::: 4 passed
• just test -p codex-exec-server: 294 passed, 2 skipped
• just test -p codex-exec-server-protocol: 5 passed
• just test -p codex-rmcp-client: 89 passed, 2 skipped
• focused Bazel //codex-rs/core:core-all-test: passed across 16 shards
• scoped just fix passed for core and exec-server
• just fmt passed

The complete workspace suite was not rerun; focused Cargo and Bazel coverage passed for the changed behavior.

[chatgpt-codex-connector]

Review source: Codex Cloud Agents (CCA)

Codex Cloud Agents (CCA) couldn't complete this review. The original Codex Review is unaffected.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 005450e87c

ℹ️ 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".

[chatgpt-codex-connector]

Avoid exposing a test-only event receiver constructor

Because ExecProcessEventReceiver is re-exported from codex-exec-server, this pub constructor becomes part of the crate’s API even though it is only used to feed canned events in tests. Please keep this behind test support or another non-public path so the production API does not grow with a hidden test helper.

AGENTS.md reference: AGENTS.md:L87-L89

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Add integration coverage for pushed exec events

This switches unified exec from wake/read polling to consuming pushed process events, including lag and legacy fallback behavior, but the diff only adds mock-based unit coverage in process_tests.rs; no core/suite TestCodex integration test exercises the real agent tool path. Please add integration coverage for the new completion path so the actual session wiring and user-facing exec behavior are protected.

AGENTS.md reference: AGENTS.md:L114-L116

Useful? React with 👍 / 👎.

[richardopenai]

After we get a wakeup, we call process.read which introduces an extra round trip

[rpelevin]

I would keep the acceptance boundary at the event stream, not at the latency win.

The correctness question here is whether "complete" becomes observable only after every authority-relevant terminal fact has crossed the same ordered channel as stdout and stderr. If output, exit, close, and sandbox-denial state can arrive on different recovery paths, the caller can accidentally treat a process as successfully complete while the denial or retained output is still sitting behind a fallback read.

The regression shape I would want is:

1. emit output, exit with sandbox-denied state, and close through the pushed stream in order;
2. assert no final retained-output read is issued on that path;
3. introduce a sequence gap and assert retained-output recovery fills only missing bytes without duplicating prior chunks;
4. emulate a legacy exit event with missing denial metadata and assert exactly one compatibility read;
5. reconnect after transport loss and assert sandbox-denied state is recovered before the process is reported complete;
6. assert the final tool result cannot be marked successful if the terminal event later resolves as denied or failed.

That gives the protocol a useful invariant: pushed completion is authoritative only when the ordered stream contains the full terminal state. The retained read is then a recovery path for lag, gaps, and legacy metadata, not a second source of truth that can silently change the meaning of a completed tool call after dispatch has already resumed.

The performance gain is real, but the governance property is more important: the model should not continue from a remote exec result until output, exit, close, and denial state have converged into one terminal process record.

Boundary: architecture and regression-test feedback only; no claim about using this project, running this branch, validating implementation behavior, implementation correctness, performance measurements, merge readiness, security review, production readiness, partnership, customer interest, official alignment, OpenAI usage, Codex usage, conformance certification, or Neura usage.