mirror of
https://github.com/openai/codex.git
synced 2026-10-30 19:18:49 +00:00
Enforce strict auto-review for MCP tool calls (#38492)
## What changed - Route MCP tool calls through the automatic reviewer when strict auto-review is enabled, even when the approval policy, tool annotations, or a remembered session decision would otherwise skip review. - Pass the strict auto-review flag into the MCP approval request so reviewer selection follows the turn setting. - Update MCP approval and turn-metadata tests to cover the forced review path and confirm that it does not request user input. GitOrigin-RevId: 2c0b5f4dc1a15cb2fb827e4b21e69167fdcf3e56
This commit is contained in:
@@ -1297,6 +1297,16 @@ async fn maybe_request_mcp_tool_approval(
|
||||
policy: McpToolApprovalPolicy,
|
||||
) -> Option<ReviewDecision> {
|
||||
let turn_context = &step_context.turn;
|
||||
let turn_state = sess
|
||||
.active_turn
|
||||
.lock()
|
||||
.await
|
||||
.as_ref()
|
||||
.map(|active| Arc::clone(&active.turn_state));
|
||||
let strict_auto_review = match turn_state {
|
||||
Some(turn_state) => turn_state.lock().await.strict_auto_review_enabled(),
|
||||
None => false,
|
||||
};
|
||||
let approvals_reviewer = connectors::mcp_approvals_reviewer_from_layers(
|
||||
&config.config_layer_stack,
|
||||
config.approvals_reviewer,
|
||||
@@ -1304,18 +1314,20 @@ async fn maybe_request_mcp_tool_approval(
|
||||
&invocation.server,
|
||||
metadata.connector_id.as_deref(),
|
||||
);
|
||||
if mcp_permission_prompt_is_auto_approved(
|
||||
config.approval_policy.value(),
|
||||
&config.permission_profile,
|
||||
McpPermissionPromptAutoApproveContext {
|
||||
tool_approval_mode: Some(policy.mode),
|
||||
},
|
||||
) {
|
||||
if !strict_auto_review
|
||||
&& mcp_permission_prompt_is_auto_approved(
|
||||
config.approval_policy.value(),
|
||||
&config.permission_profile,
|
||||
McpPermissionPromptAutoApproveContext {
|
||||
tool_approval_mode: Some(policy.mode),
|
||||
},
|
||||
)
|
||||
{
|
||||
return None;
|
||||
}
|
||||
|
||||
let annotations = metadata.annotations.as_ref();
|
||||
if !requires_mcp_tool_approval_for_mode(annotations, policy.mode) {
|
||||
if !strict_auto_review && !requires_mcp_tool_approval_for_mode(annotations, policy.mode) {
|
||||
return None;
|
||||
}
|
||||
|
||||
@@ -1326,7 +1338,8 @@ async fn maybe_request_mcp_tool_approval(
|
||||
} else {
|
||||
None
|
||||
};
|
||||
if let Some(key) = session_approval_key.as_ref()
|
||||
if !strict_auto_review
|
||||
&& let Some(key) = session_approval_key.as_ref()
|
||||
&& mcp_tool_approval_is_remembered(sess, key).await
|
||||
{
|
||||
return Some(ReviewDecision::Approved);
|
||||
@@ -1364,7 +1377,7 @@ async fn maybe_request_mcp_tool_approval(
|
||||
review_context: GuardianReviewContext::from(step_context),
|
||||
call_id: call_id.to_string(),
|
||||
tool_name: invocation_tool_name.clone(),
|
||||
strict_auto_review: false,
|
||||
strict_auto_review,
|
||||
approval_reason: None,
|
||||
retry_reason: None,
|
||||
network_approval_context: None,
|
||||
|
||||
@@ -2628,7 +2628,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_mode_mcp_denial_uses_captured_policy_and_returns_rationale_message() {
|
||||
async fn strict_auto_review_forces_guardian_for_mcp_policy_skip() {
|
||||
let server = start_mock_server().await;
|
||||
let guardian_request_log = mount_sse_once(
|
||||
&server,
|
||||
@@ -2657,7 +2657,7 @@ async fn guardian_mode_mcp_denial_uses_captured_policy_and_returns_rationale_mes
|
||||
.expect("test setup should allow updating approval policy");
|
||||
let mut config = (*turn_context.config).clone();
|
||||
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
config.approvals_reviewer = ApprovalsReviewer::User;
|
||||
let config = Arc::new(config);
|
||||
let models_manager = models_manager_with_provider(
|
||||
config.codex_home.to_path_buf(),
|
||||
@@ -2671,6 +2671,13 @@ async fn guardian_mode_mcp_denial_uses_captured_policy_and_returns_rationale_mes
|
||||
turn_context.auth_manager.clone(),
|
||||
);
|
||||
|
||||
let active_turn = ActiveTurn::default();
|
||||
active_turn
|
||||
.turn_state
|
||||
.lock()
|
||||
.await
|
||||
.enable_strict_auto_review();
|
||||
*session.active_turn.lock().await = Some(active_turn);
|
||||
let session = Arc::new(session);
|
||||
let turn_context = Arc::new(turn_context);
|
||||
let invocation = McpInvocation {
|
||||
@@ -2707,7 +2714,7 @@ async fn guardian_mode_mcp_denial_uses_captured_policy_and_returns_rationale_mes
|
||||
&HookToolName::new("mcp__test__tool"),
|
||||
&metadata,
|
||||
&captured_mcp_config,
|
||||
McpToolApprovalPolicy::for_server(AppToolApproval::Auto),
|
||||
McpToolApprovalPolicy::for_server(AppToolApproval::Approve),
|
||||
)
|
||||
.await;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user