Tighten exec-server review followups#21439
Open
starr-openai wants to merge 47 commits intostarr/exec-env-stdio-stack-5-wire-entrypointsfrom
Open
Tighten exec-server review followups#21439starr-openai wants to merge 47 commits intostarr/exec-env-stdio-stack-5-wire-entrypointsfrom
starr-openai wants to merge 47 commits intostarr/exec-env-stdio-stack-5-wire-entrypointsfrom
Conversation
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>
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>
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>
Spawn stdio exec-server commands directly from structured argv/env/cwd instead of wrapping a shell string, redact the connection label, and tie the stdio child guard to transport disconnect. 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>
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>
Co-authored-by: Codex <noreply@openai.com>
Keep transport shutdown responsible for stdio child cleanup, and remove the separate disconnect watch channel from the JSON-RPC connection/runtime. The RPC client now keeps a single closed flag for rejecting calls after the ordered reader exits. Co-authored-by: Codex <noreply@openai.com>
The stdio transport no longer adds a processor-side disconnect side channel, so drop the test that asserted that removed behavior. Client cleanup is covered at the RPC/client transport boundary instead. Co-authored-by: Codex <noreply@openai.com>
Remove the Option wrapper used only to force connection drop order and call transport shutdown explicitly instead. Also drop dead-code allowances that are no longer needed. Co-authored-by: Codex <noreply@openai.com>
Keep the server-side connection processor on the original by-value parts API, and move the compatibility needed for that shape into JsonRpcConnection. The client still borrows the connection mutably so it can keep transport ownership with ExecServerClient. Co-authored-by: Codex <noreply@openai.com>
Remove the runtime extraction helpers and make JsonRpcConnection ownership explicit at the destructuring sites. Let the stdio transport clean up through Drop so ExecServerClient no longer needs to call an explicit shutdown hook. Co-authored-by: Codex <noreply@openai.com>
Drop the separate JsonRpcConnectionRuntime wrapper so JsonRpcConnection directly owns the channels, disconnect watch, transport tasks, and transport guard. This keeps the lifetime model explicit without helper extraction methods. Co-authored-by: Codex <noreply@openai.com>
Keep the retained transport ownership needed for stdio child cleanup, but drop the broader AtomicBool closed-state behavior and its targeted tests from this PR. Co-authored-by: Codex <noreply@openai.com>
Keep the stack-introduced stdio transport variant explicit while avoiding dead-code and redundant-pattern lints reported by PR20664 CI. Co-authored-by: Codex <noreply@openai.com>
Avoid the Windows clippy large-enum-variant failure while preserving the retained stdio child cleanup guard behavior. Co-authored-by: Codex <noreply@openai.com>
Match the rustfmt shape reported by the PR20664 Format / etc CI job after boxing the retained stdio transport guard. Co-authored-by: Codex <noreply@openai.com>
Escape the JSON-RPC response quotes in the cmd.exe stdio test command so Windows emits valid JSON before the client initialize timeout. Co-authored-by: Codex <noreply@openai.com>
Avoid cmd.exe echo quoting semantics in the Windows stdio client test by reading stdin and writing the JSON-RPC initialize response from PowerShell. Co-authored-by: Codex <noreply@openai.com>
Let environment providers return an explicit default selection and let remote environments track the underlying transport instead of treating only websocket URLs as remote. This prepares the environment layer for stdio-backed remotes without introducing config-file loading. Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Have providers return a concrete default environment id after constructing their environment map, using None to disable the default. This removes the DefaultEnvironmentSelection tri-state while preserving legacy derived defaults through the trait's default implementation. Co-authored-by: Codex <noreply@openai.com>
Make environment providers return the environment map and default id together. This keeps provider-owned startup state in one boundary and removes the separate default callback over a map. Co-authored-by: Codex <noreply@openai.com>
Remove the EnvironmentProviderSnapshot wrapper. Providers now expose environments and the selected default id directly, while EnvironmentManager validates that the default id exists in the returned environment map. Co-authored-by: Codex <noreply@openai.com>
Remove the private from_provider_parts helper. EnvironmentManager::from_provider now performs the provider read, validation, and manager construction directly, and tests use a small provider implementation instead of bypassing that path. Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Keep EnvironmentManager construction async to preserve caller behavior while moving provider-owned default selection into a single snapshot object. Co-authored-by: Codex <noreply@openai.com>
Add the environments.toml schema, parser, validation, and provider implementation for configured websocket and stdio-command environments. This keeps the provider load helper available but does not make product entrypoints use it yet. Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Avoid keeping the test-only constructor in normal builds now that production construction uses the config-dir aware path. Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Make the environments.toml provider reachable from the exec-server crate API so the provider PR passes clippy before entrypoint wiring lands in the next stack PR. Co-authored-by: Codex <noreply@openai.com>
Thread codex_home into EnvironmentManager construction so app entrypoints load environments.toml when present and continue falling back to the legacy CODEX_EXEC_SERVER_URL provider otherwise. Co-authored-by: Codex <noreply@openai.com>
Pass the app-owned EnvironmentManager into ChatWidget so connector prefetch uses the same environment selection that the session was initialized with, instead of reconstructing it from config. Co-authored-by: Codex <noreply@openai.com>
Keep the direct ChatWidget test constructor aligned with the production initializer after wiring the active environment manager through the TUI. Co-authored-by: Codex <noreply@openai.com>
Apply the rustfmt indentation reported by the PR20667 Format / etc CI job after the stack rebase. Co-authored-by: Codex <noreply@openai.com>
Keep the CODEX_HOME and legacy environment manager constructors in the TOML-provider PR, leaving this PR to wire entrypoints to that API. Co-authored-by: Codex <noreply@openai.com>
Treat malformed stdio JSON-RPC output as a terminal disconnect so later calls fail through the existing closed path instead of waiting after the reader exits. Keep the environment provider snapshot/default contract internal by removing the public trait surface from the crate root. Co-authored-by: Codex <noreply@openai.com>
2675f1f to
77a9604
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Final review of the stdio/environment stack found two follow-up cleanups. This lands them as a new stacked PR so the approved earlier PRs do not need to be rebased or have CI restarted.
What Changed
EnvironmentProvidercrate-root re-export and makingEnvironmentManager::from_providercrate-private.Stack
Validation
git diff --check