agent/codex
1
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

Unify skip-review handling for approval_mode = "approve" (#20750)

Browse Source 
## Summary
- Treat `approval_mode = "approve"` as skip-review across all permission
modes.
- Remove the mode-specific split in the MCP auto-approval gate so
approved tools bypass review consistently.
- Expand regression coverage in the shared MCP helper and the core
tool-call flow.

## Testing
- `just fmt`
- `cargo test -p codex-mcp`
- `cargo test -p codex-core
approve_mode_skips_arc_and_guardian_in_every_permission_mode`
- `git diff --check`
- Full `cargo test -p codex-core` was also attempted, but the suite hit
an unrelated pre-existing stack overflow in an existing multi-agent test
This commit is contained in:
Matthew Zeng
2026-05-04 10:30:47 -07:00
committed by GitHub
Unverified
parent 83a4e3b66b
commit 1b900bee8a
3 changed files with 88 additions and 96 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/codex-mcp/src/mcp/mod.rs
+1 -6
View File
@@ -71,12 +71,7 @@ pub fn mcp_permission_prompt_is_auto_approved(
permission_profile: &PermissionProfile,
context: McpPermissionPromptAutoApproveContext,
) -> bool {
if matches!(
approval_policy,
AskForApproval::OnRequest | AskForApproval::Granular(_)
) && context.approvals_reviewer == Some(ApprovalsReviewer::AutoReview)
&& context.tool_approval_mode == Some(AppToolApproval::Approve)
{
if context.tool_approval_mode == Some(AppToolApproval::Approve) {
return true;
}
codex-rs/codex-mcp/src/mcp/mod_tests.rs
+24 -27
View File
@@ -74,16 +74,11 @@ fn mcp_prompt_auto_approval_honors_unrestricted_managed_profiles() {
}
#[test]
fn mcp_prompt_auto_approval_honors_auto_review_approved_tools() {
assert!(mcp_permission_prompt_is_auto_approved(
fn mcp_prompt_auto_approval_honors_approved_tools_in_all_permission_modes() {
for approval_policy in [
AskForApproval::UnlessTrusted,
AskForApproval::OnFailure,
AskForApproval::OnRequest,
&PermissionProfile::read_only(),
McpPermissionPromptAutoApproveContext {
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
tool_approval_mode: Some(AppToolApproval::Approve),
},
));
assert!(mcp_permission_prompt_is_auto_approved(
AskForApproval::Granular(GranularApprovalConfig {
sandbox_approval: true,
rules: true,
@@ -91,34 +86,36 @@ fn mcp_prompt_auto_approval_honors_auto_review_approved_tools() {
request_permissions: true,
mcp_elicitations: true,
}),
AskForApproval::Never,
] {
assert!(mcp_permission_prompt_is_auto_approved(
approval_policy,
&PermissionProfile::read_only(),
McpPermissionPromptAutoApproveContext {
approvals_reviewer: Some(ApprovalsReviewer::User),
tool_approval_mode: Some(AppToolApproval::Approve),
},
));
}
assert!(!mcp_permission_prompt_is_auto_approved(
AskForApproval::OnRequest,
&PermissionProfile::read_only(),
McpPermissionPromptAutoApproveContext {
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
tool_approval_mode: Some(AppToolApproval::Approve),
tool_approval_mode: Some(AppToolApproval::Auto),
},
));
}
#[test]
fn mcp_prompt_auto_approval_rejects_auto_mode_in_default_permission_mode() {
assert!(!mcp_permission_prompt_is_auto_approved(
AskForApproval::OnRequest,
&PermissionProfile::read_only(),
McpPermissionPromptAutoApproveContext {
approvals_reviewer: Some(ApprovalsReviewer::User),
tool_approval_mode: Some(AppToolApproval::Approve),
},
));
assert!(!mcp_permission_prompt_is_auto_approved(
AskForApproval::OnFailure,
&PermissionProfile::read_only(),
McpPermissionPromptAutoApproveContext {
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
tool_approval_mode: Some(AppToolApproval::Approve),
},
));
assert!(!mcp_permission_prompt_is_auto_approved(
AskForApproval::UnlessTrusted,
&PermissionProfile::read_only(),
McpPermissionPromptAutoApproveContext {
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
tool_approval_mode: Some(AppToolApproval::Approve),
tool_approval_mode: Some(AppToolApproval::Auto),
},
));
}
codex-rs/core/src/mcp_tool_call_tests.rs
+63 -63
View File
@@ -18,6 +18,7 @@ use codex_hooks::HooksConfig;
use codex_model_provider::create_model_provider;
use codex_protocol::models::PermissionProfile;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::GranularApprovalConfig;
use core_test_support::PathExt;
use core_test_support::responses::ev_assistant_message;
use core_test_support::responses::ev_completed;
@@ -2328,7 +2329,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval
}
#[tokio::test]
async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() {
async fn approve_mode_skips_arc_interrupt_for_model() {
use wiremock::Mock;
use wiremock::MockServer;
use wiremock::ResponseTemplate;
@@ -2349,7 +2350,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() {
"why": "high-risk action",
}],
})))
.expect(1)
.expect(0)
.mount(&server)
.await;
@@ -2391,16 +2392,11 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() {
)
.await;
assert_eq!(
decision,
Some(McpToolApprovalDecision::BlockedBySafetyMonitor(
"Tool call was cancelled because of safety risks: high-risk action".to_string(),
))
);
assert_eq!(decision, None);
}
#[tokio::test]
async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() {
async fn custom_approve_mode_skips_arc_interrupt_for_model() {
use wiremock::Mock;
use wiremock::MockServer;
use wiremock::ResponseTemplate;
@@ -2421,7 +2417,7 @@ async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() {
"why": "high-risk action",
}],
})))
.expect(1)
.expect(0)
.mount(&server)
.await;
@@ -2463,16 +2459,11 @@ async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() {
)
.await;
assert_eq!(
decision,
Some(McpToolApprovalDecision::BlockedBySafetyMonitor(
"Tool call was cancelled because of safety risks: high-risk action".to_string(),
))
);
assert_eq!(decision, None);
}
#[tokio::test]
async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() {
async fn approve_mode_skips_arc_interrupt_without_annotations() {
use wiremock::Mock;
use wiremock::MockServer;
use wiremock::ResponseTemplate;
@@ -2493,7 +2484,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() {
"why": "high-risk action",
}],
})))
.expect(1)
.expect(0)
.mount(&server)
.await;
@@ -2535,12 +2526,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() {
)
.await;
assert_eq!(
decision,
Some(McpToolApprovalDecision::BlockedBySafetyMonitor(
"Tool call was cancelled because of safety risks: high-risk action".to_string(),
))
);
assert_eq!(decision, None);
}
#[tokio::test]
@@ -2622,7 +2608,7 @@ async fn full_access_mode_skips_arc_monitor_for_all_approval_modes() {
}
#[tokio::test]
async fn approve_mode_skips_arc_and_guardian_when_guardian_reviewer_is_enabled() {
async fn approve_mode_skips_arc_and_guardian_in_every_permission_mode() {
use wiremock::Mock;
use wiremock::ResponseTemplate;
use wiremock::matchers::method;
@@ -2652,33 +2638,6 @@ async fn approve_mode_skips_arc_and_guardian_when_guardian_reviewer_is_enabled()
.mount(&server)
.await;
let (mut session, mut turn_context) = make_session_and_context().await;
turn_context.auth_manager = Some(crate::test_support::auth_manager_from_auth(
codex_login::CodexAuth::create_dummy_chatgpt_auth_for_testing(),
));
turn_context
.approval_policy
.set(AskForApproval::OnRequest)
.expect("test setup should allow updating approval policy");
let mut config = (*turn_context.config).clone();
config.chatgpt_base_url = server.uri();
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
let config = Arc::new(config);
let models_manager = models_manager_with_provider(
config.codex_home.to_path_buf(),
Arc::clone(&session.services.auth_manager),
config.model_provider.clone(),
);
session.services.models_manager = models_manager;
turn_context.config = Arc::clone(&config);
turn_context.provider = create_model_provider(
config.model_provider.clone(),
turn_context.auth_manager.clone(),
);
let session = Arc::new(session);
let turn_context = Arc::new(turn_context);
let invocation = McpInvocation {
server: CODEX_APPS_MCP_SERVER_NAME.to_string(),
tool: "dangerous_tool".to_string(),
@@ -2696,16 +2655,57 @@ async fn approve_mode_skips_arc_and_guardian_when_guardian_reviewer_is_enabled()
openai_file_input_params: None,
};
let decision = maybe_request_mcp_tool_approval(
&session,
&turn_context,
"call-3",
&invocation,
"mcp__test__tool",
Some(&metadata),
AppToolApproval::Approve,
)
.await;
for approval_policy in [
AskForApproval::UnlessTrusted,
AskForApproval::OnFailure,
AskForApproval::OnRequest,
AskForApproval::Granular(GranularApprovalConfig {
sandbox_approval: true,
rules: true,
skill_approval: true,
request_permissions: true,
mcp_elicitations: true,
}),
AskForApproval::Never,
] {
let (mut session, mut turn_context) = make_session_and_context().await;
turn_context.auth_manager = Some(crate::test_support::auth_manager_from_auth(
codex_login::CodexAuth::create_dummy_chatgpt_auth_for_testing(),
));
turn_context
.approval_policy
.set(approval_policy)
.expect("test setup should allow updating approval policy");
let mut config = (*turn_context.config).clone();
config.chatgpt_base_url = server.uri();
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
config.approvals_reviewer = ApprovalsReviewer::User;
let config = Arc::new(config);
let models_manager = models_manager_with_provider(
config.codex_home.to_path_buf(),
Arc::clone(&session.services.auth_manager),
config.model_provider.clone(),
);
session.services.models_manager = models_manager;
turn_context.config = Arc::clone(&config);
turn_context.provider = create_model_provider(
config.model_provider.clone(),
turn_context.auth_manager.clone(),
);
assert_eq!(decision, None);
let session = Arc::new(session);
let turn_context = Arc::new(turn_context);
let decision = maybe_request_mcp_tool_approval(
&session,
&turn_context,
"call-3",
&invocation,
"mcp__test__tool",
Some(&metadata),
AppToolApproval::Approve,
)
.await;
assert_eq!(decision, None);
}
}