Compare commits

...

4 Commits

Author SHA1 Message Date
Felipe Coury
cc22f361f1 fix(core): skip custom mcp approval for empty annotations
Treat custom MCP tools with empty `ToolAnnotations` the same as
missing annotations when the tool is using the default `auto`
approval mode.

This avoids routing trivial stdio MCP tools into the approval
elicitation path in `exec` mode, which was causing `user cancelled
MCP tool call` failures for otherwise safe servers.
2026-04-28 12:28:43 -03:00
Felipe Coury
3d4f92194c docs(core): document MCP tool approval flow and regression fix contracts
Add doc comments to maybe_request_mcp_tool_approval,
should_skip_default_custom_mcp_approval, and the non-interactive
decline guard to make the early-exit ordering and its rationale
explicit.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-28 12:11:28 -03:00
Felipe Coury
133b766e4b fix(core): annotate metadata literals in mcp tool tests
Add the required `/*metadata*/` comments to the positional `None`
arguments in the custom MCP approval regression tests.

This fixes the argument-comment lint failures on the PR branch
without changing test behavior.
2026-04-28 12:11:18 -03:00
Felipe Coury
a9093c4417 fix(core): skip default approval for custom MCP tools without annotations
Skip the default app-tool approval path for non-`codex_apps` MCP
calls when the tool is in `auto` mode and metadata annotations are
missing.

This restores the expected custom MCP behavior described in `#15824`
for both interactive and non-interactive runs, preserves explicit
per-tool approval modes such as `prompt` and `approve`, and keeps the
regression tests easy to review with small local helpers.
2026-04-28 12:11:17 -03:00
2 changed files with 255 additions and 0 deletions

View File

@@ -46,6 +46,7 @@ use codex_mcp::mcp_permission_prompt_is_auto_approved;
use codex_otel::sanitize_metric_tag_value;
use codex_protocol::mcp::CallToolResult;
use codex_protocol::openai_models::InputModality;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::McpInvocation;
use codex_protocol::protocol::McpToolCallBeginEvent;
@@ -819,6 +820,24 @@ fn mcp_tool_approval_prompt_options(
}
}
/// Determines whether an MCP tool call requires user approval and, if so,
/// obtains a decision via the appropriate channel (interactive prompt, guardian,
/// or ARC safety monitor).
///
/// Returns `None` when the call may proceed without any approval gate, or
/// `Some(decision)` when an approval path was triggered. The caller must
/// inspect the decision variant to decide whether to execute or skip the tool.
///
/// The function's early-exit order matters:
/// 1. Full-access mode → skip everything.
/// 2. Custom MCP in `Auto` mode without annotations (or with empty annotations
/// where every hint is `None`) → skip (user opted in by configuring the
/// server; see `should_skip_default_custom_mcp_approval`).
/// 3. Annotation-based check → skip if annotations say the tool is safe and
/// the mode is not `Prompt`.
/// 4. ARC safety monitor (for `Approve` mode) → may block or escalate.
/// 5. Non-interactive guard → `Decline` rather than hang.
/// 6. Guardian / interactive prompt → obtain a user or guardian decision.
async fn maybe_request_mcp_tool_approval(
sess: &Arc<Session>,
turn_context: &Arc<TurnContext>,
@@ -836,6 +855,9 @@ async fn maybe_request_mcp_tool_approval(
}
let annotations = metadata.and_then(|metadata| metadata.annotations.as_ref());
if should_skip_default_custom_mcp_approval(invocation, approval_mode, annotations) {
return None;
}
let approval_required = requires_mcp_tool_approval(annotations);
if !approval_required && approval_mode != AppToolApproval::Prompt {
return None;
@@ -866,6 +888,14 @@ async fn maybe_request_mcp_tool_approval(
}
}
// In runs with `--ask-for-approval never` (for example `codex exec`), no
// user is available to answer the approval prompt. Decline rather than
// hanging forever. This check is intentionally placed *after* the ARC
// safety-monitor block so that `Approve`-mode tools are still
// safety-checked even when running non-interactively.
if matches!(turn_context.approval_policy.value(), AskForApproval::Never) {
return Some(McpToolApprovalDecision::Decline { message: None });
}
let session_approval_key = session_mcp_tool_approval_key(invocation, metadata, approval_mode);
let persistent_approval_key =
persistent_mcp_tool_approval_key(invocation, metadata, approval_mode);
@@ -1018,6 +1048,42 @@ async fn maybe_request_mcp_tool_approval(
Some(decision)
}
/// Returns `true` when a custom (non-Codex-Apps) MCP tool call should bypass the
/// default annotation-driven approval path entirely.
///
/// Codex Apps tools always carry `ToolAnnotations`, so the annotation-based logic in
/// `requires_mcp_tool_approval` is authoritative for them. Custom MCP servers, however,
/// may omit annotations entirely **or** supply a `ToolAnnotations` struct with every
/// hint field set to `None` (the "empty annotations" case — common for stdio-based
/// servers that echo the struct without populating it). Both cases signal the same
/// thing: the server has no opinion on the tool's risk profile.
///
/// When a custom tool is in the default `Auto` approval mode and annotations are absent
/// or empty, the user has already opted in by configuring the server — prompting them
/// again would be a regression from the expected behavior (see #15824).
///
/// This gate intentionally does **not** fire for `Prompt` or `Approve` modes: those
/// represent an explicit per-tool override that should always take effect regardless
/// of annotation presence.
///
/// Callers that set any concrete hint value (e.g. `destructive_hint: Some(true)`)
/// will fall through to the normal `requires_mcp_tool_approval` path, ensuring
/// annotation-based approval is still enforced whenever the server actually provides
/// risk information.
fn should_skip_default_custom_mcp_approval(
invocation: &McpInvocation,
approval_mode: AppToolApproval,
annotations: Option<&ToolAnnotations>,
) -> bool {
invocation.server != CODEX_APPS_MCP_SERVER_NAME
&& approval_mode == AppToolApproval::Auto
&& annotations.is_none_or(|annotations| {
annotations.destructive_hint.is_none()
&& annotations.read_only_hint.is_none()
&& annotations.open_world_hint.is_none()
})
}
async fn maybe_monitor_auto_approved_mcp_tool_call(
sess: &Session,
turn_context: &TurnContext,

View File

@@ -200,6 +200,34 @@ fn openai_file_params_are_only_honored_for_codex_apps() {
);
}
/// Builds an `McpInvocation` for a non-Codex-Apps server with no annotations,
/// representing the scenario that triggered the regression in #15824.
fn custom_mcp_invocation_without_annotations() -> McpInvocation {
McpInvocation {
server: "docs".to_string(),
tool: "search".to_string(),
arguments: Some(serde_json::json!({ "query": "approval regression" })),
}
}
/// Builds metadata whose `annotations` field is `Some(...)` but every hint inside
/// is `None` — the "empty annotations" variant that stdio MCP servers commonly emit.
fn custom_mcp_metadata_with_empty_annotations() -> McpToolApprovalMetadata {
McpToolApprovalMetadata {
annotations: Some(annotations(
/*read_only*/ None, /*destructive*/ None, /*open_world*/ None,
)),
connector_id: None,
connector_name: None,
connector_description: None,
tool_title: Some("Search".to_string()),
tool_description: Some("Search docs".to_string()),
mcp_app_resource_uri: None,
codex_apps_meta: None,
openai_file_input_params: None,
}
}
#[test]
fn approval_required_when_read_only_false_and_destructive() {
let annotations = annotations(Some(false), Some(true), /*open_world*/ None);
@@ -2073,6 +2101,167 @@ async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() {
);
}
#[tokio::test]
async fn custom_auto_mode_skips_approval_when_annotations_are_missing_in_never_mode() {
let (session, mut turn_context) = make_session_and_context().await;
turn_context
.approval_policy
.set(AskForApproval::Never)
.expect("test setup should allow updating approval policy");
let session = Arc::new(session);
let turn_context = Arc::new(turn_context);
let invocation = custom_mcp_invocation_without_annotations();
let decision = maybe_request_mcp_tool_approval(
&session,
&turn_context,
"call-custom-auto",
&invocation,
"mcp__docs__search",
/*metadata*/ None,
AppToolApproval::Auto,
)
.await;
assert_eq!(decision, None);
}
#[tokio::test]
async fn custom_auto_mode_skips_approval_when_annotations_are_missing_in_on_request_mode() {
let (session, turn_context, _rx_event) = make_session_and_context_with_rx().await;
{
let mut active_turn = session.active_turn.lock().await;
*active_turn = Some(ActiveTurn::default());
}
let session = Arc::new(session);
let turn_context = Arc::new(turn_context);
let invocation = custom_mcp_invocation_without_annotations();
let mut approval_task = {
let session = Arc::clone(&session);
let turn_context = Arc::clone(&turn_context);
tokio::spawn(async move {
maybe_request_mcp_tool_approval(
&session,
&turn_context,
"call-custom-auto-on-request",
&invocation,
"mcp__docs__search",
/*metadata*/ None,
AppToolApproval::Auto,
)
.await
})
};
let decision = tokio::time::timeout(std::time::Duration::from_millis(200), &mut approval_task)
.await
.expect("custom MCP tools should not wait for approval when annotations are missing")
.expect("approval task should complete successfully");
assert_eq!(decision, None);
}
#[tokio::test]
async fn custom_auto_mode_skips_approval_when_annotations_have_no_hints_in_on_request_mode() {
let (session, turn_context, _rx_event) = make_session_and_context_with_rx().await;
{
let mut active_turn = session.active_turn.lock().await;
*active_turn = Some(ActiveTurn::default());
}
let session = Arc::new(session);
let turn_context = Arc::new(turn_context);
let invocation = custom_mcp_invocation_without_annotations();
let metadata = custom_mcp_metadata_with_empty_annotations();
let mut approval_task = {
let session = Arc::clone(&session);
let turn_context = Arc::clone(&turn_context);
tokio::spawn(async move {
maybe_request_mcp_tool_approval(
&session,
&turn_context,
"call-custom-auto-empty-annotations-on-request",
&invocation,
"mcp__docs__search",
Some(&metadata),
AppToolApproval::Auto,
)
.await
})
};
let decision = tokio::time::timeout(std::time::Duration::from_millis(200), &mut approval_task)
.await
.expect("custom MCP tools with empty annotations should not wait for approval")
.expect("approval task should complete successfully");
assert_eq!(decision, None);
}
#[tokio::test]
async fn custom_approve_mode_without_metadata_still_uses_arc_monitor() {
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": "search",
"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(
codex_login::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: "docs".to_string(),
tool: "search".to_string(),
arguments: Some(serde_json::json!({ "query": "approval regression" })),
};
let decision = maybe_request_mcp_tool_approval(
&session,
&turn_context,
"call-custom-approve-no-metadata",
&invocation,
"mcp__docs__search",
/*metadata*/ None,
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 approve_mode_blocks_when_arc_returns_interrupt_without_annotations() {
use wiremock::Mock;