mirror of
https://github.com/openai/codex.git
synced 2026-11-02 19:43:24 +00:00
Require synchronous review for sensitive MCP actions (#41094)
## What changed - Recognize `codex_sensitive_action: true` on MCP approval elicitations. - Route marked actions to the synchronous Guardian reviewer instead of extension fast approval. Absent or false values preserve the existing path. - Cover ordinary and strict auto-review configurations, including marked, unmarked, and explicitly false metadata. GitOrigin-RevId: dce13d1696568ca81c8d2b08a7ef0d5f13e155bf
This commit is contained in:
@@ -47,7 +47,8 @@ struct CuratedMcpSyncFixture {
|
||||
impl CuratedMcpSyncFixture {
|
||||
async fn set_up() -> Result<Self> {
|
||||
let responses_server = responses::start_mock_server().await;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) =
|
||||
start_mcp_server(/*sensitive_action*/ None).await?;
|
||||
let fixture_root = TempDir::new()?;
|
||||
let codex_home = fixture_root.path().join("codex-home");
|
||||
let curated_repo = fixture_root.path().join("plugins.git");
|
||||
|
||||
@@ -508,6 +508,7 @@ async fn guardian_v2_routes_tool_approvals(
|
||||
review_outcome,
|
||||
transcript_content,
|
||||
GuardianToolScope::AllTools,
|
||||
/*sensitive_action*/ None,
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -519,6 +520,7 @@ async fn guardian_v2_routes_scoped_tool_approvals(
|
||||
review_outcome: ReviewOutcome,
|
||||
transcript_content: TranscriptContent,
|
||||
scope: GuardianToolScope,
|
||||
sensitive_action: Option<bool>,
|
||||
) -> Result<()> {
|
||||
let server_name = match scope {
|
||||
GuardianToolScope::AllTools => TEST_SERVER_NAME,
|
||||
@@ -533,7 +535,7 @@ async fn guardian_v2_routes_scoped_tool_approvals(
|
||||
let node_repl_review_required = matches!(requirement, ModelReviewRequirement::Required)
|
||||
&& codex_protocol::mcp::is_node_repl_backed_server(server_name);
|
||||
let (luna_score, expected_guardian_reviews) = match risk {
|
||||
GuardianRisk::Low if classifier_in_scope => (0.25, 1),
|
||||
GuardianRisk::Low if classifier_in_scope && sensitive_action != Some(true) => (0.25, 1),
|
||||
GuardianRisk::Low | GuardianRisk::InvalidResponse => (0.25, 2),
|
||||
GuardianRisk::Threshold => (0.5, 2),
|
||||
GuardianRisk::High => (0.95, 2),
|
||||
@@ -579,7 +581,7 @@ async fn guardian_v2_routes_scoped_tool_approvals(
|
||||
let responses_server = tokio::spawn(async move {
|
||||
let _ = axum::serve(listener, router).await;
|
||||
});
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server(sensitive_action).await?;
|
||||
|
||||
let codex_home = TempDir::new()?;
|
||||
if lifecycle.has_post_tool_hook() {
|
||||
@@ -1258,7 +1260,7 @@ async fn guardian_v2_trusts_invoked_user_skills_but_rejects_repository_forgery()
|
||||
let responses_server = tokio::spawn(async move {
|
||||
let _ = axum::serve(listener, router).await;
|
||||
});
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server(/*sensitive_action*/ None).await?;
|
||||
|
||||
MockResponsesConfig::new(&responses_url)
|
||||
.with_model(MODEL)
|
||||
@@ -1393,21 +1395,27 @@ async fn guardian_v2_computer_use_only_scopes_classification_and_fast_reviews(
|
||||
ReviewOutcome::Allow,
|
||||
TranscriptContent::Normal,
|
||||
GuardianToolScope::ComputerUseOnly { server_name },
|
||||
/*sensitive_action*/ None,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
#[test_case("node_repl", GuardianRisk::Low; "browser low risk")]
|
||||
#[test_case("cua_repl", GuardianRisk::Low; "computer use low risk")]
|
||||
#[test_case("node_repl", GuardianRisk::High; "browser high risk")]
|
||||
#[test_case("cua_repl", GuardianRisk::High; "computer use high risk")]
|
||||
#[test_case("node_repl", GuardianRisk::InvalidResponse; "browser classifier failure")]
|
||||
#[test_case("cua_repl", GuardianRisk::InvalidResponse; "computer use classifier failure")]
|
||||
#[test_case(TEST_SERVER_NAME, GuardianRisk::Low; "other tools retain full review")]
|
||||
#[test_case("node_repl", GuardianRisk::Low, None; "browser low risk")]
|
||||
#[test_case("cua_repl", GuardianRisk::Low, None; "computer use low risk")]
|
||||
#[test_case("node_repl", GuardianRisk::Low, Some(false); "browser low risk sensitive action false")]
|
||||
#[test_case("cua_repl", GuardianRisk::Low, Some(false); "computer use low risk sensitive action false")]
|
||||
#[test_case("node_repl", GuardianRisk::Low, Some(true); "browser low risk sensitive action true")]
|
||||
#[test_case("cua_repl", GuardianRisk::Low, Some(true); "computer use low risk sensitive action true")]
|
||||
#[test_case("node_repl", GuardianRisk::High, None; "browser high risk")]
|
||||
#[test_case("cua_repl", GuardianRisk::High, None; "computer use high risk")]
|
||||
#[test_case("node_repl", GuardianRisk::InvalidResponse, None; "browser classifier failure")]
|
||||
#[test_case("cua_repl", GuardianRisk::InvalidResponse, None; "computer use classifier failure")]
|
||||
#[test_case(TEST_SERVER_NAME, GuardianRisk::Low, None; "other tools retain full review")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn guardian_v2_required_model_computer_use_preserves_strict_approval(
|
||||
server_name: &'static str,
|
||||
risk: GuardianRisk,
|
||||
sensitive_action: Option<bool>,
|
||||
) -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
guardian_v2_routes_scoped_tool_approvals(
|
||||
@@ -1417,6 +1425,7 @@ async fn guardian_v2_required_model_computer_use_preserves_strict_approval(
|
||||
ReviewOutcome::Allow,
|
||||
TranscriptContent::Normal,
|
||||
GuardianToolScope::ComputerUseOnly { server_name },
|
||||
sensitive_action,
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -1437,6 +1446,7 @@ async fn guardian_v2_discards_sync_reviews_after_user_input_answer(
|
||||
GuardianToolScope::ComputerUseOnly {
|
||||
server_name: "node_repl",
|
||||
},
|
||||
/*sensitive_action*/ None,
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -1458,6 +1468,7 @@ async fn guardian_v2_validates_user_input_before_history_truncation(
|
||||
GuardianToolScope::ComputerUseOnly {
|
||||
server_name: "node_repl",
|
||||
},
|
||||
/*sensitive_action*/ None,
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -1478,6 +1489,7 @@ async fn guardian_v2_propagates_root_user_input_to_worker_reviews(
|
||||
GuardianToolScope::ComputerUseOnly {
|
||||
server_name: "node_repl",
|
||||
},
|
||||
/*sensitive_action*/ None,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
@@ -100,7 +100,7 @@ const LATE_ENVIRONMENT_ID: &str = "late-environment";
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn mcp_server_tool_call_returns_tool_result() -> Result<()> {
|
||||
let responses_server = responses::start_mock_server().await;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server(/*sensitive_action*/ None).await?;
|
||||
let codex_home = TempDir::new()?;
|
||||
mcp_tool_config(&responses_server.uri(), &mcp_server_url, AUTO_COMPACT_LIMIT)
|
||||
.write(codex_home.path())?;
|
||||
@@ -166,7 +166,7 @@ async fn mcp_server_tool_call_returns_tool_result() -> Result<()> {
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn mcp_server_tool_call_forwards_only_server_extensions() -> Result<()> {
|
||||
let responses_server = responses::start_mock_server().await;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server(/*sensitive_action*/ None).await?;
|
||||
let codex_home = TempDir::new()?;
|
||||
mcp_tool_config(&responses_server.uri(), &mcp_server_url, AUTO_COMPACT_LIMIT)
|
||||
.write(codex_home.path())?;
|
||||
@@ -261,7 +261,7 @@ async fn model_mcp_tool_call_uses_session_client_extensions() -> Result<()> {
|
||||
create_final_assistant_message_sse_response("done")?,
|
||||
];
|
||||
let responses_server = create_mock_responses_server_sequence(responses).await;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server(/*sensitive_action*/ None).await?;
|
||||
let codex_home = TempDir::new()?;
|
||||
mcp_tool_config(&responses_server.uri(), &mcp_server_url, AUTO_COMPACT_LIMIT)
|
||||
.write(codex_home.path())?;
|
||||
@@ -491,7 +491,7 @@ async fn assert_full_access_form_elicitation_is_declined(
|
||||
case: FullAccessElicitationCase,
|
||||
) -> Result<()> {
|
||||
let responses_server = responses::start_mock_server().await;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server(/*sensitive_action*/ None).await?;
|
||||
let codex_home = TempDir::new()?;
|
||||
mcp_tool_config(&responses_server.uri(), &mcp_server_url, AUTO_COMPACT_LIMIT)
|
||||
.write(codex_home.path())?;
|
||||
@@ -565,7 +565,7 @@ async fn mcp_server_tool_call_round_trips_elicitation_for_thread(
|
||||
mut elicitation_thread: ElicitationThread,
|
||||
) -> Result<()> {
|
||||
let responses_server = responses::start_mock_server().await;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server(/*sensitive_action*/ None).await?;
|
||||
let codex_home = TempDir::new()?;
|
||||
mcp_tool_config(&responses_server.uri(), &mcp_server_url, AUTO_COMPACT_LIMIT)
|
||||
.write(codex_home.path())?;
|
||||
@@ -727,7 +727,7 @@ async fn initialize_elicitation_app_server(
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn mcp_server_elicitation_survives_environment_runtime_refresh() -> Result<()> {
|
||||
let responses_server = responses::start_mock_server().await;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server(/*sensitive_action*/ None).await?;
|
||||
let exec_listener = TcpListener::bind("127.0.0.1:0").await?;
|
||||
let exec_server_url = format!("ws://{}", exec_listener.local_addr()?);
|
||||
let codex_home = TempDir::new()?;
|
||||
@@ -847,7 +847,7 @@ async fn mcp_server_elicitation_survives_environment_runtime_refresh() -> Result
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn mcp_server_tool_call_forwards_url_elicitation() -> Result<()> {
|
||||
let responses_server = responses::start_mock_server().await;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server(/*sensitive_action*/ None).await?;
|
||||
let codex_home = TempDir::new()?;
|
||||
mcp_tool_config(&responses_server.uri(), &mcp_server_url, AUTO_COMPACT_LIMIT)
|
||||
.write(codex_home.path())?;
|
||||
@@ -944,7 +944,7 @@ async fn mcp_tool_call_completion_notification_contains_truncated_large_result()
|
||||
create_final_assistant_message_sse_response("done")?,
|
||||
];
|
||||
let responses_server = create_mock_responses_server_sequence(responses).await;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server(/*sensitive_action*/ None).await?;
|
||||
let codex_home = TempDir::new()?;
|
||||
mcp_tool_config(
|
||||
&responses_server.uri(),
|
||||
@@ -1057,7 +1057,7 @@ async fn mcp_tool_call_hint_survives_mid_call_thread_read_and_resume() -> Result
|
||||
create_final_assistant_message_sse_response("done")?,
|
||||
];
|
||||
let responses_server = create_mock_responses_server_sequence(responses).await;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server(/*sensitive_action*/ None).await?;
|
||||
let codex_home = TempDir::new()?;
|
||||
mcp_tool_config(&responses_server.uri(), &mcp_server_url, AUTO_COMPACT_LIMIT)
|
||||
.write(codex_home.path())?;
|
||||
@@ -1196,7 +1196,9 @@ async fn mcp_tool_call_hint_survives_mid_call_thread_read_and_resume() -> Result
|
||||
}
|
||||
|
||||
#[derive(Clone, Default)]
|
||||
struct ToolAppsMcpServer;
|
||||
struct ToolAppsMcpServer {
|
||||
sensitive_action: Option<bool>,
|
||||
}
|
||||
|
||||
impl ServerHandler for ToolAppsMcpServer {
|
||||
async fn initialize(
|
||||
@@ -1273,7 +1275,7 @@ impl ServerHandler for ToolAppsMcpServer {
|
||||
.and_then(serde_json::Value::as_bool)
|
||||
== Some(true)
|
||||
{
|
||||
let approval_meta = json!({
|
||||
let mut approval_meta = json!({
|
||||
"codex_request_type": "approval_request",
|
||||
"codex_approval_kind": "mcp_tool_call",
|
||||
"codex_strict_auto_review": true,
|
||||
@@ -1281,6 +1283,9 @@ impl ServerHandler for ToolAppsMcpServer {
|
||||
"tool_params": request.arguments,
|
||||
"x-codex-turn-metadata": turn_metadata,
|
||||
});
|
||||
if let Some(sensitive_action) = self.sensitive_action {
|
||||
approval_meta["codex_sensitive_action"] = json!(sensitive_action);
|
||||
}
|
||||
let result = context
|
||||
.peer
|
||||
.create_elicitation(ElicitRequestParams::FormElicitationParams {
|
||||
@@ -1398,11 +1403,13 @@ impl ServerHandler for ToolAppsMcpServer {
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) async fn start_mcp_server() -> Result<(String, JoinHandle<()>)> {
|
||||
pub(super) async fn start_mcp_server(
|
||||
sensitive_action: Option<bool>,
|
||||
) -> Result<(String, JoinHandle<()>)> {
|
||||
let listener = TcpListener::bind("127.0.0.1:0").await?;
|
||||
let addr = listener.local_addr()?;
|
||||
let mcp_service = StreamableHttpService::new(
|
||||
|| Ok(ToolAppsMcpServer),
|
||||
move || Ok(ToolAppsMcpServer { sensitive_action }),
|
||||
Arc::new(LocalSessionManager::default()),
|
||||
StreamableHttpServerConfig::default(),
|
||||
);
|
||||
|
||||
@@ -41,7 +41,7 @@ async fn fresh_context_subagent_inherits_disabled_view_image_and_mcp_tools() ->
|
||||
const MCP_CALL_ID: &str = "call-child-mcp-with-disabled-view-image";
|
||||
|
||||
let responses_server = responses::start_mock_server().await;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server().await?;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server(/*sensitive_action*/ None).await?;
|
||||
let codex_home = TempDir::new()?;
|
||||
MockResponsesConfig::new(&responses_server.uri())
|
||||
.with_model("gpt-5.4")
|
||||
|
||||
@@ -322,7 +322,8 @@ async fn run_guardian_review(
|
||||
options: GuardianReviewOptions,
|
||||
) -> ReviewDecision {
|
||||
let turn = Arc::clone(context.turn());
|
||||
let requires_synchronous_review = reasons.retry.is_some()
|
||||
let requires_synchronous_review = options.require_synchronous_review
|
||||
|| reasons.retry.is_some()
|
||||
|| matches!(
|
||||
&request,
|
||||
GuardianApprovalRequest::ExecCommand {
|
||||
@@ -331,7 +332,8 @@ async fn run_guardian_review(
|
||||
} if sandbox_permissions.requires_escalated_permissions()
|
||||
);
|
||||
// Guardian V2 may satisfy ordinary reviews, including required-model reviews, but broader
|
||||
// permission requests and retries must run Guardian synchronously.
|
||||
// permission requests, retries, and elicitations requiring synchronous review must not use
|
||||
// extension fast approval.
|
||||
if (!turn
|
||||
.config
|
||||
.config_layer_stack
|
||||
@@ -368,6 +370,7 @@ async fn run_guardian_review(
|
||||
plugin_attribution_override,
|
||||
approval_request_source,
|
||||
external_cancel,
|
||||
require_synchronous_review: _,
|
||||
} = options;
|
||||
let target_item_id = guardian_request_target_item_id(&request).map(str::to_string);
|
||||
let assessment_turn_id = guardian_request_turn_id(&request, &turn.sub_id).to_string();
|
||||
@@ -739,6 +742,8 @@ pub(crate) struct GuardianReviewOptions {
|
||||
pub(crate) plugin_attribution_override: Option<PluginCommandAttribution>,
|
||||
pub(crate) approval_request_source: GuardianApprovalRequestSource,
|
||||
pub(crate) external_cancel: Option<CancellationToken>,
|
||||
/// Escalate from extension fast approval to the synchronous Guardian reviewer.
|
||||
pub(crate) require_synchronous_review: bool,
|
||||
}
|
||||
|
||||
/// Public entrypoint for approval requests that should be reviewed by guardian.
|
||||
@@ -760,6 +765,7 @@ pub(crate) async fn review_approval_request(
|
||||
plugin_attribution_override: None,
|
||||
approval_request_source: GuardianApprovalRequestSource::MainTurn,
|
||||
external_cancel: None,
|
||||
require_synchronous_review: false,
|
||||
},
|
||||
));
|
||||
review.await
|
||||
|
||||
@@ -1506,6 +1506,7 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
|
||||
plugin_attribution_override: None,
|
||||
approval_request_source: GuardianApprovalRequestSource::MainTurn,
|
||||
external_cancel: Some(cancel_token),
|
||||
require_synchronous_review: false,
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -23,6 +23,7 @@ use codex_protocol::mcp_approval_meta::CONNECTOR_NAME_KEY as MCP_ELICITATION_CON
|
||||
use codex_protocol::mcp_approval_meta::PERSIST_KEY as MCP_ELICITATION_PERSIST_KEY;
|
||||
use codex_protocol::mcp_approval_meta::REQUEST_TYPE_APPROVAL_REQUEST as MCP_ELICITATION_REQUEST_TYPE_APPROVAL_REQUEST;
|
||||
use codex_protocol::mcp_approval_meta::REQUEST_TYPE_KEY as MCP_ELICITATION_REQUEST_TYPE_KEY;
|
||||
use codex_protocol::mcp_approval_meta::SENSITIVE_ACTION_KEY as MCP_ELICITATION_SENSITIVE_ACTION_KEY;
|
||||
use codex_protocol::mcp_approval_meta::STRICT_AUTO_REVIEW_KEY as MCP_ELICITATION_STRICT_AUTO_REVIEW_KEY;
|
||||
use codex_protocol::mcp_approval_meta::TOOL_DESCRIPTION_KEY as MCP_ELICITATION_TOOL_DESCRIPTION_KEY;
|
||||
use codex_protocol::mcp_approval_meta::TOOL_NAME_KEY as MCP_ELICITATION_TOOL_NAME_KEY;
|
||||
@@ -723,6 +724,14 @@ async fn review_guardian_mcp_elicitation(
|
||||
return Ok(None);
|
||||
};
|
||||
|
||||
let require_synchronous_review = matches!(
|
||||
request
|
||||
.elicitation
|
||||
.meta()
|
||||
.and_then(|meta| meta.get(MCP_ELICITATION_SENSITIVE_ACTION_KEY)),
|
||||
Some(Value::Bool(true))
|
||||
);
|
||||
|
||||
if matches!(
|
||||
request
|
||||
.elicitation
|
||||
@@ -809,6 +818,7 @@ async fn review_guardian_mcp_elicitation(
|
||||
plugin_attribution_override: None,
|
||||
approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn,
|
||||
external_cancel: Some(cancellation_token),
|
||||
require_synchronous_review,
|
||||
},
|
||||
)
|
||||
.await;
|
||||
@@ -897,6 +907,7 @@ async fn review_guardian_mcp_elicitation(
|
||||
plugin_attribution_override: None,
|
||||
approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn,
|
||||
external_cancel: Some(cancellation_token),
|
||||
require_synchronous_review,
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -631,6 +631,7 @@ impl Session {
|
||||
plugin_attribution_override: None,
|
||||
approval_request_source: GuardianApprovalRequestSource::MainTurn,
|
||||
external_cancel: Some(cancellation_token.clone()),
|
||||
require_synchronous_review: false,
|
||||
},
|
||||
);
|
||||
review.await.unwrap_or_else(|_| {
|
||||
@@ -653,6 +654,7 @@ impl Session {
|
||||
plugin_attribution_override: None,
|
||||
approval_request_source: GuardianApprovalRequestSource::MainTurn,
|
||||
external_cancel: Some(review_cancel),
|
||||
require_synchronous_review: false,
|
||||
},
|
||||
)
|
||||
.await
|
||||
|
||||
@@ -3,10 +3,12 @@ use anyhow::Result;
|
||||
use codex_core::TurnInputRequest;
|
||||
use codex_core::config::Constrained;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::openai_models::AutoReviewMessages;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::GuardianAssessmentStatus;
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::protocol::ThreadSettingsOverrides;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::responses;
|
||||
@@ -15,9 +17,13 @@ use core_test_support::skip_if_wine_exec;
|
||||
use core_test_support::test_codex::local_selections;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::wait_for_mcp_server;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::Value;
|
||||
use serde_json::json;
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
use test_case::test_case;
|
||||
use wiremock::matchers::body_partial_json;
|
||||
|
||||
// The tool itself needs no approval. Its server requests a separate Guardian
|
||||
// review after tools/call, exercising the ordinary MCP elicitation path.
|
||||
@@ -29,6 +35,7 @@ def send(message):
|
||||
print(json.dumps({"jsonrpc": "2.0", **message}), flush=True)
|
||||
|
||||
pending_call = None
|
||||
approval_meta = json.loads(sys.argv[1]) if len(sys.argv) > 1 else {}
|
||||
for line in sys.stdin:
|
||||
request = json.loads(line)
|
||||
method = request.get("method")
|
||||
@@ -46,7 +53,8 @@ for line in sys.stdin:
|
||||
"message": "Approve the server-side action?",
|
||||
"requestedSchema": {"type": "object", "properties": {}},
|
||||
"_meta": {"codex_request_type": "approval_request",
|
||||
"codex_approval_kind": "mcp_tool_call", "tool_name": "write_record"}}})
|
||||
"codex_approval_kind": "mcp_tool_call", "tool_name": "write_record",
|
||||
**approval_meta}}})
|
||||
continue
|
||||
elif method is None and request.get("id") == "server-approval":
|
||||
send({"id": pending_call, "result": {"content": [
|
||||
@@ -172,3 +180,164 @@ async fn interrupt_aborts_server_initiated_mcp_guardian_review() -> Result<()> {
|
||||
test.codex.shutdown_and_wait().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(None, false; "ordinary_absent")]
|
||||
#[test_case(Some(false), false; "ordinary_false")]
|
||||
#[test_case(Some(true), false; "ordinary_true")]
|
||||
#[test_case(None, true; "strict_absent")]
|
||||
#[test_case(Some(false), true; "strict_false")]
|
||||
#[test_case(Some(true), true; "strict_true")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn server_initiated_mcp_elicitation_can_require_synchronous_auto_review(
|
||||
sensitive_action: Option<bool>,
|
||||
strict_auto_review: bool,
|
||||
) -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_wine_exec!(Ok(()), "the MCP fixture requires a host Python interpreter");
|
||||
|
||||
const RATIONALE: &str = "The server-side action is not authorized.";
|
||||
const REJECTION_INSTRUCTIONS: &str = "Do not retry the rejected action.";
|
||||
|
||||
struct AutoApprovingReviewContributor;
|
||||
|
||||
impl codex_extension_api::ApprovalReviewContributor for AutoApprovingReviewContributor {
|
||||
fn fast_decision<'a>(
|
||||
&'a self,
|
||||
_session_store: &'a codex_extension_api::ExtensionData,
|
||||
_thread_store: &'a codex_extension_api::ExtensionData,
|
||||
_prompt: &'a str,
|
||||
_extension_metrics: Option<Arc<dyn codex_extension_api::ExtensionMetrics>>,
|
||||
) -> codex_extension_api::ExtensionFuture<'a, Option<ReviewDecision>> {
|
||||
Box::pin(async { Some(ReviewDecision::Approved) })
|
||||
}
|
||||
}
|
||||
|
||||
let mut extensions = codex_extension_api::ExtensionRegistryBuilder::new();
|
||||
extensions.approval_review_contributor(Arc::new(AutoApprovingReviewContributor));
|
||||
let mut meta = serde_json::Map::new();
|
||||
if strict_auto_review {
|
||||
meta.insert("codex_strict_auto_review".to_string(), json!(true));
|
||||
}
|
||||
if let Some(sensitive_action) = sensitive_action {
|
||||
meta.insert(
|
||||
"codex_sensitive_action".to_string(),
|
||||
json!(sensitive_action),
|
||||
);
|
||||
}
|
||||
|
||||
let server = responses::start_mock_server().await;
|
||||
let mcp_servers = serde_json::from_value(json!({
|
||||
"elicitation": {
|
||||
"command": if cfg!(windows) { "python" } else { "python3" },
|
||||
"args": ["-u", "-c", ELICITATION_SERVER, serde_json::to_string(&meta)?],
|
||||
"default_tools_approval_mode": "approve",
|
||||
}
|
||||
}))?;
|
||||
let mut builder = test_codex()
|
||||
.with_extensions(Arc::new(extensions.build()))
|
||||
.with_model_info_override("gpt-5.5", |model| {
|
||||
model
|
||||
.model_messages
|
||||
.as_mut()
|
||||
.expect("acting model messages")
|
||||
.auto_review = Some(AutoReviewMessages {
|
||||
policy: None,
|
||||
policy_template: None,
|
||||
rejection_instructions: Some(REJECTION_INSTRUCTIONS.to_string()),
|
||||
timeout_instructions: None,
|
||||
});
|
||||
})
|
||||
.with_config(move |config| {
|
||||
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||
config.approvals_reviewer = if strict_auto_review {
|
||||
ApprovalsReviewer::User
|
||||
} else {
|
||||
ApprovalsReviewer::AutoReview
|
||||
};
|
||||
config
|
||||
.mcp_servers
|
||||
.set(mcp_servers)
|
||||
.expect("set MCP fixture");
|
||||
});
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
wait_for_mcp_server(&test.codex, "elicitation").await?;
|
||||
responses::mount_sse_once(
|
||||
&server,
|
||||
responses::sse(vec![
|
||||
responses::ev_function_call_with_namespace(
|
||||
"eliciting-tool",
|
||||
"mcp__elicitation",
|
||||
"request_approval",
|
||||
"{}",
|
||||
),
|
||||
responses::ev_completed("parent-tool"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let guardian = responses::mount_sse_once_match(
|
||||
&server,
|
||||
body_partial_json(json!({"client_metadata": {"x-openai-subagent": "guardian"}})),
|
||||
responses::sse(vec![
|
||||
responses::ev_assistant_message(
|
||||
"review-result",
|
||||
&json!({
|
||||
"risk_level": "high",
|
||||
"user_authorization": "low",
|
||||
"outcome": "deny",
|
||||
"rationale": RATIONALE,
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
responses::ev_completed("guardian-review"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let parent = responses::mount_sse_once(
|
||||
&server,
|
||||
responses::sse(vec![responses::ev_completed("parent-complete")]),
|
||||
)
|
||||
.await;
|
||||
|
||||
test.submit_text_turn("Run the tool that requests server-side approval.")
|
||||
.await?;
|
||||
|
||||
let output = parent
|
||||
.single_request()
|
||||
.function_call_output("eliciting-tool");
|
||||
let response: Value = serde_json::from_str(
|
||||
output["output"][1]["text"]
|
||||
.as_str()
|
||||
.expect("MCP fixture should echo the wire response"),
|
||||
)?;
|
||||
let requires_sync = sensitive_action == Some(true);
|
||||
let expected_response = if requires_sync {
|
||||
json!({
|
||||
"action": "decline",
|
||||
"_meta": {
|
||||
"approvals_reviewer": "auto_review",
|
||||
"message": format!(
|
||||
"This action was rejected due to unacceptable risk.\nReason: {RATIONALE}\n{REJECTION_INSTRUCTIONS}"
|
||||
),
|
||||
},
|
||||
})
|
||||
} else {
|
||||
json!({
|
||||
"action": "accept",
|
||||
"content": {},
|
||||
"_meta": {"approvals_reviewer": "auto_review"},
|
||||
})
|
||||
};
|
||||
assert_eq!(response, expected_response);
|
||||
|
||||
let guardian_requests = guardian
|
||||
.requests()
|
||||
.into_iter()
|
||||
.filter(|request| request.body_json()["client_metadata"]["x-openai-subagent"] == "guardian")
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(guardian_requests.len(), usize::from(requires_sync));
|
||||
if requires_sync {
|
||||
assert!(guardian_requests[0].body_contains_text("write_record"));
|
||||
}
|
||||
test.codex.shutdown_and_wait().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -7,6 +7,9 @@ pub const APPROVAL_KIND_TOOL_SUGGESTION: &str = "tool_suggestion";
|
||||
pub const REQUEST_TYPE_KEY: &str = "codex_request_type";
|
||||
pub const REQUEST_TYPE_APPROVAL_REQUEST: &str = "approval_request";
|
||||
pub const STRICT_AUTO_REVIEW_KEY: &str = "codex_strict_auto_review";
|
||||
/// Marks a sensitive action requiring synchronous review when routed to automatic review.
|
||||
/// Absent or false preserves the existing approval path; this does not opt into review.
|
||||
pub const SENSITIVE_ACTION_KEY: &str = "codex_sensitive_action";
|
||||
pub const APPROVALS_REVIEWER_KEY: &str = "approvals_reviewer";
|
||||
pub const PERSIST_KEY: &str = "persist";
|
||||
pub const PERSIST_SESSION: &str = "session";
|
||||
|
||||
Reference in New Issue
Block a user