feat(tui): migrate cli surfaces to in-process app-server#13636
feat(tui): migrate cli surfaces to in-process app-server#13636fcoury wants to merge 19 commits intoopenai:etraut/in-process-app-server-execfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates CLI surfaces (codex-tui and codex-exec) to run against an in-process app-server instead of talking to the core runtime directly. It introduces a new codex-app-server-client crate as the shared facade, adds a new AgentCommand abstraction in the TUI layer, and expands the v2 protocol surface with new request/response/notification types.
Changes:
- Introduces
codex-app-server-clientas a new crate that wraps the in-process app-server with worker-task lifecycle management, backpressure, and shutdown handling. - Adds
AgentCommandenum in the TUI to decouple UI events from rawOpconstruction, replacing directAppEvent::CodexOp/AppEvent::SubmitThreadOpusage. - Expands the v2 app-server protocol with new API types (MCP elicitation, undo, shutdown, memory, history, shell command, etc.) and their JSON/TypeScript schema artifacts.
Reviewed changes
Copilot reviewed 118 out of 119 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
codex-rs/app-server-client/src/lib.rs |
New crate: in-process app-server client facade with worker task, backpressure, and typed request helpers |
codex-rs/app-server-client/Cargo.toml |
Cargo manifest for the new client crate |
codex-rs/app-server-client/README.md |
Documentation for the new client crate |
codex-rs/tui/src/agent_command.rs |
New AgentCommand enum bridging TUI events and protocol Op values |
codex-rs/tui/src/app_event.rs |
Adds AgentCommand, SubmitThreadAgentCommand, and ThreadCreated event variants |
codex-rs/tui/src/app_event_sender.rs |
Excludes AgentCommand events from session log double-logging |
codex-rs/tui/src/chatwidget.rs |
Removes ThreadManager dependency, exposes new agent lifecycle helpers |
codex-rs/tui/src/chatwidget/tests.rs |
Updates tests to use new models_manager construction and AgentCommand event patterns |
codex-rs/tui/src/bottom_pane/mod.rs |
Updates test assertions to use new AgentCommand::Interrupt variant |
codex-rs/tui/src/bottom_pane/approval_overlay.rs |
Switches approval sends to SubmitThreadAgentCommand with AgentCommand variants |
codex-rs/tui/src/status_indicator_widget.rs |
Uses AgentCommand::Interrupt instead of Op::Interrupt |
codex-rs/tui/src/lib.rs |
Registers new agent_command module |
codex-rs/tui/Cargo.toml |
Adds codex-app-server-client dependency |
codex-rs/exec/Cargo.toml |
Adds codex-app-server-client and codex-app-server-protocol dependencies |
codex-rs/app-server/src/lib.rs |
Exposes in_process module, passes SessionSource::VSCode for network transport |
codex-rs/app-server/src/message_processor.rs |
Adds session_source field, introduces process_client_request for typed in-process path, refactors handle_client_request |
codex-rs/app-server/src/transport.rs |
Replaces OutgoingNotification with typed CoreEvent, updates serialization and tests |
codex-rs/app-server/src/outgoing_message.rs |
Replaces OutgoingNotification with CoreEvent variant, adds legacy_event_method |
codex-rs/app-server/src/bespoke_event_handling.rs |
Adds V2 notification handling for new event types |
codex-rs/app-server/src/thread_state.rs |
Adds module-level documentation |
codex-rs/app-server/src/app_server_tracing.rs |
Adds typed_request_span for in-process requests |
codex-rs/core/src/lib.rs |
Adds lookup_global_history_entry public function |
codex-rs/app-server/README.md |
Documents in-process transport and new v2 API methods |
codex-rs/app-server-protocol/src/protocol/v2.rs |
Adds new protocol types for MCP elicitation, undo, shutdown, memory, history, shell commands, etc. |
| Schema files (JSON/TypeScript) | Generated artifacts for new v2 protocol types |
codex-rs/Cargo.toml |
Adds app-server-client to workspace and registry |
codex-rs/README.md |
Documents new crate structure |
Comments suppressed due to low confidence (2)
codex-rs/app-server-protocol/schema/typescript/v2/ThreadTurnContextSetParams.ts:1
- The generated TypeScript types for
serviceTierandeffortcontain a doubled| null | null. This is generated from theOption<Option<T>>Rust types with double-option serde helpers, but the resulting TypeScript type is incorrect —ServiceTier | null | nullandReasoningEffort | null | nullare redundant. This should beServiceTier | nullandReasoningEffort | nullrespectively. The source Rust struct inv2.rsusesOption<Option<ServiceTier>>andOption<Option<ReasoningEffort>>, so the ts-rs codegen output needs adjustment (e.g. via a#[ts(type = "ServiceTier | null")]override) to avoid confusing API consumers.
codex-rs/app-server/src/app_server_tracing.rs:1 - The
elseblock contains a nestedifchain that could be flattened usingif let Some(...) = ... { ... }directly inside theelse, which is idiomatic Rust. More importantly, the outerelsebranch with two separateif letblocks is the idiomatic pattern and is fine, but the inner blocks could useelse ifto be consistent with the surrounding style. This is a minor style point, but theelse { if ... { } if ... { } }pattern (rather than two top-levelif lets) is slightly non-idiomatic when each branch is independent.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
d6b8906 to
ab1c02f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab1c02fd73
ℹ️ 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".
ab1c02f to
b6fba47
Compare
4c3dacf to
90088ce
Compare
Switch TUI agent execution to `InProcessAppServerClient` for both new sessions and resumed sessions. This wires startup and resume requests, forwards in-process events into TUI app events, and sends core turn and interrupt operations through app-server requests instead of direct thread submission. Shutdown handling now emits `ShutdownComplete` only after client teardown finishes (with a warning fallback on teardown errors). Remove the unused thread parameter from `spawn_agent_from_existing` so the function signature matches runtime behavior.
Limit `ItemStarted(FileChange)` caching to notifications from the active thread in the in-process agent loop. This prevents cross-thread file change data from being associated with the wrong approval request. Document that `account/chatgptAuthTokens/refresh` remains intentionally deferred in this migration path and is explicitly rejected to avoid hanging request timeouts.
Add in-process handling for `Op` variants that map cleanly to existing app-server v2 methods in `chatwidget/agent.rs`, including thread maintenance, model listing, and skills operations. Map app-server responses back into core events and preserve rollback recovery behavior by emitting `ThreadRollbackFailed` metadata when `thread/rollback` fails.
Wire realtime operations in `chatwidget/agent.rs` to existing `thread/realtime/*` app-server methods for start, append-audio, append-text, and stop. Add focused in-process routing tests that assert each realtime op hits the expected method path, preventing unsupported-op fallback regressions while preserving current realtime event handling flow.
Add local in-process handling for history append/read, custom prompt listing, and reload-user-config no-op behavior while keeping deferred ops explicit with warning messages. This preserves TUI behavior without expanding app-server protocol surface. Harden the in-process auth refresh path by moving local token resolution off the async event loop, always rejecting unresolved refresh requests on encoding failures, and aligning local history writes with locking, retention, and file permission safeguards.
Normalize legacy notification methods in `legacy_notification_to_event` by stripping the `codex/event/` prefix before decoding into `EventMsg`. This also accepts wrapped legacy payloads with nested `msg` objects and adds regression tests for prefixed `warning` and `mcp_startup_complete` notifications to prevent startup decode regressions in in-process mode.
Wire mcp elicitation server requests through the in-process agent loop and resolve them via the existing ResolveElicitation op path. Keep pending elicitation entries keyed by server request id and clear them on explicit resolution or serverRequest/resolved notifications. This prevents valid elicitation responses from being dropped during turn transitions and keeps request correlation consistent with the runtime-owned server request id contract in in-process mode.
Teach the TUI in-process agent loop to merge a later authoritative `SessionConfigured` event when it enriches the synthetic startup state, so history and runtime metadata can converge without delaying first paint. Also surface in-process lag events as visible warnings in `exec` instead of only tracing them, which brings shared in-process behavior closer across the CLI surfaces.
Document the in-process client/runtime boundary across `app-server`, `app-server-client`, `exec`, and `tui`. The new comments explain why typed requests still use JSON-RPC result envelopes, how backpressure is surfaced, and where bootstrap session metadata is synthesized versus awaited from the event stream. This keeps the branch's migration intent readable against `main` without changing behavior. Reviewers can now see the main lifecycle, invariants, and remaining bootstrap limitations directly in the code.
Thread the real TUI startup context into the embedded app-server instead of rebuilding it from defaults. `Arg0DispatchPaths`, parsed CLI overrides, and `CloudRequirementsLoader` now flow through `ChatWidgetInit` into the in-process agent startup path so fresh, resumed, forked, and rebuilt sessions all see the same runtime inputs as the top-level TUI process. This also folds in two small in-process client hardening fixes: replacing the duplicated overload error code with a named constant and emitting a warning when shutdown has to abort the worker after timing out. Together these changes address the review findings around startup context loss and make slow shutdowns easier to observe.
Tighten the rustdoc around the in-process app-server runtime, client facade, tracing helpers, and TUI agent context. The added comments make lifecycle, backpressure, and transport boundaries explicit for reviewers and future maintainers. This is a documentation-only follow-up to the in-process migration. It clarifies the relationship between low-level runtime handles and the higher-level facade without changing behavior.
Use the `thread/start` and `thread/resume` responses as the bootstrap `SessionConfigured` payload in `codex-rs/exec/src/lib.rs` instead of waiting up to 10s for the later streamed event. This removes the startup delay that caused `codex exec --experimental-json` to hang long enough for SDK tests to time out. Also remove the obsolete bootstrap helper code and apply the small Clippy cleanups in `codex-rs/exec/src/lib.rs` and `codex-rs/tui/src/chatwidget/agent.rs` so the branch stays green.
Update the `codex-rs/exec/src/lib.rs` and `codex-rs/tui/src/chatwidget/agent.rs` adapters for the latest elicitation protocol shape. This threads the new `meta` and `turn_id` fields through the local response path and fixes the renamed macOS permission model type used by TUI. The change also removes the stale bootstrap `error_seen` binding left behind after the earlier `exec` startup fix. Together these updates restore local builds and unblock the current `rust-ci` compile failures.
…s path - Remove TOCTOU file existence pre-check in read_history_entry_local; handle NotFound directly at the open site in the blocking helper. - Avoid cloning ServerRequest before try_send in in_process runtime; recover the original from the error variant on backpressure.
Dismiss active approval overlays when a turn is interrupted and add a regression test that proves an interrupted exec approval cannot still submit a stale `Op::ExecApproval` afterward. This fixes the warning where exec approval responses could outlive the turn that created them. The in-process agent clears turn-scoped pending requests on `TurnAborted`, so the UI must dismiss any modal that could still try to answer one of those cleared requests.
Opt the in-process TUI out of legacy interactive notifications that now arrive as typed app-server requests. This prevents exec approvals and the other migrated interactive request classes from being delivered twice. Add defensive approval-overlay dedupe keyed by request identity and cover the behavior with regression tests so stale queued prompts do not reappear if mixed ingress slips through again.
Add duplicate-request guards and regression coverage for request-user-input and MCP elicitation overlays so stale queued copies cannot survive the first successful submission. Add a synchronization test for the in-process app-server migration boundary so typed interactive server requests and legacy notification opt-outs stay aligned as the protocol evolves.
Replace the duplicated interactive request opt-out string list in `tui/src/chatwidget/agent.rs` with a single typed mapping that also drives the in-process startup args and sync tests. This removes a maintenance drift point at the typed/legacy migration boundary so future interactive request additions stay aligned by construction.
9f0d737 to
f49bd1c
Compare
Route local `/review` through the in-process `review/start` path so it creates a real active turn. This removes the unsupported-op warning and lets `Ctrl+C` interrupt the running review instead of reporting that no turn is active. Update the TUI in-process client bootstrap to the current explicit `session_source` and `client_name` fields, and drop the old unused `default_client_name` helper so `codex-app-server-client` builds without a dead-code warning.
This is a subset of PR #13636. See that PR for a full overview of the architectural change. This PR implements the in-process app server and modifies the non-interactive "exec" entry point to use the app server. --------- Co-authored-by: Felipe Coury <felipe.coury@gmail.com>
Problem
TUI and exec both embed app-server logic but did so through ad-hoc wiring: each surface duplicated initialize handshake logic, session-source selection, event dispatch, server-request resolution, and shutdown sequencing. This made it easy for in-process behavior to drift from app-server semantics and across surfaces.
Mental model
There are now three layers between a CLI surface and
MessageProcessor:codex_app_server::in_process— low-level runtime host. Owns theMessageProcessor, outbound routing task, and bounded typed channels. Performs theinitialize/initializedhandshake before returning anInProcessClientHandle. Speaks the same JSON-RPC result envelope as stdio/websocket transports.codex-app-server-client— shared facade crate. Wraps the low-level handle behind a worker task with asyncmpscchannels, adds surface identity (ClientSurface→SessionSource), typed request helpers, backpressure-aware event forwarding, and bounded shutdown with abort fallback.Surface (TUI / exec) — consumes events from the facade and maps them into surface-specific state. No longer performs initialize, session-source selection, or shutdown directly.
graph LR subgraph "AFTER: App-server protocol" direction TB A_SURFACE["Surface<br/>(TUI / Exec)"] A_FACADE["InProcessAppServerClient<br/>(bounded channels + worker)"] A_RUNTIME["InProcessClientHandle<br/>(runtime task)"] A_MP["MessageProcessor<br/>(JSON-RPC)"] A_SURFACE <-->|"ClientRequest / InProcessServerEvent<br/>(typed app-server protocol)"| A_FACADE A_FACADE <-->|"InProcessClientMessage<br/>(bounded + try_send)"| A_RUNTIME A_RUNTIME <-->|"process_client_request()<br/>OutgoingMessage"| A_MP end subgraph "BEFORE: Direct coupling" direction TB B_SURFACE["Surface<br/>(TUI / Exec)"] B_THREAD["Arc<CodexThread><br/>.submit(Op) / .next_event()"] B_CODEX["Codex<br/>(internal channels)"] B_AGENT["Agent Loop"] B_SURFACE <-->|"Op / Event<br/>(no protocol boundary)"| B_THREAD B_THREAD <--> B_CODEX B_CODEX <--> B_AGENT end style B_THREAD fill:#c94040,color:#fff style B_CODEX fill:#c94040,color:#fff style A_FACADE fill:#1565c0,color:#fff style A_RUNTIME fill:#7b1fa2,color:#fff style A_MP fill:#2e7d32,color:#fffNon-goals
codex_protocol::Eventbridge is preserved (surfaces still consume it today); removing it is a separate follow-up.Migration boundaries
The following actions and notifications were intentionally not migrated to the app-server RPC/notification surface. They remain local to the TUI/exec surface or flow through the legacy
codex_protocol::Eventbridge.Actions kept local in TUI
Handled locally (no app-server RPC needed):
AddToHistory,GetHistoryEntryRequest,ListCustomPrompts,ReloadUserConfigDeferred (emit local warnings today):
Undo,OverrideTurnContext,DropMemories,UpdateMemories,RunUserShellCommand,ListMcpToolsShutdownis intentionally not a dedicated RPC — it maps tothread/unsubscribeplus local completion handling.Notifications staying on the legacy event bridge
These notifications are not added to the app-server typed path:
warning,backgroundEvent,turn/streamErrorthread/undo/started,thread/undo/completed,thread/shutdown/completedmcpServer/startup/updated,mcpServer/startup/completedskills/updatedIn TUI, only two server notifications use the typed app-server path today:
ServerNotification::ItemStarted(file-change caching)ServerNotification::ServerRequestResolved(MCP elicitation cleanup)Everything else in TUI flows through the legacy
LegacyNotification→EventMsgbridge, including realtime event consumption (realtime requests were migrated, but event consumption was not).execalso consumesServerNotification::Errordirectly on the typed path for fatal-error tracking.Other non-migrated pieces
account/chatgptAuthTokens/refreshremains a local server-request adapter in both TUI and exec rather than a deeper app-server-owned flow.Tradeoffs
JSON-RPC result envelope preserved on the in-process path. Typed requests still receive
serde_json::Valueresults rather than directly typed structs, becauseMessageProcessorproduces that shape internally. This trades some deserialization cost for behavioral parity with socket transports, reducing the risk of subtle execution differences.Bounded channels with overload rejection. Full queues reject server requests with
-32001(overloaded) instead of blocking or growing unbounded. This means a slow consumer can cause approval flows to be rejected rather than queued, which is intentional: hanging approvals are worse than explicit rejection.ClientSurface::Tuimaps toSessionSource::Cli. The TUI is the interactive CLI from the server's perspective, so this mapping is correct even though the names look asymmetric.Architecture
New crates / modules:
codex-rs/app-server-client/— the facade cratecodex-rs/app-server/src/in_process.rs— low-level runtime hostcodex-rs/app-server/src/app_server_tracing.rs— tracing helpers shared across transportsModified:
codex-rs/app-server/src/message_processor.rs— extractedhandle_client_request+process_client_notificationso both JSON-RPC and typed paths share the same request handler.session_sourceis now passed throughMessageProcessorArgsinstead of being hardcoded.codex-rs/exec/src/lib.rs— migrated to useInProcessAppServerClientfacade.codex-rs/tui/src/chatwidget/agent.rs— migrated to useInProcessAppServerClientfacade.Observability
app_server.requestspans withrpc.transport = "in-process", making them distinguishable from stdio/websocket spans in traces.tracing::warnwhen events are dropped or server requests are rejected due to backpressure.Tests
codex-app-server-clienthas integration tests for typed request roundtrips, JSON-RPC error propagation, surface-to-session-source mapping, minimum channel capacity, error source chaining, and lagged-event markers.codex_app_server::in_processhas integration tests for initialize handshake, session source propagation, and zero-capacity clamping.