parent
1350477150
commit
dd88ed767b
4 changed files with 160 additions and 14 deletions
|
|
@ -99,6 +99,7 @@ pub(crate) async fn monitor_action(
|
|||
sess: &Session,
|
||||
turn_context: &TurnContext,
|
||||
action: serde_json::Value,
|
||||
protection_client_callsite: &'static str,
|
||||
) -> ArcMonitorOutcome {
|
||||
let auth = match turn_context.auth_manager.as_ref() {
|
||||
Some(auth_manager) => match auth_manager.auth().await {
|
||||
|
|
@ -138,7 +139,8 @@ pub(crate) async fn monitor_action(
|
|||
return ArcMonitorOutcome::Ok;
|
||||
}
|
||||
};
|
||||
let body = build_arc_monitor_request(sess, turn_context, action).await;
|
||||
let body =
|
||||
build_arc_monitor_request(sess, turn_context, action, protection_client_callsite).await;
|
||||
let client = build_reqwest_client();
|
||||
let mut request = client
|
||||
.post(&url)
|
||||
|
|
@ -236,6 +238,7 @@ async fn build_arc_monitor_request(
|
|||
sess: &Session,
|
||||
turn_context: &TurnContext,
|
||||
action: serde_json::Map<String, serde_json::Value>,
|
||||
protection_client_callsite: &'static str,
|
||||
) -> ArcMonitorRequest {
|
||||
let history = sess.clone_history().await;
|
||||
let mut messages = build_arc_monitor_messages(history.raw_items());
|
||||
|
|
@ -254,7 +257,7 @@ async fn build_arc_monitor_request(
|
|||
codex_thread_id: conversation_id.clone(),
|
||||
codex_turn_id: turn_context.sub_id.clone(),
|
||||
conversation_id: Some(conversation_id),
|
||||
protection_client_callsite: None,
|
||||
protection_client_callsite: Some(protection_client_callsite.to_string()),
|
||||
},
|
||||
messages: Some(messages),
|
||||
input: None,
|
||||
|
|
|
|||
|
|
@ -178,6 +178,7 @@ async fn build_arc_monitor_request_includes_relevant_history_and_null_policies()
|
|||
&turn_context,
|
||||
serde_json::from_value(serde_json::json!({ "tool": "mcp_tool_call" }))
|
||||
.expect("action should deserialize"),
|
||||
"normal",
|
||||
)
|
||||
.await;
|
||||
|
||||
|
|
@ -188,7 +189,7 @@ async fn build_arc_monitor_request_includes_relevant_history_and_null_policies()
|
|||
codex_thread_id: session.conversation_id.to_string(),
|
||||
codex_turn_id: turn_context.sub_id.clone(),
|
||||
conversation_id: Some(session.conversation_id.to_string()),
|
||||
protection_client_callsite: None,
|
||||
protection_client_callsite: Some("normal".to_string()),
|
||||
},
|
||||
messages: Some(vec![
|
||||
ArcMonitorChatMessage {
|
||||
|
|
@ -285,6 +286,7 @@ async fn monitor_action_posts_expected_arc_request() {
|
|||
"codex_thread_id": session.conversation_id.to_string(),
|
||||
"codex_turn_id": turn_context.sub_id.clone(),
|
||||
"conversation_id": session.conversation_id.to_string(),
|
||||
"protection_client_callsite": "normal",
|
||||
},
|
||||
"messages": [{
|
||||
"role": "user",
|
||||
|
|
@ -320,6 +322,7 @@ async fn monitor_action_posts_expected_arc_request() {
|
|||
&session,
|
||||
&turn_context,
|
||||
serde_json::json!({ "tool": "mcp_tool_call" }),
|
||||
"normal",
|
||||
)
|
||||
.await;
|
||||
|
||||
|
|
@ -377,6 +380,7 @@ async fn monitor_action_uses_env_url_and_token_overrides() {
|
|||
&session,
|
||||
&turn_context,
|
||||
serde_json::json!({ "tool": "mcp_tool_call" }),
|
||||
"normal",
|
||||
)
|
||||
.await;
|
||||
|
||||
|
|
@ -428,6 +432,7 @@ async fn monitor_action_rejects_legacy_response_fields() {
|
|||
&session,
|
||||
&turn_context,
|
||||
serde_json::json!({ "tool": "mcp_tool_call" }),
|
||||
"normal",
|
||||
)
|
||||
.await;
|
||||
|
||||
|
|
|
|||
|
|
@ -457,6 +457,9 @@ const MCP_TOOL_APPROVAL_TOOL_TITLE_KEY: &str = "tool_title";
|
|||
const MCP_TOOL_APPROVAL_TOOL_DESCRIPTION_KEY: &str = "tool_description";
|
||||
const MCP_TOOL_APPROVAL_TOOL_PARAMS_KEY: &str = "tool_params";
|
||||
const MCP_TOOL_APPROVAL_TOOL_PARAMS_DISPLAY_KEY: &str = "tool_params_display";
|
||||
const MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_DEFAULT: &str = "mcp_tool_call__default";
|
||||
const MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_ALWAYS_ALLOW: &str = "mcp_tool_call__always_allow";
|
||||
const MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_FULL_ACCESS: &str = "mcp_tool_call__full_access";
|
||||
|
||||
pub(crate) fn is_mcp_tool_approval_question_id(question_id: &str) -> bool {
|
||||
question_id
|
||||
|
|
@ -494,14 +497,22 @@ async fn maybe_request_mcp_tool_approval(
|
|||
let annotations = metadata.and_then(|metadata| metadata.annotations.as_ref());
|
||||
let approval_required = annotations.is_some_and(requires_mcp_tool_approval);
|
||||
let mut monitor_reason = None;
|
||||
let auto_approved_by_policy = approval_mode == AppToolApproval::Approve
|
||||
|| (approval_mode == AppToolApproval::Auto && is_full_access_mode(turn_context));
|
||||
|
||||
if approval_mode == AppToolApproval::Approve {
|
||||
if auto_approved_by_policy {
|
||||
if !approval_required {
|
||||
return None;
|
||||
}
|
||||
|
||||
match maybe_monitor_auto_approved_mcp_tool_call(sess, turn_context, invocation, metadata)
|
||||
.await
|
||||
match maybe_monitor_auto_approved_mcp_tool_call(
|
||||
sess,
|
||||
turn_context,
|
||||
invocation,
|
||||
metadata,
|
||||
approval_mode,
|
||||
)
|
||||
.await
|
||||
{
|
||||
ArcMonitorOutcome::Ok => return None,
|
||||
ArcMonitorOutcome::AskUser(reason) => {
|
||||
|
|
@ -515,13 +526,8 @@ async fn maybe_request_mcp_tool_approval(
|
|||
}
|
||||
}
|
||||
|
||||
if approval_mode == AppToolApproval::Auto {
|
||||
if is_full_access_mode(turn_context) {
|
||||
return None;
|
||||
}
|
||||
if !approval_required {
|
||||
return None;
|
||||
}
|
||||
if approval_mode == AppToolApproval::Auto && !approval_required {
|
||||
return None;
|
||||
}
|
||||
|
||||
let session_approval_key = session_mcp_tool_approval_key(invocation, metadata, approval_mode);
|
||||
|
|
@ -653,9 +659,16 @@ async fn maybe_monitor_auto_approved_mcp_tool_call(
|
|||
turn_context: &TurnContext,
|
||||
invocation: &McpInvocation,
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
approval_mode: AppToolApproval,
|
||||
) -> ArcMonitorOutcome {
|
||||
let action = prepare_arc_request_action(invocation, metadata);
|
||||
monitor_action(sess, turn_context, action).await
|
||||
monitor_action(
|
||||
sess,
|
||||
turn_context,
|
||||
action,
|
||||
mcp_tool_approval_callsite_mode(approval_mode, turn_context),
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
fn prepare_arc_request_action(
|
||||
|
|
@ -749,6 +762,22 @@ fn is_full_access_mode(turn_context: &TurnContext) -> bool {
|
|||
)
|
||||
}
|
||||
|
||||
fn mcp_tool_approval_callsite_mode(
|
||||
approval_mode: AppToolApproval,
|
||||
turn_context: &TurnContext,
|
||||
) -> &'static str {
|
||||
match approval_mode {
|
||||
AppToolApproval::Approve => MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_ALWAYS_ALLOW,
|
||||
AppToolApproval::Auto | AppToolApproval::Prompt => {
|
||||
if approval_mode == AppToolApproval::Auto && is_full_access_mode(turn_context) {
|
||||
MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_FULL_ACCESS
|
||||
} else {
|
||||
MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_DEFAULT
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn lookup_mcp_tool_metadata(
|
||||
sess: &Session,
|
||||
turn_context: &TurnContext,
|
||||
|
|
|
|||
|
|
@ -776,6 +776,38 @@ fn approval_elicitation_meta_merges_session_and_always_persist_with_connector_so
|
|||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn approval_callsite_mode_distinguishes_default_always_allow_and_full_access() {
|
||||
let (_session, mut turn_context) = make_session_and_context().await;
|
||||
|
||||
assert_eq!(
|
||||
mcp_tool_approval_callsite_mode(AppToolApproval::Auto, &turn_context),
|
||||
"mcp_tool_call__default"
|
||||
);
|
||||
assert_eq!(
|
||||
mcp_tool_approval_callsite_mode(AppToolApproval::Prompt, &turn_context),
|
||||
"mcp_tool_call__default"
|
||||
);
|
||||
assert_eq!(
|
||||
mcp_tool_approval_callsite_mode(AppToolApproval::Approve, &turn_context),
|
||||
"mcp_tool_call__always_allow"
|
||||
);
|
||||
|
||||
turn_context
|
||||
.approval_policy
|
||||
.set(AskForApproval::Never)
|
||||
.expect("test setup should allow updating approval policy");
|
||||
turn_context
|
||||
.sandbox_policy
|
||||
.set(SandboxPolicy::DangerFullAccess)
|
||||
.expect("test setup should allow updating sandbox policy");
|
||||
|
||||
assert_eq!(
|
||||
mcp_tool_approval_callsite_mode(AppToolApproval::Auto, &turn_context),
|
||||
"mcp_tool_call__full_access"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn declined_elicitation_response_stays_decline() {
|
||||
let response = parse_mcp_tool_approval_elicitation_response(
|
||||
|
|
@ -1035,6 +1067,83 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() {
|
|||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn full_access_auto_mode_blocks_when_arc_returns_interrupt_for_model() {
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::ResponseTemplate;
|
||||
use wiremock::matchers::method;
|
||||
use wiremock::matchers::path;
|
||||
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/codex/safety/arc"))
|
||||
.respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({
|
||||
"outcome": "steer-model",
|
||||
"short_reason": "needs approval",
|
||||
"rationale": "high-risk action",
|
||||
"risk_score": 96,
|
||||
"risk_level": "critical",
|
||||
"evidence": [{
|
||||
"message": "dangerous_tool",
|
||||
"why": "high-risk action",
|
||||
}],
|
||||
})))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let (session, mut turn_context) = make_session_and_context().await;
|
||||
turn_context.auth_manager = Some(crate::test_support::auth_manager_from_auth(
|
||||
crate::CodexAuth::create_dummy_chatgpt_auth_for_testing(),
|
||||
));
|
||||
turn_context
|
||||
.approval_policy
|
||||
.set(AskForApproval::Never)
|
||||
.expect("test setup should allow updating approval policy");
|
||||
turn_context
|
||||
.sandbox_policy
|
||||
.set(SandboxPolicy::DangerFullAccess)
|
||||
.expect("test setup should allow updating sandbox policy");
|
||||
let mut config = (*turn_context.config).clone();
|
||||
config.chatgpt_base_url = server.uri();
|
||||
turn_context.config = Arc::new(config);
|
||||
|
||||
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(),
|
||||
arguments: Some(serde_json::json!({ "id": 1 })),
|
||||
};
|
||||
let metadata = McpToolApprovalMetadata {
|
||||
annotations: Some(annotations(Some(false), Some(true), Some(true))),
|
||||
connector_id: Some("calendar".to_string()),
|
||||
connector_name: Some("Calendar".to_string()),
|
||||
connector_description: Some("Manage events".to_string()),
|
||||
tool_title: Some("Dangerous Tool".to_string()),
|
||||
tool_description: Some("Performs a risky action.".to_string()),
|
||||
codex_apps_meta: None,
|
||||
};
|
||||
|
||||
let decision = maybe_request_mcp_tool_approval(
|
||||
&session,
|
||||
&turn_context,
|
||||
"call-2",
|
||||
&invocation,
|
||||
Some(&metadata),
|
||||
AppToolApproval::Auto,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
decision,
|
||||
Some(McpToolApprovalDecision::BlockedBySafetyMonitor(
|
||||
"Tool call was cancelled because of safety risks: high-risk action".to_string(),
|
||||
))
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn approve_mode_routes_arc_ask_user_to_guardian_when_guardian_reviewer_is_enabled() {
|
||||
use wiremock::Mock;
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue