exec-server: route remote network policy decisions - #31458
Conversation
cb396a3 to
431da13
Compare
Co-authored-by: Codex noreply@openai.com
Co-authored-by: Codex noreply@openai.com
431da13 to
080059c
Compare
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex noreply@openai.com
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14e6cbef58
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (_, Some(_), Some(_)) => Err( | ||
| "`managedNetwork` sandbox facts and `networkProxy` launch config are mutually exclusive" |
There was a problem hiding this comment.
When a newer exec-server receives a process/start payload from an older protocol client that still includes both legacy managedNetwork sandbox facts and a networkProxy launch config, this deserialization now rejects the request before the process can start. The previous wire shape exposed those fields independently and accepted that combination, so this tightens an external exec-server integration surface during rolling upgrades; keep the wire parser tolerant, for example by preferring networkProxy and ignoring the legacy facts, while preventing new Rust callers from constructing both.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| /// Managed-network intent. This serializes to the legacy `enforceManagedNetwork`, | ||
| /// `managedNetwork`, and `networkProxy` fields, but Rust callers cannot set those | ||
| /// independently. | ||
| #[serde(flatten)] | ||
| pub managed_network: ExecManagedNetwork, |
There was a problem hiding this comment.
This commit is 1,741 changed lines across 35 files, combining the protocol reshape here, exec-server relay/concurrency/server plumbing, network-proxy callback behavior, core call-site routing, and tests. That is non-mechanical complex logic and exceeds the repo's 800-line review budget; the smallest coherent first stage from this diff would be the wire/API preparation (ExecManagedNetwork compatibility plus network/policyRequest/network/policyDecision protocol types and serialization tests) without live routing, then land the proxy/exec-server relay and core hookup in follow-ups.
AGENTS.md reference: AGENTS.md:L125-L131
Useful? React with 👍 / 👎.
| let mut launch = network.remote_launch_config().await.map_err(|err| { | ||
| ToolError::Codex(CodexErr::Io(io::Error::other(err.to_string()))) | ||
| })?; | ||
| launch.proxy.request_policy_decisions = network.has_policy_decider(); |
There was a problem hiding this comment.
This opt-in is only exercised by the new remote core test with Guardian returning {"outcome":"deny"}; none of the added integration coverage proves that an Allow decision travels back over network/policyDecision and actually unblocks the executor-local proxy. A regression where approved remote network access is still denied would pass the current tests, so add a remote managed-network core/suite test that approves/returns allow and asserts the command can complete.
AGENTS.md reference: AGENTS.md:L116-L118
Useful? React with 👍 / 👎.
| Some(NetworkApprovalSpec { | ||
| network: Some(network.clone()), | ||
| mode: NetworkApprovalMode::Deferred, | ||
| execution_scoped_proxy: req.turn_environment.environment.is_remote(), |
There was a problem hiding this comment.
This is the new remote-only execution-scoped attribution path, but the added remote integration test runs only a single network command. If two remote exec calls hit blocked hosts at the same time, losing or mixing the execution id would deny the requests or attach the wrong trigger while the current single-request remote test and isolated relay/unit tests still pass; add a remote core/suite integration analogous to the existing local concurrent Guardian test.
AGENTS.md reference: AGENTS.md:L116-L118
Useful? React with 👍 / 👎.
| params: NetworkPolicyDecisionNotification, | ||
| ) -> Result<(), String> { | ||
| let session = self | ||
| .require_initialized_for("network policy decision") |
There was a problem hiding this comment.
When automatic session recovery attaches to an existing exec-server session, the client starts its RPC reader before initialize returns specifically so notifications from already-running processes are drained; a running remote process can therefore trigger network/policyRequest and the client can answer with network/policyDecision before it has sent initialized. Requiring initialized here makes that valid resume window a protocol error and closes the connection, so recovered remote networked processes can fail or loop recovery; accept this notification once the session is attached, or buffer it until initialization completes.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| (_, Some(_), Some(_)) => Err( | ||
| "`managedNetwork` sandbox facts and `networkProxy` launch config are mutually exclusive" |
There was a problem hiding this comment.
When a newer exec-server receives a process/start payload from an older protocol client that still includes both legacy managedNetwork sandbox facts and a networkProxy launch config, this deserialization now rejects the request before the process can start. The previous wire shape exposed those fields independently and accepted that combination, so this tightens an external exec-server integration surface during rolling upgrades; keep the wire parser tolerant, for example by preferring networkProxy and ignoring the legacy facts, while preventing new Rust callers from constructing both.
Useful? React with 👍 / 👎.
| /// Managed-network intent. This serializes to the legacy `enforceManagedNetwork`, | ||
| /// `managedNetwork`, and `networkProxy` fields, but Rust callers cannot set those | ||
| /// independently. | ||
| #[serde(flatten)] | ||
| pub managed_network: ExecManagedNetwork, |
There was a problem hiding this comment.
This commit is 1,741 changed lines across 35 files, combining the protocol reshape here, exec-server relay/concurrency/server plumbing, network-proxy callback behavior, core call-site routing, and tests. That is non-mechanical complex logic and exceeds the repo's 800-line review budget; the smallest coherent first stage from this diff would be the wire/API preparation (ExecManagedNetwork compatibility plus network/policyRequest/network/policyDecision protocol types and serialization tests) without live routing, then land the proxy/exec-server relay and core hookup in follow-ups.
Useful? React with 👍 / 👎.
| let mut launch = network.remote_launch_config().await.map_err(|err| { | ||
| ToolError::Codex(CodexErr::Io(io::Error::other(err.to_string()))) | ||
| })?; | ||
| launch.proxy.request_policy_decisions = network.has_policy_decider(); |
There was a problem hiding this comment.
This opt-in is only exercised by the new remote core test with Guardian returning {"outcome":"deny"}; none of the added integration coverage proves that an Allow decision travels back over network/policyDecision and actually unblocks the executor-local proxy. A regression where approved remote network access is still denied would pass the current tests, so add a remote managed-network core/suite test that approves/returns allow and asserts the command can complete.
Useful? React with 👍 / 👎.
| Some(NetworkApprovalSpec { | ||
| network: Some(network.clone()), | ||
| mode: NetworkApprovalMode::Deferred, | ||
| execution_scoped_proxy: req.turn_environment.environment.is_remote(), |
There was a problem hiding this comment.
This is the new remote-only execution-scoped attribution path, but the added remote integration test runs only a single network command. If two remote exec calls hit blocked hosts at the same time, losing or mixing the execution id would deny the requests or attach the wrong trigger while the current single-request remote test and isolated relay/unit tests still pass; add a remote core/suite integration analogous to the existing local concurrent Guardian test.
Useful? React with 👍 / 👎.
| params: NetworkPolicyDecisionNotification, | ||
| ) -> Result<(), String> { | ||
| let session = self | ||
| .require_initialized_for("network policy decision") |
There was a problem hiding this comment.
When automatic session recovery attaches to an existing exec-server session, the client starts its RPC reader before initialize returns specifically so notifications from already-running processes are drained; a running remote process can therefore trigger network/policyRequest and the client can answer with network/policyDecision before it has sent initialized. Requiring initialized here makes that valid resume window a protocol error and closes the connection, so recovered remote networked processes can fail or loop recovery; accept this notification once the session is attached, or buffer it until initialization completes.
Useful? React with 👍 / 👎.
3 participants
Summary
Stack
Depends on #31453, which starts and owns the managed proxy on the executor.
Testing
just test -p codex-network-proxy policy_decision_callback_opt_in_is_backward_compatiblejust test -p codex-exec-server network_policy_decisionsjust test -p codex-core remote_guardian_receives_exact_trigger_for_network_request(compiled and completed through the standard no-remote-environment skip locally)just fix -p codex-exec-server -p codex-network-proxy -p codex-corejust fix -p codex-exec-serverjust fmtA final core rebuild after extracting the relay module hit the machine's local disk ceiling while writing Cargo metadata; the extracted executor tests and scoped Clippy pass completed successfully.