Uh oh!
There was an error while loading. Please reload this page.
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| if method == SANDBOX_STATE_NOTIFICATION | ||
| && let Some(params) = params | ||
| { | ||
| match serde_json::from_value::<SandboxState>(params) { |
There was a problem hiding this comment.
Custom sandbox-state notifications are parsed into SandboxState here, but that struct’s sandbox_type uses the core SandboxType enum which keeps its PascalCase variant names. The new SandboxTypeWire right above is kebab-cased, matching the wire format (linux-seccomp, macos-seatbelt, etc.), yet it is never used. When a client sends the advertised codex/sandbox-state/update notification with kebab-case sandbox types, serde_json::from_value will fail and the state is ignored, so the server silently falls back to the default sandbox instead of honoring the client-provided type/policy.
Useful? React with 👍 / 👎.
0671cd5 to
c3e37e1Compare`process_exec_tool_call()` was taking `SandboxType` as a param, but in practice, the only place it was constructed was in `codex_message_processor.rs` where it was derived from the other `sandbox_policy` param, so this PR inlines the logic that decides the `SandboxType` into `process_exec_tool_call()`. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/7122). * #7112 * __->__ #7122
| #[serde(rename_all = "camelCase")] | ||
| pub struct SandboxState { | ||
| pub sandbox_policy: SandboxPolicy, | ||
| pub codex_linux_sandbox_exe: Option<PathBuf>, |
There was a problem hiding this comment.
I feel like this is something we can reasonably discover for ourselves in the shell tool; we have access to the same logic there.
This introduces a new feature to Codex when it operates as an MCP client where if an MCP server replies that it has an entry named
"codex/sandbox-state"in its server capabilities, then Codex will send it an MCP notification with the following structure:{ "method": "codex/sandbox-state/update", "params": { "sandboxPolicy": { "type": "workspace-write", "network-access": false, "exclude-tmpdir-env-var": false"exclude-slash-tmp": false }, "codexLinuxSandboxExe": null, "sandboxCwd": "/Users/mbolin/code/codex2" } }or with whatever values are appropriate for the initial
sandboxPolicy.NOTE: Codex should continue to send the MCP server notifications of the same format if these things change over the lifetime of the thread, but that isn't wired up yet.
The result is that
shell-tool-mcpcan consume these values so that when it callscodex_core::exec::process_exec_tool_call()incodex-rs/exec-server/src/posix/escalate_server.rs, it is now sure to call it with the correct values (whereas previously we relied on hardcoded values).While I would argue this is a supported use case within the MCP protocol, the
rmcpcrate that we are using today does not support custom notifications. As such, I had to patch it and I submitted it for review, so hopefully it will be accepted in some form:modelcontextprotocol/rust-sdk#556
To test out this change from end-to-end:
cargo buildin~/code/codex2/codex-rs/exec-server~/code/bash/bash~/.codex/config.toml:~/code/codex2/codex-rs, I ranjust codex --disable shell_toolworkspace-write/mcpto verify that the shell tool from the MCP is there:because this should be auto-approved with our existing dummy policy:
codex/codex-rs/exec-server/src/posix.rs
Lines 157 to 164 in af63e6e
And it worked: