Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6022174e39
ℹ️ 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".
|
|
||
| // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. | ||
| import type { AskForApproval } from "./AskForApproval"; | ||
| import type { AskForApprovalProtocolV1 } from "./AskForApprovalProtocolV1"; |
There was a problem hiding this comment.
Generate AskForApprovalProtocolV1 TypeScript definition
This generated import references ./AskForApprovalProtocolV1, but that file is missing from codex-rs/app-server-protocol/schema/typescript (only AskForApproval.ts and v2/AskForApproval.ts exist), so downstream TypeScript compilation will fail on module resolution. The same missing type is also re-exported from index.ts, so the v1 schema bundle is currently broken for consumers.
Useful? React with 👍 / 👎.
codex-rs/protocol/src/protocol.rs
Outdated
| Reject { | ||
| /// Reject approval prompts related to sandbox escalation. | ||
| sandbox_approval: bool, |
There was a problem hiding this comment.
Preserve string
reject deserialization for approval_policy
Adding Reject as a data-carrying enum variant means serde now expects an object payload for that variant, so a plain approval_policy = "reject" no longer deserializes when ConfigToml reads Option<AskForApproval> (core/src/config/mod.rs). This conflicts with the new backward-compatibility intent documented in reject_defaults() and the updated config schema that advertises string "reject", and will cause config parsing failures for users who follow that format.
Useful? React with 👍 / 👎.
codex-rs/core/src/safety.rs
Outdated
| || matches!( | ||
| policy, | ||
| AskForApproval::OnFailure | AskForApproval::Reject { .. } | ||
| ) |
There was a problem hiding this comment.
Skip prompt path when rejecting sandbox approvals
This condition now routes every AskForApproval::Reject { .. } through the first branch, which can return AskUser when no platform sandbox is available, so the explicit reject behavior at lines 84-95 is never reached for reject policies. In practice, Reject { sandbox_approval: true } can still surface approval prompts (for example on Windows with sandboxing disabled) instead of being auto-rejected.
Useful? React with 👍 / 👎.
codex-rs/core/src/codex.rs
Outdated
| self.services | ||
| .mcp_connection_manager | ||
| .read() | ||
| .await | ||
| .set_approval_policy(session_configuration.approval_policy.value()); |
There was a problem hiding this comment.
Reapply MCP approval policy after manager reinitialization
The policy is only pushed into McpConnectionManager here during turn creation, but MCP refresh/startup paths recreate the manager with default state (AskForApproval::OnRequest in ElicitationRequestManager::default). After a refresh (and before the first turn), Reject { mcp_elicitations: true } is silently lost, so elicitations can prompt instead of auto-declining.
Useful? React with 👍 / 👎.
85bfc03 to
70e0348
Compare
dylan-hurd-oai
left a comment
There was a problem hiding this comment.
couple of non-blocking comments!
| Decision::Allow | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
would be great to have tests for this piece in particular
| matches!(self, Self::Reject { rules: true, .. }) | ||
| } | ||
|
|
||
| pub const fn rejects_mcp_elicitations(self) -> bool { |
There was a problem hiding this comment.
I think we'd have some product questions to sort out before actually enforcing this, but should this also match on Never?
There was a problem hiding this comment.
Thinking about this, these methods are specific to the Reject variant, so I introduced the RejectConfig struct and moved these methods to RejectConfig.
codex-rs/protocol/src/protocol.rs
Outdated
| impl AskForApproval { | ||
| /// Backward-compatible defaults for `approval_policy = "reject"` in | ||
| /// `config.toml`. | ||
| pub const fn reject_defaults() -> Self { |
There was a problem hiding this comment.
nit: we use new_workspace_write_policy() etc. in SandboxPolicy, should we use a similar naming convention here?
34f85fb to
c132a87
Compare
1980c97 to
714342d
Compare
Why
We need a way to auto-reject specific approval prompt categories without switching all approvals off.
The goal is to let users independently control:
promptrule approvals,What changed
protocol/src/protocol.rs:Wired
RejectConfigsemantics through approval paths incore:core/src/exec_policy.rsrules = truesandbox_approval = truecore/src/tools/sandboxing.rssandbox_approvalto default exec approval decisions and sandbox-failure retry gatingcore/src/safety.rsReject { all false }behavior aligned withOnRequestfor patch safetysandbox_approval = truecore/src/mcp_connection_manager.rsmcp_elicitations = trueEnsured approval policy used by MCP elicitation flow stays in sync with constrained session policy updates.
Updated app-server v2 conversions and generated schema/TypeScript artifacts for the new
Rejectshape.Verification
Added focused unit coverage for the new behavior in:
core/src/exec_policy.rscore/src/tools/sandboxing.rscore/src/mcp_connection_manager.rscore/src/safety.rscore/src/tools/runtimes/apply_patch.rsKey cases covered include rule-vs-sandbox prompt precedence, MCP auto-decline behavior, and patch/sandbox retry behavior under
RejectConfig.