Compare commits

...

3 Commits

Author SHA1 Message Date
Fouad Matin
e635c55a3c fix: ci 2026-03-23 16:39:17 -07:00
Fouad Matin
533dfc462e fix: tests 2026-03-23 15:51:06 -07:00
Fouad Matin
d603ace047 fix(core): default approval behavior for mcp that are missing annotations 2026-03-23 09:29:43 -07:00
5 changed files with 127 additions and 15 deletions

View File

@@ -495,7 +495,7 @@ async fn maybe_request_mcp_tool_approval(
approval_mode: AppToolApproval,
) -> Option<McpToolApprovalDecision> {
let annotations = metadata.and_then(|metadata| metadata.annotations.as_ref());
let approval_required = annotations.is_some_and(requires_mcp_tool_approval);
let approval_required = requires_mcp_tool_approval(annotations);
let mut monitor_reason = None;
let auto_approved_by_policy = approval_mode == AppToolApproval::Approve
|| (approval_mode == AppToolApproval::Auto && is_full_access_mode(turn_context));
@@ -1299,12 +1299,23 @@ async fn persist_codex_app_tool_approval(
.await
}
fn requires_mcp_tool_approval(annotations: &ToolAnnotations) -> bool {
if annotations.destructive_hint == Some(true) {
fn requires_mcp_tool_approval(annotations: Option<&ToolAnnotations>) -> bool {
let destructive_hint = annotations.and_then(|annotations| annotations.destructive_hint);
if destructive_hint == Some(true) {
return true;
}
annotations.read_only_hint == Some(false) && annotations.open_world_hint == Some(true)
let read_only_hint = annotations
.and_then(|annotations| annotations.read_only_hint)
.unwrap_or(false);
if read_only_hint {
return false;
}
destructive_hint.unwrap_or(true)
|| annotations
.and_then(|annotations| annotations.open_world_hint)
.unwrap_or(true)
}
async fn notify_mcp_tool_call_skip(

View File

@@ -64,19 +64,30 @@ fn prompt_options(
#[test]
fn approval_required_when_read_only_false_and_destructive() {
let annotations = annotations(Some(false), Some(true), None);
assert_eq!(requires_mcp_tool_approval(&annotations), true);
assert_eq!(requires_mcp_tool_approval(Some(&annotations)), true);
}
#[test]
fn approval_required_when_read_only_false_and_open_world() {
let annotations = annotations(Some(false), None, Some(true));
assert_eq!(requires_mcp_tool_approval(&annotations), true);
assert_eq!(requires_mcp_tool_approval(Some(&annotations)), true);
}
#[test]
fn approval_required_when_destructive_even_if_read_only_true() {
let annotations = annotations(Some(true), Some(true), Some(true));
assert_eq!(requires_mcp_tool_approval(&annotations), true);
assert_eq!(requires_mcp_tool_approval(Some(&annotations)), true);
}
#[test]
fn approval_required_when_annotations_are_absent() {
assert_eq!(requires_mcp_tool_approval(None), true);
}
#[test]
fn approval_not_required_when_read_only_and_other_hints_are_absent() {
let annotations = annotations(Some(true), None, None);
assert_eq!(requires_mcp_tool_approval(Some(&annotations)), false);
}
#[test]
@@ -1067,6 +1078,75 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() {
);
}
#[tokio::test]
async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() {
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(),
));
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: None,
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-3",
&invocation,
Some(&metadata),
AppToolApproval::Approve,
)
.await;
assert_eq!(
decision,
Some(McpToolApprovalDecision::BlockedBySafetyMonitor(
"Tool call was cancelled because of safety risks: high-risk action".to_string(),
))
);
}
#[tokio::test]
async fn full_access_auto_mode_blocks_when_arc_returns_interrupt_for_model() {
use wiremock::Mock;

View File

@@ -185,6 +185,11 @@ impl Respond for CodexAppsJsonRpcResponder {
{
"name": "calendar_create_event",
"description": "Create a calendar event.",
"annotations": {
"readOnlyHint": false,
"destructiveHint": false,
"openWorldHint": false
},
"inputSchema": {
"type": "object",
"properties": {
@@ -209,6 +214,9 @@ impl Respond for CodexAppsJsonRpcResponder {
{
"name": "calendar_list_events",
"description": "List calendar events.",
"annotations": {
"readOnlyHint": true
},
"inputSchema": {
"type": "object",
"properties": {
@@ -241,6 +249,9 @@ impl Respond for CodexAppsJsonRpcResponder {
tools.push(json!({
"name": format!("calendar_timezone_option_{index}"),
"description": format!("Read timezone option {index}."),
"annotations": {
"readOnlyHint": true
},
"inputSchema": {
"type": "object",
"properties": {

View File

@@ -22,6 +22,7 @@ use rmcp::model::ResourceTemplate;
use rmcp::model::ServerCapabilities;
use rmcp::model::ServerInfo;
use rmcp::model::Tool;
use rmcp::model::ToolAnnotations;
use serde::Deserialize;
use serde_json::json;
use tokio::task;
@@ -85,11 +86,13 @@ impl TestToolServer {
}))
.expect("echo tool schema should deserialize");
Tool::new(
let mut tool = Tool::new(
Cow::Borrowed(name),
Cow::Borrowed(description),
Arc::new(schema),
)
);
tool.annotations = Some(ToolAnnotations::new().read_only(true));
tool
}
fn image_tool() -> Tool {
@@ -101,11 +104,13 @@ impl TestToolServer {
}))
.expect("image tool schema should deserialize");
Tool::new(
let mut tool = Tool::new(
Cow::Borrowed("image"),
Cow::Borrowed("Return a single image content block."),
Arc::new(schema),
)
);
tool.annotations = Some(ToolAnnotations::new().read_only(true));
tool
}
/// Tool intended for manual testing of Codex TUI rendering for MCP image tool results.
@@ -154,13 +159,15 @@ impl TestToolServer {
}))
.expect("image_scenario tool schema should deserialize");
Tool::new(
let mut tool = Tool::new(
Cow::Borrowed("image_scenario"),
Cow::Borrowed(
"Return content blocks for manual testing of MCP image rendering scenarios.",
),
Arc::new(schema),
)
);
tool.annotations = Some(ToolAnnotations::new().read_only(true));
tool
}
fn memo_resource() -> Resource {

View File

@@ -38,6 +38,7 @@ use rmcp::model::ResourceTemplate;
use rmcp::model::ServerCapabilities;
use rmcp::model::ServerInfo;
use rmcp::model::Tool;
use rmcp::model::ToolAnnotations;
use rmcp::transport::StreamableHttpServerConfig;
use rmcp::transport::StreamableHttpService;
use rmcp::transport::streamable_http_server::session::local::LocalSessionManager;
@@ -84,11 +85,13 @@ impl TestToolServer {
}))
.expect("echo tool schema should deserialize");
Tool::new(
let mut tool = Tool::new(
Cow::Borrowed("echo"),
Cow::Borrowed("Echo back the provided message and include environment data."),
Arc::new(schema),
)
);
tool.annotations = Some(ToolAnnotations::new().read_only(true));
tool
}
fn memo_resource() -> Resource {